Thread: Self FK oddity when attaching a partition

Hi all,

While studying the issue discussed in thread "Detaching a partition with a FK
on itself is not possible"[1], I stumbled across an oddity while attaching a
partition having the same multiple self-FK than the parent table.

Only one of the self-FK is found as a duplicate. Find in attachment some SQL to
reproduce the scenario. Below the result of this scenario (constant from v12 to
commit 7e367924e3). Why "child1_id_abc_no_part_fkey" is found duplicated but not
the three others? From pg_constraint, only "child1_id_abc_no_part_fkey" has a
"conparentid" set.


             conname           | conparentid | conrelid | confrelid 
  -----------------------------+-------------+----------+-----------
   child1_id_abc_no_part_fkey  |       16901 |    16921 |     16921
   child1_id_def_no_part_fkey  |           0 |    16921 |     16921
   child1_id_ghi_no_part_fkey  |           0 |    16921 |     16921
   child1_id_jkl_no_part_fkey  |           0 |    16921 |     16921
   parent_id_abc_no_part_fkey  |       16901 |    16921 |     16894
   parent_id_abc_no_part_fkey  |           0 |    16894 |     16894
   parent_id_abc_no_part_fkey1 |       16901 |    16894 |     16921
   parent_id_def_no_part_fkey  |       16906 |    16921 |     16894
   parent_id_def_no_part_fkey  |           0 |    16894 |     16894
   parent_id_def_no_part_fkey1 |       16906 |    16894 |     16921
   parent_id_ghi_no_part_fkey  |           0 |    16894 |     16894
   parent_id_ghi_no_part_fkey  |       16911 |    16921 |     16894
   parent_id_ghi_no_part_fkey1 |       16911 |    16894 |     16921
   parent_id_jkl_no_part_fkey  |           0 |    16894 |     16894
   parent_id_jkl_no_part_fkey  |       16916 |    16921 |     16894
   parent_id_jkl_no_part_fkey1 |       16916 |    16894 |     16921
  (16 rows)


                     Table "public.child1"
  [...]
  Partition of: parent FOR VALUES IN ('1')
  Partition constraint: ((no_part IS NOT NULL) AND (no_part = '1'::smallint))
  Indexes:
    "child1_pkey" PRIMARY KEY, btree (id, no_part)
  Check constraints:
    "child1" CHECK (no_part = 1)
  Foreign-key constraints:
    "child1_id_def_no_part_fkey"
        FOREIGN KEY (id_def, no_part)
        REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    "child1_id_ghi_no_part_fkey"
        FOREIGN KEY (id_ghi, no_part)
        REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    "child1_id_jkl_no_part_fkey"
        FOREIGN KEY (id_jkl, no_part)
        REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    TABLE "parent" CONSTRAINT "parent_id_abc_no_part_fkey"
        FOREIGN KEY (id_abc, no_part)
        REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    TABLE "parent" CONSTRAINT "parent_id_def_no_part_fkey"
        FOREIGN KEY (id_def, no_part)
        REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    TABLE "parent" CONSTRAINT "parent_id_ghi_no_part_fkey"
        FOREIGN KEY (id_ghi, no_part)
        REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    TABLE "parent" CONSTRAINT "parent_id_jkl_no_part_fkey"
        FOREIGN KEY (id_jkl, no_part)
        REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
  Referenced by:
    TABLE "child1" CONSTRAINT "child1_id_def_no_part_fkey"
        FOREIGN KEY (id_def, no_part)
        REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    TABLE "child1" CONSTRAINT "child1_id_ghi_no_part_fkey"
        FOREIGN KEY (id_ghi, no_part)
        REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    TABLE "child1" CONSTRAINT "child1_id_jkl_no_part_fkey"
        FOREIGN KEY (id_jkl, no_part)
        REFERENCES child1(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    TABLE "parent" CONSTRAINT "parent_id_abc_no_part_fkey"
        FOREIGN KEY (id_abc, no_part)
        REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    TABLE "parent" CONSTRAINT "parent_id_def_no_part_fkey"
        FOREIGN KEY (id_def, no_part)
        REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    TABLE "parent" CONSTRAINT "parent_id_ghi_no_part_fkey"
        FOREIGN KEY (id_ghi, no_part)
        REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT
    TABLE "parent" CONSTRAINT "parent_id_jkl_no_part_fkey"
        FOREIGN KEY (id_jkl, no_part)
        REFERENCES parent(id, no_part) ON UPDATE RESTRICT ON DELETE RESTRICT

Regards,

[1]
https://www.postgresql.org/message-id/flat/20220321113634.68c09d4b%40karst#83c0880a1b4921fcd00d836d4e6bceb3

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 exists in ATExecAttachPartitionIdx(),
DetachPartitionFinalize() and DefineIndex() where there's similar code and logic
using get_relation_idx_constraint_oid(). I didn't check for potential bugs there
though.

I'm not sure yet of how this bug should be fixed. Any comment?

Regards,

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/

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)

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)

Alvaro Herrera <alvherre@alvh.no-ip.org> 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

On Tue, 23 Aug 2022 18:30:06 +0200
Alvaro Herrera <alvherre@alvh.no-ip.org> 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,

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)

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,

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,

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)

On Thu, 8 Sep 2022 13:25:15 +0200
Alvaro Herrera <alvherre@alvh.no-ip.org> 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!

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

Andres Freund <andres@anarazel.de> 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

On Fri, Sep 9, 2022 at 4:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> 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

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 <tgl@sss.pgh.pa.us> 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 Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)

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 <alvherre@alvh.no-ip.org> 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.
> 

On Fri, 30 Sep 2022 16:11:09 -0700
Zhihong Yu <zyu@yugabyte.com> wrote:

> On Fri, Sep 30, 2022 at 3:30 PM Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
> 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,

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 Herrera        Breisgau, 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)

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 Herrera        Breisgau, 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)

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 Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Por suerte hoy explotó el califont porque si no me habría muerto
 de aburrido"  (Papelucho)

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)

On Thu, 3 Nov 2022 20:44:16 +0100
Alvaro Herrera <alvherre@alvh.no-ip.org> 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!