]> granicus.if.org Git - postgresql/commitdiff
Avoid repeated name lookups during table and index DDL.
authorRobert Haas <rhaas@postgresql.org>
Mon, 17 Feb 2014 14:33:31 +0000 (09:33 -0500)
committerRobert Haas <rhaas@postgresql.org>
Mon, 17 Feb 2014 14:33:36 +0000 (09:33 -0500)
If the name lookups come to different conclusions due to concurrent
activity, we might perform some parts of the DDL on a different table
than other parts.  At least in the case of CREATE INDEX, this can be
used to cause the permissions checks to be performed against a
different table than the index creation, allowing for a privilege
escalation attack.

This changes the calling convention for DefineIndex, CreateTrigger,
transformIndexStmt, transformAlterTableStmt, CheckIndexCompatible
(in 9.2 and newer), and AlterTable (in 9.1 and older).  In addition,
CheckRelationOwnership is removed in 9.2 and newer and the calling
convention is changed in older branches.  A field has also been added
to the Constraint node (FkConstraint in 8.4).  Third-party code calling
these functions or using the Constraint node will require updating.

Report by Andres Freund.  Patch by Robert Haas and Andres Freund,
reviewed by Tom Lane.

Security: CVE-2014-0062

20 files changed:
src/backend/bootstrap/bootparse.y
src/backend/catalog/index.c
src/backend/catalog/pg_constraint.c
src/backend/commands/alter.c
src/backend/commands/indexcmds.c
src/backend/commands/tablecmds.c
src/backend/commands/trigger.c
src/backend/nodes/copyfuncs.c
src/backend/nodes/equalfuncs.c
src/backend/nodes/outfuncs.c
src/backend/parser/parse_utilcmd.c
src/backend/tcop/utility.c
src/include/catalog/index.h
src/include/catalog/pg_constraint.h
src/include/commands/defrem.h
src/include/commands/tablecmds.h
src/include/commands/trigger.h
src/include/nodes/parsenodes.h
src/include/parser/parse_utilcmd.h
src/include/tcop/utility.h

index f3e85aa31bb78a26ade174cbe500c58f88bf8f42..c6b755628b2d84200885fb1a22fbb5ebab8bc94b 100644 (file)
@@ -27,6 +27,7 @@
 #include "bootstrap/bootstrap.h"
 #include "catalog/catalog.h"
 #include "catalog/heap.h"
+#include "catalog/namespace.h"
 #include "catalog/pg_am.h"
 #include "catalog/pg_attribute.h"
 #include "catalog/pg_authid.h"
