Fix concurrent update issues with MERGE.

If MERGE attempts an UPDATE or DELETE on a table with BEFORE ROW
triggers, or a cross-partition UPDATE (with or without triggers), and
a concurrent UPDATE or DELETE happens, the merge code would fail.

In some cases this would lead to a crash, while in others it would
cause the wrong merge action to be executed, or no action at all. The
immediate cause of the crash was the trigger code calling
ExecGetUpdateNewTuple() as part of the EPQ mechanism, which fails
because during a merge ri_projectNew is NULL, since merge has its own
per-action projection information, which ExecGetUpdateNewTuple() knows
nothing about.

Fix by arranging for the trigger code to exit early, returning the
TM_Result and TM_FailureData information, if a concurrent modification
is detected, allowing the merge code to do the necessary EPQ handling
in its own way. Similarly, prevent the cross-partition update code
from doing any EPQ processing for a merge, allowing the merge code to
work out what it needs to do.

This leads to a number of simplifications in nodeModifyTable.c. Most
notably, the ModifyTableContext->GetUpdateNewTuple() callback is no
longer needed, and mergeGetUpdateNewTuple() can be deleted, since
there is no longer any requirement for get-update-new-tuple during a
merge. Similarly, ModifyTableContext->cpUpdateRetrySlot is no longer
needed. Thus ExecGetUpdateNewTuple() and the retry_slot handling of
ExecCrossPartitionUpdate() can be restored to how they were in v14,
before the merge code was added, and ExecMergeMatched() no longer
needs any special-case handling for cross-partition updates.

While at it, tidy up ExecUpdateEpilogue() a bit, making it handle
recheckIndexes locally, rather than passing it in as a parameter,
ensuring that it is freed properly. This dates back to when it was
split off from ExecUpdate() to support merge.

Per bug #17809 from Alexander Lakhin, and follow-up investigation of
bug #17792, also from Alexander Lakhin.

Back-patch to v15, where MERGE was introduced, taking care to preserve
backwards-compatibility of the trigger API in v15 for any extensions
that might use it.

Discussion:
  https://postgr.es/m/17809-9e6650bef133f0fe%40postgresql.org
  https://postgr.es/m/17792-0f89452029662c36%40postgresql.org

1 files changed, 53 insertions, 120 deletions

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 6f44d71f16b..e8d655868ae 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -88,15 +88,6 @@ typedef struct ModifyTableContext
 	 */
 	TupleTableSlot *planSlot;
 
-	/*
-	 * During EvalPlanQual, project and return the new version of the new
-	 * tuple
-	 */
-	TupleTableSlot *(*GetUpdateNewTuple) (ResultRelInfo *resultRelInfo,
-										  TupleTableSlot *epqslot,
-										  TupleTableSlot *oldSlot,
-										  MergeActionState *relaction);
-
 	/* MERGE specific */
 	MergeActionState *relaction;	/* MERGE action in progress */
 
@@ -107,12 +98,6 @@ typedef struct ModifyTableContext
 	TM_FailureData tmfd;
 
 	/*
-	 * The tuple produced by EvalPlanQual to retry from, if a cross-partition
-	 * UPDATE requires it
-	 */
-	TupleTableSlot *cpUpdateRetrySlot;
-
-	/*
 	 * The tuple projected by the INSERT's RETURNING clause, when doing a
 	 * cross-partition UPDATE
 	 */
@@ -162,10 +147,6 @@ static TupleTableSlot *ExecPrepareTupleRouting(ModifyTableState *mtstate,
 											   ResultRelInfo *targetRelInfo,
 											   TupleTableSlot *slot,
 											   ResultRelInfo **partRelInfo);
