8000 Fix ALTER EXTENSION / SET SCHEMA · danielcode/postgres@fb3590d · GitHub
[go: up one dir, main page]
Skip to content

Commit fb3590d

Browse files
alvherrealvherre
committed
Fix ALTER EXTENSION / SET SCHEMA
In its original conception, it was leaving some objects into the old schema, but without their proper pg_depend entries; this meant that the old schema could be dropped, causing future pg_dump calls to fail on the affected database. This was originally reported by Jeff Frost as #6704; there have been other complaints elsewhere that can probably be traced to this bug. To fix, be more consistent about altering a table's subsidiary objects along the table itself; this requires some restructuring in how tables are relocated when altering an extension -- hence the new AlterTableNamespaceInternal routine which encapsulates it for both the ALTER TABLE and the ALTER EXTENSION cases. There was another bug lurking here, which was unmasked after fixing the previous one: certain objects would be reached twice via the dependency graph, and the second attempt to move them would cause the entire operation to fail. Per discussion, it seems the best fix for this is to do more careful tracking of objects already moved: we now maintain a list of moved objects, to avoid attempting to do it twice for the same object. Authors: Alvaro Herrera, Dimitri Fontaine Reviewed by Tom Lane
1 parent 7e951ba commit fb3590d

File tree

9 files changed

+148
-67
lines changed

9 files changed

+148
-67
lines changed

‎src/backend/catalog/pg_constraint.c

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -678,7 +678,7 @@ RenameConstraintById(Oid conId, const char *newname)
678678
*/
679679
void
680680
AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
681-
Oid newNspId, bool isType)
681+
Oid newNspId, bool isType, ObjectAddresses *objsMoved)
682682
{
683683
Relation conRel;
684684
ScanKeyData key[1];
@@ -711,6 +711,14 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
711711
while (HeapTupleIsValid((tup = systable_getnext(scan))))
712712
{
713713
Form_pg_constraint conform = (Form_pg_constraint) GETSTRUCT(tup);
714+
ObjectAddress thisobj;
715+
716+
thisobj.classId = ConstraintRelationId;
717+
thisobj.objectId = HeapTupleGetOid(tup);
718+
thisobj.objectSubId = 0;
719+
720+
if (object_address_present(&thisobj, objsMoved))
721+
continue;
714722

715723
if (conform->connamespace == oldNspId)
716724
{
@@ -728,6 +736,8 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
728736
* changeDependencyFor().
729737
*/
730738
}
739+
740+
add_exact_object_address(&thisobj, objsMoved);
731741
}
732742

733743
systable_endscan(scan);

‎src/backend/commands/alter.c

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,8 @@ ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt)
241241
* object doesn't have a schema.
242242
*/
243243
Oid
244-
AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
244+
AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid,
245+
ObjectAddresses *objsMoved)
245246
{
246247
Oid oldNspOid = InvalidOid;
247248
ObjectAddress dep;
@@ -255,20 +256,11 @@ AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
255256
case OCLASS_CLASS:
256257
{
257258
Relation rel;
258-
Relation classRel;
259259

260260
rel = relation_open(objid, AccessExclusiveLock);
261261
oldNspOid = RelationGetNamespace(rel);
262262

263-
classRel = heap_open(RelationRelationId, RowExclusiveLock);
264-
265-
AlterRelationNamespaceInternal(classRel,
266-
objid,
267-
oldNspOid,
268-
nspOid,
269-
true);
270-
271-
heap_close(classRel, RowExclusiveLock);
263+
AlterTableNamespaceInternal(rel, oldNspOid, nspOid, objsMoved);
272264

273265
relation_close(rel, NoLock);
274266
break;
@@ -279,7 +271,7 @@ AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
279271
break;
280272

281273
case OCLASS_TYPE:
282-
oldNspOid = AlterTypeNamespace_oid(objid, nspOid);
274+
oldNspOid = AlterTypeNamespace_oid(objid, nspOid, objsMoved);
283275
break;
284276

285277
case OCLASS_COLLATION:

‎src/backend/commands/extension.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2201,6 +2201,7 @@ AlterExtensionNamespace(List *names, const char *newschema)
22012201
Relation depRel;
22022202
SysScanDesc depScan;
22032203
HeapTuple depTup;
2204+
ObjectAddresses *objsMoved;
22042205

22052206
if (list_length(names) != 1)
22062207
ereport(ERROR,
@@ -2275,6 +2276,8 @@ AlterExtensionNamespace(List *names, const char *newschema)
22752276
errmsg("extension \"%s\" does not support SET SCHEMA",
22762277
NameStr(extForm->extname))));
22772278

2279+
objsMoved = new_object_addresses();
2280+
22782281
/*
22792282
* Scan pg_depend to find objects that depend directly on the extension,
22802283
* and alter each one's schema.
@@ -2314,9 +2317,11 @@ AlterExtensionNamespace(List *names, const char *newschema)
23142317
if (dep.objectSubId != 0) /* should not happen */
23152318
elog(ERROR, "extension should not have a sub-object dependency");
23162319

2320+
/* Relocate the object */
23172321
dep_oldNspOid = AlterObjectNamespace_oid(dep.classId,
23182322
dep.objectId,
2319-
nspOid);
2323+
nspOid,
2324+
objsMoved);
23202325

