Reduce lock level for FK/trigger drops to allow concurrent reads

doc/src/sgml/ref/alter_table.sgml

@@ -623,6 +623,15 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
       If <literal>IF EXISTS</literal> is specified and the constraint
       does not exist, no error is thrown. In this case a notice is issued instead.
      </para>
+     <para>
+      For foreign key constraints, this acquires <literal>ACCESS
+      EXCLUSIVE</literal> lock on the table owning the constraint (as with
+      other <command>ALTER TABLE</command> forms), but only <literal>SHARE
+      ROW EXCLUSIVE</literal> lock on the referenced table.  This allows
+      concurrent reads on the referenced table while the constraint is being
+      dropped.  Other constraint types acquire <literal>ACCESS
+      EXCLUSIVE</literal> lock on the table being altered.
+     </para>
     </listitem>
    </varlistentry>
 

src/backend/commands/tablecmds.c

@@ -14165,15 +14165,16 @@ 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))
 	{
 		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)

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 &&

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; <waiting ...>
+step s1cancel: select pg_cancel_backend(pid) from d4_pid; <waiting ...>
+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; <waiting ...>
+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;

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; <waiting ...>
+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; <waiting ...>
+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; <waiting ...>
+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; <waiting ...>
+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; <waiting ...>
+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;

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

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

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