@@ -279,9 +280,14 @@ Boot_InsertStmt:
 Boot_DeclareIndexStmt:
                  XDECLARE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN
                                {
+                                       Oid             relationId;
+
                                        do_start();
 
-                                       DefineIndex(makeRangeVar(NULL, $6, -1),
+                                       relationId = RangeVarGetRelid(makeRangeVar(NULL, $6, -1),
+                                                                                                 false);
+
+                                       DefineIndex(relationId,
                                                                $3,
                                                                $4,
                                                                $8,
@@ -297,9 +303,14 @@ Boot_DeclareIndexStmt:
 Boot_DeclareUniqueIndexStmt:
                  XDECLARE UNIQUE INDEX boot_ident oidspec ON boot_ident USING boot_ident LPAREN boot_index_params RPAREN
                                {
+                                       Oid             relationId;
+
                                        do_start();
 
-                                       DefineIndex(makeRangeVar(NULL, $7, -1),
+                                       relationId = RangeVarGetRelid(makeRangeVar(NULL, $7, -1),
+                                                                                                 false);
+
+                                       DefineIndex(relationId,
                                                                $4,
                                                                $5,
                                                                $9,
index 129a1ac11f0e79408961abd73f0a33395569c5d5..4e6222b3ba98b5e652594608925c1305299d037e 100644 (file)
@@ -115,7 +115,6 @@ static void validate_index_heapscan(Relation heapRelation,
                                                IndexInfo *indexInfo,
                                                Snapshot snapshot,
                                                v_i_state *state);
-static Oid     IndexGetRelation(Oid indexId);
 static bool ReindexIsCurrentlyProcessingIndex(Oid indexOid);
 static void SetReindexProcessing(Oid heapOid, Oid indexOid);
 static void ResetReindexProcessing(void);
@@ -1202,18 +1201,13 @@ index_constraint_create(Relation heapRelation,
         */
        if (deferrable)
        {
-               RangeVar   *heapRel;
                CreateTrigStmt *trigger;
 
-               heapRel = makeRangeVar(get_namespace_name(namespaceId),
-                                                          pstrdup(RelationGetRelationName(heapRelation)),
-                                                          -1);
-
                trigger = makeNode(CreateTrigStmt);
                trigger->trigname = (constraintType == CONSTRAINT_PRIMARY) ?
                        "PK_ConstraintTrigger" :
                        "Unique_ConstraintTrigger";
-               trigger->relation = heapRel;
+               trigger->relation = NULL;
                trigger->funcname = SystemFuncName("unique_key_recheck");
                trigger->args = NIL;
                trigger->row = true;
@@ -1226,7 +1220,8 @@ index_constraint_create(Relation heapRelation,
                trigger->initdeferred = initdeferred;
                trigger->constrrel = NULL;
 
-               (void) CreateTrigger(trigger, NULL, conOid, indexRelationId, true);
+               (void) CreateTrigger(trigger, NULL, RelationGetRelid(heapRelation),
+                                                        InvalidOid, conOid, indexRelationId, true);
        }
 
        /*
@@ -2833,7 +2828,7 @@ index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
  * IndexGetRelation: given an index's relation OID, get the OID of the
  * relation it is an index on. Uses the system cache.
  */
-static Oid
+Oid
 IndexGetRelation(Oid indexId)
 {
        HeapTuple       tuple;
index 71c48ffa92372a27f109c57ed5d109d1b4ee2960..79c5cfa345da1b9332ad382bbe881542a8b1fbe9 100644 (file)
@@ -742,6 +742,25 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
        heap_close(conRel, RowExclusiveLock);
 }
 
+/*
+ * get_constraint_relation_oids
+ *             Find the IDs of the relations to which a constraint refers.
+ */
+void
+get_constraint_relation_oids(Oid constraint_oid, Oid *conrelid, Oid *confrelid)
+{
+       HeapTuple       tup;
+       Form_pg_constraint      con;
+
+       tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constraint_oid));
+       if (!HeapTupleIsValid(tup)) /* should not happen */
+               elog(ERROR, "cache lookup failed for constraint %u", constraint_oid);
+       con = (Form_pg_constraint) GETSTRUCT(tup);
+       *conrelid = con->conrelid;
+       *confrelid = con->confrelid;
+       ReleaseSysCache(tup);
+}
+
 /*
  * get_constraint_oid
  *             Find a constraint on the specified relation with the specified name.
index 0f4e0f701abcf868748a0b0bb21a80112814be94..91df1a0aca758d33ef85d4462d99d84ae7173904 100644 (file)
@@ -105,9 +105,8 @@ ExecRenameStmt(RenameStmt *stmt)
                        {
                                Oid                     relid;
 
-                               CheckRelationOwnership(stmt->relation, true);
-
                                relid = RangeVarGetRelid(stmt->relation, false);
+                               CheckRelationOwnership(relid, true);
 
                                switch (stmt->renameType)
                                {
@@ -223,7 +222,6 @@ ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt)
                case OBJECT_TABLE:
                case OBJECT_VIEW:
                case OBJECT_FOREIGN_TABLE:
-                       CheckRelationOwnership(stmt->relation, true);
                        AlterTableNamespace(stmt->relation, stmt->newschema,
                                                                stmt->objectType, AccessExclusiveLock);
                        break;
index ab73567ad2a152a612b324f0ad198b3b6f0bb204..1a0ee84963d2c3b8f7afad41279592f0a6fe24f2 100644 (file)
@@ -76,7 +76,8 @@ static char *ChooseIndexNameAddition(List *colnames);
  * DefineIndex
  *             Creates a new index.
  *
- * 'heapRelation': the relation the index will apply to.
+ * 'relationId': the OID of the heap relation on which the index is to be
+ *             created
  * 'indexRelationName': the name for the new index, or NULL to indicate
  *             that a nonconflicting default name should be picked.
  * 'indexRelationId': normally InvalidOid, but during bootstrap can be
@@ -105,7 +106,7 @@ static char *ChooseIndexNameAddition(List *colnames);
  * 'concurrent': avoid blocking writers to the table while building.
  */
 void
-DefineIndex(RangeVar *heapRelation,
+DefineIndex(Oid relationId,
                        char *indexRelationName,
                        Oid indexRelationId,
                        char *accessMethodName,
@@ -128,7 +129,6 @@ DefineIndex(RangeVar *heapRelation,
        Oid                *collationObjectId;
        Oid                *classObjectId;
        Oid                     accessMethodId;
-       Oid                     relationId;
        Oid                     namespaceId;
        Oid                     tablespaceId;
        List       *indexColNames;
@@ -148,6 +148,7 @@ DefineIndex(RangeVar *heapRelation,
        int                     n_old_snapshots;
        LockRelId       heaprelid;
        LOCKTAG         heaplocktag;
+       LOCKMODE        lockmode;
        Snapshot        snapshot;
        int                     i;
 
@@ -166,14 +167,18 @@ DefineIndex(RangeVar *heapRelation,
                                                INDEX_MAX_KEYS)));
 
        /*
-        * Open heap relation, acquire a suitable lock on it, remember its OID
-        *
         * Only SELECT ... FOR UPDATE/SHARE are allowed while doing a standard
         * index build; but for concurrent builds we allow INSERT/UPDATE/DELETE
         * (but not VACUUM).
+        *
+        * NB: Caller is responsible for making sure that relationId refers
+        * to the relation on which the index should be built; except in bootstrap
+        * mode, this will typically require the caller to have already locked
+        * the relation.  To avoid lock upgrade hazards, that lock should be at
+        * least as strong as the one we take here.
         */
-       rel = heap_openrv(heapRelation,
-                                         (concurrent ? ShareUpdateExclusiveLock : ShareLock));
+       lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock;
+       rel = heap_open(relationId, lockmode);
 
        relationId = RelationGetRelid(rel);
        namespaceId = RelationGetNamespace(rel);
@@ -191,12 +196,12 @@ DefineIndex(RangeVar *heapRelation,
                        ereport(ERROR,
                                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                                         errmsg("cannot create index on foreign table \"%s\"",
-                                                       heapRelation->relname)));
+                                                       RelationGetRelationName(rel))));
                else
                        ereport(ERROR,
                                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                                         errmsg("\"%s\" is not a table",
-                                                       heapRelation->relname)));
+                                                       RelationGetRelationName(rel))));
        }
 
        /*
@@ -503,7 +508,7 @@ DefineIndex(RangeVar *heapRelation,
         */
 
        /* Open and lock the parent heap relation */
-       rel = heap_openrv(heapRelation, ShareUpdateExclusiveLock);
+       rel = heap_open(relationId, ShareUpdateExclusiveLock);
 
        /* And the target index relation */
        indexRelation = index_open(indexRelationId, RowExclusiveLock);
index adf278bb1fab72659f9ee8392f616211174f71b7..ae62c6984998bbc9a4e21073518f8b4e354c30e8 100644 (file)
@@ -73,6 +73,7 @@
 #include "storage/lock.h"
 #include "storage/predicate.h"
 #include "storage/smgr.h"
