make LWLockCounter a global variable
In lwlock.c, uses of LWLockCounter must first calculate its address in
shared memory with something like this:
LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
This appears to have been started by commit 82e861f in order to fix
EXEC_BACKEND builds, but it could also be fixed by adding it to the
BackendParameters struct. I find the current approach somewhat difficult
to read and understand, so I'd like to switch to the latter approach. This
is admittedly just nitpicking...
--
nathan
Attachments:
v1-0001-Make-LWLockCounter-a-global-variable.patchtext/plain; charset=us-asciiDownload
From d4b1847f7e7d1ef4b32b4a5304b2a55cabe55f56 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 28 Aug 2025 16:22:02 -0500
Subject: [PATCH v1 1/1] Make LWLockCounter a global variable.
---
src/backend/postmaster/launch_backend.c | 3 +++
src/backend/storage/lmgr/lwlock.c | 15 ++++-----------
src/include/storage/lwlock.h | 1 +
3 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/src/backend/postmaster/launch_backend.c b/src/backend/postmaster/launch_backend.c
index bf6b55ee830..cd9547b03a3 100644
--- a/src/backend/postmaster/launch_backend.c
+++ b/src/backend/postmaster/launch_backend.c
@@ -102,6 +102,7 @@ typedef struct
#endif
int NamedLWLockTrancheRequests;
NamedLWLockTranche *NamedLWLockTrancheArray;
+ int *LWLockCounter;
LWLockPadded *MainLWLockArray;
slock_t *ProcStructLock;
PROC_HDR *ProcGlobal;
@@ -761,6 +762,7 @@ save_backend_variables(BackendParameters *param,
param->NamedLWLockTrancheRequests = NamedLWLockTrancheRequests;
param->NamedLWLockTrancheArray = NamedLWLockTrancheArray;
+ param->LWLockCounter = LWLockCounter;
param->MainLWLockArray = MainLWLockArray;
param->ProcStructLock = ProcStructLock;
param->ProcGlobal = ProcGlobal;
@@ -1021,6 +1023,7 @@ restore_backend_variables(BackendParameters *param)
NamedLWLockTrancheRequests = param->NamedLWLockTrancheRequests;
NamedLWLockTrancheArray = param->NamedLWLockTrancheArray;
+ LWLockCounter = param->LWLockCounter;
MainLWLockArray = param->MainLWLockArray;
ProcStructLock = param->ProcStructLock;
ProcGlobal = param->ProcGlobal;
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index c80b43f1f55..09401515d3a 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -196,6 +196,7 @@ int NamedLWLockTrancheRequests = 0;
/* points to data in shared memory: */
NamedLWLockTranche *NamedLWLockTrancheArray = NULL;
+int *LWLockCounter = NULL;
static void InitializeLWLocks(void);
static inline void LWLockReportWaitStart(LWLock *lock);
@@ -423,13 +424,14 @@ CreateLWLocks(void)
if (!IsUnderPostmaster)
{
Size spaceLocks = LWLockShmemSize();
- int *LWLockCounter;
char *ptr;
/* Allocate space */
ptr = (char *) ShmemAlloc(spaceLocks);
- /* Leave room for dynamic allocation of tranches */
+ /* Initialize the dynamic-allocation counter for tranches */
+ LWLockCounter = (int *) ptr;
+ *LWLockCounter = LWTRANCHE_FIRST_USER_DEFINED;
ptr += sizeof(int);
/* Ensure desired alignment of LWLock array */
@@ -437,13 +439,6 @@ CreateLWLocks(void)
MainLWLockArray = (LWLockPadded *) ptr;
- /*
- * Initialize the dynamic-allocation counter for tranches, which is
- * stored just before the first LWLock.
- */
- LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
- *LWLockCounter = LWTRANCHE_FIRST_USER_DEFINED;
-
/* Initialize all LWLocks */
InitializeLWLocks();
}
@@ -574,9 +569,7 @@ int
LWLockNewTrancheId(void)
{
int result;
- int *LWLockCounter;
- LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
/* We use the ShmemLock spinlock to protect LWLockCounter */
SpinLockAcquire(ShmemLock);
result = (*LWLockCounter)++;
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 5e717765764..f9cf57f8d26 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -82,6 +82,7 @@ typedef struct NamedLWLockTranche
extern PGDLLIMPORT NamedLWLockTranche *NamedLWLockTrancheArray;
extern PGDLLIMPORT int NamedLWLockTrancheRequests;
+extern PGDLLIMPORT int *LWLockCounter;
/*
* It's a bit odd to declare NUM_BUFFER_PARTITIONS and NUM_LOCK_PARTITIONS
--
2.39.5 (Apple Git-154)
Nathan Bossart <nathandbossart@gmail.com> writes:
In lwlock.c, uses of LWLockCounter must first calculate its address in
shared memory with something like this:
LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
This appears to have been started by commit 82e861f in order to fix
EXEC_BACKEND builds, but it could also be fixed by adding it to the
BackendParameters struct. I find the current approach somewhat difficult
to read and understand, so I'd like to switch to the latter approach. This
is admittedly just nitpicking...
No objection here. As a small improvement, perhaps you could swap
around the code in LWLockShmemSize so that the order in which it
considers size contributions matches the physical layout, more
or less like
/* Calculate total number of locks needed in the main array. */
numLocks += NumLWLocksForNamedTranches();
+ /* Space for dynamic allocation counter, plus room for alignment. */
+ size = sizeof(int) + LWLOCK_PADDED_SIZE;
+
/* Space for the LWLock array. */
- size = mul_size(numLocks, sizeof(LWLockPadded));
+ size = add_size(size, mul_size(numLocks, sizeof(LWLockPadded)));- /* Space for dynamic allocation counter, plus room for alignment. */
- size = add_size(size, sizeof(int) + LWLOCK_PADDED_SIZE);
-
/* space for named tranches. */
size = add_size(size, mul_size(NamedLWLockTrancheRequests, sizeof(NamedLWLockTranche)));
I find it a little confusing that that code doesn't line up
exactly with what CreateLWLocks does.
regards, tom lane
Hi,
On Thu, Aug 28, 2025 at 05:56:07PM -0400, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
In lwlock.c, uses of LWLockCounter must first calculate its address in
shared memory with something like this:LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
This appears to have been started by commit 82e861f in order to fix
EXEC_BACKEND builds, but it could also be fixed by adding it to the
BackendParameters struct. I find the current approach somewhat difficult
to read and understand, so I'd like to switch to the latter approach. This
is admittedly just nitpicking...No objection here. As a small improvement, perhaps you could swap
around the code in LWLockShmemSize so that the order in which it
considers size contributions matches the physical layout, more
or less like/* Calculate total number of locks needed in the main array. */
numLocks += NumLWLocksForNamedTranches();+ /* Space for dynamic allocation counter, plus room for alignment. */ + size = sizeof(int) + LWLOCK_PADDED_SIZE; + /* Space for the LWLock array. */ - size = mul_size(numLocks, sizeof(LWLockPadded)); + size = add_size(size, mul_size(numLocks, sizeof(LWLockPadded)));- /* Space for dynamic allocation counter, plus room for alignment. */
- size = add_size(size, sizeof(int) + LWLOCK_PADDED_SIZE);
-
/* space for named tranches. */
size = add_size(size, mul_size(NamedLWLockTrancheRequests, sizeof(NamedLWLockTranche)));I find it a little confusing that that code doesn't line up
exactly with what CreateLWLocks does.
+1.
Another option could be to not change CreateLWLocks() at all, except removing the
local variable:
@@ -423,7 +424,6 @@ CreateLWLocks(void)
if (!IsUnderPostmaster)
{
Size spaceLocks = LWLockShmemSize();
- int *LWLockCounter;to use the global variable. That way we preserve the current memory layout and
there is no need to change LWLockShmemSize().
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Fri, Aug 29, 2025 at 06:52:48AM +0000, Bertrand Drouvot wrote:
On Thu, Aug 28, 2025 at 05:56:07PM -0400, Tom Lane wrote:
No objection here. As a small improvement, perhaps you could swap
around the code in LWLockShmemSize so that the order in which it
considers size contributions matches the physical layout, more
or less like[...]
I find it a little confusing that that code doesn't line up
exactly with what CreateLWLocks does.+1.
Good idea. Here's a new version of the patch. If CI is happy with it,
I'll go ahead and commit it.
Another option could be to not change CreateLWLocks() at all, except removing the
local variable:@@ -423,7 +424,6 @@ CreateLWLocks(void) if (!IsUnderPostmaster) { Size spaceLocks = LWLockShmemSize(); - int *LWLockCounter;to use the global variable. That way we preserve the current memory layout and
there is no need to change LWLockShmemSize().
That would make the patch smaller, but IMHO it kind-of defeats the purpose,
which is to make this stuff simpler and easier to follow.
--
nathan
Attachments:
v2-0001-Make-LWLockCounter-a-global-variable.patchtext/plain; charset=us-asciiDownload
From babaa2bfedd24932b6df13bfea0f4b01e911a311 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Fri, 29 Aug 2025 09:33:55 -0500
Subject: [PATCH v2 1/1] Make LWLockCounter a global variable.
In lwlock.c, using the LWLockCounter requires first calculating its
address in shared memory like this:
LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
Commit 82e861fbe1 started this trend in order to fix EXEC_BACKEND
builds, but it could also be fixed by adding it to the
BackendParameters struct. The current approach is somewhat
difficult to follow, so this commit switches to the latter
approach. While at it, swap around the code in LWLockShmemSize()
to match the order of the assignments in CreateLWLocks() for added
readability.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/aLDLnan9gNCS9fHx%40nathan
---
src/backend/postmaster/launch_backend.c | 3 +++
src/backend/storage/lmgr/lwlock.c | 24 ++++++++----------------
src/include/storage/lwlock.h | 1 +
3 files changed, 12 insertions(+), 16 deletions(-)
diff --git a/src/backend/postmaster/launch_backend.c b/src/backend/postmaster/launch_backend.c
index bf6b55ee830..cd9547b03a3 100644
--- a/src/backend/postmaster/launch_backend.c
+++ b/src/backend/postmaster/launch_backend.c
@@ -102,6 +102,7 @@ typedef struct
#endif
int NamedLWLockTrancheRequests;
NamedLWLockTranche *NamedLWLockTrancheArray;
+ int *LWLockCounter;
LWLockPadded *MainLWLockArray;
slock_t *ProcStructLock;
PROC_HDR *ProcGlobal;
@@ -761,6 +762,7 @@ save_backend_variables(BackendParameters *param,
param->NamedLWLockTrancheRequests = NamedLWLockTrancheRequests;
param->NamedLWLockTrancheArray = NamedLWLockTrancheArray;
+ param->LWLockCounter = LWLockCounter;
param->MainLWLockArray = MainLWLockArray;
param->ProcStructLock = ProcStructLock;
param->ProcGlobal = ProcGlobal;
@@ -1021,6 +1023,7 @@ restore_backend_variables(BackendParameters *param)
NamedLWLockTrancheRequests = param->NamedLWLockTrancheRequests;
NamedLWLockTrancheArray = param->NamedLWLockTrancheArray;
+ LWLockCounter = param->LWLockCounter;
MainLWLockArray = param->MainLWLockArray;
ProcStructLock = param->ProcStructLock;
ProcGlobal = param->ProcGlobal;
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index c80b43f1f55..a4aecd1fbc3 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -196,6 +196,7 @@ int NamedLWLockTrancheRequests = 0;
/* points to data in shared memory: */
NamedLWLockTranche *NamedLWLockTrancheArray = NULL;
+int *LWLockCounter = NULL;
static void InitializeLWLocks(void);
static inline void LWLockReportWaitStart(LWLock *lock);
@@ -397,11 +398,11 @@ LWLockShmemSize(void)
/* Calculate total number of locks needed in the main array. */
numLocks += NumLWLocksForNamedTranches();
- /* Space for the LWLock array. */
- size = mul_size(numLocks, sizeof(LWLockPadded));
-
/* Space for dynamic allocation counter, plus room for alignment. */
- size = add_size(size, sizeof(int) + LWLOCK_PADDED_SIZE);
+ size = sizeof(int) + LWLOCK_PADDED_SIZE;
+
+ /* Space for the LWLock array. */
+ size = add_size(size, mul_size(numLocks, sizeof(LWLockPadded)));
/* space for named tranches. */
size = add_size(size, mul_size(NamedLWLockTrancheRequests, sizeof(NamedLWLockTranche)));
@@ -423,27 +424,20 @@ CreateLWLocks(void)
if (!IsUnderPostmaster)
{
Size spaceLocks = LWLockShmemSize();
- int *LWLockCounter;
char *ptr;
/* Allocate space */
ptr = (char *) ShmemAlloc(spaceLocks);
- /* Leave room for dynamic allocation of tranches */
+ /* Initialize the dynamic-allocation counter for tranches */
+ LWLockCounter = (int *) ptr;
+ *LWLockCounter = LWTRANCHE_FIRST_USER_DEFINED;
ptr += sizeof(int);
/* Ensure desired alignment of LWLock array */
ptr += LWLOCK_PADDED_SIZE - ((uintptr_t) ptr) % LWLOCK_PADDED_SIZE;
-
MainLWLockArray = (LWLockPadded *) ptr;
- /*
- * Initialize the dynamic-allocation counter for tranches, which is
- * stored just before the first LWLock.
- */
- LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
- *LWLockCounter = LWTRANCHE_FIRST_USER_DEFINED;
-
/* Initialize all LWLocks */
InitializeLWLocks();
}
@@ -574,9 +568,7 @@ int
LWLockNewTrancheId(void)
{
int result;
- int *LWLockCounter;
- LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
/* We use the ShmemLock spinlock to protect LWLockCounter */
SpinLockAcquire(ShmemLock);
result = (*LWLockCounter)++;
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 5e717765764..f9cf57f8d26 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -82,6 +82,7 @@ typedef struct NamedLWLockTranche
extern PGDLLIMPORT NamedLWLockTranche *NamedLWLockTrancheArray;
extern PGDLLIMPORT int NamedLWLockTrancheRequests;
+extern PGDLLIMPORT int *LWLockCounter;
/*
* It's a bit odd to declare NUM_BUFFER_PARTITIONS and NUM_LOCK_PARTITIONS
--
2.39.5 (Apple Git-154)