Re: Confine vacuum skip logic to lazy_scan_skip

On 08/03/2024 02:46, Melanie Plageman wrote:
> On Wed, Mar 06, 2024 at 10:00:23PM -0500, Melanie Plageman wrote:
>>> I feel heap_vac_scan_get_next_block() function could use some love. Maybe
>>> just some rewording of the comments, or maybe some other refactoring; not
>>> sure. But I'm pretty happy with the function signature and how it's called.
> 
> I've cleaned up the comments on heap_vac_scan_next_block() in the first
> couple patches (not so much in the streaming read user). Let me know if
> it addresses your feelings or if I should look for other things I could
> change.

Thanks, that is better. I think I now finally understand how the 
function works, and now I can see some more issues and refactoring 
opportunities :-).

Looking at current lazy_scan_skip() code in 'master', one thing now 
caught my eye (and it's the same with your patches):

>     *next_unskippable_allvis = true;
>     while (next_unskippable_block < rel_pages)
>     {
>         uint8        mapbits = visibilitymap_get_status(vacrel->rel,
>                                                        next_unskippable_block,
>                                                        vmbuffer);
> 
>         if ((mapbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
>         {
>             Assert((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0);
>             *next_unskippable_allvis = false;
>             break;
>         }
> 
>         /*
>          * Caller must scan the last page to determine whether it has tuples
>          * (caller must have the opportunity to set vacrel->nonempty_pages).
>          * This rule avoids having lazy_truncate_heap() take access-exclusive
>          * lock on rel to attempt a truncation that fails anyway, just because
>          * there are tuples on the last page (it is likely that there will be
>          * tuples on other nearby pages as well, but those can be skipped).
>          *
>          * Implement this by always treating the last block as unsafe to skip.
>          */
>         if (next_unskippable_block == rel_pages - 1)
>             break;
> 
>         /* DISABLE_PAGE_SKIPPING makes all skipping unsafe */
>         if (!vacrel->skipwithvm)
>         {
>             /* Caller shouldn't rely on all_visible_according_to_vm */
>             *next_unskippable_allvis = false;
>             break;
>         }
> 
>         /*
>          * Aggressive VACUUM caller can't skip pages just because they are
>          * all-visible.  They may still skip all-frozen pages, which can't
>          * contain XIDs < OldestXmin (XIDs that aren't already frozen by now).
>          */
>         if ((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0)
>         {
>             if (vacrel->aggressive)
>                 break;
> 
>             /*
>              * All-visible block is safe to skip in non-aggressive case.  But
>              * remember that the final range contains such a block for later.
>              */
>             skipsallvis = true;
>         }
> 
>         /* XXX: is it OK to remove this? */
>         vacuum_delay_point();
>         next_unskippable_block++;
>         nskippable_blocks++;
>     }

Firstly, it seems silly to check DISABLE_PAGE_SKIPPING within the loop. 
When DISABLE_PAGE_SKIPPING is set, we always return the next block and 
set *next_unskippable_allvis = false regardless of the visibility map, 
so why bother checking the visibility map at all?

Except at the very last block of the relation! If you look carefully, 
at the last block we do return *next_unskippable_allvis = true, if the 
VM says so, even if DISABLE_PAGE_SKIPPING is set. I think that's wrong. 
Surely the intention was to pretend that none of the VM bits were set if 
DISABLE_PAGE_SKIPPING is used, also for the last block.

This was changed in commit 980ae17310:

> @@ -1311,7 +1327,11 @@ lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block,
>  
>                 /* DISABLE_PAGE_SKIPPING makes all skipping unsafe */
>                 if (!vacrel->skipwithvm)
> +               {
> +                       /* Caller shouldn't rely on all_visible_according_to_vm */
> +                       *next_unskippable_allvis = false;
>                         break;
> +               }

Before that, *next_unskippable_allvis was set correctly according to the 
VM, even when DISABLE_PAGE_SKIPPING was used. It's not clear to me why 
that was changed. And I think setting it to 'true' would be a more 
failsafe value than 'false'. When *next_unskippable_allvis is set to 
true, the caller cannot rely on it because a concurrent modification 
could immediately clear the VM bit. But because VACUUM is the only 
process that sets VM bits, if it's set to false, the caller can assume 
that it's still not set later on.

One consequence of that is that with DISABLE_PAGE_SKIPPING, 
lazy_scan_heap() dirties all pages, even if there are no changes. The 
attached test script demonstrates that.

ISTM we should revert the above hunk, and backpatch it to v16. I'm a 
little wary because I don't understand why that change was made in the 
first place, though. I think it was just an ill-advised attempt at 
tidying up the code as part of the larger commit, but I'm not sure. 
Peter, do you remember?

I wonder if we should give up trying to set all_visible_according_to_vm 
correctly when we decide what to skip, and always do 
"all_visible_according_to_vm = visibilitymap_get_status(...)" in 
lazy_scan_prune(). It would be more expensive, but maybe it doesn't 
matter in practice. It would get rid of this tricky bookkeeping in 
heap_vac_scan_next_block().

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