Re: Confine vacuum skip logic to lazy_scan_skip

On 11/03/2024 18:15, Melanie Plageman wrote:
> On Mon, Mar 11, 2024 at 11:29:44AM +0200, Heikki Linnakangas wrote:
>> 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
>> + * sets blkno to the next block that actually needs to be processed.
> 
> 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.

Ok yeah, there's a lot of "needs" here :-). Fixed.

>>    *
>> - * 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.

Ok, rephrased.

>> + * 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().

Moved.

>> @@ -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?

Yeah, perhaps. But I think it makes sense to treat it as a special case.

>> +    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.

Reordered them in the statement.

It feels a bit wrong to test next_block > vacrel->next_unskippable_block 
before vacrel->next_unskippable_block == InvalidBlockNumber. But it 
works, and that order makes more sense in the comment IMHO.

>> +        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.

Added comments.

> Otherwise LGTM

Ok, pushed! Thank you, this is much more understandable now!

-- 
Heikki Linnakangas
Neon (https://neon.tech)