On Fri, Jun 12, 2026 at 9:07 PM Heikki Linnakangas <[email protected]> wrote: > > On 21/04/2026 19:05, Ashutosh Bapat wrote: > > On Tue, Apr 21, 2026 at 1:10 PM Heikki Linnakangas <[email protected]> wrote: > >> > >> On 07/04/2026 17:19, Ashutosh Bapat wrote: > >>> Hi Heikki, > >>> CallShmemCallbacksAfterStartup() holds ShmemIndexLock while invoking > >>> init_fn/attach_fn callbacks. That looks wrong. Before this commit, > >>> init or attach code was not run with the lock held. Any reason the > >>> lock is held while calling init and attach callbacks. Since these > >>> function can come from extensions, we don't have control on what goes > >>> in those functions, and thus looks problematic. Further, it will > >>> serialize all the attach_fn executions across backends, since each > >>> will be run under the lock. > >> > >> This was intentional, I added a note in the docs about it: > >> > >> When <function>RegisterShmemCallbacks()</function> is called after > >> startup, it will immediately call the appropriate callbacks, > >> depending > >> on whether the requested memory areas were already initialized by > >> another backend. The callbacks will be called while holding an > >> internal > >> lock, which prevents concurrent two backends from initializing the > >> memory area concurrently. > >> > >> That "internal lock" is ShmemIndexLock. I piggybacked on that since the > >> code needs to acquire it anyway for the hash table lookups. > > > > I had read this part, but didn't realize it's ShmemIndexLock. The > > document and the code are placed far apart and the comments in the > > code do not help connecting these two. The comment before > > LWLockAcquire() call doesn't say anything about init functions. > > /* Hold ShmemIndexLock while we allocate all the shmem entries */ > > > >> With the old ShmemInitStruct() interface, extensions needed to do the > >> locking themselves, usually by holding AddinShmemInitLock. > >> > >> (Now that I read that again, the grammar on the last sentence sounds > >> awkward...) > > > > Given that the init_fn is called in only one backend which requests > > the structures first, do we need a lock? > > If two backends request the same structure concurrently, which one is > "first"? That's what the lock determines. > > It's not safe to release the lock before the init callback has finished. > Otherwise, another backend might attach to the struct before it's fully > initialized and read uninitialized values. > > >>> In my case, the init_fn was performing ShmemIndex lookup which > >>> deadlocked. It's questionable whether init function should lookup > >>> ShmemIndex but, it's not something that needs to be prohibited > >>> either. > >> Yeah I'm curious what the use case is. We could easily introduce another > >> lock or reuse AddinShmemInitLock for this. > > > > In case of resizable shared memory structures, I was adding mprotect > > to make sure that the part of the shared address space which is > > reserved but not used is protected from inadvertent access. The > > mprotect is wrapped in a shmem API which fetches the ShmemIndex entry > > of the shared structure, figures out the part of the address space to > > protect using maximum_size and current size and calls mprotect > > appropriately. To fetch the ShmemIndex entry it acquires a ShmemIndex > > lock. The shmem API was supposed to be called from init_fn() and > > attach_fn() to protect the address spaces as soon as the structure is > > attached to. See patches attached to [1] for code. > > > > [1] > > https://www.postgresql.org/message-id/CAExHW5v5muT_SKV2NCxxVmvC=_38rw0aiv-wu4cgzhabcry...@mail.gmail.com > > Ok. So if I understand correctly, holding ShmemIndexLock is not a actual > problem per se, you just didn't expect it. Right? > > I propose the attached to improve the wording a little on the docs, > comments, and error message.
The patch helps to set the expectations right. ShmemIndexLock is for protecting entries in ShmemIndex; I didn't expect it to protect the Shared structures as well. I thought a shared structure specific lock, which usually every shared structure is expected to have, would protect its initialization and content. But I see that I was wrong. Even those locks need to be initialized; so they can't be used here. ShmemIndexLock works here with the proposed comment changes. -- Best Wishes, Ashutosh Bapat
