Re: pgsql: Delay commit status checks until freezing executes.

Hi,

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