From 4709c083d23518a975d5aa4936aef1fe8b0210b7 Mon Sep 17 00:00:00 2001 From: Shayon Mukherjee Date: Wed, 8 Oct 2025 18:25:27 -0400 Subject: [PATCH v2] Reduce lock level for FK/trigger drops to allow concurrent reads Dropping a foreign key constraint or a table that owns an FK currently blocks reads on the referenced table due to AccessExclusiveLock taken while removing the FK's internal RI triggers and constraint metadata. In busy systems, this brief full-read outage can cause user-visible timeouts for otherwise read-only traffic. This change narrows the lock reduction to the safe scope, retaining AccessExclusiveLock on the table that owns the constraint and reducing locks only on the referenced table's RI action-trigger removal: 1. RemoveTriggerById(): use ShareRowExclusiveLock only for internal RI action triggers on the referenced table; all other triggers remain at AccessExclusiveLock. 2. dropconstraint_internal(): when dropping an FK, open the referenced table with ShareRowExclusiveLock to match trigger deletion and allow concurrent reads. The table being dropped or directly altered (the constraint-owning table) continues to acquire AccessExclusiveLock as before. Only the referenced tables of foreign keys see the reduced lock level. Examples of affected operations: - ALTER TABLE fktable DROP CONSTRAINT: AccessExclusive on fktable (unchanged), ShareRowExclusive on the referenced pktable, allowing SELECTs on pktable. - DROP TABLE fktable: AccessExclusive on fktable (unchanged), and ShareRowExclusive on the referenced pktable while removing RI action triggers. - Self-referential FKs: the altered table still takes AccessExclusive, so SELECTs on that table block (unchanged). Correctness is preserved because: - Writers are serialized: ShareRowExclusive conflicts with RowExclusiveLock and stronger, so no DML can race RI trigger removal. - Relcache invalidation at commit ensures metadata changes become visible to subsequent queries. - Prepared plans continue to work; plans depending on the owning table's constraints remain protected by AccessExclusive on that table. Updated isolation tests: - fk-drop-constraint-concurrency: new permutation asserting a SELECT cursor on the referencing table blocks the FK drop (owning table remains at AccessExclusive); also covers regular FKs, DROP TABLE of the FK table, self-referential FKs, and relcache invalidation. --- doc/src/sgml/ref/alter_table.sgml | 9 ++ src/backend/commands/tablecmds.c | 26 +++-- src/backend/commands/trigger.c | 58 ++++++++++- .../detach-partition-concurrently-4.out | 24 +++++ .../fk-drop-constraint-concurrency.out | 98 +++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + .../detach-partition-concurrently-4.spec | 5 + .../specs/fk-drop-constraint-concurrency.spec | 74 ++++++++++++++ 8 files changed, 285 insertions(+), 10 deletions(-) create mode 100644 src/test/isolation/expected/fk-drop-constraint-concurrency.out create mode 100644 src/test/isolation/specs/fk-drop-constraint-concurrency.spec diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 8867da6c69..646578a460 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -623,6 +623,15 @@ WITH ( MODULUS numeric_literal, REM If IF EXISTS is specified and the constraint does not exist, no error is thrown. In this case a notice is issued instead. + + For foreign key constraints, this acquires ACCESS + EXCLUSIVE lock on the table owning the constraint (as with + other ALTER TABLE forms), but only SHARE + ROW EXCLUSIVE lock on the referenced table. This allows + concurrent reads on the referenced table while the constraint is being + dropped. Other constraint types acquire ACCESS + EXCLUSIVE lock on the table being altered. + diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index fc89352b66..4bef2cfce8 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -14165,7 +14165,8 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha * and check that that's not in use, just as we've already done for the * constrained table (else we might, eg, be dropping a trigger that has * unfired events). But we can/must skip that in the self-referential - * case. + * case. Use ShareRowExclusive to allow concurrent reads on the referenced + * table. */ if (con->contype == CONSTRAINT_FOREIGN && con->confrelid != RelationGetRelid(rel)) @@ -14173,7 +14174,7 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha Relation frel; /* Must match lock taken by RemoveTriggerById: */ - frel = table_open(con->confrelid, AccessExclusiveLock); + frel = table_open(con->confrelid, ShareRowExclusiveLock); CheckAlterTableIsSafe(frel); table_close(frel, NoLock); } @@ -15474,15 +15475,26 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) /* * When rebuilding another table's constraint that references the * table we're modifying, we might not yet have any lock on the other - * table, so get one now. We'll need AccessExclusiveLock for the DROP - * CONSTRAINT step, so there's no value in asking for anything weaker. + * table, so get one now. For foreign key constraints, use + * ShareRowExclusiveLock to allow readers on the referenced table + * during FK rebuild. For other constraints, use AccessExclusiveLock + * to avoid invalidating query plans that may rely on them. */ if (relid != tab->relid) - LockRelationOid(relid, AccessExclusiveLock); + { + LOCKMODE otherRelLockmode; + + if (con->contype == CONSTRAINT_FOREIGN && OidIsValid(confrelid)) + otherRelLockmode = ShareRowExclusiveLock; + else + otherRelLockmode = AccessExclusiveLock; + + LockRelationOid(relid, otherRelLockmode); + } ATPostAlterTypeParse(oldId, relid, confrelid, - (char *) lfirst(def_item), - wqueue, lockmode, tab->rewrite); + (char *) lfirst(def_item), + wqueue, lockmode, tab->rewrite); } forboth(oid_item, tab->changedIndexOids, def_item, tab->changedIndexDefs) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 579ac8d76a..5fe318c695 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -1315,11 +1315,63 @@ RemoveTriggerById(Oid trigOid) elog(ERROR, "could not find tuple for trigger %u", trigOid); /* - * Open and exclusive-lock the relation the trigger belongs to. + * Open and lock the relation the trigger belongs to. + * + * For internal RI triggers on the referenced table (action triggers), we + * can use ShareRowExclusiveLock to allow concurrent reads while blocking + * writers. This is safe because these triggers don't affect query plans + * on the referenced table. + * + * For all other triggers (user triggers, RI check triggers on the FK + * table), we must use AccessExclusiveLock because dropping them could + * invalidate query plans that were optimized assuming the constraint or + * trigger behavior. */ - relid = ((Form_pg_trigger) GETSTRUCT(tup))->tgrelid; + { + Form_pg_trigger tgform = (Form_pg_trigger) GETSTRUCT(tup); + LOCKMODE lockmode; + + relid = tgform->tgrelid; + + /* + * Use weaker lock only for internal constraint triggers where the + * trigger's table is the referenced (not constrained) table. We + * identify these as internal triggers with a valid tgconstraint where + * tgrelid != the constraint's conrelid. + */ + if (tgform->tgisinternal && OidIsValid(tgform->tgconstraint)) + { + HeapTuple contup; + Form_pg_constraint conform; + + contup = SearchSysCache1(CONSTROID, + ObjectIdGetDatum(tgform->tgconstraint)); + if (HeapTupleIsValid(contup)) + { + conform = (Form_pg_constraint) GETSTRUCT(contup); - rel = table_open(relid, AccessExclusiveLock); + /* + * RI action triggers are on the referenced table (confrelid), + * not on the table owning the constraint (conrelid). Use + * weaker lock only for those; check triggers on the FK table + * need AccessExclusive to avoid breaking query plans. + */ + if (conform->contype == CONSTRAINT_FOREIGN && + tgform->tgrelid == conform->confrelid && + conform->confrelid != conform->conrelid) + lockmode = ShareRowExclusiveLock; + else + lockmode = AccessExclusiveLock; + ReleaseSysCache(contup); + } + else + lockmode = AccessExclusiveLock; + } + else + lockmode = AccessExclusiveLock; + + rel = table_open(relid, lockmode); + } if (rel->rd_rel->relkind != RELKIND_RELATION && rel->rd_rel->relkind != RELKIND_VIEW && diff --git a/src/test/isolation/expected/detach-partition-concurrently-4.out b/src/test/isolation/expected/detach-partition-concurrently-4.out index 79b29ae4c1..09c34512f0 100644 --- a/src/test/isolation/expected/detach-partition-concurrently-4.out +++ b/src/test/isolation/expected/detach-partition-concurrently-4.out @@ -360,6 +360,30 @@ step s3commit: commit; step s1c: commit; step s2detach: <... completed> +starting permutation: s2snitch s1b s1write s2detach s1cancel s1c +step s2snitch: insert into d4_pid select pg_backend_pid(); +step s1b: begin; +step s1write: update d4_fk set a = a where a = 2; +step s2detach: alter table d4_primary detach partition d4_primary1 concurrently; +step s1cancel: select pg_cancel_backend(pid) from d4_pid; +step s2detach: <... completed> +ERROR: canceling statement due to user request +step s1cancel: <... completed> +pg_cancel_backend +----------------- +t +(1 row) + +step s1c: commit; + +starting permutation: s2snitch s1b s1write s2detach s1c +step s2snitch: insert into d4_pid select pg_backend_pid(); +step s1b: begin; +step s1write: update d4_fk set a = a where a = 2; +step s2detach: alter table d4_primary detach partition d4_primary1 concurrently; +step s1c: commit; +step s2detach: <... completed> + starting permutation: s2snitch s1brr s1s s2detach s1cancel s1noop s3vacfreeze s1s s1insert s1c step s2snitch: insert into d4_pid select pg_backend_pid(); step s1brr: begin isolation level repeatable read; diff --git a/src/test/isolation/expected/fk-drop-constraint-concurrency.out b/src/test/isolation/expected/fk-drop-constraint-concurrency.out new file mode 100644 index 0000000000..385280861a --- /dev/null +++ b/src/test/isolation/expected/fk-drop-constraint-concurrency.out @@ -0,0 +1,98 @@ +Parsed test spec with 2 sessions + +starting permutation: s1b s1s s2drop s1c +step s1b: begin; +step s1s: select * from pk; +id +-- + 1 +(1 row) + +step s2drop: alter table fk drop constraint fk_ref_fkey; +step s1c: commit; + +starting permutation: s1b s1declare_fk s1fetch_fk s2drop s1c +step s1b: begin; +step s1declare_fk: declare cfk cursor for select * from fk; +step s1fetch_fk: fetch 1 from cfk; +id|ref +--+--- + 1| 1 +(1 row) + +step s2drop: alter table fk drop constraint fk_ref_fkey; +step s1c: commit; +step s2drop: <... completed> + +starting permutation: s1b s1wfk s2drop s1c +step s1b: begin; +step s1wfk: update fk set ref = ref where id = 1; +step s2drop: alter table fk drop constraint fk_ref_fkey; +step s1c: commit; +step s2drop: <... completed> + +starting permutation: s1b s1wpk s2drop s1c +step s1b: begin; +step s1wpk: update pk set id = id where id = 1; +step s2drop: alter table fk drop constraint fk_ref_fkey; +step s1c: commit; +step s2drop: <... completed> + +starting permutation: s1prep s1b s1exec s2drop s1exec s1c +step s1prep: prepare q as select count(*) from pk; +step s1b: begin; +step s1exec: execute q; +count +----- + 1 +(1 row) + +step s2drop: alter table fk drop constraint fk_ref_fkey; +step s1exec: execute q; +count +----- + 1 +(1 row) + +step s1c: commit; + +starting permutation: s1b s1s s2drop_table s1c +step s1b: begin; +step s1s: select * from pk; +id +-- + 1 +(1 row) + +step s2drop_table: drop table fk; +step s1c: commit; + +starting permutation: s1b s1s_self s2drop_self s1c +step s1b: begin; +step s1s_self: select * from fkself; +id|parent +--+------ + 1| +(1 row) + +step s2drop_self: alter table fkself drop constraint fkself_parent_fkey; +step s1c: commit; +step s2drop_self: <... completed> + +starting permutation: s1b s1w_self s2drop_self s1c +step s1b: begin; +step s1w_self: update fkself set id = id where id = 1; +step s2drop_self: alter table fkself drop constraint fkself_parent_fkey; +step s1c: commit; +step s2drop_self: <... completed> + +starting permutation: s1b s1s s2alter_type s1c +step s1b: begin; +step s1s: select * from pk; +id +-- + 1 +(1 row) + +step s2alter_type: alter table fk_alttype alter column ref type bigint; +step s1c: commit; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 5afae33d37..987a3c0a1a 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -120,3 +120,4 @@ test: serializable-parallel-2 test: serializable-parallel-3 test: matview-write-skew test: lock-nowait +test: fk-drop-constraint-concurrency diff --git a/src/test/isolation/specs/detach-partition-concurrently-4.spec b/src/test/isolation/specs/detach-partition-concurrently-4.spec index 829c82248f..dcd4791a2f 100644 --- a/src/test/isolation/specs/detach-partition-concurrently-4.spec +++ b/src/test/isolation/specs/detach-partition-concurrently-4.spec @@ -27,6 +27,7 @@ session s1 step s1b { begin; } step s1brr { begin isolation level repeatable read; } step s1s { select * from d4_primary; } +step s1write { update d4_fk set a = a where a = 2; } step s1cancel { select pg_cancel_backend(pid) from d4_pid; } step s1noop { } step s1insert { insert into d4_fk values (1); } @@ -77,6 +78,10 @@ permutation s2snitch s1b s1s s2detach s3insert s1c permutation s2snitch s1b s1s s2detach s3brr s3insert s3commit s1cancel(s2detach) s1c permutation s2snitch s1b s1s s2detach s3brr s3insert s3commit s1c +# Explicit coverage that a writer on the referencing table blocks detach +permutation s2snitch s1b s1write s2detach s1cancel(s2detach) s1c +permutation s2snitch s1b s1write s2detach s1c + # Try one where we VACUUM FREEZE pg_inherits (to verify that xmin change is # handled correctly). permutation s2snitch s1brr s1s s2detach s1cancel(s2detach) s1noop s3vacfreeze s1s s1insert s1c diff --git a/src/test/isolation/specs/fk-drop-constraint-concurrency.spec b/src/test/isolation/specs/fk-drop-constraint-concurrency.spec new file mode 100644 index 0000000000..a9dfe7bd24 --- /dev/null +++ b/src/test/isolation/specs/fk-drop-constraint-concurrency.spec @@ -0,0 +1,74 @@ +# Regular-table analogue of detach-partition-concurrently behavior for FK drop. +# +# This spec exercises reduced relation-level locking (ShareRowExclusive) for +# FK/trigger removal: +# 1) SELECTs on the referenced table (pk) proceed while DROP CONSTRAINT runs. +# 2) Writers (UPDATE) on the referencing (fk) table block the FK drop. +# 3) Writers (UPDATE) on the referenced (pk) table block the FK drop. +# 4) Prepared SELECTs (plan + relcache) continue to work across FK drop. +# 5) DROP TABLE fktable doesn't block SELECTs on the referenced table. +# 6) Self-referential FK drops don't block SELECTs on the table. +# 7) ALTER COLUMN TYPE with FK rebuild doesn't block SELECTs on referenced table. + +setup { + drop table if exists pk, fk, fkself, fk_alttype; + create table pk(id int primary key); + create table fk(id int primary key, ref int references pk(id)); + create table fkself(id int primary key, parent int references fkself(id)); + create table fk_alttype(id int primary key, ref int references pk(id)); + insert into pk values (1); + insert into fk values (1, 1); + insert into fkself values (1, NULL); + insert into fk_alttype values (1, 1); +} + +teardown { + drop table if exists fk, fkself, fk_alttype, pk; +} + +session s1 +step s1b { begin; } +step s1s { select * from pk; } +step s1declare_fk { declare cfk cursor for select * from fk; } +step s1fetch_fk { fetch 1 from cfk; } +step s1s_self { select * from fkself; } +step s1wfk { update fk set ref = ref where id = 1; } +step s1wpk { update pk set id = id where id = 1; } +step s1w_self { update fkself set id = id where id = 1; } +step s1c { commit; } +step s1prep { prepare q as select count(*) from pk; } +step s1exec { execute q; } + +session s2 +step s2drop { alter table fk drop constraint fk_ref_fkey; } +step s2drop_table { drop table fk; } +step s2drop_self { alter table fkself drop constraint fkself_parent_fkey; } +step s2alter_type { alter table fk_alttype alter column ref type bigint; } + +# (1) Readers do not block FK drop +permutation s1b s1s s2drop s1c + +# (1a) SELECT on referencing table blocks FK drop (ALTER TABLE waits) +permutation s1b s1declare_fk s1fetch_fk s2drop s1c + +# (2) Writer on referencing table blocks FK drop +permutation s1b s1wfk s2drop s1c + +# (3) Writer on referenced table blocks FK drop +permutation s1b s1wpk s2drop s1c + +# (4) Prepared plan remains usable across FK drop; relcache invalidation ensures correctness +permutation s1prep s1b s1exec s2drop s1exec s1c + +# (5) DROP TABLE fktable does not block SELECTs on referenced table +permutation s1b s1s s2drop_table s1c + +# (6) Self-referential FK drop: ALTER TABLE takes AccessExclusive on the table +# being altered, so even SELECTs block (different from non-self-ref case) +permutation s1b s1s_self s2drop_self s1c + +# (7) Writer on self-referential FK table blocks drop of self-referential FK +permutation s1b s1w_self s2drop_self s1c + +# (8) ALTER COLUMN TYPE rebuilding FK: SELECTs on referenced table proceed +permutation s1b s1s s2alter_type s1c -- 2.39.5 (Apple Git-154)