Re: Confine vacuum skip logic to lazy_scan_skip

On Fri, Mar 8, 2024 at 11:00 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Fri, Mar 8, 2024 at 10:48 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > Not that it will be fun to maintain another special case in the VM
> > update code in lazy_scan_prune(), but we could have a special case
> > that checks if DISABLE_PAGE_SKIPPING was passed to vacuum and if
> > all_visible_according_to_vm is true and all_visible is true, we update
> > the VM but don't dirty the page.
>
> It wouldn't necessarily have to be a special case, I think.
>
> We already conditionally set PD_ALL_VISIBLE/call PageIsAllVisible() in
> the block where lazy_scan_prune marks a previously all-visible page
> all-frozen -- we don't want to dirty the page unnecessarily there.
> Making it conditional is defensive in that particular block (this was
> also added by this same commit of mine), and avoids dirtying the page.

Ah, I see. I got confused. Even if the VM is suspect, if the page is
all visible and the heap block is already set all-visible in the VM,
there is no need to update it.

This did make me realize that it seems like there is a case we don't
handle in master with the current code that would be fixed by changing
that code Heikki mentioned:

Right now, even if the heap block is incorrectly marked all-visible in
the VM, if DISABLE_PAGE_SKIPPING is passed to vacuum,
all_visible_according_to_vm will be passed to lazy_scan_prune() as
false. Then even if lazy_scan_prune() finds that the page is not
all-visible, we won't call visibilitymap_clear().

If we revert the code setting next_unskippable_allvis to false in
lazy_scan_skip() when vacrel->skipwithvm is false and allow
all_visible_according_to_vm to be true when the VM has it incorrectly
set to true, then once lazy_scan_prune() discovers the page is not
all-visible and assuming PD_ALL_VISIBLE is not marked so
PageIsAllVisible() returns false, we will call visibilitymap_clear()
to clear the incorrectly set VM bit (without dirtying the page).

Here is a table of the variable states at the end of lazy_scan_prune()
for clarity:

master:
all_visible_according_to_vm: false
all_visible:                 false
VM says all vis:             true
PageIsAllVisible:            false

if fixed:
all_visible_according_to_vm: true
all_visible:                 false
VM says all vis:             true
PageIsAllVisible:            false

> Seems like it might be possible to simplify/consolidate the VM-setting
> code that's now located at the end of lazy_scan_prune. Perhaps the two
> distinct blocks that call visibilitymap_set() could be combined into
> one.

I agree. I have some code to do that in an unproposed patch which
combines the VM updates into the prune record. We will definitely want
to reorganize the code when we do that record combining.

- Melanie