Feedback-ID: ia2694551:Fastmail
Date: Fri, 29 Nov 2024 18:15:35 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Paul Foerster <paul.foerster@gmail.com>, 
	Adrian Klaver <adrian.klaver@aklaver.com>, pgsql-general list <pgsql-general@lists.postgresql.org>
Subject: Re: PostgreSQL 15.10 update corrective action for ATTACH
 PARTITION/DETACH PARTITION
Message-ID: <202411291715.lwona3kp2xvv@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1900196.1732733556@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/202411291715.lwona3kp2xvv%40alvherre.pgsql>
Precedence: bulk

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"