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.
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 |