On Fri, Mar 8, 2024 at 11:31 AM Melanie Plageman <melanieplage...@gmail.com> wrote: > > On Fri, Mar 8, 2024 at 11:00 AM Peter Geoghegan <p...@bowt.ie> wrote: > > > > On Fri, Mar 8, 2024 at 10:48 AM Melanie Plageman > > <melanieplage...@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
