Re: Emit fewer vacuum records by reaping removable tuples during pruning

On Mon, Nov 13, 2023 at 5:59 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Mon, Nov 13, 2023 at 2:29 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > I think it also makes it clear that we should update the VM in
> > lazy_scan_prune(). All callers of lazy_scan_prune() will now consider
> > updating the VM after returning. And most of the state communicated back
> > to lazy_scan_heap() from lazy_scan_prune() is to inform it whether or
> > not to update the VM.
>
> That makes sense.
>
> > I didn't do that in this patch set because I would
> > need to pass all_visible_according_to_vm to lazy_scan_prune() and that
> > change didn't seem worth the improvement in code clarity in
> > lazy_scan_heap().
>
> Have you thought about finding a way to get rid of
> all_visible_according_to_vm? (Not necessarily in the scope of the
> ongoing work, just in general.)
>
> all_visible_according_to_vm is inherently prone to races -- it tells
> us what the VM used to say about the page, back when we looked. It is
> very likely that the page isn't visible in this sense, anyway, because
> VACUUM is after all choosing to scan the page in the first place when
> we end up in lazy_scan_prune. (Granted, this is much less true than it
> should be due to the influence of SKIP_PAGES_THRESHOLD, which
> implements a weird and inefficient form of prefetching/readahead.)
>
> Why should we necessarily care what all_visible_according_to_vm says
> or would say at this point? We're looking at the heap page itself,
> which is more authoritative than the VM (theoretically they're equally
> authoritative, but not really, not when push comes to shove). The best
> answer that I can think of is that all_visible_according_to_vm gives
> us a way of noticing and then logging any inconsistencies between VM
> and heap that we might end up "repairing" in passing (once we've
> rechecked the VM). But maybe that could happen elsewhere.
>
> Perhaps that cross-check could be pushed into visibilitymap.c, when we
> go to set a !PageIsAllVisible(page) page all-visible in the VM. If
> we're setting it and find that it's already set from earlier on, then
> remember and complaing/LOG it. No all_visible_according_to_vm
> required, plus this seems like it might be more thorough.

Setting aside the data corruption cases (the repairing you mentioned), I
think the primary case that all_visible_according_to_vm seeks to protect
us from is if the page was already marked all visible in the VM and
pruning did not change that. So, it avoids a call to visibilitymap_set()
when the page was known to already be set all visible (as long as we
didn't newly set all frozen).

As you say, this does not seem like the common case. Pages we vacuum
will most often not be all visible.

I actually wonder if it is worse to rely on that old value of all
visible and then call visibilitymap_set(), which takes an exclusive lock
on the VM page, than it would be to just call visibilitymap_get_status()
anew. This is what we do with all frozen.

The problem is that it is kind of hard to tell because the whole thing
is a bit of a tangled knot.

I've ripped this code apart and put it back together again six ways from
Sunday trying to get rid of all_visible_according_to_vm and
next_unskippable_allvis/skipping_current_range's bootleg prefetching
without incurring any additional calls to visibilitymap_get_status().

As best I can tell, our best case scenario is Thomas' streaming read API
goes in, we add vacuum as a user, and we can likely remove the skip
range logic.

Then, the question remains about how useful it is to save the visibility
map statuses we got when deciding whether or not to skip a block.

- Melanie

[1]
https://www.postgresql.org/message-id/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2gkvA%40mail.gmail.com