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. > > I find this astonishing. Why isn't there a prominent comment that > > advertises that TransactionIdDidAbort() just doesn't work reliably? > > Arguably it works reliably, just more narrowly than one might think. Treating > "crashed transactions" as a distinct state from explicit aborts. That's quite a stretch. There are numerous comments that pretty much imply that TransactionIdDidCommit/TransactionIdDidAbort are very similar, for example any discussion of how you need to call TransactionIdIsInProgress first before calling either of the other two. -- Peter Geoghegan
