Обсуждение: GetNamedLWLockTranche crashes on Windows in normal backend

Hi,

While working on [0], I observed that $SUBJECT. I encountered this issue
while building test cases for [0], and in which GetNamedLWLockTranche is
called outside of startup.

This crash, reproducible on HEAD, occurs because NamedLWLockTrancheRequestArray
( a local array populated during shmem_request ) is not copied over in
exec:ed backends.
This is reproducible on all EXEC_BACKEND environments.
On Linux, this is not an issue because of the magic of fork.

Since this data is not in shared memory, it will need much extra handling
in launch_backend.c, if at all safe, to get it copied. I am not too
familiar with
EXEC_BACKEND, so I could be wrong.
NamedLWLockTrancheRequests, which tracks the length of
NamedLWLockTrancheRequestArray is copied successfully.

Now, I wonder if the correct solution is just to allow GetNamedLWLockTranche
to be called ONLY when !IsUnderPostmaster, and error out if that's not
the case.
The docs [1] for the function mention it should be used during startup, and I
can't envision a reason one would want to use it outside.

We can also create a new global bool for shmem_startup progress, similar to
process_shmem_requests_in_progress, and ensure that this function is only
called at that time.

I repro'd this on a Windows machine, but one can also enable EXEC_BACKEND
in pg_config_manual.h and call GetNamedLWLockTranche by a normal backend.

I did this by injecting
```
GetNamedLWLockTranche("pg_stat_statements");
```
inside pgss_ExecutorEnd


[0] https://www.postgresql.org/message-id/CAA5RZ0vvED3naph8My8Szv6DL4AxOVK3eTPS0qXsaKi%3DbVdW2A%40mail.gmail.com
[1] https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-SHARED-ADDIN-AT-STARTUP

--
Sami Imseih
Amazon Web Services (AWS)

On Fri, Aug 22, 2025 at 02:21:55PM -0500, Sami Imseih wrote:
> While working on [0], I observed that $SUBJECT. I encountered this issue
> while building test cases for [0], and in which GetNamedLWLockTranche is
> called outside of startup.
> 
> [...]
> 
> I repro'd this on a Windows machine, but one can also enable EXEC_BACKEND
> in pg_config_manual.h and call GetNamedLWLockTranche by a normal backend.
> 
> I did this by injecting
> ```
> GetNamedLWLockTranche("pg_stat_statements");
> ```
> inside pgss_ExecutorEnd

If this fails, why doesn't the call in pgss_shmem_startup() also fail?  Was
pg_stat_statements not loaded via shared_preload_libraries?  And is the
failure a segfault or a "requested tranche is not registered" error?

-- 
nathan

> On Fri, Aug 22, 2025 at 02:21:55PM -0500, Sami Imseih wrote:
> > While working on [0], I observed that $SUBJECT. I encountered this issue
> > while building test cases for [0], and in which GetNamedLWLockTranche is
> > called outside of startup.
> >
> > [...]
> >
> > I repro'd this on a Windows machine, but one can also enable EXEC_BACKEND
> > in pg_config_manual.h and call GetNamedLWLockTranche by a normal backend.
> >
> > I did this by injecting
> > ```
> > GetNamedLWLockTranche("pg_stat_statements");
> > ```
> > inside pgss_ExecutorEnd
>
> If this fails, why doesn't the call in pgss_shmem_startup() also fail?  Was
> pg_stat_statements not loaded via shared_preload_libraries?

because the array is valid in postmaster, but it's not for a normal backend,
since it's not getting copied.

Yes, pg_stat_statements was loaded via shared_preload_libraries. Nothing
was done out of the ordinary.

> And is the
> failure a segfault or a "requested tranche is not registered" error?

It's a segfault.

--
Sami

On Mon, Aug 25, 2025 at 12:58:08PM -0500, Sami Imseih wrote:
>> If this fails, why doesn't the call in pgss_shmem_startup() also fail?  Was
>> pg_stat_statements not loaded via shared_preload_libraries?
> 
> because the array is valid in postmaster, but it's not for a normal backend,
> since it's not getting copied.
> 
> Yes, pg_stat_statements was loaded via shared_preload_libraries. Nothing
> was done out of the ordinary.
> 
>> And is the
>> failure a segfault or a "requested tranche is not registered" error?
> 
> It's a segfault.

Oh, I see what's happening.  We are never calling GetNamedLWLockTranche()
in backends because ShmemInitStruct() always sets *found.

