Hi, On 2023-01-03 22:41:35 -0800, Peter Geoghegan wrote: > On Tue, Jan 3, 2023 at 10:33 PM Andres Freund <and...@anarazel.de> wrote: > > I'd say a comment above TransactionIdDidAbort() referencing an overview > > comment at the top of the file? I think it might be worth moving the comment > > from heapam_visibility.c to transam.c? > > What comments in heapam_visibility.c should we be referencing here? I > don't see anything about it there. I have long been aware that those > routines deduce that a transaction must have aborted, but surely > that's not nearly enough. That's merely not being broken, without any > explanation given as to why.
IMO the comment at the top mentioning why the TransactionIdIsInProgress() calls are crucial / need to be done first would be considerably more likely to be found in transam.c than heapam_visibility.c. 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. Greetings, Andres Freund
