pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  

help / color / mirror / Atom feed

ALTER CONSTRAINT on a partitioned FK isn't working
8+ messages / 2 participants
[nested] [flat]

Commits from this thread:
  2021-05-05 abce26c9eb a25b98d2cf 923c13520f 6f70d7ca1d  Have ALTER CONSTRAINT recurse on partitioned tables
  2021-05-04 91a6b3862f 72fabd4bfb e798d095da  Fix OID passed to object-alter hook during ALTER CONSTRAINT

* ALTER CONSTRAINT on a partitioned FK isn't working
@ 2020-12-07 19:33  Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 2 replies; 8+ messages in thread

From: Tom Lane @ 2020-12-07 19:33 UTC (permalink / raw)
  To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-bugs@lists.postgresql.org

I reproduced the problem shown in [1]:

--- snip ---
drop table if exists 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)
  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);

delete from pt;  -- expected to fail

begin;
delete from pt;  -- this should work, but does not
delete from ref;
abort;
--- snip ---

But if you create the FK in one step with

alter table ref add foreign key(f1,f2) references pt
  deferrable initially deferred;

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.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/75fe0761-a291-86a9-c8d8-4906da077469%40gmail.com





^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: ALTER CONSTRAINT on a partitioned FK isn't working
@ 2020-12-07 23:34  Alvaro Herrera <alvherre@alvh.no-ip.org>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  1 sibling, 1 reply; 8+ messages in thread

From: Alvaro Herrera @ 2020-12-07 23:34 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org

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.

Yeah, this query shows that the tgdeferrable and tginitdeferred columns
ought to be dropped:

select tgname, tgdeferrable, tginitdeferred from pg_trigger;

These are all 'true' when the constraint is created correctly, and false
when changed by ALTER CONSTRAINT.

Let me see about fixing that ...





^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: ALTER CONSTRAINT on a partitioned FK isn't working
@ 2020-12-07 23:52  Alvaro Herrera <alvherre@alvh.no-ip.org>
  parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
  0 siblings, 0 replies; 8+ messages in thread

From: Alvaro Herrera @ 2020-12-07 23:52 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org

On 2020-Dec-07, Alvaro Herrera wrote:

> Yeah, this query shows that the tgdeferrable and tginitdeferred columns
> ought to be dropped:
> 
> select tgname, tgdeferrable, tginitdeferred from pg_trigger;

Sorry, I meant "ought to be updated".  But there's more to it than that:
the pg_constraint entries themselves are not updated, and that's because
ALTER CONSTRAINT does not have recurse at all.  So the first thing to do
is add an "ATSimpleRecursion()" call for the appropriate case, but even
that is not sufficient, as we need to recurse on the referenced side
also, not just the referencing side -- and that's a tad more involved.
(ATExecAlterConstraint is explicitly not handling the case.)






^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: ALTER CONSTRAINT on a partitioned FK isn't working
@ 2021-04-29 23:02  Alvaro Herrera <alvherre@alvh.no-ip.org>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  1 sibling, 1 reply; 8+ messages in thread

From: Alvaro Herrera @ 2021-04-29 23:02 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org

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

Attachments:

constraints.patch (text/x-diff, 13.2K) download
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

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: ALTER CONSTRAINT on a partitioned FK isn't working
@ 2021-05-03 23:16  Alvaro Herrera <alvherre@alvh.no-ip.org>
  parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
  0 siblings, 2 replies; 8+ messages in thread

From: Alvaro Herrera @ 2021-05-03 23:16 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org

It turns out that more work is needed here to ensure we recurse to all
children on both sides, and so that all involved triggers are altered.

There is a case where it doesn't seem like we can easily give
non-surprising behavior: when multi-level partitioning is used, a
constraint is defined in the top level and then it is altered in the
middle level, then we don't have access to the action triggers, because
those are defined at the parent level.  I opted for adding a WARNING
message in that case.  Other options were 1) to raise an error if
altering a constraint that's not top level, but I fear that might break
restoring dumps of existing database; 2) reach up and alter the parent
constraint instead, but that seems too magical.  This doesn't seem worth
spending too much sweat on, since it's a pretty fringe case anyway.

Patch for HEAD attached.


I also noticed that we're calling InvokeObjectPostAlterHook wrong in one
place: when altering the trigger properties, we call it with the
constraint OID instead of the trigger OID.  I'll fix that in a separate
commit -- that was introduced by 578b229718e8 ("Remove WITH OIDS
support, change oid catalog column visibility.") in pg12.  It's sad that
nobody ever noticed ... looks like the sepgsql stuff isn't getting much
testing.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

Attachments:

constraints.patch (text/x-diff, 17.8K) download
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d9ba87a2a3..496d7da7f3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -355,8 +355,13 @@ 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 bool ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
+									 Relation rel, HeapTuple contuple, List **otherrelids,
+									 LOCKMODE lockmode);
 static ObjectAddress ATExecValidateConstraint(List **wqueue,
 											  Relation rel, char *constrName,
 											  bool recurse, bool recursing, LOCKMODE lockmode);
@@ -4669,6 +4674,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			break;
 		case AT_AlterConstraint:	/* ALTER CONSTRAINT */
 			ATSimplePermissions(rel, ATT_TABLE);
+			/* Recursion occurs during execution phase */
 			pass = AT_PASS_MISC;
 			break;
 		case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */
@@ -4967,7 +4973,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,28 +10196,30 @@ 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 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;
+	Relation	tgrel;
 	SysScanDesc scan;
 	ScanKeyData skey[3];
 	HeapTuple	contuple;
 	Form_pg_constraint currcon;
 	ObjectAddress address;
+	List	   *otherrelids = NIL;
+	ListCell   *lc;
 
 	cmdcon = castNode(Constraint, cmd->def);
 
 	conrel = table_open(ConstraintRelationId, RowExclusiveLock);
+	tgrel = table_open(TriggerRelationId, RowExclusiveLock);
 
 	/*
 	 * Find and check the target constraint
@@ -10245,21 +10253,88 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 				 errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint",
 						cmdcon->conname, RelationGetRelationName(rel))));
 
+	/*
+	 * If it's not the topmost constraint, raise a warning.
+	 *
+	 * The problem is that altering a non-topmost constraint leaves action
+	 * triggers untouched, since they are not directly connected to this
+	 * constraint but rather to its parent.  We would like to forbid this
+	 * action but cannot, because existing databases might be in this state
+	 * already and pg_dumps would become unrestorable.
+	 */
+	if (OidIsValid(currcon->conparentid))
+		ereport(WARNING,
+				(errmsg("constraint \"%s\" of relation \"%s\" is not top-level -- proceeding anyway",
+						cmdcon->conname, RelationGetRelationName(rel)),
+				 errhint("You may want to alter the constraint in the ancestor table instead.")));
+
+	/*
+	 * Do the actual catalog work.  We can skip changing if already in the
+	 * desired state, but not if a partitioned table: partitions need to be
+	 * processed regardless, in case they've had it locally changed.
+	 */
+	address = InvalidObjectAddress;
+	if (currcon->condeferrable != cmdcon->deferrable ||
+		currcon->condeferred != cmdcon->initdeferred ||
+		rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		if (ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, rel, contuple,
+									 &otherrelids, lockmode))
+			ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
+	}
+
+	/*
+	 * ATExecConstrRecurse already invalidated relcache for the relations
+	 * having the constraint itself; we also need to invalidate relations that
+	 * have any triggers that implement the constraint.
+	 */
+	foreach(lc, otherrelids)
+		CacheInvalidateRelcacheByRelid(lfirst_oid(lc));
+
+	systable_endscan(scan);
+
+	table_close(tgrel, RowExclusiveLock);
+	table_close(conrel, RowExclusiveLock);
+
+	return address;
+}
+
+/*
+ * Recursive subroutine of ATExecAlterConstraint
+ *
+ * *otherrelids is appended OIDs of relations containing affected triggers.
+ *
+ * Note that we must recurse even when the values are correct, in case
+ * indirect descendants have had their constraints altered locally.
+ * (This could be avoided if we forbade altering constraints in partitions
+ * but we don't do currently do that.)
+ */
+static bool
+ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
+						 Relation rel, HeapTuple contuple, List **otherrelids,
+						 LOCKMODE lockmode)
+{
+	Form_pg_constraint currcon;
+	Oid			conoid;
+	Oid			refrelid;
+	bool		changed = false;
+
+	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
+	conoid = currcon->oid;
+	refrelid = currcon->confrelid;
+
+	/*
+	 * Update pg_constraint with the flags from cmdcon.
+	 *
+	 * If called to modify a constraint that's already in the desired state,
+	 * silently do nothing.
+	 */
 	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;
@@ -10267,20 +10342,29 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 		CatalogTupleUpdate(conrel, &copyTuple->t_self, copyTuple);
 
 		InvokeObjectPostAlterHook(ConstraintRelationId,
-								  currcon->oid, 0);
+								  conoid, 0);
 
 		heap_freetuple(copyTuple);
