Re: Atomic ops for unlogged LSN

From: John Morris <john(dot)morris(at)crunchydata(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>, Nathan Bossart <nathandbossart(at)gmail(dot)com>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Michael Paquier <michael(at)paquier(dot)xyz>, Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Atomic ops for unlogged LSN
Date: 2023-07-19 18:55:10
Message-ID: MN2PR13MB2688E02CB8A1AD4F197222CCA039A@MN2PR13MB2688.namprd13.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

>> Why don't we just use a barrier when around reading the value? It's not like
>> CreateCheckPoint() is frequent?

One reason is that a barrier isn’t needed, and adding unnecessary barriers can also be confusing.

With respect to the “debug only” comment in the original code, whichever value is written to the structure during a checkpoint will be reset when restarting. Nobody will ever see that value. We could just as easily write a zero.

Shutting down is a different story. It isn’t stated, but we require the unlogged LSN be quiescent before shutting down. This patch doesn’t change that requirement.

We could also argue that memory ordering doesn’t matter when filling in a conventional, unlocked structure. But the fact we have only two cases 1) checkpoint where the value is ignored, and 2) shutdown where it is quiescent, makes the additional argument unnecessary.

Would you be more comfortable if I added comments describing the two cases? My intent was to be minimalistic, but the original code could use better explanation.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2023-07-19 19:20:22 Re: Extracting cross-version-upgrade knowledge from buildfarm client
Previous Message David Zhang 2023-07-19 18:21:17 Re: Requiring recovery.signal or standby.signal when recovering with a backup_label