From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
cc: pgsql-hackers@postgresql.org
Subject: Re: small cleanup for s_lock.h
In-reply-to: <afoWBgsygtkCNCRQ@nathan>
References: <afkUeI7UhacZ5ZFm@nathan> <369933.1777933007@sss.pgh.pa.us>
 <afoWBgsygtkCNCRQ@nathan>
Comments: In-reply-to Nathan Bossart <nathandbossart@gmail.com>
	message dated "Tue, 05 May 2026 11:08:38 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <532586.1778000057.0@sss.pgh.pa.us>
Date: Tue, 05 May 2026 12:56:09 -0400
Message-ID: <532705.1778000169@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/532705.1778000169%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <532586.1778000057.1@sss.pgh.pa.us>

Nathan Bossart <nathandbossart@gmail.com> writes:
> Okay, here's a new version of the patch that I believe addresses both
> points.

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="clarify-S_LOCK-vs-TAS.patch";
	charset="us-ascii"
Content-ID: <532586.1778000057.2@sss.pgh.pa.us>
Content-Description: clarify-S_LOCK-vs-TAS.patch
Content-Transfer-Encoding: quoted-printable

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.  Pleas=
e report this to pgsql-bugs@lists.postgresql.org.
-#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.  Pleas=
e report this to pgsql-bugs@lists.postgresql.org.
+#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 */
 =

 =


------- =_aaaaaaaaaa0--