From 4937a327afd51cddfdad7e01f3ccd9493b893bab Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 6 Aug 2024 16:59:04 -0400
Subject: [PATCH] Rework foreign key mangling during ATTACH/DETACH

... when the reference table is partitioned.

It turns out that the catalog representation we chose for foreign keys
connecting partitioned tables is suboptimal, particularly in the sense
that a standalone table has a different way to represent it when
referencing a partitioned table, than when the same table becomes a
partition (and vice versa).  This difference means we need to spawn
additional catalog rows on detach, and remove them on attach.

As a very obvious symptom, we were missing action triggers, which means
that you could update/delete rows from the referenced partitioned table
that still had referencing rows at the other side, and fail to throw the
required errors.  This means existing FKs might have rows that break
relational integrity.  Another possible problem is that trying to
reattach a table that had been detached would fail indicating that
internal triggers cannot be found, which from the user's point of view
is nonsensical.

We might want to rethink the representation in the future to avoid this
messiness, but the code now seems to do what's required to make the
constraints operate correctly.

 co-author : Tender Wang <tndrwang@gmail.com>

Reported-by: Guillaume Lelarge <guillaume@lelarge.info>
Reported-by: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Reported-by: Thomas Baehler (SBB CFF FFS) <thomas.baehler2@sbb.ch>

Discussion: https://postgr.es/m/20230420144344.40744130@karst
Discussion: https://postgr.es/m/20230705233028.2f554f73@karst
Discussion: https://postgr.es/m/GVAP278MB02787E7134FD691861635A8BC9032@GVAP278MB0278.CHEP278.PROD.OUTLOOK.COM
Discussion: https://postgr.es/m/18541-628a61bc267cd2d3@postgresql.org

verify: 20230707175859.17c91538@karst
---
 src/backend/commands/tablecmds.c          | 315 +++++++++++++++++++---
 src/test/regress/expected/foreign_key.out |  59 ++++
 src/test/regress/sql/foreign_key.sql      |  30 +++
 3 files changed, 373 insertions(+), 31 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1f94f4fdbb..bb40541ba1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10641,7 +10641,7 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel)
 		 * Because we're only expanding the key space at the referenced side,
 		 * we don't need to prevent any operation in the referencing table, so
 		 * AccessShareLock suffices (assumes that dropping the constraint
-		 * acquires AEL).
+		 * acquires AccessExclusiveLock).
 		 */
 		fkRel = table_open(constrForm->conrelid, AccessShareLock);
 
@@ -11147,6 +11147,83 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk,
 	TriggerSetParentTrigger(trigrel, updateTriggerOid, parentUpdTrigger,
 							partRelid);
 
+	/*
+	 * If the referenced table is partitioned, then the partition we're
+	 * attaching now has extra pg_constraint rows and action triggers that are
+	 * no longer needed.  Remove those.
+	 */
+	if (get_rel_relkind(fk->confrelid) == RELKIND_PARTITIONED_TABLE)
+	{
+		Relation	pg_constraint = table_open(ConstraintRelationId, RowShareLock);
+		ObjectAddresses *objs;
+		HeapTuple	consttup;
+
+		ScanKeyInit(&key,
+					Anum_pg_constraint_conrelid,
+					BTEqualStrategyNumber, F_OIDEQ,
+					ObjectIdGetDatum(fk->conrelid));
+
+		scan = systable_beginscan(pg_constraint,
+								  ConstraintRelidTypidNameIndexId,
+								  true, NULL, 1, &key);
+		objs = new_object_addresses();
+		while ((consttup = systable_getnext(scan)) != NULL)
+		{
+			Form_pg_constraint conform = (Form_pg_constraint) GETSTRUCT(consttup);
+
+			if (conform->conparentid != fk->conoid)
+				continue;
+			else
+			{
+				ObjectAddress addr;
+				int			n;
+				SysScanDesc scan2;
+				ScanKeyData key2;
+
+				ObjectAddressSet(addr, ConstraintRelationId, conform->oid);
+				add_exact_object_address(&addr, objs);
+
+				/*
+				 * First we must delete the dependency records that bind
+				 * the constraint records together.
+				 */
+				n = deleteDependencyRecordsForSpecific(ConstraintRelationId,
+													   conform->oid,
+													   DEPENDENCY_INTERNAL,
+													   ConstraintRelationId,
+													   fk->conoid);
+				if (n != 1)
+					elog(WARNING, "oops: found %d instead of 1 deps from %u to %u",
+						 n, conform->oid, fk->conoid);
+
+				/*
+				 * Now search for the triggers for this constraint and set
+				 * them up for deletion too
+				 */
+				ScanKeyInit(&key2,
+							Anum_pg_trigger_tgconstraint,
+							BTEqualStrategyNumber, F_OIDEQ,
+							ObjectIdGetDatum(conform->oid));
+				scan2 = systable_beginscan(trigrel, TriggerConstraintIndexId,
+										   true, NULL, 1, &key2);
+				while ((trigtup = systable_getnext(scan2)) != NULL)
+				{
+					ObjectAddressSet(addr, TriggerRelationId,
+									 ((Form_pg_trigger) GETSTRUCT(trigtup))->oid);
+					add_exact_object_address(&addr, objs);
+				}
+				systable_endscan(scan2);
+			}
+		}
+		/* make the dependency deletions visible */
+		CommandCounterIncrement();
+		performMultipleDeletions(objs, DROP_RESTRICT,
+								 PERFORM_DELETION_INTERNAL);
+		systable_endscan(scan);
+
+		table_close(pg_constraint, RowShareLock);
+	}
+
 	CommandCounterIncrement();
 	return true;
 }
