Обсуждение: pgsql: Move named LWLock tranche requests to shared memory.

Move named LWLock tranche requests to shared memory.

In EXEC_BACKEND builds, GetNamedLWLockTranche() can segfault when
called outside of the postmaster process, as it might access
NamedLWLockTrancheRequestArray, which won't be initialized.  Given
the lack of reports, this is apparently unusual, presumably because
it is usually called from a shmem_startup_hook like this:

    mystruct = ShmemInitStruct(..., &found);
    if (!found)
    {
        mystruct->locks = GetNamedLWLockTranche(...);
        ...
    }

This genre of shmem_startup_hook evades the aforementioned
segfaults because the struct is initialized in the postmaster, so
all other callers skip the !found path.

We considered modifying the documentation or requiring
GetNamedLWLockTranche() to be called from the postmaster, but
ultimately we decided to simply move the request array to shared
memory (and add it to the BackendParameters struct), thereby
allowing calls outside postmaster on all platforms.  Since the main
shared memory segment is initialized after accepting LWLock tranche
requests, postmaster builds the request array in local memory first
and then copies it to shared memory later.

Given the lack of reports, back-patching seems unnecessary.

Reported-by: Sami Imseih <samimseih@gmail.com>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0v1_15QPg5Sqd2Qz5rh_qcsyCeHHmRDY89xVHcy2yt5BQ%40mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/ed1aad15e09d7d523f4ef413e3c4d410497c8065

Modified Files
--------------
src/backend/postmaster/launch_backend.c |  3 +++
src/backend/storage/lmgr/lwlock.c       | 31 ++++++++++++++++++++++++++-----
src/include/storage/lwlock.h            |  4 ++++
3 files changed, 33 insertions(+), 5 deletions(-)

Ni Nathan,

On Thu, Sep 11, 2025 at 09:15:12PM +0000, Nathan Bossart wrote:
> Move named LWLock tranche requests to shared memory.
>
> In EXEC_BACKEND builds, GetNamedLWLockTranche() can segfault when
> called outside of the postmaster process, as it might access
> NamedLWLockTrancheRequestArray, which won't be initialized.  Given
> the lack of reports, this is apparently unusual, presumably because
> it is usually called from a shmem_startup_hook like this:

Since this commit has been merged, batta has kept failing.  Here is
the first failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=batta&dt=2025-09-12%2002%3A05%3A01

I use this animal with a specific configuration:
shared_preload_libraries = 'pg_stat_statements'
compute_query_id = regress
regress_dump_restore
wal_consistency_checking
--enable-injection-points

The recovery tests 013_crash_restart.pl, 022_crash_temp_files.pl and
041_checkpoint_at_promote.pl stress some restart scenarios, not all
use injection points.  I could not get a backtrace from the host.

However, I have come up with the following change in 013 that's able
to reproduce what I think is the same crash:
--- a/src/test/recovery/t/013_crash_restart.pl
+++ b/src/test/recovery/t/013_crash_restart.pl
@@ -21,6 +21,8 @@ my $psql_timeout = IPC::Run::timer($PostgreSQL::Test::Utils::timeout_default);

 my $node = PostgreSQL::Test::Cluster->new('primary');
 $node->init(allows_streaming => 1);
+$node->append_conf('postgresql.conf',
+                   "shared_preload_libraries = 'pg_stat_statements'");
 $node->start();

And here is the backtrace:
#0  0x000055fcdf6bc97a in NumLWLocksForNamedTranches () at lwlock.c:385
385 numLocks += NamedLWLockTrancheRequestArray[i].num_lwlocks;
(gdb) bt
#0  0x000055fcdf6bc97a in NumLWLocksForNamedTranches () at lwlock.c:385
#1  0x000055fcdf6bc9b3 in LWLockShmemSize () at lwlock.c:400
#2  0x000055fcdf65bda5 in CalculateShmemSize (num_semaphores=0x7ffcaf7a78e4) at ipci.c:130
#3  0x000055fcdf65c0b1 in CreateSharedMemoryAndSemaphores () at ipci.c:210
#4  0x000055fcdf42830c in PostmasterStateMachine () at postmaster.c:3223
#5  0x000055fcdf42703f in process_pm_child_exit () at postmaster.c:2558
#6  0x000055fcdf425729 in ServerLoop () at postmaster.c:1696
#7  0x000055fcdf424be1 in PostmasterMain (argc=4, argv=0x55fd0a8faa10) at postmaster.c:1403
#8  0x000055fcdef80a19 in main (argc=4, argv=0x55fd0a8faa10) at main.c:231
(gdb) p i
$3 = 0
(gdb) p NamedLWLockTrancheRequestArray[0]
Cannot access memory at address 0x7f15ee4ccc08

Thanks,
--
Michael

On Wed, Sep 17, 2025 at 11:35:56AM +0900, Michael Paquier wrote:
> Since this commit has been merged, batta has kept failing.  Here is
> the first failure:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=batta&dt=2025-09-12%2002%3A05%3A01

Thanks for bringing this to my attention.

> And here is the backtrace:
> #0  0x000055fcdf6bc97a in NumLWLocksForNamedTranches () at lwlock.c:385
> 385 numLocks += NamedLWLockTrancheRequestArray[i].num_lwlocks;
> (gdb) bt
> #0  0x000055fcdf6bc97a in NumLWLocksForNamedTranches () at lwlock.c:385 
> #1  0x000055fcdf6bc9b3 in LWLockShmemSize () at lwlock.c:400 
> #2  0x000055fcdf65bda5 in CalculateShmemSize (num_semaphores=0x7ffcaf7a78e4) at ipci.c:130 
> #3  0x000055fcdf65c0b1 in CreateSharedMemoryAndSemaphores () at ipci.c:210 
> #4  0x000055fcdf42830c in PostmasterStateMachine () at postmaster.c:3223 
> #5  0x000055fcdf42703f in process_pm_child_exit () at postmaster.c:2558 
> #6  0x000055fcdf425729 in ServerLoop () at postmaster.c:1696 
> #7  0x000055fcdf424be1 in PostmasterMain (argc=4, argv=0x55fd0a8faa10) at postmaster.c:1403 
> #8  0x000055fcdef80a19 in main (argc=4, argv=0x55fd0a8faa10) at main.c:231
> (gdb) p i
> $3 = 0
> (gdb) p NamedLWLockTrancheRequestArray[0]
> Cannot access memory at address 0x7f15ee4ccc08

It looks like the postmaster is trying to access the request array after
re-initializing shared memory, which of course fails.  So, we need to keep
the request array in postmaster's local memory, too.  Attached is a quick
attempt at a fix.

-- 
nathan

On Tue, Sep 16, 2025 at 10:27:34PM -0500, Nathan Bossart wrote:
> It looks like the postmaster is trying to access the request array after
> re-initializing shared memory, which of course fails.  So, we need to keep
> the request array in postmaster's local memory, too.  Attached is a quick
> attempt at a fix.

I was able to simplify the patch.

-- 
nathan

On Tue, Sep 16, 2025 at 11:10:34PM -0500, Nathan Bossart wrote:
> I was able to simplify the patch.

+   if (LocalNamedLWLockTrancheRequestArray)
+       NamedLWLockTrancheRequestArray = LocalNamedLWLockTrancheRequestArray;

It's not a common practice in the code to rely on a shmem state that
should be already set in a routine reporting the size of how much
should be allocated, but that seems OK to live with that here with a
local pointer used un !IsUnderPostmaster.

Perhaps LocalNamedLWLockTrancheRequestArray should be initialized to
NULL when declared, and I'd rather document the reasons behind what
this patch is doing in LWLockShmemSize() and when the state is
initialized in CreateLWLocks(), than have the reader assume why the
local pointer is needed (aka reinitialized with restart_after_crash).

013, 022 and 041 look happy with this solution, with or without
EXEC_BACKEND.

Note: I forgot for a second if shared_preload_libraries was reloaded
when the postmaster reinitializes a cluster, and the answer is that
we only go into process_shared_preload_libraries() once at startup..
I was wondering for a second about a potential case where s_p_l is
updated with more lwlocks added in-between, where a reinitialization
would mess up things.
--
Michael

On Wed, Sep 17, 2025 at 01:47:19PM +0900, Michael Paquier wrote:
> Perhaps LocalNamedLWLockTrancheRequestArray should be initialized to
> NULL when declared, and I'd rather document the reasons behind what
> this patch is doing in LWLockShmemSize() and when the state is
> initialized in CreateLWLocks(), than have the reader assume why the
> local pointer is needed (aka reinitialized with restart_after_crash).

I've added some commentary to this effect in v3.

> Note: I forgot for a second if shared_preload_libraries was reloaded
> when the postmaster reinitializes a cluster, and the answer is that
> we only go into process_shared_preload_libraries() once at startup..
> I was wondering for a second about a potential case where s_p_l is
> updated with more lwlocks added in-between, where a reinitialization
> would mess up things.

Yeah, this stuff is rather precarious, and I'm a bit surprised there
haven't been more problems in this area.

-- 
nathan

On Wed, Sep 17, 2025 at 09:34:24AM -0500, Nathan Bossart wrote:
> Yeah, this stuff is rather precarious, and I'm a bit surprised there
> haven't been more problems in this area.

LWLock handling is quite special, but I am also getting suspicious
about some more shmem areas.  Perhaps these deserve a second look,
even if the lwlock handling with its allocation of tranches is kind of
a special treatment.

Anyway, your comment additions in v3 look fine from here.
--
Michael

On Thu, Sep 18, 2025 at 08:49:20AM +0900, Michael Paquier wrote:
> Anyway, your comment additions in v3 look fine from here.

Thanks for reviewing.  Committed.

-- 
nathan

On Thu, Sep 18, 2025 at 09:56:44AM -0500, Nathan Bossart wrote:
> Thanks for reviewing.  Committed.

Thanks.  batta looks happy now.
--
Michael