Date: Thu, 8 Sep 2022 09:40:26 +0200
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: [BUG] wrong FK constraint name when colliding name on ATTACH
Message-ID: <20220908094026.640a0224@karst>
In-Reply-To: <20220901184156.738ebee5@karst>
References: <20220824122850.032f9639@karst>
	<20220824104913.hgildshd4w4l6cu4@alvherre.pgsql>
	<20220901184156.738ebee5@karst>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20220908094026.640a0224%40karst>
Precedence: bulk

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 <jgdr@dalibo.com> 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,