On Fri, Sep 8, 2023 at 11:06 AM Robert Haas <robertmh...@gmail.com> wrote: > > 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.
This is a good idea. I will work on a separate patch set to add an error context callback for on-access HOT pruning. - Melanie
