Subject: Re: Clean up NamedLWLockTranche stuff

From: Nathan Bossart <[email-masked]>
Date: Thu, 26 Mar 2026 09:37:28 -0500

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

---

From: Heikki Linnakangas <[email-masked]>
Date: Thu, 26 Mar 2026 17:21:58 +0200

> 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.

> 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.

> 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.

> be reordered to avoid this?

Makes sense.

Thanks for the review!

- Heikki

---

From: Sami Imseih <[email-masked]>
Date: Thu, 26 Mar 2026 11:34:32 -0500

Hi,

Thanks for the patches!

> I propose the attached refactorings to make this less confusing. See
> commit messages for details.

I only took a look at 0001 so far, and I do agree with this statement
in the commit message:

"The "user defined" term was already used in LWTRANCHE_FIRST_USER_DEFINED,
so let's standardize on that to mean tranches allocated with either
RequestNamedLWLockTranche() or LWLockNewTrancheId()."

I do wonder if 0001 is going far enough though.

Instead of just standardizing that "user defined" could mean tranches allocated
with RequestNamedLWLockTranche() or LWLockNewTrancheId(), how about we also
rename these APIs to reflect that as well? This way we remove all concept of
"named tranche" which is what it sounds like to me you are proposing.

rename RequestNamedLWLockTranche() to RequestUserDefinedLWLockTranche()
and LWLockNewTrancheId() to RegisterUserDefinedLWLockTranche()

RequestNamedLWLockTranche() requests the lwlock at shmem_request time,
which is later registered via LWLockNewTrancheId() when lwlocks are
initialized by the postmaster.

Also, the name LWLockNewTrancheId() is selling what this function does
too short.
It does return a new tranche ID, but it also takes in a user-defined tranche
name and copies ("registers") that name into LWLockTrancheNames.

v19 is already changing the signature of LWLockNewTrancheId(), so maybe
improving the names of these APIs makes sense to do.

--
Sami Imseih
Amazon Web Services (AWS)

---

From: Heikki Linnakangas <[email-masked]>
Date: Thu, 26 Mar 2026 18:57:51 +0200

Thanks!

> and LWLockNewTrancheId() to RegisterUserDefinedLWLockTranche()

I'd rather not change RequestNamedLWLockTranche(), because I think 
LWLockNewTrancheId() is better and should be used in new code. I 
consider RequestNamedLWLockTranche() to be a legacy function, for 
backwards compatibility.

> improving the names of these APIs makes sense to do.

Oh, I didn't realize we changed the LWLockNewTrancheId() signature! 
Yeah, if we're changing it anyway, we might as well rename it. I'm not 
sure if I like RegisterUserDefinedLWLockTranche() better, but let's 
think it through.

- Heikki

---

From: Sami Imseih <[email-masked]>
Date: Thu, 26 Mar 2026 22:57:44 -0500

Hi,

> I committed these now, but I'm all ears if you still have comments on
> the rest of the patches.

Sorry for the delay. I see you committed the rest. The only issue I found
is with d6eba30

+/* backend-local copy of NamedLWLockTranches->num_user_defined */
+static int  LocalNumUserDefinedTranches;

The comment here should reference "LWLockTranches->num_user_defined "
instead.

> LWLockNewTrancheId() is better and should be used in new code.

That's fair.

> sure if I like RegisterUserDefinedLWLockTranche() better, but let's

Maybe, RegisterNewLWLockTrancheId() could be more meaningful?

Also, there are a few places in lwlock.c where "named tranches" is mentioned.
Maybe we should just say "user-defined tranches" instead?

---

From: Sami Imseih <[email-masked]>
Date: Thu, 26 Mar 2026 23:49:11 -0500

> Maybe we should just say "user-defined tranches" instead?

Like the attached.

--
Sami

---

From: Sami Imseih <[email-masked]>
Date: Fri, 27 Mar 2026 09:53:36 -0500

> Committed with that little change, thanks!

Thanks!

I think there is one more comment cleanup in lwlock.c

 /*
- * This points to the main array of LWLocks in shared memory.  Backends inherit
- * the pointer by fork from the postmaster (except in the EXEC_BACKEND case,
- * where we have special measures to pass it down).
+ * This points to the main array of LWLocks in shared memory.
  */

we no longer need to take special measures to pass down MainLWLockArray
through the BackendParameters.

--
Sami

---

From: Andres Freund <[email-masked]>
Date: Fri, 27 Mar 2026 17:22:33 -0400

Hi,

> Committed with that little change, thanks!

This seems to have broken buildfarm animal batta:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=batta&dt=2026-03-27%2002%3A05%3A01

# Running: pg_rewind --debug --source-pgdata /home/admin/batta/buildroot/HEAD/pgsql.build/src/bin/pg_rewind/tmp_check/t_001_basic_standby_local_data/pgdata --target-pgdata /home/admin/batta/buildroot/HEAD/pgsql.build/src/bin/pg_rewind/tmp_check/t_001_basic_primary_local_data/pgdata --no-sync --config-file /home/admin/batta/buildroot/HEAD/pgsql.build/src/bin/pg_rewind/tmp_check/tmp_test_QbsG/primary-postgresql.conf.tmp
pg_rewind: executing "/home/admin/batta/buildroot/HEAD/pgsql.build/tmp_install/home/admin/batta/buildroot/HEAD/inst/bin/postgres" for target server to complete crash recovery
TRAP: failed Assert("MemoryContextIsValid(context)"), File: "mcxt.c", Line: 1270, PID: 230491
/home/admin/batta/buildroot/HEAD/pgsql.build/tmp_install/home/admin/batta/buildroot/HEAD/inst/bin/postgres(ExceptionalCondition+0x54)[0xaaaae186c204]
/home/admin/batta/buildroot/HEAD/pgsql.build/tmp_install/home/admin/batta/buildroot/HEAD/inst/bin/postgres(MemoryContextAllocExtended+0x0)[0xaaaae18a2a24]
/home/admin/batta/buildroot/HEAD/pgsql.build/tmp_install/home/admin/batta/buildroot/HEAD/inst/bin/postgres(RequestNamedLWLockTranche+0x6c)[0xaaaae16e7310]
/home/admin/batta/buildroot/HEAD/pgsql.build/tmp_install/home/admin/batta/buildroot/HEAD/inst/bin/postgres(process_shmem_requests+0x28)[0xaaaae1881628]
/home/admin/batta/buildroot/HEAD/pgsql.build/tmp_install/home/admin/batta/buildroot/HEAD/inst/bin/postgres(PostgresSingleUserMain+0xc4)[0xaaaae1701a34]
/home/admin/batta/buildroot/HEAD/pgsql.build/tmp_install/home/admin/batta/buildroot/HEAD/inst/bin/postgres(main+0x6ac)[0xaaaae12a2adc]
/lib/aarch64-linux-gnu/libc.so.6(__libc_start_main+0xe8)[0xffff99713dd8]
/home/admin/batta/buildroot/HEAD/pgsql.build/tmp_install/home/admin/batta/buildroot/HEAD/inst/bin/postgres(+0xf2b98)[0xaaaae12a2b98]
Aborted
pg_rewind: error: postgres single-user mode in target cluster failed
pg_rewind: detail: Command was: /home/admin/batta/buildroot/HEAD/pgsql.build/tmp_install/home/admin/batta/buildroot/HEAD/inst/bin/postgres --single -F -D /home/admin/batta/buildroot/HEAD/pgsql.build/src/bin/pg_rewind/tmp_check/t_001_basic_primary_local_data/pgdata -c config_file=/home/admin/batta/buildroot/HEAD/pgsql.build/src/bin/pg_rewind/tmp_check/tmp_test_QbsG/primary-postgresql.conf.tmp template1 < /dev/null

Presumably the reason that batta failed is its special configuration:

shared_preload_libraries = 'pg_stat_statements'; regress_dump_restore; wal_consistency_checking; compute_query_id = regress; --enable-injection-points

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 27 Mar 2026 16:50:12 -0500

On Fri, Mar 27, 2026 at 05:22:33PM -0400, Andres Freund wrote:
> [...](ExceptionalCondition+0x54)[0xaaaae186c204]
> [...](MemoryContextAllocExtended+0x0)[0xaaaae18a2a24]
> [...](RequestNamedLWLockTranche+0x6c)[0xaaaae16e7310]
> [...](process_shmem_requests+0x28)[0xaaaae1881628]
> [...](PostgresSingleUserMain+0xc4)[0xaaaae1701a34]
> [...](main+0x6ac)[0xaaaae12a2adc]
> [...](+0xf2b98)[0xaaaae12a2b98]
> Aborted

Hm.  AFAICT PostmasterContext isn't created in single-user mode, and the
commit in question has RequestNamedLWLockTranche() allocate requests there.
I guess the idea is to allow backends to free that memory after forking
from postmaster, but we don't do that for the NamedLWLockTrancheRequests
list.  Maybe we should surround the last part of that function with
MemoryContextSwitchTo(...) to either TopMemoryContext or PostmasterContext
depending on whether we're in single-user mode.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 27 Mar 2026 17:05:06 -0500

> depending on whether we're in single-user mode.

Concretely, like the attached.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 27 Mar 2026 17:10:16 -0500

On Sat, Mar 28, 2026 at 12:07:26AM +0200, Heikki Linnakangas wrote:
> LGTM, thanks! Will you commit or want me to pick it up?

I'm not able to commit it right this second, so feel free to take it.  Else
it'll probably be a day or two before I can get to it.

-- 
nathan

---

From: Heikki Linnakangas <[email-masked]>
Date: Sat, 28 Mar 2026 01:02:31 +0200

> it'll probably be a day or two before I can get to it.

Ok, committed, thanks!

- Heikki

---

From: Sami Imseih <[email-masked]>
Date: Sat, 28 Mar 2026 12:20:30 -0500

Hi Heikki,

Just raising this again to make sure it doesn’t get overlooked [1].

Thanks!

[1] [https://www.postgresql.org/message-id/CAA5RZ0vPWNMvTBqyH7nqDRrHd6Y4Et5iNqXFuwpbsPOk3cL4rQ%40mail.gmail.com]

--
Sami

---

From: Heikki Linnakangas <[email-masked]>
Date: Mon, 30 Mar 2026 17:18:03 +0300

> Just raising this again to make sure it doesn’t get overlooked [1].

Fixed, thanks!

- Heikki