diff options
| -rw-r--r-- | src/backend/access/common/heaptuple.c | 6 | ||||
| -rw-r--r-- | src/backend/executor/execTuples.c | 134 | ||||
| -rw-r--r-- | src/include/executor/tuptable.h | 32 | ||||
| -rw-r--r-- | src/test/regress/expected/rangefuncs.out | 20 | ||||
| -rw-r--r-- | src/test/regress/sql/rangefuncs.sql | 13 |
5 files changed, 135 insertions, 70 deletions
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c index 608f9c867ae..4affea6d04c 100644 --- a/src/backend/access/common/heaptuple.c +++ b/src/backend/access/common/heaptuple.c @@ -16,7 +16,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/access/common/heaptuple.c,v 1.112 2006/11/23 05:27:18 neilc Exp $ + * $PostgreSQL: pgsql/src/backend/access/common/heaptuple.c,v 1.112.2.1 2009/03/30 04:09:18 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -1297,7 +1297,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); } @@ -1483,7 +1483,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 fb1fdf9e723..336cf9ed030 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.98 2006/10/04 00:29:52 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/executor/execTuples.c,v 1.98.2.1 2009/03/30 04:09:18 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; } @@ -782,7 +780,6 @@ ExecFetchSlotMinimalTuple(TupleTableSlot *slot) HeapTuple ExecMaterializeSlot(TupleTableSlot *slot) { - HeapTuple newTuple; MemoryContext oldContext; /* @@ -795,24 +792,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 c34234c836d..93b3844f63c 100644 --- a/src/include/executor/tuptable.h +++ b/src/include/executor/tuptable.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/executor/tuptable.h,v 1.36 2006/10/04 00:30:08 momjian Exp $ + * $PostgreSQL: pgsql/src/include/executor/tuptable.h,v 1.36.2.1 2009/03/30 04:09:18 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 000d4e36c52..4fedc41ed86 100644 --- a/src/test/regress/expected/rangefuncs.out +++ b/src/test/regress/expected/rangefuncs.out @@ -528,3 +528,23 @@ CREATE FUNCTION bad (f1 int, out f2 anyelement, out f3 anyarray) AS 'select $1, array[$1,$1]' LANGUAGE sql; ERROR: cannot determine result data type DETAIL: A function returning "anyarray" or "anyelement" must have at least one argument of either type. +-- 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/sql/rangefuncs.sql b/src/test/regress/sql/rangefuncs.sql index c56e35ded4e..baa56c7355c 100644 --- a/src/test/regress/sql/rangefuncs.sql +++ b/src/test/regress/sql/rangefuncs.sql @@ -261,3 +261,16 @@ DROP FUNCTION dup(anyelement); -- fails, no way to deduce outputs CREATE FUNCTION bad (f1 int, out f2 anyelement, out f3 anyarray) AS 'select $1, array[$1,$1]' LANGUAGE sql; + +-- 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); |