shmem_startup_hook called twice on Windows
Hi,
While working on a related area, I noticed that shmem_startup_hook
is called twice under EXEC_BACKEND.
The first call occurs in CreateSharedMemoryAndSemaphores() during
postmaster startup (!IsUnderPostmaster), which is expected.
The second call happens in AttachSharedMemoryStructs() (when
EXEC_BACKEND is defined), and this occurs in normal backends
(IsUnderPostmaster).
The second call does not seem correct. The startup hook that should
only run during
postmaster initialization, AFAIK.
Blame shows that this change was introduced in commit 69d903367c,
but I could not determine the rationale from the discussion,
so it may have been an oversight.
Thoughts?
--
Sami Imseih
Amazon Web Services (AWS)
On Fri, Aug 15, 2025 at 09:17:22AM -0500, Sami Imseih wrote:
While working on a related area, I noticed that shmem_startup_hook
is called twice under EXEC_BACKEND.The first call occurs in CreateSharedMemoryAndSemaphores() during
postmaster startup (!IsUnderPostmaster), which is expected.The second call happens in AttachSharedMemoryStructs() (when
EXEC_BACKEND is defined), and this occurs in normal backends
(IsUnderPostmaster).The second call does not seem correct. The startup hook that should
only run during
postmaster initialization, AFAIK.Blame shows that this change was introduced in commit 69d903367c,
but I could not determine the rationale from the discussion,
so it may have been an oversight.
I think the startup hook must run in each backend for EXEC_BACKEND, else we
won't properly initialize pointers to shared memory in that case, right?
I added the following wording in commit 964152c:
+ <literal>shmem_startup_hook</literal> provides a convenient place for the
+ initialization code, but it is not strictly required that all such code
+ be placed in this hook. Each backend will execute the registered
+ <literal>shmem_startup_hook</literal> shortly after it attaches to shared
+ memory. Note that add-ins should still acquire
+ <function>AddinShmemInitLock</function> within this hook, as shown in the
+ example above.IIUC commit 69d9033 actually makes that inaccurate for the non-EXEC_BACKEND
case. Presumably this is okay because we don't need to re-initialize
pointers to shmem when forking. I must've missed this change when updating
the documentation.
--
nathan
While working on a related area, I noticed that shmem_startup_hook
is called twice under EXEC_BACKEND.The first call occurs in CreateSharedMemoryAndSemaphores() during
postmaster startup (!IsUnderPostmaster), which is expected.The second call happens in AttachSharedMemoryStructs() (when
EXEC_BACKEND is defined), and this occurs in normal backends
(IsUnderPostmaster).The second call does not seem correct. The startup hook that should
only run during
postmaster initialization, AFAIK.Blame shows that this change was introduced in commit 69d903367c,
but I could not determine the rationale from the discussion,
so it may have been an oversight.
I think the startup hook must run in each backend for EXEC_BACKEND, else we
won't properly initialize pointers to shared memory in that case,
right? I guess the
doc below is giving a vague warning that one should be careful what they
put in that hook.
I added the following wording in commit 964152c:
IIUC commit 69d9033 actually makes that inaccurate for the non-EXEC_BACKEND
case. Presumably this is okay because we don't need to re-initialize
pointers to shmem when forking. I must've missed this change when updating
the documentation.
Thanks, I missed the doc update. Yes, that is inconsistent between platforms,
and if we must live with this behavior, should the doc give a bigger warning
about the code that goes in that hook?
--
Sami
Sorry my last reply got mangled for some reason. Here it is again.
Blame shows that this change was introduced in commit 69d903367c,
but I could not determine the rationale from the discussion,
so it may have been an oversight.I think the startup hook must run in each backend for EXEC_BACKEND, else we
won't properly initialize pointers to shared memory in that case,
right? I guess the
doc below is giving a vague warning that one should be careful what they
put in that hook.
But that could potentially be dangerous if code in the startup hook
gets re-executed?
I guess the
doc below is giving a vague warning that one should be careful what they
put in that hook.
I added the following wording in commit 964152c:
Thanks, I missed the doc update. Yes, that is inconsistent between platforms,
and if we must live with this behavior, should the doc give a bigger warning
about the code that goes in that hook?
Thanks, I missed the doc update. Yes, that is inconsistent between platforms,
and if we must live with this behavior, should the doc give a bigger warning
about the code that goes in that hook?
--
Sami
On Fri, Aug 15, 2025 at 10:36:47AM -0500, Sami Imseih wrote:
But that could potentially be dangerous if code in the startup hook gets
re-executed? I guess the doc below is giving a vague warning that one
should be careful what they put in that hook.
The docs seem reasonably clear that these hooks need to be careful to not
re-initialize shared memory that was already initialized by another backend
[0]: https://www.postgresql.org/docs/devel/xfunc-c.html#XFUNC-SHARED-ADDIN
Thanks, I missed the doc update. Yes, that is inconsistent between platforms,
and if we must live with this behavior, should the doc give a bigger warning
about the code that goes in that hook?
The docs should definitely be updated for accuracy, but I'm not following
what sort of additional warning you think we need. Could you share a
concrete example of what you have in mind?
[0]: https://www.postgresql.org/docs/devel/xfunc-c.html#XFUNC-SHARED-ADDIN
--
nathan
On Fri, Aug 15, 2025 at 10:36:47AM -0500, Sami Imseih wrote:
But that could potentially be dangerous if code in the startup hook gets
re-executed? I guess the doc below is giving a vague warning that one
should be careful what they put in that hook.The docs seem reasonably clear that these hooks need to be careful to not
re-initialize shared memory that was already initialized by another backend
[0].Thanks, I missed the doc update. Yes, that is inconsistent between platforms,
and if we must live with this behavior, should the doc give a bigger warning
about the code that goes in that hook?The docs should definitely be updated for accuracy, but I'm not following
what sort of additional warning you think we need. Could you share a
concrete example of what you have in mind?
I noticed a few things where this behavior becomes very suspicious.
For example, in pgss_startup_hook, every time startup_hook is run
we take an exclusive LW lock. so, all backends now may be competing
for that lock by nature of connection establishment.
test_slru calls LWLockNewTrancheId inside that hook.
So, this tells me that the caller needs to be aware of such caveats.
I am thinking something like this:
"Because this hook is executed by the postmaster and invoked by backends via
EXEC_BACKEND, it is essential to ensure that any code intended to run only
during postmaster startup is properly protected against repeated execution.
This can be enforced by verifying !IsUnderPostmaster before invocation."
--
Sami
On Fri, Aug 15, 2025 at 11:25:55AM -0500, Sami Imseih wrote:
I noticed a few things where this behavior becomes very suspicious.
For example, in pgss_startup_hook, every time startup_hook is run
we take an exclusive LW lock. so, all backends now may be competing
for that lock by nature of connection establishment.
I suspect there's enough overhead in connection establishment for
contention to be unlikely. In any case, I'm not aware of any complaints
about this.
test_slru calls LWLockNewTrancheId inside that hook.
Hm. At first glance, that does seem bogus for EXEC_BACKEND builds. I
think the only side effect is extra tranche ID allocations and missing
tranche names, as SimpleLruInit() forgoes any shared memory initialization
in non-postmaster backends.
So, this tells me that the caller needs to be aware of such caveats.
I am thinking something like this:
"Because this hook is executed by the postmaster and invoked by backends via
EXEC_BACKEND, it is essential to ensure that any code intended to run only
during postmaster startup is properly protected against repeated execution.
This can be enforced by verifying !IsUnderPostmaster before invocation."
IMHO we should avoid talking about EXEC_BACKEND, etc. and instead make it
clear that hooks should be prepared to deal with concurrent invocations
from other backends. But taking a step back, I'm still not entirely clear
what this adds to the existing documentation, which is pretty direct about
the need for locking and how to avoid re-initializing.
--
nathan
"Because this hook is executed by the postmaster and invoked by backends via
EXEC_BACKEND, it is essential to ensure that any code intended to run only
during postmaster startup is properly protected against repeated execution.
This can be enforced by verifying !IsUnderPostmaster before invocation."IMHO we should avoid talking about EXEC_BACKEND, etc. and instead make it
clear that hooks should be prepared to deal with concurrent invocations
from other backends. But taking a step back, I'm still not entirely clear
what this adds to the existing documentation, which is pretty direct about
the need for locking and how to avoid re-initializing.
I am not sure. I read this
""
If this function sets foundPtr to false, the caller should proceed to
initialize the contents of the reserved shared memory. If foundPtr is
set to true,
the shared memory was already initialized by another backend, and the caller
need not initialize further.
"""
and it's related to ShmemInitStruct specifically.
Imagine someone adds some code in there that does more than just
ShmemInitStruct. This code will be run multiple times on EXEC_BACKEND
vs once on !EXEC_BACKEND
IMO, that is quite a large difference in behavior that should be clearly noted.
--
Sami
I quickly put together a patch for the stuff we've discussed in this
thread. WDYT?
--
nathan
Attachments:
v1-0001-Fix-shmem_startup_hook-documentation.patchtext/plain; charset=us-asciiDownload
From 11289e1e69fc7c6fdbe5a73483efc2daf1197ec9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 8 Sep 2025 16:08:00 -0500
Subject: [PATCH v1 1/1] Fix shmem_startup_hook documentation.
---
doc/src/sgml/xfunc.sgml | 10 ++++++----
src/test/modules/test_slru/test_slru.c | 21 +++++++++++++++------
2 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index da21ef56891..80d862ff9c2 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3668,11 +3668,13 @@ LWLockRelease(AddinShmemInitLock);
</programlisting>
<literal>shmem_startup_hook</literal> provides a convenient place for the
initialization code, but it is not strictly required that all such code
- be placed in this hook. Each backend will execute the registered
- <literal>shmem_startup_hook</literal> shortly after it attaches to shared
- memory. Note that add-ins should still acquire
+ be placed in this hook. In some builds, each backend will execute the
+ registered <literal>shmem_startup_hook</literal> shortly after it
+ attaches to shared memory, so add-ins should still acquire
<function>AddinShmemInitLock</function> within this hook, as shown in the
- example above.
+ example above. In other builds, the postmaster process will execute the
+ <literal>shmem_startup_hook</literal> once, and each backend will
+ automatically inherit the pointers to shared memory.
</para>
<para>
diff --git a/src/test/modules/test_slru/test_slru.c b/src/test/modules/test_slru/test_slru.c
index 8c0367eeee4..f41422cca7d 100644
--- a/src/test/modules/test_slru/test_slru.c
+++ b/src/test/modules/test_slru/test_slru.c
@@ -219,8 +219,8 @@ test_slru_shmem_startup(void)
*/
const bool long_segment_names = true;
const char slru_dir_name[] = "pg_test_slru";
- int test_tranche_id;
- int test_buffer_tranche_id;
+ int test_tranche_id = -1;
+ int test_buffer_tranche_id = -1;
if (prev_shmem_startup_hook)
prev_shmem_startup_hook();
@@ -231,10 +231,19 @@ test_slru_shmem_startup(void)
*/
(void) MakePGDirectory(slru_dir_name);
- /* initialize the SLRU facility */
- test_tranche_id = LWLockNewTrancheId("test_slru_tranche");
-
- test_buffer_tranche_id = LWLockNewTrancheId("test_buffer_tranche");
+ /*
+ * Initialize the SLRU facility.
+ *
+ * In EXEC_BACKEND builds, the shmem_startup_hook is called in the
+ * postmaster and in each backend, but we only need to generate the LWLock
+ * tranches once. Note that these IDs are not used by SimpleLruInit() in
+ * the IsUnderPostmaster (i.e., backend) case.
+ */
+ if (!IsUnderPostmaster)
+ {
+ test_tranche_id = LWLockNewTrancheId("test_slru_tranche");
+ test_buffer_tranche_id = LWLockNewTrancheId("test_buffer_tranche");
+ }
TestSlruCtl->PagePrecedes = test_slru_page_precedes_logically;
SimpleLruInit(TestSlruCtl, "TestSLRU",
--
2.39.5 (Apple Git-154)
I quickly put together a patch for the stuff we've discussed in this
thread. WDYT?--
nathan
I still think we need to mention EXEC_BACKEND somehow.
The way it's done in [0]https://www.postgresql.org/docs/current/bgworker.html, it says "On Windows (and anywhere else
where EXEC_BACKEND is defined)"
So we do have precedent of mentioning EXEC_BACKEND in docs,
and it’s clearer than the ambiguity of saying
'on some builds'/'in other builds'.
[0]: https://www.postgresql.org/docs/current/bgworker.html
--
Sami
On Mon, Sep 08, 2025 at 04:31:43PM -0500, Sami Imseih wrote:
I still think we need to mention EXEC_BACKEND somehow. The way it's done
in [0], it says "On Windows (and anywhere else where EXEC_BACKEND is
defined)"So we do have precedent of mentioning EXEC_BACKEND in docs, and it’s
clearer than the ambiguity of saying 'on some builds'/'in other builds'.
Added in v2.
--
nathan
Attachments:
v2-0001-Fix-shmem_startup_hook-documentation.patchtext/plain; charset=us-asciiDownload
From 2ed006e00e011707a1b86c31f13f7366590e56d9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 8 Sep 2025 16:08:00 -0500
Subject: [PATCH v2 1/1] Fix shmem_startup_hook documentation.
---
doc/src/sgml/xfunc.sgml | 11 +++++++----
src/test/modules/test_slru/test_slru.c | 21 +++++++++++++++------
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index da21ef56891..6c9b447c667 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3668,11 +3668,14 @@ LWLockRelease(AddinShmemInitLock);
</programlisting>
<literal>shmem_startup_hook</literal> provides a convenient place for the
initialization code, but it is not strictly required that all such code
- be placed in this hook. Each backend will execute the registered
- <literal>shmem_startup_hook</literal> shortly after it attaches to shared
- memory. Note that add-ins should still acquire
+ be placed in this hook. On Windows (and anywhere else where
+ <literal>EXEC_BACKEND</literal> is defined), each backend executes the
+ registered <literal>shmem_startup_hook</literal> shortly after it
+ attaches to shared memory, so add-ins should still acquire
<function>AddinShmemInitLock</function> within this hook, as shown in the
- example above.
+ example above. On other platforms, the postmaster process executes the
+ <literal>shmem_startup_hook</literal> once, and each backend
+ automatically inherits the pointers to shared memory.
</para>
<para>
diff --git a/src/test/modules/test_slru/test_slru.c b/src/test/modules/test_slru/test_slru.c
index 8c0367eeee4..f41422cca7d 100644
--- a/src/test/modules/test_slru/test_slru.c
+++ b/src/test/modules/test_slru/test_slru.c
@@ -219,8 +219,8 @@ test_slru_shmem_startup(void)
*/
const bool long_segment_names = true;
const char slru_dir_name[] = "pg_test_slru";
- int test_tranche_id;
- int test_buffer_tranche_id;
+ int test_tranche_id = -1;
+ int test_buffer_tranche_id = -1;
if (prev_shmem_startup_hook)
prev_shmem_startup_hook();
@@ -231,10 +231,19 @@ test_slru_shmem_startup(void)
*/
(void) MakePGDirectory(slru_dir_name);
- /* initialize the SLRU facility */
- test_tranche_id = LWLockNewTrancheId("test_slru_tranche");
-
- test_buffer_tranche_id = LWLockNewTrancheId("test_buffer_tranche");
+ /*
+ * Initialize the SLRU facility.
+ *
+ * In EXEC_BACKEND builds, the shmem_startup_hook is called in the
+ * postmaster and in each backend, but we only need to generate the LWLock
+ * tranches once. Note that these IDs are not used by SimpleLruInit() in
+ * the IsUnderPostmaster (i.e., backend) case.
+ */
+ if (!IsUnderPostmaster)
+ {
+ test_tranche_id = LWLockNewTrancheId("test_slru_tranche");
+ test_buffer_tranche_id = LWLockNewTrancheId("test_buffer_tranche");
+ }
TestSlruCtl->PagePrecedes = test_slru_page_precedes_logically;
SimpleLruInit(TestSlruCtl, "TestSLRU",
--
2.39.5 (Apple Git-154)