On Thu, Mar 26, 2026 at 02:16:52PM +0200, Heikki Linnakangas wrote:
> At postmaster startup, NamedLWLockTrancheRequests points to a
> backend-private array. But after startup, and always in backends, it points
> to a copy in shared memory and LocalNamedLWLockTrancheRequestArray is used
> to hold the original. It took me a while to realize that
> NamedLWLockTrancheRequests in shared memory is *not* updated when you call
> LWLockNewTrancheId(), it only holds the requests made with
> RequestNamedLWLockTranche() before startup.
Right. LocalNamedLWLockTrancheRequestArray is needed so that we can
re-initialize shared memory after a crash. See commit c3cc2ab87d.
> I propose the attached refactorings to make this less confusing. See commit
> messages for details.
Thanks for doing this, Heikki. I agree that we ought to make this stuff
cleaner. I've asked Sami Imseih, who worked on LWLocks with me last year,
to look at this patch set, too.
> Subject: [PATCH v1 1/5] Rename MAX_NAMED_TRANCHES to MAX_USER_DEFINED_TRANCHES
Seems fine to me.
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.
+ SpinLockAcquire(ShmemLock);
+ LocalNumUserDefinedTranches = LWLockTranches->num_user_defined;
+ SpinLockRelease(ShmemLock);
Not critical, but it might be worth making num_user_defined an atomic.
Overall, 0002 looks reasonable to me upon a first read-through.
> Subject: [PATCH v1 3/5] Use a separate spinlock to protect LWLockTranches
Seems fine to me.
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?
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?
--
nathan
