Date: Thu, 26 Mar 2026 09:37:28 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Clean up NamedLWLockTranche stuff
Message-ID: <acVEqGemyK-Yjswa@nathan>
References: <47aaf57e-1b7b-4e12-bda2-0316081ff50e@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <47aaf57e-1b7b-4e12-bda2-0316081ff50e@iki.fi>
Archived-At: <https://www.postgresql.org/message-id/acVEqGemyK-Yjswa%40nathan>
Precedence: bulk

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