Date: Mon, 3 Oct 2022 14:47:57 +0200
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Zhihong Yu <zyu@yugabyte.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK
 oddity when attaching a partition)
Message-ID: <20221003144757.1d2b86da@karst>
In-Reply-To: 
 <CALNJ-vSLdGJ+B=uZM1AQOgG6VSOoqctQkwCq2Vp_o3GqFgnqZw@mail.gmail.com>
References: <20220824122850.032f9639@karst>
	<20220824104913.hgildshd4w4l6cu4@alvherre.pgsql>
	<20221001003010.0ab8bdd0@karst>
	<CALNJ-vSLdGJ+B=uZM1AQOgG6VSOoqctQkwCq2Vp_o3GqFgnqZw@mail.gmail.com>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MP_/cUcF3dN2wElWL8VtIX_110R"
Archived-At: 
 <https://www.postgresql.org/message-id/20221003144757.1d2b86da%40karst>
Precedence: bulk

--MP_/cUcF3dN2wElWL8VtIX_110R
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

On Fri, 30 Sep 2022 16:11:09 -0700
Zhihong Yu <zyu@yugabyte.com> wrote:

> On Fri, Sep 30, 2022 at 3:30 PM Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
> wrote:
...
> 
> +        * Self-Foreign keys are ignored as the index was preliminary
> created
> 
> preliminary created -> primarily created

Thank you! This is fixed and rebased on current master branch in patches
attached.

Regards,

--MP_/cUcF3dN2wElWL8VtIX_110R
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=0001-v2-Fix-bug-between-constraint-parenting-and-self-FK.patch

From 34ee3a3737e36d440824db3e8eb7c8f802a7276a Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Date: Fri, 30 Sep 2022 23:42:21 +0200
Subject: [PATCH 1/3] Fix bug between constraint parenting and self-FK

Function get_relation_idx_constraint_oid() assumed an index can
only be associated to zero or one constraint for a given relation.

However, if the relation has a self-foreign key, the index could
be referenced as enforcing two different constraints on the same
relation: the PK and the FK.

This lead to a bug with partitionned table where the self-FK could
become the parent of a partition's PK.
---
 src/backend/catalog/pg_constraint.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index bb65fb1e0a..ba7a8ff965 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -985,8 +985,8 @@ get_relation_constraint_attnos(Oid relid, const char *conname,
 }
 
 /*
- * Return the OID of the constraint associated with the given index in the
- * given relation; or InvalidOid if no such index is catalogued.
+ * Return the OID of the original constraint enforced by the given index
+ * in the given relation; or InvalidOid if no such index is catalogued.
  */
 Oid
 get_relation_idx_constraint_oid(Oid relationId, Oid indexId)
@@ -1011,6 +1011,18 @@ get_relation_idx_constraint_oid(Oid relationId, Oid indexId)
 		Form_pg_constraint constrForm;
 
 		constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