+		changed = true;
+	}
 
-		/*
-		 * Now we need to update the multiple entries in pg_trigger that
-		 * implement the constraint.
-		 */
-		tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+	/* Make new constraint flags visible to others */
+	/* XXX only if the constraint changed? */
+	CacheInvalidateRelcache(rel);
+
+	/*
+	 * Now we need to update the multiple entries in pg_trigger that implement
+	 * the constraint.
+	 */
+	{
+		HeapTuple	tgtuple;
+		ScanKeyData tgkey;
+		SysScanDesc tgscan;
 
 		ScanKeyInit(&tgkey,
 					Anum_pg_trigger_tgconstraint,
 					BTEqualStrategyNumber, F_OIDEQ,
-					ObjectIdGetDatum(currcon->oid));
+					ObjectIdGetDatum(conoid));
 
 		tgscan = systable_beginscan(tgrel, TriggerConstraintIndexId, true,
 									NULL, 1, &tgkey);
@@ -10289,6 +10373,7 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 		{
 			Form_pg_trigger tgform = (Form_pg_trigger) GETSTRUCT(tgtuple);
 			Form_pg_trigger copy_tg;
+			HeapTuple	copyTuple;
 
 			/*
 			 * Remember OIDs of other relation(s) involved in FK constraint.
@@ -10297,8 +10382,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 			 * change, but let's be conservative.)
 			 */
 			if (tgform->tgrelid != RelationGetRelid(rel))
-				otherrelids = list_append_unique_oid(otherrelids,
-													 tgform->tgrelid);
+				*otherrelids = list_append_unique_oid(*otherrelids,
+													  tgform->tgrelid);
 
 			/*
 			 * Update deferrability of RI_FKey_noaction_del,
@@ -10319,37 +10404,52 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 			copy_tg->tginitdeferred = cmdcon->initdeferred;
 			CatalogTupleUpdate(tgrel, &copyTuple->t_self, copyTuple);
 
-			InvokeObjectPostAlterHook(TriggerRelationId, currcon->oid, 0);
+			InvokeObjectPostAlterHook(TriggerRelationId, tgform->oid, 0);
 
 			heap_freetuple(copyTuple);
 		}
 
 		systable_endscan(tgscan);
+	}
 
-		table_close(tgrel, RowExclusiveLock);
+	/*
+	 * If the table at either end of the constraint is partitioned, we need to
+	 * recurse and handle every constraint that is a child of this one.
+	 *
+	 * (This assumes that the recurse flag is forcibly set for partitioned
+	 * tables, and not set for legacy inheritance, though we don't check for
+	 * that here.)
+	 */
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
+		get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE)
+	{
+		ScanKeyData		pkey;
+		SysScanDesc		pscan;
+		HeapTuple		childtup;
 
-		/*
-		 * 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)
+		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)))
 		{
-			CacheInvalidateRelcacheByRelid(lfirst_oid(lc));
+			Form_pg_constraint childcon = (Form_pg_constraint) GETSTRUCT(childtup);
+			Relation	childrel;
+
+			childrel = table_open(childcon->conrelid, lockmode);
+			ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, childrel, childtup,
+									 otherrelids, lockmode);
+			table_close(childrel, NoLock);
 		}
 
-		ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
+		systable_endscan(pscan);
 	}
-	else
-		address = InvalidObjectAddress;
 
-	systable_endscan(scan);
-
-	table_close(conrel, RowExclusiveLock);
-
-	return address;
+	return changed;
 }
 
 /*
@@ -17764,6 +17864,8 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 		 * Make the action triggers on the referenced relation.  When this was
 		 * a partition the action triggers pointed to the parent rel (they
 		 * still do), but now we need separate ones of our own.
+		 *
+		 * XXX is this correct if the partition is partitioned?
 		 */
 		fkconstraint = makeNode(Constraint);
 		fkconstraint->conname = pstrdup(NameStr(conform->conname));
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 7386f4d635..efd061b857 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -2313,6 +2313,95 @@ SET CONSTRAINTS fk_a_fkey DEFERRED;
 DELETE FROM pk WHERE a = 1;
 DELETE FROM fk WHERE a = 1;
 COMMIT;							-- OK
+-- Verify constraint deferrability when changed by ALTER
+-- Partitioned table at referencing end
+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;
+-- Multi-level partitioning at referencing end
+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_2 PARTITION OF ref FOR VALUES IN (1, 2) PARTITION BY list (f2);
+CREATE TABLE ref1 PARTITION OF ref1_2 FOR VALUES IN (1);
+CREATE TABLE ref2 PARTITION OF ref1_2 FOR VALUES IN (2);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+INSERT INTO pt VALUES(1,2,3), (1,1,3);
+INSERT INTO ref VALUES(1,2,3), (1,1,3);
+SELECT tableoid::regclass, * from ref;
+ tableoid | f1 | f2 | f3 
+----------+----+----+----
+ ref1     |  1 |  1 |  3
+ ref2     |  1 |  2 |  3
+(2 rows)
+
+ALTER TABLE ref1_2 ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+WARNING:  constraint "ref_f1_f2_fkey" of relation "ref1_2" is not top-level -- proceeding anyway
+HINT:  You may want to alter the constraint in the ancestor table instead.
+ALTER TABLE ref2 ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY IMMEDIATE;
+WARNING:  constraint "ref_f1_f2_fkey" of relation "ref2" is not top-level -- proceeding anyway
+HINT:  You may want to alter the constraint in the ancestor table instead.
+BEGIN;
+DELETE FROM pt;		-- fails
+ERROR:  update or delete on table "pt" violates foreign key constraint "ref_f1_f2_fkey" on table "ref"
+DETAIL:  Key (f1, f2)=(1, 2) is still referenced from table "ref".
+DELETE FROM ref;
+ERROR:  current transaction is aborted, commands ignored until end of transaction block
+ABORT;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
+-- Partitioned table at referenced end
+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;
+-- Multi-level partitioning at at referenced end
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2))
+  PARTITION BY LIST(f1);
+CREATE TABLE pt1_2 PARTITION OF pt FOR VALUES IN (1, 2) PARTITION BY LIST (f1);
+CREATE TABLE pt1 PARTITION OF pt1_2 FOR VALUES IN (1);
+CREATE TABLE pt2 PARTITION OF pt1_2 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..124993263d 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1636,6 +1636,85 @@ SET CONSTRAINTS fk_a_fkey DEFERRED;
 DELETE FROM pk WHERE a = 1;
 DELETE FROM fk WHERE a = 1;
 COMMIT;							-- OK
