Re: Confine vacuum skip logic to lazy_scan_skip

On Fri, Mar 8, 2024 at 11:31 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> 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

Okay, I now see from Heikki's v8-0001 that he was already aware of this.

- Melanie