Store tuples for EvalPlanQual in slots, rather than as HeapTuples.

For the upcoming pluggable table access methods it's quite
inconvenient to store tuples as HeapTuples, as that'd require
converting tuples from a their native format into HeapTuples. Instead
use slots to manage epq tuples.

To fit into that scheme, change the foreign data wrapper callback
RefetchForeignRow, to store the tuple in a slot. Insist on using the
caller provided slot, so it conveniently can be stored in the
corresponding EPQ slot.  As there is no in core user of
RefetchForeignRow, that change was done blindly, but we plan to test
that soon.

To avoid duplicating that work for row locks, move row locks to just
directly use the EPQ slots - it previously temporarily stored tuples
in LockRowsState.lr_curtuples, but that doesn't seem beneficial, given
we'd possibly end up with a significant number of additional slots.

The behaviour of es_epqTupleSet[rti -1] is now checked by
es_epqTupleSlot[rti -1] != NULL, as that is distinguishable from a
slot containing an empty tuple.

Author: Andres Freund, Haribabu Kommi, Ashutosh Bapat
Discussion: https://postgr.es/m/20180703070645.wchpu5muyto5n647@alap3.anarazel.de

doc/src/sgml/fdwhandler.sgml

@@ -992,29 +992,31 @@ GetForeignRowMarkType(RangeTblEntry *rte,
 
     <para>
 <programlisting>
-HeapTuple
+void
 RefetchForeignRow(EState *estate,
                   ExecRowMark *erm,
                   Datum rowid,
+                  TupleTableSlot *slot,
                   bool *updated);
 </programlisting>
 
-     Re-fetch one tuple from the foreign table, after locking it if required.
+     Re-fetch one tuple slot from the foreign table, after locking it if required.
      <literal>estate</literal> is global execution state for the query.
      <literal>erm</literal> is the <structname>ExecRowMark</structname> struct describing
      the target foreign table and the row lock type (if any) to acquire.
      <literal>rowid</literal> identifies the tuple to be fetched.
-     <literal>updated</literal> is an output parameter.
+     <literal>slot</literal> contains nothing useful upon call, but can be used to
+     hold the returned tuple. <literal>updated</literal> is an output parameter.
     </para>
 
     <para>
-     This function should return a palloc'ed copy of the fetched tuple,
-     or <literal>NULL</literal> if the row lock couldn't be obtained.  The row lock
-     type to acquire is defined by <literal>erm-&gt;markType</literal>, which is the
-     value previously returned by <function>GetForeignRowMarkType</function>.
-     (<literal>ROW_MARK_REFERENCE</literal> means to just re-fetch the tuple without
-     acquiring any lock, and <literal>ROW_MARK_COPY</literal> will never be seen by
-     this routine.)
+     This function should store the tuple into the provided, or clear it if if
+     the row lock couldn't be obtained.  The row lock type to acquire is
+     defined by <literal>erm-&gt;markType</literal>, which is the value
+     previously returned by <function>GetForeignRowMarkType</function>.
+     (<literal>ROW_MARK_REFERENCE</literal> means to just re-fetch the tuple
+     without acquiring any lock, and <literal>ROW_MARK_COPY</literal> will
+     never be seen by this routine.)
     </para>
 
     <para>
@@ -1026,7 +1028,7 @@ RefetchForeignRow(EState *estate,
 
     <para>
      Note that by default, failure to acquire a row lock should result in
-     raising an error; a <literal>NULL</literal> return is only appropriate if
+     raising an error; returning with an empty slot is only appropriate if
      the <literal>SKIP LOCKED</literal> option is specified
      by <literal>erm-&gt;waitPolicy</literal>.
     </para>

src/backend/executor/execScan.c

@@ -40,7 +40,7 @@ ExecScanFetch(ScanState *node,
 
 	CHECK_FOR_INTERRUPTS();
 
-	if (estate->es_epqTuple != NULL)
+	if (estate->es_epqTupleSlot != NULL)
 	{
 		/*
 		 * We are inside an EvalPlanQual recheck.  Return the test tuple if
@@ -63,7 +63,7 @@ ExecScanFetch(ScanState *node,
 				ExecClearTuple(slot);	/* would not be returned by scan */
 			return slot;
 		}
-		else if (estate->es_epqTupleSet[scanrelid - 1])
+		else if (estate->es_epqTupleSlot[scanrelid - 1] != NULL)
 		{
 			TupleTableSlot *slot = node->ss_ScanTupleSlot;
 
@@ -73,17 +73,15 @@ ExecScanFetch(ScanState *node,
 			/* Else mark to remember that we shouldn't return more */
 			estate->es_epqScanDone[scanrelid - 1] = true;
 
-			/* Return empty slot if we haven't got a test tuple */
-			if (estate->es_epqTuple[scanrelid - 1] == NULL)
-				return ExecClearTuple(slot);
+			slot = estate->es_epqTupleSlot[scanrelid - 1];
 
-			/* Store test tuple in the plan node's scan slot */
-			ExecForceStoreHeapTuple(estate->es_epqTuple[scanrelid - 1],
-									slot);
+			/* Return empty slot if we haven't got a test tuple */
+			if (TupIsNull(slot))
+				return NULL;
 
 			/* Check if it meets the access-method conditions */
 			if (!(*recheckMtd) (node, slot))
-				ExecClearTuple(slot);	/* would not be returned by scan */
+				return ExecClearTuple(slot);	/* would not be returned by scan */
 
 			return slot;
 		}

src/backend/executor/execTuples.c

@@ -1542,6 +1542,32 @@ ExecStoreAllNullTuple(TupleTableSlot *slot)
 	return ExecStoreVirtualTuple(slot);
 }
 
+/*
+ * Store a HeapTuple in datum form, into a slot. That always requires
+ * deforming it and storing it in virtual form.
+ *
+ * Until the slot is materialized, the contents of the slot depend on the
+ * datum.
+ */
+void
+ExecStoreHeapTupleDatum(Datum data, TupleTableSlot *slot)
+{
+	HeapTupleData tuple = {0};
+	HeapTupleHeader td;
+
+	td = DatumGetHeapTupleHeader(data);
+
+	tuple.t_len = HeapTupleHeaderGetDatumLength(td);
+	tuple.t_self = td->t_ctid;
+	tuple.t_data = td;
+
+	ExecClearTuple(slot);
+
+	heap_deform_tuple(&tuple, slot->tts_tupleDescriptor,
+					  slot->tts_values, slot->tts_isnull);
+	ExecStoreVirtualTuple(slot);
+}
+
 /*
  * ExecFetchSlotHeapTuple - fetch HeapTuple representing the slot's content
  *

src/backend/executor/execUtils.c

@@ -155,8 +155,7 @@ CreateExecutorState(void)
 
 	estate->es_per_tuple_exprcontext = NULL;
 
-	estate->es_epqTuple = NULL;
-	estate->es_epqTupleSet = NULL;
+	estate->es_epqTupleSlot = NULL;
 	estate->es_epqScanDone = NULL;
 	estate->es_sourceText = NULL;
 

src/backend/executor/nodeIndexonlyscan.c

@@ -426,7 +426,7 @@ ExecIndexOnlyMarkPos(IndexOnlyScanState *node)
 {
 	EState	   *estate = node->ss.ps.state;
 
-	if (estate->es_epqTuple != NULL)
+	if (estate->es_epqTupleSlot != NULL)
 	{
 		/*
 		 * We are inside an EvalPlanQual recheck.  If a test tuple exists for
@@ -440,7 +440,7 @@ ExecIndexOnlyMarkPos(IndexOnlyScanState *node)
 		Index		scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
 
 		Assert(scanrelid > 0);
-		if (estate->es_epqTupleSet[scanrelid - 1])
+		if (estate->es_epqTupleSlot[scanrelid - 1] != NULL)
 		{
 			/* Verify the claim above */
 			if (!estate->es_epqScanDone[scanrelid - 1])
@@ -461,13 +461,13 @@ ExecIndexOnlyRestrPos(IndexOnlyScanState *node)
 {
 	EState	   *estate = node->ss.ps.state;
 
-	if (estate->es_epqTuple != NULL)
+	if (estate->es_epqTupleSlot != NULL)
 	{
 		/* See comments in ExecIndexOnlyMarkPos */
 		Index		scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
 
 		Assert(scanrelid > 0);
-		if (estate->es_epqTupleSet[scanrelid - 1])
+		if (estate->es_epqTupleSlot[scanrelid - 1])
 		{
 			/* Verify the claim above */
 			if (!estate->es_epqScanDone[scanrelid - 1])

src/backend/executor/nodeIndexscan.c

@@ -850,7 +850,7 @@ ExecIndexMarkPos(IndexScanState *node)
 {
 	EState	   *estate = node->ss.ps.state;
 
-	if (estate->es_epqTuple != NULL)
+	if (estate->es_epqTupleSlot != NULL)
 	{
 		/*
 		 * We are inside an EvalPlanQual recheck.  If a test tuple exists for
@@ -864,7 +864,7 @@ ExecIndexMarkPos(IndexScanState *node)
 		Index		scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
 
 		Assert(scanrelid > 0);
-		if (estate->es_epqTupleSet[scanrelid - 1])
+		if (estate->es_epqTupleSlot[scanrelid - 1] != NULL)
 		{
 			/* Verify the claim above */
 			if (!estate->es_epqScanDone[scanrelid - 1])
@@ -885,13 +885,13 @@ ExecIndexRestrPos(IndexScanState *node)
 {
 	EState	   *estate = node->ss.ps.state;
 
-	if (estate->es_epqTuple != NULL)
+	if (estate->es_epqTupleSlot != NULL)
 	{
 		/* See comments in ExecIndexMarkPos */
 		Index		scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
 
 		Assert(scanrelid > 0);
-		if (estate->es_epqTupleSet[scanrelid - 1])
+		if (estate->es_epqTupleSlot[scanrelid - 1] != NULL)
 		{
 			/* Verify the claim above */
 			if (!estate->es_epqScanDone[scanrelid - 1])

src/backend/executor/nodeLockRows.c

@@ -66,6 +66,12 @@ lnext:
 	/* We don't need EvalPlanQual unless we get updated tuple version(s) */
 	epq_needed = false;
 
+	/*
+	 * Initialize EPQ machinery. Need to do that early because source tuples
+	 * are stored in slots initialized therein.
+	 */
+	EvalPlanQualBegin(&node->lr_epqstate, estate);
+
 	/*
 	 * Attempt to lock the source tuple(s).  (Note we only have locking
 	 * rowmarks in lr_arowMarks.)
@@ -74,7 +80,6 @@ lnext:
 	{
 		ExecAuxRowMark *aerm = (ExecAuxRowMark *) lfirst(lc);
 		ExecRowMark *erm = aerm->rowmark;
-		HeapTuple  *testTuple;
 		Datum		datum;
 		bool		isNull;
 		HeapTupleData tuple;
@@ -82,13 +87,11 @@ lnext:
 		HeapUpdateFailureData hufd;
 		LockTupleMode lockmode;
 		HTSU_Result test;
-		HeapTuple	copyTuple;
+		TupleTableSlot *markSlot;
 
 		/* clear any leftover test tuple for this rel */
-		testTuple = &(node->lr_curtuples[erm->rti - 1]);
-		if (*testTuple != NULL)
-			heap_freetuple(*testTuple);
-		*testTuple = NULL;
+		markSlot = EvalPlanQualSlot(&node->lr_epqstate, erm->relation, erm->rti);
+		ExecClearTuple(markSlot);
 
 		/* if child rel, must check whether it produced this row */
 		if (erm->rti != erm->prti)
@@ -135,19 +138,18 @@ lnext:
 						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 						 errmsg("cannot lock rows in foreign table \"%s\"",
 								RelationGetRelationName(erm->relation))));
-			copyTuple = fdwroutine->RefetchForeignRow(estate,
-													  erm,
-													  datum,
-													  &updated);
-			if (copyTuple == NULL)
+
+			fdwroutine->RefetchForeignRow(estate,
+										  erm,
+										  datum,
+										  markSlot,
+										  &updated);
+			if (TupIsNull(markSlot))
 			{
 				/* couldn't get the lock, so skip this row */
 				goto lnext;
 			}
 
-			/* save locked tuple for possible EvalPlanQual testing below */
-			*testTuple = copyTuple;
-
 			/*
 			 * if FDW says tuple was updated before getting locked, we need to
 			 * perform EPQ testing to see if quals are still satisfied
@@ -230,11 +232,10 @@ lnext:
 				}
 
 				/* updated, so fetch and lock the updated version */
-				copyTuple = EvalPlanQualFetch(estate, erm->relation,
-											  lockmode, erm->waitPolicy,
-											  &hufd.ctid, hufd.xmax);
-
-				if (copyTuple == NULL)
+				if (!EvalPlanQualFetch(estate, erm->relation,
+									   lockmode, erm->waitPolicy,
+									   &hufd.ctid, hufd.xmax,
+									   markSlot))
 				{
 					/*
 					 * Tuple was deleted; or it's locked and we're under SKIP
@@ -243,10 +244,7 @@ lnext:
 					goto lnext;
 				}
 				/* remember the actually locked tuple's TID */
-				tuple.t_self = copyTuple->t_self;
-
-				/* Save locked tuple for EvalPlanQual testing below */
-				*testTuple = copyTuple;
+				tuple.t_self = markSlot->tts_tid;
 
 				/* Remember we need to do EPQ testing */
 				epq_needed = true;
@@ -272,42 +270,34 @@ lnext:
 	 */
 	if (epq_needed)
 	{
-		/* Initialize EPQ machinery */
-		EvalPlanQualBegin(&node->lr_epqstate, estate);
-
 		/*
-		 * Transfer any already-fetched tuples into the EPQ state, and fetch a
-		 * copy of any rows that were successfully locked without any update
-		 * having occurred.  (We do this in a separate pass so as to avoid
-		 * overhead in the common case where there are no concurrent updates.)
-		 * Make sure any inactive child rels have NULL test tuples in EPQ.
+		 * Fetch a copy of any rows that were successfully locked without any
+		 * update having occurred.  (We do this in a separate pass so as to
+		 * avoid overhead in the common case where there are no concurrent
+		 * updates.)  Make sure any inactive child rels have NULL test tuples
+		 * in EPQ.
 		 */
 		foreach(lc, node->lr_arowMarks)
 		{
 			ExecAuxRowMark *aerm = (ExecAuxRowMark *) lfirst(lc);
 			ExecRowMark *erm = aerm->rowmark;
+			TupleTableSlot *markSlot;
 			HeapTupleData tuple;
-			Buffer		buffer;
+			Buffer buffer;
+
+			markSlot = EvalPlanQualSlot(&node->lr_epqstate, erm->relation, erm->rti);
 
 			/* skip non-active child tables, but clear their test tuples */
 			if (!erm->ermActive)
 			{
 				Assert(erm->rti != erm->prti);	/* check it's child table */
-				EvalPlanQualSetTuple(&node->lr_epqstate, erm->rti, NULL);
+				ExecClearTuple(markSlot);
 				continue;
 			}
 
 			/* was tuple updated and fetched above? */
-			if (node->lr_curtuples[erm->rti - 1] != NULL)
-			{
-				/* yes, so set it as the EPQ test tuple for this rel */
-				EvalPlanQualSetTuple(&node->lr_epqstate,
-									 erm->rti,
-									 node->lr_curtuples[erm->rti - 1]);
-				/* freeing this tuple is now the responsibility of EPQ */
-				node->lr_curtuples[erm->rti - 1] = NULL;
+			if (!TupIsNull(markSlot))
 				continue;
-			}
 
 			/* foreign tables should have been fetched above */
 			Assert(erm->relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE);
@@ -318,11 +308,9 @@ lnext:
 			if (!heap_fetch(erm->relation, SnapshotAny, &tuple, &buffer,
 							false, NULL))
 				elog(ERROR, "failed to fetch tuple for EvalPlanQual recheck");
-
-			/* successful, copy and store tuple */
-			EvalPlanQualSetTuple(&node->lr_epqstate, erm->rti,
-								 heap_copytuple(&tuple));
-			ReleaseBuffer(buffer);
+			ExecStorePinnedBufferHeapTuple(&tuple, markSlot, buffer);
+			ExecMaterializeSlot(markSlot);
+			/* successful, use tuple in slot */
 		}
 
 		/*
@@ -401,13 +389,6 @@ ExecInitLockRows(LockRows *node, EState *estate, int eflags)
 	 */
 	lrstate->ps.ps_ProjInfo = NULL;
 
-	/*
-	 * Create workspace in which we can remember per-RTE locked tuples
-	 */
-	lrstate->lr_ntables = estate->es_range_table_size;
-	lrstate->lr_curtuples = (HeapTuple *)
-		palloc0(lrstate->lr_ntables * sizeof(HeapTuple));
-
 	/*
 	 * Locate the ExecRowMark(s) that this node is responsible for, and
 	 * construct ExecAuxRowMarks for them.  (InitPlan should already have
@@ -425,9 +406,6 @@ ExecInitLockRows(LockRows *node, EState *estate, int eflags)
 		if (rc->isParent)
 			continue;
 
-		/* safety check on size of lr_curtuples array */
-		Assert(rc->rti > 0 && rc->rti <= lrstate->lr_ntables);
-
 		/* find ExecRowMark and build ExecAuxRowMark */
 		erm = ExecFindRowMark(estate, rc->rti, false);
 		aerm = ExecBuildAuxRowMark(erm, outerPlan->targetlist);

src/backend/executor/nodeModifyTable.c

@@ -1803,7 +1803,7 @@ ExecModifyTable(PlanState *pstate)
 	 * case it is within a CTE subplan.  Hence this test must be here, not in
 	 * ExecInitModifyTable.)
 	 */
-	if (estate->es_epqTuple != NULL)
+	if (estate->es_epqTupleSlot != NULL)
 		elog(ERROR, "ModifyTable should not be called during EvalPlanQual");
 
 	/*

src/include/executor/executor.h

@@ -197,16 +197,16 @@ extern ExecAuxRowMark *ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist);
 extern TupleTableSlot *EvalPlanQual(EState *estate, EPQState *epqstate,
 			 Relation relation, Index rti, LockTupleMode lockmode,
 			 ItemPointer tid, TransactionId priorXmax);
-extern HeapTuple EvalPlanQualFetch(EState *estate, Relation relation,
+extern bool EvalPlanQualFetch(EState *estate, Relation relation,
 				  LockTupleMode lockmode, LockWaitPolicy wait_policy,
-				  ItemPointer tid, TransactionId priorXmax);
+				  ItemPointer tid, TransactionId priorXmax,
+				  TupleTableSlot *slot);
 extern void EvalPlanQualInit(EPQState *epqstate, EState *estate,
 				 Plan *subplan, List *auxrowmarks, int epqParam);
 extern void EvalPlanQualSetPlan(EPQState *epqstate,
 					Plan *subplan, List *auxrowmarks);
-extern void EvalPlanQualSetTuple(EPQState *epqstate, Index rti,
-					 HeapTuple tuple);
-extern HeapTuple EvalPlanQualGetTuple(EPQState *epqstate, Index rti);
+extern TupleTableSlot *EvalPlanQualSlot(EPQState *epqstate,
+			 Relation relation, Index rti);
 
 #define EvalPlanQualSetSlot(epqstate, slot)  ((epqstate)->origslot = (slot))
 extern void EvalPlanQualFetchRowMarks(EPQState *epqstate);

src/include/executor/tuptable.h

@@ -320,6 +320,7 @@ extern void ExecForceStoreMinimalTuple(MinimalTuple mtup, TupleTableSlot *slot,
 									   bool shouldFree);
 extern TupleTableSlot *ExecStoreVirtualTuple(TupleTableSlot *slot);
 extern TupleTableSlot *ExecStoreAllNullTuple(TupleTableSlot *slot);
+extern void ExecStoreHeapTupleDatum(Datum data, TupleTableSlot *slot);
 extern HeapTuple ExecFetchSlotHeapTuple(TupleTableSlot *slot, bool materialize, bool *shouldFree);
 extern MinimalTuple ExecFetchSlotMinimalTuple(TupleTableSlot *slot,
 						  bool *shouldFree);

src/include/foreign/fdwapi.h

@@ -121,10 +121,11 @@ typedef void (*EndDirectModify_function) (ForeignScanState *node);
 typedef RowMarkType (*GetForeignRowMarkType_function) (RangeTblEntry *rte,
 													   LockClauseStrength strength);
 
-typedef HeapTuple (*RefetchForeignRow_function) (EState *estate,
-												 ExecRowMark *erm,
-												 Datum rowid,
-												 bool *updated);
+typedef void (*RefetchForeignRow_function) (EState *estate,
+											ExecRowMark *erm,
+											Datum rowid,
+											TupleTableSlot *slot,
+											bool *updated);
 
 typedef void (*ExplainForeignScan_function) (ForeignScanState *node,
 											 struct ExplainState *es);

src/include/nodes/execnodes.h

@@ -562,15 +562,15 @@ typedef struct EState
 
 	/*
 	 * These fields are for re-evaluating plan quals when an updated tuple is
-	 * substituted in READ COMMITTED mode.  es_epqTuple[] contains tuples that
-	 * scan plan nodes should return instead of whatever they'd normally
-	 * return, or NULL if nothing to return; es_epqTupleSet[] is true 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.  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.
 	 */
-	HeapTuple  *es_epqTuple;	/* array of EPQ substitute tuples */
-	bool	   *es_epqTupleSet; /* true if EPQ tuple is provided */
+	TupleTableSlot **es_epqTupleSlot;	/* 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? */
@@ -2257,8 +2257,6 @@ typedef struct LockRowsState
 	PlanState	ps;				/* its first field is NodeTag */
 	List	   *lr_arowMarks;	/* List of ExecAuxRowMarks */
 	EPQState	lr_epqstate;	/* for evaluating EvalPlanQual rechecks */
-	HeapTuple  *lr_curtuples;	/* locked tuples (one entry per RT entry) */
-	int			lr_ntables;		/* length of lr_curtuples[] array */
 } LockRowsState;
 
 /* ----------------