(Pruning -committers from the list, since cross-posting to -hackers
resulted in this being held up for moderation.)
On Tue, Jan 3, 2023 at 5:15 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Tue, Jan 3, 2023 at 4:54 PM Andres Freund <andres@anarazel.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?
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?
> > 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'm including this here because presumably -hackers will have missed
it due to the moderation hold-up issue.
--
Peter Geoghegan