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