Fix an oversight in the support for storing/retrieving "minimal tuples" in

TupleTableSlots.  We have functions for retrieving a minimal tuple from a slot
after storing a regular tuple in it, or vice versa; but these were implemented
by converting the internal storage from one format to the other.  The problem
with that is it invalidates any pass-by-reference Datums that were already
fetched from the slot, since they'll be pointing into the just-freed version
of the tuple.  The known problem cases involve fetching both a whole-row
variable and a pass-by-reference value from a slot that is fed from a
tuplestore or tuplesort object.  The added regression tests illustrate some
simple cases, but there may be other failure scenarios traceable to the same
bug.  Note that the added tests probably only fail on unpatched code if it's
built with --enable-cassert; otherwise the bug leads to fetching from freed
memory, which will not have been overwritten without additional conditions.

Fix by allowing a slot to contain both formats simultaneously; which turns out
not to complicate the logic much at all, if anything it seems less contorted
than before.

Back-patch to 8.2, where minimal tuples were introduced.

src/backend/access/common/heaptuple.c

@@ -50,7 +50,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/common/heaptuple.c,v 1.125 2009/01/01 17:23:34 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/common/heaptuple.c,v 1.126 2009/03/30 04:08:43 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1183,7 +1183,7 @@ slot_getattr(TupleTableSlot *slot, int attnum, bool *isnull)
 	{
 		if (tuple == NULL)		/* internal error */
 			elog(ERROR, "cannot extract system attribute from virtual tuple");
-		if (slot->tts_mintuple) /* internal error */
+		if (tuple == &(slot->tts_minhdr))	/* internal error */
 			elog(ERROR, "cannot extract system attribute from minimal tuple");
 		return heap_getsysattr(tuple, attnum, tupleDesc, isnull);
 	}
@@ -1369,7 +1369,7 @@ slot_attisnull(TupleTableSlot *slot, int attnum)
 	{
 		if (tuple == NULL)		/* internal error */
 			elog(ERROR, "cannot extract system attribute from virtual tuple");
-		if (slot->tts_mintuple) /* internal error */
+		if (tuple == &(slot->tts_minhdr))	/* internal error */
 			elog(ERROR, "cannot extract system attribute from minimal tuple");
 		return heap_attisnull(tuple, attnum);
 	}

src/backend/executor/execTuples.c

@@ -15,7 +15,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/execTuples.c,v 1.105 2009/01/01 17:23:41 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/execTuples.c,v 1.106 2009/03/30 04:08:43 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -146,6 +146,7 @@ ExecCreateTupleTable(int tableSize)
 		slot->type = T_TupleTableSlot;
 		slot->tts_isempty = true;
 		slot->tts_shouldFree = false;
+		slot->tts_shouldFreeMin = false;
 		slot->tts_tuple = NULL;
 		slot->tts_tupleDescriptor = NULL;
 		slot->tts_mcxt = CurrentMemoryContext;
@@ -224,6 +225,7 @@ MakeSingleTupleTableSlot(TupleDesc tupdesc)
 	/* This should match ExecCreateTupleTable() */
 	slot->tts_isempty = true;
 	slot->tts_shouldFree = false;
+	slot->tts_shouldFreeMin = false;
 	slot->tts_tuple = NULL;
 	slot->tts_tupleDescriptor = NULL;
 	slot->tts_mcxt = CurrentMemoryContext;
