public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Clean up NamedLWLockTranche stuff
Date: Thu, 26 Mar 2026 09:37:28 -0500
Message-ID: <acVEqGemyK-Yjswa@nathan> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
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
view thread (14+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Clean up NamedLWLockTranche stuff
In-Reply-To: <acVEqGemyK-Yjswa@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox