Date: Mon, 12 Jun 2023 16:05:39 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>,
	John Morris <john.morris@crunchydata.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	Stephen Frost <sfrost@snowman.net>
Subject: Re: Atomic ops for unlogged LSN
Message-ID: <20230612230539.GB178071@nathanxps13>
References: <BYAPR13MB26772534335255E50318C574A0409@BYAPR13MB2677.namprd13.prod.outlook.com>
 <ZG09kSRxOdvz5TVv@paquier.xyz>
 <CA+TgmoaVokb-_VkSL=0k0XCj5ow0ew8M1+e9=7JKvgtDqibzpA@mail.gmail.com>
 <20230524214958.mt6f5xokpumvnrio@awork3.anarazel.de>
 <ZG6SkQwO2obI95js@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZG6SkQwO2obI95js@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/20230612230539.GB178071%40nathanxps13>
Precedence: bulk

On Thu, May 25, 2023 at 07:41:21AM +0900, Michael Paquier wrote:
> On Wed, May 24, 2023 at 02:49:58PM -0700, Andres Freund wrote:
>> So we indeed loose some "barrier strength" - but I think that's fine. For one,
>> it's a debugging-only value.

Is it?  I see uses in GiST indexing (62401db), so it's not immediately
obvious to me how it is debugging-only.  If it is, then I think this patch
ought to clearly document it so that nobody else tries to use it for
non-debugging-only stuff.

>> But more importantly, I don't see what reordering
>> the barrier could prevent - a barrier is useful for things like sequencing two
>> memory accesses to happen in the intended order - but unloggedLSN doesn't
>> interact with another variable that's accessed within the ControlFileLock
>> afaict.
> 
> This stuff is usually tricky enough that I am never completely sure
> whether it is fine without reading again README.barrier, which is
> where unloggedLSN is saved in the control file under its LWLock.
> Something that I find confusing in the patch is that it does not
> document the reason why this is OK.

My concern would be whether GetFakeLSNForUnloggedRel or CreateCheckPoint
might see an old value of unloggedLSN.  From the following note in
README.barrier, it sounds like this would be a problem even if we ensured
full barrier semantics:

    3. No ordering guarantees.  While memory barriers ensure that any given
    process performs loads and stores to shared memory in order, they don't
    guarantee synchronization.  In the queue example above, we can use memory
    barriers to be sure that readers won't see garbage, but there's nothing to
    say whether a given reader will run before or after a given writer.  If this
    matters in a given situation, some other mechanism must be used instead of
    or in addition to memory barriers.

IIUC we know that shared memory accesses cannot be reordered to precede
aquisition or follow release of a spinlock (thanks to 0709b7e), which is
why this isn't a problem in the current implementation.

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