]> granicus.if.org Git - postgresql/commitdiff
Fix ALTER EXTENSION / SET SCHEMA
authorAlvaro Herrera <alvherre@alvh.no-ip.org>
Wed, 31 Oct 2012 13:49:14 +0000 (10:49 -0300)
committerAlvaro Herrera <alvherre@alvh.no-ip.org>
Wed, 31 Oct 2012 13:49:14 +0000 (10:49 -0300)
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

src/backend/catalog/pg_constraint.c
src/backend/commands/alter.c
src/backend/commands/extension.c
src/backend/commands/tablecmds.c
src/backend/commands/typecmds.c
src/include/catalog/pg_constraint.h
src/include/commands/alter.h
src/include/commands/tablecmds.h
src/include/commands/typecmds.h

index 69979942af42a00922fb4bce3ada1fa622dd2212..71c48ffa92372a27f109c57ed5d109d1b4ee2960 100644 (file)
@@ -675,7 +675,7 @@ RenameConstraintById(Oid conId, const char *newname)
  */
 void
 AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
-                                                 Oid newNspId, bool isType)
+                                                 Oid newNspId, bool isType, ObjectAddresses *objsMoved)
 {
        Relation        conRel;
        ScanKeyData key[1];
@@ -708,6 +708,14 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
        while (HeapTupleIsValid((tup = systable_getnext(scan))))
        {
                Form_pg_constraint conform = (Form_pg_constraint) GETSTRUCT(tup);
+               ObjectAddress   thisobj;
+
+               thisobj.classId = ConstraintRelationId;
+               thisobj.objectId = HeapTupleGetOid(tup);
+               thisobj.objectSubId = 0;
+
+               if (object_address_present(&thisobj, objsMoved))
+                       continue;
 
                if (conform->connamespace == oldNspId)
                {
@@ -725,6 +733,8 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
                         * changeDependencyFor().
                         */
                }
+
+               add_exact_object_address(&thisobj, objsMoved);
        }
 
        systable_endscan(scan);
index d02bf7fba5d1765b6daec84a772d83bc6f76c21b..0f4e0f701abcf868748a0b0bb21a80112814be94 100644 (file)
@@ -270,7 +270,8 @@ ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt)
  * object doesn't have a schema.
  */
 Oid
-AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
+AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid,
+                                                ObjectAddresses *objsMoved)
 {
        Oid                     oldNspOid = InvalidOid;
        ObjectAddress dep;
@@ -284,20 +285,11 @@ AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
                case OCLASS_CLASS:
                        {
                                Relation        rel;
-                               Relation        classRel;
 
                                rel = relation_open(objid, AccessExclusiveLock);
                                oldNspOid = RelationGetNamespace(rel);
 
-                               classRel = heap_open(RelationRelationId, RowExclusiveLock);
-
-                               AlterRelationNamespaceInternal(classRel,
-                                                                                          objid,
-                                                                                          oldNspOid,
-                                                                                          nspOid,
-                                                                                          true);
-
-                               heap_close(classRel, RowExclusiveLock);
+                               AlterTableNamespaceInternal(rel, oldNspOid, nspOid, objsMoved);
 
                                relation_close(rel, NoLock);
                                break;
@@ -308,7 +300,7 @@ AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid)
                        break;
 
                case OCLASS_TYPE:
-                       oldNspOid = AlterTypeNamespace_oid(objid, nspOid);
+                       oldNspOid = AlterTypeNamespace_oid(objid, nspOid, objsMoved);
                        break;
 
                case OCLASS_COLLATION:
index 9bac9a3efe7174bc8b9e0578aa0cac6b5c0f469e..c2fd3aa7cc680bb0cde9398c67581252864bc4bb 100644 (file)
@@ -2266,6 +2266,7 @@ AlterExtensionNamespace(List *names, const char *newschema)
        Relation        depRel;
        SysScanDesc depScan;
        HeapTuple       depTup;
+       ObjectAddresses *objsMoved;
 
        if (list_length(names) != 1)
                ereport(ERROR,
@@ -2340,6 +2341,8 @@ AlterExtensionNamespace(List *names, const char *newschema)
                                 errmsg("extension \"%s\" does not support SET SCHEMA",
                                                NameStr(extForm->extname))));
 
+       objsMoved = new_object_addresses();
+
        /*
         * Scan pg_depend to find objects that depend directly on the extension,
         * and alter each one's schema.
@@ -2379,9 +2382,11 @@ AlterExtensionNamespace(List *names, const char *newschema)
                if (dep.objectSubId != 0)               /* should not happen */
                        elog(ERROR, "extension should not have a sub-object dependency");
 
