Date: Tue, 5 May 2026 11:08:38 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org
Subject: Re: small cleanup for s_lock.h
Message-ID: <afoWBgsygtkCNCRQ@nathan>
References: <afkUeI7UhacZ5ZFm@nathan>
 <369933.1777933007@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="UnKLvPhhsIm7C55J"
Content-Disposition: inline
In-Reply-To: <369933.1777933007@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/afoWBgsygtkCNCRQ%40nathan>
Precedence: bulk


--UnKLvPhhsIm7C55J
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, May 04, 2026 at 06:16:47PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> 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.
> 
> 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.
> 
>> 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'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 */

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

-- 
nathan

--UnKLvPhhsIm7C55J
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v2-0001-fix-up-TAS-in-s_lock.h.patch

From 0fd70f722bd9e16f82b93bf125e04dac9d599a7e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 5 May 2026 10:59:19 -0500
Subject: [PATCH v2 1/1] fix up TAS in s_lock.h

---
 src/include/storage/s_lock.h | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index c9e52511990..5ff863d1da5 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -124,7 +124,6 @@
 
 
 #ifdef __i386__		/* 32-bit i386 */
-#define HAS_TEST_AND_SET
 
 typedef unsigned char slock_t;
 
@@ -194,7 +193,6 @@ spin_delay(void)
 
 
 #ifdef __x86_64__		/* AMD Opteron, Intel EM64T */
-#define HAS_TEST_AND_SET
 
 typedef unsigned char slock_t;
 
@@ -249,7 +247,6 @@ spin_delay(void)
  */
 #if defined(__arm__) || defined(__arm) || defined(__aarch64__)
 #ifdef HAVE_GCC__SYNC_INT32_TAS
-#define HAS_TEST_AND_SET
 
 #define TAS(lock) tas(lock)
 
@@ -292,7 +289,6 @@ spin_delay(void)
 
 /* S/390 and S/390x Linux (32- and 64-bit zSeries) */
 #if defined(__s390__) || defined(__s390x__)
-#define HAS_TEST_AND_SET
 
 typedef unsigned int slock_t;
 
@@ -321,7 +317,6 @@ tas(volatile slock_t *lock)
  * acquire/release semantics. The CPU will treat superfluous members as
  * NOPs, so it's just code space.
  */
-#define HAS_TEST_AND_SET
 
 typedef unsigned char slock_t;
 
@@ -392,7 +387,6 @@ do \
 
 /* PowerPC */
 #if defined(__ppc__) || defined(__powerpc__) || defined(__ppc64__) || defined(__powerpc64__)
-#define HAS_TEST_AND_SET
 
 typedef unsigned int slock_t;
 
@@ -453,7 +447,6 @@ do \
 
 
 #if defined(__mips__) && !defined(__sgi)	/* non-SGI MIPS */
-#define HAS_TEST_AND_SET
 
 typedef unsigned int slock_t;
 
@@ -531,10 +524,9 @@ do \
  * grounds that that's known to be more likely to work in the ARM ecosystem.
  * (But we dealt with ARM above.)
  */
-#if !defined(HAS_TEST_AND_SET)
+#if !defined(TAS)
 
 #if defined(HAVE_GCC__SYNC_INT32_TAS)
-#define HAS_TEST_AND_SET
 
 #define TAS(lock) tas(lock)
 
@@ -549,7 +541,6 @@ tas(volatile slock_t *lock)
 #define S_UNLOCK(lock) __sync_lock_release(lock)
 
 #elif defined(HAVE_GCC__SYNC_CHAR_TAS)
-#define HAS_TEST_AND_SET
 
 #define TAS(lock) tas(lock)
 
@@ -565,7 +556,7 @@ tas(volatile slock_t *lock)
 
 #endif	 /* HAVE_GCC__SYNC_INT32_TAS */
 
-#endif	/* !defined(HAS_TEST_AND_SET) */
+#endif	/* !defined(TAS) */
 
 
 /*
@@ -592,12 +583,11 @@ tas(volatile slock_t *lock)
  * ---------------------------------------------------------------------
  */
 
-#if !defined(HAS_TEST_AND_SET)	/* We didn't trigger above, let's try here */
+#if !defined(TAS)				/* We didn't trigger above, let's try here */
 
 #ifdef _MSC_VER
 typedef LONG slock_t;
 
-#define HAS_TEST_AND_SET
 #define TAS(lock) (InterlockedCompareExchange(lock, 1, 0))
 
 #define SPIN_DELAY() spin_delay()
@@ -649,11 +639,13 @@ spin_delay(void)
 #endif
 
 
-#endif	/* !defined(HAS_TEST_AND_SET) */
+#endif	/* !defined(TAS) */
 
 
 /* Blow up if we didn't have any way to do spinlocks */
-#ifndef HAS_TEST_AND_SET
+#ifdef TAS
+#define HAS_TEST_AND_SET
+#else
 #error PostgreSQL does not have spinlock support on this platform.  Please report this to pgsql-bugs@lists.postgresql.org.
 #endif
 
@@ -697,13 +689,6 @@ 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)
 #define TAS_SPIN(lock)	TAS(lock)
 #endif	 /* TAS_SPIN */
-- 
2.50.1 (Apple Git-155)


--UnKLvPhhsIm7C55J--