Hi Thomas, On Mon, Aug 25, 2025 at 3:11 PM Thomas Munro <[email protected]> wrote: > > On Mon, Aug 25, 2025 at 9:10 PM Ashutosh Bapat > <[email protected]> wrote: > > Is this change correct? Was there any reason to leave it like that in > > e25626677f8076eb3ce94586136c5464ee154381? Or was it just something > > that didn't fit in that commit? > > We/I just missed that opportunity when ripping that stuff out. It > sounds like we might need a comment-only patch to back-patch to 18 > that would say something like "this is done here for historical > reasons" so as not to confuse people with obsolete nonsense, and a > follow up patch for master to do things in a more straightforward way > as you said.
Thanks for the confirmation. Attached patchset has two patches 0001 - backpatchable, adds the comment. 0002 - actual code changes for master. The changes are described in the commit message in detail. I think ProcGlobalSemas() too, can be converted into a macro or can be declared static inline, but I haven't done so. I think it eliminates all the asymmetric handling of semaphores. > > > If the change looks safe and useful, I will create CF entry for it so > > that the patch gets tested on all platforms, and thus with different > > definitions of PGReserveSemaphores(). > > +1, will review, thanks! Added a CF entry so that CI tests the changes on many platforms. https://commitfest.postgresql.org/patch/5997/ -- Best Wishes, Ashutosh Bapat
From 80d441736035f26b13afc1da70d2f065c06252d7 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <[email protected]> Date: Tue, 26 Aug 2025 09:47:52 +0530 Subject: [PATCH 1/2] PGReserveSemaphores() called from CreateSharedMemoryAndSemaphores() Before e25626677f8076eb3ce94586136c5464ee154381, PGReserveSemaphores() was required to be called before SpinlockSemaInit() since spinlocks may be implemented using semaphores on some platforms. SpinlockSemaInit() was required to be called before InitShmemAllocation() since the latter initialized a spinlock to synchronize shared memory allocations. e25626677f8076eb3ce94586136c5464ee154381 removed the call to SpinlockSemaInit() from CreateSharedMemoryAndSemaphores() but left the call to PGReserveSemaphores() in CreateSharedMemoryAndSemaphores(), even though it fits in CreateOrAttachShmemStructs() along with the calls to other functions allocating shared memory structures. Add a comment explaining this absurdity. To be backpatched to PG 18. Author: Ashutosh Bapat <[email protected]> Discussion: https://www.postgresql.org/message-id/caexhw5seszppx-znjidvznzdagghok06f+ds88mppubxd1k...@mail.gmail.com --- src/backend/storage/ipc/ipci.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c index 2fa045e6b0f..ff88cc06443 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -225,7 +225,12 @@ CreateSharedMemoryAndSemaphores(void) InitShmemAccess(seghdr); /* - * Create semaphores + * Create semaphores. This is done here because of a now-non-existent + * dependency between spinlocks, which were required for shared memory + * allocation, and semaphores, which were allocated in the shared memory + * on some platforms. Ideally it should be done in + * CreateOrAttachShmemStructs() where we allocate other shared memory + * structures. */ PGReserveSemaphores(numSemas); base-commit: 99234e9ddc02f45eb122f83d49031e2c517d0af8 -- 2.34.1
From 63b65bc64b9e8a3530cce1d0d852cafc9dff727e Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <[email protected]> Date: Mon, 25 Aug 2025 14:33:53 +0530 Subject: [PATCH 2/2] Refactor shared memory allocation for semaphores Before e25626677f8076eb3ce94586136c5464ee154381 spinlocks were implemented using semaphores on some platforms. Hence PGReserveSemaphores() was required to be called before SpinlockSemaInit(). SpinlockSemaInit() was required to be called before InitShmemAllocation() since the latter initialized a spinlock to synchronize shared memory allocations. A cyclic dependency existed because PGReserveSemaphores() uses shared memory for semaphores on some platforms. It was broken by letting PGReserveSemaphores() allocate shared memory without a spinlock using ShmemAllocUnlocked(). After e25626677f8076eb3ce94586136c5464ee154381, spinlocks are not implemented using semaphores at all. This allows us to eliminate the need to estimate and allocate shared memory required by semaphores specially and treat them similar to the other shared memory structures. Since the semaphores are used only in PGPROC array, it makes more sense to estimate size of memory required by semaphores to go along with the estimation of memory required for that array in ProcGlobalShmemSize(). Similarly, it makes sense to have the actual shared memory allocations for both to be collocated in InitProcGlobal(). With that change CalculateShmemSize() does not need to know about the number of semaphores to be allocated, eliminating other asymmetry. InitializeShmemGUCs() which used to get the number of semaphores through CalculateShmemSize() now fetches that value separately. Author: Ashutosh Bapat <[email protected]> Discussion: https://www.postgresql.org/message-id/caexhw5seszppx-znjidvznzdagghok06f+ds88mppubxd1k...@mail.gmail.com --- src/backend/port/posix_sema.c | 6 +----- src/backend/port/sysv_sema.c | 6 +----- src/backend/storage/ipc/ipci.c | 29 ++++------------------------- src/backend/storage/lmgr/proc.c | 4 ++++ src/include/storage/ipc.h | 2 +- 5 files changed, 11 insertions(+), 36 deletions(-) diff --git a/src/backend/port/posix_sema.c b/src/backend/port/posix_sema.c index 269c7460817..d7fb0c0c4da 100644 --- a/src/backend/port/posix_sema.c +++ b/src/backend/port/posix_sema.c @@ -215,12 +215,8 @@ PGReserveSemaphores(int maxSemas) elog(PANIC, "out of memory"); #else - /* - * We must use ShmemAllocUnlocked(), since the spinlock protecting - * ShmemAlloc() won't be ready yet. - */ sharedSemas = (PGSemaphore) - ShmemAllocUnlocked(PGSemaphoreShmemSize(maxSemas)); + ShmemAlloc(PGSemaphoreShmemSize(maxSemas)); #endif numSems = 0; diff --git a/src/backend/port/sysv_sema.c b/src/backend/port/sysv_sema.c index 6ac83ea1a82..9faaeeefc79 100644 --- a/src/backend/port/sysv_sema.c +++ b/src/backend/port/sysv_sema.c @@ -343,12 +343,8 @@ PGReserveSemaphores(int maxSemas) errmsg("could not stat data directory \"%s\": %m", DataDir))); - /* - * We must use ShmemAllocUnlocked(), since the spinlock protecting - * ShmemAlloc() won't be ready yet. - */ sharedSemas = (PGSemaphore) - ShmemAllocUnlocked(PGSemaphoreShmemSize(maxSemas)); + ShmemAlloc(PGSemaphoreShmemSize(maxSemas)); numSharedSemas = 0; maxSharedSemas = maxSemas; diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c index ff88cc06443..81bbb7cefaf 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -86,17 +86,9 @@ RequestAddinShmemSpace(Size size) * required. */ Size -CalculateShmemSize(int *num_semaphores) +CalculateShmemSize(void) { Size size; - int numSemas; - - /* Compute number of semaphores we'll need */ - numSemas = ProcGlobalSemas(); - - /* Return the number of semaphores if requested by the caller */ - if (num_semaphores) - *num_semaphores = numSemas; /* * Size of the Postgres shared-memory block is estimated via moderately- @@ -108,7 +100,6 @@ CalculateShmemSize(int *num_semaphores) * during the actual allocation phase. */ size = 100000; - size = add_size(size, PGSemaphoreShmemSize(numSemas)); size = add_size(size, hash_estimate_size(SHMEM_INDEX_SIZE, sizeof(ShmemIndexEnt))); size = add_size(size, dsm_estimate_size()); @@ -202,12 +193,11 @@ CreateSharedMemoryAndSemaphores(void) PGShmemHeader *shim; PGShmemHeader *seghdr; Size size; - int numSemas; Assert(!IsUnderPostmaster); /* Compute the size of the shared-memory block */ - size = CalculateShmemSize(&numSemas); + size = CalculateShmemSize(); elog(DEBUG3, "invoking IpcMemoryCreate(size=%zu)", size); /* @@ -224,16 +214,6 @@ CreateSharedMemoryAndSemaphores(void) InitShmemAccess(seghdr); - /* - * Create semaphores. This is done here because of a now-non-existent - * dependency between spinlocks, which were required for shared memory - * allocation, and semaphores, which were allocated in the shared memory - * on some platforms. Ideally it should be done in - * CreateOrAttachShmemStructs() where we allocate other shared memory - * structures. - */ - PGReserveSemaphores(numSemas); - /* * Set up shared memory allocation mechanism */ @@ -363,12 +343,11 @@ InitializeShmemGUCs(void) Size size_b; Size size_mb; Size hp_size; - int num_semas; /* * Calculate the shared memory size and round up to the nearest megabyte. */ - size_b = CalculateShmemSize(&num_semas); + size_b = CalculateShmemSize(); size_mb = add_size(size_b, (1024 * 1024) - 1) / (1024 * 1024); sprintf(buf, "%zu", size_mb); SetConfigOption("shared_memory_size", buf, @@ -388,6 +367,6 @@ InitializeShmemGUCs(void) PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); } - sprintf(buf, "%d", num_semas); + sprintf(buf, "%d", ProcGlobalSemas()); SetConfigOption("num_os_semaphores", buf, PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); } diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index e9ef0fbfe32..1893cfeba64 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -144,6 +144,7 @@ ProcGlobalShmemSize(void) size = add_size(size, sizeof(PROC_HDR)); size = add_size(size, sizeof(slock_t)); + size = add_size(size, PGSemaphoreShmemSize(ProcGlobalSemas())); size = add_size(size, PGProcShmemSize()); size = add_size(size, FastPathLockShmemSize()); @@ -286,6 +287,9 @@ InitProcGlobal(void) /* For asserts checking we did not overflow. */ fpEndPtr = fpPtr + requestSize; + /* Reserve space for semaphores. */ + PGReserveSemaphores(ProcGlobalSemas()); + for (i = 0; i < TotalProcs; i++) { PGPROC *proc = &procs[i]; diff --git a/src/include/storage/ipc.h b/src/include/storage/ipc.h index 3baf418b3d1..2a8a8f0eabd 100644 --- a/src/include/storage/ipc.h +++ b/src/include/storage/ipc.h @@ -77,7 +77,7 @@ extern void check_on_shmem_exit_lists_are_empty(void); /* ipci.c */ extern PGDLLIMPORT shmem_startup_hook_type shmem_startup_hook; -extern Size CalculateShmemSize(int *num_semaphores); +extern Size CalculateShmemSize(void); extern void CreateSharedMemoryAndSemaphores(void); #ifdef EXEC_BACKEND extern void AttachSharedMemoryStructs(void); -- 2.34.1
