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

On Fri, Sep 30, 2022 at 3:30 PM Jehan-Guillaume de Rorthais <jgdr(at)dalibo(dot)com>
wrote:

> 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.
> >
>
> Hi,

+ * Self-Foreign keys are ignored as the index was preliminary
created

preliminary created -> primarily created

Cheers