On Thu, Sep 7, 2023 at 6:23 PM Melanie Plageman <melanieplage...@gmail.com> wrote: > I mostly wanted to remove the NULL checks because I found them > distracting (so, a stylistic complaint). However, upon further > reflection, I actually think it is better if heap_page_prune_opt() > passes NULL. heap_page_prune() has no error callback mechanism that > could use it, and passing a valid value implies otherwise. Also, as > you said, off_loc will always be InvalidOffsetNumber when > heap_page_prune() returns normally, so having heap_page_prune_opt() > pass a dummy value might actually be more confusing for future > programmers.
I'll look at the new patches more next week, but I wanted to comment on this point. I think this is kind of six of one, half a dozen of the other. It's not that hard to spot a variable that's only used in a function call and never initialized beforehand or used afterward, and if someone really feels the need to hammer home the point, they could always name it dummy or dummy_loc or whatever. So this point doesn't really carry a lot of weight with me. I actually think that the proposed change is probably better, but it seems like such a minor improvement that I get slightly reluctant to make it, only because churning the source code for arguable points sometimes annoys other developers. But I also had the thought that maybe it wouldn't be such a terrible idea if heap_page_prune_opt() actually used off_loc for some error reporting goodness. I mean, if HOT pruning fails, and we don't get the detail as to which tuple caused the failure, we can always run VACUUM and it will give us that information, assuming of course that the same failure happens again. But is there any reason why HOT pruning shouldn't include that error detail? If it did, then off_loc would be passed by all callers, at which point we surely would want to get rid of the branches. -- Robert Haas EDB: http://www.enterprisedb.com
