Horton

Thread

Commits

  1. Fix documentation for shmem_startup_hook.

  2. test_slru: Fix LWLock tranche allocation in EXEC_BACKEND builds.

  3. doc: Reorganize section for shared memory and LWLocks.

  4. Refactor CreateSharedMemoryAndSemaphores

  1. shmem_startup_hook called twice on Windows

    Sami Imseih <samimseih@gmail.com> — 2025-08-15T14:17:22Z

    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)

  2. Re: shmem_startup_hook called twice on Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-15T15:20:00Z

    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

  3. Re: shmem_startup_hook called twice on Windows

    Sami Imseih <samimseih@gmail.com> — 2025-08-15T15:33:31Z

    > 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

  4. Re: shmem_startup_hook called twice on Windows

    Sami Imseih <samimseih@gmail.com> — 2025-08-15T15:36:47Z

    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

  5. Re: shmem_startup_hook called twice on Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-15T15:51:24Z

    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?

[0] https://www.postgresql.org/docs/devel/xfunc-c.html#XFUNC-SHARED-ADDIN

-- 
nathan

  6. Re: shmem_startup_hook called twice on Windows

    Sami Imseih <samimseih@gmail.com> — 2025-08-15T16:25:55Z

    > 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

  7. Re: shmem_startup_hook called twice on Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-15T18:46:39Z

    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

  8. Re: shmem_startup_hook called twice on Windows

    Sami Imseih <samimseih@gmail.com> — 2025-08-18T22:05:00Z

    > > "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

  9. Re: shmem_startup_hook called twice on Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2025-09-08T21:10:08Z

    I quickly put together a patch for the stuff we've discussed in this
thread.  WDYT?

-- 
nathan

  10. Re: shmem_startup_hook called twice on Windows

    Sami Imseih <samimseih@gmail.com> — 2025-09-08T21:31:43Z

    > 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], 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

  11. Re: shmem_startup_hook called twice on Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2025-09-08T21:45:43Z

    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

  12. Re: shmem_startup_hook called twice on Windows

    Sami Imseih <samimseih@gmail.com> — 2025-09-08T21:57:19Z

    > Added in v2.

v2 LGTM.

--
Sami

  13. Re: shmem_startup_hook called twice on Windows

    Nathan Bossart <nathandbossart@gmail.com> — 2025-09-09T19:41:53Z

    On Mon, Sep 08, 2025 at 04:57:19PM -0500, Sami Imseih wrote:
> v2 LGTM.

Committed, thanks for reviewing.  I ended up only back-patching the
documentation fix, as the test_slru bug is pretty innocuous.

-- 
nathan