Hi, PGReserveSemaphores() allocates shared memory space for semaphores. I was expecting it to be part of CreateOrAttachShmemStructs() and not be directly called from CreateSharedMemoryAndSemaphores(). But before e25626677f8076eb3ce94586136c5464ee154381, it was required to be called before SpinlockSemaInit() since spinlock may require semaphores. SpinlockSemaInit() was required to be called before InitShmemAllocation() since the latter initialized a spinlock to synchronize shared memory allocations. PGReserveSemaphores() used ShmemAllocUnlocked() to break the cyclic dependency and allocate shared memory without a spinlock. e25626677f8076eb3ce94586136c5464ee154381 removed the call to SpinlockSemaInit() from CreateSharedMemoryAndSemaphores() and modified following comment in PGReserveSemaphores(). /* * We must use ShmemAllocUnlocked(), since the spinlock protecting - * ShmemAlloc() won't be ready yet. (This ordering is necessary when we - * are emulating spinlocks with semaphores.) + * ShmemAlloc() won't be ready yet. */ It looks like we don't use semaphores for spinlocks anymore. Hence PGReserveSemaphores() doesn't need to be called before InitShmemAllocation(). In turn, it doesn't need to be directly called from CreateSharedMemoryAndSemaphores(). Instead it could be called from CreateOrAttachShmemStructs() where rest of the shared memory allocations happen. And it can use ShmemAlloc() instead of ShmemAllocUnlocked() like all other shared memory allocations. If we do that there is clean separation between shared memory creation, initialization and allocations.
I happened to look at this because of the v5-0004 patch in the patchset posted in [1]. That patch uses multiple shared memory mappings, each of which needs to be set up for shared memory allocations. So we need to call functions PGSharedMemoryCreate(), InitShmemAccess(), InitShmemAllocation() for each of the shared memory segments. PGReserveSemaphores(), which is in that sequence right now, is needed only for the main shared memory segment and thus needs to be singled out. But moving it to CreateOrAttachShmemStructs() removes that asymmetry and also does not need a comment explaining why it's called only for the main memory in that sequence. Attached patch has that change. WIth that change I did not see any failures when running regression on my Ubuntu laptop. Also we could push the change down into InitProcGlobal() where we actually create semaphores and which is already called when if (!IsUnderPostmaster). 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? 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] https://www.postgresql.org/message-id/my4hukmejato53ef465ev7lk3sqiqvneh7436rz64wmtc7rbfj%40hmuxsf2ngov2 -- Best Wishes, Ashutosh Bapat
From b38f10aa05c3ac3e9737c6c4ac7a481cece58ce8 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <[email protected]> Date: Mon, 25 Aug 2025 14:33:53 +0530 Subject: [PATCH] WIP: Move PGReserveSemaphores call to CreateOrAttachShmemStructs After e25626677f8076eb3ce94586136c5464ee154381, we don't need to call PGReserveSemaphores() before InitShmemAllocation() in CreateSharedMemoryAndSemaphores(). Instead it can be called from CreateOrAttachShmemStructs() where all the shared memory allocations happen. Author: Ashutosh Bapat <[email protected]> --- src/backend/port/posix_sema.c | 6 +----- src/backend/port/sysv_sema.c | 6 +----- src/backend/storage/ipc/ipci.c | 12 +++++++----- 3 files changed, 9 insertions(+), 15 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 2fa045e6b0f..c4ccce45b59 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -224,11 +224,6 @@ CreateSharedMemoryAndSemaphores(void) InitShmemAccess(seghdr); - /* - * Create semaphores - */ - PGReserveSemaphores(numSemas); - /* * Set up shared memory allocation mechanism */ @@ -265,6 +260,13 @@ CreateSharedMemoryAndSemaphores(void) static void CreateOrAttachShmemStructs(void) { + + if (!IsUnderPostmaster) + { + /* Set up semaphores */ + PGReserveSemaphores(ProcGlobalSemas()); + } + /* * Now initialize LWLocks, which do shared memory allocation and are * needed for InitShmemIndex. base-commit: 878656dbde0d2fd4b85a8c63566e2a9f0d0f4952 -- 2.34.1
