Skip to content

Commit 6522590

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 ff8f710 commit 6522590

File tree

9 files changed

+149
-67
lines changed

9 files changed

+149
-67
lines changed

‎src/backend/catalog/pg_constraint.c

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -675,7 +675,7 @@ RenameConstraintById(Oid conId, const char *newname)
675675
*/
676676
void
677677
AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
678-
Oid newNspId, bool isType)
678+
Oid newNspId, bool isType, ObjectAddresses *objsMoved)
679679
{
680680
Relation conRel;
681681
ScanKeyData key[1];
@@ -708,6 +708,14 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
708708
while (HeapTupleIsValid((tup = systable_getnext(scan))))
709709
{
710710
Form_pg_constraint conform = (Form_pg_constraint) GETSTRUCT(tup);
711+
ObjectAddress thisobj;
712+
713+
thisobj.classId = ConstraintRelationId;
714+
thisobj.objectId = HeapTupleGetOid(tup);
715+
thisobj.objectSubId = 0;
716+
717+
if (object_address_present(&thisobj, objsMoved))
718+
continue;
711719

712720
if (conform->connamespace == oldNspId)
713721
{
@@ -725,6 +733,8 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
725733
* changeDependencyFor().
726734
*/
727735
}
736+
737+
add_exact_object_address(&thisobj, objsMoved);
728738
}
729739

730740
systable_endscan(scan);

‎src/backend/commands/alter.c

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,8 @@ ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt)
270270
* object doesn't have a schema.
271271
*/
272272
Oid
273-
AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
273+
AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid,
274+
ObjectAddresses *objsMoved)
274275
{
275276
Oid oldNspOid = InvalidOid;
276277
ObjectAddress dep;
@@ -284,20 +285,11 @@ AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
284285
case OCLASS_CLASS:
285286
{
286287
Relation rel;
287-
Relation classRel;
288288

289289
rel = relation_open(objid, AccessExclusiveLock);
290290
oldNspOid = RelationGetNamespace(rel);
291291

292-
classRel = heap_open(RelationRelationId, RowExclusiveLock);
293-
294-
AlterRelationNamespaceInternal(classRel,
295-
objid,
296-
oldNspOid,
297-
nspOid,
298-
true);
299-
300-
heap_close(classRel, RowExclusiveLock);
292+
AlterTableNamespaceInternal(rel, oldNspOid, nspOid, objsMoved);
301293

302294
relation_close(rel, NoLock);
303295
break;
@@ -308,7 +300,7 @@ AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
308300
break;
309301

310302
case OCLASS_TYPE:
311-
oldNspOid = AlterTypeNamespace_oid(objid, nspOid);
303+
oldNspOid = AlterTypeNamespace_oid(objid, nspOid, objsMoved);
312304
break;
313305

314306
case OCLASS_COLLATION:

‎src/backend/commands/extension.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2266,6 +2266,7 @@ AlterExtensionNamespace(List *names, const char *newschema)
22662266
Relation depRel;
22672267
SysScanDesc depScan;
22682268
HeapTuple depTup;
2269+
ObjectAddresses *objsMoved;
22692270

22702271
if (list_length(names) != 1)
22712272
ereport(ERROR,
@@ -2340,6 +2341,8 @@ AlterExtensionNamespace(List *names, const char *newschema)
23402341
errmsg("extension \"%s\" does not support SET SCHEMA",
23412342
NameStr(extForm->extname))));
23422343

2344+
objsMoved = new_object_addresses();
2345+
23432346
/*
23442347
* Scan pg_depend to find objects that depend directly on the extension,
23452348
* and alter each one's schema.
@@ -2379,9 +2382,11 @@ AlterExtensionNamespace(List *names, const char *newschema)
23792382
if (dep.objectSubId != 0) /* should not happen */
23802383
elog(ERROR, "extension should not have a sub-object dependency");
23812384

2385+
/* Relocate the object */
23822386
dep_oldNspOid = AlterObjectNamespace_oid(dep.classId,
23832387
dep.objectId,
2384-
nspOid);
2388+
nspOid,
2389+
objsMoved);
23852390

