Fix uninitialized PruneFreezeResult in pruneheap and vacuumlazy

Hi Chao Li,Hackers,

&gt; PruneFreezeResult presult; <== here presult is not initialized

&gt; &nbsp; const PruneFreezeResult *presult, <== presult is a const pointer, so prune_freeze_setup won’t update its content
&gt; &nbsp; PruneState *prstate)
&gt; {
&gt; &nbsp; &nbsp; prstate-&gt;deadoffsets = (OffsetNumber *) presult-&gt;deadoffsets; <== presult-&gt;deadoffsets could be a random value
&gt; }
&gt; ```

&gt; Attached is a simple fix by just initializing presult in the first place with {0}.&nbsp;

&nbsp; &nbsp; Initializing ’presult‘ under my operating system is very effective.
&nbsp; &nbsp; I have tested the change, and "make check" passed.

Best regards！

Feilong&nbsp;Meng
feelingmeng@foxmail.com

HighGo&nbsp;Software&nbsp;Co.,&nbsp;Ltd.

https://www.highgo.com/

Original

From: Chao Li <li.evan.chao@gmail.com&gt;
Date: 2025-12-11 12:02
To: Postgres hackers <pgsql-hackers@lists.postgresql.org&gt;
Cc: Melanie Plageman <melanieplageman@gmail.com&gt;
Subject: Fix uninitialized PruneFreezeResult in pruneheap and vacuumlazy

Hi Hackers,

While reviewing Melanie's patch [1]/messages/by-id/CAAKRu_bvQiesumRhgvbcym1T9Z9=ddGfUbi-dSNxLRc6JvL1-w@mail.gmail.com, 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-&gt;rel;
PruneFreezeResult presult; <== here presult is not initialized

heap_page_prune_and_freeze(&amp;params,
&nbsp; &amp;presult, <== uninitialized presult is passed into&nbsp;heap_page_prune_and_freeze
&nbsp; &amp;vacrel-&gt;offnum,
&nbsp; &amp;vacrel-&gt;NewRelfrozenXid, &amp;vacrel-&gt;NewRelminMxid);
```

Then in&nbsp;heap_page_prune_and_freeze():
```
void
heap_page_prune_and_freeze(PruneFreezeParams *params,
&nbsp; PruneFreezeResult *presult,
&nbsp; OffsetNumber *off_loc,
&nbsp; TransactionId *new_relfrozen_xid,
&nbsp; MultiXactId *new_relmin_mxid)
{
Buffer buffer = params-&gt;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,
&nbsp; new_relfrozen_xid, new_relmin_mxid,
&nbsp; presult, &amp;prstate); <== immediately pass&nbsp;uninitialized presult to&nbsp;prune_freeze_setup
```

Then in&nbsp;prune_freeze_setup():
```
static void
prune_freeze_setup(PruneFreezeParams *params,
&nbsp; TransactionId *new_relfrozen_xid,
&nbsp; MultiXactId *new_relmin_mxid,
&nbsp; const PruneFreezeResult *presult, <== presult is a const pointer, so&nbsp;prune_freeze_setup won’t update its content
&nbsp; PruneState *prstate)
{
&nbsp; &nbsp; prstate-&gt;deadoffsets = (OffsetNumber *) presult-&gt;deadoffsets; <==&nbsp;presult-&gt;deadoffsets could be a random value
}
```

Attached is a simple fix by just initializing presult in the first place with {0}.&nbsp;

[1]: /messages/by-id/CAAKRu_bvQiesumRhgvbcym1T9Z9=ddGfUbi-dSNxLRc6JvL1-w@mail.gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

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/

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