@@ -410,18 +412,16 @@ ExecStoreTuple(HeapTuple tuple,
 	 * Free any old physical tuple belonging to the slot.
 	 */
 	if (slot->tts_shouldFree)
-	{
-		if (slot->tts_mintuple)
-			heap_free_minimal_tuple(slot->tts_mintuple);
-		else
-			heap_freetuple(slot->tts_tuple);
-	}
+		heap_freetuple(slot->tts_tuple);
+	if (slot->tts_shouldFreeMin)
+		heap_free_minimal_tuple(slot->tts_mintuple);
 
 	/*
 	 * Store the new tuple into the specified slot.
 	 */
 	slot->tts_isempty = false;
 	slot->tts_shouldFree = shouldFree;
+	slot->tts_shouldFreeMin = false;
 	slot->tts_tuple = tuple;
 	slot->tts_mintuple = NULL;
 
@@ -473,12 +473,9 @@ ExecStoreMinimalTuple(MinimalTuple mtup,
 	 * Free any old physical tuple belonging to the slot.
 	 */
 	if (slot->tts_shouldFree)
-	{
-		if (slot->tts_mintuple)
-			heap_free_minimal_tuple(slot->tts_mintuple);
-		else
-			heap_freetuple(slot->tts_tuple);
-	}
+		heap_freetuple(slot->tts_tuple);
+	if (slot->tts_shouldFreeMin)
+		heap_free_minimal_tuple(slot->tts_mintuple);
 
 	/*
 	 * Drop the pin on the referenced buffer, if there is one.
@@ -492,7 +489,8 @@ ExecStoreMinimalTuple(MinimalTuple mtup,
 	 * Store the new tuple into the specified slot.
 	 */
 	slot->tts_isempty = false;
-	slot->tts_shouldFree = shouldFree;
+	slot->tts_shouldFree = false;
+	slot->tts_shouldFreeMin = shouldFree;
 	slot->tts_tuple = &slot->tts_minhdr;
 	slot->tts_mintuple = mtup;
 
@@ -526,16 +524,14 @@ ExecClearTuple(TupleTableSlot *slot)	/* slot in which to store tuple */
 	 * Free the old physical tuple if necessary.
 	 */
 	if (slot->tts_shouldFree)
-	{
-		if (slot->tts_mintuple)
-			heap_free_minimal_tuple(slot->tts_mintuple);
-		else
-			heap_freetuple(slot->tts_tuple);
-	}
+		heap_freetuple(slot->tts_tuple);
+	if (slot->tts_shouldFreeMin)
+		heap_free_minimal_tuple(slot->tts_mintuple);
 
 	slot->tts_tuple = NULL;
 	slot->tts_mintuple = NULL;
 	slot->tts_shouldFree = false;
+	slot->tts_shouldFreeMin = false;
 
 	/*
 	 * Drop the pin on the referenced buffer, if there is one.
@@ -616,6 +612,7 @@ ExecStoreAllNullTuple(TupleTableSlot *slot)
  *		ExecCopySlotTuple
  *			Obtain a copy of a slot's regular physical tuple.  The copy is
  *			palloc'd in the current memory context.
+ *			The slot itself is undisturbed.
  *
  *		This works even if the slot contains a virtual or minimal tuple;
  *		however the "system columns" of the result will not be meaningful.
@@ -631,15 +628,12 @@ ExecCopySlotTuple(TupleTableSlot *slot)
 	Assert(!slot->tts_isempty);
 
 	/*
-	 * If we have a physical tuple then just copy it.
+	 * If we have a physical tuple (either format) then just copy it.
 	 */
-	if (slot->tts_tuple)
-	{
-		if (slot->tts_mintuple)
-			return heap_tuple_from_minimal_tuple(slot->tts_mintuple);
-		else
-			return heap_copytuple(slot->tts_tuple);
-	}
+	if (TTS_HAS_PHYSICAL_TUPLE(slot))
+		return heap_copytuple(slot->tts_tuple);
+	if (slot->tts_mintuple)
+		return heap_tuple_from_minimal_tuple(slot->tts_mintuple);
 
 	/*
 	 * Otherwise we need to build a tuple from the Datum array.
@@ -653,6 +647,7 @@ ExecCopySlotTuple(TupleTableSlot *slot)
  *		ExecCopySlotMinimalTuple
  *			Obtain a copy of a slot's minimal physical tuple.  The copy is
  *			palloc'd in the current memory context.
+ *			The slot itself is undisturbed.
  * --------------------------------
  */
 MinimalTuple
@@ -665,15 +660,13 @@ ExecCopySlotMinimalTuple(TupleTableSlot *slot)
 	Assert(!slot->tts_isempty);
 
 	/*
-	 * If we have a physical tuple then just copy it.
+	 * If we have a physical tuple then just copy it.  Prefer to copy
+	 * tts_mintuple since that's a tad cheaper.
 	 */
+	if (slot->tts_mintuple)
+		return heap_copy_minimal_tuple(slot->tts_mintuple);
 	if (slot->tts_tuple)
-	{
-		if (slot->tts_mintuple)
-			return heap_copy_minimal_tuple(slot->tts_mintuple);
-		else
-			return minimal_tuple_from_heap_tuple(slot->tts_tuple);
-	}
+		return minimal_tuple_from_heap_tuple(slot->tts_tuple);
 
 	/*
 	 * Otherwise we need to build a tuple from the Datum array.
@@ -689,9 +682,11 @@ ExecCopySlotMinimalTuple(TupleTableSlot *slot)
  *
  *		If the slot contains a virtual tuple, we convert it to physical
  *		form.  The slot retains ownership of the physical tuple.
- *		Likewise, if it contains a minimal tuple we convert to regular form.
+ *		If it contains a minimal tuple we convert to regular form and store
+ *		that in addition to the minimal tuple (not instead of, because
+ *		callers may hold pointers to Datums within the minimal tuple).
  *
- * The difference between this and ExecMaterializeSlot() is that this
+ * The main difference between this and ExecMaterializeSlot() is that this
  * does not guarantee that the contained tuple is local storage.
  * Hence, the result must be treated as read-only.
  * --------------------------------
@@ -708,7 +703,7 @@ ExecFetchSlotTuple(TupleTableSlot *slot)
 	/*
 	 * If we have a regular physical tuple then just return it.
 	 */
-	if (slot->tts_tuple && slot->tts_mintuple == NULL)
+	if (TTS_HAS_PHYSICAL_TUPLE(slot))
 		return slot->tts_tuple;
 
 	/*
@@ -722,8 +717,10 @@ ExecFetchSlotTuple(TupleTableSlot *slot)
  *			Fetch the slot's minimal physical tuple.
  *
  *		If the slot contains a virtual tuple, we convert it to minimal
- *		physical form.	The slot retains ownership of the physical tuple.
- *		Likewise, if it contains a regular tuple we convert to minimal form.
+ *		physical form.	The slot retains ownership of the minimal tuple.
+ *		If it contains a regular tuple we convert to minimal form and store
+ *		that in addition to the regular tuple (not instead of, because
+ *		callers may hold pointers to Datums within the regular tuple).
  *
  * As above, the result must be treated as read-only.
  * --------------------------------
@@ -731,7 +728,6 @@ ExecFetchSlotTuple(TupleTableSlot *slot)
 MinimalTuple
 ExecFetchSlotMinimalTuple(TupleTableSlot *slot)
 {
-	MinimalTuple newTuple;
 	MemoryContext oldContext;
 
 	/*
@@ -741,28 +737,30 @@ ExecFetchSlotMinimalTuple(TupleTableSlot *slot)
 	Assert(!slot->tts_isempty);
 
 	/*
-	 * If we have a minimal physical tuple then just return it.
+	 * If we have a minimal physical tuple (local or not) then just return it.
 	 */
 	if (slot->tts_mintuple)
 		return slot->tts_mintuple;
 
 	/*
-	 * Otherwise, build a minimal tuple, and then store it as the new slot
-	 * value.  (Note: tts_nvalid will be reset to zero here.  There are cases
-	 * in which this could be optimized but it's probably not worth worrying
-	 * about.)
+	 * Otherwise, copy or build a minimal tuple, and store it into the slot.
 	 *
 	 * We may be called in a context that is shorter-lived than the tuple
 	 * slot, but we have to ensure that the materialized tuple will survive
 	 * anyway.
 	 */
 	oldContext = MemoryContextSwitchTo(slot->tts_mcxt);
-	newTuple = ExecCopySlotMinimalTuple(slot);
+	slot->tts_mintuple = ExecCopySlotMinimalTuple(slot);
+	slot->tts_shouldFreeMin = true;
 	MemoryContextSwitchTo(oldContext);
 
-	ExecStoreMinimalTuple(newTuple, slot, true);
+	/*
+	 * Note: we may now have a situation where we have a local minimal tuple
+	 * attached to a virtual or non-local physical tuple.  There seems no
+	 * harm in that at the moment, but if any materializes, we should change
+	 * this function to force the slot into minimal-tuple-only state.
+	 */
 
-	Assert(slot->tts_mintuple);
 	return slot->tts_mintuple;
 }
 
@@ -809,7 +807,6 @@ ExecFetchSlotTupleDatum(TupleTableSlot *slot)
 HeapTuple
 ExecMaterializeSlot(TupleTableSlot *slot)
 {
-	HeapTuple	newTuple;
 	MemoryContext oldContext;
 
 	/*
@@ -822,24 +819,47 @@ ExecMaterializeSlot(TupleTableSlot *slot)
 	 * If we have a regular physical tuple, and it's locally palloc'd, we have
 	 * nothing to do.
 	 */
-	if (slot->tts_tuple && slot->tts_shouldFree && slot->tts_mintuple == NULL)
+	if (slot->tts_tuple && slot->tts_shouldFree)
 		return slot->tts_tuple;
 
 	/*
-	 * Otherwise, copy or build a tuple, and then store it as the new slot
-	 * value.  (Note: tts_nvalid will be reset to zero here.  There are cases
-	 * in which this could be optimized but it's probably not worth worrying
-	 * about.)
+	 * Otherwise, copy or build a physical tuple, and store it into the slot.
 	 *
 	 * We may be called in a context that is shorter-lived than the tuple
 	 * slot, but we have to ensure that the materialized tuple will survive
 	 * anyway.
 	 */
 	oldContext = MemoryContextSwitchTo(slot->tts_mcxt);
-	newTuple = ExecCopySlotTuple(slot);
+	slot->tts_tuple = ExecCopySlotTuple(slot);
+	slot->tts_shouldFree = true;
 	MemoryContextSwitchTo(oldContext);
 
-	ExecStoreTuple(newTuple, slot, InvalidBuffer, true);
+	/*
+	 * Drop the pin on the referenced buffer, if there is one.
+	 */
+	if (BufferIsValid(slot->tts_buffer))
+		ReleaseBuffer(slot->tts_buffer);
+
+	slot->tts_buffer = InvalidBuffer;
+
+	/*
+	 * Mark extracted state invalid.  This is important because the slot
+	 * is not supposed to depend any more on the previous external data;
+	 * we mustn't leave any dangling pass-by-reference datums in tts_values.
+	 * However, we have not actually invalidated any such datums, if there
+	 * happen to be any previously fetched from the slot.  (Note in particular
+	 * that we have not pfree'd tts_mintuple, if there is one.)
+	 */
+	slot->tts_nvalid = 0;
+
+	/*
+	 * On the same principle of not depending on previous remote storage,
+	 * forget the mintuple if it's not local storage.  (If it is local storage,
+	 * we must not pfree it now, since callers might have already fetched
+	 * datum pointers referencing it.)
+	 */
+	if (!slot->tts_shouldFreeMin)
+		slot->tts_mintuple = NULL;
 
 	return slot->tts_tuple;
 }

src/include/executor/tuptable.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/executor/tuptable.h,v 1.40 2009/01/01 17:23:59 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/executor/tuptable.h,v 1.41 2009/03/30 04:08:43 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -49,6 +49,14 @@
  * else need to be "materialized" into physical tuples.  Note also that a
  * virtual tuple does not have any "system columns".
  *
+ * It is also possible for a TupleTableSlot to hold both physical and minimal
+ * copies of a tuple.  This is done when the slot is requested to provide
+ * the format other than the one it currently holds.  (Originally we attempted
+ * to handle such requests by replacing one format with the other, but that
+ * had the fatal defect of invalidating any pass-by-reference Datums pointing
+ * into the existing slot contents.)  Both copies must contain identical data
+ * payloads when this is the case.
+ *
  * The Datum/isnull arrays of a TupleTableSlot serve double duty.  When the
  * slot contains a virtual tuple, they are the authoritative data.	When the
  * slot contains a physical tuple, the arrays contain data extracted from
@@ -91,12 +99,12 @@
  *
  * tts_mintuple must always be NULL if the slot does not hold a "minimal"
  * tuple.  When it does, tts_mintuple points to the actual MinimalTupleData
- * object (the thing to be pfree'd if tts_shouldFree is true).  In this case
- * tts_tuple points at tts_minhdr and the fields of that are set correctly
+ * object (the thing to be pfree'd if tts_shouldFreeMin is true).  If the slot
+ * has only a minimal and not also a regular physical tuple, then tts_tuple
+ * points at tts_minhdr and the fields of that struct are set correctly
  * for access to the minimal tuple; in particular, tts_minhdr.t_data points
- * MINIMAL_TUPLE_OFFSET bytes before tts_mintuple.	(tts_mintuple is therefore
- * redundant, but for code simplicity we store it explicitly anyway.)  This
- * case otherwise behaves identically to the regular-physical-tuple case.
+ * MINIMAL_TUPLE_OFFSET bytes before tts_mintuple.  This allows column
+ * extraction to treat the case identically to regular physical tuples.
  *
  * tts_slow/tts_off are saved state for slot_deform_tuple, and should not
  * be touched by any other code.
@@ -106,20 +114,24 @@ typedef struct TupleTableSlot
 {
 	NodeTag		type;			/* vestigial ... allows IsA tests */
 	bool		tts_isempty;	/* true = slot is empty */
-	bool		tts_shouldFree; /* should pfree tuple? */
+	bool		tts_shouldFree;	/* should pfree tts_tuple? */
+	bool		tts_shouldFreeMin;		/* should pfree tts_mintuple? */
 	bool		tts_slow;		/* saved state for slot_deform_tuple */
-	HeapTuple	tts_tuple;		/* physical tuple, or NULL if none */
+	HeapTuple	tts_tuple;		/* physical tuple, or NULL if virtual */
 	TupleDesc	tts_tupleDescriptor;	/* slot's tuple descriptor */
 	MemoryContext tts_mcxt;		/* slot itself is in this context */
 	Buffer		tts_buffer;		/* tuple's buffer, or InvalidBuffer */
 	int			tts_nvalid;		/* # of valid values in tts_values */
 	Datum	   *tts_values;		/* current per-attribute values */
 	bool	   *tts_isnull;		/* current per-attribute isnull flags */
-	MinimalTuple tts_mintuple;	/* set if it's a minimal tuple, else NULL */
-	HeapTupleData tts_minhdr;	/* workspace if it's a minimal tuple */
+	MinimalTuple tts_mintuple;	/* minimal tuple, or NULL if none */
+	HeapTupleData tts_minhdr;	/* workspace for minimal-tuple-only case */
 	long		tts_off;		/* saved state for slot_deform_tuple */
 } TupleTableSlot;
 
+#define TTS_HAS_PHYSICAL_TUPLE(slot)  \
+	((slot)->tts_tuple != NULL && (slot)->tts_tuple != &((slot)->tts_minhdr))
+
 /*
  * Tuple table data structure: an array of TupleTableSlots.
  */

src/test/regress/expected/rangefuncs.out

@@ -743,3 +743,23 @@ select * from tt_log;
  16 | barlog
 (2 rows)
 
+-- test case for a whole-row-variable bug
+create function foo1(n integer, out a text, out b text)
+  returns setof record
+  language sql
+  as $$ select 'foo ' || i, 'bar ' || i from generate_series(1,$1) i $$;
+set work_mem='64kB';
+select t.a, t, t.a from foo1(10000) t limit 1;
+   a   |         t         |   a   
+-------+-------------------+-------
+ foo 1 | ("foo 1","bar 1") | foo 1
+(1 row)
+
+reset work_mem;
+select t.a, t, t.a from foo1(10000) t limit 1;
+   a   |         t         |   a   
+-------+-------------------+-------
+ foo 1 | ("foo 1","bar 1") | foo 1
+(1 row)
+
+drop function foo1(n integer);

src/test/regress/expected/with.out

@@ -451,6 +451,35 @@ SELECT t1.id, count(t2.*) FROM t AS t1 JOIN t AS t2 ON
   3 |     7
 (2 rows)
 
+-- this variant tickled a whole-row-variable bug in 8.4devel
+WITH RECURSIVE t(id, path) AS (
+    VALUES(1,ARRAY[]::integer[])
+UNION ALL
+    SELECT tree.id, t.path || tree.id
+    FROM tree JOIN t ON (tree.parent_id = t.id)
+)
+SELECT t1.id, t2.path, t2 FROM t AS t1 JOIN t AS t2 ON
+(t1.id=t2.id);
+ id |    path     |         t2         
+----+-------------+--------------------
+  1 | {}          | (1,{})
+  2 | {2}         | (2,{2})
+  3 | {3}         | (3,{3})
+  4 | {2,4}       | (4,"{2,4}")
+  5 | {2,5}       | (5,"{2,5}")
+  6 | {2,6}       | (6,"{2,6}")
+  7 | {3,7}       | (7,"{3,7}")
+  8 | {3,8}       | (8,"{3,8}")
+  9 | {2,4,9}     | (9,"{2,4,9}")
+ 10 | {2,4,10}    | (10,"{2,4,10}")
+ 11 | {3,7,11}    | (11,"{3,7,11}")
+ 12 | {3,7,12}    | (12,"{3,7,12}")
+ 13 | {3,7,13}    | (13,"{3,7,13}")
+ 14 | {2,4,9,14}  | (14,"{2,4,9,14}")
+ 15 | {3,7,11,15} | (15,"{3,7,11,15}")
+ 16 | {3,7,11,16} | (16,"{3,7,11,16}")
+(16 rows)
+
 --
 -- test cycle detection
 --

src/test/regress/sql/rangefuncs.sql

@@ -338,3 +338,16 @@ select * from tt;
 -- note that nextval() gets executed a second time in the rule expansion,
 -- which is expected.
 select * from tt_log;
+
+-- test case for a whole-row-variable bug
+create function foo1(n integer, out a text, out b text)
+  returns setof record
+  language sql
+  as $$ select 'foo ' || i, 'bar ' || i from generate_series(1,$1) i $$;
+
+set work_mem='64kB';
+select t.a, t, t.a from foo1(10000) t limit 1;
+reset work_mem;
+select t.a, t, t.a from foo1(10000) t limit 1;
+
+drop function foo1(n integer);

src/test/regress/sql/with.sql

@@ -250,6 +250,16 @@ SELECT t1.id, count(t2.*) FROM t AS t1 JOIN t AS t2 ON
 	GROUP BY t1.id
 	ORDER BY t1.id;
 
+-- this variant tickled a whole-row-variable bug in 8.4devel
+WITH RECURSIVE t(id, path) AS (
+    VALUES(1,ARRAY[]::integer[])
+UNION ALL
+    SELECT tree.id, t.path || tree.id
+    FROM tree JOIN t ON (tree.parent_id = t.id)
+)
+SELECT t1.id, t2.path, t2 FROM t AS t1 JOIN t AS t2 ON
+(t1.id=t2.id);
+
 --
 -- test cycle detection
 --