@@ -19182,8 +19259,10 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 	DropClonedTriggersFromPartition(RelationGetRelid(partRel));
 
 	/*
-	 * Detach any foreign keys that are inherited.  This includes creating
-	 * additional action triggers.
+	 * Detach any foreign keys that are inherited.  This requires marking some
+	 * constraints and triggers as no longer having parents, as well as
+	 * creating additional constraint entries and triggers, depending on the
+	 * shape of each FK.
 	 */
 	fks = copyObject(RelationGetFKeyList(partRel));
 	if (fks != NIL)
@@ -19192,10 +19271,15 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 	{
 		ForeignKeyCacheInfo *fk = lfirst(cell);
 		HeapTuple	contup;
+		HeapTuple	parentConTup;
 		Form_pg_constraint conform;
+		Form_pg_constraint parentConForm;
 		Constraint *fkconstraint;
-		Oid			insertTriggerOid,
-					updateTriggerOid;
+		Oid			parentConstrOid;
+		Oid			insertCheckTrigOid,
+					updateCheckTrigOid,
+					updateActionTrigOid,
+					deleteActionTrigOid;
 
 		contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid));
 		if (!HeapTupleIsValid(contup))
@@ -19210,52 +19294,221 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 			continue;
 		}
 
-		/* unset conparentid and adjust conislocal, coninhcount, etc. */
-		ConstraintSetParentConstraint(fk->conoid, InvalidOid, InvalidOid);
-
 		/*
-		 * Also, look up the partition's "check" triggers corresponding to the
-		 * constraint being detached and detach them from the parent triggers.
+		 * To create a FK constraint in the to-be-detached partition
+		 * equivalent to what exists in the parent table, what to do depends
+		 * on whether the referenced table is partitioned or not.  If the
+		 * referenced table isn't partitioned, then just reparenting the
+		 * pg_constraint row and pg_trigger rows for the existing constraint,
+		 * and creating new action triggers, is sufficient.
+		 *
+		 * If the referenced table is partitioned, then we must create
+		 * additional pg_constraint rows -- one for each partition -- and the
+		 * necessary action triggers for each constraint.
 		 */
-		GetForeignKeyCheckTriggers(trigrel,
-								   fk->conoid, fk->confrelid, fk->conrelid,
-								   &insertTriggerOid, &updateTriggerOid);
-		Assert(OidIsValid(insertTriggerOid));
-		TriggerSetParentTrigger(trigrel, insertTriggerOid, InvalidOid,
-								RelationGetRelid(partRel));
-		Assert(OidIsValid(updateTriggerOid));
-		TriggerSetParentTrigger(trigrel, updateTriggerOid, InvalidOid,
-								RelationGetRelid(partRel));
+		parentConstrOid = conform->conparentid;
 
-		/*
-		 * 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.
-		 */
+		Assert(OidIsValid(conform->conparentid));
+		parentConTup = SearchSysCache1(CONSTROID,
+									   ObjectIdGetDatum(parentConstrOid));
+		if (!HeapTupleIsValid(parentConTup))
+			elog(ERROR, "cache lookup failed for constraint %u",
+				 conform->conparentid);
+		parentConForm = (Form_pg_constraint) GETSTRUCT(parentConTup);
+
+		/* Create a synthetic node we'll use throughout */
 		fkconstraint = makeNode(Constraint);
 		fkconstraint->contype = CONSTRAINT_FOREIGN;
 		fkconstraint->conname = pstrdup(NameStr(conform->conname));
 		fkconstraint->deferrable = conform->condeferrable;
 		fkconstraint->initdeferred = conform->condeferred;
