Hi,
On 2023-01-06 11:16:00 -0800, Peter Geoghegan wrote:
> On Tue, Jan 3, 2023 at 10:52 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > > And it'd make sense to have
> > > the explanation of why TransactionIdDidAbort() isn't the same as
> > > !TransactionIdDidCommit(), even for !TransactionIdIsInProgress() xacts, near
> > > the explanation for doing TransactionIdIsInProgress() first.
> >
> > I think that we should definitely have a comment directly over
> > TransactionIdDidAbort(). Though I wouldn't mind reorganizing these
> > other comments, or making the comment over TransactionIdDidAbort()
> > mostly just point to the other comments.
>
> 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).
> * Assumes transaction identifier is valid and exists in clog.
> + *
> + * Not all transactions that must be treated as aborted will be
> + * explicitly marked as such in clog. Transactions that were in
> + * progress during a crash are never reported as aborted by us.
> */
> bool /* true if given transaction aborted */
> TransactionIdDidAbort(TransactionId transactionId)
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().
Greetings,
Andres Freund