On Tue, Jan 3, 2023 at 4:54 PM Andres Freund <and...@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