mirrors/postgres
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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
This commit is contained in:
Tom Lane 2018-10-01 11:39:13 -04:00
parent a6949ca34d
commit e27453bd83
1 changed files with 14 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
src/backend/commands/tablecmds.c
View File

@ -9888,8 +9888,7 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
* appropriate work queue entries. We do this before dropping because in * appropriate work queue entries. We do this before dropping because in
* the case of a FOREIGN KEY constraint, we might not yet have exclusive * 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 * 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 * that before reparsing/dropping.
* at the catalogs to detect the existing entry.
* *
* We can't rely on the output of deparsing to tell us which relation to * 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 * 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; Form_pg_constraint con;
Oid relid; Oid relid;
Oid confrelid; Oid confrelid;
char contype;
bool conislocal; bool conislocal;
tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(oldId)); 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); elog(ERROR, "could not identify relation associated with constraint %u", oldId);
} }
confrelid = con->confrelid; confrelid = con->confrelid;
contype = con->contype;
conislocal = con->conislocal; conislocal = con->conislocal;
ReleaseSysCache(tup); ReleaseSysCache(tup);
ObjectAddressSet(obj, ConstraintRelationId, lfirst_oid(oid_item)); ObjectAddressSet(obj, ConstraintRelationId, oldId);
add_exact_object_address(&obj, objects); add_exact_object_address(&obj, objects);
/* /*
@ -9936,6 +9937,15 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
if (!conislocal) if (!conislocal)
continue; 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, ATPostAlterTypeParse(oldId, relid, confrelid,
(char *) lfirst(def_item), (char *) lfirst(def_item),
wqueue, lockmode, tab->rewrite); wqueue, lockmode, tab->rewrite);
@ -9951,7 +9961,7 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
(char *) lfirst(def_item), (char *) lfirst(def_item),
wqueue, lockmode, tab->rewrite); wqueue, lockmode, tab->rewrite);
ObjectAddressSet(obj, RelationRelationId, lfirst_oid(oid_item)); ObjectAddressSet(obj, RelationRelationId, oldId);
add_exact_object_address(&obj, objects); add_exact_object_address(&obj, objects);
} }