Date: Thu, 7 May 2026 16:03:09 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: small cleanup for s_lock.h
Message-ID: <afz-Da9NjHy-ubES@nathan>
References: <afkUeI7UhacZ5ZFm@nathan>
 <CALdSSPgchta97kNGkbPVXa88+d-=Z_pKsfHUx_2qT2R63N=_sw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="JmXY/Yn6swRVDyih"
Content-Disposition: inline
In-Reply-To: 
 <CALdSSPgchta97kNGkbPVXa88+d-=Z_pKsfHUx_2qT2R63N=_sw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/afz-Da9NjHy-ubES%40nathan>
Precedence: bulk


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

On Tue, May 05, 2026 at 03:20:46AM +0500, Kirill Reshke wrote:
> 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?

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

-- 
nathan

--JmXY/Yn6swRVDyih
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v4-0001-Remove-fallback-declaration-for-tas.patch

From 595bbb48a7de3dd191023d617dc2c7f4725ed2d0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 May 2026 16:04:04 -0500
Subject: [PATCH v4 1/3] Remove fallback declaration for tas().

The last definition of tas() in s_lock.c was removed in commit
718aa43a4e, and the last tas.s file was removed in commit
25f36066dd, so this is dead code.
---
 src/backend/Makefile         | 2 +-
 src/backend/port/.gitignore  | 1 -
 src/backend/port/meson.build | 2 +-
 src/include/storage/s_lock.h | 7 -------
 4 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/src/backend/Makefile b/src/backend/Makefile
index 162d3f1f2a9..4bb76d3d397 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -301,7 +301,7 @@ endif
 
 distclean: clean
 # generated by configure
-	rm -f port/tas.s port/pg_sema.c port/pg_shmem.c
+	rm -f port/pg_sema.c port/pg_shmem.c
 
 
 ##########################################################################
diff --git a/src/backend/port/.gitignore b/src/backend/port/.gitignore
index 4ef36b82c77..6c5067a4a9f 100644
--- a/src/backend/port/.gitignore
+++ b/src/backend/port/.gitignore
@@ -1,3 +1,2 @@
 /pg_sema.c
 /pg_shmem.c
-/tas.s
diff --git a/src/backend/port/meson.build b/src/backend/port/meson.build
index e8b7da8d281..29e88ef3541 100644
--- a/src/backend/port/meson.build
+++ b/src/backend/port/meson.build
@@ -30,4 +30,4 @@ if host_system == 'windows'
 endif
 
 # autoconf generates the file there, ensure we get a conflict
-generated_sources_ac += {'src/backend/port': ['pg_sema.c', 'pg_shmem.c', 'tas.s']}
+generated_sources_ac += {'src/backend/port': ['pg_sema.c', 'pg_shmem.c']}
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index c9e52511990..dcfec8ce2af 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -697,13 +697,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)


--JmXY/Yn6swRVDyih
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v4-0002-Remove-HAS_TEST_AND_SET.patch

From ea055e53d64b97e74a87b02d424dee48fc66333b Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 May 2026 16:24:30 -0500
Subject: [PATCH v4 2/3] Remove HAS_TEST_AND_SET.

This is only set when TAS is defined, so we can just check whether
TAS is defined directly instead.
---
 src/include/storage/s_lock.h | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index dcfec8ce2af..fb872edd2f0 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,11 @@ 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
+#ifndef TAS
 #error PostgreSQL does not have spinlock support on this platform.  Please report this to pgsql-bugs@lists.postgresql.org.
 #endif
 
-- 
2.50.1 (Apple Git-155)


--JmXY/Yn6swRVDyih
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v4-0003-Better-express-platform-requirements-in-s_lock.h.patch

From cd371f840ff8965b1efb0b989be3059272520e7c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 7 May 2026 15:32:50 -0500
Subject: [PATCH v4 3/3] Better express platform requirements in s_lock.h.

---
 src/include/storage/s_lock.h | 56 ++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index fb872edd2f0..c4369ee4ff6 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -44,23 +44,16 @@
  *		atomic test-and-set only when it appears free.
  *
  *	TAS() and TAS_SPIN() are NOT part of the API, and should never be called