+
+-- Verify constraint deferrability when changed by ALTER
+-- Partitioned table at referencing end
+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;
+-- Multi-level partitioning at referencing end
+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_2 PARTITION OF ref FOR VALUES IN (1, 2) PARTITION BY list (f2);
+CREATE TABLE ref1 PARTITION OF ref1_2 FOR VALUES IN (1);
+CREATE TABLE ref2 PARTITION OF ref1_2 FOR VALUES IN (2);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+INSERT INTO pt VALUES(1,2,3), (1,1,3);
+INSERT INTO ref VALUES(1,2,3), (1,1,3);
+SELECT tableoid::regclass, * from ref;
+ALTER TABLE ref1_2 ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+ALTER TABLE ref2 ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY IMMEDIATE;
+BEGIN;
+DELETE FROM pt;		-- fails
+DELETE FROM ref;
+ABORT;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
+
+-- Partitioned table at referenced end
+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;
+-- Multi-level partitioning at at referenced end
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2))
+  PARTITION BY LIST(f1);
+CREATE TABLE pt1_2 PARTITION OF pt FOR VALUES IN (1, 2) PARTITION BY LIST (f1);
+CREATE TABLE pt1 PARTITION OF pt1_2 FOR VALUES IN (1);
+CREATE TABLE pt2 PARTITION OF pt1_2 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

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: ALTER CONSTRAINT on a partitioned FK isn't working
@ 2021-05-04 00:21  Alvaro Herrera <alvherre@alvh.no-ip.org>
  parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
  1 sibling, 0 replies; 8+ messages in thread

