On 2025-Jan-20, Sami Imseih wrote: > The patch that Amul and I wrote both achieve the same result. > The approach that Amul took builds a list of constraint OIDs, > which could grow with the number of partitions and foreign keys > on those partitions. Maybe not a big deal?
Nope, not a big deal. It would be a big deal if we were talking about 268 million partitions (>1GB palloc size), but that's impractical for other reasons. > In my suggestion [1], I just do one extra pg_constraint lookup > to determine If the relation on the parent and child constraint match, > and in that case we can skip the rest of the work to cut the > link as it's not needed. A pg_constraint lookup is going to be a lot slower. I attach Amul's patch again with the comments I added and your test case. Needs adaptation for backpatching to 16 and 15. Thanks! -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
>From 1f63f9a5e6f98d5f40d3178c9bd1fb7959999ad6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvhe...@alvh.no-ip.org> Date: Mon, 20 Jan 2025 16:52:25 +0100 Subject: [PATCH] Fix detach of a partition that has a toplevel FK to a partitioned table In common cases, foreign keys are defined on the toplevel partitioned table; but if instead one is defined on a partition and references a partitioned table, and the referencing partition is detached, we would examine the pg_constraint row on the partition being detached, and fail to realize that the sub-constraints must be left alone. This causes the ALTER TABLE DETACH process to fail with ERROR: could not find ON INSERT check triggers of foreign key constraint NNN This is similar but not quite the same as what was fixed by 53af9491a043. This bug doesn't affect branches earlier than 15, because the detach procedure was different there, so we only backpatch down to 15. Fix by skipping such modifying constraints that are children of other constraints being detached. Author: Amul Sul <sula...@gmail.com> Diagnosys-by: Sami Imseih <samims...@gmail.com> Discussion: https://postgr.es/m/caaj_b97guph6wqpbxqs-zpy16oh+0amv-w64qcgrlhcozz6...@mail.gmail.com --- src/backend/commands/tablecmds.c | 26 +++++++++++++++++++++-- src/test/regress/expected/foreign_key.out | 7 ++++++ src/test/regress/sql/foreign_key.sql | 8 +++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d2420a9558c..016e9132dc4 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -19985,6 +19985,7 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, HeapTuple tuple, newtuple; Relation trigrel = NULL; + List *fkoids = NIL; if (concurrent) { @@ -20005,6 +20006,23 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, fks = copyObject(RelationGetFKeyList(partRel)); if (fks != NIL) trigrel = table_open(TriggerRelationId, RowExclusiveLock); + + /* + * It's possible that the partition being detached has a foreign key that + * references a partitioned table. When that happens, there are multiple + * pg_constraint rows for the partition: one points to the partitioned + * table itself, while the others point to each of its partitions. Only + * the topmost one is to be considered here; the child constraints must be + * left alone, because conceptually those aren't coming from our parent + * partitioned table, but from this partition itself. + * + * We implement this by collecting all the constraint OIDs in a first scan + * of the FK array, and skipping in the loop below those constraints whose + * parents are listed here. + */ + foreach_node(ForeignKeyCacheInfo, fk, fks) + fkoids = lappend_oid(fkoids, fk->conoid); + foreach(cell, fks) { ForeignKeyCacheInfo *fk = lfirst(cell); @@ -20018,9 +20036,13 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, elog(ERROR, "cache lookup failed for constraint %u", fk->conoid); conform = (Form_pg_constraint) GETSTRUCT(contup); - /* consider only the inherited foreign keys */ + /* + * Consider only inherited foreign keys, and only if their parents + * aren't in the list. + */ if (conform->contype != CONSTRAINT_FOREIGN || - !OidIsValid(conform->conparentid)) + !OidIsValid(conform->conparentid) || + list_member_oid(fkoids, conform->conparentid)) { ReleaseSysCache(contup); continue; diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 3f459f70ac1..72f7fc84c71 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -2351,6 +2351,13 @@ UPDATE pk SET a = 4002 WHERE a = 4000; DELETE FROM pk WHERE a = 4002; UPDATE pk SET a = 4502 WHERE a = 4500; DELETE FROM pk WHERE a = 4502; +-- Also, detaching a partition that has the FK itself should work +-- https://postgr.es/m/caaj_b97guph6wqpbxqs-zpy16oh+0amv-w64qcgrlhcozz6...@mail.gmail.com +CREATE TABLE ffk (a int) PARTITION BY list (a); +CREATE TABLE ffk1 PARTITION OF ffk FOR VALUES IN (1); +ALTER TABLE ffk1 ADD FOREIGN KEY (a) REFERENCES pk; +ALTER TABLE ffk DETACH PARTITION ffk1; +DROP TABLE ffk, ffk1; CREATE SCHEMA fkpart4; SET search_path TO fkpart4; -- dropping/detaching PARTITIONs is prevented if that would break diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 2e710e419c2..6f33346a35a 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1672,6 +1672,14 @@ DELETE FROM pk WHERE a = 4002; UPDATE pk SET a = 4502 WHERE a = 4500; DELETE FROM pk WHERE a = 4502; +-- Also, detaching a partition that has the FK itself should work +-- https://postgr.es/m/caaj_b97guph6wqpbxqs-zpy16oh+0amv-w64qcgrlhcozz6...@mail.gmail.com +CREATE TABLE ffk (a int) PARTITION BY list (a); +CREATE TABLE ffk1 PARTITION OF ffk FOR VALUES IN (1); +ALTER TABLE ffk1 ADD FOREIGN KEY (a) REFERENCES pk; +ALTER TABLE ffk DETACH PARTITION ffk1; +DROP TABLE ffk, ffk1; + CREATE SCHEMA fkpart4; SET search_path TO fkpart4; -- dropping/detaching PARTITIONs is prevented if that would break -- 2.39.5