-		fkconstraint->location = -1;
-		fkconstraint->pktable = NULL;
-		fkconstraint->fk_attrs = NIL;
-		fkconstraint->pk_attrs = NIL;
+		fkconstraint->skip_validation = true;
+		fkconstraint->initially_valid = true;
+		/* a few irrelevant fields omitted here */
+		fkconstraint->pktable = NULL;	/* not needed */
+		fkconstraint->fk_attrs = NIL;	/* not needed */
+		fkconstraint->pk_attrs = NIL;	/* not needed */
 		fkconstraint->fk_matchtype = conform->confmatchtype;
 		fkconstraint->fk_upd_action = conform->confupdtype;
 		fkconstraint->fk_del_action = conform->confdeltype;
 		fkconstraint->fk_del_set_cols = NIL;
 		fkconstraint->old_conpfeqop = NIL;
 		fkconstraint->old_pktable_oid = InvalidOid;
-		fkconstraint->skip_validation = false;
-		fkconstraint->initially_valid = true;
+		fkconstraint->location = -1;
 
+		/*
+		 * The constraint on this table must be marked no longer a child of
+		 * the parent's constraint, as do its check triggers.
+		 */
+		ConstraintSetParentConstraint(fk->conoid, InvalidOid, InvalidOid);
+
+		GetForeignKeyCheckTriggers(trigrel,
+								   fk->conoid, fk->confrelid, fk->conrelid,
+								   &insertCheckTrigOid, &updateCheckTrigOid);
+		Assert(OidIsValid(insertCheckTrigOid));
+		TriggerSetParentTrigger(trigrel, insertCheckTrigOid, InvalidOid,
+								RelationGetRelid(partRel));
+		Assert(OidIsValid(updateCheckTrigOid));
+		TriggerSetParentTrigger(trigrel, updateCheckTrigOid, InvalidOid,
+								RelationGetRelid(partRel));
+
+		/*
+		 * Now create the action triggers in this table that point to the
+		 * referenced table.
+		 */
 		createForeignKeyActionTriggers(partRel, conform->confrelid,
 									   fkconstraint, fk->conoid,
 									   conform->conindid,
 									   InvalidOid, InvalidOid,
-									   NULL, NULL);
+									   &deleteActionTrigOid, &updateActionTrigOid);
 
