On Fri, Oct 30, 2026 at 11:25 PM Nathan Bossart <[email protected]> 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
From a0edb157f68b0c2fb443c0a75150fab69cd7db7b Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <[email protected]> Date: Mon, 2 Nov 2026 15:00:10 +0530 Subject: [PATCH v20261102 2/2] Introduce SRS_SIZED ... as mentioned in the commit message of previous commit. --- src/backend/storage/ipc/ipci.c | 4 ---- src/backend/storage/ipc/shmem.c | 13 +++++++++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c index 64e16aeaa69..1c4cd27d2be 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -52,10 +52,6 @@ RequestAddinShmemSpace(Size size) /* * 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. */ Size CalculateShmemSize(void) diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c index 71bf22d2438..1eb70cf58d3 100644 --- a/src/backend/storage/ipc/shmem.c +++ b/src/backend/storage/ipc/shmem.c @@ -181,7 +181,7 @@ static List *pending_shmem_requests; /* List of ShmemRequests */ * right order. * * Postmaster: - * INITIAL -> REQUESTING -> INITIALIZING -> DONE + * INITIAL -> REQUESTING -> SIZED -> INITIALIZING -> DONE * * Backends in EXEC_BACKEND mode: * INITIAL -> REQUESTING -> ATTACHING -> DONE @@ -201,6 +201,13 @@ enum shmem_request_state */ SRS_REQUESTING, + /* + * The required size of the shared memory segment has been calculated. No + * further requests may be registered before creating the shared memory + * segment. + */ + SRS_SIZED, + /* * Postmaster has finished all shmem requests, and is now initializing the * shared memory segment. init_fn callbacks are called in this state. @@ -420,6 +427,8 @@ ShmemGetRequestedSize(void) size = add_size(size, request->options->size); } + shmem_request_state = SRS_SIZED; + return size; } @@ -695,7 +704,7 @@ InitShmemAllocator(PGShmemHeader *seghdr) } else { - Assert(shmem_request_state == SRS_REQUESTING); + Assert(shmem_request_state == SRS_SIZED); shmem_request_state = SRS_INITIALIZING; } -- 2.34.1
From 195170e43e414d999ec51b6ebde20d3b1875eb7d Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <[email protected]> Date: Thu, 29 Oct 2026 15:12:37 +0530 Subject: [PATCH v20261102 1/2] Call CalculateShmemSize() only once when creating shared memory During normal startup, CalculateShmemSize() is called twice: first in InitializeShmemGUCs() and then in CreateSharedMemoryAndSemaphores(). When the server is invoked only to report a runtime-computed GUC, the second call is not reached. Calculate the size after collecting shared memory requests and pass it to both functions. Besides avoiding duplicate calculation, this ensures that GUC reporting and shared memory creation use the same calculated size. Note: It might be worth considering adding a separate state SRS_SIZING after SRS_REQUESTING and SRS_INITIALIZING to make sure that no new requests are added between calculating the total amount of memory required and allocating the shared memory. But it's not done in this commit. Author: Ashutosh Bapat <[email protected]> Reviewed-by: Nathan Bossart <[email protected]> Discussion: https://postgr.es/m/caexhw5ttqauxiycm4okl8xkhzrndtn2jmn4wqjm1pkpnuqx...@mail.gmail.com --- src/backend/bootstrap/bootstrap.c | 2 +- src/backend/postmaster/postmaster.c | 8 +++++--- src/backend/storage/ipc/ipci.c | 21 +++++++++++---------- src/backend/storage/ipc/shmem.c | 2 ++ src/backend/tcop/postgres.c | 6 ++++-- src/include/storage/ipc.h | 4 ++-- 6 files changed, 25 insertions(+), 18 deletions(-) diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 63e4d71b96b..063666a8424 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -377,7 +377,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only) InitializeFastPathLocks(); ShmemCallRequestCallbacks(); - CreateSharedMemoryAndSemaphores(); + CreateSharedMemoryAndSemaphores(CalculateShmemSize()); /* * Estimate number of openable files. This is essential too in --check diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index f53aa9c404d..2b6e5f9fc6c 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -501,6 +501,7 @@ PostmasterMain(int argc, char *argv[]) char *userDoption = NULL; bool listen_addr_saved = false; char *output_config_variable = NULL; + Size shmem_size; InitProcessGlobals(); @@ -973,13 +974,14 @@ PostmasterMain(int argc, char *argv[]) * shared memory needs. */ ShmemCallRequestCallbacks(); + shmem_size = CalculateShmemSize(); /* * Now that loadable modules have had their chance to request additional * shared memory, determine the value of any runtime-computed GUCs that * depend on the amount of shared memory required. */ - InitializeShmemGUCs(); + InitializeShmemGUCs(shmem_size); /* * Now that modules have been loaded, we can process any custom resource @@ -1014,7 +1016,7 @@ PostmasterMain(int argc, char *argv[]) * normally choose the same IPC keys. This helps ensure that we will * clean up dead IPC objects if the postmaster crashes and is restarted. */ - CreateSharedMemoryAndSemaphores(); + CreateSharedMemoryAndSemaphores(shmem_size); /* * Estimate number of openable files. This must happen after setting up @@ -3280,7 +3282,7 @@ PostmasterStateMachine(void) */ ResetShmemAllocator(); ShmemCallRequestCallbacks(); - CreateSharedMemoryAndSemaphores(); + CreateSharedMemoryAndSemaphores(CalculateShmemSize()); UpdatePMState(PM_STARTUP); diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c index e149a738c8d..64e16aeaa69 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -52,6 +52,10 @@ RequestAddinShmemSpace(Size size) /* * 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. */ Size CalculateShmemSize(void) @@ -114,19 +118,16 @@ AttachSharedMemoryStructs(void) /* * CreateSharedMemoryAndSemaphores - * Creates and initializes shared memory and semaphores. + * Creates and initializes shared memory of given size and semaphores. */ void -CreateSharedMemoryAndSemaphores(void) +CreateSharedMemoryAndSemaphores(Size size) { PGShmemHeader *shim; PGShmemHeader *seghdr; - Size size; Assert(!IsUnderPostmaster); - /* Compute the size of the shared-memory block */ - size = CalculateShmemSize(); elog(DEBUG3, "invoking IpcMemoryCreate(size=%zu)", size); /* @@ -183,20 +184,20 @@ RegisterBuiltinShmemCallbacks(void) * InitializeShmemGUCs * * This function initializes runtime-computed GUCs related to the amount of - * shared memory required for the current configuration. + * shared memory required for the current configuration. The input size is + * expected to be the total shared memory size (in bytes) computed for the + * current configuration. */ void -InitializeShmemGUCs(void) +InitializeShmemGUCs(Size size_b) { char buf[64]; - Size size_b; Size size_mb; Size hp_size; /* - * Calculate the shared memory size and round up to the nearest megabyte. + * Round the shared memory size up to the nearest megabyte. */ - size_b = CalculateShmemSize(); size_mb = add_size(size_b, (1024 * 1024) - 1) / (1024 * 1024); sprintf(buf, "%zu", size_mb); SetConfigOption("shared_memory_size", buf, diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c index 8d81893b470..71bf22d2438 100644 --- a/src/backend/storage/ipc/shmem.c +++ b/src/backend/storage/ipc/shmem.c @@ -400,6 +400,8 @@ ShmemGetRequestedSize(void) { size_t size; + Assert(shmem_request_state == SRS_REQUESTING); + /* memory needed for the ShmemIndex */ size = hash_estimate_size(list_length(pending_shmem_requests) + SHMEM_INDEX_ADDITIONAL_SIZE, sizeof(ShmemIndexEnt)); diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index b6bdfe213fe..944aefe3699 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -4222,6 +4222,7 @@ PostgresSingleUserMain(int argc, char *argv[], const char *username) { const char *dbname = NULL; + Size shmem_size; Assert(!IsUnderPostmaster); @@ -4303,13 +4304,14 @@ PostgresSingleUserMain(int argc, char *argv[], * add-ins, a chance to chance to adjust their requested shmem sizes. */ ShmemCallRequestCallbacks(); + shmem_size = CalculateShmemSize(); /* * Now that loadable modules have had their chance to request additional * shared memory, determine the value of any runtime-computed GUCs that * depend on the amount of shared memory required. */ - InitializeShmemGUCs(); + InitializeShmemGUCs(shmem_size); /* * Now that modules have been loaded, we can process any custom resource @@ -4321,7 +4323,7 @@ PostgresSingleUserMain(int argc, char *argv[], * Create shared memory etc. (Nothing's really "shared" in single-user * mode, but we must have these data structures anyway.) */ - CreateSharedMemoryAndSemaphores(); + CreateSharedMemoryAndSemaphores(shmem_size); /* * Estimate number of openable files. This must happen after setting up diff --git a/src/include/storage/ipc.h b/src/include/storage/ipc.h index b205b00e7a1..db5c69a08db 100644 --- a/src/include/storage/ipc.h +++ b/src/include/storage/ipc.h @@ -79,10 +79,10 @@ extern PGDLLIMPORT shmem_startup_hook_type shmem_startup_hook; extern void RegisterBuiltinShmemCallbacks(void); extern Size CalculateShmemSize(void); -extern void CreateSharedMemoryAndSemaphores(void); +extern void CreateSharedMemoryAndSemaphores(Size size); #ifdef EXEC_BACKEND extern void AttachSharedMemoryStructs(void); #endif -extern void InitializeShmemGUCs(void); +extern void InitializeShmemGUCs(Size size_b); #endif /* IPC_H */ base-commit: cea96cdfecbf40aa3f12c1c2572c191f27c318d0 -- 2.34.1
