Обсуждение: 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]. > 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
Вложения
> 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
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
Вложения
> Added in v2. v2 LGTM. -- Sami
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