+		/*
+		 * If the referenced side is partitioned (which we know because our
+		 * parent's constraint points to a different relation than ours) then
+		 * we must, in addition to the above, create pg_constraint rows that
+		 * point to each partition, each with its own action triggers.
+		 */
+		if (parentConForm->conrelid != conform->conrelid)
+		{
+			List	   *children;
+			ListCell   *child;
+			Relation	refdRel;
+			Oid			indexOid;
+			int			numfks;
+			AttrNumber	conkey[INDEX_MAX_KEYS];
+			AttrNumber	confkey[INDEX_MAX_KEYS];
+			Oid			conpfeqop[INDEX_MAX_KEYS];
+			Oid			conppeqop[INDEX_MAX_KEYS];
+			Oid			conffeqop[INDEX_MAX_KEYS];
+			int			numfkdelsetcols;
+			AttrNumber	confdelsetcols[INDEX_MAX_KEYS];
+			List	   *colnames = NIL;
+			char	   *fkaddition;
+
+			DeconstructFkConstraintRow(contup,
+									   &numfks,
+									   conkey,
+									   confkey,
+									   conpfeqop,
+									   conppeqop,
+									   conffeqop,
+									   &numfkdelsetcols,
+									   confdelsetcols);
+
+			/*
+			 * Choose the "name2" part to give to ChooseConstraintName.  This
+			 * is common to all constraints we create below.
+			 */
+			for (int i = 0; i < numfks; i++)
+			{
+				Form_pg_attribute att;
+
+				att = TupleDescAttr(RelationGetDescr(partRel),
+									conkey[i] - 1);
+				colnames = lappend(colnames, makeString(NameStr(att->attname)));
+			}
+			fkaddition = ChooseForeignKeyConstraintNameAddition(colnames);
+
+			/*
+			 * We're not expanding nor shrinking key space, so AccessShareLock
+			 * is sufficient here given that dropping a constraint requires
+			 * AccessExclusiveLock.
+			 */
+			refdRel = table_open(fk->confrelid, AccessShareLock);
+
+			/*
+			 * When the referenced table is partitioned, we need new
+			 * pg_constraint entries that point from our partition to each
+			 * partition of the other table, and also the action triggers on
+			 * each.
+			 */
+			children = find_all_inheritors(fk->confrelid, NoLock, NULL);
+			foreach(child, children)
+			{
+				Oid			childoid = lfirst_oid(child);
+				Relation	fchild;
+				AttrNumber	mapped_confkey[INDEX_MAX_KEYS];
+				AttrMap    *attmap;
+				char	   *conname;
+				Oid			constrOid;
+				ObjectAddress address,
+							referenced;
+
+				/* A constraint for the top rel already exists, so skip it */
+				if (childoid == fk->confrelid)
+					continue;
+
+				fchild = table_open(childoid, AccessShareLock);
+
+				/*
+				 * Map the referenced attnums in case this partition has a
+				 * different column layout.
+				 */
+				attmap = build_attrmap_by_name(RelationGetDescr(refdRel),
+											   RelationGetDescr(fchild),
+											   false);
+				for (int i = 0; i < numfks; i++)
+					mapped_confkey[i] = attmap->attnums[confkey[i] - 1];
+
+				conname = ChooseConstraintName(RelationGetRelationName(partRel),
+											   fkaddition, "fkey",
+											   RelationGetNamespace(partRel), NIL);
+
+				/* Create the pg_constraint row to this specific partition */
+				indexOid = index_get_partition(fchild, conform->conindid);
+				constrOid =
+					CreateConstraintEntry(conname,
+										  RelationGetNamespace(partRel),
+										  CONSTRAINT_FOREIGN,
+										  fkconstraint->deferrable,
+										  fkconstraint->initdeferred,
+										  fkconstraint->initially_valid,
+										  fk->conoid,
+										  RelationGetRelid(partRel),
+										  conkey,
+										  numfks,
+										  numfks,
+										  InvalidOid,
+										  indexOid,
+										  childoid,
+										  mapped_confkey,
+										  conpfeqop,
+										  conppeqop,
+										  conffeqop,
+										  numfks,
+										  fkconstraint->fk_upd_action,
+										  fkconstraint->fk_del_action,
+										  confdelsetcols,
+										  numfkdelsetcols,
+										  fkconstraint->fk_matchtype,
+										  NULL,
+										  NULL,
+										  NULL,
+										  false,
+										  1,
+										  false,
+										  false);
+
+				/* Give it a dependency on the "real" (parent) constraint */
+				ObjectAddressSet(address, ConstraintRelationId, constrOid);
+				ObjectAddressSet(referenced, ConstraintRelationId, fk->conoid);
+				recordDependencyOn(&address, &referenced, DEPENDENCY_INTERNAL);
+
+				/* And create the action triggers that go with it */
+				createForeignKeyActionTriggers(partRel, childoid,
+											   fkconstraint,
+											   constrOid, indexOid,
+											   deleteActionTrigOid,
+											   updateActionTrigOid,
+											   NULL, NULL);
+
+				table_close(fchild, NoLock);
+			}
+
+			table_close(refdRel, NoLock);
+		}
+
+		ReleaseSysCache(parentConTup);
 		ReleaseSysCache(contup);
 	}
 	list_free_deep(fks);
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 46764bd9e3..2fc280d3bd 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -2917,3 +2917,62 @@ DETAIL:  drop cascades to table fkpart11.pk
 drop cascades to table fkpart11.fk_parted
 drop cascades to table fkpart11.fk_another
 drop cascades to function fkpart11.print_row()
