Обсуждение: 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]. 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

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