Re: a misbehavior of partition row movement (?)

On Wed, Jan 20, 2021 at 7:04 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Wed, Jan 20, 2021 at 4:13 PM Peter Eisentraut
> <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
> > On 2021-01-08 09:54, Amit Langote wrote:
> > >>> I don't quite recall if the decision to implement it like this was
> > >>> based on assuming that this is what users would like to see happen in
> > >>> this case or the perceived difficulty of implementing it the other way
> > >>> around, that is, of firing AFTER UPDATE triggers in this case.
> > >> I tried to look that up, but I couldn't find any discussion about this. Do you have any ideas in which thread that was handled?
> > > It was discussed here:
> > >
> > > https://www.postgresql.org/message-id/flat/CAJ3gD9do9o2ccQ7j7%2BtSgiE1REY65XRiMb%3DyJO3u3QhyP8EEPQ%40mail.gmail.com
> > >
> > > It's a huge discussion, so you'll have to ctrl+f "trigger" to spot
> > > relevant emails. You might notice that the developers who
> > > participated in that discussion gave various opinions and what we have
> > > today got there as a result of a majority of them voting for the
> > > current approach. Someone also said this during the discussion:
> > > "Regarding the trigger issue, I can't claim to have a terribly strong
> > > opinion on this. I think that practically anything we do here might
> > > upset somebody, but probably any halfway-reasonable thing we choose to
> > > do will be OK for most people." So what we've got is that
> > > "halfway-reasonable" thing, YMMV. :)
> >
> > Could you summarize here what you are trying to do with respect to what
> > was decided before? I'm a bit confused, looking through the patches you
> > have posted. The first patch you posted hard-coded FK trigger OIDs
> > specifically, other patches talk about foreign key triggers in general
> > or special case internal triggers or talk about all triggers.
>
> The original problem statement is this: the way we generally fire
> row-level triggers of a partitioned table can lead to some unexpected
> behaviors of the foreign keys pointing to that partitioned table
> during its cross-partition updates.
>
> Let's start with an example that shows how triggers are fired during a
> cross-partition update:
>
> create table p (a numeric primary key) partition by list (a);
> create table p1 partition of p for values in (1);
> create table p2 partition of p for values in (2);
> create or replace function report_trig_details() returns trigger as $$
> begin raise notice '% % on %', tg_when, tg_op, tg_relname; if tg_op =
> 'DELETE' then return old; end if; return new; end; $$ language
> plpgsql;
> create trigger trig1 before update on p for each row execute function
> report_trig_details();
> create trigger trig2 after update on p for each row execute function
> report_trig_details();
> create trigger trig3 before delete on p for each row execute function
> report_trig_details();
> create trigger trig4 after delete on p for each row execute function
> report_trig_details();
> create trigger trig5 before insert on p for each row execute function
> report_trig_details();
> create trigger trig6 after insert on p for each row execute function
> report_trig_details();
>
> insert into p values (1);
> update p set a = 2;
> NOTICE: BEFORE UPDATE on p1
> NOTICE: BEFORE DELETE on p1
> NOTICE: BEFORE INSERT on p2
> NOTICE: AFTER DELETE on p1
> NOTICE: AFTER INSERT on p2
> UPDATE 1
>
> (AR update triggers are not fired.)
>
> For contrast, here is an intra-partition update:
>
> update p set a = a;
> NOTICE: BEFORE UPDATE on p2
> NOTICE: AFTER UPDATE on p2
> UPDATE 1
>
> Now, the trigger machinery makes no distinction between user-defined
> and internal triggers, which has implications for the foreign key
> enforcing triggers on partitions. Consider the following example:
>
> create table q (a bigint references p);
> insert into q values (2);
> update p set a = 1;
> NOTICE: BEFORE UPDATE on p2
> NOTICE: BEFORE DELETE on p2
> NOTICE: BEFORE INSERT on p1
> ERROR: update or delete on table "p2" violates foreign key constraint
> "q_a_fkey2" on table "q"
> DETAIL: Key (a)=(2) is still referenced from table "q".
>
> So the RI delete trigger (NOT update) on p2 prevents the DELETE that
> occurs as part of the row movement. One might make the updates
> cascade and expect that to prevent the error:
>
> drop table q;
> create table q (a bigint references p on update cascade);
> insert into q values (2);
> update p set a = 1;
> NOTICE: BEFORE UPDATE on p2
> NOTICE: BEFORE DELETE on p2
> NOTICE: BEFORE INSERT on p1
> ERROR: update or delete on table "p2" violates foreign key constraint
> "q_a_fkey2" on table "q"
> DETAIL: Key (a)=(2) is still referenced from table "q".
>
> No luck, because again it's the RI delete trigger on p2 that gets
> fired. If you make deletes cascade too, an even worse thing happens:
>
> drop table q;
> create table q (a bigint references p on update cascade on delete cascade);
> insert into q values (2);
> update p set a = 1;
> NOTICE: BEFORE UPDATE on p2
> NOTICE: BEFORE DELETE on p2
> NOTICE: BEFORE INSERT on p1
> NOTICE: AFTER DELETE on p2
> NOTICE: AFTER INSERT on p1
> UPDATE 1
> select * from q;
> a
> ---
> (0 rows)
>
> The RI delete trigger deleted 2 from q, whereas the expected result
> would've been for q to be updated to change 2 to 1.

Thank you for a good summary. That's helpful to catch up on this thread.

>
> This shows that the way we've made these triggers behave in general
> can cause some unintended behaviors for foreign keys during
> cross-partition updates. I started this thread to do something about
> that and sent a patch to prevent cross-partition updates at all when
> there are foreign keys pointing to it. As others pointed out, that's
> not a great long-term solution to the problem, but that's what we may
> have to do in the back-branches if anything at all.

I've started by reviewing the patch for back-patching that the first
patch you posted[1].

+ for (i = 0; i < trigdesc->numtriggers; i++)
+ {
+ Trigger *trigger = &trigdesc->triggers[i];
+
+ if (trigger->tgisinternal &&
+ OidIsValid(trigger->tgconstrrelid) &&
+ trigger->tgfoid == F_RI_FKEY_CASCADE_DEL)
+ {
+ found = true;
+ break;
+ }
+ }

IIUC the above checks if the partition table is referenced by a
foreign key constraint on another table with ON DELETE CASCADE option.
I think we should prevent cross-partition update also when ON DELETE
SET NULL and ON DELETE SET DEFAULT. For example, with the patch, a
tuple in a partition table is still updated to NULL when
cross-partition update as follows:

postgres=# create table p (a numeric primary key) partition by list (a);
CREATE TABLE
postgres=# create table p1 partition of p for values in (1);
CREATE TABLE
postgres=# create table p2 partition of p for values in (2);
CREATE TABLE
postgres=# insert into p values (1);
INSERT 0 1
postgres=# create table q (a int references p(a) on delete set null);
CREATE TABLE
postgres=# insert into q values (1);
INSERT 0 1
postgres=# update p set a = 2;
UPDATE 1
postgres=# table p;
a
---
2
(1 row)

postgres=# table q;
a
---

(1 row)

Regards,

[1] https://www.postgresql.org/message-id/CA%2BHiwqFvkBCmfwkQX_yBqv2Wz8ugUGiBDxum8%3DWvVbfU1TXaNg%40mail.gmail.com

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/