From: Alvaro Herrera @ 2021-05-04 00:21 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org

This backpatches to 12 and 13 cleanly, but 11 needs a completely
different patch -- attached.


-- 
Álvaro Herrera       Valdivia, Chile
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)

Attachments:

  0001-fix-ALTER-CONSTRAINT-for-partitioned-tables.patch (text/x-diff, 0B) download

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: ALTER CONSTRAINT on a partitioned FK isn't working
@ 2021-05-04 16:26  Alvaro Herrera <alvherre@alvh.no-ip.org>
  parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
  1 sibling, 1 reply; 8+ messages in thread

From: Alvaro Herrera @ 2021-05-04 16:26 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org, Ron <ronljohnsonjr@gmail.com>

On 2021-May-03, Alvaro Herrera wrote:

> There is a case where it doesn't seem like we can easily give
> non-surprising behavior: when multi-level partitioning is used, a
> constraint is defined in the top level and then it is altered in the
> middle level, then we don't have access to the action triggers, because
> those are defined at the parent level.  I opted for adding a WARNING
> message in that case.  Other options were 1) to raise an error if
> altering a constraint that's not top level, but I fear that might break
> restoring dumps of existing database;

Actually, I tested this and it turns out to be a nonexistant problem: if
you alter a constraint that's not top-level, then pg_dump silently omits
the deferrability change there.  So we can just have this emit ERROR and
nothing that works today would break.  Users *would* get an error, but
then the alteration would not have the desired effect anyway.  I suppose
we would have already got complaints if anybody were trying.  (The error
message could stand some wording improvement ...)

So the attached version does that and I intend to get it pushed to all
branches (since 11) this afternoon unless I get any objections.

I'm CC'ing Ron as the reporter of the original bug.

-- 
Álvaro Herrera       Valdivia, Chile

Attachments:

constraints.patch (text/x-diff, 17.8K) download
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a992f1bef8..b6689268c5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -355,8 +355,13 @@ 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 bool ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
+									 Relation rel, HeapTuple contuple, List **otherrelids,
+									 LOCKMODE lockmode);
 static ObjectAddress ATExecValidateConstraint(List **wqueue,
 											  Relation rel, char *constrName,
 											  bool recurse, bool recursing, LOCKMODE lockmode);
@@ -4669,6 +4674,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			break;
 		case AT_AlterConstraint:	/* ALTER CONSTRAINT */
 			ATSimplePermissions(rel, ATT_TABLE);
+			/* Recursion occurs during execution phase */
 			pass = AT_PASS_MISC;
 			break;
 		case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */
@@ -4967,7 +4973,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,28 +10196,30 @@ 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 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;
+	Relation	tgrel;
 	SysScanDesc scan;
 	ScanKeyData skey[3];
 	HeapTuple	contuple;
 	Form_pg_constraint currcon;
 	ObjectAddress address;
+	List	   *otherrelids = NIL;
+	ListCell   *lc;
 
 	cmdcon = castNode(Constraint, cmd->def);
 
 	conrel = table_open(ConstraintRelationId, RowExclusiveLock);
