public inbox for [email protected]  

help / color / mirror / Atom feed

From: Heikki Linnakangas <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Clean up NamedLWLockTranche stuff
Date: Thu, 26 Mar 2026 17:21:58 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <acVEqGemyK-Yjswa@nathan>
References: <[email protected]>
	<acVEqGemyK-Yjswa@nathan>

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

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: <[email protected]>

* 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