Thread: ALTER CONSTRAINT on a partitioned FK isn't working

I reproduced the problem shown in [1]:

--- snip ---
drop table if exists pt, ref;

create table pt(f1 int, f2 int, f3 int, primary key(f1,f2))
  partition by list(f1);
create table pt1 partition of pt for values in (1);
create table pt2 partition of pt for values in (2);

create table ref(f1 int, f2 int, f3 int)
  partition by list(f1);
create table ref1 partition of ref for values in (1);
create table ref2 partition of ref for values in (2);

alter table ref add foreign key(f1,f2) references pt;

alter table ref alter constraint ref_f1_f2_fkey
  deferrable initially deferred;

insert into pt values(1,2,3);
insert into ref values(1,2,3);

delete from pt;  -- expected to fail

begin;
delete from pt;  -- this should work, but does not
delete from ref;
abort;
--- snip ---

But if you create the FK in one step with

alter table ref add foreign key(f1,f2) references pt
  deferrable initially deferred;

then everything behaves as expected.  So something is broken
about propagating deferred-ness to partition triggers in an
ALTER CONSTRAINT.  Oddly, it *looks* like it worked if you
examine the child tables with "\d+".  I surmise that ALTER CONSTRAINT
fixes whatever catalog fields psql looks at, but there's some other
fields that also need to be updated and aren't being.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/75fe0761-a291-86a9-c8d8-4906da077469%40gmail.com

On 2020-Dec-07, Tom Lane wrote:

> then everything behaves as expected.  So something is broken
> about propagating deferred-ness to partition triggers in an
> ALTER CONSTRAINT.  Oddly, it *looks* like it worked if you
> examine the child tables with "\d+".  I surmise that ALTER CONSTRAINT
> fixes whatever catalog fields psql looks at, but there's some other
> fields that also need to be updated and aren't being.

Yeah, this query shows that the tgdeferrable and tginitdeferred columns
ought to be dropped:

select tgname, tgdeferrable, tginitdeferred from pg_trigger;

These are all 'true' when the constraint is created correctly, and false
when changed by ALTER CONSTRAINT.

Let me see about fixing that ...

On 2020-Dec-07, Alvaro Herrera wrote:

> Yeah, this query shows that the tgdeferrable and tginitdeferred columns
> ought to be dropped:
> 
> select tgname, tgdeferrable, tginitdeferred from pg_trigger;

Sorry, I meant "ought to be updated".  But there's more to it than that:
the pg_constraint entries themselves are not updated, and that's because
ALTER CONSTRAINT does not have recurse at all.  So the first thing to do
is add an "ATSimpleRecursion()" call for the appropriate case, but even
that is not sufficient, as we need to recurse on the referenced side
also, not just the referencing side -- and that's a tad more involved.
(ATExecAlterConstraint is explicitly not handling the case.)

On 2020-Dec-07, Tom Lane wrote:

> then everything behaves as expected.  So something is broken
> about propagating deferred-ness to partition triggers in an
> ALTER CONSTRAINT.  Oddly, it *looks* like it worked if you
> examine the child tables with "\d+".  I surmise that ALTER CONSTRAINT
> fixes whatever catalog fields psql looks at, but there's some other
> fields that also need to be updated and aren't being.

I came up with this.  As I mentioned in my earlier reply, handling
recursion in the usual way doesn't fix the whole problem, because we
need to recurse on possibly both sides of the constraint.  So I made it
recurse using pg_constraint.conparentid, which seems easier and cover
both ends.  This is pretty crude so far but it handles the trivial
cases.  What I did is split the existing routine in two, and the "inner"
part now recurses on itself if it sees that either table is partitioned.

I'll polish it tomorrow -- intend to get this pushed to branches back to
11, depending on what's needed.

-- 
Álvaro Herrera       Valdivia, Chile

It turns out that more work is needed here to ensure we recurse to all
children on both sides, and so that all involved triggers are altered.

There is a case where it doesn't seem like we can easily give
non-surprising behavior: when multi-level partitioning is used, a
constraint is defined in the top level and then it is altered in the
middle level, then we don't have access to the action triggers, because
those are defined at the parent level.  I opted for adding a WARNING
message in that case.  Other options were 1) to raise an error if
altering a constraint that's not top level, but I fear that might break
restoring dumps of existing database; 2) reach up and alter the parent
constraint instead, but that seems too magical.  This doesn't seem worth
spending too much sweat on, since it's a pretty fringe case anyway.

Patch for HEAD attached.


I also noticed that we're calling InvokeObjectPostAlterHook wrong in one
place: when altering the trigger properties, we call it with the
constraint OID instead of the trigger OID.  I'll fix that in a separate
commit -- that was introduced by 578b229718e8 ("Remove WITH OIDS
support, change oid catalog column visibility.") in pg12.  It's sad that
nobody ever noticed ... looks like the sepgsql stuff isn't getting much
testing.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

This backpatches to 12 and 13 cleanly, but 11 needs a completely
different patch -- attached.


-- 
Álvaro Herrera       Valdivia, Chile
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)

On 2021-May-03, Alvaro Herrera wrote:

> There is a case where it doesn't seem like we can easily give
> non-surprising behavior: when multi-level partitioning is used, a
> constraint is defined in the top level and then it is altered in the
> middle level, then we don't have access to the action triggers, because
> those are defined at the parent level.  I opted for adding a WARNING
> message in that case.  Other options were 1) to raise an error if
> altering a constraint that's not top level, but I fear that might break
> restoring dumps of existing database;

Actually, I tested this and it turns out to be a nonexistant problem: if
you alter a constraint that's not top-level, then pg_dump silently omits
the deferrability change there.  So we can just have this emit ERROR and
nothing that works today would break.  Users *would* get an error, but
then the alteration would not have the desired effect anyway.  I suppose
we would have already got complaints if anybody were trying.  (The error
message could stand some wording improvement ...)

So the attached version does that and I intend to get it pushed to all
branches (since 11) this afternoon unless I get any objections.

I'm CC'ing Ron as the reporter of the original bug.

-- 
Álvaro Herrera       Valdivia, Chile

On 2021-May-04, Alvaro Herrera wrote:

> So the attached version does that and I intend to get it pushed to all
> branches (since 11) this afternoon unless I get any objections.

Pushed it now, with some further adjustments.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W