On 24/09/2026 16:07, Ashutosh Bapat wrote:
On Wed, Sep 23, 2026 at 6:50 PM Ayush Tiwari
<[email protected]> wrote:
On Wed, 23 Sept 2026 at 17:53, Heikki Linnakangas <[email protected]> wrote:
On 19/09/2026 22:12, Ayush Tiwari wrote:
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.
Thanks for the review, I committed this part now.
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?
Hmm, I don't quite understand. ShmemInitStruct() didn't have an attach
callback at all, so I don't know what there is to document here.
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.
Yeah, perhaps. For now, I decided to leave out this
"test_shmem_legacy()" test.
(But thanks, Ayush, for including it! As Jacob said earlier, it's really
useful to have reproducers for every issue, even though they're not all
committed to the repository.)
+ $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.
Fair. My goal was that ShmemInitStruct() would be just a thin wrapper
around the same functions that the new API uses, so that there's little
chance of breaking just the legacy function. But I'm not sure I managed
to make it quite so thin...
- Heikki
