Minor code review for tuple slot rewrite.

tglsfdc · tglsfdc · commit 9dbc6d25a97d · 2019-11-06T12:00:17.000-05:00
Avoid creating transiently-inconsistent slot states where possible, by not setting TTS_FLAG_SHOULDFREE until after the slot actually has a free'able tuple pointer, and by making sure that we reset tts_nvalid and related derived state before we replace the tuple contents. This would only matter if something were to examine the slot after we'd suffered some kind of error (e.g. out of memory) while manipulating the slot. We typically don't do that, so these changes might just be cosmetic --- but even if so, it seems like good future-proofing. Also remove some redundant Asserts, and add a couple for consistency. Back-patch to v12 where all this code was rewritten. Discussion: https://postgr.es/m/16095-c3ff2e5283b8dba5@postgresql.org
diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c
@@ -19,7 +19,7 @@
  *
  *		At ExecutorStart()
  *		----------------
-
+ *
  *		- ExecInitSeqScan() calls ExecInitScanTupleSlot() to construct a
  *		  TupleTableSlots for the tuples returned by the access method, and
  *		  ExecInitResultTypeTL() to define the node's return
@@ -272,7 +272,6 @@ tts_virtual_copy_heap_tuple(TupleTableSlot *slot)
 	return heap_form_tuple(slot->tts_tupleDescriptor,
 						   slot->tts_values,
 						   slot->tts_isnull);
-
 }
 
 static MinimalTuple
@@ -334,6 +333,8 @@ tts_heap_getsysattr(TupleTableSlot *slot, int attnum, bool *isnull)
 {
 	HeapTupleTableSlot *hslot = (HeapTupleTableSlot *) slot;
 
+	Assert(!TTS_EMPTY(slot));
+
 	return heap_getsysattr(hslot->tuple, attnum,
 						   slot->tts_tupleDescriptor, isnull);
 }
@@ -346,14 +347,19 @@ tts_heap_materialize(TupleTableSlot *slot)
 
 	Assert(!TTS_EMPTY(slot));
 
-	/* This slot has it's tuple already materialized. Nothing to do. */
+	/* If slot has its tuple already materialized, nothing to do. */
 	if (TTS_SHOULDFREE(slot))
 		return;
 
-	slot->tts_flags |= TTS_FLAG_SHOULDFREE;
-
 	oldContext = MemoryContextSwitchTo(slot->tts_mcxt);
 
+	/*
+	 * Have to deform from scratch, otherwise tts_values[] entries could point
+	 * into the non-materialized tuple (which might be gone when accessed).
+	 */
+	slot->tts_nvalid = 0;
+	hslot->off = 0;
+
 	if (!hslot->tuple)
 		hslot->tuple = heap_form_tuple(slot->tts_tupleDescriptor,
 									   slot->tts_values,
@@ -368,12 +374,7 @@ tts_heap_materialize(TupleTableSlot *slot)
 		hslot->tuple = heap_copytuple(hslot->tuple);
 	}
 
-	/*
-	 * Have to deform from scratch, otherwise tts_values[] entries could point
-	 * into the non-materialized tuple (which might be gone when accessed).
-	 */
-	slot->tts_nvalid = 0;
-	hslot->off = 0;
+	slot->tts_flags |= TTS_FLAG_SHOULDFREE;
 
 	MemoryContextSwitchTo(oldContext);
 }
@@ -436,7 +437,7 @@ tts_heap_store_tuple(TupleTableSlot *slot, HeapTuple tuple, bool shouldFree)
 	slot->tts_nvalid = 0;
 	hslot->tuple = tuple;
 	hslot->off = 0;
-	slot->tts_flags &= ~TTS_FLAG_EMPTY;
+	slot->tts_flags &= ~(TTS_FLAG_EMPTY | TTS_FLAG_SHOULDFREE);
 	slot->tts_tid = tuple->t_self;
 
 	if (shouldFree)
