Hi,
Please, find in attachment a small serie of patch:
0001 fix the constraint parenting bug. Not much to say. It's basically your
patch we discussed with some more comments and the check on contype equals to
either primary, unique or exclusion.
0002 fix the self-FK being cloned twice on partitions
0003 add a regression test validating both fix.
I should confess than even with these fix, I'm still wondering about this code
sanity as we could still end up with a PK on a partition being parented with a
simple unique constraint from the table, on a field not even NOT NULL:
DROP TABLE IF EXISTS parted_self_fk, part_with_pk;
CREATE TABLE parted_self_fk (
id bigint,
id_abc bigint,
FOREIGN KEY (id_abc) REFERENCES parted_self_fk(id),
UNIQUE (id)
)
PARTITION BY RANGE (id);
CREATE TABLE part_with_pk (
id bigint PRIMARY KEY,
id_abc bigint,
CHECK ((id >= 0 AND id < 10))
);
ALTER TABLE parted_self_fk ATTACH
PARTITION part_with_pk FOR VALUES FROM (0) TO (10);
SELECT cr.relname, co.conname, co.contype, p.conname AS conparentrelname
FROM pg_catalog.pg_constraint co
JOIN pg_catalog.pg_class cr ON cr.oid = co.conrelid
LEFT JOIN pg_catalog.pg_constraint p ON p.oid = co.conparentid
WHERE cr.relname IN ('parted_self_fk', 'part_with_pk')
AND co.contype IN ('u', 'p');
DROP TABLE parted_self_fk;
DROP TABLE
CREATE TABLE
CREATE TABLE
ALTER TABLE
relname | conname | contype | conparentrelname
----------------+-----------------------+---------+-----------------------
parted_self_fk | parted_self_fk_id_key | u |
part_with_pk | part_with_pk_pkey | p | parted_self_fk_id_key
(2 rows)
Nothing forbid the partition to have stricter constraints than the parent
table, but it feels weird, so it might worth noting it here.
I wonder if AttachPartitionEnsureConstraints() should exists and take care of
comparing/cloning constraints before calling AttachPartitionEnsureIndexes()
which would handle missing index without paying attention to related
constraints?
Regards,
On Wed, 24 Aug 2022 12:49:13 +0200
Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote:
> On 2022-Aug-24, Jehan-Guillaume de Rorthais wrote:
>
> > I was naively wondering about such a patch, but was worrying about potential
> > side effects on ATExecAttachPartitionIdx(), DetachPartitionFinalize() and
> > DefineIndex() where I didn't had a single glance. Did you had a look?
>
> No. But AFAIR all the code there is supposed to worry about unique
> constraints and PK only, not FKs. So if something changes, then most
> likely it was wrong to begin with.
>
> > I did a quick ATTACH + DETACH test, and it seems DETACH partly fails with
> > its housecleaning:
>
> Ugh. More fixes required, then.
>
> > Looking for few minutes in ATExecDetachPartitionFinalize(), it seems it only
> > support removing the parental link on FK, not to clean the FKs added during
> > the ATTACH DDL anyway. That explains the FK child1->parent left behind. But
> > in fact, this let me wonder if this part of the code ever considered
> > implication of self-FK during the ATTACH and DETACH process?
>
> No, or at least I don't remember thinking about self-referencing FKs.
> If there are no tests for it, then that's likely what happened.
>
> > Why in the first place TWO FK are created during the ATTACH DDL?
>
> That's probably a bug too.
>
>From 146f40285ee5f773f68205f1cbafe1cbec46c5c4 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..be26dbe81d 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 preliminary 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 8e12a08248affdf81caa99ea3e1cfa411c8323a5 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 8eef33154a7faea8aab7a1b423abb44e35fbfdac 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
