Date: Thu, 1 Sep 2022 18:41:56 +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: [BUG] wrong FK constraint name when colliding name on ATTACH
Message-ID: <20220901184156.738ebee5@karst>
In-Reply-To: <20220824104913.hgildshd4w4l6cu4@alvherre.pgsql>
References: <20220824122850.032f9639@karst>
	<20220824104913.hgildshd4w4l6cu4@alvherre.pgsql>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MP_/dajHgyhUOhl1wTbdYuIRFBX"
Archived-At: 
 <https://www.postgresql.org/message-id/20220901184156.738ebee5%40karst>
Precedence: bulk

--MP_/dajHgyhUOhl1wTbdYuIRFBX
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

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,

--MP_/dajHgyhUOhl1wTbdYuIRFBX
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=0001-Fix-FK-name-when-colliding-during-partition-attachme.patch

From f1eeacb1eb3face38d76325666aff57019ef84c9 Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
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
"<relname>__fkey" instead of "<relname>_<attrs>_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(NameStr(constrForm->conname));
 
 		indexOid = constrForm->conindid;
 		constrOid =
-- 
2.37.2


--MP_/dajHgyhUOhl1wTbdYuIRFBX--