On 31/03/2026 04:09, Xuneng Zhou wrote:
> On Tue, Mar 24, 2026 at 8:16 PM Japin Li <japinli@hotmail.com> wrote:
>>> 在 2026年3月24日，14:57，Chao Li <li.evan.chao@gmail.com> 写道：
>>> 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
neededeither.
>>>
>>> This appears to have been there since 5db6df0c011, where the function was introduced. It looks like a copy-pasto
fromthe previous function, heapam_tuple_insert_speculative(), which does need to fetch and possibly free the tuple.
I have a slightly different theory on how it came to be. Before commit
5db6df0c011, the heap_finish_speculative() and heap_abort_speculative()
functions took HeapTuple as argument, but only needed the TID from it
(tuple->t_self). Commit 5db6df0c011 changed it so that they are passed
just the TID directly, because with that commit, the caller didn't have
a HeapTuple readily at hand anymore. But the commit *also* made the
caller fetch the tuple. So I think during the development of that patch,
at some point the caller really had to fetch the tuple, so that it could
pass it to the heap_finish/abort_speculative() functions. Those
functions were later changed to take just the TID, but we forgot to
change the caller to take advantage of that.
Anyway, doesn't matter how exactly we got here, the end result is the
same, and the patch looks good to me.
>>> I tried simply removing ExecFetchSlotHeapTuple(), and "make check" still passes. But I may be missing something, so
I’dlike to confirm.
>>>
>>> The attached patch just removes the unused tuple and shouldFree from this function. As touching the file, I also
fixeda 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.
Committed, thanks!
- Heikki
