On 26/03/2026 16:37, Nathan Bossart wrote:
On Thu, Mar 26, 2026 at 02:16:52PM +0200, Heikki Linnakangas wrote:
0002:
+ foreach(lc, NamedLWLockTrancheRequests)
nitpick: These foreach loops seem like good opportunities to use
foreach_ptr.
The comment atop NumLWLocksForNamedTranches might benefit from mentioning
RequestNamedLWLockTranche() and the fact that it only works in the
postmaster. Perhaps an assertion is warranted, too.
There's already this check in RequestNamedLWLockTranche():
if (!process_shmem_requests_in_progress)
elog(FATAL, "cannot request additional LWLocks outside
shmem_request_hook");
shmem_request_hooks are only called early at postmaster startup.
+ SpinLockAcquire(ShmemLock);
+ LocalNumUserDefinedTranches = LWLockTranches->num_user_defined;
+ SpinLockRelease(ShmemLock);
Not critical, but it might be worth making num_user_defined an atomic.
Yeah I considered that. The lock is still needed in
LWLockNewTrancheId(), though, to prevent two concurrent
LWLockNewTrancheId() calls from running concurrently. Using an atomic
would allow the extra optimization of reading the value without
acquiring spinlock, but it seems more clear to have a clear-cut rule
that you must always hold the spinlock whenever accessing the field.
0004:
+++ b/src/backend/storage/ipc/shmem.c
@@ -379,7 +379,8 @@ ShmemInitStruct(const char *name, Size size, bool *foundPtr)
Assert(ShmemIndex != NULL);
- LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE);
+ if (IsUnderPostmaster)
+ LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE);
Am I understanding that we assume ShmemInitStruct() is only called by the
postmaster when there are no other backends yet?
Yeah. LWLockAcquire has this:
/*
* We can't wait if we haven't got a PGPROC. This should only occur
* during bootstrap or shared memory initialization. Put an Assert
here
* to catch unsafe coding practices.
*/
Assert(!(proc == NULL && IsUnderPostmaster));
To be honest I didn't realize we tolerate that, calling LWLockAcquire in
postmaster, until I started to work on this. It might be worth having
some extra sanity checks here, to e.g. to throw an error if
LWLockAcquire is called from postmaster after startup. But this isn't new.
0005:
- if (IsUnderPostmaster)
- LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE);
+ LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE);
Oh, this reverts many of these changes from 0004. Maybe the patches could
be reordered to avoid this?
Makes sense.
Thanks for the review!
- Heikki
