[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)