Restructure foreign key handling code for ATTACH/DETACH

alvherre · ioguix · tenderwg · alvherre · commit d20194cead75 · 2024-10-22T16:01:18.000+02:00
... to fix bugs when the referenced table is partitioned. The catalog representation we chose for foreign keys connecting partitioned tables (in commit f56f8f8) is inconvenient, in the sense that a standalone table has a different way to represent the constraint when referencing a partitioned table, than when the same table becomes a partition (and vice versa). Because of this, we need to create additional catalog rows on detach (pg_constraint and pg_trigger), and remove them on attach. We were doing some of those things, but not all of them, leading to missing catalog rows in certain cases. The worst problem seems to be that we are missing action triggers after detaching a partition, which means that you could update/delete rows from the referenced partitioned table that still had referencing rows on that table, the server failing to throw the required errors. !!! Note that this means existing databases with FKs that reference partitioned tables might have rows that break relational integrity, on tables that were once partitions on the referencing side of the FK. 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. In branches 15 and above, we fix this by creating a new helper function addFkConstraint() which is in charge of creating a standalone pg_constraint row, and repurposing addFkRecurseReferencing() and addFkRecurseReferenced() so that they're only the recursive routine for each side of the FK, and they call addFkConstraint() to create pg_constraint at each partitioning level and add the necessary triggers. These new routines can be used during partition creation, partition attach and detach, and foreign key creation. This reduces redundant code and simplifies the flow. In branches 14 and 13, we have a much simpler fix that consists on simply removing the constraint on detach. The reason is that those branches are missing commit f456634, which reworked the way this works in a way that we didn't consider back-patchable at the time. We opted to leave branch 12 alone, because it's different from branch 13 enough that the fix doesn't apply; and because it is going in EOL mode very soon, patching it now might be worse since there's no way to undo the damage if it goes wrong. Existing databases might need to be repaired. In the future we might want to rethink the catalog representation to avoid this problem, but for now the code seems to do what's required to make the constraints operate correctly. Co-authored-by: Jehan-Guillaume de Rorthais <jgdr@dalibo.com> Co-authored-by: Tender Wang <tndrwang@gmail.com> Co-authored-by: Alvaro Herrera <alvherre@alvh.no-ip.org> 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
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
@@ -10102,6 +10102,82 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk,
 	table_close(trigrel, RowExclusiveLock);
 
 	ConstraintSetParentConstraint(fk->conoid, parentConstrOid, 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;
+				SysScanDesc scan2;
+				ScanKeyData key2;
+				int         n PG_USED_FOR_ASSERTS_ONLY;
+
+				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);
+				Assert(n == 1); /* actually only one is expected */
+
+				/*
+				 * 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;
 }
@@ -17470,6 +17546,7 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
 	List	   *indexes;
 	List	   *fks;
 	ListCell   *cell;
+	ObjectAddresses *dropobjs = NULL;
 
 	/*
 	 * We must lock the default partition, because detaching this partition
@@ -17567,16 +17644,15 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
 	DropClonedTriggersFromPartition(RelationGetRelid(partRel));
 
 	/*
-	 * Detach any foreign keys that are inherited.  This includes creating
-	 * additional action triggers.
+	 * Detach any foreign keys that are inherited -- or, if they reference
+	 * partitioned tables, drop them.
 	 */
 	fks = copyObject(RelationGetFKeyList(partRel));
 	foreach(cell, fks)
 	{
 		ForeignKeyCacheInfo *fk = lfirst(cell);
 		HeapTuple	contup;
 		Form_pg_constraint conform;
-		Constraint *fkconstraint;
 
 		contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid));
 		if (!HeapTupleIsValid(contup))
@@ -17591,39 +17667,71 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
 			continue;
 		}
 