+-- When a table is attached as partition to a partitioned table that has
+-- a foreign key to another partitioned table, it acquires a clone of the
+-- FK.  Upon detach, this clone is not removed, but instead becomes an
+-- independent FK.  If it then attaches to the partitioned table again,
+-- the FK from the parent "takes over" ownership of the independent FK rather
+-- than creating a separate one.
+CREATE SCHEMA fkpart12
+  CREATE TABLE fk_p ( id bigint PRIMARY KEY ) PARTITION BY list (id)
+  CREATE TABLE fk_p_1 PARTITION OF fk_p FOR VALUES IN (1)
+  CREATE TABLE fk_p_2 PARTITION OF fk_p FOR VALUES IN (2)
+  CREATE TABLE fk_r_1 ( id bigint PRIMARY KEY, p_id bigint NOT NULL)
+  CREATE TABLE fk_r_2 ( id bigint PRIMARY KEY, p_id bigint NOT NULL)
+  CREATE TABLE fk_r   ( id bigint PRIMARY KEY, p_id bigint NOT NULL,
+	FOREIGN KEY (p_id) REFERENCES fk_p (id)
+  ) PARTITION BY list (id);
+SET search_path TO fkpart12;
+ALTER TABLE fk_r ATTACH PARTITION fk_r_2 FOR VALUES IN (2);
+ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
+\d fk_r_1
+             Table "fkpart12.fk_r_1"
+ Column |  Type  | Collation | Nullable | Default 
+--------+--------+-----------+----------+---------
+ id     | bigint |           | not null | 
+ p_id   | bigint |           | not null | 
+Partition of: fk_r FOR VALUES IN ('1')
+Indexes:
+    "fk_r_1_pkey" PRIMARY KEY, btree (id)
+Foreign-key constraints:
+    TABLE "fk_r" CONSTRAINT "fk_r_p_id_fkey" FOREIGN KEY (p_id) REFERENCES fk_p(id)
+
+ALTER TABLE fk_r DETACH PARTITION fk_r_1;
+\d fk_r_1
+             Table "fkpart12.fk_r_1"
+ Column |  Type  | Collation | Nullable | Default 
+--------+--------+-----------+----------+---------
+ id     | bigint |           | not null | 
+ p_id   | bigint |           | not null | 
+Indexes:
+    "fk_r_1_pkey" PRIMARY KEY, btree (id)
+Foreign-key constraints:
+    "fk_r_p_id_fkey" FOREIGN KEY (p_id) REFERENCES fk_p(id)
+
+ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
+\d fk_r_1
+             Table "fkpart12.fk_r_1"
+ Column |  Type  | Collation | Nullable | Default 
+--------+--------+-----------+----------+---------
+ id     | bigint |           | not null | 
+ p_id   | bigint |           | not null | 
+Partition of: fk_r FOR VALUES IN ('1')
+Indexes:
+    "fk_r_1_pkey" PRIMARY KEY, btree (id)
+Foreign-key constraints:
+    TABLE "fk_r" CONSTRAINT "fk_r_p_id_fkey" FOREIGN KEY (p_id) REFERENCES fk_p(id)
+
+SET client_min_messages TO warning;
+DROP SCHEMA fkpart12 CASCADE;
+RESET client_min_messages;
+RESET search_path;
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index f5e0938999..359eb6eefe 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2069,3 +2069,33 @@ UPDATE fkpart11.pk SET a = 3 WHERE a = 4;
 UPDATE fkpart11.pk SET a = 1 WHERE a = 2;
 
 DROP SCHEMA fkpart11 CASCADE;
+
+-- When a table is attached as partition to a partitioned table that has
+-- a foreign key to another partitioned table, it acquires a clone of the
+-- FK.  Upon detach, this clone is not removed, but instead becomes an
+-- independent FK.  If it then attaches to the partitioned table again,
+-- the FK from the parent "takes over" ownership of the independent FK rather
+-- than creating a separate one.
+CREATE SCHEMA fkpart12
+  CREATE TABLE fk_p ( id bigint PRIMARY KEY ) PARTITION BY list (id)
+  CREATE TABLE fk_p_1 PARTITION OF fk_p FOR VALUES IN (1)
+  CREATE TABLE fk_p_2 PARTITION OF fk_p FOR VALUES IN (2)
+  CREATE TABLE fk_r_1 ( id bigint PRIMARY KEY, p_id bigint NOT NULL)
+  CREATE TABLE fk_r_2 ( id bigint PRIMARY KEY, p_id bigint NOT NULL)
+  CREATE TABLE fk_r   ( id bigint PRIMARY KEY, p_id bigint NOT NULL,
+	FOREIGN KEY (p_id) REFERENCES fk_p (id)
+  ) PARTITION BY list (id);
+SET search_path TO fkpart12;
+ALTER TABLE fk_r ATTACH PARTITION fk_r_2 FOR VALUES IN (2);
+
+ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
+\d fk_r_1
+ALTER TABLE fk_r DETACH PARTITION fk_r_1;
+\d fk_r_1
+ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
+\d fk_r_1
+
+SET client_min_messages TO warning;
+DROP SCHEMA fkpart12 CASCADE;
+RESET client_min_messages;
+RESET search_path;
-- 
2.39.2

