Re: Confine vacuum skip logic to lazy_scan_skip

On Fri, 12 Jan 2024 at 05:12, Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> v3 attached
>
> On Thu, Jan 4, 2024 at 3:23 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > On 2024-01-02 12:36:18 -0500, Melanie Plageman wrote:
> > > Subject: [PATCH v2 1/6] lazy_scan_skip remove unnecessary local var rel_pages
> > > Subject: [PATCH v2 2/6] lazy_scan_skip remove unneeded local var
> > >  nskippable_blocks
> >
> > I think these may lead to worse code - the compiler has to reload
> > vacrel->rel_pages/next_unskippable_block for every loop iteration, because it
> > can't guarantee that they're not changed within one of the external functions
> > called in the loop body.
>
> I buy that for 0001 but 0002 is still using local variables.
> nskippable_blocks was just another variable to keep track of even
> though we could already get that info from local variables
> next_unskippable_block and next_block.
>
> In light of this comment, I've refactored 0003/0004 (0002 and 0003 in
> this version [v3]) to use local variables in the loop as well. I had
> started using the members of the VacSkipState which I introduced.
>
> > > Subject: [PATCH v2 3/6] Add lazy_scan_skip unskippable state
> > >
> > > Future commits will remove all skipping logic from lazy_scan_heap() and
> > > confine it to lazy_scan_skip(). To make those commits more clear, first
> > > introduce the struct, VacSkipState, which will maintain the variables
> > > needed to skip ranges less than SKIP_PAGES_THRESHOLD.
> >
> > Why not add this to LVRelState, possibly as a struct embedded within it?
>
> Done in attached.
>
> > > From 335faad5948b2bec3b83c2db809bb9161d373dcb Mon Sep 17 00:00:00 2001
> > > From: Melanie Plageman <melanieplageman@gmail.com>
> > > Date: Sat, 30 Dec 2023 16:59:27 -0500
> > > Subject: [PATCH v2 4/6] Confine vacuum skip logic to lazy_scan_skip
> >
> > > By always calling lazy_scan_skip() -- instead of only when we have reached the
> > > next unskippable block, we no longer need the skipping_current_range variable.
> > > lazy_scan_heap() no longer needs to manage the skipped range -- checking if we
> > > reached the end in order to then call lazy_scan_skip(). And lazy_scan_skip()
> > > can derive the visibility status of a block from whether or not we are in a
> > > skippable range -- that is, whether or not the next_block is equal to the next
> > > unskippable block.
> >
> > I wonder if it should be renamed as part of this - the name is somewhat
> > confusing now (and perhaps before)? lazy_scan_get_next_block() or such?
>
> Why stop there! I've removed lazy and called it
> heap_vac_scan_get_next_block() -- a little long, but...
>
> > > +     while (true)
> > >       {
> > >               Buffer          buf;
> > >               Page            page;
> > > -             bool            all_visible_according_to_vm;
> > >               LVPagePruneState prunestate;
> > >
> > > -             if (blkno == vacskip.next_unskippable_block)
> > > -             {
> > > -                     /*
> > > -                      * Can't skip this page safely.  Must scan the page.  But
> > > -                      * determine the next skippable range after the page first.
> > > -                      */
> > > -                     all_visible_according_to_vm = vacskip.next_unskippable_allvis;
> > > -                     lazy_scan_skip(vacrel, &vacskip, blkno + 1);
> > > -
> > > -                     Assert(vacskip.next_unskippable_block >= blkno + 1);
> > > -             }
> > > -             else
> > > -             {
> > > -                     /* Last page always scanned (may need to set nonempty_pages) */
> > > -                     Assert(blkno < rel_pages - 1);
> > > -
> > > -                     if (vacskip.skipping_current_range)
> > > -                             continue;
> > > +             blkno = lazy_scan_skip(vacrel, &vacskip, blkno + 1,
> > > +                                                        &all_visible_according_to_vm);
> > >
> > > -                     /* Current range is too small to skip -- just scan the page */
> > > -                     all_visible_according_to_vm = true;
> > > -             }
> > > +             if (blkno == InvalidBlockNumber)
> > > +                     break;
> > >
> > >               vacrel->scanned_pages++;
> > >
> >
> > I don't like that we still do determination about the next block outside of
> > lazy_scan_skip() and have duplicated exit conditions between lazy_scan_skip()
> > and lazy_scan_heap().
> >
> > I'd probably change the interface to something like
> >
> > while (lazy_scan_get_next_block(vacrel, &blkno))
> > {
> > ...
> > }
>
> I've done this. I do now find the parameter names a bit confusing.
> There is next_block (which is the "next block in line" and is an input
> parameter) and blkno, which is an output parameter with the next block
> that should actually be processed. Maybe it's okay?
>
> > > From b6603e35147c4bbe3337280222e6243524b0110e Mon Sep 17 00:00:00 2001
> > > From: Melanie Plageman <melanieplageman@gmail.com>
> > > Date: Sun, 31 Dec 2023 09:47:18 -0500
> > > Subject: [PATCH v2 5/6] VacSkipState saves reference to LVRelState
> > >
> > > The streaming read interface can only give pgsr_next callbacks access to
> > > two pieces of private data. As such, move a reference to the LVRelState
> > > into the VacSkipState.
> > >
> > > This is a separate commit (as opposed to as part of the commit
> > > introducing VacSkipState) because it is required for using the streaming
> > > read interface but not a natural change on its own. VacSkipState is per
> > > block and the LVRelState is referenced for the whole relation vacuum.
> >
> > I'd do it the other way round, i.e. either embed VacSkipState ino LVRelState
> > or point to it from VacSkipState.
> >
> > LVRelState is already tied to the iteration state, so I don't think there's a
> > reason not to do so.
>
> Done, and, as such, this patch is dropped from the set.

CFBot shows that the patch does not apply anymore as in [1]:
=== applying patch
./v3-0002-Add-lazy_scan_skip-unskippable-state-to-LVRelStat.patch
patching file src/backend/access/heap/vacuumlazy.c
...
Hunk #10 FAILED at 1042.
Hunk #11 FAILED at 1121.
Hunk #12 FAILED at 1132.
Hunk #13 FAILED at 1161.
Hunk #14 FAILED at 1172.
Hunk #15 FAILED at 1194.
...
6 out of 21 hunks FAILED -- saving rejects to file
src/backend/access/heap/vacuumlazy.c.rej

Please post an updated version for the same.

[1] - http://cfbot.cputube.org/patch_46_4755.log

Regards,
Vignesh