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

Hi,

On 2023-01-03 17:54:37 -0800, Peter Geoghegan wrote:
> (Pruning -committers from the list, since cross-posting to -hackers
> resulted in this being held up for moderation.)

I still think these moderation rules are deeply unhelpful...

> On Tue, Jan 3, 2023 at 5:15 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> >
> > On Tue, Jan 3, 2023 at 4:54 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > There's some changes from TransactionIdDidCommit() to !TransactionIdDidAbort()
> > > that don't look right to me. If the server crashed while xid X was
> > > in-progress, TransactionIdDidCommit(X) will return false, but so will
> > > TransactionIdDidAbort(X). So besides moving when the check happens you also
> > > changed what's being checked in a more substantial way.
> >
> > I did point this out on the thread. I made this change with the
> > intention of making the check more robust. Apparently this was
> > misguided.
> >
> > Where is the behavior that you describe documented, if anywhere?

I don't know - I think there's a explicit comment somewhere, but I couldn't
find it immediately. There's a bunch of indirect references to in in
heapam_visibility.c, with comments like "it must have aborted or
crashed".

The reason for the behaviour is that we do not have any mechanism for going
through the clog and aborting all in-progress-during-crash transactions. So
we'll end up with the clog for all in-progress-during-crash transaction being
zero / TRANSACTION_STATUS_IN_PROGRESS.

IMO it's almost always wrong to use TransactionIdDidAbort().

> When the server crashes, and we have a problem case, what does
> TransactionLogFetch()/TransactionIdGetStatus() (which are the guts of
> both TransactionIdDidCommit and TransactionIdDidAbort) report about
> the XID?

Depends a bit on the specifics, but mostly TRANSACTION_STATUS_IN_PROGRESS.

> > > Also, why did you change when MarkBufferDirty() happens? Previously it
> > > happened before we modify the page contents, now after. That's probably fine
> > > (it's the order suggested in transam/README), but seems like a mighty subtle
> > > thing to change at the same time as something unrelated, particularly without
> > > even mentioning it?
> >
> > I changed it because the new order is idiomatic. I didn't think that
> > this was particularly worth mentioning, or even subtle. The logic from
> > heap_execute_freeze_tuple() only performs simple in-place
> > modifications.

I think changes in how WAL logging is done are just about always worth
mentioning in a commit message...

Greetings,

Andres Freund