I'd argue that calling GetNamedLWLockTranche() outside of an "if (!found)"
block in a shmem_startup_hook doesn't follow convention, but the docs have
this note:

    shmem_startup_hook provides a convenient place for the initialization
    code, but it is not strictly required that all such code be placed in
    this hook.

Like the inaccurate sentence immediately following this one [0], it was
added by commit 964152c.  IMHO we should adjust the documentation to match
reality.

[0] https://postgr.es/m/aJ9QIF3g2QQko9wq%40nathan

-- 
nathan

> On Mon, Aug 25, 2025 at 12:58:08PM -0500, Sami Imseih wrote:
> >> If this fails, why doesn't the call in pgss_shmem_startup() also fail?  Was
> >> pg_stat_statements not loaded via shared_preload_libraries?
> >
> > because the array is valid in postmaster, but it's not for a normal backend,
> > since it's not getting copied.
> >
> > Yes, pg_stat_statements was loaded via shared_preload_libraries. Nothing
> > was done out of the ordinary.
> >
> >> And is the
> >> failure a segfault or a "requested tranche is not registered" error?
> >
> > It's a segfault.
>
> Oh, I see what's happening.  We are never calling GetNamedLWLockTranche()
> in backends because ShmemInitStruct() always sets *found.
>
> I'd argue that calling GetNamedLWLockTranche() outside of an "if (!found)"
> block in a shmem_startup_hook doesn't follow convention, but the docs have
> this note:
>
>         shmem_startup_hook provides a convenient place for the initialization
>         code, but it is not strictly required that all such code be placed in
>         this hook.
>
> Like the inaccurate sentence immediately following this one [0], it was
> added by commit 964152c.  IMHO we should adjust the documentation to match
> reality.
>
> [0] https://postgr.es/m/aJ9QIF3g2QQko9wq%40nathan

Why not ERROR out completely if we are calling this function outside
of postmaster
startup? I don't think we should be satisfied with just a
documentation change, since this
leads to a segfault. We can do something like below at the top of
GetNamedLWLockTranche

```
if (IsUnderPostmaster)
     elog(ERROR, "GetNamedLWLockTranche can only be called during
postmaster startup");
```

Another approach is to just change GetNamedLWLockTranche to use
NamedLWLockTrancheArray since that is already copied in EXEC_BACKEND, and allow
GetNamedLWLockTranche to continue to be used outside of startup. In
this case, we
will need to add num_lwlocks field to NamedLWLockTrancheArray. This
might be better
to backpatch, since we will not be changing user facing behavior.

Attached is a repro patch. You will need to set
shared_preload_libraries = 'pg_stat_statements'
as well.


--
Sami

> Attached is a repro patch. You will need to set
> shared_preload_libraries = 'pg_stat_statements'
> as well.

I forgot to mention. Once patched, you can run a simple
select statement from a new connection to force the crash.
This is because GetNamedLWLockTranche will be called
at pgss_ExecutorEnd.

--
Sami

On Mon, Aug 25, 2025 at 01:44:22PM -0500, Sami Imseih wrote:
> Another approach is to just change GetNamedLWLockTranche to use
> NamedLWLockTrancheArray since that is already copied in EXEC_BACKEND, and
> allow GetNamedLWLockTranche to continue to be used outside of startup. In
> this case, we will need to add num_lwlocks field to
> NamedLWLockTrancheArray. This might be better to backpatch, since we will
> not be changing user facing behavior.

That seems like a reasonable thing to try.  It looks like
NamedLWLockTrancheRequests is copied, too.  Could we used that instead of
adding a new variable?

My guess is that this has been broken since this code was introduced in
commit c1772ad from 2016/v9.6, and AFAIK you are the first to report it, so
I don't feel a tremendous amount of urgency to fix it on the back-branches.

-- 
nathan

> > Another approach is to just change GetNamedLWLockTranche to use
> > NamedLWLockTrancheArray since that is already copied in EXEC_BACKEND, and
> > allow GetNamedLWLockTranche to continue to be used outside of startup. In
> > this case, we will need to add num_lwlocks field to
> > NamedLWLockTrancheArray. This might be better to backpatch, since we will
> > not be changing user facing behavior.
>
> That seems like a reasonable thing to try.  It looks like
> NamedLWLockTrancheRequests is copied, too.  Could we used that instead of
> adding a new variable?

We would not need a new variable. Currently it's assumed that
NamedLWLockTrancheRequests will be the size of both
NamedLWLockTrancheRequests and NamedLWLockTrancheArray.

```
/*
* NamedLWLockTrancheRequests is both the valid length of the request array,
* and the length of the shared-memory NamedLWLockTrancheArray later on.
* This variable and NamedLWLockTrancheArray are non-static so that
* postmaster.c can copy them to child processes in EXEC_BACKEND builds.
*/
int NamedLWLockTrancheRequests = 0;
```

