Thread: spin.c includes pg_sema.h even if unnecessary

Ouch! Sorry for the bogus subject of just-submitted mail.
I'll make another thread with the fixed subject.

====
Hello.

As in another mail just before, spin.c seems a bit strange
(without acutual harm).

spin.h doesn't include pg_sema.h when HAVE_SPINLOCKS is defined,
but spin.c always includes it even in the case. The file is
included only to use sizeof(PGSemaphore) to calcualte
SpinlockSemaSize as 0.

The codes that requires PGSempaphore is inactivated when the
symbol is defined in all other places so it seems to me that we
ought to refrain from using it there, too. The attched patch does
that.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/storage/lmgr/spin.c b/src/backend/storage/lmgr/spin.c
index 6d59a7f..353a3a9 100644
--- a/src/backend/storage/lmgr/spin.c
+++ b/src/backend/storage/lmgr/spin.c
@@ -22,13 +22,15 @@
  */
 #include "postgres.h"
 
-#include "storage/pg_sema.h"
 #include "storage/shmem.h"
 #include "storage/spin.h"
 
 
 #ifndef HAVE_SPINLOCKS
+#define RequredShmemSize (SpinlockSemas() * sizeof(PGSemaphore))
 PGSemaphore *SpinlockSemaArray;
+#else
+#define RequiredShmemSize 0
 #endif
 
 /*
@@ -38,7 +40,7 @@ PGSemaphore *SpinlockSemaArray;
 Size
 SpinlockSemaSize(void)
 {
-    return SpinlockSemas() * sizeof(PGSemaphore);
+    return RequiredShmemSize;
 }
 
 #ifdef HAVE_SPINLOCKS

On 2018-02-15 20:11:07 +0900, Kyotaro HORIGUCHI wrote:
> As in another mail just before, spin.c seems a bit strange
> (without acutual harm).
> 
> spin.h doesn't include pg_sema.h when HAVE_SPINLOCKS is defined,
> but spin.c always includes it even in the case. The file is
> included only to use sizeof(PGSemaphore) to calcualte
> SpinlockSemaSize as 0.
> 
> The codes that requires PGSempaphore is inactivated when the
> symbol is defined in all other places so it seems to me that we
> ought to refrain from using it there, too. The attched patch does

IDK, I don't quite see the point of the change here...

Greetings,

Andres Freund

At Thu, 15 Feb 2018 10:01:57 -0800, Andres Freund <andres@anarazel.de> wrote in
<20180215180157.7q55aytrhif7vvzw@alap3.anarazel.de>
> On 2018-02-15 20:11:07 +0900, Kyotaro HORIGUCHI wrote:
> > As in another mail just before, spin.c seems a bit strange
> > (without acutual harm).
> > 
> > spin.h doesn't include pg_sema.h when HAVE_SPINLOCKS is defined,
> > but spin.c always includes it even in the case. The file is
> > included only to use sizeof(PGSemaphore) to calcualte
> > SpinlockSemaSize as 0.
> > 
> > The codes that requires PGSempaphore is inactivated when the
> > symbol is defined in all other places so it seems to me that we
> > ought to refrain from using it there, too. The attched patch does
> 
> IDK, I don't quite see the point of the change here...

No actual gain, but I just feel it uneasy that utterly-unused
symbol is used to yield zero. From other point of view, it seems
to be inconsistency that a header file is disabled in a file, but
not disabled in another on one configuration.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center