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

On Tue, Dec 3, 2019 at 8:53 PM Etsuro Fujita <etsuro.fujita@gmail.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