+               /* Relocate the object */
                dep_oldNspOid = AlterObjectNamespace_oid(dep.classId,
                                                                                                 dep.objectId,
-                                                                                                nspOid);
+                                                                                                nspOid,
+                                                                                                objsMoved);
 
                /*
                 * Remember previous namespace of first object that has one
index 437c2773af0d09145a3e6e9d12886450b002fc39..3e3778987c06befe973bed7afe8244274f6e5780 100644 (file)
@@ -254,10 +254,10 @@ static void StoreCatalogInheritance1(Oid relationId, Oid parentOid,
 static int     findAttrByName(const char *attributeName, List *schema);
 static void setRelhassubclassInRelation(Oid relationId, bool relhassubclass);
 static void AlterIndexNamespaces(Relation classRel, Relation rel,
-                                        Oid oldNspOid, Oid newNspOid);
+                                        Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved);
 static void AlterSeqNamespaces(Relation classRel, Relation rel,
-                                  Oid oldNspOid, Oid newNspOid,
-                                  const char *newNspName, LOCKMODE lockmode);
+                                  Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved,
+                                  LOCKMODE lockmode);
 static void ATExecValidateConstraint(Relation rel, char *constrName);
 static int transformColumnNameList(Oid relId, List *colList,
                                                int16 *attnums, Oid *atttypids);
@@ -8950,7 +8950,7 @@ AlterTableNamespace(RangeVar *relation, const char *newschema,
        Oid                     relid;
        Oid                     oldNspOid;
        Oid                     nspOid;
-       Relation        classRel;
+       ObjectAddresses *objsMoved;
 
        rel = relation_openrv(relation, lockmode);
 
@@ -9042,26 +9042,47 @@ AlterTableNamespace(RangeVar *relation, const char *newschema,
        /* common checks on switching namespaces */
        CheckSetNamespace(oldNspOid, nspOid, RelationRelationId, relid);
 
+       objsMoved = new_object_addresses();
+       AlterTableNamespaceInternal(rel, oldNspOid, nspOid, objsMoved);
+       free_object_addresses(objsMoved);
+
+       /* close rel, but keep lock until commit */
+       relation_close(rel, NoLock);
+}
+
+/*
+ * The guts of relocating a table to another namespace: besides moving
+ * the table itself, its dependent objects are relocated to the new schema.
+ */
+void
+AlterTableNamespaceInternal(Relation rel, Oid oldNspOid, Oid nspOid,
+                                                       ObjectAddresses *objsMoved)
+{
+       Relation        classRel;
+
+       Assert(objsMoved != NULL);
+
        /* OK, modify the pg_class row and pg_depend entry */
        classRel = heap_open(RelationRelationId, RowExclusiveLock);
 
-       AlterRelationNamespaceInternal(classRel, relid, oldNspOid, nspOid, true);
+       AlterRelationNamespaceInternal(classRel, RelationGetRelid(rel), oldNspOid,
+                                                                  nspOid, true, objsMoved);
 
        /* Fix the table's row type too */
-       AlterTypeNamespaceInternal(rel->rd_rel->reltype, nspOid, false, false);
+       AlterTypeNamespaceInternal(rel->rd_rel->reltype,
+                                                          nspOid, false, false, objsMoved);
 
        /* Fix other dependent stuff */
        if (rel->rd_rel->relkind == RELKIND_RELATION)
        {
-               AlterIndexNamespaces(classRel, rel, oldNspOid, nspOid);
-               AlterSeqNamespaces(classRel, rel, oldNspOid, nspOid, newschema, lockmode);
-               AlterConstraintNamespaces(relid, oldNspOid, nspOid, false);
+               AlterIndexNamespaces(classRel, rel, oldNspOid, nspOid, objsMoved);
+               AlterSeqNamespaces(classRel, rel, oldNspOid, nspOid,
+                                                  objsMoved, AccessExclusiveLock);
+               AlterConstraintNamespaces(RelationGetRelid(rel), oldNspOid, nspOid,
+                                                                 false, objsMoved);
        }
 
        heap_close(classRel, RowExclusiveLock);
-
-       /* close rel, but keep lock until commit */
-       relation_close(rel, NoLock);
 }
 
 /*
@@ -9072,10 +9093,11 @@ AlterTableNamespace(RangeVar *relation, const char *newschema,
 void
 AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
                                                           Oid oldNspOid, Oid newNspOid,
-                                                          bool hasDependEntry)
+                                                          bool hasDependEntry, ObjectAddresses *objsMoved)
 {
        HeapTuple       classTup;
        Form_pg_class classForm;
+       ObjectAddress   thisobj;
 
        classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relOid));
        if (!HeapTupleIsValid(classTup))
@@ -9084,27 +9106,39 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
 
        Assert(classForm->relnamespace == oldNspOid);
 
-       /* check for duplicate name (more friendly than unique-index failure) */
-       if (get_relname_relid(NameStr(classForm->relname),
-                                                 newNspOid) != InvalidOid)
-               ereport(ERROR,
-                               (errcode(ERRCODE_DUPLICATE_TABLE),
-                                errmsg("relation \"%s\" already exists in schema \"%s\"",
-                                               NameStr(classForm->relname),
-                                               get_namespace_name(newNspOid))));
+       thisobj.classId = RelationRelationId;
+       thisobj.objectId = relOid;
+       thisobj.objectSubId = 0;
+
+       /*
+        * Do nothing when there's nothing to do.
+        */
+       if (!object_address_present(&thisobj, objsMoved))
+       {
+               /* check for duplicate name (more friendly than unique-index failure) */
+               if (get_relname_relid(NameStr(classForm->relname),
+                                                         newNspOid) != InvalidOid)
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_DUPLICATE_TABLE),
+                                        errmsg("relation \"%s\" already exists in schema \"%s\"",
+                                                       NameStr(classForm->relname),
+                                                       get_namespace_name(newNspOid))));
 
