Re: Eliminate redundant tuple visibility check in vacuum

From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, David Geier <geidav(dot)pg(at)gmail(dot)com>
Subject: Re: Eliminate redundant tuple visibility check in vacuum
Date: 2023-09-11 12:04:22
Message-ID: CAAKRu_a-PE8vYBfBe8jyk2FdN9=XLiN1UtJsGgWB1H42W4hrJg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2023-09-11 12:11:13 Re: Build the docs if there are changes in docs and don't run other tasks if the changes are only in docs
Previous Message Jelte Fennema 2023-09-11 12:03:22 Re: proposal: psql: show current user in prompt