Обсуждение: [BUG] Fix DETACH with FK pointing to a partitioned table fails

Hi,

(patch proposal below).

Consider a table with a FK pointing to a partitioned table.

  CREATE TABLE p ( id bigint PRIMARY KEY )
    PARTITION BY list (id);
  CREATE TABLE p_1 PARTITION OF p FOR VALUES IN (1);

  CREATE TABLE r_1 (
    id   bigint PRIMARY KEY,
    p_id bigint NOT NULL,
    FOREIGN KEY (p_id) REFERENCES p (id)
  );

Now, attach this table "refg_1" as partition of another one having the same FK:

  CREATE TABLE r (
    id   bigint PRIMARY KEY,
    p_id bigint NOT NULL,
    FOREIGN KEY (p_id) REFERENCES p (id)
  ) PARTITION BY list (id);

  ALTER TABLE r ATTACH PARTITION r_1 FOR VALUES IN (1);

The old sub-FKs (below 18289) created in this table to enforce the action
triggers on referenced partitions are not deleted when the table becomes a
partition. Because of this, we have additional and useless triggers on the
referenced partitions and we can not DETACH this partition on the referencing
side anymore:

  => ALTER TABLE r DETACH PARTITION r_1;
  ERROR:  could not find ON INSERT check triggers of foreign key
          constraint 18289

  => SELECT c.oid, conparentid,
       conrelid::regclass,
       confrelid::regclass,
       t.tgfoid::regproc
     FROM pg_constraint c
     JOIN pg_trigger t ON t.tgconstraint = c.oid
     WHERE confrelid::regclass = 'p_1'::regclass;
    oid  │ conparentid │ conrelid │ confrelid │         tgfoid
  ───────┼─────────────┼──────────┼───────────┼────────────────────────
   18289 │       18286 │ r_1      │ p_1       │ "RI_FKey_noaction_del"
   18289 │       18286 │ r_1      │ p_1       │ "RI_FKey_noaction_upd"
   18302 │       18299 │ r        │ p_1       │ "RI_FKey_noaction_del"
   18302 │       18299 │ r        │ p_1       │ "RI_FKey_noaction_upd"
  (4 rows)

The legitimate constraint and triggers here are 18302. The old sub-FK
18289 having 18286 as parent should have gone during the ATTACH PARTITION.

Please, find in attachment a patch dropping old "sub-FK" during the ATTACH
PARTITION command and adding a regression test about it. At the very least, it
help understanding the problem and sketch a possible solution.

Regards,

On 2023-Jul-05, Jehan-Guillaume de Rorthais wrote:

>   ALTER TABLE r ATTACH PARTITION r_1 FOR VALUES IN (1); 
> 
> The old sub-FKs (below 18289) created in this table to enforce the action
> triggers on referenced partitions are not deleted when the table becomes a
> partition. Because of this, we have additional and useless triggers on the
> referenced partitions and we can not DETACH this partition on the referencing
> side anymore:

Oh, hm, interesting.  Thanks for the report and patch.  I found a couple
of minor issues with it (most serious one: nkeys should be 3, not 2;
also sysscan should use conrelid index), but I'll try and complete it so
that it's ready for 2023-08-10's releases.

Regards

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On 2023-Aug-03, tender wang wrote:

> I think  old "sub-FK" should not be dropped, that will be violates foreign
> key constraint.

Yeah, I've been playing more with the patch and it is definitely not
doing the right things.  Just eyeballing the contents of pg_trigger and
pg_constraint for partitions added by ALTER...ATTACH shows that the
catalog contents are inconsistent with those added by CREATE TABLE
PARTITION OF.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 2023-Aug-07, tender wang wrote:

> The foreign key still works even though partition was detached. Is this
> behavior expected?

Well, there's no reason for it not to, right?  For example, if you
detach a partition and then attach it again, you don't have to scan the
partition on attach, because you know the constraint has remained valid
all along.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"In fact, the basic problem with Perl 5's subroutines is that they're not
crufty enough, so the cruft leaks out into user-defined code instead, by
the Conservation of Cruft Principle."  (Larry Wall, Apocalypse 6)

On Thu, 3 Aug 2023 11:02:43 +0200
Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> On 2023-Aug-03, tender wang wrote:
> 
> > I think  old "sub-FK" should not be dropped, that will be violates foreign
> > key constraint.  
> 
> Yeah, I've been playing more with the patch and it is definitely not
> doing the right things.  Just eyeballing the contents of pg_trigger and
> pg_constraint for partitions added by ALTER...ATTACH shows that the
> catalog contents are inconsistent with those added by CREATE TABLE
> PARTITION OF.

Well, as stated in my orignal message, at the patch helps understanding the
problem and sketch a possible solution. It definitely is not complete.

After DETACHing the table, we surely needs to check everything again and
recreating what is needed to keep the FK consistent.

But should we keep the FK after DETACH? Did you check the two other discussions
related to FK, self-FK & partition? Unfortunately, as Tender experienced, the
more we dig the more we find bugs. Moreover, the second one might seems
unsolvable and deserve a closer look. See:

* FK broken after DETACHing referencing part
  https://www.postgresql.org/message-id/20230420144344.40744130%40karst
* Issue attaching a table to a partitioned table with an  auto-referenced
  foreign key
  https://www.postgresql.org/message-id/20230707175859.17c91538%40karst

On 2023-Oct-25, tender wang wrote:

> Hi
>    Is there any conclusion to this issue?

None yet.  I intend to work on this at some point, hopefully soon.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Hello,

I think the fix for the check triggers should be as the attached.  Very
close to what you did, but you were skipping some operations that needed
to be kept.  AFAICS this patch works correctly for the posted cases.

I haven't looked at the action triggers yet; I think we need to create
one trigger for each partition of the referenced side, so we need to
loop instead of doing a single one.



I find this pair of queries useful; they show which constraints exist
and which triggers belong to each.  We need to make the constraints and
triggers match after a detach right as things would be if the
just-detached partition were an individual table having the same foreign
key.


-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"People get annoyed when you try to debug them."  (Larry Wall)

On Mon, Jul 22, 2024 at 1:52 PM Tender Wang <tndrwang@gmail.com> wrote:
>
>
>
> Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年7月19日周五 21:18写道：
>>
>> Hello,
>>
>> I think the fix for the check triggers should be as the attached.  Very
>> close to what you did, but you were skipping some operations that needed
>> to be kept.  AFAICS this patch works correctly for the posted cases.
>
>
> After applying the attached, the r_1_p_id_fkey1 will have redundant action
> triggers, as below:
> postgres=# select oid, conname, contype, conrelid, conindid,conparentid, confrelid,conislocal,coninhcount,
connoinheritfrom pg_constraint where oid = 16402; 
>   oid  |    conname     | contype | conrelid | conindid | conparentid | confrelid | conislocal | coninhcount |
connoinherit
>
-------+----------------+---------+----------+----------+-------------+-----------+------------+-------------+--------------
>  16402 | r_1_p_id_fkey1 | f       |    16394 |    16392 |           0 |     16389 | t          |           0 | f
> (1 row)
>
> postgres=# select oid, tgrelid, tgparentid, tgconstrrelid, tgconstrindid, tgconstraint from pg_trigger where
tgconstraint= 16402; 
>   oid  | tgrelid | tgparentid | tgconstrrelid | tgconstrindid | tgconstraint
> -------+---------+------------+---------------+---------------+--------------
>  16403 |   16389 |      16400 |         16394 |         16392 |        16402
>  16404 |   16389 |      16401 |         16394 |         16392 |        16402
>  16422 |   16389 |          0 |         16394 |         16392 |        16402
>  16423 |   16389 |          0 |         16394 |         16392 |        16402
> (4 rows)
>

Yes, seems Alvaro has mentioned that he hasn't looked at the
action triggers, in the attached patch, I add some logic that
first check if there exists action triggers, if yes, just update
their Parent Trigger to InvalidOid.

>
> --
> Tender Wang



--
Regards
Junwang Zhao

On Fri, Jul 26, 2024 at 2:36 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Mon, Jul 22, 2024 at 1:52 PM Tender Wang <tndrwang@gmail.com> wrote:
> >
> >
> >
> > Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年7月19日周五 21:18写道：
> >>
> >> Hello,
> >>
> >> I think the fix for the check triggers should be as the attached.  Very
> >> close to what you did, but you were skipping some operations that needed
> >> to be kept.  AFAICS this patch works correctly for the posted cases.
> >
> >
> > After applying the attached, the r_1_p_id_fkey1 will have redundant action
> > triggers, as below:
> > postgres=# select oid, conname, contype, conrelid, conindid,conparentid, confrelid,conislocal,coninhcount,
connoinheritfrom pg_constraint where oid = 16402; 
> >   oid  |    conname     | contype | conrelid | conindid | conparentid | confrelid | conislocal | coninhcount |
connoinherit
> >
-------+----------------+---------+----------+----------+-------------+-----------+------------+-------------+--------------
> >  16402 | r_1_p_id_fkey1 | f       |    16394 |    16392 |           0 |     16389 | t          |           0 | f
> > (1 row)
> >
> > postgres=# select oid, tgrelid, tgparentid, tgconstrrelid, tgconstrindid, tgconstraint from pg_trigger where
tgconstraint= 16402; 
> >   oid  | tgrelid | tgparentid | tgconstrrelid | tgconstrindid | tgconstraint
> > -------+---------+------------+---------------+---------------+--------------
> >  16403 |   16389 |      16400 |         16394 |         16392 |        16402
> >  16404 |   16389 |      16401 |         16394 |         16392 |        16402
> >  16422 |   16389 |          0 |         16394 |         16392 |        16402
> >  16423 |   16389 |          0 |         16394 |         16392 |        16402
> > (4 rows)
> >
>
> Yes, seems Alvaro has mentioned that he hasn't looked at the
> action triggers, in the attached patch, I add some logic that
> first check if there exists action triggers, if yes, just update
> their Parent Trigger to InvalidOid.
>
> >
> > --
> > Tender Wang
>
>
>
> --
> Regards
> Junwang Zhao

There is a bug report[0] Tender comments might be the same
issue as this one, but I tried Alvaro's and mine patch, neither
could solve that problem, I did not tried Tender's earlier patch
thought. I post the test script below in case you are interested.

CREATE TABLE t1 (a int, PRIMARY KEY (a));
CREATE TABLE t (a int, PRIMARY KEY (a), FOREIGN KEY (a) REFERENCES t1)
PARTITION BY LIST (a);
ALTER TABLE t ATTACH PARTITION t1 FOR VALUES IN (1);
ALTER TABLE t DETACH PARTITION t1;
ALTER TABLE t ATTACH PARTITION t1 FOR VALUES IN (1);


[0] https://www.postgresql.org/message-id/18541-628a61bc267cd2d3@postgresql.org

--
Regards
Junwang Zhao

On 2024-Jul-26, Junwang Zhao wrote:

> There is a bug report[0] Tender comments might be the same
> issue as this one, but I tried Alvaro's and mine patch, neither
> could solve that problem, I did not tried Tender's earlier patch
> thought. I post the test script below in case you are interested.

Yeah, I've been looking at this whole debacle this week and after
looking at it more closely, I realized that the overall problem requires
a much more invasive solution -- namely, that on DETACH, if the
referenced table is partitioned, we need to create additional
pg_constraint entries from the now-standalone table (was partition) to
each of the partitions of the referenced table; and also add action
triggers to each of those.  Without that, the constraint is incomplete
and doesn't work (as reported multiple times already).

One thing I have not yet tried is what if the partition being detach is
also partitioned.  I mean, do we need to handle each sub-partition
explicitly in some way?  I think the answer is no, but it needs tests.

I have written the patch to do this on detach, and AFAICS it works well,
though it changes the behavior of some existing tests (IIRC related to
self-referencing FKs).  Also, the next problem is making sure that
ATTACH deals with it correctly.  I'm on this bit today.

Self-referencing FKs seem to have additional problems :-(

The queries I was talking about are these

\set tables ''''prim.*''',''forign.*''',''''lone''''

select oid, conparentid, contype, conname, conrelid::regclass, confrelid::regclass, conkey, confkey, conindid::regclass
frompg_constraint where contype = 'f' and (conrelid::regclass::text ~ any (array[:tables]) or confrelid::regclass::text
~any (array[:tables])) order by contype, conrelid, confrelid; select tgconstraint, oid, tgrelid::regclass,
tgconstrrelid::regclass,tgname, tgparentid, tgconstrindid::regclass, tgfoid::regproc from pg_trigger where tgconstraint
in(select oid from pg_constraint where conrelid::regclass::text ~ any (array[:tables]) or confrelid::regclass::text ~
any(array[:tables])) order by tgconstraint, tgrelid::regclass::text, tgfoid;
 

Written as a single line in psql they let you quickly see all the
constraints and their associated triggers, so for instance you can see
whether this sequence

create table prim (a int primary key) partition by list (a);
create table prim1 partition of prim for values in (1);
create table prim2 partition of prim for values in (2);
create table forign (a int references prim) partition by list (a);
create table forign1 partition of forign for values in (1);
create table forign2 partition of forign for values in (2);
alter table forign detach partition forign1;

produces the same set of constraints and triggers as this other sequence

create table prim (a int primary key) partition by list (a);
create table prim1 partition of prim for values in (1);
create table prim2 partition of prim for values in (2);
create table forign (a int references prim) partition by list (a);
create table forign2 partition of forign for values in (2);
create table forign1 (a int references prim);


The patch is more or less like the attached, far from ready.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

On 2024-Jul-26, Tender Wang wrote:

> Junwang Zhao <zhjwpku@gmail.com> 于2024年7月26日周五 14:57写道：
> 
> > There is a bug report[0] Tender comments might be the same issue as
> > this one, but I tried Alvaro's and mine patch, neither could solve
> > that problem, I did not tried Tender's earlier patch thought. I post
> > the test script below in case you are interested.
> 
> My earlier patch should handle Alexander reported case. But I did not
> do more test. I'm not sure that wether or not has dismatching between
> pg_constraint and pg_trigger.
> 
> I aggred with Alvaro said that "requires a much more invasive
> solution".

Here's the patch which, as far as I can tell, fixes all the reported
problems (other than the one in bug 18541, for which I proposed an
unrelated fix in that thread[1]).  If you can double-check, I would very
much appreciate that.  Also, I think the test cases the patch adds
reflect the provided examples sufficiently, but if we're still failing
to cover some, please let me know.


As I understand, this fix needs to be applied all the way back to 12,
because the overall functionality is that old.  However, in branches 14
and back, the patch doesn't apply cleanly, because of the changes we
made in commit f4566345cf40 :-(  I'm tempted to fix it in branches 15,
16, 17, master now and potentially backpatch later, to avoid dragging
things along further.  It's taken long enough already.

[1]  https://postgr.es/m/202408072222.icgykv5yrql5@alvherre.pgsql

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"World domination is proceeding according to plan"        (Andrew Morton)