From a5e517366955c1a417c8138eca7782e7db27f224 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 31 Jul 2019 21:27:04 -0700
Subject: [PATCH v1] WIP-fix-crash-ROW_MARK_COPY.

Author:
Reviewed-By:
Discussion: https://postgr.es/m/CAHU7rYZo_C4ULsAx_LAj8az9zqgrD8WDd4hTegDTMM1LMqrBsg@mail.gmail.com
Backpatch:
---
 src/backend/executor/execMain.c  | 30 +++++++++++++++++++-----------
 src/backend/executor/execScan.c  | 21 +++++++++++++++++++--
 src/backend/executor/execUtils.c |  1 +
 src/include/nodes/execnodes.h    | 18 +++++++++++-------
 4 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index dbd7dd9bcd4..3b619db691b 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -981,6 +981,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
 
 	/* mark EvalPlanQual not active */
 	estate->es_epqTupleSlot = NULL;
+	estate->es_epqTupleDatum = NULL;
 	estate->es_epqScanDone = NULL;
 
 	/*
@@ -2541,6 +2542,8 @@ EvalPlanQualSlot(EPQState *epqstate,
 	TupleTableSlot **slot;
 
 	Assert(rti > 0 && rti <= epqstate->estate->es_range_table_size);
+	Assert(relation != NULL);
+
 	slot = &epqstate->estate->es_epqTupleSlot[rti - 1];
 
 	if (*slot == NULL)
@@ -2549,13 +2552,7 @@ EvalPlanQualSlot(EPQState *epqstate,
 
 		oldcontext = MemoryContextSwitchTo(epqstate->estate->es_query_cxt);
 
-		if (relation)
-			*slot = table_slot_create(relation,
-									  &epqstate->estate->es_tupleTable);
-		else
-			*slot = ExecAllocTableSlot(&epqstate->estate->es_tupleTable,
-									   epqstate->origslot->tts_tupleDescriptor,
-									   &TTSOpsVirtual);
+		*slot = table_slot_create(relation, &epqstate->estate->es_tupleTable);
 
 		MemoryContextSwitchTo(oldcontext);
 	}
@@ -2586,9 +2583,11 @@ EvalPlanQualFetchRowMarks(EPQState *epqstate)
 		if (RowMarkRequiresRowShareLock(erm->markType))
 			elog(ERROR, "EvalPlanQual doesn't support locking rowmarks");
 
-		/* clear any leftover test tuple for this rel */
-		slot = EvalPlanQualSlot(epqstate, erm->relation, erm->rti);
-		ExecClearTuple(slot);
+		/* clear any leftover data for this mark */
+		epqstate->estate->es_epqTupleDatum[erm->rti - 1] = 0;
+		slot = epqstate->estate->es_epqTupleSlot[erm->rti - 1];
+		if (slot)
+			ExecClearTuple(slot);
 
 		/* if child rel, must check whether it produced this row */
 		if (erm->rti != erm->prti)
@@ -2623,6 +2622,9 @@ EvalPlanQualFetchRowMarks(EPQState *epqstate)
 			if (isNull)
 				continue;
 
+			if (!slot)
+				slot = EvalPlanQualSlot(epqstate, erm->relation, erm->rti);
+
 			/* fetch requests on foreign tables must be passed to their FDW */
 			if (erm->relation->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
 			{
@@ -2672,7 +2674,10 @@ EvalPlanQualFetchRowMarks(EPQState *epqstate)
 			if (isNull)
 				continue;
 
-			ExecStoreHeapTupleDatum(datum, slot);
+			// XXX: Do we need to care about memory lifetime here?  Seems like
+			// not, as origslot ought to live long enough for the entire round
+			// of EPQ?
+			epqstate->estate->es_epqTupleDatum[erm->rti - 1] = datum;
 		}
 	}
 }
