Re: Atomic ops for unlogged LSN

From: Nathan Bossart <nathandbossart(at)gmail(dot)com>
To: John Morris <john(dot)morris(at)crunchydata(dot)com>
Cc: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, 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-11-02 03:40:06
Message-ID: 20231102034006.GA85609@nathanxps13
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Nov 01, 2023 at 09:15:20PM +0000, John Morris wrote:
> This is a rebased version . Even though I labelled it “v3”, there should be no changes from “v2”.

Thanks. I think this is almost ready, but I have to harp on the
pg_atomic_read_u64() business once more. The relevant comment in atomics.h
has this note:

* The read is guaranteed to return a value as it has been written by this or
* another process at some point in the past. There's however no cache
* coherency interaction guaranteeing the value hasn't since been written to
* again.

However unlikely, this seems to suggest that CreateCheckPoint() could see
an old value with your patch. Out of an abundance of caution, I'd
recommend changing this to pg_atomic_compare_exchange_u64() like
pg_atomic_read_u64_impl() does in generic.h.

@@ -4635,7 +4629,6 @@ XLOGShmemInit(void)

- SpinLockInit(&XLogCtl->ulsn_lck);

Shouldn't we do the pg_atomic_init_u64() here? We can still set the
initial value in StartupXLOG(), but it might be safer to initialize the
variable where we are initializing the other shared memory stuff.

Since this isn't a tremendously performance-sensitive area, IMHO we should
code defensively to eliminate any doubts about correctness and to make it
easier to reason about.

Nathan Bossart
Amazon Web Services:

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Nathan Bossart 2023-11-02 03:45:57 Re: Don't pass NULL pointer to strcmp().
Previous Message Peter Smith 2023-11-02 03:32:07 Re: A recent message added to pg_upgade