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
isno way to distinct if a field has been set or not because of random values. From this perspective, zero-out presult
maymake 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