+
+		/*
+		 * Self-Foreign keys are ignored as the index was primarily created
+		 * to enforce a PK or unique constraint in the first place.  This means
+		 * only primary key, unique constraint or an exlusion one can be
+		 * returned.
+		 */
+		if (constrForm->contype != CONSTRAINT_PRIMARY
+			&& constrForm->contype != CONSTRAINT_UNIQUE
+			&& constrForm->contype != CONSTRAINT_EXCLUSION)
+			continue;
+
 		if (constrForm->conindid == indexId)
 		{
 			constraintId = constrForm->oid;
-- 
2.37.3


--MP_/cUcF3dN2wElWL8VtIX_110R
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=0002-v2-Fix-bug-where-a-self-FK-was-cloned-twice-on-partitio.patch

From 6c8d9595dc1b2b62e3cc5ce3bc1c970b6eedcbc2 Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Date: Fri, 30 Sep 2022 23:54:04 +0200
Subject: [PATCH 2/3] Fix bug where a self-FK was cloned twice on partitions

A tbale with a self-foreign keys appears on both the referenced
and referencing side of the constraint.

Because of this, when creating or attaching a new partition to
a table, the self-FK was cloned twice, by CloneFkReferenced() and
CloneFkReferencing() functions.
---
 src/backend/commands/tablecmds.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7d8a75d23c..d19d917571 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9968,6 +9968,9 @@ CloneForeignKeyConstraints(List **wqueue, Relation parentRel,
  * clone those constraints to the given partition.  This is to be called
  * when the partition is being created or attached.
  *
+ * Note that this ignore self-referenced FK. Those are handled in
+ * CloneFkReferencing().
+ *
  * This recurses to partitions, if the relation being attached is partitioned.
  * Recursion is done by calling addFkRecurseReferenced.
  */
@@ -10047,10 +10050,14 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel)
 		constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
 
 		/*
-		 * As explained above: don't try to clone a constraint for which we're
-		 * going to clone the parent.
+		 * As explained above and in the function header:
+		 * - don't try to clone a constraint for which we're going to clone
+		 *   the parent.
+		 * - don't clone a self-referenced constraint here as it is handled
+		 *   on the CloneFkReferencing() side
 		 */
-		if (list_member_oid(clone, constrForm->conparentid))
+		if (list_member_oid(clone, constrForm->conparentid) ||
+			constrForm->conrelid == RelationGetRelid(parentRel))
 		{
 			ReleaseSysCache(tuple);
 			continue;
-- 
2.37.3


--MP_/cUcF3dN2wElWL8VtIX_110R
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=0003-v2-Add-regression-tests-about-self-FK-with-partitions.patch

From a08ed953e3cd559ea1fec78301cb72e611f9387e Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Date: Sat, 1 Oct 2022 00:00:28 +0200
Subject: [PATCH 3/3] Add regression tests about self-FK with partitions

---
 src/test/regress/expected/constraints.out | 37 +++++++++++++++++++++++
 src/test/regress/sql/constraints.sql      | 31 +++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index e6f6602d95..49aa659330 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -626,6 +626,43 @@ SELECT conname FROM pg_constraint WHERE conrelid = 'parted_fk_naming_1'::regclas
 (1 row)
 
 DROP TABLE parted_fk_naming;
+--
+-- Test self-referencing foreign key with partition.
+-- This should create only one fk constraint per partition
+--
+CREATE TABLE parted_self_fk (
+    id bigint NOT NULL PRIMARY KEY,
+    id_abc bigint,
+    FOREIGN KEY (id_abc) REFERENCES parted_self_fk(id)
+)
+PARTITION BY RANGE (id);
+-- test with an existing table attached
+CREATE TABLE part1_self_fk (
+    id bigint NOT NULL PRIMARY KEY,
+    id_abc bigint
+);
+ALTER TABLE parted_self_fk ATTACH PARTITION part1_self_fk FOR VALUES FROM (0) TO (10);
+-- test with a newly created partition
+CREATE TABLE part2_self_fk PARTITION OF parted_self_fk FOR VALUES FROM (10) TO (20);
+SELECT cr.relname, co.conname, co.contype, p.conname AS conparentrelname,
+       cf.relname AS conforeignrelname
+FROM pg_catalog.pg_constraint co
+JOIN pg_catalog.pg_class cr ON cr.oid = co.conrelid
+LEFT JOIN pg_catalog.pg_class cf ON cf.oid = co.confrelid
+LEFT JOIN pg_catalog.pg_constraint p ON p.oid = co.conparentid
+WHERE cr.relname IN ('parted_self_fk', 'part1_self_fk', 'part2_self_fk')
+ORDER BY cr.relname, co.conname, p.conname;
+    relname     |          conname           | contype |      conparentrelname      | conforeignrelname 
+----------------+----------------------------+---------+----------------------------+-------------------
+ part1_self_fk  | part1_self_fk_pkey         | p       | parted_self_fk_pkey        | 
+ part1_self_fk  | parted_self_fk_id_abc_fkey | f       | parted_self_fk_id_abc_fkey | parted_self_fk
+ part2_self_fk  | part2_self_fk_pkey         | p       | parted_self_fk_pkey        | 
+ part2_self_fk  | parted_self_fk_id_abc_fkey | f       | parted_self_fk_id_abc_fkey | parted_self_fk
+ parted_self_fk | parted_self_fk_id_abc_fkey | f       |                            | parted_self_fk
+ parted_self_fk | parted_self_fk_pkey        | p       |                            | 
+(6 rows)
+
+DROP TABLE parted_self_fk;
 -- test a HOT update that invalidates the conflicting tuple.
 -- the trigger should still fire and catch the violation
 BEGIN;
diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql
index 5ffcd4ffc7..bdfc9f1455 100644
--- a/src/test/regress/sql/constraints.sql
+++ b/src/test/regress/sql/constraints.sql
@@ -449,6 +449,37 @@ ALTER TABLE parted_fk_naming ATTACH PARTITION parted_fk_naming_1 FOR VALUES IN (
 SELECT conname FROM pg_constraint WHERE conrelid = 'parted_fk_naming_1'::regclass AND contype = 'f';
 DROP TABLE parted_fk_naming;
 
+--
+-- Test self-referencing foreign key with partition.
+-- This should create only one fk constraint per partition
+--
+CREATE TABLE parted_self_fk (
+    id bigint NOT NULL PRIMARY KEY,
+    id_abc bigint,
+    FOREIGN KEY (id_abc) REFERENCES parted_self_fk(id)
+)
+PARTITION BY RANGE (id);
+
+-- test with an existing table attached
+CREATE TABLE part1_self_fk (
+    id bigint NOT NULL PRIMARY KEY,
+    id_abc bigint
+);
+ALTER TABLE parted_self_fk ATTACH PARTITION part1_self_fk FOR VALUES FROM (0) TO (10);
+
+-- test with a newly created partition
+CREATE TABLE part2_self_fk PARTITION OF parted_self_fk FOR VALUES FROM (10) TO (20);
+
+SELECT cr.relname, co.conname, co.contype, p.conname AS conparentrelname,
+       cf.relname AS conforeignrelname
+FROM pg_catalog.pg_constraint co
+JOIN pg_catalog.pg_class cr ON cr.oid = co.conrelid
+LEFT JOIN pg_catalog.pg_class cf ON cf.oid = co.confrelid
+LEFT JOIN pg_catalog.pg_constraint p ON p.oid = co.conparentid
+WHERE cr.relname IN ('parted_self_fk', 'part1_self_fk', 'part2_self_fk')
+ORDER BY cr.relname, co.conname, p.conname;
+
+DROP TABLE parted_self_fk;
 -- test a HOT update that invalidates the conflicting tuple.
 -- the trigger should still fire and catch the violation
 
-- 
2.37.3


--MP_/cUcF3dN2wElWL8VtIX_110R--