-       /* classTup is a copy, so OK to scribble on */
-       classForm->relnamespace = newNspOid;
+               /* classTup is a copy, so OK to scribble on */
+               classForm->relnamespace = newNspOid;
 
-       simple_heap_update(classRel, &classTup->t_self, classTup);
-       CatalogUpdateIndexes(classRel, classTup);
+               simple_heap_update(classRel, &classTup->t_self, classTup);
+               CatalogUpdateIndexes(classRel, classTup);
 
-       /* Update dependency on schema if caller said so */
-       if (hasDependEntry &&
-               changeDependencyFor(RelationRelationId, relOid,
-                                                       NamespaceRelationId, oldNspOid, newNspOid) != 1)
-               elog(ERROR, "failed to change schema dependency for relation \"%s\"",
-                        NameStr(classForm->relname));
+               /* Update dependency on schema if caller said so */
+               if (hasDependEntry &&
+                       changeDependencyFor(RelationRelationId, relOid,
+                                                               NamespaceRelationId, oldNspOid, newNspOid) != 1)
+                       elog(ERROR, "failed to change schema dependency for relation \"%s\"",
+                                NameStr(classForm->relname));
+
+               add_exact_object_address(&thisobj, objsMoved);
+       }
 
        heap_freetuple(classTup);
 }
