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
<[email protected]> 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
