public inbox for [email protected]
help / color / mirror / Atom feedheapam_tuple_complete_speculative : remove unnecessary tuple fetch
4+ messages / 4 participants
[nested] [flat]
* heapam_tuple_complete_speculative : remove unnecessary tuple fetch
@ 2026-03-24 06:56 Chao Li <[email protected]>
0 siblings, 2 replies; 4+ messages in thread
From: Chao Li @ 2026-03-24 06:56 UTC (permalink / raw)
To: PostgreSQL Hackers <[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.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v1-0001-heapam_handler-remove-unused-tuple-fetch-in-specu.patch (1.7K, 2-v1-0001-heapam_handler-remove-unused-tuple-fetch-in-specu.patch)
download | inline diff:
From 576c596d20c67a92713da971e01a2b567901b76d Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Wed, 11 Mar 2026 14:14:57 +0800
Subject: [PATCH v1] heapam_handler: remove unused tuple fetch in speculative
completion
heapam_tuple_complete_speculative() fetched a tuple from the slot only to
free it immediately afterwards, without ever using it.
The function only needs slot->tts_tid to complete or abort the
speculative insertion, so remove the unnecessary fetch and pfree().
Author: Chao Li <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/backend/access/heap/heapam_handler.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 253a735b6c1..f0e2be3e85d 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -12,7 +12,7 @@
*
*
* NOTES
- * This files wires up the lower level heapam.c et al routines with the
+ * This file wires up the lower level heapam.c et al routines with the
* tableam abstraction.
*
*-------------------------------------------------------------------------
@@ -295,17 +295,11 @@ static void
heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *slot,
uint32 specToken, bool succeeded)
{
- bool shouldFree = true;
- HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
-
/* 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);
}
static TM_Result
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: heapam_tuple_complete_speculative : remove unnecessary tuple fetch
@ 2026-03-24 12:15 Japin Li <[email protected]>
parent: Chao Li <[email protected]>
1 sibling, 1 reply; 4+ messages in thread
From: Japin Li @ 2026-03-24 12:15 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
> 在 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.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: heapam_tuple_complete_speculative : remove unnecessary tuple fetch
@ 2026-03-31 01:09 Xuneng Zhou <[email protected]>
parent: Japin Li <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Xuneng Zhou @ 2026-03-31 01:09 UTC (permalink / raw)
To: Japin Li <[email protected]>; +Cc: Chao Li <[email protected]>; PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: heapam_tuple_complete_speculative : remove unnecessary tuple fetch
@ 2026-04-02 23:51 surya poondla <[email protected]>
parent: Chao Li <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: surya poondla @ 2026-04-02 23:51 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Hi Chao,
That's a nice catch.
I applied the patch, it passes make check, make check-world.
The change looks good to me.
Regards,
Surya Poondla
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-04-02 23:51 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-03-24 06:56 heapam_tuple_complete_speculative : remove unnecessary tuple fetch Chao Li <[email protected]>
2026-03-24 12:15 ` Japin Li <[email protected]>
2026-03-31 01:09 ` Xuneng Zhou <[email protected]>
2026-04-02 23:51 ` surya poondla <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox