Обсуждение: heapam_tuple_complete_speculative : remove unnecessary tuple fetch

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
theprevious 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
liketo confirm. 

The attached patch just removes the unused tuple and shouldFree from this function. As touching the file, I also fixed
atypo in the file header comment. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

> 在 2026年3月24日，14:57，Chao Li <li.evan.chao@gmail.com> 写道：
> 
> 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
neededeither.
 
> 
> This appears to have been there since 5db6df0c011, where the function was introduced. It looks like a copy-pasto from
theprevious 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’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.

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> 写道：
> >
> > 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
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 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.

--
Best,
Xuneng