On Tue, Mar 24, 2026 at 8:16 PM Japin Li <[email protected]> wrote: > > > > 在 2026年3月24日，14:57，Chao Li <[email protected]> 写道： > > > > Hi, > > > > While reviewing another patch, I noticed this: > > ``` > > static void > > heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *slot, > > uint32 specToken, bool succeeded) > > { > > bool shouldFree = true; > > HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree); // > > <== tuple is not used > > > > /* adjust the tuple's state accordingly */ > > if (succeeded) > > heap_finish_speculative(relation, &slot->tts_tid); > > else > > heap_abort_speculative(relation, &slot->tts_tid); > > > > if (shouldFree) > > pfree(tuple); > > } > > ``` > > > > In this function, tuple is not used at all, so there seems to be no need to > > fetch it, and shouldFree is thus not needed either. > > > > This appears to have been there since 5db6df0c011, where the function was > > introduced. It looks like a copy-pasto from the previous function, > > heapam_tuple_insert_speculative(), which does need to fetch and possibly > > free the tuple. > > > > I tried simply removing ExecFetchSlotHeapTuple(), and "make check" still > > passes. But I may be missing something, so I’d like to confirm. > > > > The attached patch just removes the unused tuple and shouldFree from this > > function. As touching the file, I also fixed a typo in the file header > > comment. > > Makes sense! All test cases passed with make check-world. >
+1, looks like a simple copy-pasto and the patch LGTM. -- Best, Xuneng
