Jehan-Guillaume de Rorthais <j...@dalibo.com> 于2024年9月3日周二 05:02写道：
> Hi, > > On Tue, 20 Aug 2024 23:09:27 -0400 > Alvaro Herrera <alvhe...@alvh.no-ip.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] > The third issue has been fixed, and codes have been pushed. Because of my misunderstanding, It should not be here. > 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. > It's ok that these two issues are fixed together. It is because current codes don't handle better when the referenced side is the partition table. > 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.17c91538@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 > > > > -- Tender Wang