> My guess is that this has been broken since this code was introduced in
> commit c1772ad from 2016/v9.6, and AFAIK you are the first to report it, so
> I don't feel a tremendous amount of urgency to fix it on the back-branches.

Yeah, I was doing something for testing that is not likely to be done in
the real-world. Also, considering this is an EXEC_BACKEND case only,
it makes it even less likely.

--
Sami

Here it is.

I was hoping we can even get rid of NamedLWLockTrancheRequests
altogether, but that's not going to be possible because
RequestNamedLWLockTranche happens earlier than
CreateLWLocks, so NamedLWLockTrancheRequests is essentially
tracking the requested lwlocks until we get a chance to create and
initialize the LWLocks and store them in the shared memory,
NamedLWLockTrancheArray.

--
Sami

On Mon, Aug 25, 2025 at 04:28:09PM -0500, Sami Imseih wrote:
> Here it is.

Looks reasonable to me.  I'll leave this one around for a week or two to
give others a chance to comment.

-- 
nathan

Well, commit 38b602b certainly doesn't do us any favors here since it
removed NamedLWLockTrancheArray.  Given the lack of reports from the field,
I suspect the best path forward is to add an ERROR for unsafe accesses and
to fix the documentation, as discussed upthread.

-- 
nathan

commit 38b602b certainly doesn't do us any favors here since it
> removed NamedLWLockTrancheArray.  Given the lack of reports from the field,
> I suspect the best path forward is to add an ERROR for unsafe accesses and
> to fix the documentation, as discussed upthread.

I suggest we set an in progres flag for shmem_startup_hook, as we do for
shmem_request_hook, and error out this function if called outside of this
hook. The error will be something like this:

```
ERROR:  cannot return the address of LWLocks outside of shmem_startup_hook
```

We can also update the docs.

If you agree with the above, I'll get the patch ready?

--
Sami Imseih
Amazon Web Services (AWS)

On Thu, Sep 04, 2025 at 01:23:26PM -0500, Sami Imseih wrote:
> I suggest we set an in progres flag for shmem_startup_hook, as we do for
> shmem_request_hook, and error out this function if called outside of this
> hook. The error will be something like this:
> 
> ```
> ERROR:  cannot return the address of LWLocks outside of shmem_startup_hook
> ```
> 
> We can also update the docs.
> 
> If you agree with the above, I'll get the patch ready?

Yeah, I think modeling this after commit 4f2400c is a reasonable thing to
explore.

-- 
nathan

> Yeah, I think modeling this after commit 4f2400c is a reasonable thing to
> explore.

Here it is as described above.

--
Sami

On Thu, Sep 04, 2025 at 04:12:09PM -0500, Sami Imseih wrote:
>> Yeah, I think modeling this after commit 4f2400c is a reasonable thing to
>> explore.
> 
> Here it is as described above.

Thanks.  This looks like the right idea to me, but let's give some time for
others to comment.

-- 
nathan

On Thu, Sep 04, 2025 at 08:35:13PM -0500, Nathan Bossart wrote:
> On Thu, Sep 04, 2025 at 04:12:09PM -0500, Sami Imseih wrote:
>>> Yeah, I think modeling this after commit 4f2400c is a reasonable thing to
>>> explore.
>> 
>> Here it is as described above.
> 
> Thanks.  This looks like the right idea to me, but let's give some time for
> others to comment.

I've started preparing this for commit, and I realized that restricting
GetNamedLWLockTranche() to shmem_startup_hook is not sufficient.
EXEC_BACKEND builds will run this hook in every backend, so unless it's
guarded behind some sort of "if (!found)" condition (i.e., only run in the
postmaster), it'll still crash.  I think we just need to add some extra
notes to the docs and check for !IsUnderPostmaster, as discussed upthread.

-- 
nathan

On Mon, Sep 08, 2025 at 11:55:14AM -0500, Nathan Bossart wrote:
> I've started preparing this for commit, and I realized that restricting
> GetNamedLWLockTranche() to shmem_startup_hook is not sufficient.
> EXEC_BACKEND builds will run this hook in every backend, so unless it's
> guarded behind some sort of "if (!found)" condition (i.e., only run in the
> postmaster), it'll still crash.  I think we just need to add some extra
> notes to the docs and check for !IsUnderPostmaster, as discussed upthread.

Or... what if we just moved the request array to shared memory?

-- 
nathan

