Message-ID: <4ae3a6dd-a4ae-43f9-9625-4dbf2084f790@iki.fi>
Date: Wed, 8 Jul 2026 01:17:25 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: heapam_tuple_complete_speculative : remove unnecessary tuple
 fetch
To: Xuneng Zhou <xunengzhou@gmail.com>, Japin Li <japinli@hotmail.com>,
 Chao Li <li.evan.chao@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <FCB61654-575D-4F08-AA7E-ED462EDE48A7@gmail.com>
 <SY7PR01MB1092127415D38E676B1B0BAA0B648A@SY7PR01MB10921.ausprd01.prod.outlook.com>
 <CABPTF7XJ3gv+R+4+eGMX=ggN1m6bD7=rbxDi-cHN+GJUV5jSYg@mail.gmail.com>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: 
 <CABPTF7XJ3gv+R+4+eGMX=ggN1m6bD7=rbxDi-cHN+GJUV5jSYg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/4ae3a6dd-a4ae-43f9-9625-4dbf2084f790%40iki.fi>
Precedence: bulk

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 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