Fix ALTER COLUMN TYPE to not open a relation without any lock.

If the column being modified is referenced by a foreign key constraint
of another table, ALTER TABLE would open the other table (to re-parse
the constraint's definition) without having first obtained a lock on it.
This was evidently intentional, but that doesn't mean it's really safe.
It's especially not safe in 9.3, which pre-dates use of MVCC scans for
catalog reads, but even in current releases it doesn't seem like a good
idea.

We know we'll need AccessExclusiveLock shortly to drop the obsoleted
constraint, so just get that a little sooner to close the hole.

Per testing with a patch that complains if we open a relation without
holding any lock on it.  I don't plan to back-patch that patch, but we
should close the holes it identifies in all supported branches.

Discussion: https://postgr.es/m/2038.1538335244@sss.pgh.pa.us

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 955d8a630c2c857cdd4d108ee66daf2c55818bef..b0cbf2e1378250ce7016947407940a757d37607d 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9857,8 +9857,7 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
         * appropriate work queue entries.  We do this before dropping because in
         * the case of a FOREIGN KEY constraint, we might not yet have exclusive
         * 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.
+        * that before reparsing/dropping.
         *
         * 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
@@ -9874,6 +9873,7 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
                Form_pg_constraint con;
                Oid                     relid;
                Oid                     confrelid;
+               char            contype;
                bool            conislocal;
 
                tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(oldId));
@@ -9890,10 +9890,11 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
                                elog(ERROR, "could not identify relation associated with constraint %u", oldId);
                }
                confrelid = con->confrelid;
+               contype = con->contype;
                conislocal = con->conislocal;
                ReleaseSysCache(tup);
 
-               ObjectAddressSet(obj, ConstraintRelationId, lfirst_oid(oid_item));
+               ObjectAddressSet(obj, ConstraintRelationId, oldId);
                add_exact_object_address(&obj, objects);
 
                /*
@@ -9905,6 +9906,15 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
                if (!conislocal)
                        continue;
 
+               /*
+                * When rebuilding an FK constraint that references the table we're
+                * modifying, we might not yet have any lock on the FK's table, so get
+                * one now.  We'll need AccessExclusiveLock for the DROP CONSTRAINT
+                * step, so there's no value in asking for anything weaker.
+                */
+               if (relid != tab->relid && contype == CONSTRAINT_FOREIGN)
+                       LockRelationOid(relid, AccessExclusiveLock);
+
                ATPostAlterTypeParse(oldId, relid, confrelid,
                                                         (char *) lfirst(def_item),
                                                         wqueue, lockmode, tab->rewrite);
@@ -9920,7 +9930,7 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
                                                         (char *) lfirst(def_item),
                                                         wqueue, lockmode, tab->rewrite);
 
-               ObjectAddressSet(obj, RelationRelationId, lfirst_oid(oid_item));
+               ObjectAddressSet(obj, RelationRelationId, oldId);
                add_exact_object_address(&obj, objects);
        }