On Tue, Jan 3, 2023 at 10:33 PM Andres Freund <andres@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