Subject: Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails

From: Alvaro Herrera <[email-masked]>
Date: Wed, 25 Oct 2023 14:12:53 +0200

On 2023-Oct-25, tender wang wrote:

> Hi
>    Is there any conclusion to this issue?

None yet.  I intend to work on this at some point, hopefully soon.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

---

From: tender wang <[email-masked]>
Date: Fri, 27 Oct 2023 17:05:49 +0800

Hi Alvaro,
I re-analyzed this issue, and here is my analysis process.
step 1: CREATE TABLE p ( id bigint PRIMARY KEY )
    PARTITION BY list (id);
 step2: CREATE TABLE p_1 PARTITION OF p FOR VALUES IN (1);
 step3: CREATE TABLE r_1 (
    id   bigint PRIMARY KEY,
    p_id bigint NOT NULL,
    FOREIGN KEY (p_id) REFERENCES p (id)
  );
After above step 3 operations, we have below catalog tuples:
postgres=# select oid, relname from pg_class where relname = 'p';
oid | relname
-------+---------
16384 | p
(1 row)
postgres=# select oid, relname from pg_class where relname = 'p_1';
oid | relname
-------+---------
16389 | p_1
(1 row)
postgres=# select oid, relname from pg_class where relname = 'r_1';
oid | relname
-------+---------
16394 | r_1
(1 row)
postgres=# select oid, conname,conrelid,conparentid,confrelid from
pg_constraint where conrelid = 16394;
oid | conname | conrelid | conparentid | confrelid
-------+-------------------+----------+-------------+-----------
16397 | r_1_p_id_not_null | 16394 | 0 | 0
16399 | r_1_pkey | 16394 | 0 | 0
16400 | r_1_p_id_fkey | 16394 | 0 | 16384
16403 | r_1_p_id_fkey1 | 16394 | 16400 | 16389
(4 rows)
postgres=# select oid, tgrelid, tgparentid,
tgconstrrelid,tgconstrindid,tgconstraint from pg_trigger where tgconstraint
= 16403;
oid | tgrelid | tgparentid | tgconstrrelid | tgconstrindid | tgconstraint
-------+---------+------------+---------------+---------------+--------------
16404 | 16389 | 16401 | 16394 | 16392 | 16403
16405 | 16389 | 16402 | 16394 | 16392 | 16403
(2 rows)
postgres=# select oid, tgrelid, tgparentid,
tgconstrrelid,tgconstrindid,tgconstraint from pg_trigger where tgconstraint
= 16400;
oid | tgrelid | tgparentid | tgconstrrelid | tgconstrindid | tgconstraint
-------+---------+------------+---------------+---------------+--------------
16401 | 16384 | 0 | 16394 | 16387 | 16400
16402 | 16384 | 0 | 16394 | 16387 | 16400
16406 | 16394 | 0 | 16384 | 16387 | 16400
16407 | 16394 | 0 | 16384 | 16387 | 16400
(4 rows)
Because table p is partitioned table and it has one child table p_1. So
when r_1 add foreign key constraint, according to addFkRecurseReferenced(),
each partition should have one pg_constraint row(e.g. r_1_p_id_fkey1).
After called addFkRecurseReferenced() in ATAddForeignKeyConstraint(),
addFkRecurseReferencing() will be called, in which
it will add INSERT check trigger and UPDATE check trigger for r_1_p_id_fkey
but not for r_1_p_id_fkey1.
So when detach r_1 from r, according to DetachPartitionFinalize(), the
inherited fks should unlink relationship from parent.
The created INSERT and UPDATE check triggers should unlink relationship
link fks. But just like I said above, the r_1_p_id_fkey1
actually doesn't have INSERT check trigger.

