Re: BUG #16139: Assertion fails on INSERT into a postgres_fdw' table with two AFTER INSERT triggers
Etsuro Fujita <[email protected]>
| Newsgroups | gmane.comp.db.postgresql.bugs |
|---|---|
| Message-ID | <CAPmGK17mQuwhiavHGbq6fKNXkr4sjTLvetBkv2rbiwz1D-DMYQ@mail.gmail.com> |
On Thu, Nov 28, 2019 at 8:25 PM Etsuro Fujita <[email protected]> wrote: > On Wed, Nov 27, 2019 at 8:35 PM Etsuro Fujita <[email protected]> wrote: > > A bit of investigation showed that this is caused by commit > > ff11e7f4b9ae017585c3ba146db7ba39c31f209a. I haven't yet looked at the > > commit in any detail, though. > > I spent some time studying the commit. Another minor thing I noticed about the commit is this: @@ -4281,31 +4266,38 @@ AfterTriggerExecute(AfterTriggerEvent event, * that is stored as a heap tuple, constructed in different memory * context, in the slot anyway. */ - LocTriggerData.tg_trigtuple = ExecFetchSlotHeapTuple(trig_tuple_slot1, - true, NULL); - LocTriggerData.tg_trigtuplebuf = InvalidBuffer; + LocTriggerData.tg_trigslot = trig_tuple_slot1; + LocTriggerData.tg_trigtuple = + ExecFetchSlotHeapTuple(trig_tuple_slot1, true, &should_free_trig); + LocTriggerData.tg_newslot = trig_tuple_slot2; LocTriggerData.tg_newtuple = ((evtshared->ats_event & TRIGGER_EVENT_OPMASK) == TRIGGER_EVENT_UPDATE) ? - ExecFetchSlotHeapTuple(trig_tuple_slot2, true, NULL) : NULL; - LocTriggerData.tg_newtuplebuf = InvalidBuffer; + ExecFetchSlotHeapTuple(trig_tuple_slot2, true, &should_free_new) : NULL; break; LocTriggerData.tg_newslot is always set to trig_tuple_slot2, but I think we should set it to trig_tuple_slot2 if UPDATE, NULL otherwise, to ensure that it's a NULL pointer if INSERT/DELETE, which I think trigger-function authors would assume. So I updated the patch. Attached is an updated version of the patch. Other changes: * The commit forgot to update the documentation on the trigger interface, so I updated it (as such). * I added a bit more regression test cases. Best regards, Etsuro Fujita
fix-AfterTriggerExecute-2.patch
(application/octet-stream, 6.7 KB)
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 6459,6464 **** DROP TRIGGER trig_row_after ON rem1;
--- 6459,6493 ----
DROP TRIGGER trig_stmt_before ON rem1;
DROP TRIGGER trig_stmt_after ON rem1;
DELETE from rem1;
+ CREATE TRIGGER trig_row_after1
+ AFTER INSERT OR UPDATE OR DELETE ON rem1
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+ CREATE TRIGGER trig_row_after2
+ AFTER INSERT OR UPDATE OR DELETE ON rem1
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+ insert into rem1 values(1,'insert');
+ NOTICE: trig_row_after1(23, skidoo) AFTER ROW INSERT ON rem1
+ NOTICE: NEW: (1,insert)
+ NOTICE: trig_row_after2(23, skidoo) AFTER ROW INSERT ON rem1
+ NOTICE: NEW: (1,insert)
+ update rem1 set f2 = 'update' where f1 = 1;
+ NOTICE: trig_row_after1(23, skidoo) AFTER ROW UPDATE ON rem1
+ NOTICE: OLD: (1,insert),NEW: (1,update)
+ NOTICE: trig_row_after2(23, skidoo) AFTER ROW UPDATE ON rem1
+ NOTICE: OLD: (1,insert),NEW: (1,update)
+ update rem1 set f2 = f2 || f2;
+ NOTICE: trig_row_after1(23, skidoo) AFTER ROW UPDATE ON rem1
+ NOTICE: OLD: (1,update),NEW: (1,updateupdate)
+ NOTICE: trig_row_after2(23, skidoo) AFTER ROW UPDATE ON rem1
+ NOTICE: OLD: (1,update),NEW: (1,updateupdate)
+ delete from rem1;
+ NOTICE: trig_row_after1(23, skidoo) AFTER ROW DELETE ON rem1
+ NOTICE: OLD: (1,updateupdate)
+ NOTICE: trig_row_after2(23, skidoo) AFTER ROW DELETE ON rem1
+ NOTICE: OLD: (1,updateupdate)
+ -- cleanup
+ DROP TRIGGER trig_row_after1 ON rem1;
+ DROP TRIGGER trig_row_after2 ON rem1;
-- Test WHEN conditions
CREATE TRIGGER trig_row_before_insupd
BEFORE INSERT OR UPDATE ON rem1
***************
*** 6689,6695 **** NOTICE: trig_row_after(23, skidoo) AFTER ROW INSERT ON rem1
NOTICE: NEW: (13,"test triggered !")
ctid
--------
! (0,29)
(1 row)
-- cleanup
--- 6718,6724 ----
NOTICE: NEW: (13,"test triggered !")
ctid
--------
! (0,32)
(1 row)
-- cleanup
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 1485,1490 **** DROP TRIGGER trig_stmt_after ON rem1;
--- 1485,1506 ----
DELETE from rem1;
+ CREATE TRIGGER trig_row_after1
+ AFTER INSERT OR UPDATE OR DELETE ON rem1
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+
+ CREATE TRIGGER trig_row_after2
+ AFTER INSERT OR UPDATE OR DELETE ON rem1
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+
+ insert into rem1 values(1,'insert');
+ update rem1 set f2 = 'update' where f1 = 1;
+ update rem1 set f2 = f2 || f2;
+ delete from rem1;
+
+ -- cleanup
+ DROP TRIGGER trig_row_after1 ON rem1;
+ DROP TRIGGER trig_row_after2 ON rem1;
-- Test WHEN conditions
*** a/doc/src/sgml/trigger.sgml
--- b/doc/src/sgml/trigger.sgml
***************
*** 511,518 **** typedef struct TriggerData
HeapTuple tg_trigtuple;
HeapTuple tg_newtuple;
Trigger *tg_trigger;
! Buffer tg_trigtuplebuf;
! Buffer tg_newtuplebuf;
Tuplestorestate *tg_oldtable;
Tuplestorestate *tg_newtable;
} TriggerData;
--- 511,518 ----
HeapTuple tg_trigtuple;
HeapTuple tg_newtuple;
Trigger *tg_trigger;
! TupleTableSlot *tg_trigslot;
! TupleTableSlot *tg_newslot;
Tuplestorestate *tg_oldtable;
Tuplestorestate *tg_newtable;
} TriggerData;
***************
*** 714,734 **** typedef struct Trigger
</varlistentry>
<varlistentry>
! <term><structfield>tg_trigtuplebuf</structfield></term>
<listitem>
<para>
! The buffer containing <structfield>tg_trigtuple</structfield>, or <symbol>InvalidBuffer</symbol> if there
! is no such tuple or it is not stored in a disk buffer.
</para>
</listitem>
</varlistentry>
<varlistentry>
! <term><structfield>tg_newtuplebuf</structfield></term>
<listitem>
<para>
! The buffer containing <structfield>tg_newtuple</structfield>, or <symbol>InvalidBuffer</symbol> if there
! is no such tuple or it is not stored in a disk buffer.
</para>
</listitem>
</varlistentry>
--- 714,734 ----
</varlistentry>
<varlistentry>
! <term><structfield>tg_trigslot</structfield></term>
<listitem>
<para>
! The slot containing <structfield>tg_trigtuple</structfield>,
! or a <symbol>NULL</symbol> pointer if there is no such tuple.
</para>
</listitem>
</varlistentry>
<varlistentry>
! <term><structfield>tg_newslot</structfield></term>
<listitem>
<para>
! The slot containing <structfield>tg_newtuple</structfield>,
! or a <symbol>NULL</symbol> pointer if there is no such tuple.
</para>
</listitem>
</varlistentry>
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
***************
*** 4255,4266 **** AfterTriggerExecute(EState *estate,
LocTriggerData.tg_trigtuple =
ExecFetchSlotHeapTuple(trig_tuple_slot1, true, &should_free_trig);
! LocTriggerData.tg_newslot = trig_tuple_slot2;
! LocTriggerData.tg_newtuple =
! ((evtshared->ats_event & TRIGGER_EVENT_OPMASK) ==
! TRIGGER_EVENT_UPDATE) ?
! ExecFetchSlotHeapTuple(trig_tuple_slot2, true, &should_free_new) : NULL;
!
break;
default:
--- 4255,4271 ----
LocTriggerData.tg_trigtuple =
ExecFetchSlotHeapTuple(trig_tuple_slot1, true, &should_free_trig);
! if ((evtshared->ats_event & TRIGGER_EVENT_OPMASK) ==
! TRIGGER_EVENT_UPDATE)
! {
! LocTriggerData.tg_newslot = trig_tuple_slot2;
! LocTriggerData.tg_newtuple =
! ExecFetchSlotHeapTuple(trig_tuple_slot2, true, &should_free_new);
! }
! else
! {
! LocTriggerData.tg_newtuple = NULL;
! }
break;
default:
***************
*** 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);
! }
/*
* If doing EXPLAIN ANALYZE, stop charging time to this trigger, and count