Обсуждение: 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