23212326
/*
23222327
* Remember previous namespace of first object that has one

‎src/backend/commands/tablecmds.c

Lines changed: 85 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -261,10 +261,10 @@ static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
261261
int16 seqNumber, Relation inhRelation);
262262
static int findAttrByName(const char *attributeName, List *schema);
263263
static void AlterIndexNamespaces(Relation classRel, Relation rel,
264-
Oid oldNspOid, Oid newNspOid);
264+
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved);
265265
static void AlterSeqNamespaces(Relation classRel, Relation rel,
266-
Oid oldNspOid, Oid newNspOid,
267-
const char *newNspName, LOCKMODE lockmode);
266+
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved,
267+
LOCKMODE lockmode);
268268
static void ATExecValidateConstraint(Relation rel, char *constrName,
269269
bool recurse, bool recursing, LOCKMODE lockmode);
270270
static int transformColumnNameList(Oid relId, List *colList,
@@ -9732,8 +9732,8 @@ AlterTableNamespace(AlterObjectSchemaStmt *stmt)
97329732
Oid relid;
97339733
Oid oldNspOid;
97349734
Oid nspOid;
9735-
Relation classRel;
97369735
RangeVar *newrv;
9736+
ObjectAddresses *objsMoved;
97379737

97389738
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
97399739
stmt->missing_ok, false,
@@ -9774,27 +9774,47 @@ AlterTableNamespace(AlterObjectSchemaStmt *stmt)
97749774
/* common checks on switching namespaces */
97759775
CheckSetNamespace(oldNspOid, nspOid, RelationRelationId, relid);
97769776

