From e27453bd839f3d0f55f94afa554be7066a841ab3 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 1 Oct 2018 11:39:13 -0400 Subject: [PATCH] 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 --- src/backend/commands/tablecmds.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 9e60bb37a7..c145385f84 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -9888,8 +9888,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 @@ -9905,6 +9904,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)); @@ -9921,10 +9921,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); /* @@ -9936,6 +9937,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); @@ -9951,7 +9961,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); } -- 2.40.0