Subject: small cleanup for s_lock.h

From: Nathan Bossart <[email-masked]>
Date: Mon, 4 May 2026 16:49:44 -0500

I noticed that s_lock.h points to a default implementation of tas() in
tas.s or s_lock.c, but AFAICT there hasn't been a tas() implementation in
s_lock.c since commit 718aa43a4e, and commit 25f36066dd seems to have
removed the last remaining tas.s files.  So, I think this is dead code.

I also noticed that HAS_TEST_AND_SET just means that TAS is defined, so I
wrote a 0002 that removes it in favor of checking TAS directly.  I'd like
to rewrite the comment at the top of the file, too, but haven't gotten to
that yet.  I find it a little misleading, especially because we #error if
TAS isn't defined.

-- 
nathan

---

From: "Tristan Partin" <[email-masked]>
Date: Mon, 04 May 2026 22:11:02 +0000

> that yet.  I find it a little misleading, especially because we #error if

This looks pretty reasonable to me.

-- 
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)

---

From: Tom Lane <[email-masked]>
Date: Mon, 04 May 2026 18:16:47 -0400

> removed the last remaining tas.s files.  So, I think this is dead code.

It is, but I think the 0001 patch should be more like

 #if !defined(TAS)
-extern int	tas(volatile slock_t *lock);		/* in port/.../tas.s, or
-							 * s_lock.c */
-
-#define TAS(lock)		tas(lock)
+#error "must provide a spinlock implementation"
 #endif	 /* TAS */

Perhaps this could be merged with the earlier bit about erroring
if not HAS_TEST_AND_SET.

> wrote a 0002 that removes it in favor of checking TAS directly.

I'm pretty much -1 on that; HAS_TEST_AND_SET is clearer than TAS, and
removing it seems quite likely to break someone's code.  We could
perhaps collect all the separate instances into this end location:

#if defined(TAS)
#define HAS_TEST_AND_SET
#else
#error "must provide a spinlock implementation"
#endif	 /* TAS */

> I'd like

No objection in principle to improving that comment, but what did you
have in mind exactly?

			regards, tom lane

---

From: Kirill Reshke <[email-masked]>
Date: Tue, 5 May 2026 03:20:46 +0500

> removed the last remaining tas.s files.  So, I think this is dead code.

This indeed looks like a dead code. I also noticed `tas.s` is present
in meson.build, gitignore and src/backend/Makefile
should we remove that too?

-- 
Best regards,
Kirill Reshke

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 5 May 2026 10:49:30 -0500

On Mon, May 04, 2026 at 06:16:47PM -0400, Tom Lane wrote:
>> I'd like to rewrite the comment at the top of the file, too, but haven't
>> gotten to that yet.  I find it a little misleading, especially because
>> we #error if TAS isn't defined.
> 

I think the way the comment presents the macros gives a potentially
misleading impression about what you typically need to do to get a new
platform working, and you basically need to read through the whole file to
make sense of what's going on.  Some of the macros it mentions have a
default implementation that we use everywhere (e.g., S_INIT_LOCK), and if
you're using gcc, you may be able to just use the __sync_lock_test_and_set
versions.  If you _did_ need to add a new section for a new platform, you'd
probably be more interested in defining slock_t, HAS_TEST_AND_TEST/TAS,
S_UNLOCK, SPIN_DELAY, and maybe TAS_SPIN.  In fact, you _must_ ensure TAS
is defined or else we'll fail to compile.

Although as I write this e-mail and think about how exactly I'd rewrite the
comment, I grow less confident about doing so...

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 5 May 2026 11:08:38 -0500

> #error "must provide a spinlock implementation"

Okay, here's a new version of the patch that I believe addresses both
points.

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Tue, 05 May 2026 12:56:09 -0400

> Okay, here's a new version of the patch that I believe addresses both

This seems cleaner than what we have ... but ...

After thinking some more I realized that what's confusing us here
is an API-level problem.  s_lock.h's header comment says

 *  Usually, S_LOCK() is implemented in terms of even lower-level macros
 *  TAS() and TAS_SPIN():

As things stand, we have no platforms where that's not the case,
and so we've lost sight of the fact that the contract shouldn't be
"you must provide TAS()".  It should be "you must either provide
S_LOCK(), or provide TAS() to base it on".

A rough cut as to the right way to do this is attached.  The
main loose end here is that it's not very clear what s_lock.c's
s_lock() should do if there's no TAS (and hence no TAS_SPIN).
Maybe we should just not compile that function at all without
TAS; if a platform provides a non-default S_LOCK that needs a
helper function, it's on the platform to supply that helper.

Also, after noting that HAS_TEST_AND_SET is referenced nowhere
outside s_lock.h, I'm coming around to your previous position
that it's redundant and we should drop it.  This is mainly because
it's not clear to me whether it should be set on a platform that
provides S_LOCK but not TAS.  I didn't touch that here though.

Lastly, I definitely agree now that the file's header comment needs
some work.  Maybe this insight helps you with that?  (One thing
I noticed is that the ending comment about "Equivalent OS-supplied
mutex routines could be used too" feels pretty obsolete.  Maybe
instead, "Equivalent compiler intrinsics are another popular option".)

			regards, tom lane

diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index c9e52511990..0a13c958fc8 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -652,19 +652,18 @@ spin_delay(void)
 #endif	/* !defined(HAS_TEST_AND_SET) */

-/* Blow up if we didn't have any way to do spinlocks */
-#ifndef HAS_TEST_AND_SET
-#error PostgreSQL does not have spinlock support on this platform.  Please report this to [email-masked].
-#endif
-
-
 /*
  * Default Definitions - override these above as needed.
  */
 
 #if !defined(S_LOCK)
+#if defined(TAS)
 #define S_LOCK(lock) \
 	(TAS(lock) ? s_lock((lock), __FILE__, __LINE__, __func__) : 0)
+#else
+/* Must provide S_LOCK, or a TAS macro to base it on */
+#error PostgreSQL does not have spinlock support on this platform.  Please report this to [email-masked].
+#endif	 /* TAS */
 #endif	 /* S_LOCK */
 
 #if !defined(S_UNLOCK)
@@ -697,15 +696,10 @@ extern void s_unlock(volatile slock_t *lock);
 #define SPIN_DELAY()	((void) 0)
 #endif	 /* SPIN_DELAY */
 
-#if !defined(TAS)
-extern int	tas(volatile slock_t *lock);		/* in port/.../tas.s, or
-												 * s_lock.c */
-
-#define TAS(lock)		tas(lock)
-#endif	 /* TAS */
-
 #if !defined(TAS_SPIN)
+#if defined(TAS)
 #define TAS_SPIN(lock)	TAS(lock)
+#endif	 /* TAS */
 #endif	 /* TAS_SPIN */

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 5 May 2026 12:57:10 -0500

> helper function, it's on the platform to supply that helper.

That makes sense to me.

> instead, "Equivalent compiler intrinsics are another popular option".)

I think it does help, thanks.  I'll give it a whirl.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 7 May 2026 15:41:56 -0500

> I think it does help, thanks.  I'll give it a whirl.

Here is a first try.  While at it, I removed the note about using
"volatile" prior to v9.5, as well as the note about using TAS() in a loop.
I couldn't really understand why the latter part needs to be said out loud,
so I did some more digging.  It was added by commit 7f60b81e and refers to
an unsupported platform.  At the time, it was apparently normal to use
TAS() outside of s_lock.h, but the "NOT part of the API" note above it was
added the following year in commit 499abb0c.

Two other things I noticed after staring at s_lock.h for a while:

* If we're willing to define TAS_SPIN to first do an unlocked test
everywhere, we can simplify the ARM and x86 code.  Specifically, we can
merge the x86 blocks together, and we can remove all but the SPIN_DELAY
definition for AArch64.  We've thus far been hesistant to add the unlocked
test to platforms without evidence of improvements, but I'm a little
skeptical that folks have performance-critical workloads on architectures
that don't already have it.

* Furthermore, do we really need architecture-specific implementations of
anything except for SPIN_DELAY()?  I suspect that
__sync_lock_test_and_set() and __sync_lock_release() work pretty well most
of the time, and they've been available in gcc and clang for ~20 years.
Perhaps we could even start using the __atomic builtins if available.
We've been slowly trying to move away from spinlocks, anyway, and I'm not
seeing huge differences in generated code on https://godbolt.org/ for newer
compiler versions.

Of course, further research and benchmarking would be needed, but I figured
I'd at least jot down these thoughts.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 7 May 2026 16:03:09 -0500

> should we remove that too?

I forgot to remove these in v3.  Here's a new patch set with that taken
care of.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 7 May 2026 16:12:09 -0500

On Thu, May 07, 2026 at 03:41:56PM -0500, Nathan Bossart wrote:
> +/*
> + * We can only define TAS_SPIN if TAS was defined.  Otherwise, the platform
> + * defined its own S_LOCK without TAS, and therefore is responsible for
> + * defining its own TAS_SPIN as well.  (Note that we currently do not have any
> + * platforms that don't define TAS.)
> + */
>  #if !defined(TAS_SPIN)
> +#if defined(TAS)
> -#endif	 /* TAS_SPIN */
> +#error Neither TAS nor TAS_SPIN defined on this platform.  Please report this to [email-masked].
> +#endif	 /* TAS */
> +#endif	 /* ! TAS_SPIN */

Wait, this isn't right.  TAS_SPIN is only used by s_lock(), which is only
used by the default S_LOCK.  We should just not compile s_lock() if the
platform defines its own S_LOCK, and we shouldn't #error here if TAS is not
defined.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 7 May 2026 16:28:09 -0500

> platform defines its own S_LOCK, and we shouldn't #error here if TAS is not

Should be fixed in v5, sorry for the noise.

-- 
nathan