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

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(at)alvh(dot)no-ip(dot)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.
>