-static TupleTableSlot *internalGetUpdateNewTuple(ResultRelInfo *relinfo,
-												 TupleTableSlot *planSlot,
-												 TupleTableSlot *oldSlot,
-												 MergeActionState *relaction);
 
 static TupleTableSlot *ExecMerge(ModifyTableContext *context,
 								 ResultRelInfo *resultRelInfo,
@@ -179,10 +160,6 @@ static bool ExecMergeMatched(ModifyTableContext *context,
 static void ExecMergeNotMatched(ModifyTableContext *context,
 								ResultRelInfo *resultRelInfo,
 								bool canSetTag);
-static TupleTableSlot *mergeGetUpdateNewTuple(ResultRelInfo *relinfo,
-											  TupleTableSlot *planSlot,
-											  TupleTableSlot *oldSlot,
-											  MergeActionState *relaction);
 
 
 /*
@@ -738,26 +715,14 @@ ExecGetUpdateNewTuple(ResultRelInfo *relinfo,
 					  TupleTableSlot *planSlot,
 					  TupleTableSlot *oldSlot)
 {
+	ProjectionInfo *newProj = relinfo->ri_projectNew;
+	ExprContext *econtext;
+
 	/* Use a few extra Asserts to protect against outside callers */
 	Assert(relinfo->ri_projectNewInfoValid);
 	Assert(planSlot != NULL && !TTS_EMPTY(planSlot));
 	Assert(oldSlot != NULL && !TTS_EMPTY(oldSlot));
 
-	return internalGetUpdateNewTuple(relinfo, planSlot, oldSlot, NULL);
-}
-
-/*
- * Callback for ModifyTableState->GetUpdateNewTuple for use by regular UPDATE.
- */
-static TupleTableSlot *
-internalGetUpdateNewTuple(ResultRelInfo *relinfo,
-						  TupleTableSlot *planSlot,
-						  TupleTableSlot *oldSlot,
-						  MergeActionState *relaction)
-{
-	ProjectionInfo *newProj = relinfo->ri_projectNew;
-	ExprContext *econtext;
-
 	econtext = newProj->pi_exprContext;
 	econtext->ecxt_outertuple = planSlot;
 	econtext->ecxt_scantuple = oldSlot;
@@ -1336,8 +1301,11 @@ ExecPendingInserts(EState *estate)
 static bool
 ExecDeletePrologue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
 				   ItemPointer tupleid, HeapTuple oldtuple,
-				   TupleTableSlot **epqreturnslot)
+				   TupleTableSlot **epqreturnslot, TM_Result *result)
 {
+	if (result)
+		*result = TM_Ok;
+
 	/* BEFORE ROW DELETE triggers */
 	if (resultRelInfo->ri_TrigDesc &&
 		resultRelInfo->ri_TrigDesc->trig_delete_before_row)
@@ -1348,7 +1316,7 @@ ExecDeletePrologue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
 
 		return ExecBRDeleteTriggers(context->estate, context->epqstate,
 									resultRelInfo, tupleid, oldtuple,
-									epqreturnslot);
+									epqreturnslot, result, &context->tmfd);
 	}
 
 	return true;
@@ -1465,7 +1433,7 @@ ExecDelete(ModifyTableContext *context,
 	 * done if it says we are.
 	 */
 	if (!ExecDeletePrologue(context, resultRelInfo, tupleid, oldtuple,
-							epqreturnslot))
+							epqreturnslot, NULL))
 		return NULL;
 
 	/* INSTEAD OF ROW DELETE Triggers */
@@ -1746,8 +1714,10 @@ ldelete:
  *
  * False is returned if the tuple we're trying to move is found to have been
  * concurrently updated.  In that case, the caller must check if the updated