+	tgrel = table_open(TriggerRelationId, RowExclusiveLock);
 
 	/*
 	 * Find and check the target constraint
@@ -10245,21 +10253,104 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 				 errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key constraint",
 						cmdcon->conname, RelationGetRelationName(rel))));
 
+	/*
+	 * If it's not the topmost constraint, raise an error.
+	 *
+	 * The problem is that altering a non-topmost constraint leaves some
+	 * triggers untouched, since they are not directly connected to this
+	 * constraint; also, pg_dump would ignore the deferrability status of the
+	 * individual constraint, since it only dumps topmost constraints.
+	 */
+	if (OidIsValid(currcon->conparentid))
+	{
+		HeapTuple	tp;
+		char	   *ancestorname = NULL;
+		char	   *ancestortable = NULL;
+
+		tp = SearchSysCache1(CONSTROID, ObjectIdGetDatum(currcon->conparentid));
+		if (HeapTupleIsValid(tp))
+		{
+			Form_pg_constraint contup = (Form_pg_constraint) GETSTRUCT(tp);
+
+			ancestorname = pstrdup(NameStr(contup->conname));
+			ancestortable = get_rel_name(contup->conrelid);
+			ReleaseSysCache(tp);
+		}
+
+		ereport(ERROR,
+				(errmsg("cannot alter non-toplevel constraint"),
+				 ancestorname && ancestortable ?
+				 errdetail("Constraint \"%s\" descends from constraint \"%s\" of relation \"%s\".",
+						   cmdcon->conname, ancestorname, ancestortable) : 0));
+	}
+
+	/*
+	 * Do the actual catalog work.  We can skip changing if already in the
+	 * desired state, but not if a partitioned table: partitions need to be
+	 * processed regardless, in case they've had it locally changed.
+	 */
+	address = InvalidObjectAddress;
+	if (currcon->condeferrable != cmdcon->deferrable ||
+		currcon->condeferred != cmdcon->initdeferred ||
+		rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		if (ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, rel, contuple,
+									 &otherrelids, lockmode))
+			ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
+	}
+
+	/*
+	 * ATExecConstrRecurse already invalidated relcache for the relations
+	 * having the constraint itself; we also need to invalidate relations that
+	 * have any triggers that implement the constraint.
+	 */
+	foreach(lc, otherrelids)
+		CacheInvalidateRelcacheByRelid(lfirst_oid(lc));
+
+	systable_endscan(scan);
+
+	table_close(tgrel, RowExclusiveLock);
+	table_close(conrel, RowExclusiveLock);
+
+	return address;
+}
+
+/*
+ * Recursive subroutine of ATExecAlterConstraint
+ *
+ * *otherrelids is appended OIDs of relations containing affected triggers.
+ *
+ * Note that we must recurse even when the values are correct, in case
+ * indirect descendants have had their constraints altered locally.
+ * (This could be avoided if we forbade altering constraints in partitions
+ * but we don't do currently do that.)
+ */
+static bool
+ATExecAlterConstrRecurse(Constraint *cmdcon, Relation conrel, Relation tgrel,
+						 Relation rel, HeapTuple contuple, List **otherrelids,
+						 LOCKMODE lockmode)
+{
+	Form_pg_constraint currcon;
+	Oid			conoid;
+	Oid			refrelid;
+	bool		changed = false;
+
+	currcon = (Form_pg_constraint) GETSTRUCT(contuple);
+	conoid = currcon->oid;
+	refrelid = currcon->confrelid;
+
+	/*
+	 * Update pg_constraint with the flags from cmdcon.
+	 *
+	 * If called to modify a constraint that's already in the desired state,
+	 * silently do nothing.
+	 */
 	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;
@@ -10267,20 +10358,29 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 		CatalogTupleUpdate(conrel, &copyTuple->t_self, copyTuple);
 
 		InvokeObjectPostAlterHook(ConstraintRelationId,
-								  currcon->oid, 0);
+								  conoid, 0);
 
 		heap_freetuple(copyTuple);
+		changed = true;
+	}
 
-		/*
-		 * Now we need to update the multiple entries in pg_trigger that
-		 * implement the constraint.
-		 */
-		tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+	/* Make new constraint flags visible to others */
+	/* XXX only if the constraint changed? */
+	CacheInvalidateRelcache(rel);
+
+	/*
+	 * Now we need to update the multiple entries in pg_trigger that implement
+	 * the constraint.
+	 */
+	{
+		HeapTuple	tgtuple;
+		ScanKeyData tgkey;
+		SysScanDesc tgscan;
 
 		ScanKeyInit(&tgkey,
 					Anum_pg_trigger_tgconstraint,
 					BTEqualStrategyNumber, F_OIDEQ,
-					ObjectIdGetDatum(currcon->oid));
+					ObjectIdGetDatum(conoid));
 
 		tgscan = systable_beginscan(tgrel, TriggerConstraintIndexId, true,
 									NULL, 1, &tgkey);
