From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Zhihong Yu <zyu@yugabyte.com>
To: 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)
Date: Mon, 3 Oct 2022 14:47:57 +0200
Message-ID: <20221003144757.1d2b86da@karst> (raw)
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>
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,
Attachments:download all (3 files, .tar.gz)0001-v2-Fix-bug-between-constraint-parenting-and-self-FK.patch (text/x-patch, 2.0K) download
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.cindex 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
0002-v2-Fix-bug-where-a-self-FK-was-cloned-twice-on-partitio.patch (text/x-patch, 2.0K) download
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.cindex 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
0003-v2-Add-regression-tests-about-self-FK-with-partitions.patch (text/x-patch, 4.4K) download
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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: jgdr@dalibo.com, zyu@yugabyte.com, alvherre@alvh.no-ip.org
Subject: Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
In-Reply-To: <20221003144757.1d2b86da@karst>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox