Re: Eliminate redundant tuple visibility check in vacuum

On Fri, Sep 8, 2023 at 11:06 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Sep 7, 2023 at 6:23 PM Melanie Plageman
> <melanieplageman(at)gmail(dot)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