@@ -10289,6 +10389,7 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 		{
 			Form_pg_trigger tgform = (Form_pg_trigger) GETSTRUCT(tgtuple);
 			Form_pg_trigger copy_tg;
+			HeapTuple	copyTuple;
 
 			/*
 			 * Remember OIDs of other relation(s) involved in FK constraint.
@@ -10297,8 +10398,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 			 * change, but let's be conservative.)
 			 */
 			if (tgform->tgrelid != RelationGetRelid(rel))
-				otherrelids = list_append_unique_oid(otherrelids,
-													 tgform->tgrelid);
+				*otherrelids = list_append_unique_oid(*otherrelids,
+													  tgform->tgrelid);
 
 			/*
 			 * Update deferrability of RI_FKey_noaction_del,
@@ -10325,31 +10426,46 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
 		}
 
 		systable_endscan(tgscan);
+	}
 
-		table_close(tgrel, RowExclusiveLock);
+	/*
+	 * If the table at either end of the constraint is partitioned, we need to
+	 * recurse and handle every constraint that is a child of this one.
+	 *
+	 * (This assumes that the recurse flag is forcibly set for partitioned
+	 * tables, and not set for legacy inheritance, though we don't check for
+	 * that here.)
+	 */
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE ||
+		get_rel_relkind(refrelid) == RELKIND_PARTITIONED_TABLE)
+	{
+		ScanKeyData pkey;
+		SysScanDesc pscan;
+		HeapTuple	childtup;
 
-		/*
-		 * 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)
+		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)))
 		{
-			CacheInvalidateRelcacheByRelid(lfirst_oid(lc));
+			Form_pg_constraint childcon = (Form_pg_constraint) GETSTRUCT(childtup);
+			Relation	childrel;
+
+			childrel = table_open(childcon->conrelid, lockmode);
+			ATExecAlterConstrRecurse(cmdcon, conrel, tgrel, childrel, childtup,
+									 otherrelids, lockmode);
+			table_close(childrel, NoLock);
 		}
 
-		ObjectAddressSet(address, ConstraintRelationId, currcon->oid);
+		systable_endscan(pscan);
 	}
-	else
-		address = InvalidObjectAddress;
 
-	systable_endscan(scan);
-
-	table_close(conrel, RowExclusiveLock);
-
-	return address;
+	return changed;
 }
 
 /*
@@ -17824,6 +17940,8 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 		 * Make the action triggers on the referenced relation.  When this was
 		 * a partition the action triggers pointed to the parent rel (they
 		 * still do), but now we need separate ones of our own.
+		 *
+		 * XXX is this correct if the partition is partitioned?
 		 */
 		fkconstraint = makeNode(Constraint);
 		fkconstraint->conname = pstrdup(NameStr(conform->conname));
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 7386f4d635..421aadbf33 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -2313,6 +2313,95 @@ SET CONSTRAINTS fk_a_fkey DEFERRED;
 DELETE FROM pk WHERE a = 1;
 DELETE FROM fk WHERE a = 1;
 COMMIT;							-- OK
+-- Verify constraint deferrability when changed by ALTER
+-- Partitioned table at referencing end
+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;
+-- Multi-level partitioning at referencing end
+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_2 PARTITION OF ref FOR VALUES IN (1, 2) PARTITION BY list (f2);
+CREATE TABLE ref1 PARTITION OF ref1_2 FOR VALUES IN (1);
+CREATE TABLE ref2 PARTITION OF ref1_2 FOR VALUES IN (2);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+INSERT INTO pt VALUES(1,2,3), (1,1,3);
+INSERT INTO ref VALUES(1,2,3), (1,1,3);
+SELECT tableoid::regclass, * from ref;
+ tableoid | f1 | f2 | f3 
+----------+----+----+----
+ ref1     |  1 |  1 |  3
+ ref2     |  1 |  2 |  3
+(2 rows)
+
+ALTER TABLE ref1_2 ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+ERROR:  cannot alter non-toplevel constraint
+DETAIL:  Constraint "ref_f1_f2_fkey" descends from constraint "ref_f1_f2_fkey" of relation "ref".
+ALTER TABLE ref2 ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY IMMEDIATE;
+ERROR:  cannot alter non-toplevel constraint
+DETAIL:  Constraint "ref_f1_f2_fkey" descends from constraint "ref_f1_f2_fkey" of relation "ref1_2".
+BEGIN;
+DELETE FROM pt;		-- fails
+ERROR:  update or delete on table "pt" violates foreign key constraint "ref_f1_f2_fkey" on table "ref"
+DETAIL:  Key (f1, f2)=(1, 2) is still referenced from table "ref".
+DELETE FROM ref;
+ERROR:  current transaction is aborted, commands ignored until end of transaction block
+ABORT;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
+-- Partitioned table at referenced end
+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;
+-- Multi-level partitioning at at referenced end
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2))
+  PARTITION BY LIST(f1);
+CREATE TABLE pt1_2 PARTITION OF pt FOR VALUES IN (1, 2) PARTITION BY LIST (f1);
+CREATE TABLE pt1 PARTITION OF pt1_2 FOR VALUES IN (1);
+CREATE TABLE pt2 PARTITION OF pt1_2 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..124993263d 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1636,6 +1636,85 @@ SET CONSTRAINTS fk_a_fkey DEFERRED;
 DELETE FROM pk WHERE a = 1;
 DELETE FROM fk WHERE a = 1;
 COMMIT;							-- OK
