Call CalculateShmemSize() only once

On Fri, Oct 30, 2026 at 12:17:33PM +0100, Ashutosh Bapat wrote:

During normal startup, we call CalculateShmemSize() twice: once to
initialize the shared-memory-related GUCs and again to create the
shared memory segment. We can avoid the second calculation by calling
CalculateShmemSize() after ShmemCallRequestCallbacks() and passing the
result to InitializeShmemGUCs() and CreateSharedMemoryAndSemaphores().
The attached patch does this, ensuring that GUC reporting and shared
memory creation use the same calculated size.

+1. I think it's my fault it's called twice. I don't recall why I didn't
bother trying to fix it, but I do think it's worth fixing.

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.

-	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())?

/*
* 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.

-CreateSharedMemoryAndSemaphores(void)
+CreateSharedMemoryAndSemaphores(Size size)

Should we verify the size looks reasonable? A "!= 0" check might be
worthwhile, at least.

- *	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.

- * 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.

--
nathan

Hi Ashutosh,

On Fri, Oct 30, 2026 at 4:48 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

Hi all,

During normal startup, we call CalculateShmemSize() twice: once to
initialize the shared-memory-related GUCs and again to create the
shared memory segment. We can avoid the second calculation by calling
CalculateShmemSize() after ShmemCallRequestCallbacks() and passing the
result to InitializeShmemGUCs() and CreateSharedMemoryAndSemaphores().
The attached patch does this, ensuring that GUC reporting and shared
memory creation use the same calculated size.

+1, the patch LGTM except what Nathan already pointed out :)

I considered combining these four calls into a single function.
However, when postgres -C reports a runtime-computed GUC, it exits
after GUC initialization without creating shared memory. Keeping
sizing and GUC initialization separate from shared memory creation
preserves that distinction.

I also wanted the same but somehow the API is ending up ugly as we need to
bring some stuff which doesn't "naturally" belong into this 1 API.

We could also invoke ShmemCallRequestCallbacks() from
CalculateShmemSize() to keep request collection and sizing together.
That alone would not prevent additional requests after sizing, though.

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.

+1, i think this helps.

I’m proposing this change for master. It may also be worth considering
for PG 19 to reduce conflicts when backpatching future changes to this
code, rather than as a bug fix in itself.

+1

--
Thanks :)
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

On Fri, Oct 30, 2026 at 11:25 PM Nathan Bossart
<nathandbossart@gmail.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