Re: Calling PGReserveSemaphores() from CreateOrAttachShmemStructs

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