> On Thu, Sep 04, 2025 at 08:35:13PM -0500, Nathan Bossart wrote:
> > On Thu, Sep 04, 2025 at 04:12:09PM -0500, Sami Imseih wrote:
> >>> Yeah, I think modeling this after commit 4f2400c is a reasonable thing to
> >>> explore.
> >>
> >> Here it is as described above.
> >
> > Thanks.  This looks like the right idea to me, but let's give some time for
> > others to comment.
>
> I've started preparing this for commit, and I realized that restricting
> GetNamedLWLockTranche() to shmem_startup_hook is not sufficient.
> EXEC_BACKEND builds will run this hook in every backend, so unless it's
> guarded behind some sort of "if (!found)" condition (i.e., only run in the
> postmaster), it'll still crash.  I think we just need to add some extra
> notes to the docs and check for !IsUnderPostmaster, as discussed upthread.

I think v2 is fine because it is perfectly fine for a normal backend (
EXEC_BACKEND)
to call this function as long as it's processing the startup hook. The
goal is to prevent it
from being called outside of the startup hook.

>  Or... what if we just moved the request array to shared memory?

I guess that works also, if we want to maintain the existing behavior.
I am OK with this
as well, and I don't see anything wrong with v3.

FWIW, I got the tests discussed in [0] commit ready and also included
tests for this
crash.

Attached is the patch with the tests. v4-0001 and v3-0001 are identical.
v4-0002 includes the tests.

I think we should commit these tests as well. If  you think the tests should
be a separate thread, let me know.

--
Sami

[0] https://www.postgresql.org/message-id/aLcPbyWLawp5_rdt%40nathan

On Mon, Sep 08, 2025 at 02:47:26PM -0500, Sami Imseih wrote:
> I think v2 is fine because it is perfectly fine for a normal backend
> (EXEC_BACKEND) to call this function as long as it's processing the
> startup hook. The goal is to prevent it from being called outside of the
> startup hook.

I thought the goal was to prevent the crashes...

> I think we should commit these tests as well. If  you think the tests
> should be a separate thread, let me know.

Thanks.  I think we can discuss them here once we fix $SUBJECT.  (BTW it
looks like you forgot to attach the patches.)

-- 
nathan

> On Mon, Sep 08, 2025 at 02:47:26PM -0500, Sami Imseih wrote:
> > I think v2 is fine because it is perfectly fine for a normal backend
> > (EXEC_BACKEND) to call this function as long as it's processing the
> > startup hook. The goal is to prevent it from being called outside of the
> > startup hook.
>
> I thought the goal was to prevent the crashes...

yes it is. Ok, I see what I did wrong with my test that showed v2 working in
EXEC_BACKEND. when it was reentering the startup hook, it was skipping
the code under (!found) for shared memory existing. That is also where I
had the code to call GetNamedLWLockTranche. So, that's wrong
( which is what you also mentioned above) I moved
the GetNamedLWLockTranche outside of the (!found) block in the test.

> > I think we should commit these tests as well. If  you think the tests
> > should be a separate thread, let me know.
>
> Thanks.  I think we can discuss them here once we fix $SUBJECT.  (BTW it
> looks like you forgot to attach the patches.)

oops. Attached now.


--
Sami

I've committed 0001, and I plan to look closer at 0002 soon.

-- 
nathan

On Thu, Sep 11, 2025 at 04:18:00PM -0500, Nathan Bossart wrote:
> I've committed 0001, and I plan to look closer at 0002 soon.

I ended up rewriting 0002 as an ordinary regression test, which resulted in
a much smaller patch with comparable coverage.  (Yes, it needs some
comments.)  Thoughts?

-- 
nathan

On Tue, Sep 16, 2025 at 02:35:55PM -0500, Nathan Bossart wrote:
> On Thu, Sep 11, 2025 at 04:18:00PM -0500, Nathan Bossart wrote:
>> I've committed 0001, and I plan to look closer at 0002 soon.
> 
> I ended up rewriting 0002 as an ordinary regression test, which resulted in
> a much smaller patch with comparable coverage.  (Yes, it needs some
> comments.)  Thoughts?

Sorry for the noise.  I was able to simplify the test code further.

-- 
nathan

> On Tue, Sep 16, 2025 at 02:35:55PM -0500, Nathan Bossart wrote:
> > On Thu, Sep 11, 2025 at 04:18:00PM -0500, Nathan Bossart wrote:
> >> I've committed 0001, and I plan to look closer at 0002 soon.
> >
> > I ended up rewriting 0002 as an ordinary regression test, which resulted in
> > a much smaller patch with comparable coverage.  (Yes, it needs some
> > comments.)  Thoughts?
>
> Sorry for the noise.  I was able to simplify the test code further.

