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)