public inbox for [email protected]help / color / mirror / Atom feed
Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION 5+ messages / 3 participants [nested] [flat]
* Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION @ 2024-11-27 18:52 Tom Lane <[email protected]> 0 siblings, 3 replies; 5+ messages in thread From: Tom Lane @ 2024-11-27 18:52 UTC (permalink / raw) To: Paul Foerster <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Adrian Klaver <[email protected]>; pgsql-general list <[email protected]> Paul Foerster <[email protected]> writes: > On 26 Nov 2024, at 22:25, Tom Lane <[email protected]> wrote: >> I'm suspicious that our repair recipe might not have accounted >> for self-reference FKs fully, but that's just a gut feeling at >> this point. > Of course, it contains no secret data. Please find the full log below. According to the add constraint statement, it is a self reference. > Thanks for looking into it. Okay, so I was able to reproduce this from scratch on HEAD: regression=# create table p_ci_pipelines(partition_id int, id int, primary key(partition_id,id), auto_canceled_by_partition_id int, auto_canceled_by_id int) partition by LIST (partition_id); CREATE TABLE regression=# create table ci_pipelines partition of p_ci_pipelines FOR VALUES IN ('100', '101', '102'); CREATE TABLE regression=# ALTER TABLE p_ci_pipelines ADD CONSTRAINT fk_262d4c2d19_p FOREIGN KEY (auto_canceled_by_partition_id, auto_canceled_by_id) REFERENCES p_ci_pipelines(partition_id, id) ON UPDATE CASCADE ON DELETE SET NULL; ALTER TABLE regression=# SELECT conrelid::pg_catalog.regclass AS "constrained table", conname AS constraint, confrelid::pg_catalog.regclass AS "references", pg_catalog.format('ALTER TABLE %s DROP CONSTRAINT %I;', conrelid::pg_catalog.regclass, conname) AS "drop", pg_catalog.format('ALTER TABLE %s ADD CONSTRAINT %I %s;', conrelid::pg_catalog.regclass, conname, pg_catalog.pg_get_constraintdef(oid)) AS "add" FROM pg_catalog.pg_constraint c WHERE contype = 'f' AND conparentid = 0 AND (SELECT count(*) FROM pg_catalog.pg_constraint c2 WHERE c2.conparentid = c.oid) <> (SELECT count(*) FROM pg_catalog.pg_inherits i WHERE (i.inhparent = c.conrelid OR i.inhparent = c.confrelid) AND EXISTS (SELECT 1 FROM pg_catalog.pg_partitioned_table WHERE partrelid = i.inhparent)); constrained table | constraint | references | drop | add -------------------+-----------------+----------------+-------------------------------------------------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- p_ci_pipelines | fk_262d4c2d19_p | p_ci_pipelines | ALTER TABLE p_ci_pipelines DROP CONSTRAINT fk_262d4c2d19_p; | ALTER TABLE p_ci_pipelines ADD CONSTRAINT fk_262d4c2d19_p FOREIGN KEY (auto_canceled_by_partition_id, auto_canceled_by_id) REFERENCES p_ci_pipelines(partition_id, id) ON UPDATE CASCADE ON DELETE SET NULL; (1 row) I doubt that there's anything actually wrong with the catalog state at this point (perhaps Alvaro would confirm that). That leads to the conclusion that what's wrong is the release notes' query for fingering broken constraints, and it needs some additional test to avoid complaining about (I suspect) self-reference cases. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION @ 2024-11-27 19:01 Alvaro Herrera <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 0 replies; 5+ messages in thread From: Alvaro Herrera @ 2024-11-27 19:01 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Paul Foerster <[email protected]>; Adrian Klaver <[email protected]>; pgsql-general list <[email protected]> On 2024-Nov-27, Tom Lane wrote: > I doubt that there's anything actually wrong with the catalog state at > this point (perhaps Alvaro would confirm that). That leads to the > conclusion that what's wrong is the release notes' query for fingering > broken constraints, and it needs some additional test to avoid > complaining about (I suspect) self-reference cases. Ugh, I hadn't noticed this report, thanks for CCing me. I'll have a look at this tomorrow. You're right that I didn't think of checking self-referencing FKs with the query. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ <Schwern> It does it in a really, really complicated way <crab> why does it need to be complicated? <Schwern> Because it's MakeMaker. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION @ 2024-11-29 06:58 Paul Foerster <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 0 replies; 5+ messages in thread From: Paul Foerster @ 2024-11-29 06:58 UTC (permalink / raw) To: Tom Lane <[email protected]>; Alvaro Herrera <[email protected]>; +Cc: Adrian Klaver <[email protected]>; pgsql-general list <[email protected]> Hi Tom, hi Alvaro, > On 27 Nov 2024, at 19:52, Tom Lane <[email protected]> wrote: > > Okay, so I was able to reproduce this from scratch on HEAD: great, thanks. > I doubt that there's anything actually wrong with the catalog state at > this point (perhaps Alvaro would confirm that). That leads to the > conclusion that what's wrong is the release notes' query for fingering > broken constraints, and it needs some additional test to avoid > complaining about (I suspect) self-reference cases. In the meantime, I updated the whole company. The one test database actually was the only database that this was returned. I found no other occurrences. As I understand it, the worst thing that could happen is that one or more rows end up in a detached partition table which should actually be in another partition, right? Since there were no rows, no harm could have been done. Also, since this is a self reference, the wrong table is also the right one. Again, thanks very much for clarifying this. Cheers Paul ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION @ 2024-11-29 17:15 Alvaro Herrera <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 1 reply; 5+ messages in thread From: Alvaro Herrera @ 2024-11-29 17:15 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Paul Foerster <[email protected]>; Adrian Klaver <[email protected]>; pgsql-general list <[email protected]> On 2024-Nov-27, Tom Lane wrote: > I doubt that there's anything actually wrong with the catalog state at > this point (perhaps Alvaro would confirm that). That leads to the > conclusion that what's wrong is the release notes' query for fingering > broken constraints, and it needs some additional test to avoid > complaining about (I suspect) self-reference cases. Yes, I think the catalog state is correct and the release notes query is wrong. I propose a repaired version below. But first, I think there's still a problem specific to partition creation when a self-referencing FKs exists. If you do create table / create partition / add FK, then the query from the release notes does report the FK. But if you do create table / add FK / create partition, nothing is reported. Clearly, both those things cannot be simultaneously correct. -- Case 1: create the partition when the FK already exists drop table if exists p_ci_pipelines; create table p_ci_pipelines(partition_id int, id int, primary key(partition_id,id), auto_canceled_by_partition_id int, auto_canceled_by_id int) partition by LIST (partition_id); ALTER TABLE p_ci_pipelines ADD CONSTRAINT fk_262d4c2d19_p FOREIGN KEY (auto_canceled_by_partition_id, auto_canceled_by_id) REFERENCES p_ci_pipelines (partition_id, id) ON UPDATE CASCADE ON DELETE SET NULL; create table ci_pipelines partition of p_ci_pipelines FOR VALUES IN ('100', '101', '102'); -- Case 2: create both tables, then add the FK drop table if exists p_ci_pipelines; create table p_ci_pipelines(partition_id int, id int, primary key(partition_id,id), auto_canceled_by_partition_id int, auto_canceled_by_id int) partition by LIST (partition_id); create table ci_pipelines partition of p_ci_pipelines FOR VALUES IN ('100', '101', '102'); ALTER TABLE p_ci_pipelines ADD CONSTRAINT fk_262d4c2d19_p FOREIGN KEY (auto_canceled_by_partition_id, auto_canceled_by_id) REFERENCES p_ci_pipelines (partition_id, id) ON UPDATE CASCADE ON DELETE SET NULL; Naturally, if in any of those situations you drop and recreate the FK, it degenerates to case 2, so if you do what the release notes say, it'll continue to report the FK. We can use the following query (which lists the constraint and its derivate pg_constraint rows) to see what goes wrong: WITH RECURSIVE arrh AS ( SELECT oid, conrelid, conname, confrelid, NULL::name AS conparent FROM pg_constraint WHERE connamespace = 'public'::regnamespace AND contype = 'f' AND conparentid = 0 UNION ALL SELECT c.oid, c.conrelid, c.conname, c.confrelid, (pg_identify_object('pg_constraint'::regclass, arrh.oid, 0)).identity FROM pg_constraint c JOIN arrh ON c.conparentid = arrh.oid ) SELECT conrelid::regclass, conname, confrelid::regclass, conparent FROM arrh ORDER BY conrelid::regclass::text, conname; For case 2, this is the result: conrelid │ conname │ confrelid │ conparent ────────────────┼─────────────────────────────────────────────────────────────────┼────────────────┼────────────────────────────────────────── ci_pipelines │ fk_262d4c2d19_p │ p_ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines p_ci_pipelines │ fk_262d4c2d19_p │ p_ci_pipelines │ p_ci_pipelines │ p_ci_pipelines_auto_canceled_by_partition_id_auto_canceled_fkey │ ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines For case 1, where the release notes query reports nothing, we get the following list of constraints instead: conrelid │ conname │ confrelid │ conparent ────────────────┼─────────────────┼────────────────┼────────────────────────────────────────── ci_pipelines │ fk_262d4c2d19_p │ p_ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines p_ci_pipelines │ fk_262d4c2d19_p │ p_ci_pipelines │ (2 filas) Let's look at the triggers. For case 1 we have the following triggers: WITH RECURSIVE arrh AS ( SELECT t.oid, t.tgrelid::regclass as tablename, tgname, t.tgfoid::regproc as trigfn, (pg_identify_object('pg_constraint'::regclass, c.oid, 0)).identity as constr, NULL::bool as samefunc, NULL::name AS parent FROM pg_trigger t LEFT JOIN pg_constraint c ON c.oid = t.tgconstraint WHERE (SELECT relnamespace FROM pg_class WHERE oid = t.tgrelid) = 'public'::regnamespace AND c.contype = 'f' AND t.tgparentid = 0 UNION ALL SELECT t2.oid, t2.tgrelid::regclass as tablename, t2.tgname, t2.tgfoid::regproc as trigfn, (pg_identify_object('pg_constraint'::regclass, c2.oid, 0)).identity, arrh.trigfn = t2.tgfoid as samefunc, replace((pg_identify_object('pg_trigger'::regclass, t2.tgparentid, 0)).identity, t2.tgparentid::text, 'TGOID') FROM pg_trigger t2 LEFT JOIN pg_constraint c2 ON c2.oid = t2.tgconstraint JOIN arrh ON t2.tgparentid = arrh.oid ) SELECT tgname, tablename, constr, samefunc, parent FROM arrh ORDER BY tablename::text, constr; tgname │ tablename │ constr │ samefunc │ parent ──────────────────────────────┼────────────────┼──────────────────────────────────────────┼──────────┼───────────────────────────────────────────────────────── RI_ConstraintTrigger_c_16659 │ ci_pipelines │ fk_262d4c2d19_p on public.ci_pipelines │ t │ "RI_ConstraintTrigger_c_TGOID" on public.p_ci_pipelines RI_ConstraintTrigger_c_16658 │ ci_pipelines │ fk_262d4c2d19_p on public.ci_pipelines │ t │ "RI_ConstraintTrigger_c_TGOID" on public.p_ci_pipelines RI_ConstraintTrigger_a_16648 │ p_ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines │ │ RI_ConstraintTrigger_a_16649 │ p_ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines │ │ RI_ConstraintTrigger_c_16650 │ p_ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines │ │ RI_ConstraintTrigger_c_16651 │ p_ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines │ │ (6 filas) For case 2 we have this: tgname │ tablename │ constr │ samefunc │ parent ──────────────────────────────┼────────────────┼──────────────────────────────────────────────────────────────────────────────────────────┼──────────┼───────────────────────────────────────────────────────── RI_ConstraintTrigger_c_16680 │ ci_pipelines │ fk_262d4c2d19_p on public.ci_pipelines │ t │ "RI_ConstraintTrigger_c_TGOID" on public.p_ci_pipelines RI_ConstraintTrigger_c_16679 │ ci_pipelines │ fk_262d4c2d19_p on public.ci_pipelines │ t │ "RI_ConstraintTrigger_c_TGOID" on public.p_ci_pipelines RI_ConstraintTrigger_a_16675 │ ci_pipelines │ p_ci_pipelines_auto_canceled_by_partition_id_auto_canceled_fkey on public.p_ci_pipelines │ t │ "RI_ConstraintTrigger_a_TGOID" on public.p_ci_pipelines RI_ConstraintTrigger_a_16674 │ ci_pipelines │ p_ci_pipelines_auto_canceled_by_partition_id_auto_canceled_fkey on public.p_ci_pipelines │ t │ "RI_ConstraintTrigger_a_TGOID" on public.p_ci_pipelines RI_ConstraintTrigger_a_16671 │ p_ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines │ │ RI_ConstraintTrigger_a_16672 │ p_ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines │ │ RI_ConstraintTrigger_c_16676 │ p_ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines │ │ RI_ConstraintTrigger_c_16677 │ p_ci_pipelines │ fk_262d4c2d19_p on public.p_ci_pipelines │ │ (8 filas) Here, the difference is the two action triggers on the partition, which hang under the secondary constraint for the partition. And those are critical, because without them, the ON DELETE clause is not executed: insert into ci_pipelines values (100, 1, null, null); -- create referenced row insert into ci_pipelines values (101, 2, 100, 1); -- create the reference delete from ci_pipelines where id = 1; -- should SET NULL but doesn't select * from p_ci_pipelines ; partition_id │ id │ auto_canceled_by_partition_id │ auto_canceled_by_id ──────────────┼────┼───────────────────────────────┼───────────────────── 101 │ 2 │ 100 │ 1 (1 fila) (Obviously if we drop the constraint at this point and try to recreate, it'll complain that the referenced row doesn't exist). This doesn't happen with the tables defined as case 2; the FK columns are set to NULL, as intended. partition_id │ id │ auto_canceled_by_partition_id │ auto_canceled_by_id ──────────────┼────┼───────────────────────────────┼───────────────────── 101 │ 2 │ │ (1 fila) This all was to say that the query in the release notes is undoubtedly wrong. After thinking some more about it, I think the fix is to add 1 to the number of constraints: SELECT conrelid::pg_catalog.regclass AS "constrained table", conname AS constraint, confrelid::pg_catalog.regclass AS "references", pg_catalog.format('ALTER TABLE %s DROP CONSTRAINT %I;', conrelid::pg_catalog.regclass, conname) AS "drop", pg_catalog.format('ALTER TABLE %s ADD CONSTRAINT %I %s;', conrelid::pg_catalog.regclass, conname, pg_catalog.pg_get_constraintdef(oid)) AS "add" FROM pg_catalog.pg_constraint c WHERE contype = 'f' AND conparentid = 0 AND (SELECT count(*) FROM pg_catalog.pg_constraint c2 WHERE c2.conparentid = c.oid) <> ((SELECT count(*) FROM pg_catalog.pg_inherits i WHERE (i.inhparent = c.conrelid OR i.inhparent = c.confrelid) AND EXISTS (SELECT 1 FROM pg_catalog.pg_partitioned_table WHERE partrelid = i.inhparent)) + CASE when pg_partition_root(conrelid) = confrelid THEN 1 ELSE 0 END); This reports case 2 as OK and case 1 as bogus, as should be. I tried adding more partitions and this seems to hold correctly. I was afraid though that this would fail if we create an FK in an intermediate level of the partition hierarchy ... but experimentation doesn't seem to give that result. I've run out of time today to continue to look though. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "La vida es para el que se aventura" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION @ 2024-11-29 20:24 Paul Foerster <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Paul Foerster @ 2024-11-29 20:24 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Tom Lane <[email protected]>; Adrian Klaver <[email protected]>; pgsql-general list <[email protected]> Hi Alvaro, > On 29 Nov 2024, at 18:15, Alvaro Herrera <[email protected]> wrote: > > This all was to say that the query in the release notes is undoubtedly > wrong. After thinking some more about it, I think the fix is to add 1 > to the number of constraints: > > SELECT conrelid::pg_catalog.regclass AS "constrained table", > conname AS constraint, > confrelid::pg_catalog.regclass AS "references", > pg_catalog.format('ALTER TABLE %s DROP CONSTRAINT %I;', > conrelid::pg_catalog.regclass, conname) AS "drop", > pg_catalog.format('ALTER TABLE %s ADD CONSTRAINT %I %s;', > conrelid::pg_catalog.regclass, conname, > pg_catalog.pg_get_constraintdef(oid)) AS "add" > FROM pg_catalog.pg_constraint c > WHERE contype = 'f' AND conparentid = 0 AND > (SELECT count(*) FROM pg_catalog.pg_constraint c2 > WHERE c2.conparentid = c.oid) <> > ((SELECT count(*) FROM pg_catalog.pg_inherits i > WHERE (i.inhparent = c.conrelid OR i.inhparent = c.confrelid) AND > EXISTS (SELECT 1 FROM pg_catalog.pg_partitioned_table > WHERE partrelid = i.inhparent)) + > CASE when pg_partition_root(conrelid) = confrelid THEN 1 ELSE 0 END); > > This reports case 2 as OK and case 1 as bogus, as should be. I tried > adding more partitions and this seems to hold correctly. I was afraid > though that this would fail if we create an FK in an intermediate level > of the partition hierarchy ... but experimentation doesn't seem to give > that result. I've run out of time today to continue to look though. Thanks very much for this really detailed analysis and sharing your insights. I'll give the new query a try on Monday when I'm back at work. Do I also need to recheck all other databases with this new query which didn't report anything with the original query? > Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ > "La vida es para el que se aventura" You're located in the middle of the forest east of Freiburg im Breisgau in Germany? 🤣 Cheers, Paul ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-11-29 20:24 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-11-27 18:52 Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION Tom Lane <[email protected]> 2024-11-27 19:01 ` Alvaro Herrera <[email protected]> 2024-11-29 06:58 ` Paul Foerster <[email protected]> 2024-11-29 17:15 ` Alvaro Herrera <[email protected]> 2024-11-29 20:24 ` Paul Foerster <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox