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

On Tue, 23 Aug 2022 18:30:06 +0200
Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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,