Re: [BUG] parenting a PK constraint to a self-FK one (Was: Self FK oddity when attaching a partition)

On Tue, Aug 23, 2022 at 8:07 AM Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
wrote:

> 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,
>
> Hi,
In this case the confrelid field of FormData_pg_constraint for the first
constraint would carry a valid Oid.
Can we use this information and continue searching in
get_relation_idx_constraint_oid() until an entry with 0 confrelid is found ?
If there is no such (secondary) entry, we return the first entry.

Cheers