Re: [PATCH] Two remaining shmem attachment issues in single-user mode - Mailing list pgsql-hackers

On 25/09/2026 10:07, Ashutosh Bapat wrote:
> Replacing the IsUnderPostmaster check with a check for whether shared
> memory has been initialized seems appropriate. However, ShmemIndex
> remains non-NULL when the postmaster restarts after a backend crash
> and recreates shared memory. The registered callbacks are retained,
> but the pending requests are rebuilt by calling those callbacks again.
> If the callbacks return the same sizes as before, we will not
> encounter an unknown size. Still, the check seems brittle: it would
> not reject an unknown size supplied by a callback during restart, and
> the request would instead fail later in the size calculation.

A-ha, good catch.

> We need to reject SHMEM_ATTACH_UNKNOWN_SIZE before allocating the
> shared-memory segment because its size must be known. Could we perform
> this check in ShmemGetRequestedSize()? That seems like a more specific
> place for it than ShmemRequestInternal(), which handles requests both
> at startup and afterward.

Hmm, I guess that'd work too. It feels a little weird to not get the 
error immediately in the ShmemRequestStruct() call though.

On 29/09/2026 15:07, Ashutosh Bapat wrote:
> On Mon, Sep 28, 2026 at 12:44 PM Ayush Tiwari
> <ayushtiwari.slg01@gmail.com> wrote:
>>
>>> It would also be useful to have Assert(!ShmemIndex) and
>>> Assert(!ShmemAllocator) in ShmemGetRequestedSize() to ensure that the
>>> function is never called after the shared-memory segment has been
>>> created.
>>
>> I left those out for now, since ResetShmemAllocator() would have to clear
>> the old pointers too? I guess that could be part of a separate patch.
> 
> Ok. I don't remember why didn't ResetShmemAllocator() cleared those
> pointers as well. I vaguely remember that it was discussed but have
> forgotten now. Let's see what Heikki says.

+1 for clearing those pointers in ResetShmemAllocator(). When it's 
called, the pointers are pointing to garbage or an area that's already 
been free'd.

>>>> I also reworked the tests. I added a very generic test_shmem_register()
>>>> function that [registers a callback that] calls ShmemRequestStruct()
>>>> with given name and size. And then the perl script can call it with
>>>> different sizes, to test the "unknown-size" case, as well as trying to
>>>> attach with incorrect size etc. So most of the logic is now in the perl
>>>> script.
>>>
>>> I like the idea of test_shmem_register(). Could we convert the
>>> existing out-of-memory test to use this function as well? That would
>>> allow us to remove the test_shmem.area_size GUC and simplify
>>> test_shmem.c. It would exercise the same shared-memory allocation
>>> mechanism, although we would lose coverage of a failed _PG_init()
>>> followed by another library-load attempt in the same backend. Is that
>>> additional coverage worth keeping the GUC?
>>
>> I lean toward keeping it. The out-of-memory test fails while loading the
>> library, then retries in the same session. IIUC, test_shmem_register()
>> wouldn't cover that path?
> 
> Let's defer this to Heikki.

Yeah, I'd like to keep the coverage for out of memory while loading the 
library. That's how I expect these functions to be called most of the 
time, during library loading. There's not much reason to think that it 
would work differently from _PG_init() or from another function, but still.

>>> Probably you were just expecting an opinion on the idea, but here's a
>>> full review as well.
>>>
>>> ###
>>> -# Test allocating memory after startup, i.e. when the library is not
>>> -# in shared_preload_libraries
>>> -
>>> ... snip ...
>>> - ok($result, "shmem area is initialized in single-user mode");
>>> -}
>>>
>>> This also seems like a natural place to test after-startup
>>> allocations: the extension has been created, and the library has not
>>> been loaded into the newly started server. Is there a reason for
>>> moving these tests later?
>>
>> I didn't see a reason to move them either, so v3 keeps the
>> after-startup tests near the start of the script.

I wanted to re-arrange them so that you have the more common scenarios, 
i.e. loading at startup from shared_preload_libraries -- that's all. 
That felt more natural to me.

>>>   ###
>>>   # Test "out of shared memory" in an after-startup request
>>>   ###
>>>
>>> You have removed the test that checks whether an unknown-size request
>>> for a nonexistent structure is rejected. Is that deliberate? Could we
>>> retain it using a call such as the following?
>>
>> I put the missing-area check back in v3 using
>> test_shmem_register(), and also tried an error followed by a
>> valid request in the same single-user process. Heikki, you're
>> better placed to judge whether that second case is worth keeping
>> here. I'm happy to drop it if you'd prefer that.
> 
> Looks good for coverage but not directly related to the code changes.
> Worth discussing as a separate patch.

Ok, I kept those.

> +(undef, undef, $stderr) = $node->psql("postgres",
> + "SELECT test_shmem_register('test_shmem too small', 1, 3);");
> +like($stderr, qr/shared memory size must be at least \d+ bytes/,
> + "request smaller than an integer fails");
> 
> I don't think we need this test - we aren't testing test_shmem.c. The
> corresponding check in test_shmem.c is just for safety.

Agreed

> -$node->adjust_conf('postgresql.conf', "shared_preload_libraries", undef);
> +$node->adjust_conf('postgresql.conf', 'shared_preload_libraries', undef);
> 
> What's the value addition by changing " to '?

pgperltidy does that. I don't know which is better, but it makes sense 
to be consistent.

>   $session->{stderr} = '';
> -$session->query("SET test_shmem.area_size = default;");
> -$session->query_safe("SELECT get_test_shmem_attach_count();");
> +$session->query("SET test_shmem.area_size = default;", verbose => 0);
> +$session->query_safe("SELECT get_test_shmem_attach_count();", verbose => 0);
>   $session->quit;
>   $node->stop;
> 
>   Thanks. Can you please separate the verbose => 0 changes into a separate patch?
> 
> +# clean up
> +$node->stop;
> +$node->adjust_conf('postgresql.conf', "shared_preload_libraries", undef);
> +
> 
> Didn't we stop the node already? Also why to remove the
> shared_preload_libraries setting here when we are about to end the
> test? This might be redundant.
> 
> Sorry for two diffs, I missed some changes when creating the first.

Ok, I picked a mix of these test changes that I liked the best, and 
committed :-). Thank you both!

- Heikki