On Mon, Jan 2, 2023 at 6:26 PM Jeff Davis <pgsql@j-davis.com> wrote:
> On Mon, 2023-01-02 at 11:45 -0800, Peter Geoghegan wrote:
> > What do you think of the wording adjustments in the attached patch?
> > It's based on your suggested wording.
>
> Great, thank you.
Pushed that today.
Attached is v14.
v14 simplifies the handling of setting the visibility map at the end
of the blkno-wise loop in lazy_scan_heap(). And,
visibilitymap_snap_next() doesn't tell its caller (lazy_scan_heap)
anything about the visibility status of each returned block -- we no
longer need a all_visible_according_to_vm local variable to help with
setting the visibility map.
This new approach to setting the VM is related to hardening that I
plan on adding, which makes the visibility map robust against certain
race conditions that can lead to setting a page all-frozen but not
all-visible. I go into that here:
https://postgr.es/m/CAH2-WznuNGSzF8v6OsgjaC5aYsb3cZ6HW6MLm30X0d65cmSH6A@mail.gmail.com
(It's the second patch -- the first patch already became yesterday's
commit 6daeeb1f.)
In general I don't think that we should be using
all_visible_according_to_vm for anything, especially not anything
critical -- it is just information about how the page used to be in
the past, after all. This will be more of a problem with visibility
map snapshots, since all_visible_according_to_vm could be information
that is hours old by the time it's actually used by lazy_scan_heap().
But it is an existing issue.
BTW, it would be helpful if I could get a +1 to the visibility map
patch posted on that other thread. It's practically a bug fix -- the
VM shouldn't be able to show contradictory information about any given
heap page (i.e. "page is all-frozen but not all-visible"), no matter
what. Just on general principle.
--
Peter Geoghegan
