Minor code review for tuple slot rewrite.

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
index 87bc510b31bff2a1a8946a15232441fb137a0ff4..46d55d944af53fe9d02440acd480a53e1dff397a 100644 (file)
--- 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
@@ -273,7 +273,6 @@ tts_virtual_copy_heap_tuple(TupleTableSlot *slot)
        return heap_form_tuple(slot->tts_tupleDescriptor,
                                                   slot->tts_values,
                                                   slot->tts_isnull);
-
 }
 
 static MinimalTuple
@@ -335,6 +334,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);
 }
@@ -347,14 +348,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,
@@ -369,12 +375,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);
 }
@@ -437,7 +438,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)
@@ -510,13 +511,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,
@@ -533,19 +540,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
@@ -616,8 +618,6 @@ tts_minimal_store_tuple(TupleTableSlot *slot, MinimalTuple mtup, bool shouldFree
 
        if (shouldFree)
                slot->tts_flags |= TTS_FLAG_SHOULDFREE;
-       else
-               Assert(!TTS_SHOULDFREE(slot));
 }
 
 
@@ -652,8 +652,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))
@@ -682,6 +680,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);
 }
@@ -694,14 +694,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)
        {
                /*
@@ -714,7 +719,6 @@ tts_buffer_heap_materialize(TupleTableSlot *slot)
                bslot->base.tuple = heap_form_tuple(slot->tts_tupleDescriptor,
                                                                                        slot->tts_values,
                                                                                        slot->tts_isnull);
-
        }
        else
        {
@@ -724,19 +728,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
@@ -757,10 +763,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
@@ -1445,10 +1451,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)
@@ -1857,7 +1863,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
index 885b481d9a476d2b604875d47c9d6dcf61cc8167..b7f977233be9c19b3f5d6aae4922908abe339bda 100644 (file)
--- 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;