Обсуждение: 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
Вложения
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