Re: Call CalculateShmemSize() only once

On Fri, Oct 30, 2026 at 11:25 PM Nathan Bossart
<nathandbossart(at)gmail(dot)com> wrote:
>
> > As a follow-up, would it make sense to introduce an SRS_SIZED state
> > between SRS_REQUESTING and SRS_INITIALIZING? We could enter it when
> > ShmemGetRequestedSize() finishes and adjust the state checks
> > accordingly. This would prevent new requests from being registered
> > after the required shared memory size has been calculated.
> > EXEC_BACKEND child attachment and late allocation would retain their
> > existing paths without entering this state. I can update the patch if
> > this seems worthwhile.
>
> Seems worth exploring.

Done. This is a small change that fits the existing state machine. I
have included it as a separate patch for review; it could be merged
into the CalculateShmemSize() patch with an updated commit message.

> > /*
> > * CalculateShmemSize
> > * Calculates the amount of shared memory needed.
> > + *
> > + * Expected to be called only once after collecting all shared memory requests
> > + * but before allocating the shared memory segment at the startup or server
> > + * restart.
> > */
>
> Should we ERROR if it is called more than once? That might need some
> special handling for the restart_after_crash case.
>

With this change, a second call to ShmemGetRequestedSize() during the
same shared-memory initialization cycle fails
Assert(shmem_request_state == SRS_REQUESTING) in assertion-enabled
builds. This also detects repeated calls through CalculateShmemSize().
I think an assertion is sufficient here because this is an internal
lifecycle invariant, consistent with the other state-transition
assertions. Attempts to register requests after sizing are still
rejected by the existing runtime checks.

For restart_after_crash, ResetShmemAllocator() resets the state to
SRS_INITIAL. The request callbacks then start a fresh request phase,
allowing the size to be calculated again for the new segment.

>
> > - CreateSharedMemoryAndSemaphores();
> > + shmem_size = CalculateShmemSize();
> > + CreateSharedMemoryAndSemaphores(shmem_size);
>
> nitpick: When the variable is only used in one place, is there any reason
> not to just do CreateSharedMemoryAndSemaphores(CalculateShmemSize())?
>

I find a separate variable more readable. Your suggestion looks ok
too. Changed as suggested in the bootstrap and crash-restart paths.
The normal postmaster and single-user startup paths retain the
variable because they also pass the size to InitializeShmemGUCs().

> > -CreateSharedMemoryAndSemaphores(void)
> > +CreateSharedMemoryAndSemaphores(Size size)
>
> Should we verify the size looks reasonable? A "!= 0" check might be
> worthwhile, at least.

Both the Unix and Windows implementations of PGSharedMemoryCreate(),
called by CreateSharedMemoryAndSemaphores(), already contain:
/* Room for a header? */
Assert(size > MAXALIGN(sizeof(PGShmemHeader)));

These assertions run before allocating the shared-memory segment and
enforce a stronger condition than size != 0. I have therefore left out
the additional check in CreateSharedMemoryAndSemaphores(). Happy to
add it if you would prefer an explicit assertion at that entry point
too.

>
> > - * ShmemGetRequestedSize() --- estimate the total size of all registered shared
> > - * memory structures.
> > + * ShmemGetRequestedSize() --- estimate the total size of all registered shared
> > + * memory structures.
>
> nitpick: Unnecessary changes.
>

+1. Fixed.

> > - * This is called at postmaster startup, before the shared memory segment has
> > - * been created.
> > + * This is called at postmaster startup or server restart, before the shared
> > + * memory segment has been created.
>
> nitpick: This also seems unnecessary, or at least unrelated to the patch at
> hand.
>

"or server restart" should have been mentioned explicitly here even
without this change, and now that we are tightening this contract
further, I thought it would be better to mention it. Reverted it now.

Please find attached the revised patches.

--
Best Wishes,
Ashutosh Bapat