The first patch on my "Refactoring backend fork+exec code" thread [0]
changes the allocations of BackgroundWorkerList from plain malloc() to
MemoryContextAlloc(PostmasterContext). However, that actually caused a
segfault in worker_spi tests in EXEC_BACKEND mode.
BackgroundWorkerList is a postmaster-private data structure and should
not be accessed in backends. That assumption failed in
RegisterBackgroundWorker(). When you put worker_spi in
shared_preload_libraries, its _PG_init() function calls
RegisterBackgroundWorker(), as expected. But in EXEC_BACKEND mode, the
library is loaded *again* in each backend process, and each of those
loads also call RegisterBackgroundWorker(). It's too late to correctly
register any static background workers at that stage, but
RegisterBackgroundWorker() still goes through the motions and adds the
element to BackgroundWorkerList. If you change the malloc() to
MemoryContextAlloc(PostmasterContext), it segfaults because
PostmasterContext == NULL in a backend process.
In summary, RegisterBackgroundWorker() is doing some questionable and
useless work, when a shared preload library is loaded to a backend
process in EXEC_BACKEND mode.
Attached patches:
1. Tighten/clarify those checks. See also commit message for details.
2. The patch from the other thread to switch to
MemoryContextAlloc(PostmasterContext)
3. A fix for a highly misleading comment in the same file.
Any comments?
[0]
https://www.postgresql.org/message-id/flat/7a59b073-5b5b-151e-7ed3-8b01ff7ce9ef%40iki.fi
P.S. In addition to those, I also considered these changes but didn't
implement them yet:
- Change RegisterBackgroundWorker() to return true/false to indicate
whether the registration succeeded. Currently, the caller has no way of
knowing. In many cases, I think even an ERROR and refusing to start up
the server would be appropriate. But at least we should let the caller
know and decide.
- Add "Assert(am_postmaster)" assertions to the functions in bgworker.c
that are only supposed to be called in postmaster process. The functions
have good explicit comments on that, but wouldn't hurt to also assert.
(There is no 'am_postmaster' flag, the equivalent is actually
(IsPostmasterEnvironment && !IsUnderPostmaster), but perhaps we should
define a macro or flag for that)
--
Heikki Linnakangas
Neon (https://neon.tech)
