Re: BUG #16139: Assertion fails on INSERT into a postgres_fdw' table with two AFTER INSERT triggers

On Tue, Dec 3, 2019 at 8:53 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> Attached is an updated version of the patch.

I noticed that the patch I proposed has an issue for some cases. Let
me explain. I assumed that the slots trig_tuple_slot1 and
trig_tuple_slot2 passed to AfterTriggerExecute() are NULL if the
target table is not a foreign table, from thiscomment for that
function:

* trig_tuple_slot1: scratch slot for tg_trigtuple (foreign tables only)
* trig_tuple_slot2: scratch slot for tg_newtuple (foreign tables only)

but actually that's not true. Consider an example:

postgres=# create table t1 (a int, b int);
postgres=# create table t2 (a int, b int);
postgres=# create server loopback foreign data wrapper postgres_fdw
options (dbname 'postgres');
postgres=# create user MAPPING FOR CURRENT_USER server loopback ;
postgres=# create foreign table ft1 (a int, b int) server loopback
options (table_name 't1');
postgres=# insert into t1 values (1, 1);
postgres=# create trigger trig_row_after_foreign after insert or
update or delete on ft1 for each row execute procedure trigger_data
(23, 'skidoo');
postgres=# create trigger trig_row_after_regular after insert or
update or delete on t2 for each row execute procedure trigger_data
(23, 'skidoo');

postgres=# with moved_rows as (update ft1 set a = 2, b = 2 where a = 1
returning *) insert into t2 select * from moved_rows;
NOTICE: trig_row_after_foreign(23, skidoo) AFTER ROW UPDATE ON ft1
NOTICE: OLD: (1,1),NEW: (2,2)
NOTICE: trig_row_after_regular(23, skidoo) AFTER ROW INSERT ON t2
NOTICE: NEW: (2,2)
INSERT 0 1

For this query, when executing the trigger trig_row_after_regular
(trigger on a regular table!) by AfterTriggerExecute(), the caller of
that function passes to it non-NULL trig_tuple_slot1 and
trig_tuple_slot2 made for the previous trigger trig_row_after_foreign.
And this causes AfterTriggerExecute() to incorrectly skip clearing
tg_trigtuple and tg_newtuple at the final step, because I made this
change to that function:

*** 4355,4364 **** AfterTriggerExecute(EState *estate,
if (should_free_new)
heap_freetuple(LocTriggerData.tg_newtuple);

! if (LocTriggerData.tg_trigslot)
! ExecClearTuple(LocTriggerData.tg_trigslot);
! if (LocTriggerData.tg_newslot)
! ExecClearTuple(LocTriggerData.tg_newslot);

/*
* If doing EXPLAIN ANALYZE, stop charging time to this trigger, and count
--- 4360,4373 ----
if (should_free_new)
heap_freetuple(LocTriggerData.tg_newtuple);

! /* don't clear slots' contents if foreign table */
! if (trig_tuple_slot1 == NULL)
! {
! if (LocTriggerData.tg_trigslot)
! ExecClearTuple(LocTriggerData.tg_trigslot);
! if (LocTriggerData.tg_newslot)
! ExecClearTuple(LocTriggerData.tg_newslot);
! }

To fix, I propose to modify the caller (ie,
afterTriggerInvokeEvents()) so that it sets the passed-in slots
trig_tuple_slot1 and trig_tuple_slot2 to NULL if the target table is
not a foreign table. Attached is an updated patch for that. I also
added the commit message. If no objections, I'll push the patch.

Best regards,
Etsuro Fujita