[BUG] wrong FK constraint name when colliding name on ATTACH
Hi,
While studying and hacking on the parenting constraint issue, I found an
incoherent piece of code leading to badly chosen fk name. If a constraint
name collision is detected, while choosing a new name for the constraint,
the code uses fkconstraint->fk_attrs which is not yet populated:
/* No dice. Set up to create our own constraint */
fkconstraint = makeNode(Constraint);
if (ConstraintNameIsUsed(CONSTRAINT_RELATION,
RelationGetRelid(partRel),
NameStr(constrForm->conname)))
fkconstraint->conname =
ChooseConstraintName(RelationGetRelationName(partRel),
ChooseForeignKeyConstraintNameAddition(
fkconstraint->fk_attrs), // <= WOO000OPS
"fkey",
RelationGetNamespace(partRel), NIL);
else
fkconstraint->conname = pstrdup(NameStr(constrForm->conname));
fkconstraint->fk_upd_action = constrForm->confupdtype;
fkconstraint->fk_del_action = constrForm->confdeltype;
fkconstraint->deferrable = constrForm->condeferrable;
fkconstraint->initdeferred = constrForm->condeferred;
fkconstraint->fk_matchtype = constrForm->confmatchtype;
for (int i = 0; i < numfks; i++)
{
Form_pg_attribute att;
att = TupleDescAttr(RelationGetDescr(partRel),
mapped_conkey[i] - 1);
fkconstraint->fk_attrs = lappend(fkconstraint->fk_attrs, // <= POPULATING
makeString(NameStr(att->attname)));
}
The following SQL script showcase the bad constraint name:
DROP TABLE IF EXISTS parent, child1;
CREATE TABLE parent (
id bigint NOT NULL default 1,
no_part smallint NOT NULL,
id_abc bigint,
CONSTRAINT dummy_constr 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 dummy_constr CHECK ((no_part = 1))
);
ALTER TABLE parent ATTACH PARTITION child1 FOR VALUES IN ('1');
SELECT conname
FROM pg_constraint
WHERE conrelid = 'child1'::regclass
AND contype = 'f';
DROP TABLE
CREATE TABLE
CREATE TABLE
ALTER TABLE
conname
--
child1__fkey
(1 row)
The resulting constraint name "child1__fkey" is missing the attributes name the
original code wanted to add. The expected name is "child1_id_abc_no_part_fkey".
Find in attachment a simple fix, moving the name assignation after the
FK attributes are populated.
Regards,
>From f1eeacb1eb3face38d76325666aff57019ef84c9 Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais
Date: Thu, 1 Sep 2022 17:41:55 +0200
Subject: [PATCH] Fix FK name when colliding during partition attachment
During ATLER TABLE ATTACH PARTITION, if the name of a parent's
foreign key constraint is already used on the partition, the code
try to choose another one before the FK attributes list has been
populated. The resulting constraint name was a strange
"__fkey" instead of "__fkey".
---
src/backend/commands/tablecmds.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index dacc989d85..53b0f3a9c1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10304,16 +10304,6 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel)
/* No dice. Set up to create our own constraint */
fkconstraint = makeNode(Constraint);
- if (ConstraintNameIsUsed(CONSTRAINT_RELATION,
- RelationGetRelid(partRel),
- NameStr(constrForm->conname)))
- fkconstraint->conname =
-ChooseConstraintName(RelationGetRelationName(partRel),
- ChooseForeignKeyConstraintNameAddition(fkconstraint->fk_attrs),
- "fkey",
- RelationGetNamespace(partRel), NIL);
- else
- fkconstraint->conname = pstrdup(NameStr(constrForm->conname));
fkconstraint->fk_upd_action = constrForm->confupdtype;
fkconstraint->fk_del_action = constrForm->confdeltype;
fkconstraint->deferrable = constrForm->condeferrable;
@@ -10328,6 +10318,16 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel)
fkconstraint->fk_attrs = lappend(fkconstraint->fk_attrs,
makeString(NameStr(att->attname)));
}
+ if (ConstraintNameIsUsed(CONSTRAINT_RELATION,
+ RelationGetRelid(partRel),
+ NameStr(constrForm->conname)))
+ fkconstraint->conname =
+ChooseConstraintName(RelationGetRelationName(partRel),
+ ChooseForeignKeyConstraintNameAddition(fkconstraint->fk_attrs),
+ "fkey",
+ RelationGetNamespace(partRel), NIL);
+ else
+ fkconstraint->conname = pstrdup(Nam
Re: [BUG] wrong FK constraint name when colliding name on ATTACH
Hi there,
I believe this very small bug and its fix are really trivial and could be push
out of the way quite quickly. It's just about a bad constraint name fixed by
moving one assignation after the next one. This could easily be fixed for next
round of releases.
Well, I hope I'm not wrong :)
Regards,
On Thu, 1 Sep 2022 18:41:56 +0200
Jehan-Guillaume de Rorthais wrote:
> While studying and hacking on the parenting constraint issue, I found an
> incoherent piece of code leading to badly chosen fk name. If a constraint
> name collision is detected, while choosing a new name for the constraint,
> the code uses fkconstraint->fk_attrs which is not yet populated:
>
> /* No dice. Set up to create our own constraint */
> fkconstraint = makeNode(Constraint);
> if (ConstraintNameIsUsed(CONSTRAINT_RELATION,
>RelationGetRelid(partRel),
>NameStr(constrForm->conname)))
> fkconstraint->conname =
> ChooseConstraintName(RelationGetRelationName(partRel),
>ChooseForeignKeyConstraintNameAddition(
> fkconstraint->fk_attrs), // <= WOO000OPS
>"fkey",
>RelationGetNamespace(partRel), NIL);
> else
> fkconstraint->conname = pstrdup(NameStr(constrForm->conname));
> fkconstraint->fk_upd_action = constrForm->confupdtype;
> fkconstraint->fk_del_action = constrForm->confdeltype;
> fkconstraint->deferrable = constrForm->condeferrable;
> fkconstraint->initdeferred = constrForm->condeferred;
> fkconstraint->fk_matchtype = constrForm->confmatchtype;
> for (int i = 0; i < numfks; i++)
> {
> Form_pg_attribute att;
>
> att = TupleDescAttr(RelationGetDescr(partRel),
> mapped_conkey[i] - 1);
> fkconstraint->fk_attrs = lappend(fkconstraint->fk_attrs, // <=
> POPULATING makeString(NameStr(att->attname)));
> }
>
> The following SQL script showcase the bad constraint name:
>
> DROP TABLE IF EXISTS parent, child1;
>
> CREATE TABLE parent (
> id bigint NOT NULL default 1,
> no_part smallint NOT NULL,
> id_abc bigint,
> CONSTRAINT dummy_constr 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 dummy_constr CHECK ((no_part = 1))
> );
>
> ALTER TABLE parent ATTACH PARTITION child1 FOR VALUES IN ('1');
>
> SELECT conname
> FROM pg_constraint
> WHERE conrelid = 'child1'::regclass
> AND contype = 'f';
>
> DROP TABLE
> CREATE TABLE
> CREATE TABLE
> ALTER TABLE
>
> conname
> --
>child1__fkey
> (1 row)
>
> The resulting constraint name "child1__fkey" is missing the attributes name
> the original code wanted to add. The expected name is
> "child1_id_abc_no_part_fkey".
>
> Find in attachment a simple fix, moving the name assignation after the
> FK attributes are populated.
>
> Regards,
Re: [BUG] wrong FK constraint name when colliding name on ATTACH
On 2022-Sep-08, Jehan-Guillaume de Rorthais wrote:
> Hi there,
>
> I believe this very small bug and its fix are really trivial and could be push
> out of the way quite quickly. It's just about a bad constraint name fixed by
> moving one assignation after the next one. This could easily be fixed for next
> round of releases.
>
> Well, I hope I'm not wrong :)
I think you're right, so pushed, and backpatched to 12. I added the
test case to regression also.
For 11, I adjusted the test case so that it didn't depend on an FK
pointing to a partitioned table (which is not supported there); it turns
out that the old code is not smart enough to get into the problem in the
first place. Setup is
CREATE TABLE parted_fk_naming_pk (id bigint primary key);
CREATE TABLE parted_fk_naming (
id_abc bigint,
CONSTRAINT dummy_constr FOREIGN KEY (id_abc)
REFERENCES parted_fk_naming_pk (id)
)
PARTITION BY LIST (id_abc);
CREATE TABLE parted_fk_naming_1 (
id_abc bigint,
CONSTRAINT dummy_constr CHECK (true)
);
and then
ALTER TABLE parted_fk_naming ATTACH PARTITION parted_fk_naming_1 FOR VALUES IN
('1');
throws this error:
ERROR: duplicate key value violates unique constraint
"pg_constraint_conrelid_contypid_conname_index"
DETALLE: Key (conrelid, contypid, conname)=(686125, 0, dummy_constr) already
exists.
It seems fair to say that this case, with pg11, is unsupported and
people should upgrade if they want better behavior.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Those who use electric razors are infidels destined to burn in hell while
we drink from rivers of beer, download free vids and mingle with naked
well shaved babes." (http://slashdot.org/comments.pl?sid=44793&cid=4647152)
Re: [BUG] wrong FK constraint name when colliding name on ATTACH
On Thu, 8 Sep 2022 13:25:15 +0200 Alvaro Herrera wrote: > On 2022-Sep-08, Jehan-Guillaume de Rorthais wrote: > > > Hi there, > > > > I believe this very small bug and its fix are really trivial and could be > > push out of the way quite quickly. It's just about a bad constraint name > > fixed by moving one assignation after the next one. This could easily be > > fixed for next round of releases. > > > > Well, I hope I'm not wrong :) > > I think you're right, so pushed, and backpatched to 12. I added the > test case to regression also. Great, thank you for the additional work on the regression test and the commit! > For 11, I adjusted the test case so that it didn't depend on an FK > pointing to a partitioned table (which is not supported there); it turns > out that the old code is not smart enough to get into the problem in the > first place. [...] > It seems fair to say that this case, with pg11, is unsupported and > people should upgrade if they want better behavior. That works for me. Thanks!
Re: [BUG] wrong FK constraint name when colliding name on ATTACH
Hi, On 2022-09-08 13:25:15 +0200, Alvaro Herrera wrote: > I think you're right, so pushed, and backpatched to 12. I added the > test case to regression also. Something here doesn't look to be quite right. Starting with this commit CI [1] started to fail on freebsd (stack trace [2]), and in the meson branch I've also seen the crash on windows (CI run[3], stack trace [4]). There does appear to be some probabilistic aspect, I also saw a run succeed. Greetings, Andres Freund [1] https://cirrus-ci.com/github/postgres/postgres/ [2] https://api.cirrus-ci.com/v1/task/6180840047640576/logs/cores.log [3] https://cirrus-ci.com/task/6629440791773184 [4] https://api.cirrus-ci.com/v1/artifact/task/6629440791773184/crashlog/crashlog-postgres.exe_1468_2022-09-08_17-05-24-591.txt
Re: [BUG] wrong FK constraint name when colliding name on ATTACH
Andres Freund writes: > Something here doesn't look to be quite right. Starting with this commit CI > [1] started to fail on freebsd (stack trace [2]), and in the meson branch I've > also seen the crash on windows (CI run[3], stack trace [4]). The crash seems 100% reproducible if I remove the early-exit optimization from GetForeignKeyActionTriggers: diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 53b0f3a9c1..112ca77d97 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -10591,8 +10591,6 @@ GetForeignKeyActionTriggers(Relation trigrel, Assert(*updateTriggerOid == InvalidOid); *updateTriggerOid = trgform->oid; } - if (OidIsValid(*deleteTriggerOid) && OidIsValid(*updateTriggerOid)) - break; } if (!OidIsValid(*deleteTriggerOid)) With that in place, it's probabilistic whether the Asserts notice anything wrong, and mostly they don't. But there are multiple matching triggers: regression=# select oid, tgconstraint, tgrelid,tgconstrrelid, tgtype, tgname from pg_trigger where tgconstraint = 104301; oid | tgconstraint | tgrelid | tgconstrrelid | tgtype |tgname +--+-+---++--- 104302 | 104301 | 104294 |104294 | 9 | RI_ConstraintTrigger_a_104302 104303 | 104301 | 104294 |104294 | 17 | RI_ConstraintTrigger_a_104303 104304 | 104301 | 104294 |104294 | 5 | RI_ConstraintTrigger_c_104304 104305 | 104301 | 104294 |104294 | 17 | RI_ConstraintTrigger_c_104305 (4 rows) I suspect that the filter conditions being applied are inadequate for the case of a self-referential FK, which this evidently is given that tgrelid and tgconstrrelid are equal. I'd counsel dropping the early-exit optimization; it doesn't save much I expect, and it evidently hides bugs. Or maybe make it conditional on !USE_ASSERT_CHECKING. regards, tom lane
Re: [BUG] wrong FK constraint name when colliding name on ATTACH
On Fri, Sep 9, 2022 at 4:54 AM Tom Lane wrote: > Andres Freund writes: > > Something here doesn't look to be quite right. Starting with this commit CI > > [1] started to fail on freebsd (stack trace [2]), and in the meson branch > > I've > > also seen the crash on windows (CI run[3], stack trace [4]). > > The crash seems 100% reproducible if I remove the early-exit optimization > from GetForeignKeyActionTriggers: Indeed, reproduced here. > diff --git a/src/backend/commands/tablecmds.c > b/src/backend/commands/tablecmds.c > index 53b0f3a9c1..112ca77d97 100644 > --- a/src/backend/commands/tablecmds.c > +++ b/src/backend/commands/tablecmds.c > @@ -10591,8 +10591,6 @@ GetForeignKeyActionTriggers(Relation trigrel, > Assert(*updateTriggerOid == InvalidOid); > *updateTriggerOid = trgform->oid; > } > - if (OidIsValid(*deleteTriggerOid) && OidIsValid(*updateTriggerOid)) > - break; > } > > if (!OidIsValid(*deleteTriggerOid)) > > With that in place, it's probabilistic whether the Asserts notice anything > wrong, and mostly they don't. But there are multiple matching triggers: > > regression=# select oid, tgconstraint, tgrelid,tgconstrrelid, tgtype, tgname > from pg_trigger where tgconstraint = 104301; > oid | tgconstraint | tgrelid | tgconstrrelid | tgtype |tgname > +--+-+---++--- > 104302 | 104301 | 104294 |104294 | 9 | > RI_ConstraintTrigger_a_104302 > 104303 | 104301 | 104294 |104294 | 17 | > RI_ConstraintTrigger_a_104303 > 104304 | 104301 | 104294 |104294 | 5 | > RI_ConstraintTrigger_c_104304 > 104305 | 104301 | 104294 |104294 | 17 | > RI_ConstraintTrigger_c_104305 > (4 rows) > > I suspect that the filter conditions being applied are inadequate > for the case of a self-referential FK, which this evidently is > given that tgrelid and tgconstrrelid are equal. Yes, the loop in GetForeignKeyActionTriggers() needs this: + /* Only ever look at "action" triggers on the PK side. */ + if (RI_FKey_trigger_type(trgform->tgfoid) != RI_TRIGGER_PK) + continue; Likewise, GetForeignKeyActionTriggers() needs this: + /* Only ever look at "check" triggers on the FK side. */ + if (RI_FKey_trigger_type(trgform->tgfoid) != RI_TRIGGER_FK) + continue; We evidently missed this in f4566345cf40b0. > I'd counsel dropping the early-exit optimization; it doesn't > save much I expect, and it evidently hides bugs. Or maybe > make it conditional on !USE_ASSERT_CHECKING. While neither of these functions are called in hot paths, I am inclined to keep the early-exit bit in non-assert builds. Attached a patch. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com action-check-trigger-filter.patch Description: Binary data
Re: [BUG] wrong FK constraint name when colliding name on ATTACH
On 2022-Sep-09, Amit Langote wrote: > Yes, the loop in GetForeignKeyActionTriggers() needs this: > > + /* Only ever look at "action" triggers on the PK side. */ > + if (RI_FKey_trigger_type(trgform->tgfoid) != RI_TRIGGER_PK) > + continue; > > Likewise, GetForeignKeyActionTriggers() needs this: > > + /* Only ever look at "check" triggers on the FK side. */ > + if (RI_FKey_trigger_type(trgform->tgfoid) != RI_TRIGGER_FK) > + continue; > > We evidently missed this in f4566345cf40b0. Ouch. Thank you, pushed. > On Fri, Sep 9, 2022 at 4:54 AM Tom Lane wrote: > > I'd counsel dropping the early-exit optimization; it doesn't > > save much I expect, and it evidently hides bugs. Or maybe > > make it conditional on !USE_ASSERT_CHECKING. > > While neither of these functions are called in hot paths, I am > inclined to keep the early-exit bit in non-assert builds. I kept it that way. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)
