public inbox for [email protected]
help / color / mirror / Atom feedFrom: Heikki Linnakangas <[email protected]>
To: Ashutosh Bapat <[email protected]>
Cc: Dagfinn Ilmari Mannsåker <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: [email protected]
Subject: Re: Better shared data structure management and resizable shared data structures
Date: Fri, 12 Jun 2026 18:37:21 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAExHW5uDJdmfUVX=meyecbY2sGKq07Y33NQ68Wv2-ktaag=9qg@mail.gmail.com>
References: <CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com>
<[email protected]>
<CAExHW5uTNWOSxJDWQAUnS0tZawob2_J3dRAtc67NHNZ98X4_xA@mail.gmail.com>
<CAExHW5t439y61YD9bc7d5wZWHp6J=M43Qu3eEZOBPguZML7o2A@mail.gmail.com>
<CAExHW5v5FVZbsO9sLzztMZ11C3hgGStE=HkkV2bQkCyncess4w@mail.gmail.com>
<[email protected]>
<CAExHW5tCC0T1ky=Jnq-AvMxa67Adaw7aQ4iQAO=BSdHcbSNBVg@mail.gmail.com>
<[email protected]>
<CAExHW5tS7GncN90oJWOSzW_3F1EHL9xwe59L7Req3nUVgmObUw@mail.gmail.com>
<[email protected]>
<CAExHW5sYgt=XekOoAE-Tu_Dv8obWZCjCqAPT9vtN2D4m=M=drQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAExHW5uMQGvQH6GKaBZVtH4S9O13TwN+_0Vy1gUpAW=_T_AmRA@mail.gmail.com>
<[email protected]>
<CAExHW5uDJdmfUVX=meyecbY2sGKq07Y33NQ68Wv2-ktaag=9qg@mail.gmail.com>
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/[email protected]...
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.
- Heikki
Attachments:
[text/x-patch] improve-shmem-init-callback-locking-docs.patch (1.6K, 2-improve-shmem-init-callback-locking-docs.patch)
download | inline diff:
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index bae16d7fb53..cb3cc09f16d 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3739,8 +3739,8 @@ my_shmem_init(void *arg)
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.
+ lock (ShmemIndexLock), which prevents the race condition of two backends
+ trying to initializing the memory area at the same time.
</para>
</sect3>
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index f1f7cd3a4ff..1fbba9c3a4c 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -918,7 +918,10 @@ CallShmemCallbacksAfterStartup(const ShmemCallbacks *callbacks)
return;
}
- /* Hold ShmemIndexLock while we allocate all the shmem entries */
+ /*
+ * Hold ShmemIndexLock while we allocate all the shmem entries and run all
+ * the initializers.
+ */
LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE);
/*
@@ -937,7 +940,7 @@ CallShmemCallbacksAfterStartup(const ShmemCallbacks *callbacks)
notfound_any = true;
}
if (found_any && notfound_any)
- elog(ERROR, "found some but not all");
+ elog(ERROR, "some of the requested shmem areas have already been initialized");
/*
* Allocate or attach all the shmem areas requested by the request_fn
view thread (82+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Better shared data structure management and resizable shared data structures
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox