(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 <p...@bowt.ie> wrote: > > 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? 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
