mirror of
https://git.postgresql.org/git/postgresql.git
synced 2024-10-04 08:56:50 +02:00
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
This commit is contained in:
parent
2855421ec7
commit
0360c539f2
@ -8686,8 +8686,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
|
||||||
@ -8703,6 +8702,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));
|
||||||
@ -8711,6 +8711,7 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
|
|||||||
con = (Form_pg_constraint) GETSTRUCT(tup);
|
con = (Form_pg_constraint) GETSTRUCT(tup);
|
||||||
relid = con->conrelid;
|
relid = con->conrelid;
|
||||||
confrelid = con->confrelid;
|
confrelid = con->confrelid;
|
||||||
|
contype = con->contype;
|
||||||
conislocal = con->conislocal;
|
conislocal = con->conislocal;
|
||||||
ReleaseSysCache(tup);
|
ReleaseSysCache(tup);
|
||||||
|
|
||||||
@ -8723,6 +8724,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);
|
||||||
|
Loading…
Reference in New Issue
Block a user