23862391
/*
23872392
* Remember previous namespace of first object that has one

‎src/backend/commands/tablecmds.c

Lines changed: 85 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -254,10 +254,10 @@ static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
254254
static int findAttrByName(const char *attributeName, List *schema);
255255
static void setRelhassubclassInRelation(Oid relationId, bool relhassubclass);
256256
static void AlterIndexNamespaces(Relation classRel, Relation rel,
257-
Oid oldNspOid, Oid newNspOid);
257+
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved);
258258
static void AlterSeqNamespaces(Relation classRel, Relation rel,
259-
Oid oldNspOid, Oid newNspOid,
260-
const char *newNspName, LOCKMODE lockmode);
259+
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved,
260+
LOCKMODE lockmode);
261261
static void ATExecValidateConstraint(Relation rel, char *constrName);
262262
static int transformColumnNameList(Oid relId, List *colList,
263263
int16 *attnums, Oid *atttypids);
@@ -8950,7 +8950,7 @@ AlterTableNamespace(RangeVar *relation, const char *newschema,
89508950
Oid relid;
89518951
Oid oldNspOid;
89528952
Oid nspOid;
8953-
Relation classRel;
8953+
ObjectAddresses *objsMoved;
89548954

89558955
rel = relation_openrv(relation, lockmode);
89568956

@@ -9042,26 +9042,47 @@ AlterTableNamespace(RangeVar *relation, const char *newschema,
90429042
/* common checks on switching namespaces */
90439043
CheckSetNamespace(oldNspOid, nspOid, RelationRelationId, relid);
90449044

9045+
objsMoved = new_object_addresses();
9046+
AlterTableNamespaceInternal(rel, oldNspOid, nspOid, objsMoved);
9047+
free_object_addresses(objsMoved);
9048+
9049+
/* close rel, but keep lock until commit */
9050+
relation_close(rel, NoLock);
9051+
}
9052+
9053+
/*
9054+
* The guts of relocating a table to another namespace: besides moving
9055+
* the table itself, its dependent objects are relocated to the new schema.
9056+
*/
9057+
void
9058+
AlterTableNamespaceInternal(Relation rel, Oid oldNspOid, Oid nspOid,
9059+
ObjectAddresses *objsMoved)
9060+
{
9061+
Relation classRel;
9062+
9063+
Assert(objsMoved != NULL);
9064+
90459065
/* OK, modify the pg_class row and pg_depend entry */
90469066
classRel = heap_open(RelationRelationId, RowExclusiveLock);
90479067

9048-
AlterRelationNamespaceInternal(classRel, relid, oldNspOid, nspOid, true);
9068+
AlterRelationNamespaceInternal(classRel, RelationGetRelid(rel), oldNspOid,
9069+
nspOid, true, objsMoved);
90499070

90509071
/* Fix the table's row type too */
9051-
AlterTypeNamespaceInternal(rel->rd_rel->reltype, nspOid, false, false);
9072+
AlterTypeNamespaceInternal(rel->rd_rel->reltype,
9073+
nspOid, false, false, objsMoved);
90529074

90539075
/* Fix other dependent stuff */
90549076
if (rel->rd_rel->relkind == RELKIND_RELATION)
90559077
{
9056-
AlterIndexNamespaces(classRel, rel, oldNspOid, nspOid);
9057-
AlterSeqNamespaces(classRel, rel, oldNspOid, nspOid, newschema, lockmode);
9058-
AlterConstraintNamespaces(relid, oldNspOid, nspOid, false);
9078+
AlterIndexNamespaces(classRel, rel, oldNspOid, nspOid, objsMoved);
9079+
AlterSeqNamespaces(classRel, rel, oldNspOid, nspOid,
9080+
objsMoved, AccessExclusiveLock);
9081+
AlterConstraintNamespaces(RelationGetRelid(rel), oldNspOid, nspOid,
9082+
false, objsMoved);
90599083
}
90609084

90619085
heap_close(classRel, RowExclusiveLock);
9062-
9063-
/* close rel, but keep lock until commit */
9064-
relation_close(rel, NoLock);
90659086
}
90669087

