Обсуждение: 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

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

On Fri, Aug 29, 2025 at 09:43:26AM -0500, Nathan Bossart wrote:
> Good idea.  Here's a new version of the patch.  If CI is happy with it,
> I'll go ahead and commit it.

Committed.

-- 
nathan