Thread
Commits
-
Make LWLockCounter a global variable.
- 67fcf48c3bb3 19 (unreleased) landed
-
Fix LWLockAssign() so that it can safely be executed after postmaster
- 82e861fbe1a1 8.1.0 cited
-
make LWLockCounter a global variable
Nathan Bossart <nathandbossart@gmail.com> — 2025-08-28T21:35:25Z
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
-
Re: make LWLockCounter a global variable
Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-28T21:56:07Z
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
-
Re: make LWLockCounter a global variable
Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-08-29T06:52:48Z
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 -
Re: make LWLockCounter a global variable
Nathan Bossart <nathandbossart@gmail.com> — 2025-08-29T14:43:26Z
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 -
Re: make LWLockCounter a global variable
Nathan Bossart <nathandbossart@gmail.com> — 2025-08-29T17:15:28Z
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