+
+-- Verify constraint deferrability when changed by ALTER
+-- Partitioned table at referencing end
+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;
+-- Multi-level partitioning at referencing end
+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_2 PARTITION OF ref FOR VALUES IN (1, 2) PARTITION BY list (f2);
+CREATE TABLE ref1 PARTITION OF ref1_2 FOR VALUES IN (1);
+CREATE TABLE ref2 PARTITION OF ref1_2 FOR VALUES IN (2);
+ALTER TABLE ref ADD FOREIGN KEY(f1,f2) REFERENCES pt;
+INSERT INTO pt VALUES(1,2,3), (1,1,3);
+INSERT INTO ref VALUES(1,2,3), (1,1,3);
+SELECT tableoid::regclass, * from ref;
+ALTER TABLE ref1_2 ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+ALTER TABLE ref2 ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY IMMEDIATE;
+BEGIN;
+DELETE FROM pt;		-- fails
+DELETE FROM ref;
+ABORT;
+ALTER TABLE ref ALTER CONSTRAINT ref_f1_f2_fkey
+  DEFERRABLE INITIALLY DEFERRED;
+BEGIN;
+DELETE FROM pt;
+DELETE FROM ref;
+ABORT;
+DROP TABLE pt, ref;
+
+-- Partitioned table at referenced end
+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;
+-- Multi-level partitioning at at referenced end
+CREATE TABLE pt(f1 int, f2 int, f3 int, PRIMARY KEY(f1,f2))
+  PARTITION BY LIST(f1);
+CREATE TABLE pt1_2 PARTITION OF pt FOR VALUES IN (1, 2) PARTITION BY LIST (f1);
+CREATE TABLE pt1 PARTITION OF pt1_2 FOR VALUES IN (1);
+CREATE TABLE pt2 PARTITION OF pt1_2 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

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: ALTER CONSTRAINT on a partitioned FK isn't working
@ 2021-05-05 16:24  Alvaro Herrera <alvherre@alvh.no-ip.org>
  parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
  0 siblings, 0 replies; 8+ messages in thread

From: Alvaro Herrera @ 2021-05-05 16:24 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org, Ron <ronljohnsonjr@gmail.com>

On 2021-May-04, Alvaro Herrera wrote:

> So the attached version does that and I intend to get it pushed to all
> branches (since 11) this afternoon unless I get any objections.

Pushed it now, with some further adjustments.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W





^ permalink  raw  reply  [nested|flat] 8+ messages in thread

end of thread, other threads:[~2021-05-05 16:24 UTC | newest]

Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-07 19:33 ALTER CONSTRAINT on a partitioned FK isn't working Tom Lane <tgl@sss.pgh.pa.us>
2020-12-07 23:34 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2020-12-07 23:52   ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-04-29 23:02 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-05-03 23:16   ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-05-04 00:21     ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-05-04 16:26     ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-05-05 16:24       ` Alvaro Herrera <alvherre@alvh.no-ip.org>

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox