On Thu, Sep 24, 2026 at 9:03 PM Heikki Linnakangas <[email protected]> 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.
>
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.
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.
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. Calling this function with after-startup requests in
pending_shmem_requests would produce a misleading total anyway.
However, those assertions would fail during crash restart unless
ResetShmemAllocator() also cleared the pointers.
Assert(!IsUnderPostmaster) would be consistent with the current
callers. ShmemRequestInternal() could then just check that the size is
either SHMEM_ATTACH_UNKNOWN_SIZE or positive, a requirement common to
requests made both at startup and afterward.
> 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?
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? The later location appears to preserve those
conditions as well but with an extra statement setting
shared_preload_libraries to undef.
###
# 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?
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([@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.
--
Best Wishes,
Ashutosh Bapat
