Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails

Hi,

On Tue, 20 Aug 2024 23:09:27 -0400
Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:

> On 2024-Aug-20, Jehan-Guillaume de Rorthais wrote:
>
> > I'm back on this issue as well. I start poking at this patch to review it,
> > test it, challenge it and then report here.
> >
> > I'll try to check if some other issues might have lost/forgot on they way as
> > well.
>
> Thanks, much appreciated, looking forward to your feedback.

Sorry, it took me a while to come back to you on this topic. It has been hard to
untangle subjects, reproductions and patch…

There's three distinct issues/thread:

* Constraint & trigger catalog cleanup [1] (this thread)
* FK broken after DETACH [2]
* Maintenance consideration about self referencing FK between partitions [3]

0. Splitting in two commits

Your patch addresses two bugs:

* one for the constraint & trigger catalog cleanup;
* one for the FK broken after DETACH.

These issues are unrelated, therefore I am wondering if it would be better
to split their resolution in two different patches.

Last year, I reported them in two different threads [1][2]. The first with
implementation consideration, the second with a demo/proposal/draft fix.

Unfortunately, this discussion about the first bug slipped to the second one
when Tender stumbled on this bug as well and reported it. But, both bugs can
be triggered independently, and have distinct fixes.

Finally, splitting the patch might help setting finer patch co-authoring. I
know my patch for [2] was a draft and somewhat trivial, but I spend a fair
amount of time to report, then produce a draft patch, so I was wondering if
it would be candidate to a co-author flag on this (small, humble and
refactored by you) patch?

I'm definitely not involved (yet) in the second part though.

1. Constraint & trigger catalog cleanup [1]

I have been focusing on the current master branch and haven't taken into
consideration backpatching related issues yet.

When I first studied this bug and reported it, I held on writing a patch
because it seemed it would duplicate some existing code. I wrote:

> I poked around DetachPartitionFinalize() to try to find a way to fix this,
> but it looks like it would duplicate a bunch of code from other code path
> (eg. from CloneFkReferenced).

My proposal was to clean everything related to the old FK and use some
existing code path to create a fresh and cleaner one. This requires some
refactoring in existing code, but we would win a common path of code between
create/attach/detach, a cleaner catalog and easier code maintenance.

I've finally been able to write a PoC that implement this by calling
addFkRecurseReferenced() from DetachPartitionFinalize(). I can't join
it here because it is currently an ugly draft and I still have some work
to do. But I would really like to have a little more time (one or two days) to
explore this avenue further before you commit yours, if you don't mind? Or
maybe you already have considered this avenue and rejected it?

2. FK broken after DETACH [2]

Comparing your patch to my draft from [2], I just have a question about the
refactoring.

Fencing the constraint/trigger removal inside a conditional
RELKIND_PARTITIONED_TABLE block of code was obvious. It avoids some useless
catalog scan compared to my draft patch.

Also, the "contype == CONSTRAINT_FOREIGN" I had sounds safe to remove.

However, is it clean/light enough to add the "conparentid == fk->conoid" in
the scan key as I did? I'm not sure it saves anything else but the small
conditional block you inserted inside the loop, but I wonder if there's a
serious concern about this anyway?

Last, considering the tests, I think we should add some rows in the tables,
to make sure the FK is correctly enforced after DETACH. Something like:

CREATE SCHEMA fkpart12
CREATE TABLE fk_p ( id bigint PRIMARY KEY ) PARTITION BY list (id)
CREATE TABLE fk_p_1 PARTITION OF fk_p FOR VALUES IN (1)
CREATE TABLE fk_p_2 PARTITION OF fk_p FOR VALUES IN (2)
CREATE TABLE fk_r_1 ( id bigint PRIMARY KEY, p_id bigint NOT NULL)
CREATE TABLE fk_r_2 ( id bigint PRIMARY KEY, p_id bigint NOT NULL)
CREATE TABLE fk_r ( id bigint PRIMARY KEY, p_id bigint NOT NULL,
FOREIGN KEY (p_id) REFERENCES fk_p (id)
) PARTITION BY list (id);
SET search_path TO fkpart12;

INSERT INTO fk_p VALUES (1);

ALTER TABLE fk_r ATTACH PARTITION fk_r_2 FOR VALUES IN (2);

ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
\d fk_r_1

INSERT INTO fk_r VALUES (1,1);

ALTER TABLE fk_r DETACH PARTITION fk_r_1;
\d fk_r_1

INSERT INTO c_1 VALUES (2,2); -- fails as EXPECTED
DELETE FROM p; -- should fails but was buggy

ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
\d fk_r_1

3. Self referencing FK between partitions [3]

You added to your commit message:

verify: 20230707175859(dot)17c91538(at)karst

I'm not sure what the "verify" flag means. Unfortunately, your patch doesn't
help on this topic.

This bug really needs more discussion and design consideration. I have
thought about this problem and haven't found any solution that don't involve
breaking the current core behavior. It really looks like an impossible bug to
fix without dropping the constraint itself. On both side. Maybe the only sane
behavior would be to forbid detaching the partition if it would break the
constraint.

But let's discuss this on the related thread, should we?

Thank you for reading me all the way down to the bottom!

Regards,

[1] https://www.postgresql.org/message-id/20230705233028.2f554f73%40karst
[2] https://www.postgresql.org/message-id/20230420144344.40744130%40karst
[3] https://www.postgresql.org/message-id/20230707175859.17c91538%40karst