Обсуждение: small cleanup for s_lock.h

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

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)

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

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

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

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

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

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 */

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