-		/* unset conparentid and adjust conislocal, coninhcount, etc. */
+		/* Mark the constraint as independent */
 		ConstraintSetParentConstraint(fk->conoid, InvalidOid, InvalidOid);
 
 		/*
-		 * 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.
+		 * If the constraint references a partitioned table, just drop the
+		 * constraint, because it's more work to preserve the constraint
+		 * correctly.
+		 *
+		 * If it references a plain table, then we can create the action
+		 * triggers and it'll be okay.
 		 */
-		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->fk_matchtype = conform->confmatchtype;
-		fkconstraint->fk_upd_action = conform->confupdtype;
-		fkconstraint->fk_del_action = conform->confdeltype;
-		fkconstraint->old_conpfeqop = NIL;
-		fkconstraint->old_pktable_oid = InvalidOid;
-		fkconstraint->skip_validation = false;
-		fkconstraint->initially_valid = true;
+		if (get_rel_relkind(fk->confrelid) == RELKIND_PARTITIONED_TABLE)
+		{
+			ObjectAddress constraddr;
 
-		createForeignKeyActionTriggers(partRel, conform->confrelid,
-									   fkconstraint, fk->conoid,
-									   conform->conindid);
+			/* make the dependency deletions above visible */
+			CommandCounterIncrement();
+
+			/*
+			 * Remember the constraint and its triggers for later deletion.
+			 */
+			if (dropobjs == NULL)
+				dropobjs = new_object_addresses();
+			ObjectAddressSet(constraddr, ConstraintRelationId, fk->conoid);
+			add_exact_object_address(&constraddr, dropobjs);
+		}
+		else
+		{
+			Constraint *fkconstraint;
+
+			/*
+			 * 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.
+			 */
+			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->fk_matchtype = conform->confmatchtype;
+			fkconstraint->fk_upd_action = conform->confupdtype;
+			fkconstraint->fk_del_action = conform->confdeltype;
+			fkconstraint->old_conpfeqop = NIL;
+			fkconstraint->old_pktable_oid = InvalidOid;
+			fkconstraint->skip_validation = false;
+			fkconstraint->initially_valid = true;
+
+			createForeignKeyActionTriggers(partRel, conform->confrelid,
+										   fkconstraint, fk->conoid,
+										   conform->conindid);
+		}
 
 		ReleaseSysCache(contup);
 	}
 	list_free_deep(fks);
 
