From 22e44e8dbcfe4b9f3c4189f07a2361c951d72514 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 6 Nov 2019 12:00:17 -0500 Subject: [PATCH] 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 --- src/backend/executor/execTuples.c | 81 ++++++++++++++++--------------- src/include/executor/tuptable.h | 5 +- 2 files changed, 45 insertions(+), 41 deletions(-) diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c index 87bc510b31..46d55d944a 100644 --- 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 885b481d9a..b7f977233b 100644 --- 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;