On Thu, Dec 11, 2025 at 5:37 PM Chao Li <[email protected]> wrote: > > > On Dec 11, 2025, at 22:59, Melanie Plageman <[email protected]> > > 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
