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

On Sat, Jan 7, 2023 at 1:47 PM Andres Freund <andres@anarazel.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