9777+
objsMoved = new_object_addresses();
9778+
AlterTableNamespaceInternal(rel, oldNspOid, nspOid, objsMoved);
9779+
free_object_addresses(objsMoved);
9780+
9781+
/* close rel, but keep lock until commit */
9782+
relation_close(rel, NoLock);
9783+
}
9784+
9785+
/*
9786+
* The guts of relocating a table to another namespace: besides moving
9787+
* the table itself, its dependent objects are relocated to the new schema.
9788+
*/
9789+
void
9790+
AlterTableNamespaceInternal(Relation rel, Oid oldNspOid, Oid nspOid,
9791+
ObjectAddresses *objsMoved)
9792+
{
9793+
Relation classRel;
9794+
9795+
Assert(objsMoved != NULL);
9796+
97779797
/* OK, modify the pg_class row and pg_depend entry */
97789798
classRel = heap_open(RelationRelationId, RowExclusiveLock);
97799799

9780-
AlterRelationNamespaceInternal(classRel, relid, oldNspOid, nspOid, true);
9800+
AlterRelationNamespaceInternal(classRel, RelationGetRelid(rel), oldNspOid,
9801+
nspOid, true, objsMoved);
97819802

97829803
/* Fix the table's row type too */
9783-
AlterTypeNamespaceInternal(rel->rd_rel->reltype, nspOid, false, false);
9804+
AlterTypeNamespaceInternal(rel->rd_rel->reltype,
9805+
nspOid, false, false, objsMoved);
97849806

97859807
/* Fix other dependent stuff */
97869808
if (rel->rd_rel->relkind == RELKIND_RELATION)
97879809
{
9788-
AlterIndexNamespaces(classRel, rel, oldNspOid, nspOid);
9789-
AlterSeqNamespaces(classRel, rel, oldNspOid, nspOid, stmt->newschema,
9790-
AccessExclusiveLock);
9791-
AlterConstraintNamespaces(relid, oldNspOid, nspOid, false);
9810+
AlterIndexNamespaces(classRel, rel, oldNspOid, nspOid, objsMoved);
9811+
AlterSeqNamespaces(classRel, rel, oldNspOid, nspOid,
9812+
objsMoved, AccessExclusiveLock);
9813+
AlterConstraintNamespaces(RelationGetRelid(rel), oldNspOid, nspOid,
9814+
false, objsMoved);
97929815
}
97939816

97949817
heap_close(classRel, RowExclusiveLock);
9795-
9796-
/* close rel, but keep lock until commit */
9797-
relation_close(rel, NoLock);
97989818
}
97999819

98009820
/*
@@ -9805,10 +9825,11 @@ AlterTableNamespace(AlterObjectSchemaStmt *stmt)
98059825
void
98069826
AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
98079827
Oid oldNspOid, Oid newNspOid,
9808-
bool hasDependEntry)
9828+
bool hasDependEntry, ObjectAddresses *objsMoved)
98099829
{
98109830
HeapTuple classTup;
98119831
Form_pg_class classForm;
9832+
ObjectAddress thisobj;
98129833

98139834
classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relOid));
98149835
if (!HeapTupleIsValid(classTup))
@@ -9817,27 +9838,39 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
98179838

98189839
Assert(classForm->relnamespace == oldNspOid);
98199840

9820-
/* check for duplicate name (more friendly than unique-index failure) */
9821-
if (get_relname_relid(NameStr(classForm->relname),
9822-
newNspOid) != InvalidOid)
9823-
ereport(ERROR,
9824-
(errcode(ERRCODE_DUPLICATE_TABLE),
9825-
errmsg("relation \"%s\" already exists in schema \"%s\"",
9826-
NameStr(classForm->relname),
9827-
get_namespace_name(newNspOid))));
9841+
thisobj.classId = RelationRelationId;
9842+
thisobj.objectId = relOid;
9843+
thisobj.objectSubId = 0;
9844+
9845+
/*
9846+
* Do nothing when there's nothing to do.
9847+
*/
9848+
if (!object_address_present(&thisobj, objsMoved))
9849+
{
9850+
/* check for duplicate name (more friendly than unique-index failure) */
9851+
if (get_relname_relid(NameStr(classForm->relname),
9852+
newNspOid) != InvalidOid)
9853+
ereport(ERROR,
9854+
(errcode(ERRCODE_DUPLICATE_TABLE),
9855+
errmsg("relation \"%s\" already exists in schema \"%s\"",
9856+
NameStr(classForm->relname),
9857+
get_namespace_name(newNspOid))));
98289858

9829-
/* classTup is a copy, so OK to scribble on */
9830-
classForm->relnamespace = newNspOid;
9859+
/* classTup is a copy, so OK to scribble on */
9860+
classForm->relnamespace = newNspOid;
98319861

9832-
simple_heap_update(classRel, &classTup->t_self, classTup);
9833-
CatalogUpdateIndexes(classRel, classTup);
9862+
simple_heap_update(classRel, &classTup->t_self, classTup);
9863+
CatalogUpdateIndexes(classRel, classTup);
98349864

9835-
/* Update dependency on schema if caller said so */
9836-
if (hasDependEntry &&
9837-
changeDependencyFor(RelationRelationId, relOid,
9838-
NamespaceRelationId, oldNspOid, newNspOid) != 1)
9839-
elog(ERROR, "failed to change schema dependency for relation \"%s\"",
9840-
NameStr(classForm->relname));
9865+
/* Update dependency on schema if caller said so */
9866+
if (hasDependEntry &&
9867+
changeDependencyFor(RelationRelationId, relOid,
9868+
NamespaceRelationId, oldNspOid, newNspOid) != 1)
9869+
elog(ERROR, "failed to change schema dependency for relation \"%s\"",
9870+
NameStr(classForm->relname));
9871+
9872+
add_exact_object_address(&thisobj, objsMoved);
9873+
}
98419874

