From 271458940927232e886854086c4b4d8a3c8c0d03 Mon Sep 17 00:00:00 2001 From: Etsuro Fujita Date: Fri, 6 Dec 2019 17:15:07 +0900 Subject: [PATCH] Fix handling of multiple AFTER ROW triggers on foreign tables. AfterTriggerExecute() retrieves a fresh tuple or pair of tuples from a tuplestore and then stores the tuple(s) in the passed-in slot(s) if AFTER_TRIGGER_FDW_FETCH, while it uses the most-recently-retrieved tuple(s) stored in the slot(s) if AFTER_TRIGGER_FDW_REUSE. This was done correctly before v12, but commit ff11e7f4b broke it by mistakenly clearing the tuple(s) stored in the slot(s) in that function, leading to an assertion failure, as reported in bug #16139 from Alexander Lakhin. (In non-assertion-enabled builds, this bug would crash the backend or cause wrong results.) Also, for the TriggerData's slot tg_newslot, which stores new updated tuples, the aforementioned commit didn't ensure it is a NULL pointer if there is no such tuple. Also, the aforementioned commit changed the TriggerData struct, but failed to update the documentation to match. Backpatch-through: 12 Discussion: https://postgr.es/m/16139-94f9ccf0db6119ec%40postgresql.org --- .../postgres_fdw/expected/postgres_fdw.out | 32 ++++++++++++++- contrib/postgres_fdw/sql/postgres_fdw.sql | 17 ++++++++ doc/src/sgml/trigger.sgml | 16 ++++---- src/backend/commands/trigger.c | 40 ++++++++++++------- 4 files changed, 81 insertions(+), 24 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 14180ee469..cc06670c59 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -6459,6 +6459,36 @@ DROP TRIGGER trig_row_after ON rem1; DROP TRIGGER trig_stmt_before ON rem1; DROP TRIGGER trig_stmt_after ON rem1; DELETE from rem1; +-- Test multiple AFTER ROW triggers on a foreign table +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,7 +6719,7 @@ NOTICE: trig_row_after(23, skidoo) AFTER ROW INSERT ON rem1 NOTICE: NEW: (13,"test triggered !") ctid -------- - (0,29) + (0,32) (1 row) -- cleanup diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 1c5c37b783..ecfa43efc8 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -1485,6 +1485,23 @@ DROP TRIGGER trig_stmt_after ON rem1; DELETE from rem1; +-- Test multiple AFTER ROW triggers on a foreign table +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 diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml index f62f420c19..ba94acad69 100644 --- a/doc/src/sgml/trigger.sgml +++ b/doc/src/sgml/trigger.sgml @@ -511,8 +511,8 @@ typedef struct TriggerData HeapTuple tg_trigtuple; HeapTuple tg_newtuple; Trigger *tg_trigger; - Buffer tg_trigtuplebuf; - Buffer tg_newtuplebuf; + TupleTableSlot *tg_trigslot; + TupleTableSlot *tg_newslot; Tuplestorestate *tg_oldtable; Tuplestorestate *tg_newtable; } TriggerData; @@ -714,21 +714,21 @@ typedef struct Trigger - tg_trigtuplebuf + tg_trigslot - The buffer containing tg_trigtuple, or InvalidBuffer if there - is no such tuple or it is not stored in a disk buffer. + The slot containing tg_trigtuple, + or a NULL pointer if there is no such tuple. - tg_newtuplebuf + tg_newslot - The buffer containing tg_newtuple, or InvalidBuffer if there - is no such tuple or it is not stored in a disk buffer. + The slot containing tg_newtuple, + or a NULL pointer if there is no such tuple. diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 8d22d43bc3..faeea16d21 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -4255,12 +4255,17 @@ 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; - + 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,10 +4360,14 @@ 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); + /* 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 @@ -4512,13 +4521,14 @@ afterTriggerInvokeEvents(AfterTriggerEventList *events, trigdesc = rInfo->ri_TrigDesc; finfo = rInfo->ri_TrigFunctions; instr = rInfo->ri_TrigInstrument; + if (slot1 != NULL) + { + ExecDropSingleTupleTableSlot(slot1); + ExecDropSingleTupleTableSlot(slot2); + slot1 = slot2 = NULL; + } if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) { - if (slot1 != NULL) - { - ExecDropSingleTupleTableSlot(slot1); - ExecDropSingleTupleTableSlot(slot2); - } slot1 = MakeSingleTupleTableSlot(rel->rd_att, &TTSOpsMinimalTuple); slot2 = MakeSingleTupleTableSlot(rel->rd_att, -- 2.19.2