Subject: Re: PostgreSQL 15.10 update corrective action for ATTACH PARTITION/DETACH PARTITION

From: Tom Lane <[email-masked]>
Date: Wed, 27 Nov 2024 13:52:36 -0500

Paul Foerster <[email-masked]> writes:
> On 26 Nov 2024, at 22:25, Tom Lane <[email-masked]> 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

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 27 Nov 2024 20:01:09 +0100

> 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.

---

From: Paul Foerster <[email-masked]>
Date: Fri, 29 Nov 2024 07:58:38 +0100

Hi Tom, hi Alvaro,

> On 27 Nov 2024, at 19:52, Tom Lane <[email-masked]> wrote:
> 

great, thanks.

> 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

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 29 Nov 2024 18:15:35 +0100

> 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"

---

From: Paul Foerster <[email-masked]>
Date: Fri, 29 Nov 2024 21:24:17 +0100

Hi Alvaro,

> On 29 Nov 2024, at 18:15, Alvaro Herrera <[email-masked]> wrote:
> 

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?

> "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