Date: Thu, 19 Feb 2026 10:08:20 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: convert SpinLock* macros to static inline functions
Message-ID: <aZc1dO205sbKvkZp@nathan>
References: <aZX2oUcKf7IzHnnK@nathan>
 <wbbl44cvhsbt5nm2yi7e67mpgztn56j65yznlkkzlcp3sriy6f@t4p5e6knf5hv>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <wbbl44cvhsbt5nm2yi7e67mpgztn56j65yznlkkzlcp3sriy6f@t4p5e6knf5hv>
Archived-At: <https://www.postgresql.org/message-id/aZc1dO205sbKvkZp%40nathan>
Precedence: bulk

Thanks for looking.

On Wed, Feb 18, 2026 at 04:23:28PM -0500, Andres Freund wrote:
> Not quite as sure it's the right thing to remove it from SpinLockAcquire()
> though. I think removing it more widely would likely cause issues, e.g. if you
> removed it from s_lock(), the compiler would be in its right to just turn:
> 
> int
> s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
> ...
>         while (TAS_SPIN(lock))
>         {
>                 perform_spin_delay(&delayStatus);
>         }
> ...
> 
> into
> ...
>     if (*lock)
>     {
>         while (true)
>             perform_spin_delay(&delayStatus);
>     }
>     else
>         TAS(lock);
> ...
> 
> as without the volatile, or a compiler barrier, the compiler can assume that
> nothing will change the the value of *lock (at least theoretically, if it can
> prove that perform_spin_delay() doesn't change the value of *lock).

This crossed my mind, but I couldn't see how it could introduce any issues
that don't already exist.  Both tas() and s_lock() have the lock parameter
marked as volatile, and S_LOCK is often expanded into functions where the
lock variable is not marked volatile.  Why would those existing cases not
be subject to this problem but the static inline function would?  And why
would marking SpinLockAcquire()'s lock parameter volatile fix it if doing
so for tas() and s_lock() doesn't?  FWIW I don't see any changes to the
code generated for s_lock() with these patches, although I only looked on a
couple of machines.

Perhaps it's still worth doing out of an abundance of caution, or maybe I
am missing something subtle about the liberties that compilers take in this
area...

-- 
nathan