On Thu, Dec 29, 2022 at 12:20 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Thu, Dec 29, 2022 at 12:00 PM Andres Freund <andres@anarazel.de> wrote:
> > > I could just move the same tests from heap_prepare_freeze_tuple() to
> > > heap_freeze_execute_prepared(), without changing any of the details.
> >
> > That might work, yes.
>
> Attached patch shows how that could work.
My plan is to push something like this next week, barring objections.
Note that I've inverted the xmax "!TransactionIdDidCommit()" test --
it is now "TransactionIdDidAbort()" instead. I believe that this makes
the test more likely to catch problems, since we really should expect
relevant xmax XIDs to have aborted, specifically -- since the xmax
XIDs in question are always < OldestXmin. (There is a need to use a
"!TransactionIdDidCommit()" test specifically in nearby code in
FreezeMultiXactId(), because that code has to also deal with
in-progress XIDs that are multi members, but that's not the case
here.)
I'm also going to create a CF entry for the other patch posted to this
thread -- the enhancement to FreezeMultiXactId() that aims to get the
most out of any expensive calls to GetMultiXactIdMembers(). That
approach seems quite promising, and relatively simple. I'm not
particularly worried about wasting a call to GetMultiXactIdMembers()
during VACUUM, though. I'm more concerned about the actual impact of
not doing our best to outright remove Multis during VACUUM. The
application itself can experience big performance cliffs from SLRU
buffer misses, which VACUUM should do its utmost to prevent. That is
an occasional source of big problems [1].
I'm particularly concerned about the possibility that having an
updater XID will totally change the characteristics of how multis are
processed inside FreezeMultiXactId(). That seems like it might be a
really sharp edge. I believe that the page-level freezing patch has
already ameliorated the problem, since it made us much less reliant on
the case where GetMultiXactIdMembers() returns "nmembers <= 0" for a
Multi that happens to be HEAP_XMAX_IS_LOCKED_ONLY(). But we can and
should go further than that.
[1] https://buttondown.email/nelhage/archive/notes-on-some-postgresql-implementation-details/
--
Peter Geoghegan