Re: Emit fewer vacuum records by reaping removable tuples during pruning

On Fri, Jan 5, 2024 at 2:47 PM Andres Freund <andres@anarazel.de> wrote:
> On 2024-01-04 17:37:27 -0500, Melanie Plageman wrote:
> > On Thu, Jan 4, 2024 at 3:03 PM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > On 2023-11-17 18:12:08 -0500, Melanie Plageman wrote:
> > > > @@ -972,20 +970,21 @@ lazy_scan_heap(LVRelState *vacrel)
> > > >                               continue;
> > > >                       }
> > > >
> > > > -                     /* Collect LP_DEAD items in dead_items array, count tuples */
> > > > -                     if (lazy_scan_noprune(vacrel, buf, blkno, page, &hastup,
> > > > +                     /*
> > > > +                      * Collect LP_DEAD items in dead_items array, count tuples,
> > > > +                      * determine if rel truncation is safe
> > > > +                      */
> > > > +                     if (lazy_scan_noprune(vacrel, buf, blkno, page,
> > > >                                                                 &recordfreespace))
> > > >                       {
> > > >                               Size            freespace = 0;
> > > >
> > > >                               /*
> > > >                                * Processed page successfully (without cleanup lock) -- just
> > > > -                              * need to perform rel truncation and FSM steps, much like the
> > > > -                              * lazy_scan_prune case.  Don't bother trying to match its
> > > > -                              * visibility map setting steps, though.
> > > > +                              * need to update the FSM, much like the lazy_scan_prune case.
> > > > +                              * Don't bother trying to match its visibility map setting
> > > > +                              * steps, though.
> > > >                                */
> > > > -                             if (hastup)
> > > > -                                     vacrel->nonempty_pages = blkno + 1;
> > > >                               if (recordfreespace)
> > > >                                       freespace = PageGetHeapFreeSpace(page);
> > > >                               UnlockReleaseBuffer(buf);
> > >
> > > The comment continues to say that we "determine if rel truncation is safe" -
> > > but I don't see that?  Oh, I see, it's done inside lazy_scan_noprune(). This
> > > doesn't seem like a clear improvement to me. Particularly because it's only
> > > set if lazy_scan_noprune() actually does something.
> >
> > I don't get what the last sentence means ("Particularly because...").
>
> Took me a second to understand myself again too, oops. What I think I meant is
> that it seems error-prone that it's only set in some paths inside
> lazy_scan_noprune(). Previously it was at least a bit clearer in
> lazy_scan_heap() that it would be set for the different possible paths.

I see what you are saying. But if lazy_scan_noprune() doesn't do
anything, then it calls lazy_scan_prune(), which does set hastup and
update vacrel->nonempty_pages if needed.

Using hastup in lazy_scan_[no]prune() also means that they are
directly updating LVRelState after determining how to update it.
lazy_scan_heap() isn't doing responsible anymore. I don't see a reason
to be passing information back to lazy_scan_heap() to update
LVRelState when lazy_scan_[no]prune() has access to the LVRelState.

Importantly, once I combine the prune and freeze records, hastup is
set in heap_page_prune() instead of lazy_scan_prune() (that whole loop
in lazy_scan_prune() is eliminated()). And I don't like having to pass
hastup back through lazy_scan_prune() and then to lazy_scan_heap() so
that lazy_scan_heap() can use it (and use it to update a data
structure available in lazy_scan_prune()).

- Melanie