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 <CAPmGK167zknpXrvBaJStUqxCK9RnQQ4CjXbyK3a_3uaUvdTbmg@mail.gmail.com> 
On Tue, Dec 3, 2019 at 8:53 PM Etsuro Fujita <[email protected]> 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
fix-AfterTriggerExecute-3.patch (application/octet-stream, 8.3 KB) 
From 271458940927232e886854086c4b4d8a3c8c0d03 Mon Sep 17 00:00:00 2001
From: Etsuro Fujita <[email protected]>
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
      </varlistentry>
 
      <varlistentry>
-      <term><structfield>tg_trigtuplebuf</structfield></term>
+      <term><structfield>tg_trigslot</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.
+        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_newtuplebuf</structfield></term>
+      <term><structfield>tg_newslot</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.
+        The slot containing <structfield>tg_newtuple</structfield>,
+        or a <symbol>NULL</symbol> pointer if there is no such tuple.
        </para>
       </listitem>
      </varlistentry>
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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.