I slightly modified the previous patch，but I didn't add test case, because
I found another issue.
After done ALTER TABLE r ATTACH PARTITION r_1 FOR VALUES IN (1);
I run the oidjoins.sql and has warnings as belwo:
psql:/tender/postgres/src/test/regress/sql/oidjoins.sql:49: WARNING:  FK
VIOLATION IN pg_trigger({tgparentid}): ("(0,3)",16401)
psql:/tender/postgres/src/test/regress/sql/oidjoins.sql:49: WARNING:  FK
VIOLATION IN pg_trigger({tgparentid}): ("(0,4)",16402)
postgres=# select oid, tgrelid, tgparentid,
tgconstrrelid,tgconstrindid,tgconstraint from pg_trigger where oid >= 16384;
  oid  | tgrelid | tgparentid | tgconstrrelid | tgconstrindid |
tgconstraint
-------+---------+------------+---------------+---------------+--------------
 16404 |   16389 |      16401 |         16394 |         16392 |        16403
 16405 |   16389 |      16402 |         16394 |         16392 |        16403
 16415 |   16384 |          0 |         16408 |         16387 |        16414
 16416 |   16384 |          0 |         16408 |         16387 |        16414
 16418 |   16389 |      16415 |         16408 |         16392 |        16417
 16419 |   16389 |      16416 |         16408 |         16392 |        16417
 16420 |   16408 |          0 |         16384 |         16387 |        16414
 16421 |   16408 |          0 |         16384 |         16387 |        16414
 16406 |   16394 |      16420 |         16384 |         16387 |        16400
 16407 |   16394 |      16421 |         16384 |         16387 |        16400
(10 rows)
oid = 16401 and oid = 16402 has been deleted.
The two trigger tuples are deleted in tryAttachPartitionForeignKey called
by CloneFkReferencing.
/*
* Looks good!  Attach this constraint.  The action triggers in the new
* partition become redundant -- the parent table already has equivalent
* ones, and those will be able to reach the partition.  Remove the ones
* in the partition.  We identify them because they have our constraint
* OID, as well as being on the referenced rel.
*/
The attached patch can't fix above issue. I'm not sure about the impact of
this issue. Maybe redundant triggers no need removed.

But it surely make oidjoings.sql fail if I add test case into v2 patch, so
I don't add test case in v2 patch.
No test case is not good patch. I just share my idea about this issue. Hope
to get your reply.

---

From: Tender Wang <[email-masked]>
Date: Thu, 18 Jul 2024 14:34:43 +0800

Hi Alvaro,

Recently, Alexander reported the same issue on [1]. And before that,
another same issue was reported on [2].
So I try to re-work those  issues.  In my last email on this thread, I said
that
"
I slightly modified the previous patch，but I didn't add test case, because
I found another issue.
After done ALTER TABLE r ATTACH PARTITION r_1 FOR VALUES IN (1);
I run the oidjoins.sql and has warnings as belwo:
psql:/tender/postgres/src/test/regress/sql/oidjoins.sql:49: WARNING:  FK
VIOLATION IN pg_trigger({tgparentid}): ("(0,3)",16401)
psql:/tender/postgres/src/test/regress/sql/oidjoins.sql:49: WARNING:  FK
VIOLATION IN pg_trigger({tgparentid}): ("(0,4)",16402)
"

And I gave the explanation:
"
The two trigger tuples are deleted in tryAttachPartitionForeignKey called
by CloneFkReferencing.
/*
* Looks good!  Attach this constraint.  The action triggers in the new
* partition become redundant -- the parent table already has equivalent
* ones, and those will be able to reach the partition.  Remove the ones
* in the partition.  We identify them because they have our constraint
* OID, as well as being on the referenced rel.
*/
"
I try to fix above fk violation. I have two ideas.
i. Do not remove redundant, but when detaching parittion, the action
trigger on referenced side will be create again.
I have consider about this situation.

ii. We still remove redundant, and the remove the child action trigger,
too.  If we do this way.
Should we create action trigger recursively on referced side when detaching
partition.

I can't decide which one is better. And I'm not sure that keep this FK
VIOLATION will cause some problem.
I rebase and send v3 patch, which only fix NOT FOUND INSERT CHECK TRIGGER.

