Date: Wed, 1 Nov 2023 22:40:06 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: John Morris <john.morris@crunchydata.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Stephen Frost <sfrost@snowman.net>,
	Michael Paquier <michael@paquier.xyz>,
	Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Atomic ops for unlogged LSN
Message-ID: <20231102034006.GA85609@nathanxps13>
References: <MN2PR13MB2688FD8B757316CB5C54C8A2A0DDA@MN2PR13MB2688.namprd13.prod.outlook.com>
 <20231026203433.GA1088329@nathanxps13>
 <BYAPR13MB26779988F4C73DD395401C59A0DCA@BYAPR13MB2677.namprd13.prod.outlook.com>
 <BYAPR13MB267762560386F4FB5BFDA235A0A7A@BYAPR13MB2677.namprd13.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <BYAPR13MB267762560386F4FB5BFDA235A0A7A@BYAPR13MB2677.namprd13.prod.outlook.com>
Archived-At: <https://www.postgresql.org/message-id/20231102034006.GA85609%40nathanxps13>
Precedence: bulk

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->Insert.insertpos_lck);
 	SpinLockInit(&XLogCtl->info_lck);
-	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: https://aws.amazon.com