Re: Fixing a couple of buglets in how VACUUM sets visibility map bits

On Mon, Jan 9, 2023 at 11:44 AM Andres Freund <andres@anarazel.de> wrote:
> I think that's just an imprecise formulation though - the problem is that we
> can call visibilitymap_set() with just VISIBILITYMAP_ALL_FROZEN, even though
> VISIBILITYMAP_ALL_VISIBLE was concurrently unset.

That's correct.

You're right that my description of the problem from the commit
message was confusing. But we're on the same page about the problem
now.

> ISTM that we ought to update all_visible_according_to_vm from
> PageIsAllVisible() once we've locked the page. Even if we avoid this specific
> case, it seems a recipe for future bugs to have a potentially outdated
> variable around.

I basically agree, but some of the details are tricky.

As I mentioned already, my work on visibility map snapshots just gets
rid of all_visible_according_to_vm, which is my preferred long term
approach. We will very likely need to keep all_visible_according_to_vm
as a cache for performance reasons for as long as we have
SKIP_PAGES_THRESHOLD.

Can we just update all_visible_according_to_vm using
PageIsAllVisible(), without making all_visible_according_to_vm
significantly less useful as a cache? Maybe. Not sure offhand.

-- 
Peter Geoghegan