Re: pgsql: Delay commit status checks until freezing executes.

On Sat, Jan 7, 2023 at 1:47 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > What do you think of the attached patch, which revises comments over
> > TransactionIdDidAbort, and adds something about it to the top of
> > heapam_visbility.c?
>
> Mostly looks good to me. I think it'd be good to add a reference to the
> heapam_visbility.c? comment to the top of transam.c (or move it).

Makes sense.

> I think it's currently very likely to be true, but I'd weaken the "never" a
> bit nonetheless. I think it'd also be good to point to what to do instead. How
> about:
> Note that TransactionIdDidAbort() returns true only for explicitly aborted
> transactions, as transactions implicitly aborted due to a crash will
> commonly still appear to be in-progress in the clog. Most of the time
> TransactionIdDidCommit(), with a preceding TransactionIdIsInProgress()
> check, should be used instead of TransactionIdDidAbort().

That does seem better.

Do we need to do anything about this to the "pg_xact and pg_subtrans"
section of the transam README? Also, does amcheck's get_xid_status()
need a reference to these rules?

FWIW, I found an existing comment about this rule in the call to
TransactionIdAbortTree() from RecordTransactionAbort() -- which took
me quite a while to find. So you might have been remembering that
comment before.

--
Peter Geoghegan