- * tuple (in updateCxt->cpUpdateRetrySlot) still needs to be re-routed, and
- * call this function again or perform a regular update accordingly.
+ * tuple that's returned in *retry_slot still needs to be re-routed, and call
+ * this function again or perform a regular update accordingly.  For MERGE,
+ * the updated tuple is not returned in *retry_slot; it has its own retry
+ * logic.
  */
 static bool
 ExecCrossPartitionUpdate(ModifyTableContext *context,
@@ -1756,6 +1726,7 @@ ExecCrossPartitionUpdate(ModifyTableContext *context,
 						 TupleTableSlot *slot,
 						 bool canSetTag,
 						 UpdateContext *updateCxt,
+						 TupleTableSlot **retry_slot,
 						 TupleTableSlot **inserted_tuple,
 						 ResultRelInfo **insert_destrel)
 {
@@ -1766,7 +1737,7 @@ ExecCrossPartitionUpdate(ModifyTableContext *context,
 	TupleTableSlot *epqslot = NULL;
 
 	context->cpUpdateReturningSlot = NULL;
-	context->cpUpdateRetrySlot = NULL;
+	*retry_slot = NULL;
 
 	/*
 	 * Disallow an INSERT ON CONFLICT DO UPDATE that causes the original row
@@ -1845,9 +1816,13 @@ ExecCrossPartitionUpdate(ModifyTableContext *context,
 		 * another transaction has concurrently updated the same row, it
 		 * re-fetches the row, skips the delete, and epqslot is set to the
 		 * re-fetched tuple slot.  In that case, we need to do all the checks
-		 * again.
+		 * again.  For MERGE, we leave everything to the caller (it must do
+		 * additional rechecking, and might end up executing a different
+		 * action entirely).
 		 */
-		if (TupIsNull(epqslot))
+		if (context->relaction != NULL)
+			return false;
+		else if (TupIsNull(epqslot))
 			return true;
 		else
 		{
@@ -1864,9 +1839,8 @@ ExecCrossPartitionUpdate(ModifyTableContext *context,
 											   oldSlot))
 				elog(ERROR, "failed to fetch tuple being updated");
 			/* and project the new tuple to retry the UPDATE with */
-			context->cpUpdateRetrySlot =
-				context->GetUpdateNewTuple(resultRelInfo, epqslot, oldSlot,
-										   context->relaction);
+			*retry_slot = ExecGetUpdateNewTuple(resultRelInfo, epqslot,
+												oldSlot);
 			return false;
 		}
 	}
@@ -1907,10 +1881,14 @@ ExecCrossPartitionUpdate(ModifyTableContext *context,
  */
 static bool
 ExecUpdatePrologue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
-				   ItemPointer tupleid, HeapTuple oldtuple, TupleTableSlot *slot)
+				   ItemPointer tupleid, HeapTuple oldtuple, TupleTableSlot *slot,
+				   TM_Result *result)
 {
 	Relation	resultRelationDesc = resultRelInfo->ri_RelationDesc;
 
+	if (result)
+		*result = TM_Ok;
+
 	ExecMaterializeSlot(slot);
 
 	/*
@@ -1931,7 +1909,7 @@ ExecUpdatePrologue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
 
 		return ExecBRUpdateTriggers(context->estate, context->epqstate,
 									resultRelInfo, tupleid, oldtuple, slot,
-									&context->tmfd);
+									result, &context->tmfd);
 	}
 
 	return true;
@@ -2032,7 +2010,8 @@ lreplace:
 	 */
 	if (partition_constraint_failed)
 	{
-		TupleTableSlot *inserted_tuple;
+		TupleTableSlot *inserted_tuple,
+				   *retry_slot;
 		ResultRelInfo *insert_destrel = NULL;
 
 		/*
@@ -2044,6 +2023,7 @@ lreplace:
 		if (ExecCrossPartitionUpdate(context, resultRelInfo,
 									 tupleid, oldtuple, slot,
 									 canSetTag, updateCxt,
+									 &retry_slot,
 									 &inserted_tuple,
 									 &insert_destrel))
 		{
@@ -2088,7 +2068,7 @@ lreplace:
 		 * ExecCrossPartitionUpdate installed an updated version of the new
 		 * tuple in the retry slot; start over.
 		 */
-		slot = context->cpUpdateRetrySlot;
+		slot = retry_slot;
 		goto lreplace;
 	}
 
@@ -2132,10 +2112,10 @@ lreplace:
 static void
 ExecUpdateEpilogue(ModifyTableContext *context, UpdateContext *updateCxt,
 				   ResultRelInfo *resultRelInfo, ItemPointer tupleid,
-				   HeapTuple oldtuple, TupleTableSlot *slot,
-				   List *recheckIndexes)
+				   HeapTuple oldtuple, TupleTableSlot *slot)
 {
 	ModifyTableState *mtstate = context->mtstate;
+	List	   *recheckIndexes = NIL;
 
 	/* insert index entries for tuple if necessary */
 	if (resultRelInfo->ri_NumIndices > 0 && updateCxt->updateIndexes)
@@ -2154,6 +2134,8 @@ ExecUpdateEpilogue(ModifyTableContext *context, UpdateContext *updateCxt,
 						 mtstate->mt_transition_capture,
 						 false);
 
+	list_free(recheckIndexes);
+
 	/*
 	 * Check any WITH CHECK OPTION constraints from parent views.  We are
 	 * required to do this after testing all constraints and uniqueness
@@ -2272,7 +2254,6 @@ ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
 	EState	   *estate = context->estate;
 	Relation	resultRelationDesc = resultRelInfo->ri_RelationDesc;
 	UpdateContext updateCxt = {0};
-	List	   *recheckIndexes = NIL;
 	TM_Result	result;
 
 	/*
@@ -2285,7 +2266,7 @@ ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
 	 * Prepare for the update.  This includes BEFORE ROW triggers, so we're
 	 * done if it says we are.
 	 */
-	if (!ExecUpdatePrologue(context, resultRelInfo, tupleid, oldtuple, slot))
+	if (!ExecUpdatePrologue(context, resultRelInfo, tupleid, oldtuple, slot, NULL))
 		return NULL;
 
 	/* INSTEAD OF ROW UPDATE Triggers */
@@ -2486,9 +2467,7 @@ redo_act:
 		(estate->es_processed)++;
 
 	ExecUpdateEpilogue(context, &updateCxt, resultRelInfo, tupleid, oldtuple,
-					   slot, recheckIndexes);
-
-	list_free(recheckIndexes);
+					   slot);
 
 	/* Process RETURNING if present */
 	if (resultRelInfo->ri_projectReturning)
