Thread
Commits
-
tableam: Add tuple_{insert, delete, update, lock} and use.
- 5db6df0c0117 12.0 cited
-
heapam_tuple_complete_speculative : remove unnecessary tuple fetch
Chao Li <li.evan.chao@gmail.com> — 2026-03-24T06:56:28Z
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/ -
Re: heapam_tuple_complete_speculative : remove unnecessary tuple fetch
Japin Li <japinli@hotmail.com> — 2026-03-24T12:15:50Z
> 在 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 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. -
Re: heapam_tuple_complete_speculative : remove unnecessary tuple fetch
Xuneng Zhou <xunengzhou@gmail.com> — 2026-03-31T01:09:47Z
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 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 -
Re: heapam_tuple_complete_speculative : remove unnecessary tuple fetch
surya poondla <suryapoondla4@gmail.com> — 2026-04-02T23:51:09Z
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
-
Re: heapam_tuple_complete_speculative : remove unnecessary tuple fetch
Heikki Linnakangas <hlinnaka@iki.fi> — 2026-07-07T22:17:25Z
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