agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedsmall cleanup for s_lock.h
14+ messages / 4 participants
[nested] [flat]
* small cleanup for s_lock.h
@ 2026-05-04 21:49 Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 3 replies; 14+ messages in thread
From: Nathan Bossart @ 2026-05-04 21:49 UTC (permalink / raw)
To: pgsql-hackers
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 a254cee054ccd22c6c11f876c5f8549f28f6499c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 May 2026 16:04:04 -0500
Subject: [PATCH v1 1/2] 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/include/storage/s_lock.h | 7 -------
1 file changed, 7 deletions(-)
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)
From 60d6d767fc8e318f303c72b5883e3a15656b9ff5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 May 2026 16:24:30 -0500
Subject: [PATCH v1 2/2] 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)
Attachments:
[text/plain] v1-0001-Remove-fallback-declaration-for-tas.patch (1.0K, ../../afkUeI7UhacZ5ZFm@nathan/2-v1-0001-Remove-fallback-declaration-for-tas.patch)
download | inline diff:
From a254cee054ccd22c6c11f876c5f8549f28f6499c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 May 2026 16:04:04 -0500
Subject: [PATCH v1 1/2] 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/include/storage/s_lock.h | 7 -------
1 file changed, 7 deletions(-)
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)
[text/plain] v1-0002-Remove-HAS_TEST_AND_SET.patch (3.2K, ../../afkUeI7UhacZ5ZFm@nathan/3-v1-0002-Remove-HAS_TEST_AND_SET.patch)
download | inline diff:
From 60d6d767fc8e318f303c72b5883e3a15656b9ff5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 May 2026 16:24:30 -0500
Subject: [PATCH v1 2/2] 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)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-04 22:11 Tristan Partin <tristan@partin.io>
parent: Nathan Bossart <nathandbossart@gmail.com>
2 siblings, 0 replies; 14+ messages in thread
From: Tristan Partin @ 2026-05-04 22:11 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
On Mon May 4, 2026 at 4:50 PM CDT, Nathan Bossart wrote:
> 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.
This looks pretty reasonable to me.
--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-04 22:16 Tom Lane <tgl@sss.pgh.pa.us>
parent: Nathan Bossart <nathandbossart@gmail.com>
2 siblings, 2 replies; 14+ messages in thread
From: Tom Lane @ 2026-05-04 22:16 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
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 */
> 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.
No objection in principle to improving that comment, but what did you
have in mind exactly?
regards, tom lane
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-04 22:20 Kirill Reshke <reshkekirill@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
2 siblings, 1 reply; 14+ messages in thread
From: Kirill Reshke @ 2026-05-04 22:20 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
On Tue, 5 May 2026 at 02:49, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> 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.
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 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
--
Best regards,
Kirill Reshke
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-05 15:49 Nathan Bossart <nathandbossart@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 14+ messages in thread
From: Nathan Bossart @ 2026-05-05 15:49 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
On Mon, May 04, 2026 at 06:16:47PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> 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.
>
> No objection in principle to improving that comment, but what did you
> have in mind exactly?
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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-05 16:08 Nathan Bossart <nathandbossart@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2026-05-05 16:08 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
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
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)
Attachments:
[text/plain] v2-0001-fix-up-TAS-in-s_lock.h.patch (3.6K, ../../afoWBgsygtkCNCRQ@nathan/2-v2-0001-fix-up-TAS-in-s_lock.h.patch)
download | inline diff:
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)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-05 16:56 Tom Lane <tgl@sss.pgh.pa.us>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Tom Lane @ 2026-05-05 16:56 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
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
Attachments:
[text/x-diff] clarify-S_LOCK-vs-TAS.patch (1.3K, ../../532705.1778000169@sss.pgh.pa.us/2-clarify-S_LOCK-vs-TAS.patch)
download | inline diff:
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 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. Please 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 */
=
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-05 17:57 Nathan Bossart <nathandbossart@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2026-05-05 17:57 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
On Tue, May 05, 2026 at 12:56:09PM -0400, Tom Lane wrote:
> 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.
That makes sense to me.
> 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".)
I think it does help, thanks. I'll give it a whirl.
--
nathan
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-07 20:41 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2026-05-07 20:41 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
On Tue, May 05, 2026 at 12:57:10PM -0500, Nathan Bossart wrote:
> On Tue, May 05, 2026 at 12:56:09PM -0400, Tom Lane wrote:
>> 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".)
>
> 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 a04b7aa50afe792615e5d41abb8238b14557062a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 May 2026 16:04:04 -0500
Subject: [PATCH v3 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/include/storage/s_lock.h | 7 -------
1 file changed, 7 deletions(-)
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)
From f21928086155631299334d7dbbf453ddd3da65e8 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 May 2026 16:24:30 -0500
Subject: [PATCH v3 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)
From 41c899ab334d156f296343f53123152d3228a810 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 7 May 2026 15:32:50 -0500
Subject: [PATCH v3 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)
Attachments:
[text/plain] v3-0001-Remove-fallback-declaration-for-tas.patch (1.0K, ../../afz5FHWr6rOB7bsL@nathan/2-v3-0001-Remove-fallback-declaration-for-tas.patch)
download | inline diff:
From a04b7aa50afe792615e5d41abb8238b14557062a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 May 2026 16:04:04 -0500
Subject: [PATCH v3 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/include/storage/s_lock.h | 7 -------
1 file changed, 7 deletions(-)
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)
[text/plain] v3-0002-Remove-HAS_TEST_AND_SET.patch (3.2K, ../../afz5FHWr6rOB7bsL@nathan/3-v3-0002-Remove-HAS_TEST_AND_SET.patch)
download | inline diff:
From f21928086155631299334d7dbbf453ddd3da65e8 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 4 May 2026 16:24:30 -0500
Subject: [PATCH v3 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)
[text/plain] v3-0003-Better-express-platform-requirements-in-s_lock.h.patch (4.4K, ../../afz5FHWr6rOB7bsL@nathan/4-v3-0003-Better-express-platform-requirements-in-s_lock.h.patch)
download | inline diff:
From 41c899ab334d156f296343f53123152d3228a810 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 7 May 2026 15:32:50 -0500
Subject: [PATCH v3 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)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-07 21:03 Nathan Bossart <nathandbossart@gmail.com>
parent: Kirill Reshke <reshkekirill@gmail.com>
0 siblings, 0 replies; 14+ messages in thread
From: Nathan Bossart @ 2026-05-07 21:03 UTC (permalink / raw)
To: Kirill Reshke <reshkekirill@gmail.com>; +Cc: pgsql-hackers
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
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)
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)
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)
Attachments:
[text/plain] v4-0001-Remove-fallback-declaration-for-tas.patch (2.2K, ../../afz-Da9NjHy-ubES@nathan/2-v4-0001-Remove-fallback-declaration-for-tas.patch)
download | inline diff:
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)
[text/plain] v4-0002-Remove-HAS_TEST_AND_SET.patch (3.2K, ../../afz-Da9NjHy-ubES@nathan/3-v4-0002-Remove-HAS_TEST_AND_SET.patch)
download | inline diff:
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)
[text/plain] v4-0003-Better-express-platform-requirements-in-s_lock.h.patch (4.4K, ../../afz-Da9NjHy-ubES@nathan/4-v4-0003-Better-express-platform-requirements-in-s_lock.h.patch)
download | inline diff:
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)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-07 21:12 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2026-05-07 21:12 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
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)
> #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 */
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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-05-07 21:28 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2026-05-07 21:28 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
On Thu, May 07, 2026 at 04:12:09PM -0500, Nathan Bossart wrote:
> 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)
>> #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 */
>
> 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.
Should be fixed in v5, sorry for the noise.
--
nathan
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 v5 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)
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 v5 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)
From 572c8fae484955bcbcecc97d63536ce5c9f5ee10 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 7 May 2026 15:32:50 -0500
Subject: [PATCH v5 3/3] Better express platform requirements in s_lock.h.
---
src/backend/storage/lmgr/s_lock.c | 2 ++
src/include/storage/s_lock.h | 59 +++++++++++++++++--------------
2 files changed, 35 insertions(+), 26 deletions(-)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 6df568eccb3..34c6de66773 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -91,6 +91,7 @@ s_lock_stuck(const char *file, int line, const char *func)
#endif
}
+#ifdef USE_DEFAULT_S_LOCK
/*
* s_lock(lock) - platform-independent portion of waiting for a spinlock.
*/
@@ -110,6 +111,7 @@ s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
return delayStatus.delays;
}
+#endif
#ifdef USE_DEFAULT_S_UNLOCK
void
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index fb872edd2f0..65f00c06518 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().
+ * 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,27 @@ 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 USE_DEFAULT_S_LOCK
+extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
#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,15 +687,22 @@ extern void s_unlock(volatile slock_t *lock);
#define SPIN_DELAY() ((void) 0)
#endif /* SPIN_DELAY */
-#if !defined(TAS_SPIN)
+/*
+ * We can only define TAS_SPIN if TAS was defined. Otherwise, the platform
+ * defined its own S_LOCK without TAS. Since TAS_SPIN is only used by the
+ * default S_LOCK's helper function, there's no need to define TAS_SPIN at all
+ * in that case, unless you plan to use it in a platform-specific S_LOCK helper
+ * function. (Note that we currently do not have any platforms that don't
+ * define TAS.)
+ */
+#if !defined(TAS_SPIN) && defined(TAS)
#define TAS_SPIN(lock) TAS(lock)
-#endif /* TAS_SPIN */
+#endif /* ! TAS_SPIN && TAS */
/*
* Platform-independent out-of-line support routines
*/
-extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
/* Support for dynamic adjustment of spins_per_delay */
#define DEFAULT_SPINS_PER_DELAY 100
--
2.50.1 (Apple Git-155)
Attachments:
[text/plain] v5-0001-Remove-fallback-declaration-for-tas.patch (2.2K, ../../af0D6fhLfVmG2fr7@nathan/2-v5-0001-Remove-fallback-declaration-for-tas.patch)
download | inline diff:
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 v5 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)
[text/plain] v5-0002-Remove-HAS_TEST_AND_SET.patch (3.2K, ../../af0D6fhLfVmG2fr7@nathan/3-v5-0002-Remove-HAS_TEST_AND_SET.patch)
download | inline diff:
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 v5 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)
[text/plain] v5-0003-Better-express-platform-requirements-in-s_lock.h.patch (5.3K, ../../af0D6fhLfVmG2fr7@nathan/4-v5-0003-Better-express-platform-requirements-in-s_lock.h.patch)
download | inline diff:
From 572c8fae484955bcbcecc97d63536ce5c9f5ee10 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 7 May 2026 15:32:50 -0500
Subject: [PATCH v5 3/3] Better express platform requirements in s_lock.h.
---
src/backend/storage/lmgr/s_lock.c | 2 ++
src/include/storage/s_lock.h | 59 +++++++++++++++++--------------
2 files changed, 35 insertions(+), 26 deletions(-)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 6df568eccb3..34c6de66773 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -91,6 +91,7 @@ s_lock_stuck(const char *file, int line, const char *func)
#endif
}
+#ifdef USE_DEFAULT_S_LOCK
/*
* s_lock(lock) - platform-independent portion of waiting for a spinlock.
*/
@@ -110,6 +111,7 @@ s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
return delayStatus.delays;
}
+#endif
#ifdef USE_DEFAULT_S_UNLOCK
void
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index fb872edd2f0..65f00c06518 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().
+ * 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,27 @@ 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 USE_DEFAULT_S_LOCK
+extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
#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,15 +687,22 @@ extern void s_unlock(volatile slock_t *lock);
#define SPIN_DELAY() ((void) 0)
#endif /* SPIN_DELAY */
-#if !defined(TAS_SPIN)
+/*
+ * We can only define TAS_SPIN if TAS was defined. Otherwise, the platform
+ * defined its own S_LOCK without TAS. Since TAS_SPIN is only used by the
+ * default S_LOCK's helper function, there's no need to define TAS_SPIN at all
+ * in that case, unless you plan to use it in a platform-specific S_LOCK helper
+ * function. (Note that we currently do not have any platforms that don't
+ * define TAS.)
+ */
+#if !defined(TAS_SPIN) && defined(TAS)
#define TAS_SPIN(lock) TAS(lock)
-#endif /* TAS_SPIN */
+#endif /* ! TAS_SPIN && TAS */
/*
* Platform-independent out-of-line support routines
*/
-extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
/* Support for dynamic adjustment of spins_per_delay */
#define DEFAULT_SPINS_PER_DELAY 100
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-09-25 21:03 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: Nathan Bossart @ 2026-09-25 21:03 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
I've committed v5-{0001,0002} and attached a rebased version of 0003.
Besides fixing some small things discovered during LLM review, I decided to
remove the restriction that only one of TAS or S_LOCK can be defined.
Having both doesn't break anything; we just use the platform's S_LOCK in
that case.
--
nathan
From fe366a26ea50a50f0e810a8dd51069285ceefcf9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 25 Sep 2026 16:01:13 -0500
Subject: [PATCH v6 1/1] Better express platform requirements in s_lock.h.
Presently, s_lock.h insists that every platform provide TAS(), and
its header comment doesn't say which of the macros a new platform
actually needs to supply. The real contract is that a platform
must provide either S_LOCK() or a TAS() for the default S_LOCK() to
be built on.
To better express that, this commit moves the "no spinlock support"
error to where the default S_LOCK() is defined, compiles s_lock()
only when the default S_LOCK() is in use, and provides a default
TAS_SPIN() only when there is a TAS() to base it on. The direct
callers of s_lock() in the spinlock tests are adjusted to match.
The header comment is reworked accordingly, dropping the note about
pre-9.5 volatile requirements and the caution about TAS()
spuriously failing along the way. The latter was added by commit
7f60b81e1a for a long-unsupported platform, and it only matters to
callers outside this file, which commit 499abb0c0f disallowed.
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/afkUeI7UhacZ5ZFm%40nathan
---
src/backend/storage/lmgr/s_lock.c | 4 ++-
src/include/storage/s_lock.h | 46 +++++++++++++++----------------
src/test/regress/regress.c | 2 +-
3 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 6df568eccb3..795aeed1028 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -91,6 +91,7 @@ s_lock_stuck(const char *file, int line, const char *func)
#endif
}
+#ifdef USE_DEFAULT_S_LOCK
/*
* s_lock(lock) - platform-independent portion of waiting for a spinlock.
*/
@@ -110,6 +111,7 @@ s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
return delayStatus.delays;
}
+#endif
#ifdef USE_DEFAULT_S_UNLOCK
void
@@ -291,7 +293,7 @@ main()
printf(" if S_LOCK() and TAS() are working.\n");
fflush(stdout);
- s_lock(&test_lock.lock, __FILE__, __LINE__, __func__);
+ S_LOCK(&test_lock.lock);
printf("S_LOCK_TEST: failed, lock not locked\n");
return 1;
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 17edc058b74..7e049dfac18 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -44,23 +44,14 @@
* 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. A platform must provide either S_LOCK() or a TAS() for the
+ * default S_LOCK() to be built on. Currently, all supported platforms do
+ * the latter, so that is probably the best 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 +63,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,19 +633,23 @@ 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.
*/
+/*
+ * Make sure S_LOCK is defined, either explicitly for the platform or via a TAS
+ * macro for the platform.
+ */
#if !defined(S_LOCK)
+#ifdef TAS
+#define USE_DEFAULT_S_LOCK
+extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
#define S_LOCK(lock) \
(TAS(lock) ? s_lock((lock), __FILE__, __LINE__, __func__) : 0)
+#else
+#error PostgreSQL does not have spinlock support on this platform. Please report this to pgsql-bugs@lists.postgresql.org.
+#endif /* TAS */
#endif /* S_LOCK */
#if !defined(S_UNLOCK)
@@ -687,15 +682,18 @@ extern void s_unlock(volatile slock_t *lock);
#define SPIN_DELAY() ((void) 0)
#endif /* SPIN_DELAY */
-#if !defined(TAS_SPIN)
+/*
+ * TAS_SPIN is only needed by the default S_LOCK's helper function (s_lock()),
+ * so we only provide a default when there is a TAS to base it on.
+ */
+#if !defined(TAS_SPIN) && defined(TAS)
#define TAS_SPIN(lock) TAS(lock)
-#endif /* TAS_SPIN */
+#endif /* ! TAS_SPIN && TAS */
/*
* Platform-independent out-of-line support routines
*/
-extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
/* Support for dynamic adjustment of spins_per_delay */
#define DEFAULT_SPINS_PER_DELAY 100
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index c72ee31cdce..3deb4ed7203 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -675,7 +675,7 @@ test_spinlock(void)
S_UNLOCK(&struct_w_lock.lock);
/* and that "contended" acquisition works */
- s_lock(&struct_w_lock.lock, "testfile", 17, "testfunc");
+ S_LOCK(&struct_w_lock.lock);
S_UNLOCK(&struct_w_lock.lock);
/*
--
2.55.0
Attachments:
[text/plain] v6-0001-Better-express-platform-requirements-in-s_lock.h.patch (6.4K, ../../arbhohGoRM2Phzm4@nathan/2-v6-0001-Better-express-platform-requirements-in-s_lock.h.patch)
download | inline diff:
From fe366a26ea50a50f0e810a8dd51069285ceefcf9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 25 Sep 2026 16:01:13 -0500
Subject: [PATCH v6 1/1] Better express platform requirements in s_lock.h.
Presently, s_lock.h insists that every platform provide TAS(), and
its header comment doesn't say which of the macros a new platform
actually needs to supply. The real contract is that a platform
must provide either S_LOCK() or a TAS() for the default S_LOCK() to
be built on.
To better express that, this commit moves the "no spinlock support"
error to where the default S_LOCK() is defined, compiles s_lock()
only when the default S_LOCK() is in use, and provides a default
TAS_SPIN() only when there is a TAS() to base it on. The direct
callers of s_lock() in the spinlock tests are adjusted to match.
The header comment is reworked accordingly, dropping the note about
pre-9.5 volatile requirements and the caution about TAS()
spuriously failing along the way. The latter was added by commit
7f60b81e1a for a long-unsupported platform, and it only matters to
callers outside this file, which commit 499abb0c0f disallowed.
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/afkUeI7UhacZ5ZFm%40nathan
---
src/backend/storage/lmgr/s_lock.c | 4 ++-
src/include/storage/s_lock.h | 46 +++++++++++++++----------------
src/test/regress/regress.c | 2 +-
3 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 6df568eccb3..795aeed1028 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -91,6 +91,7 @@ s_lock_stuck(const char *file, int line, const char *func)
#endif
}
+#ifdef USE_DEFAULT_S_LOCK
/*
* s_lock(lock) - platform-independent portion of waiting for a spinlock.
*/
@@ -110,6 +111,7 @@ s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
return delayStatus.delays;
}
+#endif
#ifdef USE_DEFAULT_S_UNLOCK
void
@@ -291,7 +293,7 @@ main()
printf(" if S_LOCK() and TAS() are working.\n");
fflush(stdout);
- s_lock(&test_lock.lock, __FILE__, __LINE__, __func__);
+ S_LOCK(&test_lock.lock);
printf("S_LOCK_TEST: failed, lock not locked\n");
return 1;
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 17edc058b74..7e049dfac18 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -44,23 +44,14 @@
* 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. A platform must provide either S_LOCK() or a TAS() for the
+ * default S_LOCK() to be built on. Currently, all supported platforms do
+ * the latter, so that is probably the best 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 +63,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,19 +633,23 @@ 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.
*/
+/*
+ * Make sure S_LOCK is defined, either explicitly for the platform or via a TAS
+ * macro for the platform.
+ */
#if !defined(S_LOCK)
+#ifdef TAS
+#define USE_DEFAULT_S_LOCK
+extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
#define S_LOCK(lock) \
(TAS(lock) ? s_lock((lock), __FILE__, __LINE__, __func__) : 0)
+#else
+#error PostgreSQL does not have spinlock support on this platform. Please report this to pgsql-bugs@lists.postgresql.org.
+#endif /* TAS */
#endif /* S_LOCK */
#if !defined(S_UNLOCK)
@@ -687,15 +682,18 @@ extern void s_unlock(volatile slock_t *lock);
#define SPIN_DELAY() ((void) 0)
#endif /* SPIN_DELAY */
-#if !defined(TAS_SPIN)
+/*
+ * TAS_SPIN is only needed by the default S_LOCK's helper function (s_lock()),
+ * so we only provide a default when there is a TAS to base it on.
+ */
+#if !defined(TAS_SPIN) && defined(TAS)
#define TAS_SPIN(lock) TAS(lock)
-#endif /* TAS_SPIN */
+#endif /* ! TAS_SPIN && TAS */
/*
* Platform-independent out-of-line support routines
*/
-extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
/* Support for dynamic adjustment of spins_per_delay */
#define DEFAULT_SPINS_PER_DELAY 100
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index c72ee31cdce..3deb4ed7203 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -675,7 +675,7 @@ test_spinlock(void)
S_UNLOCK(&struct_w_lock.lock);
/* and that "contended" acquisition works */
- s_lock(&struct_w_lock.lock, "testfile", 17, "testfunc");
+ S_LOCK(&struct_w_lock.lock);
S_UNLOCK(&struct_w_lock.lock);
/*
--
2.55.0
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: small cleanup for s_lock.h
@ 2026-09-28 18:54 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 0 replies; 14+ messages in thread
From: Nathan Bossart @ 2026-09-28 18:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
On Fri, Sep 25, 2026 at 04:03:30PM -0500, Nathan Bossart wrote:
> I've committed v5-{0001,0002} and attached a rebased version of 0003.
> Besides fixing some small things discovered during LLM review, I decided to
> remove the restriction that only one of TAS or S_LOCK can be defined.
> Having both doesn't break anything; we just use the platform's S_LOCK in
> that case.
I've now committed 0003.
--
nathan
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2026-09-28 18:54 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-05-04 21:49 small cleanup for s_lock.h Nathan Bossart <nathandbossart@gmail.com>
2026-05-04 22:11 ` Tristan Partin <tristan@partin.io>
2026-05-04 22:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-05-05 15:49 ` Nathan Bossart <nathandbossart@gmail.com>
2026-05-05 16:08 ` Nathan Bossart <nathandbossart@gmail.com>
2026-05-05 16:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-05-05 17:57 ` Nathan Bossart <nathandbossart@gmail.com>
2026-05-07 20:41 ` Nathan Bossart <nathandbossart@gmail.com>
2026-05-07 21:12 ` Nathan Bossart <nathandbossart@gmail.com>
2026-05-07 21:28 ` Nathan Bossart <nathandbossart@gmail.com>
2026-09-25 21:03 ` Nathan Bossart <nathandbossart@gmail.com>
2026-09-28 18:54 ` Nathan Bossart <nathandbossart@gmail.com>
2026-05-04 22:20 ` Kirill Reshke <reshkekirill@gmail.com>
2026-05-07 21:03 ` Nathan Bossart <nathandbossart@gmail.com>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox