On Wed, Sep 6, 2023 at 1:04 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Aug 31, 2023 at 6:29 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > I have changed this.
>
> I spent a bunch of time today looking at this, thinking maybe I could
> commit it. But then I got cold feet.
>
> With these patches applied, PruneResult ends up being declared in
> heapam.h, with a comment that says /* State returned from pruning */.
> But that comment isn't accurate. The two new members that get added to
> the structure by 0002, namely nnewlpdead and htsv, are in fact state
> that is returned from pruning. But the other 5 members aren't. They're
> just initialized to constant values by pruning and then filled in for
> real by the vacuum logic. That's extremely weird. It would be fine if
> heap_page_prune() just grew a new output argument that only returned
> the HTSV results, or perhaps it could make sense to bundle any
> existing out parameters together into a struct and then add new things
> to that struct instead of adding even more parameters to the function
> itself. But there doesn't seem to be any good reason to muddle
> together the new output parameters for heap_page_prune() with a bunch
> of state that is currently internal to vacuumlazy.c.
>
> I realize that the shape of the patches probably stems from the fact
> that they started out life as part of a bigger patch set. But to be
> committed independently, they need to be shaped in a way that makes
> sense independently, and I don't think this qualifies. On the plus
> side, it seems to me that it's probably not that much work to fix this
> issue and that the result would likely be a smaller patch than what
> you have now, which is something.
Yeah, I think this is a fair concern. I have addressed it in the
attached patches.
I thought a lot about whether or not adding a PruneResult which
contains only the output parameters and result of heap_page_prune() is
annoying since we have so many other *Prune* data structures. I
decided it's not annoying. In some cases, four outputs don't merit a
new structure. In this case, I think it declutters the code a bit --
independent of any other patches I may be writing :)
- Melanie