+#include "tcop/utility.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
@@ -272,7 +273,8 @@ static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts);
 static void validateForeignKeyConstraint(char *conname,
                                                         Relation rel, Relation pkrel,
                                                         Oid pkindOid, Oid constraintOid);
-static void createForeignKeyTriggers(Relation rel, Constraint *fkconstraint,
+static void createForeignKeyTriggers(Relation rel, Oid refRelOid,
+                                                Constraint *fkconstraint,
                                                 Oid constraintOid, Oid indexOid);
 static void ATController(Relation rel, List *cmds, bool recurse, LOCKMODE lockmode);
 static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
@@ -346,7 +348,8 @@ static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno);
 static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
                                          AlterTableCmd *cmd, LOCKMODE lockmode);
 static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode);
-static void ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode);
+static void ATPostAlterTypeParse(Oid oldRelId, Oid refRelId, char *cmd,
+                                        List **wqueue, LOCKMODE lockmode);
 static void change_owner_fix_column_acls(Oid relationOid,
                                                         Oid oldOwnerId, Oid newOwnerId);
 static void change_owner_recurse_to_sequences(Oid relationOid,
@@ -2378,15 +2381,13 @@ CheckTableNotInUse(Relation rel, const char *stmt)
  * rather than reassess it at lower levels.
  */
 void
-AlterTable(AlterTableStmt *stmt)
+AlterTable(Oid relid, AlterTableStmt *stmt)
 {
        Relation        rel;
        LOCKMODE        lockmode = AlterTableGetLockLevel(stmt->cmds);
 
-       /*
-        * Acquire same level of lock as already acquired during parsing.
-        */
-       rel = relation_openrv(stmt->relation, lockmode);
+       /* Caller is required to provide an adequate lock. */
+       rel = relation_open(relid, NoLock);
 
        CheckTableNotInUse(rel, "ALTER TABLE");
 
@@ -5153,7 +5154,7 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
 
        /* The IndexStmt has already been through transformIndexStmt */
 
-       DefineIndex(stmt->relation, /* relation */
+       DefineIndex(RelationGetRelid(rel), /* relation */
                                stmt->idxname,  /* index name */
                                InvalidOid,             /* no predefined OID */
                                stmt->accessMethod,             /* am name */
@@ -5465,7 +5466,10 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
         * table; trying to start with a lesser lock will just create a risk of
         * deadlock.)
         */
-       pkrel = heap_openrv(fkconstraint->pktable, AccessExclusiveLock);
+       if (OidIsValid(fkconstraint->old_pktable_oid))
+               pkrel = heap_open(fkconstraint->old_pktable_oid, AccessExclusiveLock);
+       else
+               pkrel = heap_openrv(fkconstraint->pktable, AccessExclusiveLock);
 
        /*
         * Validity checks (permission checks wait till we have the column
@@ -5713,7 +5717,8 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
        /*
         * Create the triggers that will enforce the constraint.
         */
-       createForeignKeyTriggers(rel, fkconstraint, constrOid, indexOid);
+       createForeignKeyTriggers(rel, RelationGetRelid(pkrel), fkconstraint,
+                                                        constrOid, indexOid);
 
        /*
         * Tell Phase 3 to check that the constraint is satisfied by existing rows.
@@ -6202,14 +6207,14 @@ validateForeignKeyConstraint(char *conname,
 }
 
 static void
-CreateFKCheckTrigger(RangeVar *myRel, Constraint *fkconstraint,
+CreateFKCheckTrigger(Oid myRelOid, Oid refRelOid, Constraint *fkconstraint,
                                         Oid constraintOid, Oid indexOid, bool on_insert)
 {
        CreateTrigStmt *fk_trigger;
 
        fk_trigger = makeNode(CreateTrigStmt);
        fk_trigger->trigname = "RI_ConstraintTrigger";
-       fk_trigger->relation = myRel;
+       fk_trigger->relation = NULL;
        fk_trigger->row = true;
        fk_trigger->timing = TRIGGER_TYPE_AFTER;
 
@@ -6230,10 +6235,11 @@ CreateFKCheckTrigger(RangeVar *myRel, Constraint *fkconstraint,
        fk_trigger->isconstraint = true;
        fk_trigger->deferrable = fkconstraint->deferrable;
        fk_trigger->initdeferred = fkconstraint->initdeferred;
-       fk_trigger->constrrel = fkconstraint->pktable;
+       fk_trigger->constrrel = NULL;
        fk_trigger->args = NIL;
 
-       (void) CreateTrigger(fk_trigger, NULL, constraintOid, indexOid, true);
+       (void) CreateTrigger(fk_trigger, NULL, myRelOid, refRelOid, constraintOid,
+                                                indexOid, true);
 
        /* Make changes-so-far visible */
        CommandCounterIncrement();
@@ -6243,18 +6249,13 @@ CreateFKCheckTrigger(RangeVar *myRel, Constraint *fkconstraint,
  * Create the triggers that implement an FK constraint.
  */
 static void
-createForeignKeyTriggers(Relation rel, Constraint *fkconstraint,
+createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint,
                                                 Oid constraintOid, Oid indexOid)
 {
-       RangeVar   *myRel;
+       Oid                     myRelOid;
        CreateTrigStmt *fk_trigger;
 
-       /*
-        * Reconstruct a RangeVar for my relation (not passed in, unfortunately).
-        */
-       myRel = makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
-                                                pstrdup(RelationGetRelationName(rel)),
-                                                -1);
+       myRelOid = RelationGetRelid(rel);
 
        /* Make changes-so-far visible */
        CommandCounterIncrement();
@@ -6265,14 +6266,14 @@ createForeignKeyTriggers(Relation rel, Constraint *fkconstraint,
         */
        fk_trigger = makeNode(CreateTrigStmt);
        fk_trigger->trigname = "RI_ConstraintTrigger";
-       fk_trigger->relation = fkconstraint->pktable;
+       fk_trigger->relation = NULL;
        fk_trigger->row = true;
        fk_trigger->timing = TRIGGER_TYPE_AFTER;
        fk_trigger->events = TRIGGER_TYPE_DELETE;
        fk_trigger->columns = NIL;
        fk_trigger->whenClause = NULL;
        fk_trigger->isconstraint = true;
-       fk_trigger->constrrel = myRel;
+       fk_trigger->constrrel = NULL;
        switch (fkconstraint->fk_del_action)
        {
                case FKCONSTR_ACTION_NOACTION:
@@ -6307,7 +6308,8 @@ createForeignKeyTriggers(Relation rel, Constraint *fkconstraint,
        }
        fk_trigger->args = NIL;
 
-       (void) CreateTrigger(fk_trigger, NULL, constraintOid, indexOid, true);
+       (void) CreateTrigger(fk_trigger, NULL, refRelOid, myRelOid, constraintOid,
+                                                indexOid, true);
 
        /* Make changes-so-far visible */
        CommandCounterIncrement();
@@ -6318,14 +6320,14 @@ createForeignKeyTriggers(Relation rel, Constraint *fkconstraint,
         */
        fk_trigger = makeNode(CreateTrigStmt);
        fk_trigger->trigname = "RI_ConstraintTrigger";
-       fk_trigger->relation = fkconstraint->pktable;
+       fk_trigger->relation = NULL;
        fk_trigger->row = true;
        fk_trigger->timing = TRIGGER_TYPE_AFTER;
        fk_trigger->events = TRIGGER_TYPE_UPDATE;
        fk_trigger->columns = NIL;
        fk_trigger->whenClause = NULL;
        fk_trigger->isconstraint = true;
-       fk_trigger->constrrel = myRel;
+       fk_trigger->constrrel = NULL;
        switch (fkconstraint->fk_upd_action)
        {
                case FKCONSTR_ACTION_NOACTION:
@@ -6360,7 +6362,8 @@ createForeignKeyTriggers(Relation rel, Constraint *fkconstraint,
        }
        fk_trigger->args = NIL;
 
-       (void) CreateTrigger(fk_trigger, NULL, constraintOid, indexOid, true);
+       (void) CreateTrigger(fk_trigger, NULL, refRelOid, myRelOid, constraintOid,
+                                                indexOid, true);
 
        /* Make changes-so-far visible */
        CommandCounterIncrement();
@@ -6380,8 +6383,10 @@ createForeignKeyTriggers(Relation rel, Constraint *fkconstraint,
         * and the use of self-referential FKs is rare enough, that we live with
         * it for now.  There will be a real fix in PG 9.2.
         */
-       CreateFKCheckTrigger(myRel, fkconstraint, constraintOid, indexOid, true);
-       CreateFKCheckTrigger(myRel, fkconstraint, constraintOid, indexOid, false);
+       CreateFKCheckTrigger(myRelOid, refRelOid, fkconstraint, constraintOid,
+                                                indexOid, true);
+       CreateFKCheckTrigger(myRelOid, refRelOid, fkconstraint, constraintOid,
+                                                indexOid, false);
 }
 
 /*
@@ -7179,7 +7184,8 @@ static void
 ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
 {
        ObjectAddress obj;
-       ListCell   *l;
+       ListCell   *def_item;
+       ListCell   *oid_item;
 
        /*
         * Re-parse the index and constraint definitions, and attach them to the
@@ -7188,11 +7194,36 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
         * lock on the table the constraint is attached to, and we need to get
         * that before dropping.  It's safe because the parser won't actually look
         * at the catalogs to detect the existing entry.
+        *
+        * We can't rely on the output of deparsing to tell us which relation
+        * to operate on, because concurrent activity might have made the name
+        * resolve differently.  Instead, we've got to use the OID of the
+        * constraint or index we're processing to figure out which relation
+        * to operate on.
         */
-       foreach(l, tab->changedIndexDefs)
-               ATPostAlterTypeParse((char *) lfirst(l), wqueue, lockmode);
-       foreach(l, tab->changedConstraintDefs)
-               ATPostAlterTypeParse((char *) lfirst(l), wqueue, lockmode);
+       forboth(oid_item, tab->changedConstraintOids,
+                       def_item, tab->changedConstraintDefs)
+       {
+               Oid             oldId = lfirst_oid(oid_item);
+               Oid             relid;
+               Oid             confrelid;
+
+               get_constraint_relation_oids(oldId, &relid, &confrelid);
+               ATPostAlterTypeParse(relid, confrelid,
+                                                        (char *) lfirst(def_item),
+                                                        wqueue, lockmode);
+       }
+       forboth(oid_item, tab->changedIndexOids,
+                       def_item, tab->changedIndexDefs)
+       {
+               Oid             oldId = lfirst_oid(oid_item);
+               Oid             relid;
+
+               relid = IndexGetRelation(oldId);
+               ATPostAlterTypeParse(relid, InvalidOid,
+                                                        (char *) lfirst(def_item),
+                                                        wqueue, lockmode);
+       }
 
        /*
         * Now we can drop the existing constraints and indexes --- constraints
@@ -7202,18 +7233,18 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
         * should be okay to use DROP_RESTRICT here, since nothing else should be
         * depending on these objects.
         */
-       foreach(l, tab->changedConstraintOids)
+       foreach(oid_item, tab->changedConstraintOids)
        {
                obj.classId = ConstraintRelationId;
-               obj.objectId = lfirst_oid(l);
+               obj.objectId = lfirst_oid(oid_item);
                obj.objectSubId = 0;
                performDeletion(&obj, DROP_RESTRICT);
        }
 
-       foreach(l, tab->changedIndexOids)
+       foreach(oid_item, tab->changedIndexOids)
        {
                obj.classId = RelationRelationId;
-               obj.objectId = lfirst_oid(l);
+               obj.objectId = lfirst_oid(oid_item);
                obj.objectSubId = 0;
                performDeletion(&obj, DROP_RESTRICT);
        }
@@ -7225,11 +7256,13 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
 }
 
 static void
-ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
+ATPostAlterTypeParse(Oid oldRelId, Oid refRelId, char *cmd, List **wqueue,
+                                        LOCKMODE lockmode)
 {
        List       *raw_parsetree_list;
        List       *querytree_list;
        ListCell   *list_item;
+       Relation        rel;
 
        /*
         * We expect that we will get only ALTER TABLE and CREATE INDEX
@@ -7245,16 +7278,21 @@ ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
 
                if (IsA(stmt, IndexStmt))
                        querytree_list = lappend(querytree_list,
-                                                                        transformIndexStmt((IndexStmt *) stmt,
+                                                                        transformIndexStmt(oldRelId,
+                                                                                                               (IndexStmt *) stmt,
                                                                                                                cmd));
                else if (IsA(stmt, AlterTableStmt))
                        querytree_list = list_concat(querytree_list,
-                                                        transformAlterTableStmt((AlterTableStmt *) stmt,
+                                                        transformAlterTableStmt(oldRelId,
+                                                                                                        (AlterTableStmt *) stmt,
                                                                                                         cmd));
                else
                        querytree_list = lappend(querytree_list, stmt);
        }
 
+       /* Caller should already have acquired whatever lock we need. */
+       rel = relation_open(oldRelId, NoLock);
+
        /*
         * Attach each generated command to the proper place in the work queue.
         * Note this could result in creation of entirely new work-queue entries.
@@ -7266,7 +7304,6 @@ ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
        foreach(list_item, querytree_list)
        {
                Node       *stm = (Node *) lfirst(list_item);
-               Relation        rel;
                AlteredTableInfo *tab;
 
                switch (nodeTag(stm))
@@ -7276,14 +7313,12 @@ ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
                                        IndexStmt  *stmt = (IndexStmt *) stm;
                                        AlterTableCmd *newcmd;
 
-                                       rel = relation_openrv(stmt->relation, lockmode);
                                        tab = ATGetQueueEntry(wqueue, rel);
                                        newcmd = makeNode(AlterTableCmd);
                                        newcmd->subtype = AT_ReAddIndex;
                                        newcmd->def = (Node *) stmt;
                                        tab->subcmds[AT_PASS_OLD_INDEX] =
                                                lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
-                                       relation_close(rel, NoLock);
                                        break;
                                }
                        case T_AlterTableStmt:
@@ -7291,7 +7326,6 @@ ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
                                        AlterTableStmt *stmt = (AlterTableStmt *) stm;
                                        ListCell   *lcmd;
 
-                                       rel = relation_openrv(stmt->relation, lockmode);
                                        tab = ATGetQueueEntry(wqueue, rel);
                                        foreach(lcmd, stmt->cmds)
                                        {
@@ -7314,7 +7348,6 @@ ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
                                                                         (int) cmd->subtype);
                                                }
                                        }
-                                       relation_close(rel, NoLock);
                                        break;
                                }
                        default:
@@ -7322,8 +7355,9 @@ ATPostAlterTypeParse(char *cmd, List **wqueue, LOCKMODE lockmode)
                                         (int) nodeTag(stm));
                }
        }
-}
 
+       relation_close(rel, NoLock);
+}
 
 /*
  * ALTER TABLE OWNER
@@ -9002,7 +9036,8 @@ ATExecGenericOptions(Relation rel, List *options)
 /*
  * Execute ALTER TABLE SET SCHEMA
  *
- * Note: caller must have checked ownership of the relation already
+ * WARNING WARNING WARNING: In previous *minor* releases the caller was
+ * responsible for checking ownership of the relation, but now we do it here.
  */
 void
 AlterTableNamespace(RangeVar *relation, const char *newschema,
@@ -9017,6 +9052,7 @@ AlterTableNamespace(RangeVar *relation, const char *newschema,
        rel = relation_openrv(relation, lockmode);
 
        relid = RelationGetRelid(rel);
+       CheckRelationOwnership(relid, true);
        oldNspOid = RelationGetNamespace(rel);
 
        /* Check relation type against type specified in the ALTER command */
index ab87562d0cef3763a4bc56156d57c7d7d2cf32d9..3c2447671c7266738a594a4b2e025e5ed53773cd 100644 (file)
@@ -92,6 +92,13 @@ static void AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
  * queryString is the source text of the CREATE TRIGGER command.
  * This must be supplied if a whenClause is specified, else it can be NULL.
  *
+ * relOid, if nonzero, is the relation on which the trigger should be
+ * created.  If zero, the name provided in the statement will be looked up.
+ *
+ * refRelOid, if nonzero, is the relation to which the constraint trigger
+ * refers.  If zero, the constraint relation name provided in the statement
+ * will be looked up as needed.
+ *
  * constraintOid, if nonzero, says that this trigger is being created
  * internally to implement that constraint.  A suitable pg_depend entry will
  * be made to link the trigger to that constraint.     constraintOid is zero when
@@ -114,7 +121,7 @@ static void AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
  */
 Oid
 CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
-                         Oid constraintOid, Oid indexOid,
+                         Oid relOid, Oid refRelOid, Oid constraintOid, Oid indexOid,
                          bool isInternal)
 {
        int16           tgtype;
@@ -143,7 +150,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
        ObjectAddress myself,
                                referenced;
 
-       rel = heap_openrv(stmt->relation, AccessExclusiveLock);
+       if (OidIsValid(relOid))
+               rel = heap_open(relOid, AccessExclusiveLock);
+       else
+               rel = heap_openrv(stmt->relation, AccessExclusiveLock);
 
        /*
         * Triggers must be on tables or views, and there are additional
@@ -192,8 +202,13 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
                                 errmsg("permission denied: \"%s\" is a system catalog",
                                                RelationGetRelationName(rel))));
 
-       if (stmt->isconstraint && stmt->constrrel != NULL)
-               constrrelid = RangeVarGetRelid(stmt->constrrel, false);
+       if (stmt->isconstraint)
+       {
+               if (OidIsValid(refRelOid))
+                       constrrelid = refRelOid;
+               else if (stmt->constrrel != NULL)
+                       constrrelid = RangeVarGetRelid(stmt->constrrel, false);
+       }
 
        /* permission checks */
        if (!isInternal)
@@ -501,7 +516,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
                                ereport(ERROR,
                                                (errcode(ERRCODE_DUPLICATE_OBJECT),
                                  errmsg("trigger \"%s\" for relation \"%s\" already exists",
-                                                trigname, stmt->relation->relname)));
+                                                trigname, RelationGetRelationName(rel))));
                }
                systable_endscan(tgscan);
        }
index 6f5ebb277d039cb3e63bdd872ffd05c21716083f..09ba0732b4389779fbbd059a610adb53898f3ccc 100644 (file)
@@ -2343,6 +2343,7 @@ _copyConstraint(Constraint *from)
        COPY_SCALAR_FIELD(fk_del_action);
        COPY_SCALAR_FIELD(skip_validation);
        COPY_SCALAR_FIELD(initially_valid);
+       COPY_SCALAR_FIELD(old_pktable_oid);
 
        return newnode;
 }
index debde12ca9ec014fa86290aae298fa69801f0a0f..0de60d5c6c1e91f0b301ad35a0ddd1dfe4a3be04 100644 (file)
@@ -2271,6 +2271,7 @@ _equalConstraint(Constraint *a, Constraint *b)
        COMPARE_SCALAR_FIELD(fk_del_action);
        COMPARE_SCALAR_FIELD(skip_validation);
        COMPARE_SCALAR_FIELD(initially_valid);
+       COMPARE_SCALAR_FIELD(old_pktable_oid);
 
        return true;
 }
index 085a45808538b833e03551b4acf20fb5372bb557..bb342ca6d39ae6b8d99a2eae9301fe6b2dab4c5b 100644 (file)
@@ -2630,6 +2630,7 @@ _outConstraint(StringInfo str, Constraint *node)
                        WRITE_CHAR_FIELD(fk_del_action);
                        WRITE_BOOL_FIELD(skip_validation);
                        WRITE_BOOL_FIELD(initially_valid);
+                       WRITE_OID_FIELD(old_pktable_oid);
                        break;
 
                case CONSTR_ATTR_DEFERRABLE:
index 9c39bacc64fb45979b678115942d456b8d2a6878..387bcb8ab91c8e7fde375c3ff940b7513990dc99 100644 (file)
@@ -1860,14 +1860,18 @@ transformFKConstraints(CreateStmtContext *cxt,
  * a predicate expression.     There are several code paths that create indexes
  * without bothering to call this, because they know they don't have any
  * such expressions to deal with.
+ *
+ * To avoid race conditions, it's important that this function rely only on
+ * the passed-in relid (and not on stmt->relation) to determine the target
+ * relation.
  */
 IndexStmt *
-transformIndexStmt(IndexStmt *stmt, const char *queryString)
+transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString)
 {
-       Relation        rel;
        ParseState *pstate;
        RangeTblEntry *rte;
        ListCell   *l;
+       Relation        rel;
 
        /*
         * We must not scribble on the passed-in IndexStmt, so copy it.  (This is
@@ -1875,26 +1879,17 @@ transformIndexStmt(IndexStmt *stmt, const char *queryString)
         */
        stmt = (IndexStmt *) copyObject(stmt);
 
-       /*
-        * Open the parent table with appropriate locking.      We must do this
-        * because addRangeTableEntry() would acquire only AccessShareLock,
-        * leaving DefineIndex() needing to do a lock upgrade with consequent risk
-        * of deadlock.  Make sure this stays in sync with the type of lock
-        * DefineIndex() wants. If we are being called by ALTER TABLE, we will
-        * already hold a higher lock.
-        */
-       rel = heap_openrv(stmt->relation,
-                                 (stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock));
-
        /* Set up pstate */
        pstate = make_parsestate(NULL);
        pstate->p_sourcetext = queryString;
 
        /*
         * Put the parent table into the rtable so that the expressions can refer
-        * to its fields without qualification.
+        * to its fields without qualification.  Caller is responsible for locking
+        * relation, but we still need to open it.
         */
-       rte = addRangeTableEntry(pstate, stmt->relation, NULL, false, true);
+       rel = relation_open(relid, NoLock);
+       rte = addRangeTableEntryForRelation(pstate, rel, NULL, false, true);
 
        /* no to join list, yes to namespaces */
        addRTEtoQuery(pstate, rte, false, true, true);
@@ -1948,7 +1943,7 @@ transformIndexStmt(IndexStmt *stmt, const char *queryString)
 
        free_parsestate(pstate);
 
-       /* Close relation, but keep the lock */
+       /* Close relation */
        heap_close(rel, NoLock);
 
        return stmt;
@@ -2270,9 +2265,14 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString,
  * Returns a List of utility commands to be done in sequence.  One of these
  * will be the transformed AlterTableStmt, but there may be additional actions
  * to be done before and after the actual AlterTable() call.
+ *
+ * To avoid race conditions, it's important that this function rely only on
+ * the passed-in relid (and not on stmt->relation) to determine the target
+ * relation.
  */
 List *
-transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
+transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
+                                               const char *queryString)
 {
        Relation        rel;
        ParseState *pstate;
@@ -2284,7 +2284,6 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
        List       *newcmds = NIL;
        bool            skipValidation = true;
        AlterTableCmd *newcmd;
-       LOCKMODE        lockmode;
 
        /*
         * We must not scribble on the passed-in AlterTableStmt, so copy it. (This
@@ -2292,21 +2291,8 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
         */
        stmt = (AlterTableStmt *) copyObject(stmt);
 
-       /*
-        * Determine the appropriate lock level for this list of subcommands.
-        */
-       lockmode = AlterTableGetLockLevel(stmt->cmds);
-
-       /*
-        * Acquire appropriate lock on the target relation, which will be held
-        * until end of transaction.  This ensures any decisions we make here
-        * based on the state of the relation will still be good at execution. We
-        * must get lock now because execution will later require it; taking a
-        * lower grade lock now and trying to upgrade later risks deadlock.  Any
-        * new commands we add after this must not upgrade the lock level
-        * requested here.
-        */
-       rel = relation_openrv(stmt->relation, lockmode);
+       /* Caller is responsible for locking the relation */
+       rel = relation_open(relid, NoLock);
 
        /* Set up pstate and CreateStmtContext */
        pstate = make_parsestate(NULL);
@@ -2419,7 +2405,7 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
                IndexStmt  *idxstmt = (IndexStmt *) lfirst(l);
 
                Assert(IsA(idxstmt, IndexStmt));
-               idxstmt = transformIndexStmt(idxstmt, queryString);
+               idxstmt = transformIndexStmt(relid, idxstmt, queryString);
                newcmd = makeNode(AlterTableCmd);
                newcmd->subtype = OidIsValid(idxstmt->indexOid) ? AT_AddIndexConstraint : AT_AddIndex;
                newcmd->def = (Node *) idxstmt;
@@ -2443,7 +2429,7 @@ transformAlterTableStmt(AlterTableStmt *stmt, const char *queryString)
                newcmds = lappend(newcmds, newcmd);
        }
 
-       /* Close rel but keep lock */
+       /* Close rel */
        relation_close(rel, NoLock);
 
        /*
index f45d92df73dc04add770c6d62482635f8fdc35d7..beaa08358a2363f664c5f3304fb47faceb90829c 100644 (file)
 #include "rewrite/rewriteDefine.h"
 #include "rewrite/rewriteRemove.h"
 #include "storage/fd.h"
+#include "storage/lmgr.h"
 #include "tcop/pquery.h"
 #include "tcop/utility.h"
 #include "utils/acl.h"
 #include "utils/guc.h"
 #include "utils/syscache.h"
+#include "utils/lsyscache.h"
 
 
 /* Hook for plugins to get control in ProcessUtility() */
@@ -72,19 +74,17 @@ ProcessUtility_hook_type ProcessUtility_hook = NULL;
  * except when allowSystemTableMods is true.
  */
 void
-CheckRelationOwnership(RangeVar *rel, bool noCatalogs)
+CheckRelationOwnership(Oid relOid, bool noCatalogs)
 {
-       Oid                     relOid;
        HeapTuple       tuple;
 
-       relOid = RangeVarGetRelid(rel, false);
        tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relOid));
        if (!HeapTupleIsValid(tuple))           /* should not happen */
                elog(ERROR, "cache lookup failed for relation %u", relOid);
 
        if (!pg_class_ownercheck(relOid, GetUserId()))
                aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
-                                          rel->relname);
+                                          get_rel_name(relOid));
 
        if (noCatalogs)
        {
@@ -93,7 +93,7 @@ CheckRelationOwnership(RangeVar *rel, bool noCatalogs)
                        ereport(ERROR,
                                        (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                                         errmsg("permission denied: \"%s\" is a system catalog",
-                                                       rel->relname)));
+                                                       get_rel_name(relOid))));
        }
 
        ReleaseSysCache(tuple);
