public inbox for [email protected]  

help / color / mirror / Atom feed

From: Heikki Linnakangas <[email protected]>
To: Xuneng Zhou <[email protected]>
To: Japin Li <[email protected]>
To: Chao Li <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: heapam_tuple_complete_speculative : remove unnecessary tuple fetch
Date: Wed, 8 Jul 2026 01:17:25 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CABPTF7XJ3gv+R+4+eGMX=ggN1m6bD7=rbxDi-cHN+GJUV5jSYg@mail.gmail.com>
References: <[email protected]>
	<SY7PR01MB1092127415D38E676B1B0BAA0B648A@SY7PR01MB10921.ausprd01.prod.outlook.com>
	<CABPTF7XJ3gv+R+4+eGMX=ggN1m6bD7=rbxDi-cHN+GJUV5jSYg@mail.gmail.com>

On 31/03/2026 04:09, Xuneng Zhou wrote:
> On Tue, Mar 24, 2026 at 8:16 PM Japin Li <[email protected]> wrote:
>>> 在 2026年3月24日，14:57，Chao Li <[email protected]> 写道：
>>> 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 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’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.

Committed, thanks!

- Heikki

view thread (5+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: heapam_tuple_complete_speculative : remove unnecessary tuple fetch
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox