Обсуждение: [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/
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
INSERT 0 1
postgres=# ALTER TABLE r DETACH PARTITION r_1;
ALTER TABLE
postgres=# delete from p_1 where id = 1;
DELETE 1
DELETE 1
postgres=# select * from r_1;
id | p_id
----+------
1 | 1
(1 row)
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".
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-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/
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写道:
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/
Вложения
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".
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写道:
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写道: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/
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写道:
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写道: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写道: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/
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写道:
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写道: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写道: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)
Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
От
Jehan-Guillaume de Rorthais
Дата:
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
Hi
Is there any conclusion to this issue?
Jehan-Guillaume de Rorthais <jgdr@dalibo.com> 于2023年8月10日周四 23:03写道:
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/
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)
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 | 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.
*/
* 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写道:
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,
Recently, Alexander reported the same issue on [1]. And before that, another same issue was reported on [2].
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)
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.
*/
* 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.
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)
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, 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.
Вложения
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".
--
Tender Wang
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)
Вложения
Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年8月8日周四 06:50写道:
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.
Thanks for your work. I will take some time to look it in detail.
Tender Wang
Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年8月8日周四 06:50写道:
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.
I did a lot of tests, and did not report error and did not find any warnings using oidjoins.sql.
+1
Tender Wang