Re: [PATCH] Two remaining shmem attachment issues in single-user mode

Hi,

On Fri, 25 Sept 2026 at 16:27, Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Thu, Sep 24, 2026 at 9:03 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> + /* Check that we're in the right state */
> + if (shmem_request_state != SRS_REQUESTING)
> + elog(ERROR, "ShmemRequestStruct can only be called from a
> shmem_request callback");
> +
>
> Checking whether we can accept a request before looking at the request
> itself seems like a good reordering, even though it is not directly
> related to the fix.
>
> > I came up with a simpler idea: we can check "ShmemIndex == NULL" to know
> > if shared memory has already been initialized and we're in the "after
> > startup" case, or not. That feels like a pretty direct way of checking
> > for exactly the property we care about, without needing another state.

Thanks for the updated patch, Heikki! It is much simplified now.

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

You're right about the ShmemIndex check in v2. I tried changing
test_shmem.area_size to -1 after startup and killing a backend. On restart,
the request callbacks run with ShmemIndex still set.

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

That's what I tried in the attached v3. We allow -1 when registering a
request, but reject it when sizing a new segment. The crash-restart probe
now gets the startup error there.

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

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

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

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

> SELECT test_shmem_register('test_shmem unknown size after startup', -1, 3);
>
> +
> +###
> +# Test allocating memory after startup in single-user mode
> +###
> +SKIP:
> +{
> + # Skip the test on Windows, as single-user mode would fail on permission
> + # failure with privileged accounts.
> + skip 'single-user test is not supported by this platform', 1
> + if $windows_os;
>
> This block now has two tests, so the skip count should be 2.
> Alternately, omitting the count defaults to one skipped test; it does
> not report both tests as skipped, even though it skips execution of
> the entire block.
>
> +
> + my @command = (
> + 'postgres', '--single', '-F',
> + '-c' => 'exit_on_error=true',
> + '-D' => $node->data_dir,
> + 'postgres');
> +
> + my $queries = "SELECT get_test_shmem_attach_count();\n";
> + my $result = run_log([(at)command], '<' => \$queries);
> + ok($result, "shmem area is initialized in single-user mode");
> +
> + $queries = qq{
> +-- allocate
> +SELECT test_shmem_register('test_shmem after startup', 25, 1);
> +-- attach
> +SELECT test_shmem_register('test_shmem after startup', 25, 2);
> +-- attach with SHMEM_ATTACH_UNKNOWN_SIZE
> +SELECT test_shmem_register('test_shmem after startup', -1, 3);
> +};
>
> An optional nit: we could store the common queries in variables and
> reuse them here and in the earlier block to keep the allocation and
> attachment requests consistent. The scenarios would still differ: the
> earlier block uses separate backend connections and also checks a size
> mismatch.
>
> The two assertions in this block have the same description. Could the
> second say "request with various attachment parameters succeeds in
> single-user mode" or some such?
>
> + /*
> + * Callback for test_shmem_register(). test_shmem_register() provides the
> + * options, we just pass them through to ShmemRequestStruct.
> + */
> s/ShmemRequestStruct/ShmemRequestStructWithOpts/
>
> +
> +/*
> + * Allocate or attach to a shmem segment, with the caller-supplied name and
> + * size.
>
> s/shmem segment/shared memory structure/
>
> + *
> + * The given integer 'new_value' is stored in the segment, and the old value
> + * is returned.
>
> The given integer 'new_value' is stored at the beginning of the shared
> memory structure, and the old value there is returned.
>
> test_shmem_register() should reject positive sizes smaller than
> sizeof(int), since it reads and writes an int.
> SHMEM_ATTACH_UNKNOWN_SIZE must remain allowed for attachment tests;
> those tests must ensure that the existing structure is large enough.
>
> I think we should pass verbose => 0 to the background_psql session's
> query methods to suppress query and result logging where it is
> unnecessary. I missed this in the original implementation.

I left the queries separate since the normal-backend and
single-user cases run differently. Does that seem fine?

The single-user SKIP count is now 5. I also changed the duplicate
description, fixed the helper comments, added the sizeof(int)
check, and used verbose => 0

v3 is attached with all the above changes.

Regards,
Ayush