Hi, On 2023-01-06 11:16:00 -0800, Peter Geoghegan wrote: > On Tue, Jan 3, 2023 at 10:52 PM Peter Geoghegan <p...@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