@@ -2885,11 +2890,14 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree)
 	if (parentestate->es_epqTupleSlot != NULL)
 	{
 		estate->es_epqTupleSlot = parentestate->es_epqTupleSlot;
+		estate->es_epqTupleDatum = parentestate->es_epqTupleDatum;
 	}
 	else
 	{
 		estate->es_epqTupleSlot = (TupleTableSlot **)
 			palloc0(rtsize * sizeof(TupleTableSlot *));
+		estate->es_epqTupleDatum = (Datum *)
+			palloc0(rtsize * sizeof(Datum));
 	}
 
 	/*
diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index c0e4a5376c3..1ab8c0a3bce 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -63,7 +63,8 @@ ExecScanFetch(ScanState *node,
 				ExecClearTuple(slot);	/* would not be returned by scan */
 			return slot;
 		}
-		else if (estate->es_epqTupleSlot[scanrelid - 1] != NULL)
+		else if (estate->es_epqTupleSlot[scanrelid - 1] != NULL ||
+				 estate->es_epqTupleDatum[scanrelid - 1] != 0)
 		{
 			TupleTableSlot *slot = node->ss_ScanTupleSlot;
 
@@ -73,7 +74,23 @@ ExecScanFetch(ScanState *node,
 			/* Else mark to remember that we shouldn't return more */
 			estate->es_epqScanDone[scanrelid - 1] = true;
 
-			slot = estate->es_epqTupleSlot[scanrelid - 1];
+			if (estate->es_epqTupleSlot[scanrelid - 1] != NULL)
+			{
+				slot = estate->es_epqTupleSlot[scanrelid - 1];
+			}
+			else
+			{
+				ExecRowMark *erm = estate->es_rowmarks[scanrelid - 1];
+				Datum datum;
+
+				Assert(erm->markType == ROW_MARK_COPY);
+
+				datum = estate->es_epqTupleDatum[scanrelid - 1];
+				ExecStoreHeapTupleDatum(datum, slot);
+
+				// XXX: Do we need to materialize this slot? I don't
+				// immediately see why we would.
+			}
 
 			/* Return empty slot if we haven't got a test tuple */
 			if (TupIsNull(slot))
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index c1fc0d54e95..b1c925665f6 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -158,6 +158,7 @@ CreateExecutorState(void)
 	estate->es_per_tuple_exprcontext = NULL;
 
 	estate->es_epqTupleSlot = NULL;
+	estate->es_epqTupleDatum = NULL;
 	estate->es_epqScanDone = NULL;
 	estate->es_sourceText = NULL;
 
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 98bdcbcef5a..0811382e076 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -572,15 +572,19 @@ typedef struct EState
 
 	/*
 	 * These fields are for re-evaluating plan quals when an updated tuple is
-	 * substituted in READ COMMITTED mode.  es_epqTupleSlot[] contains test
-	 * tuples that scan plan nodes should return instead of whatever they'd
-	 * normally return, or an empty slot if there is nothing to return; if
-	 * es_epqTupleSlot[] is not NULL if a particular array entry is valid; and
-	 * es_epqScanDone[] is state to remember if the tuple has been returned
-	 * already.  Arrays are of size es_range_table_size and are indexed by
-	 * scan node scanrelid - 1.
+	 * substituted in READ COMMITTED mode.  The arrays are of size
+	 * es_range_table_size and are indexed by scan node scanrelid - 1.
+
+	 * es_epqTupleSlot[] and es_epqTupleDatum[] contain test tuples that scan
+	 * plan nodes should return instead of whatever they'd normally
+	 * return. es_epqTupleDatum is used for ROW_MARK_COPY rowmarks,
+	 * es_epqTupleSlot for everything else. If there is nothing to return the
+	 * corresponding es_epqTupleSlot is empty, and the corresponding
+	 * es_epqTupleDatum is 0. es_epqScanDone[] is state to remember if the
+	 * tuple has been returned already.
 	 */
 	TupleTableSlot **es_epqTupleSlot;	/* array of EPQ substitute tuples */
+	Datum	   *es_epqTupleDatum;	/* array of EPQ substitute tuples */
 	bool	   *es_epqScanDone; /* true if EPQ tuple has been fetched */
 
 	bool		es_use_parallel_mode;	/* can we use parallel workers? */
-- 
2.22.0.545.g9c9b961d7e