@@ -751,9 +751,23 @@ standard_ProcessUtility(Node *parsetree,
                        {
                                List       *stmts;
                                ListCell   *l;
+                               AlterTableStmt *atstmt = (AlterTableStmt *) parsetree;
+                               Oid                     relid;
+                               LOCKMODE        lockmode;
+
+                               /*
+                                * Look up the relation OID just once, right here at the
+                                * beginning, so that we don't end up repeating the name
+                                * lookup later and latching onto a different relation
+                                * partway through.
+                                */
+                               lockmode = AlterTableGetLockLevel(atstmt->cmds);
+                               relid = RangeVarGetRelid(atstmt->relation, false);
+                               LockRelationOid(relid, lockmode);
 
                                /* Run parse analysis ... */
-                               stmts = transformAlterTableStmt((AlterTableStmt *) parsetree,
+                               stmts = transformAlterTableStmt(relid,
+                                                                                               atstmt,
                                                                                                queryString);
 
                                /* ... and do it */
@@ -764,7 +778,7 @@ standard_ProcessUtility(Node *parsetree,
                                        if (IsA(stmt, AlterTableStmt))
                                        {
                                                /* Do the table alteration proper */
-                                               AlterTable((AlterTableStmt *) stmt);
+                                               AlterTable(relid, (AlterTableStmt *) stmt);
                                        }
                                        else
                                        {
@@ -927,18 +941,33 @@ standard_ProcessUtility(Node *parsetree,
                case T_IndexStmt:               /* CREATE INDEX */
                        {
                                IndexStmt  *stmt = (IndexStmt *) parsetree;
+                               Oid                     relid;
+                               LOCKMODE        lockmode;
 
                                if (stmt->concurrent)
                                        PreventTransactionChain(isTopLevel,
                                                                                        "CREATE INDEX CONCURRENTLY");
 
-                               CheckRelationOwnership(stmt->relation, true);
+                               /*
+                                * Look up the relation OID just once, right here at the
+                                * beginning, so that we don't end up repeating the name
+                                * lookup later and latching onto a different relation
+                                * partway through.  To avoid lock upgrade hazards, it's
+                                * important that we take the strongest lock that will
+                                * eventually be needed here, so the lockmode calculation
+                                * needs to match what DefineIndex() does.
+                                */
+                               lockmode = stmt->concurrent ? ShareUpdateExclusiveLock
+                                       : ShareLock;
+                               relid = RangeVarGetRelid(stmt->relation, false);
+                               LockRelationOid(relid, lockmode);
+                               CheckRelationOwnership(relid, true);
 
                                /* Run parse analysis ... */
-                               stmt = transformIndexStmt(stmt, queryString);
+                               stmt = transformIndexStmt(relid, stmt, queryString);
 
                                /* ... and do it */
-                               DefineIndex(stmt->relation,             /* relation */
+                               DefineIndex(relid,              /* relation */
                                                        stmt->idxname,          /* index name */
                                                        InvalidOid, /* no predefined OID */
                                                        stmt->accessMethod, /* am name */
@@ -1105,7 +1134,8 @@ standard_ProcessUtility(Node *parsetree,
 
                case T_CreateTrigStmt:
                        (void) CreateTrigger((CreateTrigStmt *) parsetree, queryString,
-                                                                InvalidOid, InvalidOid, false);
+                                                                InvalidOid, InvalidOid, InvalidOid,
+                                                                InvalidOid, false);
                        break;
 
                case T_DropPropertyStmt:
index 9c9987e8765408d3365642686ed4291ed61913be..4c27021b8b42f5762ef716349b1ebe3d7183c2e3 100644 (file)
@@ -108,5 +108,6 @@ extern bool reindex_relation(Oid relid, int flags);
 
 extern bool ReindexIsProcessingHeap(Oid heapOid);
 extern bool ReindexIsProcessingIndex(Oid indexOid);
+extern Oid     IndexGetRelation(Oid indexId);
 
 #endif   /* INDEX_H */
index 56b6ca2f50f08b89b51843ec2a57f869a55f7404..a12e901fc64691fc8a9a7a9ae653862e33c7922b 100644 (file)
@@ -242,6 +242,7 @@ extern char *ChooseConstraintName(const char *name1, const char *name2,
 
 extern void AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
                                                  Oid newNspId, bool isType, ObjectAddresses *objsMoved);
+extern void get_constraint_relation_oids(Oid constraint_oid, Oid *conrelid, Oid *confrelid);
 extern Oid     get_constraint_oid(Oid relid, const char *conname, bool missing_ok);
 
 extern bool check_functional_grouping(Oid relid,
index d2a635282268b94312a9ecf0e46a42b211514e9a..65121b944cf77311f800534e01ae243f21a29dc4 100644 (file)
@@ -18,7 +18,7 @@
 
 
 /* commands/indexcmds.c */
-extern void DefineIndex(RangeVar *heapRelation,
+extern void DefineIndex(Oid relationId,
                        char *indexRelationName,
                        Oid indexRelationId,
                        char *accessMethodName,
index 293e4e5bc9e0d3fc91c5bd61c7ea5fb4abeb2445..cafcf20fbd6a21f49fc2908892f4acb88ca57a38 100644 (file)
@@ -25,7 +25,7 @@ extern Oid    DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId);
 
 extern void RemoveRelations(DropStmt *drop);
 
-extern void AlterTable(AlterTableStmt *stmt);
+extern void AlterTable(Oid relid, AlterTableStmt *stmt);
 
 extern LOCKMODE AlterTableGetLockLevel(List *cmds);
 
index ad97871d98afdab71ae62d270fed1860cab7cee0..9ec3f875134f8a359f021a2b5b2c3b573e3d61cc 100644 (file)
@@ -109,7 +109,7 @@ extern PGDLLIMPORT int SessionReplicationRole;
 #define TRIGGER_DISABLED                                       'D'
 
 extern Oid CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
-                         Oid constraintOid, Oid indexOid,
+                         Oid relOid, Oid refRelOid, Oid constraintOid, Oid indexOid,
                          bool isInternal);
 
 extern void DropTrigger(Oid relid, const char *trigname,
index 8358723cff29b02ae07b77abae0f7485d5d8514c..1301463c6261aa28c19d03da66845cd0d0cc2bc0 100644 (file)
@@ -1553,6 +1553,8 @@ typedef struct Constraint
        char            fk_del_action;  /* ON DELETE action */
        bool            skip_validation;        /* skip validation of existing rows? */
        bool            initially_valid;        /* mark the new constraint as valid? */
+
+       Oid                     old_pktable_oid; /* pg_constraint.confrelid of my former self */
 } Constraint;
 
 /* ----------------------
index 3c8b9ab8819b7f71d0804fd2513ec425ba21fe16..6c73a28a4cd69d6d6a23f4307dfe7cd9433062a3 100644 (file)
 
 
 extern List *transformCreateStmt(CreateStmt *stmt, const char *queryString);
-extern List *transformAlterTableStmt(AlterTableStmt *stmt,
+extern List *transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
                                                const char *queryString);
-extern IndexStmt *transformIndexStmt(IndexStmt *stmt, const char *queryString);
+extern IndexStmt *transformIndexStmt(Oid relid, IndexStmt *stmt,
+                                  const char *queryString);
 extern void transformRuleStmt(RuleStmt *stmt, const char *queryString,
                                  List **actions, Node **whereClause);
 extern List *transformCreateSchemaStmt(CreateSchemaStmt *stmt);
index c21857af4d2294f902db098ebe89cd2343c64c03..ec585bc80326ea43d8842342323ead1172eedc17 100644 (file)
@@ -40,6 +40,6 @@ extern LogStmtLevel GetCommandLogLevel(Node *parsetree);
 
 extern bool CommandIsReadOnly(Node *parsetree);
 
-extern void CheckRelationOwnership(RangeVar *rel, bool noCatalogs);
+extern void CheckRelationOwnership(Oid relOid, bool noCatalogs);
 
 #endif   /* UTILITY_H */