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)