On Fri, 30 Sep 2022 16:11:09 -0700
Zhihong Yu <z...@yugabyte.com> wrote:
> On Fri, Sep 30, 2022 at 3:30 PM Jehan-Guillaume de Rorthais <j...@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,
>From 34ee3a3737e36d440824db3e8eb7c8f802a7276a Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <j...@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
>From 6c8d9595dc1b2b62e3cc5ce3bc1c970b6eedcbc2 Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <j...@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
>From a08ed953e3cd559ea1fec78301cb72e611f9387e Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <j...@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
