diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c index d3232017e0..6499549177 100644 --- a/src/backend/access/common/heaptuple.c +++ b/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); } diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c index 30fa903a89..1d0a5854fb 100644 --- a/src/backend/executor/execTuples.c +++ b/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; } diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h index 1cd624b085..d712106c52 100644 --- a/src/include/executor/tuptable.h +++ b/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. */ diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out index 8b475834b9..a5cff8ca43 100644 --- a/src/test/regress/expected/rangefuncs.out +++ b/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); diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out index db4097401c..4a2f18c9e9 100644 --- a/src/test/regress/expected/with.out +++ b/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 -- diff --git a/src/test/regress/sql/rangefuncs.sql b/src/test/regress/sql/rangefuncs.sql index 6d10c99aab..3c68f0aa44 100644 --- a/src/test/regress/sql/rangefuncs.sql +++ b/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); diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql index d79be72a35..c736441f53 100644 --- a/src/test/regress/sql/with.sql +++ b/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 --