Обсуждение: Re: pgsql: Delay commit status checks until freezing executes.
Hi, On 2023-01-03 19:23:41 +0000, Peter Geoghegan wrote: > Delay commit status checks until freezing executes. > > pg_xact lookups are relatively expensive. Move the xmin/xmax commit > status checks from the point that freeze plans are prepared to the point > that they're actually executed. Otherwise we'll repeat many commit > status checks whenever multiple successive VACUUM operations scan the > same pages and decide against freezing each time, which is a waste of > cycles. > > Oversight in commit 1de58df4, which added page-level freezing. > > Author: Peter Geoghegan <pg@bowt.ie> > Discussion: https://postgr.es/m/CAH2-WzkZpe4K6qMfEt8H4qYJCKc2R7TPvKsBva7jc9w7iGXQSw@mail.gmail.com 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. 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? Greetings, Andres Freund
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