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
