Hi, On Sun, 20 Sept 2026 at 13:58, Chao Li <[email protected]> wrote: > > 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.) > > I think it’s reasonable to remove the IsUnderPostmaster check for two reasons: > > * IsUnderPostmaster is always false in single-user mode, so the current check > prevents reattachment there. > * The current logic feels unnecessarily asymmetric. The natural pattern for > ShmemInitStruct() is to first look for an existing allocation and create one > only if none exists. With the IsUnderPostmaster check, however, the > postmaster always goes directly to creation, while a child process first > looks for an existing allocation and creates one if it is not found. That > makes the code a little confusing to reason about. It suggests that the > create-or-attach behavior depends on whether the process is under the > postmaster, even though I don't see why that distinction is needed here. In > practice, the postmaster initializes shared memory before child processes are > started anyway. > > So, removing the check makes the behavior simpler and more consistent, always > look for an existing allocation first, and create one only if it doesn't > exist.
Thanks for taking a look. > IsUnderPostmaster is still needed for cases such as EXEC_BACKEND > children, so the new state seems to be added specifically to cover > the single-user after-startup case. Yes, I think we'd still need that allowance for EXEC_BACKEND. The new state would be used for late requests in normal backends too, though it's the standalone case where it changes what we accept. > But the next state is still AFTER_STARTUP_ATTACH_OR_INIT, so the new > state is not really describing an attach-only phase. I was thinking of it as distinguishing when the request happens, rather than making it attach-only. A late request with a known size could still allocate a new area; an unknown-size request would need to find an existing one. Perhaps the comment could make that clearer? I'm not wedded to another enum value here. I just couldn't see how to keep the startup and late-request checks separate with the existing state. > If the only additional case we need to distinguish here is a late > request in single-user mode, would it be simpler to check > MyBackendType == B_STANDALONE_BACKEND instead of adding a new request > state? AFAICS, InitStandaloneProcess() sets B_STANDALONE_BACKEND before we get to ShmemCallRequestCallbacks(). Wouldn't that also let an unknown-size request through during standalone startup, when we're still working out how much memory to allocate? That's the distinction I was trying to preserve in 0002. I may be missing another condition we could use alongside the backend type, though. > Also, for v1-0001, I personally don't feel that the new test is really > necessary. Fair point about the extra startup. I'd lean towards keeping a small reproducer, since the existing tests hadn't caught the double-call case, but perhaps a separate invocation is unnecessary. Would folding the legacy check into the existing single-user run be a reasonable compromise? With exit_on_error already enabled, adding the query there should let us cover it without another backend startup. [I haven't yet tried it though] Regards, Ayush
