Fix concurrent update trigger issues with MERGE in a CTE.

If a MERGE inside a CTE attempts an UPDATE or DELETE on a table with
BEFORE ROW triggers, and a concurrent UPDATE or DELETE happens, the
merge code would fail (crashing in the case of an UPDATE action, and
potentially executing the wrong action for a DELETE action).

This is the same issue that 9321c79c86 attempted to fix, except now
for a MERGE inside a CTE. As noted in 9321c79c86, what needs to happen
is for the trigger code to exit early, returning the TM_Result and
TM_FailureData information to the merge code, if a concurrent
modification is detected, rather than attempting to do an EPQ
recheck. The merge code will then do its own rechecking, and rescan
the action list, potentially executing a different action in light of
the concurrent update. In particular, the trigger code must never call
ExecGetUpdateNewTuple() for MERGE, since that is bound to fail because
MERGE has its own per-action projection information.

Commit 9321c79c86 did this using estate->es_plannedstmt->commandType
in the trigger code to detect that a MERGE was being executed, which
is fine for a plain MERGE command, but does not work for a MERGE
inside a CTE. Fix by passing that information to the trigger code as
an additional parameter passed to ExecBRUpdateTriggers() and
ExecBRDeleteTriggers().

Back-patch as far as v17 only, since MERGE cannot appear inside a CTE
prior to that. Additionally, take care to preserve the trigger ABI in
v17 (though not in v18, which is still in beta).

Bug: #18986
Reported-by: Yaroslav Syrytsia <me@ys.lc>
Author: Dean Rasheed <dean.a.rasheed@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/18986-e7a8aac3d339fa47@postgresql.org
Backpatch-through: 17

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index ea5cc10919d9c732fe8c673b44ed6f9603e9ffe6..ad28abe085f42984e7f48824da9594ea3962e35d 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -79,6 +79,7 @@ static bool GetTupleForTrigger(EState *estate,
                                                           ItemPointer tid,
                                                           LockTupleMode lockmode,
                                                           TupleTableSlot *oldslot,
+                                                          bool do_epq_recheck,
                                                           TupleTableSlot **epqslot,
                                                           TM_Result *tmresultp,
                                                           TM_FailureData *tmfdp);
@@ -2679,13 +2680,14 @@ ExecASDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
  * back the concurrently updated tuple if any.
  */
 bool
-ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
-                                        ResultRelInfo *relinfo,
-                                        ItemPointer tupleid,
-                                        HeapTuple fdw_trigtuple,
-                                        TupleTableSlot **epqslot,
-                                        TM_Result *tmresult,
-                                        TM_FailureData *tmfd)
+ExecBRDeleteTriggersNew(EState *estate, EPQState *epqstate,
+                                               ResultRelInfo *relinfo,
+                                               ItemPointer tupleid,
+                                               HeapTuple fdw_trigtuple,
+                                               TupleTableSlot **epqslot,
+                                               TM_Result *tmresult,
+                                               TM_FailureData *tmfd,
+                                               bool is_merge_delete)
 {
        TupleTableSlot *slot = ExecGetTriggerOldSlot(estate, relinfo);
        TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
@@ -2700,9 +2702,17 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
        {
                TupleTableSlot *epqslot_candidate = NULL;
 
+               /*
+                * Get a copy of the on-disk tuple we are planning to delete.  In
+                * general, if the tuple has been concurrently updated, we should
+                * recheck it using EPQ.  However, if this is a MERGE DELETE action,
+                * we skip this EPQ recheck and leave it to the caller (it must do
+                * additional rechecking, and might end up executing a different
+                * action entirely).
+                */
                if (!GetTupleForTrigger(estate, epqstate, relinfo, tupleid,
-                                                               LockTupleExclusive, slot, &epqslot_candidate,
-                                                               tmresult, tmfd))
+                                                               LockTupleExclusive, slot, !is_merge_delete,
+                                                               &epqslot_candidate, tmresult, tmfd))
                        return false;
 
                /*
@@ -2765,6 +2775,24 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
        return result;
 }
 
+/*
+ * ABI-compatible wrapper to emulate old version of the above function.
+ * Do not call this version in new code.
+ */
+bool
+ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
+                                        ResultRelInfo *relinfo,
+                                        ItemPointer tupleid,
+                                        HeapTuple fdw_trigtuple,
+                                        TupleTableSlot **epqslot,
+                                        TM_Result *tmresult,
+                                        TM_FailureData *tmfd)
+{
+       return ExecBRDeleteTriggersNew(estate, epqstate, relinfo, tupleid,
+                                                                  fdw_trigtuple, epqslot, tmresult, tmfd,
+                                                                  false);
+}
+
 /*
  * Note: is_crosspart_update must be true if the DELETE is being performed
  * as part of a cross-partition update.
@@ -2792,6 +2820,7 @@ ExecARDeleteTriggers(EState *estate,
                                                           tupleid,
                                                           LockTupleExclusive,
                                                           slot,
+                                                          false,
                                                           NULL,
                                                           NULL,
                                                           NULL);
@@ -2930,13 +2959,14 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
 }
 
 bool
-ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
-                                        ResultRelInfo *relinfo,
-                                        ItemPointer tupleid,
-                                        HeapTuple fdw_trigtuple,
-                                        TupleTableSlot *newslot,
-                                        TM_Result *tmresult,
-                                        TM_FailureData *tmfd)
+ExecBRUpdateTriggersNew(EState *estate, EPQState *epqstate,
+                                               ResultRelInfo *relinfo,
+                                               ItemPointer tupleid,
+                                               HeapTuple fdw_trigtuple,
+                                               TupleTableSlot *newslot,
+                                               TM_Result *tmresult,
+                                               TM_FailureData *tmfd,
+                                               bool is_merge_update)
 {
        TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
        TupleTableSlot *oldslot = ExecGetTriggerOldSlot(estate, relinfo);
@@ -2957,10 +2987,17 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
        {
                TupleTableSlot *epqslot_candidate = NULL;
 
-               /* get a copy of the on-disk tuple we are planning to update */
+               /*
+                * Get a copy of the on-disk tuple we are planning to update.  In
+                * general, if the tuple has been concurrently updated, we should
+                * recheck it using EPQ.  However, if this is a MERGE UPDATE action,
+                * we skip this EPQ recheck and leave it to the caller (it must do
+                * additional rechecking, and might end up executing a different
+                * action entirely).
+                */
                if (!GetTupleForTrigger(estate, epqstate, relinfo, tupleid,
-                                                               lockmode, oldslot, &epqslot_candidate,
-                                                               tmresult, tmfd))
+                                                               lockmode, oldslot, !is_merge_update,
+                                                               &epqslot_candidate, tmresult, tmfd))
                        return false;           /* cancel the update action */
 
                /*
@@ -3082,6 +3119,24 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
        return true;
 }
 
+/*
+ * ABI-compatible wrapper to emulate old version of the above function.
+ * Do not call this version in new code.
+ */
+bool
+ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
+                                        ResultRelInfo *relinfo,
+                                        ItemPointer tupleid,
+                                        HeapTuple fdw_trigtuple,
+                                        TupleTableSlot *newslot,
+                                        TM_Result *tmresult,
+                                        TM_FailureData *tmfd)
+{
+       return ExecBRUpdateTriggersNew(estate, epqstate, relinfo, tupleid,
+                                                                  fdw_trigtuple, newslot, tmresult, tmfd,
+                                                                  false);
+}
+
 /*
  * Note: 'src_partinfo' and 'dst_partinfo', when non-NULL, refer to the source
  * and destination partitions, respectively, of a cross-partition update of
@@ -3132,6 +3187,7 @@ ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
                                                           tupleid,
                                                           LockTupleExclusive,
                                                           oldslot,
+                                                          false,
                                                           NULL,
                                                           NULL,
                                                           NULL);
@@ -3288,6 +3344,7 @@ GetTupleForTrigger(EState *estate,
                                   ItemPointer tid,
                                   LockTupleMode lockmode,
                                   TupleTableSlot *oldslot,
+                                  bool do_epq_recheck,
                                   TupleTableSlot **epqslot,
                                   TM_Result *tmresultp,
                                   TM_FailureData *tmfdp)
@@ -3347,29 +3404,30 @@ GetTupleForTrigger(EState *estate,
                                if (tmfd.traversed)
                                {
                                        /*
-                                        * Recheck the tuple using EPQ. For MERGE, we leave this
-                                        * to the caller (it must do additional rechecking, and
-                                        * might end up executing a different action entirely).
+                                        * Recheck the tuple using EPQ, if requested.  Otherwise,
+                                        * just return that it was concurrently updated.
                                         */
