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