On Thu, Sep 24, 2026 at 8:53 PM Heikki Linnakangas <[email protected]> wrote: > > 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. >
Earlier there was no concept of explicit attach, the backends did some local things but they didn't call it "attach". However, we now have an explicit "attach" callback that goes with request - and users will get "hooked" to that concept. It's true that ShmemInitStruct() has no way to specify attach call and thus can not carry out attach tasks. But a structure was requested with an attach call earlier. Maybe extension developers are smart enough to know that that attach won't work here. But maybe they will find it confusing. Let's wait to see reports from the field. > > 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... Codewise it's still a thin wrapper over new APIs, but the new APIs do much more than ShmemInitStruct and that leaks through ShmemInitStruct(). If we have plans to retire the old APIs eventually, maybe we should not let the users mix those. I know you have mentioned earlier that you don't have those plans, but in case that has changed after these reports. -- Best Wishes, Ashutosh Bapat