-                                       if (estate->es_plannedstmt->commandType == CMD_MERGE)
+                                       if (do_epq_recheck)
                                        {
-                                               if (tmresultp)
-                                                       *tmresultp = TM_Updated;
-                                               return false;
+                                               *epqslot = EvalPlanQual(epqstate,
+                                                                                               relation,
+                                                                                               relinfo->ri_RangeTableIndex,
+                                                                                               oldslot);
+
+                                               /*
+                                                * If PlanQual failed for updated tuple - we must not
+                                                * process this tuple!
+                                                */
+                                               if (TupIsNull(*epqslot))
+                                               {
+                                                       *epqslot = NULL;
+                                                       return false;
+                                               }
                                        }
-
-                                       *epqslot = EvalPlanQual(epqstate,
-                                                                                       relation,
-                                                                                       relinfo->ri_RangeTableIndex,
-                                                                                       oldslot);
-
-                                       /*
-                                        * If PlanQual failed for updated tuple - we must not
-                                        * process this tuple!
-                                        */
-                                       if (TupIsNull(*epqslot))
+                                       else
                                        {
-                                               *epqslot = NULL;
+                                               if (tmresultp)
+                                                       *tmresultp = TM_Updated;
                                                return false;
                                        }
                                }

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index a0d1091ec014aec904840625d9579cae2e48ac48..c230b666706e381ca649126a057879295a8d7bc3 100644 (file)
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1349,9 +1349,10 @@ ExecDeletePrologue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
                if (context->estate->es_insert_pending_result_relations != NIL)
                        ExecPendingInserts(context->estate);
 
-               return ExecBRDeleteTriggers(context->estate, context->epqstate,
-                                                                       resultRelInfo, tupleid, oldtuple,
-                                                                       epqreturnslot, result, &context->tmfd);
+               return ExecBRDeleteTriggersNew(context->estate, context->epqstate,
+                                                                          resultRelInfo, tupleid, oldtuple,
+                                                                          epqreturnslot, result, &context->tmfd,
+                                                                          context->mtstate->operation == CMD_MERGE);
        }
 
        return true;
@@ -1947,9 +1948,10 @@ ExecUpdatePrologue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
                if (context->estate->es_insert_pending_result_relations != NIL)
                        ExecPendingInserts(context->estate);
 
-               return ExecBRUpdateTriggers(context->estate, context->epqstate,
-                                                                       resultRelInfo, tupleid, oldtuple, slot,
-                                                                       result, &context->tmfd);
+               return ExecBRUpdateTriggersNew(context->estate, context->epqstate,
+                                                                          resultRelInfo, tupleid, oldtuple, slot,
+                                                                          result, &context->tmfd,
+                                                                          context->mtstate->operation == CMD_MERGE);
        }
 
        return true;

diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 8a5a9fe642274a27bc9b1bcfbbef2208f7206f8e..f9e4dc4f3cdbc55bfd816ba9af32451999d68765 100644 (file)
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -206,6 +206,15 @@ extern void ExecBSDeleteTriggers(EState *estate,
 extern void ExecASDeleteTriggers(EState *estate,
                                                                 ResultRelInfo *relinfo,
                                                                 TransitionCaptureState *transition_capture);
+extern bool ExecBRDeleteTriggersNew(EState *estate,
+                                                                       EPQState *epqstate,
+                                                                       ResultRelInfo *relinfo,
+                                                                       ItemPointer tupleid,
+                                                                       HeapTuple fdw_trigtuple,
+                                                                       TupleTableSlot **epqslot,
+                                                                       TM_Result *tmresult,
+                                                                       TM_FailureData *tmfd,
+                                                                       bool is_merge_delete);
 extern bool ExecBRDeleteTriggers(EState *estate,
                                                                 EPQState *epqstate,
                                                                 ResultRelInfo *relinfo,
@@ -228,6 +237,15 @@ extern void ExecBSUpdateTriggers(EState *estate,
 extern void ExecASUpdateTriggers(EState *estate,
                                                                 ResultRelInfo *relinfo,
                                                                 TransitionCaptureState *transition_capture);
+extern bool ExecBRUpdateTriggersNew(EState *estate,
+                                                                       EPQState *epqstate,
+                                                                       ResultRelInfo *relinfo,
+                                                                       ItemPointer tupleid,
+                                                                       HeapTuple fdw_trigtuple,
+                                                                       TupleTableSlot *newslot,
+                                                                       TM_Result *tmresult,
+                                                                       TM_FailureData *tmfd,
+                                                                       bool is_merge_update);
 extern bool ExecBRUpdateTriggers(EState *estate,
                                                                 EPQState *epqstate,
                                                                 ResultRelInfo *relinfo,

diff --git a/src/test/isolation/expected/merge-match-recheck.out b/src/test/isolation/expected/merge-match-recheck.out
index 9a44a5959270b796aa4b1661ab06ee833c695cc9..90300f1db5ab38789120835803c31ce4e6058395 100644 (file)
--- a/src/test/isolation/expected/merge-match-recheck.out
+++ b/src/test/isolation/expected/merge-match-recheck.out
@@ -241,19 +241,28 @@ starting permutation: update_bal1_tg merge_bal_tg c2 select1_tg c1
 s2: NOTICE:  Update: (1,160,s1,setup) -> (1,50,s1,"setup updated by update_bal1_tg")
 step update_bal1_tg: UPDATE target_tg t SET balance = 50, val = t.val || ' updated by update_bal1_tg' WHERE t.key = 1;
 step merge_bal_tg: 
-  MERGE INTO target_tg t
-  USING (SELECT 1 as key) s
-  ON s.key = t.key
-  WHEN MATCHED AND balance < 100 THEN
-       UPDATE SET balance = balance * 2, val = t.val || ' when1'
-  WHEN MATCHED AND balance < 200 THEN
-       UPDATE SET balance = balance * 4, val = t.val || ' when2'
-  WHEN MATCHED AND balance < 300 THEN
-       UPDATE SET balance = balance * 8, val = t.val || ' when3';
+  WITH t AS (
+    MERGE INTO target_tg t
+    USING (SELECT 1 as key) s
+    ON s.key = t.key
+    WHEN MATCHED AND balance < 100 THEN
+      UPDATE SET balance = balance * 2, val = t.val || ' when1'
+    WHEN MATCHED AND balance < 200 THEN
+      UPDATE SET balance = balance * 4, val = t.val || ' when2'
+    WHEN MATCHED AND balance < 300 THEN
+      UPDATE SET balance = balance * 8, val = t.val || ' when3'
+    RETURNING t.*
+  )
+  SELECT * FROM t;
  <waiting ...>
 step c2: COMMIT;
 s1: NOTICE:  Update: (1,50,s1,"setup updated by update_bal1_tg") -> (1,100,s1,"setup updated by update_bal1_tg when1")
 step merge_bal_tg: <... completed>
+key|balance|status|val                                  
+---+-------+------+-------------------------------------
+  1|    100|s1    |setup updated by update_bal1_tg when1
+(1 row)
+
 step select1_tg: SELECT * FROM target_tg;
 key|balance|status|val                                  
 ---+-------+------+-------------------------------------

diff --git a/src/test/isolation/specs/merge-match-recheck.spec b/src/test/isolation/specs/merge-match-recheck.spec
index 298b2bfdcd6099179b3e904881b5f25a3bbc67a1..22688bb635525866978f31529485af7cf5f74810 100644 (file)
--- a/src/test/isolation/specs/merge-match-recheck.spec
+++ b/src/test/isolation/specs/merge-match-recheck.spec
@@ -99,15 +99,19 @@ step "merge_bal_pa"
 }
 step "merge_bal_tg"
 {
-  MERGE INTO target_tg t
-  USING (SELECT 1 as key) s
-  ON s.key = t.key
-  WHEN MATCHED AND balance < 100 THEN
-       UPDATE SET balance = balance * 2, val = t.val || ' when1'
-  WHEN MATCHED AND balance < 200 THEN
-       UPDATE SET balance = balance * 4, val = t.val || ' when2'
-  WHEN MATCHED AND balance < 300 THEN
-       UPDATE SET balance = balance * 8, val = t.val || ' when3';
+  WITH t AS (
+    MERGE INTO target_tg t
+    USING (SELECT 1 as key) s
+    ON s.key = t.key
+    WHEN MATCHED AND balance < 100 THEN
+      UPDATE SET balance = balance * 2, val = t.val || ' when1'
+    WHEN MATCHED AND balance < 200 THEN
+      UPDATE SET balance = balance * 4, val = t.val || ' when2'
+    WHEN MATCHED AND balance < 300 THEN
+      UPDATE SET balance = balance * 8, val = t.val || ' when3'
+    RETURNING t.*
+  )
+  SELECT * FROM t;
 }
 
 step "merge_delete"