Re: Eagerly scan all-visible pages to amortize aggressive vacuum

Attached v16 implements the logic to not count pages we failed to
freeze because of cleanup lock contention as eager freeze failures.

Re the code: I didn't put it in the same if statement block as
lazy_scan_prune() because I wanted to avoid another level of
indentation, but I am open to changing it if people hate it.

On Wed, Feb 5, 2025 at 11:47 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, Feb 4, 2025 at 5:34 PM Melanie Plageman
> <melanieplageman(at)gmail(dot)com> wrote:
> > I think I misspoke when I said we are unlikely to have contended
> > all-visible pages. I suppose it is trivial to concoct a scenario where
> > there are many pinned all-visible pages.
>
> It's hard to keep heap pages pinned for a really long time, so there
> aren't likely to be a lot of them at once. Maybe async I/O will change
> that somewhat, but the word in this sentence that I would emphasize is
> "concoct". I don't think it's normal for there to be lots of pinned
> pages just hanging around.
>
> (It's a bit easier to have index pages that stay pinned for a long
> time because an index scan can, or at least could last I checked, hold
> onto a pin while the cursor was open even if we're not currently
> executing the query. But for a heap page that doesn't happen, AFAIK.)

I started getting worried thinking about this. If you have a cursor
for select * from a table and fetch forward far enough, couldn't
vacuum fail to get cleanup locks on a whole range of blocks? If those
are all-visible and we don't count them as failures, we could end up
doing the overhead of lazy_scan_noprune() on all of those blocks. Even
though we wouldn't end up doing extra read I/O, I wonder if the extra
CPU overhead is going to be noticeable.

> > However, if we don't count an eagerly scanned page as a failure when
> > we don't get the cleanup lock because we won't have incurred a read,
> > then why would we count any eagerly scanned page in shared buffers as
> > a failure? In the case that we actually tried freezing the page and
> > failed because it was too new, that is giving us information about
> > whether or not we should keep trying to freeze. So, it is not just
> > about doing the read but also about what the failure indicates about
> > the data.
>
> That's a good point, but failure to get a tuple lock isn't a judgement
> either way on whether the XIDs in the page are old or new.

Yea, I guess that means the freeze limit caps wasted read I/O -- but
you could end up doing no read I/O and still hitting the freeze fail
limit because it is trying to detect when data is too new to be worth
eager scanning. But that's probably the behavior we want.

> > Interestingly, we call heap_tuple_should_freeze() in
> > lazy_scan_noprune(), so we actually could tell whether or not there
> > are some tuples on the page old enough to trigger freezing if we had
> > gotten the cleanup lock. One option would be to add a new output
> > parameter to lazy_scan_noprune() that indicates whether or not there
> > were tuples with xids older than the FreezeLimit, and only if there
> > were not, count it as a failed eager scan.
>
> Yeah, that could be done, and it doesn't sound like a bad idea.
> However, I'm also unsure whether we need to add additional logic for
> this case. My intuition is that it just won't happen very often. I'm
> not quite confident that I'm correct about that, but if I had to
> guess, my guess would be "rare scenario".

I've not done this in the attached v16. I have added a comment about it.
I think not doing it is a judgment call and not a bug, right?

- Melanie