Date: Thu, 29 Apr 2021 19:02:28 -0400
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: ALTER CONSTRAINT on a partitioned FK isn't working
Message-ID: <20210429230228.GA25949@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="yrj/dFKFPuw6o+aM"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3144850.1607369633@sss.pgh.pa.us>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: 
 <https://www.postgresql.org/message-id/20210429230228.GA25949%40alvherre.pgsql>
Precedence: bulk


--yrj/dFKFPuw6o+aM
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2020-Dec-07, Tom Lane wrote:

> then everything behaves as expected.  So something is broken
> about propagating deferred-ness to partition triggers in an
> ALTER CONSTRAINT.  Oddly, it *looks* like it worked if you
> examine the child tables with "\d+".  I surmise that ALTER CONSTRAINT
> fixes whatever catalog fields psql looks at, but there's some other
> fields that also need to be updated and aren't being.

I came up with this.  As I mentioned in my earlier reply, handling
recursion in the usual way doesn't fix the whole problem, because we
need to recurse on possibly both sides of the constraint.  So I made it
recurse using pg_constraint.conparentid, which seems easier and cover
both ends.  This is pretty crude so far but it handles the trivial
cases.  What I did is split the existing routine in two, and the "inner"
part now recurses on itself if it sees that either table is partitioned.

I'll polish it tomorrow -- intend to get this pushed to branches back to
11, depending on what's needed.

-- 
Álvaro Herrera       Valdivia, Chile

--yrj/dFKFPuw6o+aM
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="constraints.patch"

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d9ba87a2a3..deeafb44e4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -355,8 +355,10 @@ static void AlterIndexNamespaces(Relation classRel, Relation rel,
 static void AlterSeqNamespaces(Relation classRel, Relation rel,
 							   Oid oldNspOid, Oid newNspOid, ObjectAddresses *objsMoved,
 							   LOCKMODE lockmode);
-static ObjectAddress ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
-										   bool recurse, bool recursing, LOCKMODE lockmode);
+static ObjectAddress ATExecAlterConstraint(List **wqueue, AlteredTableInfo *tab,
+										   Relation rel, AlterTableCmd *cmd,
+										   bool recurse, bool recursing,
+										   LOCKMODE lockmode);
 static ObjectAddress ATExecValidateConstraint(List **wqueue,
 											  Relation rel, char *constrName,
 											  bool recurse, bool recursing, LOCKMODE lockmode);
@@ -4967,7 +4969,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
 											   lockmode);
 			break;
 		case AT_AlterConstraint:	/* ALTER CONSTRAINT */
-			address = ATExecAlterConstraint(rel, cmd, false, false, lockmode);
+			address = ATExecAlterConstraint(wqueue, tab, rel, cmd, false, false, lockmode);
 			break;
 		case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */
 			address = ATExecValidateConstraint(wqueue, rel, cmd->name, false,
@@ -10190,16 +10192,18 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk,
  * Update the attributes of a constraint.
  *
  * Currently only works for Foreign Key constraints.
- * Foreign keys do not inherit, so we purposely ignore the
- * recursion bit here, but we keep the API the same for when
- * other constraint types are supported.
  *
  * If the constraint is modified, returns its address; otherwise, return
  * InvalidObjectAddress.
  */
+static void
+ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel, Relation rel,
+						 HeapTuple contuple, List **otherrelids,
+						 LOCKMODE lockmode);
 static ObjectAddress
-ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
-					  bool recurse, bool recursing, LOCKMODE lockmode)
+ATExecAlterConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
+					  AlterTableCmd *cmd, bool recurse, bool recursing,
+					  LOCKMODE lockmode)
 {
 	Constraint *cmdcon;
 	Relation	conrel;
@@ -10208,6 +10212,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 	HeapTuple	contuple;
 	Form_pg_constraint currcon;
 	ObjectAddress address;
+	List	   *otherrelids = NIL;
+	ListCell   *lc;
 
 	cmdcon = castNode(Constraint, cmd->def);
 
@@ -10248,103 +10254,30 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 	if (currcon->condeferrable != cmdcon->deferrable ||
 		currcon->condeferred != cmdcon->initdeferred)
 	{
-		HeapTuple	copyTuple;
-		HeapTuple	tgtuple;
-		Form_pg_constraint copy_con;
-		List	   *otherrelids = NIL;
-		ScanKeyData tgkey;
-		SysScanDesc tgscan;
 		Relation	tgrel;
-		ListCell   *lc;
 
-		/*
-		 * Now update the catalog, while we have the door open.
-		 */
-		copyTuple = heap_copytuple(contuple);
-		copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple);
-		copy_con->condeferrable = cmdcon->deferrable;
-		copy_con->condeferred = cmdcon->initdeferred;
-		CatalogTupleUpdate(conrel, &copyTuple->t_self, copyTuple);
-
-		InvokeObjectPostAlterHook(ConstraintRelationId,
-								  currcon->oid, 0);
-
-		heap_freetuple(copyTuple);
-
-		/*
-		 * Now we need to update the multiple entries in pg_trigger that
-		 * implement the constraint.
-		 */
 		tgrel = table_open(TriggerRelationId, RowExclusiveLock);
-
-		ScanKeyInit(&tgkey,
-					Anum_pg_trigger_tgconstraint,
-					BTEqualStrategyNumber, F_OIDEQ,
-					ObjectIdGetDatum(currcon->oid));
-
-		tgscan = systable_beginscan(tgrel, TriggerConstraintIndexId, true,
-									NULL, 1, &tgkey);
-
-		while (HeapTupleIsValid(tgtuple = systable_getnext(tgscan)))
-		{
-			Form_pg_trigger tgform = (Form_pg_trigger) GETSTRUCT(tgtuple);
-			Form_pg_trigger copy_tg;
-
-			/*
-			 * Remember OIDs of other relation(s) involved in FK constraint.
-			 * (Note: it's likely that we could skip forcing a relcache inval
-			 * for other rels that don't have a trigger whose properties
-			 * change, but let's be conservative.)
-			 */
-			if (tgform->tgrelid != RelationGetRelid(rel))
-				otherrelids = list_append_unique_oid(otherrelids,
-													 tgform->tgrelid);
-
-			/*
-			 * Update deferrability of RI_FKey_noaction_del,
-			 * RI_FKey_noaction_upd, RI_FKey_check_ins and RI_FKey_check_upd
-			 * triggers, but not others; see createForeignKeyActionTriggers
-			 * and CreateFKCheckTrigger.
-			 */
-			if (tgform->tgfoid != F_RI_FKEY_NOACTION_DEL &&
-				tgform->tgfoid != F_RI_FKEY_NOACTION_UPD &&
-				tgform->tgfoid != F_RI_FKEY_CHECK_INS &&
-				tgform->tgfoid != F_RI_FKEY_CHECK_UPD)
-				continue;
-
-			copyTuple = heap_copytuple(tgtuple);
-			copy_tg = (Form_pg_trigger) GETSTRUCT(copyTuple);
-
-			copy_tg->tgdeferrable = cmdcon->deferrable;
-			copy_tg->tginitdeferred = cmdcon->initdeferred;
-			CatalogTupleUpdate(tgrel, &copyTuple->t_self, copyTuple);
-
-			InvokeObjectPostAlterHook(TriggerRelationId, currcon->oid, 0);
-
-			heap_freetuple(copyTuple);
-		}
-
-		systable_endscan(tgscan);
+		ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, rel, contuple,
+								 &otherrelids, lockmode);
+		ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
 
 		table_close(tgrel, RowExclusiveLock);
-
-		/*
-		 * Invalidate relcache so that others see the new attributes.  We must
-		 * inval both the named rel and any others having relevant triggers.
-		 * (At present there should always be exactly one other rel, but
-		 * there's no need to hard-wire such an assumption here.)
-		 */
-		CacheInvalidateRelcache(rel);
-		foreach(lc, otherrelids)
-		{
-			CacheInvalidateRelcacheByRelid(lfirst_oid(lc));
-		}
-
-		ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
 	}
 	else
 		address = InvalidObjectAddress;
 
