On Mon, Jan 9, 2023 at 11:44 AM Andres Freund <and...@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
