From 1ca312686e4c4b270cb598429983a07c73a5c44a Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 9 Oct 2023 11:23:47 +0300 Subject: [PATCH] Clarify the checks in RegisterBackgroundWorker. In EXEC_BACKEND or single-user mode, we process shared_preload_libraries at postmaster startup as usual, but also at backend startup. When a library calls RegisterBackgroundWorker() when being loaded into a backend process, we go through the motions to add the worker to BackgroundWorkerList, even though that is a postmaster-private data structure. Make it return early when called in a backend process, without changing BackgroundWorkerList. You could argue that it was intentional: In non-EXEC_BACKEND mode, the backend processes inherit BackgroundWorkerList at fork(), so it does make some sense to initialize it to the same state in EXEC_BACKEND mode, too. It's clearly a postmaster-private structure, though, and all the functions that use it are clearly marked as "should only be called in postmaster". You could also argue that libraries should not call RegisterBackgroundWorker() during backend startup. It's too late to correctly register any static background workers at that stage. But it's a common pattern in extensions, and it doesn't seem worth the churn to require all extensions to change it. Another sloppiness was the exception for "internal" background workers. We checked that RegisterBackgroundWorker() was called during shared_preload_libraries processing, or the background worker was an internal one. That exception was made in commit 665d1fad99 to allow postmaster to register the logical apply launcher in ApplyLauncherRegister(). The way the check was written, it would not complain if you registered an internal background worker in a regular backend process. But it would complain if postmaster registered a background worker defined in a shared library, outside shared_preload_libraries processing. I think the correct rule is that you can only register static background workers in the postmaster process, and only before the bgworker shared memory array has been initialized. Check for that more directly. Reviewed-by: Thomas Munro Discussion: https://www.postgresql.org/message-id/4f95c1fc-ad3c-7974-3a8c-6faa3931804c@iki.fi --- src/backend/postmaster/bgworker.c | 44 +++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c index 505e38376c..31350d6401 100644 --- a/src/backend/postmaster/bgworker.c +++ b/src/backend/postmaster/bgworker.c @@ -880,21 +880,43 @@ RegisterBackgroundWorker(BackgroundWorker *worker) RegisteredBgWorker *rw; static int numworkers = 0; - if (!IsUnderPostmaster) - ereport(DEBUG1, - (errmsg_internal("registering background worker \"%s\"", worker->bgw_name))); - - if (!process_shared_preload_libraries_in_progress && - strcmp(worker->bgw_library_name, "postgres") != 0) + /* + * Static background workers can only be registered in the postmaster + * process. + */ + if (IsUnderPostmaster || !IsPostmasterEnvironment) { - if (!IsUnderPostmaster) - ereport(LOG, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("background worker \"%s\": must be registered in shared_preload_libraries", - worker->bgw_name))); + /* + * In EXEC_BACKEND or single-user mode, we process + * shared_preload_libraries in backend processes too. We cannot + * register static background workers at that stage, but many + * libraries' _PG_init() functions don't distinguish whether they're + * being loaded in the postmaster or in a backend, they just check + * process_shared_preload_libraries_in_progress. It's a bit sloppy, + * but for historical reasons we tolerate it. In EXEC_BACKEND mode, + * the background workers should already have been registered when the + * library was loaded in postmaster. + */ + if (process_shared_preload_libraries_in_progress) + return; + ereport(LOG, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("background worker \"%s\": must be registered in shared_preload_libraries", + worker->bgw_name))); return; } + /* + * Cannot register static background workers after calling + * BackgroundWorkerShmemInit(). + */ + if (BackgroundWorkerData != NULL) + elog(ERROR, "cannot register background worker \"%s\" after shmem init", + worker->bgw_name); + + ereport(DEBUG1, + (errmsg_internal("registering background worker \"%s\"", worker->bgw_name))); + if (!SanityCheckBackgroundWorker(worker, LOG)) return;