Date: Tue, 15 Aug 2023 08:22:24 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michail Nikolaev <michail.nikolaev@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Replace known_assigned_xids_lck by memory barrier
Message-ID: <20230815152224.GA2296544@nathanxps13>
References: <CANtu0oh0si=jG5z_fLeFtmYcETssQ08kLEa8b6TQqDm_cinroA@mail.gmail.com>
 <20230814153634.GB1395427@nathanxps13>
 <CANtu0ohnGXW6wfM7eXts=m23A1g9D7s1X0ms7bV3-x5oQugX5g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANtu0ohnGXW6wfM7eXts=m23A1g9D7s1X0ms7bV3-x5oQugX5g@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230815152224.GA2296544%40nathanxps13>
Precedence: bulk

On Tue, Aug 15, 2023 at 12:29:24PM +0200, Michail Nikolaev wrote:
>> What sort of benefits do you see from this patch? It might be worthwhile
>> in itself to remove spinlocks when possible, but IME it's much easier to
>> justify such changes when there is a tangible benefit we can point to.
> 
> Oh, it is not an easy question :)
> 
> The answer, probably, looks like this:
> 1) performance benefits of spin lock acquire removing in
> KnownAssignedXidsGetOldestXmin and KnownAssignedXidsSearch
> 2) it is closing 13-year-old tech depth
> 
> But in reality, it is not easy to measure performance improvement
> consistently for this change.

Okay.  Elsewhere, it seems like folks are fine with patches that reduce
shared memory space via atomics or barriers even if there's no immediate
benefit [0], so I think it's fine to proceed.

>> Are the assignments in question guaranteed to be atomic? IIUC we assume
>> that aligned 4-byte loads/stores are atomic, so we should be okay as long
>> as we aren't handling anything larger.
> 
> Yes, 4-bytes assignment are atomic, locking is used to ensure memory
> write ordering in this place.

Yeah, it looks like both the values that are protected by
known_assigned_xids_lck are integers, so this should be okay.  One
remaining question I have is whether it is okay if we see an updated value
for one of the head/tail variables but not the other.  It looks like the
tail variable is only updated with ProcArrayLock held exclusively, which
IIUC wouldn't prevent such mismatches even today, since we use a separate
spinlock for reading them in some cases.

[0] https://postgr.es/m/20230524214958.mt6f5xokpumvnrio%40awork3.anarazel.de

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