90679088
/*
@@ -9072,10 +9093,11 @@ AlterTableNamespace(RangeVar *relation, const char *newschema,
90729093
void
90739094
AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
90749095
Oid oldNspOid, Oid newNspOid,
9075-
bool hasDependEntry)
9096+
bool hasDependEntry, ObjectAddresses *objsMoved)
90769097
{
90779098
HeapTuple classTup;
90789099
Form_pg_class classForm;
9100+
ObjectAddress thisobj;
90799101

90809102
classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relOid));
90819103
if (!HeapTupleIsValid(classTup))
@@ -9084,27 +9106,39 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
90849106

90859107
Assert(classForm->relnamespace == oldNspOid);
90869108

9087-
/* check for duplicate name (more friendly than unique-index failure) */
9088-
if (get_relname_relid(NameStr(classForm->relname),
9089-
newNspOid) != InvalidOid)
9090-
ereport(ERROR,
9091-
(errcode(ERRCODE_DUPLICATE_TABLE),
9092-
errmsg("relation \"%s\" already exists in schema \"%s\"",
9093-
NameStr(classForm->relname),
9094-
get_namespace_name(newNspOid))));
9109+
thisobj.classId = RelationRelationId;
9110+
thisobj.objectId = relOid;
9111+
thisobj.objectSubId = 0;
9112+
9113+
/*
9114+
* Do nothing when there's nothing to do.
9115+
*/
9116+
if (!object_address_present(&thisobj, objsMoved))
9117+
{
9118+
/* check for duplicate name (more friendly than unique-index failure) */
9119+
if (get_relname_relid(NameStr(classForm->relname),
9120+
newNspOid) != InvalidOid)
9121+
ereport(ERROR,
9122+
(errcode(ERRCODE_DUPLICATE_TABLE),
9123+
errmsg("relation \"%s\" already exists in schema \"%s\"",
9124+
NameStr(classForm->relname),
9125+
get_namespace_name(newNspOid))));
90959126

9096-
/* classTup is a copy, so OK to scribble on */
9097-
classForm->relnamespace = newNspOid;
9127+
/* classTup is a copy, so OK to scribble on */
9128+
classForm->relnamespace = newNspOid;
90989129

9099-
simple_heap_update(classRel, &classTup->t_self, classTup);
9100-
CatalogUpdateIndexes(classRel, classTup);
9130+
simple_heap_update(classRel, &classTup->t_self, classTup);
9131+
CatalogUpdateIndexes(classRel, classTup);
91019132

9102-
/* Update dependency on schema if caller said so */
9103-
if (hasDependEntry &&
9104-
changeDependencyFor(RelationRelationId, relOid,
9105-
NamespaceRelationId, oldNspOid, newNspOid) != 1)
9106-
elog(ERROR, "failed to change schema dependency for relation \"%s\"",
9107-
NameStr(classForm->relname));
9133+
/* Update dependency on schema if caller said so */
9134+
if (hasDependEntry &&
9135+
changeDependencyFor(RelationRelationId, relOid,
9136+
NamespaceRelationId, oldNspOid, newNspOid) != 1)
9137+
elog(ERROR, "failed to change schema dependency for relation \"%s\"",
9138+
NameStr(classForm->relname));
9139+
9140+
add_exact_object_address(&thisobj, objsMoved);
9141+
}
91089142

91099143
heap_freetuple(classTup);
91109144
}
@@ -9117,7 +9151,7 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
91179151
*/
91189152
static void
91199153
AlterIndexNamespaces(Relation classRel, Relation rel,
9120-
Oid oldNspOid, Oid newNspOid)
9154+
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved)
91219155
{
91229156
List *indexList;
91239157
ListCell *l;
@@ -9127,15 +9161,27 @@ AlterIndexNamespaces(Relation classRel, Relation rel,
91279161
foreach(l, indexList)
91289162
{
91299163
Oid indexOid = lfirst_oid(l);
9164+
ObjectAddress thisobj;
9165+
9166+
thisobj.classId = RelationRelationId;
9167+
thisobj.objectId = indexOid;
9168+
thisobj.objectSubId = 0;
91309169

91319170
/*
91329171
* Note: currently, the index will not have its own dependency on the
91339172
* namespace, so we don't need to do changeDependencyFor(). There's no
91349173
* row type in pg_type, either.
9174+
*
9175+
* XXX this objsMoved test may be pointless -- surely we have a single
9176+
* dependency link from a relation to each index?
91359177
*/
9136-
AlterRelationNamespaceInternal(classRel, indexOid,
9137-
oldNspOid, newNspOid,
9138-
false);
9178+
if (!object_address_present(&thisobj, objsMoved))
9179+
{
9180+
AlterRelationNamespaceInternal(classRel, indexOid,
9181+
oldNspOid, newNspOid,
9182+
false, objsMoved);
9183+
add_exact_object_address(&thisobj, objsMoved);
9184+
}
91399185
}
91409186

91419187
list_free(indexList);
@@ -9150,7 +9196,8 @@ AlterIndexNamespaces(Relation classRel, Relation rel,
91509196
*/
91519197
static void
91529198
AlterSeqNamespaces(Relation classRel, Relation rel,
9153-
Oid oldNspOid, Oid newNspOid, const char *newNspName, LOCKMODE lockmode)
9199+
Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved,
9200+
LOCKMODE lockmode)
91549201
{
91559202
Relation depRel;
91569203
SysScanDesc scan;
@@ -9202,14 +9249,14 @@ AlterSeqNamespaces(Relation classRel, Relation rel,
92029249
/* Fix the pg_class and pg_depend entries */
92039250
AlterRelationNamespaceInternal(classRel, depForm->objid,
92049251
oldNspOid, newNspOid,
9205-
true);
9252+
true, objsMoved);
92069253

92079254
/*
92089255
* Sequences have entries in pg_type. We need to be careful to move
92099256
* them to the new namespace, too.
92109257
*/
92119258
AlterTypeNamespaceInternal(RelationGetForm(seqRel)->reltype,
9212-
newNspOid, false, false);
9259+
newNspOid, false, false, objsMoved);
92139260

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

0 commit comments

Comments
 (0)