This is a trimmed down test suite, but it does cover quite a bit:
tranche name length, NULL tranche names, number of tranches registered,
tranche name lookup. Overall this is still good test coverage.

A few comments.

1/
startup tranches should be:

#define NUM_STARTUP_TRANCHES (2)

instead of:

#define NUM_STARTUP_TRANCHES (256 - 2)

2/
I do think we should add tests for LWLockInitialize to test error for
an unregistered tranche.

```
Datum
test_lwlock_tranche_initialize(PG_FUNCTION_ARGS)
{
LWLock lock;

LWLockInitialize(&lock, LWTRANCHE_FIRST_USER_DEFINED + PG_GETARG_INT32(0));

PG_RETURN_VOID();
}
```

and we can run this test as the first query, before we create any
dynamic tranches
```
SELECT test_lwlock_tranche_initialize(2);
```

The error message will change when a built-in tranche is added, so
the out file will need to be updated at that point, but that does not occur
too often.

--
Sami

On Tue, Sep 16, 2025 at 04:04:39PM -0500, Sami Imseih wrote:
> startup tranches should be:
> 
> #define NUM_STARTUP_TRANCHES (2)
> 
> instead of:
> 
> #define NUM_STARTUP_TRANCHES (256 - 2)

Why?

> I do think we should add tests for LWLockInitialize to test error for
> an unregistered tranche.

I added an initialization test.

-- 
nathan

> On Tue, Sep 16, 2025 at 04:04:39PM -0500, Sami Imseih wrote:
> > startup tranches should be:
> >
> > #define NUM_STARTUP_TRANCHES (2)
> >
> > instead of:
> >
> > #define NUM_STARTUP_TRANCHES (256 - 2)
>
> Why?

It does not really matter for the tests being done, but it just
seems odd that you would create 254 tranches during startup and
leave room for only 2 dynamic tranches. It is not the typical
pattern out there, in my opinion.

--
Sami

On Wed, Sep 17, 2025 at 11:08:12AM -0500, Sami Imseih wrote:
>> On Tue, Sep 16, 2025 at 04:04:39PM -0500, Sami Imseih wrote:
>> > startup tranches should be:
>> >
>> > #define NUM_STARTUP_TRANCHES (2)
>> >
>> > instead of:
>> >
>> > #define NUM_STARTUP_TRANCHES (256 - 2)
>>
>> Why?
> 
> It does not really matter for the tests being done, but it just
> seems odd that you would create 254 tranches during startup and
> leave room for only 2 dynamic tranches. It is not the typical
> pattern out there, in my opinion.

Okay.  I changed it in v8.

-- 
nathan

> Okay.  I changed it in v8.

Thanks! v8 LGTM.

--
Sami

On Wed, Sep 17, 2025 at 11:43:31AM -0500, Sami Imseih wrote:
> Thanks! v8 LGTM.

Actually, we do want to have a few more startup tranches in order to get
some coverage on the repalloc() path in RequestNamedLWLockTranche().  I've
bumped it up to 32 and added a commit message to the patch.  Barring more
feedback, I'm planning to commit this soon.

-- 
nathan

> Actually, we do want to have a few more startup tranches in order to get
> some coverage on the repalloc() path in RequestNamedLWLockTranche().  I've
> bumped it up to 32 and added a commit message to the patch.  Barring more
> feedback, I'm planning to commit this soon.

Makes sense. Thanks!

--
Sami

Hello Nathan,

12.09.2025 00:18, Nathan Bossart wrote:
> I've committed 0001, and I plan to look closer at 0002 soon.
>

Please look at the recovery test failures produced since ed1aad15e, with
pg_stat_statements loaded: [1]. I've reproduced them locally with:
./configure --enable-cassert --enable-debug --enable-tap-tests -q && make -s -j8
echo "
debug_parallel_query = regress
shared_preload_libraries = 'pg_stat_statements'
" >/tmp/test.config
TEMP_CONFIG=/tmp/test.config make -s check -C src/test/recovery/

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=batta&dt=2025-09-12%2002%3A05%3A01

Best regards,
Alexander

On Wed, Sep 17, 2025 at 11:00:00PM +0300, Alexander Lakhin wrote:
> Please look at the recovery test failures produced since ed1aad15e, with
> pg_stat_statements loaded:

I'm tracking a fix for that in another thread [0].

[0] https://postgr.es/m/aMrG8F-NkiedEugc%40nathan

-- 
nathan

Committed.

-- 
nathan