@@ -509,13 +510,19 @@ tts_minimal_materialize(TupleTableSlot *slot)
 
 	Assert(!TTS_EMPTY(slot));
 
-	/* This slot has it's tuple already materialized. Nothing to do. */
+	/* If slot has its tuple already materialized, nothing to do. */
 	if (TTS_SHOULDFREE(slot))
 		return;
 
-	slot->tts_flags |= TTS_FLAG_SHOULDFREE;
 	oldContext = MemoryContextSwitchTo(slot->tts_mcxt);
 
+	/*
+	 * Have to deform from scratch, otherwise tts_values[] entries could point
+	 * into the non-materialized tuple (which might be gone when accessed).
+	 */
+	slot->tts_nvalid = 0;
+	mslot->off = 0;
+
 	if (!mslot->mintuple)
 	{
 		mslot->mintuple = heap_form_minimal_tuple(slot->tts_tupleDescriptor,
@@ -532,19 +539,14 @@ tts_minimal_materialize(TupleTableSlot *slot)
 		mslot->mintuple = heap_copy_minimal_tuple(mslot->mintuple);
 	}
 
+	slot->tts_flags |= TTS_FLAG_SHOULDFREE;
+
 	Assert(mslot->tuple == &mslot->minhdr);
 
 	mslot->minhdr.t_len = mslot->mintuple->t_len + MINIMAL_TUPLE_OFFSET;
 	mslot->minhdr.t_data = (HeapTupleHeader) ((char *) mslot->mintuple - MINIMAL_TUPLE_OFFSET);
 
 	MemoryContextSwitchTo(oldContext);
-
-	/*
-	 * Have to deform from scratch, otherwise tts_values[] entries could point
-	 * into the non-materialized tuple (which might be gone when accessed).
-	 */
-	slot->tts_nvalid = 0;
-	mslot->off = 0;
 }
 
 static void
@@ -615,8 +617,6 @@ tts_minimal_store_tuple(TupleTableSlot *slot, MinimalTuple mtup, bool shouldFree
 
 	if (shouldFree)
 		slot->tts_flags |= TTS_FLAG_SHOULDFREE;
-	else
-		Assert(!TTS_SHOULDFREE(slot));
 }
 
 
@@ -651,8 +651,6 @@ tts_buffer_heap_clear(TupleTableSlot *slot)
 
 		heap_freetuple(bslot->base.tuple);
 		slot->tts_flags &= ~TTS_FLAG_SHOULDFREE;
-
-		Assert(!BufferIsValid(bslot->buffer));
 	}
 
 	if (BufferIsValid(bslot->buffer))
@@ -681,6 +679,8 @@ tts_buffer_heap_getsysattr(TupleTableSlot *slot, int attnum, bool *isnull)
 {
 	BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot;
 
+	Assert(!TTS_EMPTY(slot));
+
 	return heap_getsysattr(bslot->base.tuple, attnum,
 						   slot->tts_tupleDescriptor, isnull);
 }
@@ -693,14 +693,19 @@ tts_buffer_heap_materialize(TupleTableSlot *slot)
 
 	Assert(!TTS_EMPTY(slot));
 
-	/* If already materialized nothing to do. */
+	/* If slot has its tuple already materialized, nothing to do. */
 	if (TTS_SHOULDFREE(slot))
 		return;
 
-	slot->tts_flags |= TTS_FLAG_SHOULDFREE;
-
 	oldContext = MemoryContextSwitchTo(slot->tts_mcxt);
 