[1]
https://www.postgresql.org/message-id/18541-628a61bc267cd2d3%40postgresql.org
[2]
https://www.postgresql.org/message-id/GVAP278MB02787E7134FD691861635A8BC9032%40GVAP278MB0278.CHEP278.PROD.OUTLOOK.COM

-- 
Tender Wang

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 19 Jul 2024 15:18:28 +0200

Hello,

I think the fix for the check triggers should be as the attached.  Very
close to what you did, but you were skipping some operations that needed
to be kept.  AFAICS this patch works correctly for the posted cases.

I haven't looked at the action triggers yet; I think we need to create
one trigger for each partition of the referenced side, so we need to
loop instead of doing a single one.

I find this pair of queries useful; they show which constraints exist
and which triggers belong to each.  We need to make the constraints and
triggers match after a detach right as things would be if the
just-detached partition were an individual table having the same foreign
key.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"People get annoyed when you try to debug them."  (Larry Wall)
>From 925739ef0c6b64c73b021ed930f65f140004ceb5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email-masked]>
Date: Fri, 19 Jul 2024 15:12:38 +0200
Subject: [PATCH] Fix partition detach on tables with FKs to partitioned tables

---
 src/backend/commands/tablecmds.c | 49 +++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 721d24783b..ca777d3c88 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -19185,8 +19185,11 @@ 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			parentConstrOid;
 		Oid			insertTriggerOid,
 					updateTriggerOid;
 
@@ -19203,22 +19206,46 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 			continue;
 		}
 
+		parentConstrOid = conform->conparentid;
+
 		/* 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.
+		 * Search for the partition's check triggers that implement the
+		 * constraint being detached, and make them no longer children of the
+		 * triggers on the parent table.  However, if the referenced side is a
+		 * partitioned table, there are no such check triggers (we know that
+		 * the referenced side is partitioned because our constraint row points
+		 * to a partition, whereas our parent constraint points to its parent
+		 * partitioned table.)
 		 */
-		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));
+		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);
+
+		if (parentConForm->conrelid != conform->conrelid)
+		{
+			/*
+			 * Also, look up the partition's "check" triggers corresponding to the
+			 * constraint being detached and detach them from the parent triggers.
+			 */
+			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));
+		}
+
+		ReleaseSysCache(parentConTup);
 
 		/*
 		 * Make the action triggers on the referenced relation.  When this was
-- 
2.39.2

---

From: Tender Wang <[email-masked]>
Date: Mon, 22 Jul 2024 13:52:19 +0800

> to be kept.  AFAICS this patch works correctly for the posted cases.

After applying the attached, the r_1_p_id_fkey1 will have redundant action
triggers, as below:
postgres=# select oid, conname, contype, conrelid, conindid,conparentid,
confrelid,conislocal,coninhcount, connoinherit from pg_constraint where oid
= 16402;
  oid  |    conname     | contype | conrelid | conindid | conparentid |
confrelid | conislocal | coninhcount | connoinherit
-------+----------------+---------+----------+----------+-------------+-----------+------------+-------------+--------------
 16402 | r_1_p_id_fkey1 | f       |    16394 |    16392 |           0 |
16389 | t          |           0 | f
(1 row)

postgres=# select oid, tgrelid, tgparentid, tgconstrrelid, tgconstrindid,
tgconstraint from pg_trigger where tgconstraint = 16402;
  oid  | tgrelid | tgparentid | tgconstrrelid | tgconstrindid | tgconstraint
-------+---------+------------+---------------+---------------+--------------
 16403 |   16389 |      16400 |         16394 |         16392 |        16402
 16404 |   16389 |      16401 |         16394 |         16392 |        16402
 16422 |   16389 |          0 |         16394 |         16392 |        16402
 16423 |   16389 |          0 |         16394 |         16392 |        16402
(4 rows)

-- 
Tender Wang

---

From: Tender Wang <[email-masked]>
Date: Tue, 23 Jul 2024 10:15:47 +0800

> just-detached partition were an individual table having the same foreign

I don't find the useful queries in your last email. Can you provide them.
Thanks.

-- 
Tender Wang