Date: Mon, 6 Nov 2023 21:35:58 -0600
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: <20231107033558.GC729644@nathanxps13>
References: <MN2PR13MB2688FD8B757316CB5C54C8A2A0DDA@MN2PR13MB2688.namprd13.prod.outlook.com>
 <20231026203433.GA1088329@nathanxps13>
 <BYAPR13MB26779988F4C73DD395401C59A0DCA@BYAPR13MB2677.namprd13.prod.outlook.com>
 <BYAPR13MB267762560386F4FB5BFDA235A0A7A@BYAPR13MB2677.namprd13.prod.outlook.com>
 <20231102034006.GA85609@nathanxps13>
 <20231106203350.GA598180@nathanxps13>
 <BYAPR13MB26777BA0E688ECE294BDE4A6A0AAA@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: <BYAPR13MB26777BA0E688ECE294BDE4A6A0AAA@BYAPR13MB2677.namprd13.prod.outlook.com>
Archived-At: <https://www.postgresql.org/message-id/20231107033558.GC729644%40nathanxps13>
Precedence: bulk

On Tue, Nov 07, 2023 at 12:57:32AM +0000, John Morris wrote:
> I incorporated your suggestions and added a few more. The changes are
> mainly related to catching potential errors if some basic assumptions
> aren’t met.

Hm.  Could we move that to a separate patch?  We've lived without these
extra checks for a very long time, and I'm not aware of any related issues,
so I'm not sure it's worth the added complexity.  And IMO it'd be better to
keep it separate from the initial atomics conversion, anyway.

> I found the comment about cache coherency a bit confusing. We are dealing
> with a single address, so there should be no memory ordering or coherency
> issues. (Did I misunderstand?) I see it more as a race condition. Rather
> than merely explaining why it shouldn’t happen, the new version verifies
> the assumptions and throws an Assert() if something goes wrong.

I was thinking of the comment for pg_atomic_read_u32() that I cited earlier
[0].  This comment also notes that pg_atomic_read_u32/64() has no barrier
semantics.  My interpretation of that comment is that these functions
provide no guarantee that the value returned is the most up-to-date value.
But my interpretation could be wrong, and maybe this is meant to highlight
that the value might change before we can use the return value in a
compare/exchange or something.

I spent a little time earlier today reviewing the various underlying
implementations, but apparently I need to spend some more time looking at
those...

[0] https://postgr.es/m/20231102034006.GA85609%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com