- *	directly.
- *
- *	CAUTION: on some platforms TAS() and/or TAS_SPIN() may sometimes report
- *	failure to acquire a lock even when the lock is not locked.  For example,
- *	on Alpha TAS() will "fail" if interrupted.  Therefore a retry loop must
- *	always be used, even if you are certain the lock is free.
+ *	directly.  If a platform-specific TAS() is defined, the platform must
+ *	_not_ define its own S_LOCK().  Conversely, if a platform-specific
+ *	S_LOCK() is defined, the platform must _not_ define its own TAS(), but
+ *	it does need to define its own TAS_SPIN().  Currently, all supported
+ *	platforms define TAS() and use the default S_LOCK() implementation, so
+ *	that is probably a good place to start if adding a new one.
  *
  *	It is the responsibility of these macros to make sure that the compiler
  *	does not re-order accesses to shared memory to precede the actual lock
- *	acquisition, or follow the lock release.  Prior to PostgreSQL 9.5, this
- *	was the caller's responsibility, which meant that callers had to use
- *	volatile-qualified pointers to refer to both the spinlock itself and the
- *	shared data being accessed within the spinlocked critical section.  This
- *	was notationally awkward, easy to forget (and thus error-prone), and
- *	prevented some useful compiler optimizations.  For these reasons, we
- *	now require that the macros themselves prevent compiler re-ordering,
- *	so that the caller doesn't need to take special precautions.
+ *	acquisition, or follow the lock release.
  *
  *	On platforms with weak memory ordering, the TAS(), TAS_SPIN(), and
  *	S_UNLOCK() macros must further include hardware-level memory fence
@@ -72,7 +65,7 @@
  *
  *	On most supported platforms, TAS() uses a tas() function written
  *	in assembly language to execute a hardware atomic-test-and-set
- *	instruction.  Equivalent OS-supplied mutex routines could be used too.
+ *	instruction.  Equivalent compiler intrinsics are another popular option.
  *
  *
  * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
@@ -642,20 +635,25 @@ spin_delay(void)
 #endif	/* !defined(TAS) */
 
 
-/* Blow up if we didn't have any way to do spinlocks */
-#ifndef TAS
-#error PostgreSQL does not have spinlock support on this platform.  Please report this to pgsql-bugs@lists.postgresql.org.
-#endif
-
-
 /*
  * Default Definitions - override these above as needed.
  */
 
-#if !defined(S_LOCK)
+/*
+ * Make sure S_LOCK is defined, either explicitly for the platform or via a TAS
+ * macro for the platform.  Exactly one of either S_LOCK or TAS should be
+ * defined for a supported platform at this point in the file.
+ */
+#if defined(S_LOCK)
+#if defined(TAS)
+#error Both TAS and S_LOCK defined on this platform.  Please report this to pgsql-bugs@lists.postgresql.org.
+#endif
+#elif defined(TAS)
 #define S_LOCK(lock) \
 	(TAS(lock) ? s_lock((lock), __FILE__, __LINE__, __func__) : 0)
-#endif	 /* S_LOCK */
+#else
+#error Neither TAS nor S_LOCK defined on this platform.  Please report this to pgsql-bugs@lists.postgresql.org.
+#endif
 
 #if !defined(S_UNLOCK)
 /*
@@ -687,9 +685,19 @@ extern void s_unlock(volatile slock_t *lock);
 #define SPIN_DELAY()	((void) 0)
 #endif	 /* SPIN_DELAY */
 
+/*
+ * 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)
 #define TAS_SPIN(lock)	TAS(lock)
-#endif	 /* TAS_SPIN */
+#else
+#error Neither TAS nor TAS_SPIN defined on this platform.  Please report this to pgsql-bugs@lists.postgresql.org.
+#endif	 /* TAS */
+#endif	 /* ! TAS_SPIN */
 
 
 /*
-- 
2.50.1 (Apple Git-155)


--JmXY/Yn6swRVDyih--