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(at)dalibo(dot)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