+	/* If we collected any constraints for deletion, do so now. */
+	if (dropobjs != NULL)
+		performMultipleDeletions(dropobjs, DROP_CASCADE, 0);
+
 	/*
 	 * Any sub-constraints that are in the referenced-side of a larger
 	 * constraint have to be removed.  This partition is no longer part of the
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
@@ -2647,3 +2647,94 @@ DROP SCHEMA fkpart9 CASCADE;
 NOTICE:  drop cascades to 2 other objects
 DETAIL:  drop cascades to table fkpart9.pk
 drop cascades to table fkpart9.fk
+-- 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, Postgres 14 and earlier remove the foreign key (newer
+-- versions make it a standalone constraint.)
+CREATE SCHEMA fkpart12
+  CREATE TABLE fk_p ( id int, jd int, PRIMARY KEY(id, jd)) PARTITION BY list (id)
+  CREATE TABLE fk_p_1 PARTITION OF fk_p FOR VALUES IN (1) PARTITION BY list (jd)
+  CREATE TABLE fk_p_1_1 PARTITION OF fk_p_1 FOR VALUES IN (1)
+  CREATE TABLE fk_p_1_2 PARTITION OF fk_p_1 FOR VALUES IN (2)
+  CREATE TABLE fk_p_2 PARTITION OF fk_p FOR VALUES IN (2) PARTITION BY list (jd)
+  CREATE TABLE fk_p_2_1 PARTITION OF fk_p_2 FOR VALUES IN (1)
+  CREATE TABLE fk_p_2_2 PARTITION OF fk_p_2 FOR VALUES IN (2)
+  CREATE TABLE fk_r_1 ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL)
+  CREATE TABLE fk_r_2 ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL) PARTITION BY list (id)
+  CREATE TABLE fk_r_2_1 PARTITION OF fk_r_2 FOR VALUES IN (2, 1)
+  CREATE TABLE fk_r   ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL,
+       FOREIGN KEY (p_id, p_jd) REFERENCES fk_p (id, jd)
+  ) PARTITION BY list (id);
+SET search_path TO fkpart12;
+INSERT INTO fk_p VALUES (1, 1);
+ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
+ALTER TABLE fk_r ATTACH PARTITION fk_r_2 FOR VALUES IN (2);
+\d fk_r_2
+        Partitioned table "fkpart12.fk_r_2"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ id     | integer |           | not null | 
+ p_id   | integer |           | not null | 
+ p_jd   | integer |           | not null | 
+Partition of: fk_r FOR VALUES IN (2)
+Partition key: LIST (id)
+Indexes:
+    "fk_r_2_pkey" PRIMARY KEY, btree (id)
+Foreign-key constraints:
+    TABLE "fk_r" CONSTRAINT "fk_r_p_id_p_jd_fkey" FOREIGN KEY (p_id, p_jd) REFERENCES fk_p(id, jd)
+Number of partitions: 1 (Use \d+ to list them.)
+
+INSERT INTO fk_r VALUES (1, 1, 1);
+INSERT INTO fk_r VALUES (2, 2, 1);	-- fails
+ERROR:  insert or update on table "fk_r_2_1" violates foreign key constraint "fk_r_p_id_p_jd_fkey"
+DETAIL:  Key (p_id, p_jd)=(2, 1) is not present in table "fk_p".
+ALTER TABLE fk_r DETACH PARTITION fk_r_1;
+ALTER TABLE fk_r DETACH PARTITION fk_r_2;
+\d fk_r_2
+        Partitioned table "fkpart12.fk_r_2"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ id     | integer |           | not null | 
+ p_id   | integer |           | not null | 
+ p_jd   | integer |           | not null | 
+Partition key: LIST (id)
+Indexes:
+    "fk_r_2_pkey" PRIMARY KEY, btree (id)
+Number of partitions: 1 (Use \d+ to list them.)
+
+INSERT INTO fk_r_1 VALUES (2, 1, 2); -- works: there's no FK anymore
+DELETE FROM fk_p;	-- works
+ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);	-- fails
+ERROR:  partition constraint of relation "fk_r_1" is violated by some row
+INSERT INTO fk_r_2 VALUES (2, 2, 2);
+INSERT INTO fk_p VALUES (2, 2);
+ALTER TABLE fk_r ATTACH PARTITION fk_r_2 FOR VALUES IN (2);
+\d fk_r_2
+        Partitioned table "fkpart12.fk_r_2"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ id     | integer |           | not null | 
+ p_id   | integer |           | not null | 
+ p_jd   | integer |           | not null | 
+Partition of: fk_r FOR VALUES IN (2)
+Partition key: LIST (id)
+Indexes:
+    "fk_r_2_pkey" PRIMARY KEY, btree (id)
+Foreign-key constraints:
+    TABLE "fk_r" CONSTRAINT "fk_r_p_id_p_jd_fkey" FOREIGN KEY (p_id, p_jd) REFERENCES fk_p(id, jd)
+Number of partitions: 1 (Use \d+ to list them.)
+
+DELETE FROM fk_p;	-- fails
+ERROR:  update or delete on table "fk_p_2_2" violates foreign key constraint "fk_r_p_id_p_jd_fkey6" on table "fk_r"
+DETAIL:  Key (id, jd)=(2, 2) is still referenced from table "fk_r".
+-- these should all fail
+ALTER TABLE fk_r_1 DROP CONSTRAINT fk_r_p_id_p_jd_fkey;
+ERROR:  constraint "fk_r_p_id_p_jd_fkey" of relation "fk_r_1" does not exist
+ALTER TABLE fk_r DROP CONSTRAINT fk_r_p_id_p_jd_fkey1;
+ERROR:  cannot drop inherited constraint "fk_r_p_id_p_jd_fkey1" of relation "fk_r"
+ALTER TABLE fk_r_2 DROP CONSTRAINT fk_r_p_id_p_jd_fkey;
+ERROR:  cannot drop inherited constraint "fk_r_p_id_p_jd_fkey" of relation "fk_r_2"
+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
@@ -1886,3 +1886,59 @@ DELETE FROM fkpart9.pk WHERE a=35;
 SELECT * FROM fkpart9.pk;
 SELECT * FROM fkpart9.fk;
 DROP SCHEMA fkpart9 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, Postgres 14 and earlier remove the foreign key (newer
+-- versions make it a standalone constraint.)
+CREATE SCHEMA fkpart12
+  CREATE TABLE fk_p ( id int, jd int, PRIMARY KEY(id, jd)) PARTITION BY list (id)
+  CREATE TABLE fk_p_1 PARTITION OF fk_p FOR VALUES IN (1) PARTITION BY list (jd)
+  CREATE TABLE fk_p_1_1 PARTITION OF fk_p_1 FOR VALUES IN (1)
+  CREATE TABLE fk_p_1_2 PARTITION OF fk_p_1 FOR VALUES IN (2)
+  CREATE TABLE fk_p_2 PARTITION OF fk_p FOR VALUES IN (2) PARTITION BY list (jd)
+  CREATE TABLE fk_p_2_1 PARTITION OF fk_p_2 FOR VALUES IN (1)
+  CREATE TABLE fk_p_2_2 PARTITION OF fk_p_2 FOR VALUES IN (2)
+  CREATE TABLE fk_r_1 ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL)
+  CREATE TABLE fk_r_2 ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL) PARTITION BY list (id)
+  CREATE TABLE fk_r_2_1 PARTITION OF fk_r_2 FOR VALUES IN (2, 1)
+  CREATE TABLE fk_r   ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL,
+       FOREIGN KEY (p_id, p_jd) REFERENCES fk_p (id, jd)
+  ) PARTITION BY list (id);
+SET search_path TO fkpart12;
+
+INSERT INTO fk_p VALUES (1, 1);
+
+ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
+ALTER TABLE fk_r ATTACH PARTITION fk_r_2 FOR VALUES IN (2);
+
+\d fk_r_2
+
+INSERT INTO fk_r VALUES (1, 1, 1);
+INSERT INTO fk_r VALUES (2, 2, 1);	-- fails
+
+ALTER TABLE fk_r DETACH PARTITION fk_r_1;
+ALTER TABLE fk_r DETACH PARTITION fk_r_2;
+
+\d fk_r_2
+
+INSERT INTO fk_r_1 VALUES (2, 1, 2); -- works: there's no FK anymore
+DELETE FROM fk_p;	-- works
+
+ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);	-- fails
+
+INSERT INTO fk_r_2 VALUES (2, 2, 2);
+INSERT INTO fk_p VALUES (2, 2);
+ALTER TABLE fk_r ATTACH PARTITION fk_r_2 FOR VALUES IN (2);
+\d fk_r_2
+DELETE FROM fk_p;	-- fails
+
+-- these should all fail
+ALTER TABLE fk_r_1 DROP CONSTRAINT fk_r_p_id_p_jd_fkey;
+ALTER TABLE fk_r DROP CONSTRAINT fk_r_p_id_p_jd_fkey1;
+ALTER TABLE fk_r_2 DROP CONSTRAINT fk_r_p_id_p_jd_fkey;
+
+SET client_min_messages TO warning;
+DROP SCHEMA fkpart12 CASCADE;
+RESET client_min_messages;
+RESET search_path;
