Calling PGReserveSemaphores() from CreateOrAttachShmemStructs
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]/messages/by-id/my4hukmejato53ef465ev7lk3sqiqvneh7436rz64wmtc7rbfj@hmuxsf2ngov2. 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]: /messages/by-id/my4hukmejato53ef465ev7lk3sqiqvneh7436rz64wmtc7rbfj@hmuxsf2ngov2
--
Best Wishes,
Ashutosh Bapat
Attachments:
0001-WIP-Move-PGReserveSemaphores-call-to-Create-20250825.patchtext/x-patch; charset=US-ASCII; name=0001-WIP-Move-PGReserveSemaphores-call-to-Create-20250825.patchDownload
From b38f10aa05c3ac3e9737c6c4ac7a481cece58ce8 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
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 <ashutosh.bapat.oss@gmail.com>
---
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
On Mon, Aug 25, 2025 at 9:10 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> 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.
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!
Hi Thomas,
On Mon, Aug 25, 2025 at 3:11 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Mon, Aug 25, 2025 at 9:10 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> 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
Attachments:
0001-PGReserveSemaphores-called-from-CreateShare-20250826.patchtext/x-patch; charset=US-ASCII; name=0001-PGReserveSemaphores-called-from-CreateShare-20250826.patchDownload
From 80d441736035f26b13afc1da70d2f065c06252d7 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
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 <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAExHW5seSZpPx-znjidVZNzdagGHOk06F+Ds88MpPUbxd1kTaA@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
0002-Refactor-shared-memory-allocation-for-semap-20250826.patchtext/x-patch; charset=US-ASCII; name=0002-Refactor-shared-memory-allocation-for-semap-20250826.patchDownload
From 63b65bc64b9e8a3530cce1d0d852cafc9dff727e Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
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 <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAExHW5seSZpPx-znjidVZNzdagGHOk06F+Ds88MpPUbxd1kTaA@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
On 26/08/2025 10:11, Ashutosh Bapat wrote:
On Mon, Aug 25, 2025 at 3:11 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Mon, Aug 25, 2025 at 9:10 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> 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/
Looks good to me. I can commit this, but since Thomas said he'd review
it, I'll give him a few days for that if he finds the time.
- Heikki
On 31/10/2025 19:11, Heikki Linnakangas wrote:
On 26/08/2025 10:11, Ashutosh Bapat wrote:
On Mon, Aug 25, 2025 at 3:11 PM Thomas Munro <thomas.munro@gmail.com>
wrote:On Mon, Aug 25, 2025 at 9:10 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> 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/Looks good to me. I can commit this, but since Thomas said he'd review
it, I'll give him a few days for that if he finds the time.
Committed with minor changes: I made the comments a little less
detailed, updated the comment above CalculateShmemSize() now that it
doesn't have the 'num_semaphores' argument anymore, and made
ShmemAllocUnlocked static. Thanks!
- Heikki
On Thu, Nov 6, 2025 at 6:16 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 31/10/2025 19:11, Heikki Linnakangas wrote:
On 26/08/2025 10:11, Ashutosh Bapat wrote:
On Mon, Aug 25, 2025 at 3:11 PM Thomas Munro <thomas.munro@gmail.com>
wrote:On Mon, Aug 25, 2025 at 9:10 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> 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/Looks good to me. I can commit this, but since Thomas said he'd review
it, I'll give him a few days for that if he finds the time.Committed with minor changes: I made the comments a little less
detailed,
updated the comment above CalculateShmemSize() now that it
doesn't have the 'num_semaphores' argument anymore,
sorry for missing this.
and made
ShmemAllocUnlocked static. Thanks!
Thanks a lot. Helps me with shared buffer resizing patches.
--
Best Wishes,
Ashutosh Bapat
On Aug 26, 2025, at 15:11, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
Hi Thomas,
On Mon, Aug 25, 2025 at 3:11 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Mon, Aug 25, 2025 at 9:10 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> 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
<0001-PGReserveSemaphores-called-from-CreateShare-20250826.patch><0002-Refactor-shared-memory-allocation-for-semap-20250826.patch>
I just reviewed the patch and got a couple of comments:
1 - 0001
```
+ * 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);
```Looking into implementations of PGReserveSemaphores(), only win32 implementation use local memory, so can we be more specific, like changing “some platforms” to “non-windows platforms”?
2 - 0002
```
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());
```
As PGReserveSemaphores() on win32 doesn’t use shared memory, do we want to skip the new “add_size” for win32?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi Chao,
On Thu, Nov 6, 2025 at 8:53 PM Chao Li <li.evan.chao@gmail.com> wrote:
I just reviewed the patch and got a couple of comments:
1 - 0001 ``` + * 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); ```Looking into implementations of PGReserveSemaphores(), only win32 implementation use local memory, so can we be more specific, like changing “some platforms” to “non-windows platforms”?
Thanks for the comments. The patches were committed already. Please
feel free to propose changes again based on my response here.
Being specific has the advantage that people know where to look for,
instead of everywhere. But in case we grow such platforms tomorrow,
that advantage is null and could be even misleading. The risks of the
latter outweigh the advantages, I believe.
2 - 0002 ``` 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());
```As PGReserveSemaphores() on win32 doesn’t use shared memory, do we want to skip the new “add_size” for win32?
ProcGlobalShmemSize() defined in win32_sema.c returns 0. So the effect is same.
--
Best Wishes,
Ashutosh Bapat