On Thu, Aug 31, 2023 at 6:29 PM Melanie Plageman <melanieplage...@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. -- Robert Haas EDB: http://www.enterprisedb.com
