public inbox for [email protected]
help / color / mirror / Atom feedRe: 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]>
2024-11-27 19:01 ` Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION Alvaro Herrera <[email protected]>
2024-11-29 06:58 ` Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION Paul Foerster <[email protected]>
2024-11-29 17:15 ` Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION Alvaro Herrera <[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 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]>
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-27 18:52 Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION Tom Lane <[email protected]>
@ 2024-11-29 06:58 ` Paul Foerster <[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-27 18:52 Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION Tom Lane <[email protected]>
@ 2024-11-29 17:15 ` Alvaro Herrera <[email protected]>
2024-11-29 20:24 ` Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION Paul Foerster <[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-27 18:52 Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION Tom Lane <[email protected]>
2024-11-29 17:15 ` Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION Alvaro Herrera <[email protected]>
@ 2024-11-29 20:24 ` Paul Foerster <[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