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

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/

I think old "sub-FK" should not be dropped, that will be violates foreign
key constraint. For example :
postgres=# insert into r values(1,1);
INSERT 0 1
postgres=# ALTER TABLE r DETACH PARTITION r_1;
ALTER TABLE
postgres=# delete from p_1 where id = 1;
DELETE 1
postgres=# select * from r_1;
id | p_id
----+------
1 | 1
(1 row)

If I run above SQLs on pg12.12, it will report error below:
postgres=# delete from p_1 where id = 1;
ERROR: update or delete on table "p_1" violates foreign key constraint
"r_1_p_id_fkey1" on table "r_1"
DETAIL: Key (id)=(1) is still referenced from table "r_1".

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2023年7月31日周一 20:58写道：

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/

I think the code to determine that fk of a partition is inherited or not is
not enough.
For example, in this case, foreign key r_1_p_id_fkey1 is not inherited
from parent.

If conform->conparentid(in DetachPartitionFinalize func) is valid, we
should recheck confrelid(pg_constraint) field.

I try to fix this problem in the attached patch.
Any thoughts.

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2023年8月3日周四 17:02写道：

Oversight the DetachPartitionFinalize(), I found another bug below:

postgres=# CREATE TABLE p ( id bigint PRIMARY KEY ) PARTITION BY list (id);
CREATE TABLE
postgres=# CREATE TABLE p_1 PARTITION OF p FOR VALUES IN (1);
CREATE TABLE
postgres=# CREATE TABLE r_1 (
postgres(# id bigint PRIMARY KEY,
postgres(# p_id bigint NOT NULL
postgres(# );
CREATE TABLE
postgres=# CREATE TABLE r (
postgres(# id bigint PRIMARY KEY,
postgres(# p_id bigint NOT NULL,
postgres(# FOREIGN KEY (p_id) REFERENCES p (id)
postgres(# ) PARTITION BY list (id);
CREATE TABLE
postgres=# ALTER TABLE r ATTACH PARTITION r_1 FOR VALUES IN (1);
ALTER TABLE
postgres=# ALTER TABLE r DETACH PARTITION r_1;
ALTER TABLE
postgres=# insert into r_1 values(1,1);
ERROR: insert or update on table "r_1" violates foreign key constraint
"r_p_id_fkey"
DETAIL: Key (p_id)=(1) is not present in table "p".

After detach operation, r_1 is normal relation and the inherited foreign
key 'r_p_id_fkey' should be removed.

tender wang <tndrwang@gmail.com> 于2023年8月3日周四 17:34写道：

Oversight the DetachPartitionFinalize() again, I found the root cause why
'r_p_id_fkey' wat not removed.

DetachPartitionFinalize() call the GetParentedForeignKeyRefs() func to get
tuple from pg_constraint that will be delete but failed.
according to the comments, the GetParentedForeignKeyRefs() func get the
tuple reference me not I reference others.

I try to fix this bug :
i. ConstraintSetParentConstraint() should not be called in
DetachPartitionFinalize(), because after conparentid was set to 0,
we can not find inherited foreign keys.
ii. create another function like GetParentedForeignKeyRefs(), but the
ScanKey should be conrelid field not confrelid.

I quickly test on my above solution in my env, can be solve above issue.

tender wang <tndrwang@gmail.com> 于2023年8月4日周五 17:04写道：

The foreign key still works even though partition was detached. Is this
behavior expected?
I can't find the answer in the document. If it is expected behavior ,
please ignore the bug I reported a few days ago.

tender wang <tndrwang@gmail.com> 于2023年8月4日周五 17:04写道：

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
/messages/by-id/20230420144344.40744130@karst
* Issue attaching a table to a partitioned table with an auto-referenced
foreign key
/messages/by-id/20230707175859.17c91538@karst

Hi
Is there any conclusion to this issue?

Jehan-Guillaume de Rorthais <jgdr@dalibo.com> 于2023年8月10日周四 23:03写道：

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/

Hi Alvaro,
I re-analyzed this issue, and here is my analysis process.
step 1: CREATE TABLE p ( id bigint PRIMARY KEY )
PARTITION BY list (id);
step2: CREATE TABLE p_1 PARTITION OF p FOR VALUES IN (1);
step3: CREATE TABLE r_1 (
id bigint PRIMARY KEY,
p_id bigint NOT NULL,
FOREIGN KEY (p_id) REFERENCES p (id)
);
After above step 3 operations, we have below catalog tuples:
postgres=# select oid, relname from pg_class where relname = 'p';
oid | relname
-------+---------
16384 | p
(1 row)
postgres=# select oid, relname from pg_class where relname = 'p_1';
oid | relname
-------+---------
16389 | p_1
(1 row)
postgres=# select oid, relname from pg_class where relname = 'r_1';
oid | relname
-------+---------
16394 | r_1
(1 row)
postgres=# select oid, conname,conrelid,conparentid,confrelid from
pg_constraint where conrelid = 16394;
oid | conname | conrelid | conparentid | confrelid
-------+-------------------+----------+-------------+-----------
16397 | r_1_p_id_not_null | 16394 | 0 | 0
16399 | r_1_pkey | 16394 | 0 | 0
16400 | r_1_p_id_fkey | 16394 | 0 | 16384
16403 | r_1_p_id_fkey1 | 16394 | 16400 | 16389
(4 rows)
postgres=# select oid, tgrelid, tgparentid,
tgconstrrelid,tgconstrindid,tgconstraint from pg_trigger where tgconstraint
= 16403;
oid | tgrelid | tgparentid | tgconstrrelid | tgconstrindid | tgconstraint
-------+---------+------------+---------------+---------------+--------------
16404 | 16389 | 16401 | 16394 | 16392 | 16403
16405 | 16389 | 16402 | 16394 | 16392 | 16403
(2 rows)
postgres=# select oid, tgrelid, tgparentid,
tgconstrrelid,tgconstrindid,tgconstraint from pg_trigger where tgconstraint
= 16400;
oid | tgrelid | tgparentid | tgconstrrelid | tgconstrindid | tgconstraint
-------+---------+------------+---------------+---------------+--------------
16401 | 16384 | 0 | 16394 | 16387 | 16400
16402 | 16384 | 0 | 16394 | 16387 | 16400
16406 | 16394 | 0 | 16384 | 16387 | 16400
16407 | 16394 | 0 | 16384 | 16387 | 16400
(4 rows)
Because table p is partitioned table and it has one child table p_1. So
when r_1 add foreign key constraint, according to addFkRecurseReferenced(),
each partition should have one pg_constraint row(e.g. r_1_p_id_fkey1).
After called addFkRecurseReferenced() in ATAddForeignKeyConstraint(),
addFkRecurseReferencing() will be called, in which
it will add INSERT check trigger and UPDATE check trigger for r_1_p_id_fkey
but not for r_1_p_id_fkey1.
So when detach r_1 from r, according to DetachPartitionFinalize(), the
inherited fks should unlink relationship from parent.
The created INSERT and UPDATE check triggers should unlink relationship
link fks. But just like I said above, the r_1_p_id_fkey1
actually doesn't have INSERT check trigger.

I slightly modified the previous patch，but I didn't add test case, because
I found another issue.
After done ALTER TABLE r ATTACH PARTITION r_1 FOR VALUES IN (1);
I run the oidjoins.sql and has warnings as belwo:
psql:/tender/postgres/src/test/regress/sql/oidjoins.sql:49: WARNING: FK
VIOLATION IN pg_trigger({tgparentid}): ("(0,3)",16401)
psql:/tender/postgres/src/test/regress/sql/oidjoins.sql:49: WARNING: FK
VIOLATION IN pg_trigger({tgparentid}): ("(0,4)",16402)
postgres=# select oid, tgrelid, tgparentid,
tgconstrrelid,tgconstrindid,tgconstraint from pg_trigger where oid >= 16384;
oid | tgrelid | tgparentid | tgconstrrelid | tgconstrindid |
tgconstraint
-------+---------+------------+---------------+---------------+--------------
16404 | 16389 | 16401 | 16394 | 16392 | 16403
16405 | 16389 | 16402 | 16394 | 16392 | 16403
16415 | 16384 | 0 | 16408 | 16387 | 16414
16416 | 16384 | 0 | 16408 | 16387 | 16414
16418 | 16389 | 16415 | 16408 | 16392 | 16417
16419 | 16389 | 16416 | 16408 | 16392 | 16417
16420 | 16408 | 0 | 16384 | 16387 | 16414
16421 | 16408 | 0 | 16384 | 16387 | 16414
16406 | 16394 | 16420 | 16384 | 16387 | 16400
16407 | 16394 | 16421 | 16384 | 16387 | 16400
(10 rows)
oid = 16401 and oid = 16402 has been deleted.
The two trigger tuples are deleted in tryAttachPartitionForeignKey called
by CloneFkReferencing.
/*
* Looks good! Attach this constraint. The action triggers in the new
* partition become redundant -- the parent table already has equivalent
* ones, and those will be able to reach the partition. Remove the ones
* in the partition. We identify them because they have our constraint
* OID, as well as being on the referenced rel.
*/
The attached patch can't fix above issue. I'm not sure about the impact of
this issue. Maybe redundant triggers no need removed.

But it surely make oidjoings.sql fail if I add test case into v2 patch, so
I don't add test case in v2 patch.
No test case is not good patch. I just share my idea about this issue. Hope
to get your reply.

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2023年10月25日周三 20:13写道：

Hi Alvaro,

Recently, Alexander reported the same issue on [1]/messages/by-id/18541-628a61bc267cd2d3@postgresql.org. And before that,
another same issue was reported on [2]/messages/by-id/GVAP278MB02787E7134FD691861635A8BC9032@GVAP278MB0278.CHEP278.PROD.OUTLOOK.COM.
So I try to re-work those issues. In my last email on this thread, I said
that
"
I slightly modified the previous patch，but I didn't add test case, because
I found another issue.
After done ALTER TABLE r ATTACH PARTITION r_1 FOR VALUES IN (1);
I run the oidjoins.sql and has warnings as belwo:
psql:/tender/postgres/src/test/regress/sql/oidjoins.sql:49: WARNING: FK
VIOLATION IN pg_trigger({tgparentid}): ("(0,3)",16401)
psql:/tender/postgres/src/test/regress/sql/oidjoins.sql:49: WARNING: FK
VIOLATION IN pg_trigger({tgparentid}): ("(0,4)",16402)
"

And I gave the explanation:
"
The two trigger tuples are deleted in tryAttachPartitionForeignKey called
by CloneFkReferencing.
/*
* Looks good! Attach this constraint. The action triggers in the new
* partition become redundant -- the parent table already has equivalent
* ones, and those will be able to reach the partition. Remove the ones
* in the partition. We identify them because they have our constraint
* OID, as well as being on the referenced rel.
*/
"
I try to fix above fk violation. I have two ideas.
i. Do not remove redundant, but when detaching parittion, the action
trigger on referenced side will be create again.
I have consider about this situation.

ii. We still remove redundant, and the remove the child action trigger,
too. If we do this way.
Should we create action trigger recursively on referced side when detaching
partition.

I can't decide which one is better. And I'm not sure that keep this FK
VIOLATION will cause some problem.
I rebase and send v3 patch, which only fix NOT FOUND INSERT CHECK TRIGGER.

[1]: /messages/by-id/18541-628a61bc267cd2d3@postgresql.org
/messages/by-id/18541-628a61bc267cd2d3@postgresql.org
[2]: /messages/by-id/GVAP278MB02787E7134FD691861635A8BC9032@GVAP278MB0278.CHEP278.PROD.OUTLOOK.COM
/messages/by-id/GVAP278MB02787E7134FD691861635A8BC9032@GVAP278MB0278.CHEP278.PROD.OUTLOOK.COM

--
Tender Wang

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)

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, connoinherit from 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)

--
Tender Wang

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年7月19日周五 21:18写道：

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.

I don't find the useful queries in your last email. Can you provide them.
Thanks.

--
Tender Wang

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, connoinherit from 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, connoinherit from 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]/messages/by-id/18541-628a61bc267cd2d3@postgresql.org 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]: /messages/by-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 from pg_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.