Re: Avoiding unnecessary clog lookups while freezing

On Wed, Dec 28, 2022 at 4:20 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > Theoretically this is an old issue that dates back to commit
> > 699bf7d05c, as opposed to an issue in the page-level freezing patch.
> > But that's not really true in a real practical sense. In practice the
> > calls to TransactionIdDidCommit() will happen far more frequently
> > following today's commit 1de58df4fe (since we're using OldestXmin as
> > the cutoff that gates performing freeze_xmin/freeze_xmax processing --
> > not FreezeLimit).
>
> Hm. But we still only do the check when we actually freeze, rather than just
> during the pre-check in heap_tuple_should_freeze(). So we'll only incur the
> increased overhead when we also do more WAL logging etc. Correct?

Yes, that's how it worked up until today's commit 1de58df4fe.

I don't have strong feelings about back patching a fix, but this does
seem like something that I should fix now, on HEAD.

> Hm. I dimply recall that we had repeated cases where the hint bits were set
> wrongly due to some of the multixact related bugs. I think I was trying to be
> paranoid about not freezing stuff in those situations, since it can lead to
> reviving dead tuples, which obviously is bad.

I think that it's a reasonable check, and I'm totally in favor of
keeping it (or something very close, at least).

> There's practically no tuple-level concurrent activity in a normal regression
> test. So I'm a bit doubtful that is an interesting metric. It'd be a tad more
> interesting to run tests, including isolationtester and pgbench, against a
> running cluster.

Even on HEAD, with page-level freezing in place, we're only going to
test XIDs that are < OldestXmin, that appear on pages tha VACUUM
actually scans in the first place. Just checking tuple-level hint bits
should be effective. But even if it isn't (for whatever reason) then
it's similar to cases where our second heap pass has to do clog
lookups in heap_page_is_all_visible() just because hint bits couldn't
be set earlier on, back when lazy_scan_prune() processed the same page
during VACUUM's initial heap pass.

--
Peter Geoghegan