Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails - Mailing list pgsql-hackers

From Tender Wang
Subject Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
Date
Msg-id CAHewXN=pPz-8_qxUJuD5jhm3evXBeta1LgzW4PHArJ-iYz7qHQ@mail.gmail.com
Whole thread Raw
In response to [BUG] Fix DETACH with FK pointing to a partitioned table fails  (Jehan-Guillaume de Rorthais <jgdr@dalibo.com>)
List pgsql-hackers


Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年10月18日周五 22:52写道:
On 2024-Sep-26, Jehan-Guillaume de Rorthais wrote:

> REL_14_STABLE backport doesn't seem trivial, so I'll wait for some
> feedback, review & decision before going further down in backpatching.

Hi, thanks for these patches.  I have made some edits of my own.  In the
end I decided that I quite liked the new structure of the
addFkConstraint() function and friends.  I added a bunch of comments and
changed names somewhat.  Also, I think the benefit of the refactoring is
more obvious when all patches are squashed together, so I did that.

For branch 14, I opted to make it delete the constraints on detach.
This isn't ideal but unless somebody wants to spend a lot more time on
this, it seems the best we can do.  Leaving broken constraints around
seems worse.  The patch for 14 doesn't apply to 13 sadly.  I didn't have
time to verify why, but it seems there's some rather larger conflict in
one spot.

I hope to be able to get these pushed over the weekend.  That would give
us a couple of weeks before the November releases (but my availability
in those two weeks might be spotty.)

I spent some time going through your test additions and ended up with
your functions in this form:

-- displays constraints in schema fkpart12
CREATE OR REPLACE FUNCTION
fkpart12_constraints(OUT conrel regclass, OUT conname name,
                     OUT confrelid regclass, OUT conparent text)
        RETURNS SETOF record AS $$
  WITH RECURSIVE arrh AS (
       SELECT oid, conrelid, conname, confrelid, NULL::name AS conparent
         FROM pg_constraint
        WHERE connamespace = 'fkpart12'::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;
$$
LANGUAGE SQL;

-- displays triggers in tables in schema fkpart12
CREATE OR REPLACE FUNCTION
fkpart12_triggers(OUT tablename regclass, OUT constr text,
                  OUT samefunc boolean, OUT parent text)
        RETURNS SETOF record AS $$
  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) = 'fkpart12'::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 tablename, constr, samefunc, parent
      FROM arrh
  ORDER BY tablename::text, constr;
$$
LANGUAGE SQL;

However, in the end I think this is a very good technique to verify that
the fix works correctly, but it's excessive to include these results in
the tests forevermore.  So I've left them out for now.  Maybe we should
reconsider on the older branches?

Hi,

I looked at the patch on master.   I gave a little comment in [1]
I reconsider the codes.  I suspect that we don't need the below if statement anymore.
/*
* If the referenced side is partitioned (which we know because our
* parent's constraint points to a different relation than ours) then
* we must, in addition to the above, create pg_constraint rows that
* point to each partition, each with its own action triggers.
*/
if (parentConForm->conrelid != conform->conrelid)
{
...
}

The above contidion is always true according to my test.
I haven't figured out an anti-case.
Any thoughts?


--
Thanks,
Tender Wang

pgsql-hackers by date:

Previous
From: Alexander Korotkov
Date:
Subject: Re: type cache cleanup improvements
Next
From: Amit Kapila
Date:
Subject: Re: Make default subscription streaming option as Parallel