Re: Eliminate redundant tuple visibility check in vacuum

[ sorry for the delay getting back to this ]

On Wed, Sep 13, 2023 at 1:29 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> I think it might be worth making the names a bit less ambiguous than they
> were. It's a bit odd that one has "new" in the name, the other doesn't,
> despite both being about newly marked things. And "deleted" seems somewhat
> ambiguous, it could also be understood as marking things LP_DEAD. Maybe
> nnewunused?

I like it the better the way Melanie did it. The current name may not
be for the best, but that can be changed some other time, in a
separate patch, if someone likes. For now, changing the name seems
like a can of worms we don't need to open; the existing names have
precedent on their side if nothing else.

> > static int heap_prune_chain(Buffer buffer,
> > OffsetNumber rootoffnum,
> > + int8 *htsv,
> > PruneState *prstate);
>
> Hm, do we really want to pass this explicitly to a bunch of functions? Seems
> like it might be better to either pass the PruneResult around or to have a
> pointer in PruneState?

As far as I can see, 0002 adds it to one function (heap_page_pune) and
0003 adds it to one more (heap_prune_chain). That's not much of a
bunch.

> > /*
> > * The criteria for counting a tuple as live in this block need to
> > @@ -1682,7 +1664,7 @@ retry:
> > * (Cases where we bypass index vacuuming will violate this optimistic
> > * assumption, but the overall impact of that should be negligible.)
> > */
> > - switch (res)
> > + switch ((HTSV_Result) presult.htsv[offnum])
> > {
> > case HEAPTUPLE_LIVE:
>
> I think we should assert that we have a valid HTSV_Result here, i.e. not
> -1. You could wrap the cast and Assert into an inline funciton as well.

This isn't a bad idea, although I don't find it completely necessary either.

--
Robert Haas
EDB: http://www.enterprisedb.com