Обсуждение: VACUUM can set pages all-frozen without also setting them all-visible

Attached are two patches. The first patch adds assertions that verify
that calls to visibilitymap_set() don't set VISIBILITYMAP_ALL_FROZEN
for a page without also setting VISIBILITYMAP_ALL_VISIBLE (plus a
similar assertion for visibilitymap_clear()). When I run "make
check-world" with just the first patch, several tests fail because one
of the new assertions fails.

The second patch tentatively addresses the issue by making two of the
calls to visibilitymap_set() made by vacuumlazy.c set
VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN -- as opposed to
just setting VISIBILITYMAP_ALL_FROZEN, which is what happens on HEAD.

It's not clear whether or not it's strictly correct to only set
VISIBILITYMAP_ALL_FROZEN, but it seems questionable. It's also likely
to have negative performance implications. Non-aggressive VACUUMs
won't actually use VISIBILITYMAP_ALL_FROZEN to skip -- they could in
principle, but they don't.

VACUUM's skipping logic doesn't just refuse to skip
VISIBILITYMAP_ALL_VISIBLE pages during an aggressive VACUUM (which is
necessary and makes sense); it also refuses to skip
VISIBILITYMAP_ALL_FROZEN pages during non-aggressive VACUUMs. And so
any page that just has its VISIBILITYMAP_ALL_FROZEN bit set is not
skippable by non-aggressive VACUUMs. I'm referring to this code:

                while (next_unskippable_block < rel_pages)
                {
                    uint8       vmskipflags;

                    vmskipflags = visibilitymap_get_status(vacrel->rel,

next_unskippable_block,
                                                           &vmbuffer);
                    if (vacrel->aggressive)
                    {
                        if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0)
                            break;
                    }
                    else
                    {
                        if ((vmskipflags & VISIBILITYMAP_ALL_VISIBLE) == 0)
                            break;
                    }
                    vacuum_delay_point();
                    next_unskippable_block++;
                }

-- 
Peter Geoghegan

On Wed, Mar 16, 2022 at 12:24 AM Peter Geoghegan <pg@bowt.ie> wrote:
> It's not clear whether or not it's strictly correct to only set
> VISIBILITYMAP_ALL_FROZEN, but it seems questionable. It's also likely
> to have negative performance implications. Non-aggressive VACUUMs
> won't actually use VISIBILITYMAP_ALL_FROZEN to skip -- they could in
> principle, but they don't.

I believe that at least some of the assertion failures are related to
heap_lock_tuple()'s tendency to clear VISIBILITYMAP_ALL_FROZEN without
also clearing VISIBILITYMAP_ALL_VISIBLE (in its cleared_all_frozen
path).

It's possible for an aggressive VACUUM to see an all-frozen page as
all_visible_according_to_vm that's considered skippable (it's
skippable in principle), that isn't actually skipped due to VACUUM not
crossing SKIP_PAGES_THRESHOLD. The variable
all_visible_according_to_vm from lazy_scan_heap() is inherently
race-prone, which vacuumlazy.c accounts for -- mostly. But I don't
think it's quite safe to assume that the page hasn't changed
(invalidating all_visible_according_to_vm in the process) when an
all_visible_according_to_vm page is still at least all-visible
according to lazy_scan_prune() -- a concurrent heap_lock_tuple() could
make that not work out.

Is it really worth the trouble it takes to make the
visibilitymap_set() interface support "|= VISIBILITYMAP_ALL_FROZEN",
rather than requiring the caller to clobber both of the page's bits?
It seems like it'd be a good idea to just not support that -- that
would make it easy to enforce a rule that says that
VISIBILITYMAP_ALL_FROZEN can never be set on its own.

-- 
Peter Geoghegan