+	/*
+	 * Invalidate relcache so that others see the new attributes.  We must
+	 * inval both the named rel and any others having relevant triggers.
+	 * (At present there should always be exactly one other rel, but
+	 * there's no need to hard-wire such an assumption here.)
+	 */
+	CacheInvalidateRelcache(rel);
+	foreach(lc, otherrelids)
+	{
+		CacheInvalidateRelcacheByRelid(lfirst_oid(lc));
+	}
+
 	systable_endscan(scan);
 
 	table_close(conrel, RowExclusiveLock);
@@ -10352,6 +10285,131 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 	return address;
 }
 
+/*
+ * Do the actual job of updating pg_constraint and pg_trigger tuples for
+ * ALTER TABLE ALTER CONSTRAINT.  This is split out of ATExecAlterConstraint
+ * because we need to recurse to children when processing partitioned tables.
+ */
+static void
+ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel, Relation rel,
+						 HeapTuple contuple, List **otherrelids,
+						 LOCKMODE lockmode)
+{
+	Form_pg_constraint currcon;
+	Form_pg_constraint copy_con;
+	HeapTuple	copyTuple;
+	HeapTuple	tgtuple;
+	ScanKeyData tgkey;
+	SysScanDesc tgscan;
+	Oid			conoid;
+	Oid			refrelid;
+
+	/*
+	 * Now update the catalog, while we have the door open.
+	 */
+	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
+	refrelid = currcon->confrelid;
+	conoid = currcon->oid;
+	copyTuple = heap_copytuple(contuple);
+	copy_con = (Form_pg_constraint) GETSTRUCT(copyTuple);
+	copy_con->condeferrable = cmdcon->deferrable;
+	copy_con->condeferred = cmdcon->initdeferred;
+	CatalogTupleUpdate(conrel, &copyTuple->t_self, copyTuple);
+
+	InvokeObjectPostAlterHook(ConstraintRelationId,
+							  conoid, 0);
+
+	heap_freetuple(copyTuple);
+
+	/*
+	 * Now we need to update the multiple entries in pg_trigger that
+	 * implement the constraint.
+	 */
+	ScanKeyInit(&tgkey,
+				Anum_pg_trigger_tgconstraint,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(conoid));
+
+	tgscan = systable_beginscan(tgrel, TriggerConstraintIndexId, true,
+								NULL, 1, &tgkey);
+
+	while (HeapTupleIsValid(tgtuple = systable_getnext(tgscan)))
+	{
+		Form_pg_trigger tgform = (Form_pg_trigger) GETSTRUCT(tgtuple);
+		Form_pg_trigger copy_tg;
+
+		/*
+		 * Remember OIDs of other relation(s) involved in FK constraint.
+		 * (Note: it's likely that we could skip forcing a relcache inval
+		 * for other rels that don't have a trigger whose properties
+		 * change, but let's be conservative.)
+		 */
+		if (tgform->tgrelid != RelationGetRelid(rel))
+			*otherrelids = list_append_unique_oid(*otherrelids,
+												  tgform->tgrelid);
+
+		/*
+		 * Update deferrability of RI_FKey_noaction_del,
+		 * RI_FKey_noaction_upd, RI_FKey_check_ins and RI_FKey_check_upd
+		 * triggers, but not others; see createForeignKeyActionTriggers
+		 * and CreateFKCheckTrigger.
+		 */
+		if (tgform->tgfoid != F_RI_FKEY_NOACTION_DEL &&
+			tgform->tgfoid != F_RI_FKEY_NOACTION_UPD &&
+			tgform->tgfoid != F_RI_FKEY_CHECK_INS &&
+			tgform->tgfoid != F_RI_FKEY_CHECK_UPD)
+			continue;
+
+		copyTuple = heap_copytuple(tgtuple);
+		copy_tg = (Form_pg_trigger) GETSTRUCT(copyTuple);
+
+		copy_tg->tgdeferrable = cmdcon->deferrable;
+		copy_tg->tginitdeferred = cmdcon->initdeferred;
+		CatalogTupleUpdate(tgrel, &copyTuple->t_self, copyTuple);
+
+		InvokeObjectPostAlterHook(TriggerRelationId, conoid, 0);
+
+		heap_freetuple(copyTuple);
+	}
+
+	systable_endscan(tgscan);
+
+	/*
+	 * If the table that the partition is on is a partitioned table, we need
+	 * to recurse and handle every constraint that is a child of this
+	 * constraint.
+	 */
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
+		get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE)
+	{
+		ScanKeyData		pkey;
+		SysScanDesc		pscan;
+		HeapTuple		childtup;
+
+		ScanKeyInit(&pkey,
+					Anum_pg_constraint_conparentid,
+					BTEqualStrategyNumber, F_OIDEQ,
+					ObjectIdGetDatum(conoid));
+
+		pscan = systable_beginscan(conrel, ConstraintParentIndexId,
+								  true, NULL, 1, &pkey);
+
+		while (HeapTupleIsValid(childtup = systable_getnext(pscan)))
+		{
+			Oid			childrelid;
+			Relation	childrel;
+
+			childrelid = ((Form_pg_constraint) GETSTRUCT(childtup))->conrelid;
+			childrel = table_open(childrelid, lockmode);
+			ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, childrel, childtup,
+									 otherrelids, lockmode);
+			table_close(childrel, NoLock);
+		}
+
+		systable_endscan(pscan);
+	}
+}
+
 /*
  * ALTER TABLE VALIDATE CONSTRAINT
  *
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 7386f4d635..f87cd876d5 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -2313,6 +2313,36 @@ SET CONSTRAINTS fk_a_fkey DEFERRED;
 DELETE FROM pk WHERE a = 1;
 DELETE FROM fk WHERE a = 1;
 COMMIT;							-- OK
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2));
+CREATE TABLE ref(f1 int, f2 int, f3 int)
+  PARTITION BY list(f1);
+CREATE TABLE ref1 PARTITION OF REF FOR VALUES IN (1);
+CREATE TABLE ref2 PARTITION OF REF FOR VALUES in (2);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+INSERT INTO pt VALUES(1,2,3);
+INSERT INTO ref VALUES(1,2,3);
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2))
+  PARTITION BY LIST(f1);
+CREATE TABLE pt1 PARTITION OF pt FOR VALUES IN (1);
+CREATE TABLE pt2 PARTITION OF pt FOR VALUES IN (2);
+CREATE TABLE ref(f1 int, f2 int, f3 int);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+INSERT INTO pt VALUES(1,2,3);
+INSERT INTO ref VALUES(1,2,3);
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
 DROP SCHEMA fkpart9 CASCADE;
 NOTICE:  drop cascades to 2 other objects
 DETAIL:  drop cascades to table pk
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 67aa20435d..da5f9fa951 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1636,6 +1636,39 @@ SET CONSTRAINTS fk_a_fkey DEFERRED;
 DELETE FROM pk WHERE a = 1;
 DELETE FROM fk WHERE a = 1;
 COMMIT;							-- OK
+
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2));
+CREATE TABLE ref(f1 int, f2 int, f3 int)
+  PARTITION BY list(f1);
+CREATE TABLE ref1 PARTITION OF REF FOR VALUES IN (1);
+CREATE TABLE ref2 PARTITION OF REF FOR VALUES in (2);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+INSERT INTO pt VALUES(1,2,3);
+INSERT INTO ref VALUES(1,2,3);
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
+
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2))
+  PARTITION BY LIST(f1);
+CREATE TABLE pt1 PARTITION OF pt FOR VALUES IN (1);
+CREATE TABLE pt2 PARTITION OF pt FOR VALUES IN (2);
+CREATE TABLE ref(f1 int, f2 int, f3 int);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+INSERT INTO pt VALUES(1,2,3);
+INSERT INTO ref VALUES(1,2,3);
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
+
 DROP SCHEMA fkpart9 CASCADE;
 
 -- Verify ON UPDATE/DELETE behavior

--yrj/dFKFPuw6o+aM--