Message-ID: <adab98a3-a0e3-4455-9b63-ae6f1ecd18c9@iki.fi>
Date: Thu, 26 Mar 2026 17:21:58 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Clean up NamedLWLockTranche stuff
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <47aaf57e-1b7b-4e12-bda2-0316081ff50e@iki.fi>
 <acVEqGemyK-Yjswa@nathan>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <acVEqGemyK-Yjswa@nathan>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/adab98a3-a0e3-4455-9b63-ae6f1ecd18c9%40iki.fi>
Precedence: bulk

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