On Thu, Sep 7, 2023 at 6:23 PM Melanie Plageman
<melanieplageman@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