On Wed, Sep 23, 2026 at 6:50 PM Ayush Tiwari
<[email protected]> wrote:
>
> Hi,
>
> On Wed, 23 Sept 2026 at 17:53, Heikki Linnakangas <[email protected]> wrote:
> >
> > On 19/09/2026 22:12, Ayush Tiwari wrote:
> > > Hi,
> > >
> > > I found two more shmem attachment issues in single-user mode after
> > > the recent fixes. Patches attached.
> > >
> > > With ShmemInitStruct(), a second call for the same name and size errors
> > > out with "already initialized". We only look for the old allocation if
> > > IsUnderPostmaster is true, so a standalone backend goes straight down
> > > the allocation path again.
> > >
> > > 0001 drops that condition. It fixes postmaster-startup reattachment
> > > too, which I did hesitate over at first. But AFAICS that was supported
> > > before the refactoring, and it's what the legacy API still promises.
> > > So I'd lean towards restoring that behaviour in both places. Is there
> > > a reason not to? (The size and initialization checks are still there.)
> >
> > Hmm, so the scenario is:
> >
> > 1. Start postgres in single-user mode
> > 2. Load an extension that calls ShmemInitStruct() to alloc a shmem area
> > 3. The extension calls ShmemInitStruct() again, to get a pointer to the
> > already-initialized area.
> >
> > I didn't think of that scenario. There's no reason to not support that,
> > although it's pretty weird for an extension to do that. One use case
> > might be to have a shared struct between two cooperating extensions, so
> > that they both call ShmemInitStruct() to get a pointer to the same area.
> > I'm not aware of any extensions actually doing that, though.
> >
> > I'll commit that fix, thanks!
The code changes look good to me. There are opportunities to make the
test less confusing and cover more scenarios.
- *foundPtr = false;
- if (IsUnderPostmaster)
- *foundPtr = AttachShmemIndexEntry(&request, true);
+ *foundPtr = AttachShmemIndexEntry(&request, true);
Before the refactor, ShmemInitStruct() did not distinguish between
IsUnderPostmaster being true or false for ordinary allocations. It did
distinguish between those modes when creating ShmemIndex. However, in
the current code, ShmemIndex creation does not go through regular
requests. So I think we are safe removing the condition.
When attaching to existing shared memory through the callback-based
API after startup, the supplied attach callback is invoked. In
contrast, ShmemInitStruct() does not invoke an attach callback. People
using both the legacy and new APIs may find this difference
surprising. Should we document it explicitly in the ShmemInitStruct()
documentation in a separate patch?
I think there's also some value in porting parts of the
001_late_shmem_alloc test to the PostgreSQL 18 stable branch to test
and document the legacy behaviour. The code has changed so much that
it's hard to know what the legacy behaviour was. Even porting the
whole test and adjusting it for the APIs there might be worth it. That
way we can easily compare the new behaviour with the legacy behaviour
simply by looking at the differences in the test. This could be a
separate patch.
+
+ $query = "SELECT test_shmem_legacy();\n";
+ $result = run_log(\@command, '<' => \$query);
+ ok($result, "legacy shared memory reattachment works in single-user mode");
I think we should add this test to the shared_preload_libraries
section as well to check that legacy shared memory reattachment works
correctly when the module is loaded through that GUC. Over time, we
may fail to maintain compatibility with the legacy APIs and won't
notice the drift without tests covering those cases.
+
+PG_FUNCTION_INFO_V1(test_shmem_legacy);
+Datum
+test_shmem_legacy(PG_FUNCTION_ARGS)
+{
I would rename this to test_shmem_legacy_api() for the sake of clarity
and also add a prologue to the function explaining what it tests.
+ void *first;
+ void *second;
+ bool found;
+
+ first = ShmemInitStruct("test_shmem legacy", 64, &found);
+ second = ShmemInitStruct("test_shmem legacy", 64, &found);
Could we also check the first call's found result when the test
guarantees a fresh allocation? It is currently overwritten by the
second call, so an incorrect found result from the first call would go
unnoticed.
In addition to these two calls, I would test
ShmemInitStruct("test_shmem area", test_shmem_area_size, &found),
checking both found and that the returned pointer equals TestShmem.
That area will already have been created through the new API when the
library is loaded. This would test interoperability between the new
and old APIs while retaining coverage of legacy allocation followed by
legacy reattachment.
>
> Thanks for looking into it.
>
> > > The other one is a bit odd: ask for an existing area with
> > > SHMEM_ATTACH_UNKNOWN_SIZE after startup in single-user mode, and we
> > > tell you it "cannot be used during startup".
> > >
> > > IIUC, the distinction we need here is whether we're still working out
> > > the initial shmem requirements, not whether we have a postmaster.
> > > 0002 adds SRS_REQUESTING_AFTER_STARTUP for that. I couldn't see a clean
> > > way to reuse SRS_REQUESTING without mixing those cases up.
> > >
> > > I'm a bit on the fence about adding another state just for this.
> > > With the PG19 release getting close, I thought I'd send this out for
> > > feedback before spending more time iterating on it. Does the extra
> > > state seem like the right approach?
> >
> > Extra state sounds reasonable. Thanks, I'll take a closer look, and I'll
> > double-check all the other places in shmem.c where we use
> > IsUnderPostmaster, too.
> >
> > The IsUnderPostmaster variable is deceptive. It's easy to forget about
> > single-user code, and incorrectly assume that IsUnderPostmaster == true
> > means you're a backend and IsUnderPostmaster == false means you're
> > postmaster. I think that's what happened to me here and with the
> > previous single-user mode bugs. I remember I've struggled to keep that
> > in mind in the past too. We should perhaps replace IsUnderPostmaster
> > with a three-valued enum or something (postmaster, backend, single-user
> > backend).
If we do this, do we still need extra state?
>
> I agree. It's v v deceptive.
> [Though I did not know much about single user modes till tripping]
Is being in postmaster = IsPostmasterEnvironment && !IsUnderPostmaster
single user = !IsPostmasterEnvironment
normal backend = IsPostmasterEnvironment && IsUnderPostmaster
If we are going to a three value enum, is it worth folding
IsPostmasterEnvironment and IsUnderPostmaster into that enum? Will
that be too much to change in PG 19 at this stage? Here's review of
the test changes. I will review the code once we settle the questions
above.
+is($node->safe_psql("postgres", "SELECT test_shmem_unknown_size();"), 't',
+ "unknown-size attachment works in a normal backend");
This adds a test assertion to the cluster setup block. Could we move
it into a separate test section with a short comment explaining its
purpose.
+SKIP:
+{
+ skip 'single-user test is not supported by this platform', 2
+ if $windows_os;
+ my @command = (
+ 'postgres', '--single', '-F',
+ '-c' => 'exit_on_error=true',
+ '-D' => $node->data_dir);
+ my $query = "SELECT test_shmem_unknown_size();\n";
+ my $result = run_log([@command, 'postgres'], '<' => \$query);
+ ok($result, "unknown-size attachment works in single-user mode");
This tests attachment through the callback-based API, whereas the
previous patch tests attachment through the legacy API. It would be
good to make the API distinction explicit in the test description.
Could we also test a late unknown-size request for a nonexistent area
in single-user mode, followed by a valid request in the same process?
With exit_on_error=false, we could check both the expected error and
the subsequent successful result. This would exercise the missing-area
guard and restoration of the request state after an error. The
existing missing-area test covers a normal backend only.
+
+ my $stderr;
+ $result = run_log(
+ [@command,
+ '-c' => 'shared_preload_libraries=test_shmem',
+ '-c' => 'test_shmem.area_size=-1', 'postgres'],
+ '<' => \$query, '2>' => \$stderr);
+ ok(!$result && $stderr =~ /SHMEM_ATTACH_UNKNOWN_SIZE cannot be used
during startup/,
+ "unknown-size requests are rejected during single-user startup");
Time to rename 001_late_shmem_alloc.pl to reflect its broader scope?
:) It already includes tests for allocation during startup, and this
patch adds a startup rejection test.
This test already loads the module through shared_preload_libraries in
single-user mode. It would be useful to add an equivalent test that
postmaster startup rejects SHMEM_ATTACH_UNKNOWN_SIZE. Separately, the
existing shared_preload_libraries section could test successful
unknown-size attachment after startup to an area allocated by the
preloaded module with a known size. These could be follow-up tests in
a separate patch.
Could we combine the two SKIP blocks for single-user tests, since they
have the same skip condition? Keeping them separate is also reasonable
if each block has a short comment identifying its purpose.
The new unknown-size test block and helper would be easier to
understand with short comments explaining their purpose. Most existing
sections have descriptive headings; similar comments for the additions
in these patches would save readers some effort.
+PG_FUNCTION_INFO_V1(test_shmem_unknown_size);
+Datum
+test_shmem_unknown_size(PG_FUNCTION_ARGS)
+{
+ TestShmemData *attached = NULL;
+ ShmemCallbacks callbacks = {
+ .flags = SHMEM_CALLBACKS_ALLOW_AFTER_STARTUP,
+ .request_fn = test_shmem_unknown_request,
+ .opaque_arg = &attached,
Wicked :). I didn't envision this usage when working on the patch. Add
a comment though explaining the use of opaque_arg.
Also note that the helper supplies a request callback but no attach
callback for a function which the test uses for testing "attach". It
may be a bit confusing at first glance. It would be good to clarify
that too in the code.
+ };
+
+ RegisterShmemCallbacks(&callbacks);
+ if (attached == NULL || attached != TestShmem || !attached->initialized)
+ elog(ERROR, "could not attach to shared memory with unknown size");
+
+ PG_RETURN_BOOL(true);
+}
+
This function tests attachment to an existing shared memory area using
SHMEM_ATTACH_UNKNOWN_SIZE after startup. I would name it
test_shmem_attach_after_starutp() or some such and add a short
prologue explaining that purpose. Note, we already have
test_shmem_attach() in the module, so that name is ruled out. Moving
test_shmem_unknown_request() closer to this function and naming it
after this function's name would make the relationship clearer. Like
adding _request suffix to the SQL-callable function's name.
II had a bit of a hard time reviewing the test code and making sure
that the changes are fine. That's partly because of lack of comments
but also because the functions for different purposes are mixed in the
test file and c code. I think we should separate the C code into
sections - one for code which uses TestShmemCallbacks, another for
code which uses callbacks and then third for legacy APIs. That way we
can easily see which code affects what and make sure that they don't
step on each other's shoes. Similarly we should separate the single
user tests into their own file. They hardly share any test code and
having them in the middle of non-single user tests is confusing.
Further we can then clearly see dependencies between various blocks in
the test file and again reason easily that they don't affect each
other.
--
Best Wishes,
Ashutosh Bapat
