Hi,
On 2023-01-03 22:41:35 -0800, Peter Geoghegan wrote:
> 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.
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