Re: Confine vacuum skip logic to lazy_scan_skip

On Mon, Mar 11, 2024 at 11:29:44AM +0200, Heikki Linnakangas wrote:
>
> > I see why you removed my treatise-level comment that was here about
> > unskipped skippable blocks. However, when I was trying to understand
> > this code, I did wish there was some comment that explained to me why we
> > needed all of the variables next_unskippable_block,
> > next_unskippable_allvis, all_visible_according_to_vm, and current_block.
> > 
> > The idea that we would choose not to skip a skippable block because of
> > kernel readahead makes sense. The part that I had trouble wrapping my
> > head around was that we want to also keep the visibility status of both
> > the beginning and ending blocks of the skippable range and then use
> > those to infer the visibility status of the intervening blocks without
> > another VM lookup if we decide not to skip them.
> 
> Right, I removed the comment because looked a little out of place and it
> duplicated the other comments sprinkled in the function. I agree this could
> still use some more comments though.
> 
> Here's yet another attempt at making this more readable. I moved the logic
> to find the next unskippable block to a separate function, and added
> comments to make the states more explicit. What do you think?

Oh, I like the new structure. Very cool! Just a few remarks:

> From c21480e9da61e145573de3b502551dde1b8fa3f6 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
> Date: Fri, 8 Mar 2024 17:32:19 +0200
> Subject: [PATCH v9 1/2] Confine vacuum skip logic to lazy_scan_skip()
> 
> Rename lazy_scan_skip() to heap_vac_scan_next_block() and move more
> code into the function, so that the caller doesn't need to know about
> ranges or skipping anymore. heap_vac_scan_next_block() returns the
> next block to process, and the logic for determining that block is all
> within the function. This makes the skipping logic easier to
> understand, as it's all in the same function, and makes the calling
> code easier to understand as it's less cluttered. The state variables
> needed to manage the skipping logic are moved to LVRelState.
> 
> heap_vac_scan_next_block() now manages its own VM buffer separately
> from the caller's vmbuffer variable. The caller's vmbuffer holds the
> VM page for the current block its processing, while
> heap_vac_scan_next_block() keeps a pin on the VM page for the next
> unskippable block. Most of the time they are the same, so we hold two
> pins on the same buffer, but it's more convenient to manage them
> separately.
> 
> For readability inside heap_vac_scan_next_block(), move the logic of
> finding the next unskippable block to separate function, and add some
> comments.
> 
> This refactoring will also help future patches to switch to using a
> streaming read interface, and eventually AIO
> (https://postgr.es/m/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2gkvA%40mail.gmail.com)
> 
> Author: Melanie Plageman, with some changes by me

I'd argue you earned co-authorship by now :)

> Discussion: https://postgr.es/m/CAAKRu_Yf3gvXGcCnqqfoq0Q8LX8UM-e-qbm_B1LeZh60f8WhWA%40mail.gmail.com
> ---
>  src/backend/access/heap/vacuumlazy.c | 233 +++++++++++++++++----------
>  1 file changed, 146 insertions(+), 87 deletions(-)
> 
> diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
> index ac55ebd2ae..1757eb49b7 100644
> --- a/src/backend/access/heap/vacuumlazy.c
> +++ b/src/backend/access/heap/vacuumlazy.c
> +

>  /*
> - *    lazy_scan_skip() -- set up range of skippable blocks using visibility map.
> + *    heap_vac_scan_next_block() -- get next block for vacuum to process
>   *
> - * lazy_scan_heap() calls here every time it needs to set up a new range of
> - * blocks to skip via the visibility map.  Caller passes the next block in
> - * line.  We return a next_unskippable_block for this range.  When there are
> - * no skippable blocks we just return caller's next_block.  The all-visible
> - * status of the returned block is set in *next_unskippable_allvis for caller,
> - * too.  Block usually won't be all-visible (since it's unskippable), but it
> - * can be during aggressive VACUUMs (as well as in certain edge cases).
> + * lazy_scan_heap() calls here every time it needs to get the next block to
> + * prune and vacuum.  The function uses the visibility map, vacuum options,
> + * and various thresholds to skip blocks which do not need to be processed and

I wonder if "need" is too strong a word since this function
(heap_vac_scan_next_block()) specifically can set blkno to a block which
doesn't *need* to be processed but which it chooses to process because
of SKIP_PAGES_THRESHOLD.

> + * sets blkno to the next block that actually needs to be processed.
>   *
> - * Sets *skipping_current_range to indicate if caller should skip this range.
> - * Costs and benefits drive our decision.  Very small ranges won't be skipped.
> + * The block number and visibility status of the next block to process are set
> + * in *blkno and *all_visible_according_to_vm.  The return value is false if
> + * there are no further blocks to process.
> + *
> + * vacrel is an in/out parameter here; vacuum options and information about
> + * the relation are read, and vacrel->skippedallvis is set to ensure we don't
> + * advance relfrozenxid when we have skipped vacuuming all-visible blocks.  It

Maybe this should say when we have skipped vacuuming all-visible blocks
which are not all-frozen or just blocks which are not all-frozen.

> + * also holds information about the next unskippable block, as bookkeeping for
> + * this function.
>   *
>   * Note: our opinion of which blocks can be skipped can go stale immediately.
>   * It's okay if caller "misses" a page whose all-visible or all-frozen marking

Wonder if it makes sense to move this note to
find_next_nunskippable_block().

> @@ -1098,26 +1081,119 @@ lazy_scan_heap(LVRelState *vacrel)
>   * older XIDs/MXIDs.  The vacrel->skippedallvis flag will be set here when the
>   * choice to skip such a range is actually made, making everything safe.)
>   */
> -static BlockNumber
> -lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block,
> -               bool *next_unskippable_allvis, bool *skipping_current_range)
> +static bool
> +heap_vac_scan_next_block(LVRelState *vacrel, BlockNumber *blkno,
> +                         bool *all_visible_according_to_vm)
>  {
> -    BlockNumber rel_pages = vacrel->rel_pages,
> -                next_unskippable_block = next_block,
> -                nskippable_blocks = 0;
> -    bool        skipsallvis = false;
> +    BlockNumber next_block;
>  
> -    *next_unskippable_allvis = true;
> -    while (next_unskippable_block < rel_pages)
> +    /* relies on InvalidBlockNumber + 1 overflowing to 0 on first call */
> +    next_block = vacrel->current_block + 1;
> +
> +    /* Have we reached the end of the relation? */

No strong opinion on this, but I wonder if being at the end of the
relation counts as a fourth state?

> +    if (next_block >= vacrel->rel_pages)
> +    {
> +        if (BufferIsValid(vacrel->next_unskippable_vmbuffer))
> +        {
> +            ReleaseBuffer(vacrel->next_unskippable_vmbuffer);
> +            vacrel->next_unskippable_vmbuffer = InvalidBuffer;
> +        }
> +        *blkno = vacrel->rel_pages;
> +        return false;
> +    }
> +
> +    /*
> +     * We must be in one of the three following states:
> +     */
> +    if (vacrel->next_unskippable_block == InvalidBlockNumber ||
> +        next_block > vacrel->next_unskippable_block)
> +    {
> +        /*
> +         * 1. We have just processed an unskippable block (or we're at the
> +         * beginning of the scan).  Find the next unskippable block using the
> +         * visibility map.
> +         */

I would reorder the options in the comment or in the if statement since
they seem to be in the reverse order.

> +        bool        skipsallvis;
> +
> +        find_next_unskippable_block(vacrel, &skipsallvis);
> +
> +        /*
> +         * We now know the next block that we must process.  It can be the
> +         * next block after the one we just processed, or something further
> +         * ahead.  If it's further ahead, we can jump to it, but we choose to
> +         * do so only if we can skip at least SKIP_PAGES_THRESHOLD consecutive
> +         * pages.  Since we're reading sequentially, the OS should be doing
> +         * readahead for us, so there's no gain in skipping a page now and
> +         * then.  Skipping such a range might even discourage sequential
> +         * detection.
> +         *
> +         * This test also enables more frequent relfrozenxid advancement
> +         * during non-aggressive VACUUMs.  If the range has any all-visible
> +         * pages then skipping makes updating relfrozenxid unsafe, which is a
> +         * real downside.
> +         */
> +        if (vacrel->next_unskippable_block - next_block >= SKIP_PAGES_THRESHOLD)
> +        {
> +            next_block = vacrel->next_unskippable_block;
> +            if (skipsallvis)
> +                vacrel->skippedallvis = true;
> +        }

> +
> +/*
> + * Find the next unskippable block in a vacuum scan using the visibility map.

To expand this comment, I might mention it is a helper function for
heap_vac_scan_next_block(). I would also say that the next unskippable
block and its visibility information are recorded in vacrel. And that
skipsallvis is set to true if any of the intervening skipped blocks are
not all-frozen.

> + */
> +static void
> +find_next_unskippable_block(LVRelState *vacrel, bool *skipsallvis)
> +{
> +    BlockNumber rel_pages = vacrel->rel_pages;
> +    BlockNumber next_unskippable_block = vacrel->next_unskippable_block + 1;
> +    Buffer        next_unskippable_vmbuffer = vacrel->next_unskippable_vmbuffer;
> +    bool        next_unskippable_allvis;
> +
> +    *skipsallvis = false;
> +
> +    for (;;)
>      {
>          uint8        mapbits = visibilitymap_get_status(vacrel->rel,
>                                                         next_unskippable_block,
> -                                                       vmbuffer);
> +                                                       &next_unskippable_vmbuffer);
>  
> -        if ((mapbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
> +        next_unskippable_allvis = (mapbits & VISIBILITYMAP_ALL_VISIBLE) != 0;

Otherwise LGTM

- Melanie