Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On Thu, 3 Nov 2022 20:44:16 +0100 Alvaro Herrera wrote: > On 2022-Oct-05, Alvaro Herrera wrote: > > > I've been giving the patches a look and it caused me to notice two > > additional bugs in the same area: > > > > - FKs in partitions are sometimes marked NOT VALID. This is because of > > missing initialization when faking up a Constraint node in > > CloneFkReferencing. Easy to fix, have patch, running tests now. > > I have pushed the fix for this now. Thank you Alvaro!
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On 2022-Oct-05, Alvaro Herrera wrote: > I've been giving the patches a look and it caused me to notice two > additional bugs in the same area: > > - FKs in partitions are sometimes marked NOT VALID. This is because of > missing initialization when faking up a Constraint node in > CloneFkReferencing. Easy to fix, have patch, running tests now. I have pushed the fix for this now. > - The feature added by d6f96ed94e73 (ON DELETE SET NULL (...)) is not > correctly propagated. This should be an easy fix also, haven't tried, > need to add a test case. There was no bug here actually: it's true that the struct member is left uninitialized, but in practice that doesn't matter, because the set of columns is propagated separately from the node. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Las navajas y los monos deben estar siempre distantes" (Germán Poo)
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On 2022-Oct-05, Alvaro Herrera wrote: > Backpatching this to 12 shows yet another problem -- the topmost > relation acquires additional FK constraints, not yet sure why. I think > we must have fixed something in 13 that wasn't backpatched, but I can't > remember what it is and whether it was intentionally not backpatched. This was actually a mismerge. Once I fixed that, it worked properly. However, there was another bug, which only showed up when I did a DETACH, ATTACH, and repeat. The problem is that when we detach, the no-longer-partition retains an FK constraint to the partitioned table. This is good -- we want that one -- but when we reattach, then we see that the partitioned table is being referenced from outside, so we consider that another constraint that we need to add the partition to, *in addition to the constraint that we need to clone*. So we need to ignore both a self-referencing FK that goes to the partitioned table, as well as a self-referencing one that comes from the partition-to-be. When we do that, then the clone correctly uses that one as the constraint to retain and attach into the hierarchy of constraints, and everything [appears to] work correctly. So I've pushed this, and things are now mostly good. Two problems remain, though I don't think either of them is terribly serious: 1. one of the constraints in the final hierarchy is marked as not validated. I mentioned this before. 2. (only in 15) There are useless pg_depend rows for the pg_trigger rows, which make them depend on their parent pg_trigger rows. This is not related to self-referencing foreign keys, but I just happened to notice because I was examining the catalog contents with the added test case. I think this breakage is due to f4566345cf40. I couldn't find any actual misbehavior caused by these extra pg_depend entries, but we should not be creating them anyway. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Por suerte hoy explotó el califont porque si no me habría muerto de aburrido" (Papelucho)
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
Backpatching this to 12 shows yet another problem -- the topmost relation acquires additional FK constraints, not yet sure why. I think we must have fixed something in 13 that wasn't backpatched, but I can't remember what it is and whether it was intentionally not backpatched. Looking ... -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "I can see support will not be a problem. 10 out of 10."(Simon Wittber) (http://archives.postgresql.org/pgsql-general/2004-12/msg00159.php)
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On 2022-Oct-03, Jehan-Guillaume de Rorthais wrote: > Thank you! This is fixed and rebased on current master branch in patches > attached. Thanks. As far as I can see this fixes the bugs that were reported. I've been giving the patches a look and it caused me to notice two additional bugs in the same area: - FKs in partitions are sometimes marked NOT VALID. This is because of missing initialization when faking up a Constraint node in CloneFkReferencing. Easy to fix, have patch, running tests now. - The feature added by d6f96ed94e73 (ON DELETE SET NULL (...)) is not correctly propagated. This should be an easy fix also, haven't tried, need to add a test case. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "La primera ley de las demostraciones en vivo es: no trate de usar el sistema. Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On Fri, 30 Sep 2022 16:11:09 -0700
Zhihong Yu wrote:
> On Fri, Sep 30, 2022 at 3:30 PM Jehan-Guillaume de Rorthais
> 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
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
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
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
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On Fri, Sep 30, 2022 at 3:30 PM Jehan-Guillaume de Rorthais
wrote:
> 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 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.
> >
>
> Hi,
+* Self-Foreign keys are ignored as the index was preliminary
created
preliminary created -> primarily created
Cheers
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
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 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
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);
+
+ /*
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
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. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "The eagle never lost so much time, as when he submitted to learn of the crow." (William Blake)
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On Tue, 23 Aug 2022 18:30:06 +0200
Alvaro Herrera wrote:
> On 2022-Aug-23, Jehan-Guillaume de Rorthais wrote:
>
> Hi,
>
> [...]
>
> > However, it seems get_relation_idx_constraint_oid(), introduced in
> > eb7ed3f3063, assume there could be only ONE constraint depending to an
> > index. But in fact, multiple constraints can rely on the same index, eg.:
> > the PK and a self referencing FK. In consequence, when looking for a
> > constraint depending on an index for the given relation, either the FK or a
> > PK can appears first depending on various conditions. It is then possible
> > to trick it make a FK constraint a parent of a PK...
>
> Hmm, wow, that sounds extremely stupid. I think a sufficient fix might
> be to have get_relation_idx_constraint_oid ignore any constraints that
> are not unique or primary keys. I tried your scenario with the attached
> and it seems to work correctly. Can you confirm? (I only ran the
> pg_regress tests, not anything else for now.)
>
> If this is OK, we should make this API quirkiness very explicit in the
> comments, so the patch needs to be a few lines larger in order to be
> committable. Also, perhaps the check should be that contype equals
> either primary or unique, rather than it doesn't equal foreign.
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?
I did a quick ATTACH + DETACH test, and it seems DETACH partly fails with its
housecleaning:
DROP TABLE IF EXISTS parent, child1;
CREATE TABLE parent (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
FOREIGN KEY (id_abc, no_part) REFERENCES parent(id, no_part)
ON UPDATE RESTRICT ON DELETE RESTRICT,
PRIMARY KEY (id, no_part)
)
PARTITION BY LIST (no_part);
CREATE TABLE child1 (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
PRIMARY KEY (id, no_part),
CONSTRAINT child1 CHECK ((no_part = 1))
);
\C 'Before ATTACH'
SELECT oid, conname, conparentid, conrelid, confrelid
FROM pg_constraint
WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
ORDER BY 1;
ALTER TABLE parent ATTACH PARTITION child1 FOR VALUES IN ('1');
\C 'After ATTACH'
SELECT oid, conname, conparentid, conrelid, confrelid
FROM pg_constraint
WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
ORDER BY 1;
ALTER TABLE parent DETACH PARTITION child1;
\C 'After DETACH'
SELECT oid, conname, conparentid, conrelid, confrelid
FROM pg_constraint
WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
ORDER BY 1;
Before ATTACH
oid | conname | conparentid | conrelid | confrelid
---++-+--+---
24711 | parent_pkey| 0 |24706 | 0
24712 | parent_id_abc_no_part_fkey | 0 |24706 | 24706
24721 | child1 | 0 |24717 | 0
24723 | child1_pkey| 0 |24717 | 0
(4 rows)
After ATTACH
oid | conname | conparentid | conrelid | confrelid
---+-+-+--+---
24711 | parent_pkey | 0 |24706 | 0
24712 | parent_id_abc_no_part_fkey | 0 |24706 | 24706
24721 | child1 | 0 |24717 | 0
24723 | child1_pkey | 24711 |24717 | 0
24724 | parent_id_abc_no_part_fkey1 | 24712 |24706 | 24717
24727 | parent_id_abc_no_part_fkey | 24712 |24717 | 24706
(6 rows)
After DETACH
oid | conname | conparentid | conrelid | confrelid
---++-+--+---
24711 | parent_pkey| 0 |24706 | 0
24712 | parent_id_abc_no_part_fkey | 0 |24706 | 24706
24721 | child1 | 0 |24717 | 0
24723 | child1_pkey| 0 |24717 | 0
24727 | parent_id_abc_no_part_fkey | 0 |24717 | 24706
(5 rows)
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? Why in the first place TWO FK
are created during the ATTACH DDL?
Regards,
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
Alvaro Herrera writes: > If this is OK, we should make this API quirkiness very explicit in the > comments, so the patch needs to be a few lines larger in order to be > committable. Also, perhaps the check should be that contype equals > either primary or unique, rather than it doesn't equal foreign. Yeah. See lsyscache.c's get_constraint_index(), as well as commit 641f3dffc which fixed a mighty similar-seeming bug. One question that precedent raises is whether to also include CONSTRAINT_EXCLUSION. But in any case a positive test for the constraint types to allow seems best. regards, tom lane
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On 2022-Aug-23, Zhihong Yu wrote: > A bigger question I have, even with the additional filtering, is what if > there are multiple constraints ? > How do we decide which unique / primary key constraint to return ? > > Looks like there is no known SQL statements leading to such state, but > should we consider such possibility ? I don't think we care, but feel free to experiment and report any problems. You should be able to have multiple UNIQUE constraints on the same column, for example. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Postgres is bloatware by design: it was built to house PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On Tue, Aug 23, 2022 at 9:47 AM Alvaro Herrera
wrote:
> On 2022-Aug-23, Zhihong Yu wrote:
>
> > I was thinking of the following patch.
> > Basically, if there is only one matching constraint. we still return it.
> >
> > diff --git a/src/postgres/src/backend/catalog/pg_constraint.c
> > b/src/postgres/src/backend/catalog/pg_constraint.c
> > index f0726e9aa0..ddade138b4 100644
> > --- a/src/postgres/src/backend/catalog/pg_constraint.c
> > +++ b/src/postgres/src/backend/catalog/pg_constraint.c
> > @@ -1003,7 +1003,8 @@ get_relation_idx_constraint_oid(Oid relationId, Oid
> > indexId)
> > constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
> > if (constrForm->conindid == indexId)
> > {
> > - constraintId = HeapTupleGetOid(tuple);
> > + if (constraintId == InvalidOid || constrForm->confrelid == 0)
> > + constraintId = HeapTupleGetOid(tuple);
> > break;
> > }
> > }
>
> We could do this, but what do we gain by doing so? It seems to me that
> my proposed formulation achieves the same and is less fuzzy about what
> the returned constraint is. Please try to write a code comment that
> explains what this does and see if it makes sense.
>
> For my proposal, it would be "return the OID of a primary key or unique
> constraint associated with the given index in the given relation, or OID
> if no such index is catalogued". This definition is clearly useful for
> partitioned tables, on which the unique and primary key constraints are
> useful elements. There's nothing that cares about foreign keys.
>
> --
> Álvaro Herrera PostgreSQL Developer —
> https://www.EnterpriseDB.com/
> "La virtud es el justo medio entre dos defectos" (Aristóteles)
>
A bigger question I have, even with the additional filtering, is what if
there are multiple constraints ?
How do we decide which unique / primary key constraint to return ?
Looks like there is no known SQL statements leading to such state, but
should we consider such possibility ?
Cheers
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On 2022-Aug-23, Zhihong Yu wrote:
> I was thinking of the following patch.
> Basically, if there is only one matching constraint. we still return it.
>
> diff --git a/src/postgres/src/backend/catalog/pg_constraint.c
> b/src/postgres/src/backend/catalog/pg_constraint.c
> index f0726e9aa0..ddade138b4 100644
> --- a/src/postgres/src/backend/catalog/pg_constraint.c
> +++ b/src/postgres/src/backend/catalog/pg_constraint.c
> @@ -1003,7 +1003,8 @@ get_relation_idx_constraint_oid(Oid relationId, Oid
> indexId)
> constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
> if (constrForm->conindid == indexId)
> {
> - constraintId = HeapTupleGetOid(tuple);
> + if (constraintId == InvalidOid || constrForm->confrelid == 0)
> + constraintId = HeapTupleGetOid(tuple);
> break;
> }
> }
We could do this, but what do we gain by doing so? It seems to me that
my proposed formulation achieves the same and is less fuzzy about what
the returned constraint is. Please try to write a code comment that
explains what this does and see if it makes sense.
For my proposal, it would be "return the OID of a primary key or unique
constraint associated with the given index in the given relation, or OID
if no such index is catalogued". This definition is clearly useful for
partitioned tables, on which the unique and primary key constraints are
useful elements. There's nothing that cares about foreign keys.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On Tue, Aug 23, 2022 at 9:30 AM Alvaro Herrera
wrote:
> On 2022-Aug-23, Jehan-Guillaume de Rorthais wrote:
>
> Hi,
>
> [...]
>
> > However, it seems get_relation_idx_constraint_oid(), introduced in
> eb7ed3f3063,
> > assume there could be only ONE constraint depending to an index. But in
> fact,
> > multiple constraints can rely on the same index, eg.: the PK and a self
> > referencing FK. In consequence, when looking for a constraint depending
> on an
> > index for the given relation, either the FK or a PK can appears first
> depending
> > on various conditions. It is then possible to trick it make a FK
> constraint a
> > parent of a PK...
>
> Hmm, wow, that sounds extremely stupid. I think a sufficient fix might
> be to have get_relation_idx_constraint_oid ignore any constraints that
> are not unique or primary keys. I tried your scenario with the attached
> and it seems to work correctly. Can you confirm? (I only ran the
> pg_regress tests, not anything else for now.)
>
> If this is OK, we should make this API quirkiness very explicit in the
> comments, so the patch needs to be a few lines larger in order to be
> committable. Also, perhaps the check should be that contype equals
> either primary or unique, rather than it doesn't equal foreign.
>
> --
> Álvaro Herrera 48°01'N 7°57'E —
> https://www.EnterpriseDB.com/
I was thinking of the following patch.
Basically, if there is only one matching constraint. we still return it.
diff --git a/src/postgres/src/backend/catalog/pg_constraint.c
b/src/postgres/src/backend/catalog/pg_constraint.c
index f0726e9aa0..ddade138b4 100644
--- a/src/postgres/src/backend/catalog/pg_constraint.c
+++ b/src/postgres/src/backend/catalog/pg_constraint.c
@@ -1003,7 +1003,8 @@ get_relation_idx_constraint_oid(Oid relationId, Oid
indexId)
constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
if (constrForm->conindid == indexId)
{
- constraintId = HeapTupleGetOid(tuple);
+ if (constraintId == InvalidOid || constrForm->confrelid == 0)
+ constraintId = HeapTupleGetOid(tuple);
break;
}
}
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On 2022-Aug-23, Jehan-Guillaume de Rorthais wrote:
Hi,
[...]
> However, it seems get_relation_idx_constraint_oid(), introduced in
> eb7ed3f3063,
> assume there could be only ONE constraint depending to an index. But in fact,
> multiple constraints can rely on the same index, eg.: the PK and a self
> referencing FK. In consequence, when looking for a constraint depending on an
> index for the given relation, either the FK or a PK can appears first
> depending
> on various conditions. It is then possible to trick it make a FK constraint a
> parent of a PK...
Hmm, wow, that sounds extremely stupid. I think a sufficient fix might
be to have get_relation_idx_constraint_oid ignore any constraints that
are not unique or primary keys. I tried your scenario with the attached
and it seems to work correctly. Can you confirm? (I only ran the
pg_regress tests, not anything else for now.)
If this is OK, we should make this API quirkiness very explicit in the
comments, so the patch needs to be a few lines larger in order to be
committable. Also, perhaps the check should be that contype equals
either primary or unique, rather than it doesn't equal foreign.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
>From a39299db23f4b2495b3ba10e7adb1121f0271694 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 23 Aug 2022 18:17:24 +0200
Subject: [PATCH] test fix
---
src/backend/catalog/pg_constraint.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index bb65fb1e0a..71b6fb35e6 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -1011,6 +1011,14 @@ get_relation_idx_constraint_oid(Oid relationId, Oid indexId)
Form_pg_constraint constrForm;
constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
+
+ /*
+ * Foreign key constraints are ignored for these purposes,
+ * because ...?
+ */
+ if (constrForm->contype == CONSTRAINT_FOREIGN)
+ continue;
+
if (constrForm->conindid == indexId)
{
constraintId = constrForm->oid;
--
2.30.2
Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
On Tue, Aug 23, 2022 at 8:07 AM Jehan-Guillaume de Rorthais
wrote:
> Hi all,
>
> I've been able to work on this issue and isolate where in the code the
> oddity
> is laying.
>
> During ATExecAttachPartition(), AttachPartitionEnsureIndexes() look for
> existing
> required index on the partition to attach. It creates missing index, or
> sets the
> parent's index when a matching one exists on the partition. Good.
>
> When a matching index is found, if the parent index enforce a constraint,
> the
> function look for the similar constraint in the partition-to-be, and set
> the
> constraint parent as well:
>
> constraintOid =
> get_relation_idx_constraint_oid(RelationGetRelid(rel),
> idx);
>
> [...]
>
> /*
> * If this index is being created in the parent because of a
> * constraint, then the child needs to have a constraint also,
> * so look for one. If there is no such constraint, this
> * index is no good, so keep looking.
> */
> if (OidIsValid(constraintOid))
> {
> cldConstrOid = get_relation_idx_constraint_oid(
> RelationGetRelid(attachrel),
> cldIdxId);
> /* no dice */
> if (!OidIsValid(cldConstrOid))
> continue;
> }
> /* bingo. */
> IndexSetParentIndex(attachrelIdxRels[i], idx);
> if (OidIsValid(constraintOid))
> ConstraintSetParentConstraint(cldConstrOid, constraintOid,
> RelationGetRelid(attachrel));
>
> However, it seems get_relation_idx_constraint_oid(), introduced in
> eb7ed3f3063,
> assume there could be only ONE constraint depending to an index. But in
> fact,
> multiple constraints can rely on the same index, eg.: the PK and a self
> referencing FK. In consequence, when looking for a constraint depending on
> an
> index for the given relation, either the FK or a PK can appears first
> depending
> on various conditions. It is then possible to trick it make a FK
> constraint a
> parent of a PK...
>
> In the following little scenario, when looking at the constraint linked to
> the PK unique index using the same index than
> get_relation_idx_constraint_oid
> use, this is the self-FK that is actually returned first by
> get_relation_idx_constraint_oid(), NOT the PK:
>
> postgres=# DROP TABLE IF EXISTS parent, child1;
>
> CREATE TABLE parent (
> id bigint NOT NULL default 1,
> no_part smallint NOT NULL,
> id_abc bigint,
> FOREIGN KEY (id_abc, no_part) REFERENCES parent(id, no_part)
> ON UPDATE RESTRICT ON DELETE RESTRICT,
> PRIMARY KEY (id, no_part)
> )
> PARTITION BY LIST (no_part);
>
> CREATE TABLE child1 (
> id bigint NOT NULL default 1,
> no_part smallint NOT NULL,
> id_abc bigint,
> PRIMARY KEY (id, no_part),
> CONSTRAINT child1 CHECK ((no_part = 1))
> );
>
> -- force an indexscan as get_relation_idx_constraint_oid() use the
> unique
> -- index on (conrelid, contypid, conname) to scan pg_cosntraint
> set enable_seqscan TO off;
> set enable_bitmapscan TO off;
>
> SELECT conname
> FROM pg_constraint
> WHERE conrelid = 'parent'::regclass <=== parent
> AND conindid = 'parent_pkey'::regclass; <=== PK index
>
> DROP TABLE
> CREATE TABLE
> CREATE TABLE
> SET
> SET
> conname
>
>parent_id_abc_no_part_fkey < WOOPS!
>parent_pkey
> (2 rows)
>
> In consequence, when attaching the partition, the PK of child1 is not
> marked as
> partition of the parent's PK, which is wrong. WORST, the PK of child1 is
> actually unexpectedly marked as a partition of the parent's **self-FK**:
>
> postgres=# ALTER TABLE ONLY parent ATTACH PARTITION child1
> FOR VALUES IN ('1');
>
> SELECT oid, conname, conparentid, conrelid, confrelid
> FROM pg_constraint
> WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
> ORDER BY 1;
>
> ALTER TABLE
> oid | conname | conparentid | conrelid |
> confrelid
>
> ---+-+-+--+---
>16700 | parent_pkey | 0 |16695 | 0
>16701 | parent_id_abc_no_part_fkey | 0 |16695 | 16695
>16706 | child1 | 0 |16702 | 0
>16708 | **child1_pkey** | **16701** |16702 | 0
>16709 | parent_id_abc_no_part_fkey1 | 16701 |16695 | 16702
>16712 | parent_id_abc_no_part_fkey | 16701 |16702 | 16695
> (6 rows)
>
> The expected result should probably be something like:
>
> oid | conname | conparentid | conrelid
[BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)
Hi all,
I've been able to work on this issue and isolate where in the code the oddity
is laying.
During ATExecAttachPartition(), AttachPartitionEnsureIndexes() look for existing
required index on the partition to attach. It creates missing index, or sets the
parent's index when a matching one exists on the partition. Good.
When a matching index is found, if the parent index enforce a constraint, the
function look for the similar constraint in the partition-to-be, and set the
constraint parent as well:
constraintOid = get_relation_idx_constraint_oid(RelationGetRelid(rel),
idx);
[...]
/*
* If this index is being created in the parent because of a
* constraint, then the child needs to have a constraint also,
* so look for one. If there is no such constraint, this
* index is no good, so keep looking.
*/
if (OidIsValid(constraintOid))
{
cldConstrOid = get_relation_idx_constraint_oid(
RelationGetRelid(attachrel),
cldIdxId);
/* no dice */
if (!OidIsValid(cldConstrOid))
continue;
}
/* bingo. */
IndexSetParentIndex(attachrelIdxRels[i], idx);
if (OidIsValid(constraintOid))
ConstraintSetParentConstraint(cldConstrOid, constraintOid,
RelationGetRelid(attachrel));
However, it seems get_relation_idx_constraint_oid(), introduced in eb7ed3f3063,
assume there could be only ONE constraint depending to an index. But in fact,
multiple constraints can rely on the same index, eg.: the PK and a self
referencing FK. In consequence, when looking for a constraint depending on an
index for the given relation, either the FK or a PK can appears first depending
on various conditions. It is then possible to trick it make a FK constraint a
parent of a PK...
In the following little scenario, when looking at the constraint linked to
the PK unique index using the same index than get_relation_idx_constraint_oid
use, this is the self-FK that is actually returned first by
get_relation_idx_constraint_oid(), NOT the PK:
postgres=# DROP TABLE IF EXISTS parent, child1;
CREATE TABLE parent (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
FOREIGN KEY (id_abc, no_part) REFERENCES parent(id, no_part)
ON UPDATE RESTRICT ON DELETE RESTRICT,
PRIMARY KEY (id, no_part)
)
PARTITION BY LIST (no_part);
CREATE TABLE child1 (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
PRIMARY KEY (id, no_part),
CONSTRAINT child1 CHECK ((no_part = 1))
);
-- force an indexscan as get_relation_idx_constraint_oid() use the unique
-- index on (conrelid, contypid, conname) to scan pg_cosntraint
set enable_seqscan TO off;
set enable_bitmapscan TO off;
SELECT conname
FROM pg_constraint
WHERE conrelid = 'parent'::regclass <=== parent
AND conindid = 'parent_pkey'::regclass; <=== PK index
DROP TABLE
CREATE TABLE
CREATE TABLE
SET
SET
conname
parent_id_abc_no_part_fkey < WOOPS!
parent_pkey
(2 rows)
In consequence, when attaching the partition, the PK of child1 is not marked as
partition of the parent's PK, which is wrong. WORST, the PK of child1 is
actually unexpectedly marked as a partition of the parent's **self-FK**:
postgres=# ALTER TABLE ONLY parent ATTACH PARTITION child1
FOR VALUES IN ('1');
SELECT oid, conname, conparentid, conrelid, confrelid
FROM pg_constraint
WHERE conrelid in ('parent'::regclass, 'child1'::regclass)
ORDER BY 1;
ALTER TABLE
oid | conname | conparentid | conrelid | confrelid
---+-+-+--+---
16700 | parent_pkey | 0 |16695 | 0
16701 | parent_id_abc_no_part_fkey | 0 |16695 | 16695
16706 | child1 | 0 |16702 | 0
16708 | **child1_pkey** | **16701** |16702 | 0
16709 | parent_id_abc_no_part_fkey1 | 16701 |16695 | 16702
16712 | parent_id_abc_no_part_fkey | 16701 |16702 | 16695
(6 rows)
The expected result should probably be something like:
oid | conname | conparentid | conrelid | confrelid
---+-+-+--+---
16700 | parent_pkey | 0 |16695 | 0
...
16708 | child1_pkey | 16700 |16702 | 0
I suppose this bug might