@@ -2855,7 +2834,6 @@ lmerge_matched:
 	{
 		MergeActionState *relaction = (MergeActionState *) lfirst(l);
 		CmdType		commandType = relaction->mas_action->commandType;
-		List	   *recheckIndexes = NIL;
 		TM_Result	result;
 		UpdateContext updateCxt = {0};
 
@@ -2902,13 +2880,10 @@ lmerge_matched:
 				newslot = ExecProject(relaction->mas_proj);
 
 				context->relaction = relaction;
-				context->GetUpdateNewTuple = mergeGetUpdateNewTuple;
-				context->cpUpdateRetrySlot = NULL;
-
 				if (!ExecUpdatePrologue(context, resultRelInfo,
-										tupleid, NULL, newslot))
+										tupleid, NULL, newslot, &result))
 				{
-					result = TM_Ok;
+					/* Blocked by trigger, or concurrent update/delete */
 					break;
 				}
 				result = ExecUpdateAct(context, resultRelInfo, tupleid, NULL,
@@ -2916,18 +2891,17 @@ lmerge_matched:
 				if (result == TM_Ok && updateCxt.updated)
 				{
 					ExecUpdateEpilogue(context, &updateCxt, resultRelInfo,
-									   tupleid, NULL, newslot, recheckIndexes);
+									   tupleid, NULL, newslot);
 					mtstate->mt_merge_updated += 1;
 				}
-
 				break;
 
 			case CMD_DELETE:
 				context->relaction = relaction;
 				if (!ExecDeletePrologue(context, resultRelInfo, tupleid,
-										NULL, NULL))
+										NULL, NULL, &result))
 				{
-					result = TM_Ok;
+					/* Blocked by trigger, or concurrent update/delete */
 					break;
 				}
 				result = ExecDeleteAct(context, resultRelInfo, tupleid, false);
@@ -2994,34 +2968,13 @@ lmerge_matched:
 
 					/*
 					 * The target tuple was concurrently updated by some other
-					 * transaction.
-					 */
-
-					/*
-					 * During an UPDATE, if cpUpdateRetrySlot is set, then
-					 * ExecCrossPartitionUpdate() must have detected that the
-					 * tuple was concurrently updated, so we restart the
-					 * search for an appropriate WHEN MATCHED clause to
-					 * process the updated tuple.
-					 *
-					 * In this case, ExecDelete() would already have performed
-					 * EvalPlanQual() on the latest version of the tuple,
-					 * which in turn would already have been loaded into
-					 * ri_oldTupleSlot, so no need to do either of those
-					 * things.
-					 */
-					if (commandType == CMD_UPDATE &&
-						!TupIsNull(context->cpUpdateRetrySlot))
-						goto lmerge_matched;
-
-					/*
-					 * Otherwise, we run the EvalPlanQual() with the new
-					 * version of the tuple. If EvalPlanQual() does not return
-					 * a tuple, then we switch to the NOT MATCHED list of
-					 * actions. If it does return a tuple and the join qual is
-					 * still satisfied, then we just need to recheck the
-					 * MATCHED actions, starting from the top, and execute the
-					 * first qualifying action.
+					 * transaction. Run EvalPlanQual() with the new version of
+					 * the tuple. If it does not return a tuple, then we
+					 * switch to the NOT MATCHED list of actions. If it does
+					 * return a tuple and the join qual is still satisfied,
+					 * then we just need to recheck the MATCHED actions,
+					 * starting from the top, and execute the first qualifying
+					 * action.
 					 */
 					resultRelationDesc = resultRelInfo->ri_RelationDesc;
 					lockmode = ExecUpdateLockMode(estate, resultRelInfo);
@@ -3397,25 +3350,6 @@ ExecInitMergeTupleSlots(ModifyTableState *mtstate,
 }
 
 /*
- * Callback for ModifyTableContext->GetUpdateNewTuple for use by MERGE.  It
- * computes the updated tuple by projecting from the current merge action's
- * projection.
- */
-static TupleTableSlot *
-mergeGetUpdateNewTuple(ResultRelInfo *relinfo,
-					   TupleTableSlot *planSlot,
-					   TupleTableSlot *oldSlot,
-					   MergeActionState *relaction)
-{
-	ExprContext *econtext = relaction->mas_proj->pi_exprContext;
-
-	econtext->ecxt_scantuple = oldSlot;
-	econtext->ecxt_innertuple = planSlot;
-
-	return ExecProject(relaction->mas_proj);
-}
-
-/*
  * Process BEFORE EACH STATEMENT triggers
  */
 static void
@@ -3870,9 +3804,8 @@ ExecModifyTable(PlanState *pstate)
 													   oldSlot))
 						elog(ERROR, "failed to fetch tuple being updated");
 				}
-				slot = internalGetUpdateNewTuple(resultRelInfo, context.planSlot,
-												 oldSlot, NULL);
-				context.GetUpdateNewTuple = internalGetUpdateNewTuple;
+				slot = ExecGetUpdateNewTuple(resultRelInfo, context.planSlot,
+											 oldSlot);
 				context.relaction = NULL;
 
 				/* Now apply the update. */