Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Add explanatory comment to prune_freeze_setup()
- bfe5c4bec750 19 (unreleased) landed
-
Fix uninitialized PruneFreezeResult in pruneheap and vacuumlazy
Chao Li <li.evan.chao@gmail.com> — 2025-12-11T04:02:35Z
Hi Hackers, While reviewing Melanie's patch [1], I found this bug where presult is not initialized. Let me explain the logic. In the first place: ``` static int lazy_scan_prune(LVRelState *vacrel, Buffer buf, BlockNumber blkno, Page page, Buffer vmbuffer, bool all_visible_according_to_vm, bool *has_lpdead_items, bool *vm_page_frozen) { Relation rel = vacrel->rel; PruneFreezeResult presult; <== here presult is not initialized heap_page_prune_and_freeze(¶ms, &presult, <== uninitialized presult is passed into heap_page_prune_and_freeze &vacrel->offnum, &vacrel->NewRelfrozenXid, &vacrel->NewRelminMxid); ``` Then in heap_page_prune_and_freeze(): ``` void heap_page_prune_and_freeze(PruneFreezeParams *params, PruneFreezeResult *presult, OffsetNumber *off_loc, TransactionId *new_relfrozen_xid, MultiXactId *new_relmin_mxid) { Buffer buffer = params->buffer; Page page = BufferGetPage(buffer); PruneState prstate; bool do_freeze; bool do_prune; bool do_hint_prune; bool did_tuple_hint_fpi; int64 fpi_before = pgWalUsage.wal_fpi; /* Initialize prstate */ prune_freeze_setup(params, new_relfrozen_xid, new_relmin_mxid, presult, &prstate); <== immediately pass uninitialized presult to prune_freeze_setup ``` Then in prune_freeze_setup(): ``` static void prune_freeze_setup(PruneFreezeParams *params, TransactionId *new_relfrozen_xid, MultiXactId *new_relmin_mxid, const PruneFreezeResult *presult, <== presult is a const pointer, so prune_freeze_setup won’t update its content PruneState *prstate) { prstate->deadoffsets = (OffsetNumber *) presult->deadoffsets; <== presult->deadoffsets could be a random value } ``` Attached is a simple fix by just initializing presult in the first place with {0}. [1] https://postgr.es/m/CAAKRu_bvQiesumRhgvbcym1T9Z9= ddGfUbi-dSNxLRc6JvL1-w@mail.gmail.com Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix uninitialized PruneFreezeResult in pruneheap and vacuumlazy
Feilong Meng <feelingmeng@foxmail.com> — 2025-12-11T10:17:39Z
Hi Chao Li,Hackers, > PruneFreezeResult presult; <== here presult is not initialized > const PruneFreezeResult *presult, <== presult is a const pointer, so prune_freeze_setup won’t update its content > PruneState *prstate) > { > prstate->deadoffsets = (OffsetNumber *) presult->deadoffsets; <== presult->deadoffsets could be a random value > } > ``` > Attached is a simple fix by just initializing presult in the first place with {0}. Initializing ’presult‘ under my operating system is very effective. I have tested the change, and "make check" passed. Best regards！ Feilong Meng feelingmeng@foxmail.com HighGo Software Co., Ltd. https://www.highgo.com/ Original From: Chao Li <li.evan.chao@gmail.com> Date: 2025-12-11 12:02 To: Postgres hackers <pgsql-hackers@lists.postgresql.org> Cc: Melanie Plageman <melanieplageman@gmail.com> Subject: Fix uninitialized PruneFreezeResult in pruneheap and vacuumlazy Hi Hackers, While reviewing Melanie's patch [1], I found this bug where presult is not initialized. Let me explain the logic. In the first place: ``` static int lazy_scan_prune(LVRelState *vacrel, Buffer buf, BlockNumber blkno, Page page, Buffer vmbuffer, bool all_visible_according_to_vm, bool *has_lpdead_items, bool *vm_page_frozen) { Relation rel = vacrel->rel; PruneFreezeResult presult; <== here presult is not initialized heap_page_prune_and_freeze(&params, &presult, <== uninitialized presult is passed into heap_page_prune_and_freeze &vacrel->offnum, &vacrel->NewRelfrozenXid, &vacrel->NewRelminMxid); ``` Then in heap_page_prune_and_freeze(): ``` void heap_page_prune_and_freeze(PruneFreezeParams *params, PruneFreezeResult *presult, OffsetNumber *off_loc, TransactionId *new_relfrozen_xid, MultiXactId *new_relmin_mxid) { Buffer buffer = params->buffer; Page page = BufferGetPage(buffer); PruneState prstate; bool do_freeze; bool do_prune; bool do_hint_prune; bool did_tuple_hint_fpi; int64 fpi_before = pgWalUsage.wal_fpi; /* Initialize prstate */ prune_freeze_setup(params, new_relfrozen_xid, new_relmin_mxid, presult, &prstate); <== immediately pass uninitialized presult to prune_freeze_setup ``` Then in prune_freeze_setup(): ``` static void prune_freeze_setup(PruneFreezeParams *params, TransactionId *new_relfrozen_xid, MultiXactId *new_relmin_mxid, const PruneFreezeResult *presult, <== presult is a const pointer, so prune_freeze_setup won’t update its content PruneState *prstate) { prstate->deadoffsets = (OffsetNumber *) presult->deadoffsets; <== presult->deadoffsets could be a random value } ``` Attached is a simple fix by just initializing presult in the first place with {0}. [1] https://postgr.es/m/CAAKRu_bvQiesumRhgvbcym1T9Z9=ddGfUbi-dSNxLRc6JvL1-w@mail.gmail.com Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix uninitialized PruneFreezeResult in pruneheap and vacuumlazy
Melanie Plageman <melanieplageman@gmail.com> — 2025-12-11T14:59:04Z
On Wed, Dec 10, 2025 at 11:02 PM Chao Li <li.evan.chao@gmail.com> wrote: > > While reviewing Melanie's patch [1], I found this bug where presult is not initialized. Let me explain the logic. Thanks for looking closely at the code. > static int > lazy_scan_prune(LVRelState *vacrel, > presult, &prstate); <== immediately pass uninitialized presult to prune_freeze_setup > > Then in prune_freeze_setup(): > > prstate->deadoffsets = (OffsetNumber *) presult->deadoffsets; <== presult->deadoffsets could be a random value > > Attached is a simple fix by just initializing presult in the first place with {0}. I don't think zero-initializing deadoffsets is needed. We don't read offsets from it in heap_page_prune_and_freeze() -- it's a result variable. We only set offsets in it (see heap_prune_record_dead()). And because we track exactly how many are initialized in prstate->lpdead_items, the caller (lazy_scan_heap() via lazy_scan_prune()) will only access those dead offsets which have been initialized. I think this is a pretty common pattern in C. We don't zero-initialize the other arrays of offsets in the PruneState (redirected, dead, etc) and, for example, lazy_scan_noprune() doesn't zero-initialize the deadoffsets array that it fills in. The reason PruneFreezeResult is passed into prune_freeze_setup() is that we save a pointer to the deadoffsets array in the PruneState instead of having a copy of the whole array (to save stack space and effort copying the array from PruneState into PruneFreezeResult at the end). Other than that, we wait to initialize PruneFreezeResult's members until the end of heap_page_prune_and_freeze() to make it clear that we are actually setting all the members. If we filled them out throughout the various functions and helpers, it would be less clear that we have filled in all the return values. I could add a comment to prune_freeze_setup() where we save the deadoffsets pointer that explains why we are doing that instead of just having a deadoffsets array in the PruneState. Would that help with the confusion? - Melanie -
Re: Fix uninitialized PruneFreezeResult in pruneheap and vacuumlazy
Chao Li <li.evan.chao@gmail.com> — 2025-12-11T22:36:50Z
> On Dec 11, 2025, at 22:59, Melanie Plageman <melanieplageman@gmail.com> wrote: > > On Wed, Dec 10, 2025 at 11:02 PM Chao Li <li.evan.chao@gmail.com> wrote: >> >> While reviewing Melanie's patch [1], I found this bug where presult is not initialized. Let me explain the logic. > > Thanks for looking closely at the code. > >> static int >> lazy_scan_prune(LVRelState *vacrel, >> presult, &prstate); <== immediately pass uninitialized presult to prune_freeze_setup >> >> Then in prune_freeze_setup(): >> >> prstate->deadoffsets = (OffsetNumber *) presult->deadoffsets; <== presult->deadoffsets could be a random value >> >> Attached is a simple fix by just initializing presult in the first place with {0}. > > I don't think zero-initializing deadoffsets is needed. We don't read > offsets from it in heap_page_prune_and_freeze() -- it's a result > variable. We only set offsets in it (see heap_prune_record_dead()). > And because we track exactly how many are initialized in > prstate->lpdead_items, the caller (lazy_scan_heap() via > lazy_scan_prune()) will only access those dead offsets which have been > initialized. I think this is a pretty common pattern in C. We don't > zero-initialize the other arrays of offsets in the PruneState > (redirected, dead, etc) and, for example, lazy_scan_noprune() doesn't > zero-initialize the deadoffsets array that it fills in. Thanks for the explanation. I didn’t notice deadoffsets is an array, so prune_freeze_setup() only assign the array address to prstate, which doesn’t care about content stored in the array. In this case, initializing presult is not required. > > The reason PruneFreezeResult is passed into prune_freeze_setup() is > that we save a pointer to the deadoffsets array in the PruneState > instead of having a copy of the whole array (to save stack space and > effort copying the array from PruneState into PruneFreezeResult at the > end). > > Other than that, we wait to initialize PruneFreezeResult's members > until the end of heap_page_prune_and_freeze() to make it clear that we > are actually setting all the members. If we filled them out throughout > the various functions and helpers, it would be less clear that we have > filled in all the return values. I don’t get this point. presult is a local variable defined in the caller function, filling with random values, there is no way to distinct if a field has been set or not because of random values. From this perspective, zero-out presult may make it clear that we are actually setting the members. > > I could add a comment to prune_freeze_setup() where we save the > deadoffsets pointer that explains why we are doing that instead of > just having a deadoffsets array in the PruneState. Would that help > with the confusion? > That will be great. From “clearly knowing which members are set” perspective, I still feel initializing presult = {0} is useful, at least harmless. There are only 2 places, not a big change. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix uninitialized PruneFreezeResult in pruneheap and vacuumlazy
Melanie Plageman <melanieplageman@gmail.com> — 2025-12-16T16:05:56Z
On Thu, Dec 11, 2025 at 5:37 PM Chao Li <li.evan.chao@gmail.com> wrote: > > > On Dec 11, 2025, at 22:59, Melanie Plageman <melanieplageman@gmail.com> wrote: > > > > The reason PruneFreezeResult is passed into prune_freeze_setup() is > > that we save a pointer to the deadoffsets array in the PruneState > > instead of having a copy of the whole array (to save stack space and > > effort copying the array from PruneState into PruneFreezeResult at the > > end). > > > > Other than that, we wait to initialize PruneFreezeResult's members > > until the end of heap_page_prune_and_freeze() to make it clear that we > > are actually setting all the members. If we filled them out throughout > > the various functions and helpers, it would be less clear that we have > > filled in all the return values. > > I don’t get this point. presult is a local variable defined in the caller function, filling with random values, there is no way to distinct if a field has been set or not because of random values. From this perspective, zero-out presult may make it clear that we are actually setting the members. The PruneFreezeResult is only initialized in a single place: at the end of heap_page_prune_and_freeze() right before returning to the caller. I find that more clear. In fact, zero-initializing it can make it less clear if the fields have been initialized. Valgrind or other tools can't detect uninitialized access if you zero it out. And, though currently most fields in PruneFreezeResult have zero as a default value, future fields may not have zero as the default value. For example, vm_conflict_horizon cannot be zero (InvalidTransactionId) if the page is all-visible but not all-frozen. > > I could add a comment to prune_freeze_setup() where we save the > > deadoffsets pointer that explains why we are doing that instead of > > just having a deadoffsets array in the PruneState. Would that help > > with the confusion? > > That will be great. I've committed this. Thanks again for taking a close look at my patches! - Melanie