@@ -9117,7 +9151,7 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
  */
 static void
 AlterIndexNamespaces(Relation classRel, Relation rel,
-                                        Oid oldNspOid, Oid newNspOid)
+                                        Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved)
 {
        List       *indexList;
        ListCell   *l;
@@ -9127,15 +9161,27 @@ AlterIndexNamespaces(Relation classRel, Relation rel,
        foreach(l, indexList)
        {
                Oid                     indexOid = lfirst_oid(l);
+               ObjectAddress thisobj;
+
+               thisobj.classId = RelationRelationId;
+               thisobj.objectId = indexOid;
+               thisobj.objectSubId = 0;
 
                /*
                 * Note: currently, the index will not have its own dependency on the
                 * namespace, so we don't need to do changeDependencyFor(). There's no
                 * row type in pg_type, either.
+                *
+                * XXX this objsMoved test may be pointless -- surely we have a single
+                * dependency link from a relation to each index?
                 */
-               AlterRelationNamespaceInternal(classRel, indexOid,
-                                                                          oldNspOid, newNspOid,
-                                                                          false);
+               if (!object_address_present(&thisobj, objsMoved))
+               {
+                       AlterRelationNamespaceInternal(classRel, indexOid,
+                                                                                  oldNspOid, newNspOid,
+                                                                                  false, objsMoved);
+                       add_exact_object_address(&thisobj, objsMoved);
+               }
        }
 
        list_free(indexList);
@@ -9150,7 +9196,8 @@ AlterIndexNamespaces(Relation classRel, Relation rel,
  */
 static void
 AlterSeqNamespaces(Relation classRel, Relation rel,
-        Oid oldNspOid, Oid newNspOid, const char *newNspName, LOCKMODE lockmode)
+                                  Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved,
+                                  LOCKMODE lockmode)
 {
        Relation        depRel;
        SysScanDesc scan;
@@ -9202,14 +9249,14 @@ AlterSeqNamespaces(Relation classRel, Relation rel,
                /* Fix the pg_class and pg_depend entries */
                AlterRelationNamespaceInternal(classRel, depForm->objid,
                                                                           oldNspOid, newNspOid,
-                                                                          true);
+                                                                          true, objsMoved);
 
                /*
                 * Sequences have entries in pg_type. We need to be careful to move
                 * them to the new namespace, too.
                 */
                AlterTypeNamespaceInternal(RelationGetForm(seqRel)->reltype,
-                                                                  newNspOid, false, false);
+                                                                  newNspOid, false, false, objsMoved);
 
                /* Now we can close it.  Keep the lock till end of transaction. */
                relation_close(seqRel, NoLock);
index 18e80221070c7d873aba06db31893ef81a2d3c51..4943411de0b18544345ea3071c24a7f60effe4ed 100644 (file)
@@ -2806,6 +2806,7 @@ AlterTypeNamespace(List *names, const char *newschema)
        TypeName   *typename;
        Oid                     typeOid;
        Oid                     nspOid;
+       ObjectAddresses *objsMoved;
 
        /* Make a TypeName so we can use standard type lookup machinery */
        typename = makeTypeNameFromNameList(names);
@@ -2814,11 +2815,13 @@ AlterTypeNamespace(List *names, const char *newschema)
        /* get schema OID and check its permissions */
        nspOid = LookupCreationNamespace(newschema);
 
-       AlterTypeNamespace_oid(typeOid, nspOid);
+       objsMoved = new_object_addresses();
+       AlterTypeNamespace_oid(typeOid, nspOid, objsMoved);
+       free_object_addresses(objsMoved);
 }
 
 Oid
-AlterTypeNamespace_oid(Oid typeOid, Oid nspOid)
+AlterTypeNamespace_oid(Oid typeOid, Oid nspOid, ObjectAddresses *objsMoved)
 {
        Oid                     elemOid;
 
@@ -2838,7 +2841,7 @@ AlterTypeNamespace_oid(Oid typeOid, Oid nspOid)
                                                 format_type_be(elemOid))));
 
        /* and do the work */
-       return AlterTypeNamespaceInternal(typeOid, nspOid, false, true);
+       return AlterTypeNamespaceInternal(typeOid, nspOid, false, true, objsMoved);
 }
 
 /*
@@ -2859,7 +2862,8 @@ AlterTypeNamespace_oid(Oid typeOid, Oid nspOid)
 Oid
 AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
                                                   bool isImplicitArray,
-                                                  bool errorOnTableType)
+                                                  bool errorOnTableType,
+                                                  ObjectAddresses *objsMoved)
 {
        Relation        rel;
        HeapTuple       tup;
@@ -2867,6 +2871,17 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
        Oid                     oldNspOid;
        Oid                     arrayOid;
        bool            isCompositeType;
+       ObjectAddress thisobj;
+
+       /*
+        * Make sure we haven't moved this object previously.
+        */
+       thisobj.classId = TypeRelationId;
+       thisobj.objectId = typeOid;
+       thisobj.objectSubId = 0;
+
+       if (object_address_present(&thisobj, objsMoved))
+               return InvalidOid;
 
        rel = heap_open(TypeRelationId, RowExclusiveLock);
 
@@ -2927,7 +2942,7 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
 
                AlterRelationNamespaceInternal(classRel, typform->typrelid,
                                                                           oldNspOid, nspOid,
-                                                                          false);
+                                                                          false, objsMoved);
 
                heap_close(classRel, RowExclusiveLock);
 
@@ -2936,13 +2951,14 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
                 * currently support this, but probably will someday).
                 */
                AlterConstraintNamespaces(typform->typrelid, oldNspOid,
-                                                                 nspOid, false);
+                                                                 nspOid, false, objsMoved);
        }
        else
        {
                /* If it's a domain, it might have constraints */
                if (typform->typtype == TYPTYPE_DOMAIN)
-                       AlterConstraintNamespaces(typeOid, oldNspOid, nspOid, true);
+                       AlterConstraintNamespaces(typeOid, oldNspOid, nspOid, true,
+                                                                         objsMoved);
        }
 
        /*
@@ -2960,9 +2976,11 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
 
        heap_close(rel, RowExclusiveLock);
 
+       add_exact_object_address(&thisobj, objsMoved);
+
        /* Recursively alter the associated array type, if any */
        if (OidIsValid(arrayOid))
-               AlterTypeNamespaceInternal(arrayOid, nspOid, true, true);
+               AlterTypeNamespaceInternal(arrayOid, nspOid, true, true, objsMoved);
 
        return oldNspOid;
 }
index 1566af2973549f7a9b2c9fd8de34b004ca41b105..56b6ca2f50f08b89b51843ec2a57f869a55f7404 100644 (file)
@@ -20,6 +20,7 @@
 #define PG_CONSTRAINT_H
 
 #include "catalog/genbki.h"
+#include "catalog/dependency.h"
 #include "nodes/pg_list.h"
 
 /* ----------------
@@ -240,7 +241,7 @@ extern char *ChooseConstraintName(const char *name1, const char *name2,
                                         List *others);
 
 extern void AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
-                                                 Oid newNspId, bool isType);
+                                                 Oid newNspId, bool isType, ObjectAddresses *objsMoved);
 extern Oid     get_constraint_oid(Oid relid, const char *conname, bool missing_ok);
 
 extern bool check_functional_grouping(Oid relid,
index e942b538af915528e25f958667f4667c092a9be7..c34c5bc090d7dd73efd75b578780a49295b0d8f5 100644 (file)
 #ifndef ALTER_H
 #define ALTER_H
 
+#include "catalog/dependency.h"
 #include "nodes/parsenodes.h"
 #include "utils/acl.h"
 #include "utils/relcache.h"
 
 extern void ExecRenameStmt(RenameStmt *stmt);
 extern void ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt);
-extern Oid     AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid);
-extern Oid AlterObjectNamespace(Relation rel, int oidCacheId, int nameCacheId,
+extern Oid     AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid,
+                                                ObjectAddresses *objsMoved);
+extern Oid     AlterObjectNamespace(Relation rel, int oidCacheId, int nameCacheId,
                                         Oid objid, Oid nspOid,
                                         int Anum_name, int Anum_namespace, int Anum_owner,
                                         AclObjectKind acl_kind);
index c0ce9755b43fad93ab38aca11aeadd49d7f3889e..293e4e5bc9e0d3fc91c5bd61c7ea5fb4abeb2445 100644 (file)
@@ -15,6 +15,7 @@
 #define TABLECMDS_H
 
 #include "access/htup.h"
+#include "catalog/dependency.h"
 #include "nodes/parsenodes.h"
 #include "storage/lock.h"
 #include "utils/relcache.h"
@@ -35,9 +36,13 @@ extern void AlterTableInternal(Oid relid, List *cmds, bool recurse);
 extern void AlterTableNamespace(RangeVar *relation, const char *newschema,
                                        ObjectType stmttype, LOCKMODE lockmode);
 
+extern void AlterTableNamespaceInternal(Relation rel, Oid oldNspOid,
+                                                       Oid nspOid, ObjectAddresses *objsMoved);
+
 extern void AlterRelationNamespaceInternal(Relation classRel, Oid relOid,
                                                           Oid oldNspOid, Oid newNspOid,
-                                                          bool hasDependEntry);
+                                                          bool hasDependEntry,
+                                                          ObjectAddresses *objsMoved);
 
 extern void CheckTableNotInUse(Relation rel, const char *stmt);
 
index 6d9d1ccaa95d86e07177d634fbc136071b5b5d77..e22a699657d2f6e7630e54266515cef5338cb5f5 100644 (file)
@@ -14,6 +14,7 @@
 #ifndef TYPECMDS_H
 #define TYPECMDS_H
 
+#include "catalog/dependency.h"
 #include "nodes/parsenodes.h"
 
 
@@ -41,9 +42,10 @@ extern void AlterTypeOwner(List *names, Oid newOwnerId);
 extern void AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId,
                                           bool hasDependEntry);
 extern void AlterTypeNamespace(List *names, const char *newschema);
-extern Oid     AlterTypeNamespace_oid(Oid typeOid, Oid nspOid);
-extern Oid AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
+extern Oid     AlterTypeNamespace_oid(Oid typeOid, Oid nspOid, ObjectAddresses *objsMoved);
+extern Oid     AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
                                                   bool isImplicitArray,
-                                                  bool errorOnTableType);
+                                                  bool errorOnTableType,
+                                                  ObjectAddresses *objsMoved);
 
 #endif   /* TYPECMDS_H */