Re: Confine vacuum skip logic to lazy_scan_skip

On Fri, Mar 8, 2024 at 8:49 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> 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.

I agree that having next_unskippable_allvis and, as a consequence,
all_visible_according_to_vm set to true for the last block seems
wrong. And It makes sense from a loop efficiency standpoint also to
move it up to the top. However, making that change would have us end
up dirtying all pages in your example.

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

This does seem undesirable.

However, if we do as you suggest above and don't check
DISABLE_PAGE_SKIPPING in the loop and instead return without checking
the VM when DISABLE_PAGE_SKIPPING is passed, setting
next_unskippable_allvis = false, we will end up dirtying all pages as
in your example. It would fix the last block issue but it would result
in dirtying all pages in your example.

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

If we revert this, then the when all_visible_according_to_vm and
all_visible are true in lazy_scan_prune(), the VM will only get
updated when all_frozen is true and the VM doesn't have all frozen set
yet, so maybe that is inconsistent with the goal of
DISABLE_PAGE_SKIPPING to update the VM when its contents are "suspect"
(according to docs).

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

I did some experiments on this in the past and thought that it did
have a perf impact to call visibilitymap_get_status() every time. But
let me try and dig those up. (doesn't speak to whether or not in
matters in practice)

- Melanie