Thread: Re: pgsql: Delay commit status checks until freezing executes.

Re: pgsql: Delay commit status checks until freezing executes.

From
Andres Freund
Date:
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



Re: pgsql: Delay commit status checks until freezing executes.

From
Peter Geoghegan
Date:
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