+	/*
+	 * Have to deform from scratch, otherwise tts_values[] entries could point
+	 * into the non-materialized tuple (which might be gone when accessed).
+	 */
+	bslot->base.off = 0;
+	slot->tts_nvalid = 0;
+
 	if (!bslot->base.tuple)
 	{
 		/*
@@ -713,7 +718,6 @@ tts_buffer_heap_materialize(TupleTableSlot *slot)
 		bslot->base.tuple = heap_form_tuple(slot->tts_tupleDescriptor,
 											slot->tts_values,
 											slot->tts_isnull);
-
 	}
 	else
 	{
@@ -723,19 +727,21 @@ tts_buffer_heap_materialize(TupleTableSlot *slot)
 		 * A heap tuple stored in a BufferHeapTupleTableSlot should have a
 		 * buffer associated with it, unless it's materialized or virtual.
 		 */
-		Assert(BufferIsValid(bslot->buffer));
 		if (likely(BufferIsValid(bslot->buffer)))
 			ReleaseBuffer(bslot->buffer);
 		bslot->buffer = InvalidBuffer;
 	}
-	MemoryContextSwitchTo(oldContext);
 
 	/*
-	 * Have to deform from scratch, otherwise tts_values[] entries could point
-	 * into the non-materialized tuple (which might be gone when accessed).
+	 * We don't set TTS_FLAG_SHOULDFREE until after releasing the buffer, if
+	 * any.  This avoids having a transient state that would fall foul of our
+	 * assertions that a slot with TTS_FLAG_SHOULDFREE doesn't own a buffer.
+	 * In the unlikely event that ReleaseBuffer() above errors out, we'd
+	 * effectively leak the copied tuple, but that seems fairly harmless.
 	 */
-	bslot->base.off = 0;
-	slot->tts_nvalid = 0;
+	slot->tts_flags |= TTS_FLAG_SHOULDFREE;
+
+	MemoryContextSwitchTo(oldContext);
 }
 
 static void
@@ -756,10 +762,10 @@ tts_buffer_heap_copyslot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
 		MemoryContext oldContext;
 
 		ExecClearTuple(dstslot);
-		dstslot->tts_flags |= TTS_FLAG_SHOULDFREE;
 		dstslot->tts_flags &= ~TTS_FLAG_EMPTY;
 		oldContext = MemoryContextSwitchTo(dstslot->tts_mcxt);
 		bdstslot->base.tuple = ExecCopySlotHeapTuple(srcslot);
+		dstslot->tts_flags |= TTS_FLAG_SHOULDFREE;
 		MemoryContextSwitchTo(oldContext);
 	}
 	else
@@ -1444,10 +1450,10 @@ ExecForceStoreHeapTuple(HeapTuple tuple,
 		BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot;
 
 		ExecClearTuple(slot);
-		slot->tts_flags |= TTS_FLAG_SHOULDFREE;
 		slot->tts_flags &= ~TTS_FLAG_EMPTY;
 		oldContext = MemoryContextSwitchTo(slot->tts_mcxt);
 		bslot->base.tuple = heap_copytuple(tuple);
+		slot->tts_flags |= TTS_FLAG_SHOULDFREE;
 		MemoryContextSwitchTo(oldContext);
 
 		if (shouldFree)
@@ -1856,7 +1862,6 @@ slot_getmissingattrs(TupleTableSlot *slot, int startAttNum, int lastAttNum)
 			slot->tts_values[missattnum] = attrmiss[missattnum].am_value;
 			slot->tts_isnull[missattnum] = !attrmiss[missattnum].am_present;
 		}
-
 	}
 }
 
diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h
@@ -261,9 +261,8 @@ typedef struct BufferHeapTupleTableSlot
 	/*
 	 * If buffer is not InvalidBuffer, then the slot is holding a pin on the
 	 * indicated buffer page; drop the pin when we release the slot's
-	 * reference to that buffer.  (TTS_FLAG_SHOULDFREE should not be set be
-	 * false in such a case, since presumably tts_tuple is pointing at the
-	 * buffer page.)
+	 * reference to that buffer.  (TTS_FLAG_SHOULDFREE should not be set in
+	 * such a case, since presumably tts_tuple is pointing into the buffer.)
 	 */
 	Buffer		buffer;			/* tuple's buffer, or InvalidBuffer */
 } BufferHeapTupleTableSlot;