98429875
heap_freetuple(classTup);
98439876
}
@@ -9850,7 +9883,7 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
98509883
*/
98519884
static void
98529885
AlterIndexNamespaces(Relation classRel, Relation rel,
9853-
Oid oldNspOid, Oid newNspOid)
9886+
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved)
98549887
{
98559888
List *indexList;
98569889
ListCell *l;
@@ -9860,15 +9893,27 @@ AlterIndexNamespaces(Relation classRel, Relation rel,
98609893
foreach(l, indexList)
98619894
{
98629895
Oid indexOid = lfirst_oid(l);
9896+
ObjectAddress thisobj;
9897+
9898+
thisobj.classId = RelationRelationId;
9899+
thisobj.objectId = indexOid;
9900+
thisobj.objectSubId = 0;
98639901

98649902
/*
98659903
* Note: currently, the index will not have its own dependency on the
98669904
* namespace, so we don't need to do changeDependencyFor(). There's no
98679905
* row type in pg_type, either.
9906+
*
9907+
* XXX this objsMoved test may be pointless -- surely we have a single
9908+
* dependency link from a relation to each index?
98689909
*/
9869-
AlterRelationNamespaceInternal(classRel, indexOid,
9870-
oldNspOid, newNspOid,
9871-
false);
9910+
if (!object_address_present(&thisobj, objsMoved))
9911+
{
9912+
AlterRelationNamespaceInternal(classRel, indexOid,
9913+
oldNspOid, newNspOid,
9914+
false, objsMoved);
9915+
add_exact_object_address(&thisobj, objsMoved);
9916+
}
98729917
}
98739918

98749919
list_free(indexList);
@@ -9883,7 +9928,8 @@ AlterIndexNamespaces(Relation classRel, Relation rel,
98839928
*/
98849929
static void
98859930
AlterSeqNamespaces(Relation classRel, Relation rel,
9886-
Oid oldNspOid, Oid newNspOid, const char *newNspName, LOCKMODE lockmode)
9931+
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved,
9932+
LOCKMODE lockmode)
98879933
{
98889934
Relation depRel;
98899935
SysScanDesc scan;
@@ -9935,14 +9981,14 @@ AlterSeqNamespaces(Relation classRel, Relation rel,
99359981
/* Fix the pg_class and pg_depend entries */
99369982
AlterRelationNamespaceInternal(classRel, depForm->objid,
99379983
oldNspOid, newNspOid,
9938-
true);
9984+
true, objsMoved);
99399985

99409986
/*
99419987
* Sequences have entries in pg_type. We need to be careful to move
99429988
* them to the new namespace, too.
99439989
*/
99449990
AlterTypeNamespaceInternal(RelationGetForm(seqRel)->reltype,
9945-
newNspOid, false, false);
9991+
newNspOid, false, false, objsMoved);
99469992

99479993
/* Now we can close it. Keep the lock till end of transaction. */
99489994
relation_close(seqRel, NoLock);

0 commit comments

Comments
 (0)
0