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

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.

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

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

Looks good for coverage but not directly related to the code changes.
Worth discussing as a separate patch.

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

Ok.

Some comments on v3.

+ if (options->size <= 0 && options->size != SHMEM_ATTACH_UNKNOWN_SIZE)
+ elog(ERROR, "invalid size %zd for shared memory request for \"%s\"",
+ options->size, options->name);

I am tempted to add some comments as attached. They are optional if
found superfluous.

  *
- * This is called at postmaster startup, before the shared memory segment has
- * been created.
+ * This is called when sizing a new segment at postmaster or standalone
+ * startup, including a postmaster crash restart.

I would prefer "before creating the main shared memory segment"
instead of "sizing new segment" since we don't use sizing often and a
new segment is too general. But I like mentioning standalone, and
postmaster restart. See attached.


+ if (request->options->size == SHMEM_ATTACH_UNKNOWN_SIZE)
+ elog(ERROR, "SHMEM_ATTACH_UNKNOWN_SIZE cannot be used during startup");
+

A comment addition again.

-# Check that the attach counter is incremented on a new connection
+# This first call to the function after startup loads the library
+# and initializes the shmem area.
 my $attach_count1 =
   $node->safe_psql("postgres", "SELECT get_test_shmem_attach_count();");
+
+# Check that the attach counter is incremented on a new connection
 my $attach_count2 =
   $node->safe_psql("postgres", "SELECT get_test_shmem_attach_count();");
 cmp_ok($attach_count2, '>', $attach_count1,
  "attach callback is called in each backend");

I think these are good comments but introduce unnecessary and
unrelated diffs. I would remove them from this patch.

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

+
+$node->stop;

I am a bit surprised by these diffs though. Why aren't they being
shown as simple additions? If you apply the attached patches the
resulting diffs look saner.

 ###
-# Test that loading via shared_preload_libraries also works
+# Test that loading via shared_preload_libraries works

Doesn't make much sense to just remove "also".

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

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

--
Best Wishes,
Ashutosh Bapat