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+8-12
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