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?
> 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.
--
Peter Geoghegan