Here's a new version of these patches. I fixed one comment and ran
pgindent, no other changes.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 1984e81c4191427fa2e358a664291edbf8d566ea Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 27 Sep 2023 23:39:09 +0300
Subject: [PATCH v2 1/3] 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.
---
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 505e38376c3..31350d64013 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;
--
2.39.2
From c28a16ccbc10eea01478a0f839bad0404870aba1 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Thu, 24 Aug 2023 18:08:44 +0300
Subject: [PATCH v2 2/3] Allocate Backend structs in PostmasterContext.
The child processes don't need them. By allocating them in
PostmasterContext, the memory gets free'd and is made available for
other stuff in the child processes.
---
src/backend/postmaster/bgworker.c | 11 ++++++++---
src/backend/postmaster/postmaster.c | 24 +++++++++++-------------
2 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 31350d64013..cc66c61dee7 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -33,6 +33,7 @@
#include "storage/shmem.h"
#include "tcop/tcopprot.h"
#include "utils/ascii.h"
+#include "utils/memutils.h"
#include "utils/ps_status.h"
#include "utils/timeout.h"
@@ -347,7 +348,9 @@ BackgroundWorkerStateChange(bool allow_new_workers)
/*
* Copy the registration data into the registered workers list.
*/
- rw = malloc(sizeof(RegisteredBgWorker));
+ rw = MemoryContextAllocExtended(PostmasterContext,
+ sizeof(RegisteredBgWorker),
+ MCXT_ALLOC_NO_OOM);
if (rw == NULL)
{
ereport(LOG,
@@ -455,7 +458,7 @@ ForgetBackgroundWorker(slist_mutable_iter *cur)
rw->rw_worker.bgw_name)));
slist_delete_current(cur);
- free(rw);
+ pfree(rw);
}
/*
@@ -951,7 +954,9 @@ RegisterBackgroundWorker(BackgroundWorker *worker)
/*
* Copy the registration data into the registered workers list.
*/
- rw = malloc(sizeof(RegisteredBgWorker));
+ rw = MemoryContextAllocExtended(PostmasterContext,
+ sizeof(RegisteredBgWorker),
+ MCXT_ALLOC_NO_OOM);
if (rw == NULL)
{
ereport(LOG,
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 54e9bfb8c48..692c70b38df 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -3335,7 +3335,7 @@ CleanupBackgroundWorker(int pid,
*/
if (rw->rw_backend->bgworker_notify)
BackgroundWorkerStopNotifications(rw->rw_pid);
- free(rw->rw_backend);
+ pfree(rw->rw_backend);
rw->rw_backend = NULL;
rw->rw_pid = 0;
rw->rw_child_slot = 0;
@@ -3428,7 +3428,7 @@ CleanupBackend(int pid,
BackgroundWorkerStopNotifications(bp->pid);
}
dlist_delete(iter.cur);
- free(bp);
+ pfree(bp);
break;
}
}
@@ -3484,7 +3484,7 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
#ifdef EXEC_BACKEND
ShmemBackendArrayRemove(rw->rw_backend);
#endif
- free(rw->rw_backend);
+ pfree(rw->rw_backend);
rw->rw_backend = NULL;
rw->rw_pid = 0;
rw->rw_child_slot = 0;
@@ -3521,7 +3521,7 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
#endif
}
dlist_delete(iter.cur);
- free(bp);
+ pfree(bp);
/* Keep looping so we can signal remaining backends */
}
else
@@ -4097,7 +4097,7 @@ BackendStartup(Port *port)
* Create backend data structure. Better before the fork() so we can
* handle failure cleanly.
*/
- bn = (Backend *) malloc(sizeof(Backend));
+ bn = (Backend *) palloc_extended(sizeof(Backend), MCXT_ALLOC_NO_OOM);
if (!bn)
{
ereport(LOG,
@@ -4113,7 +4113,7 @@ BackendStartup(Port *port)
*/
if (!RandomCancelKey(&MyCancelKey))
{
- free(bn);
+ pfree(bn);
ereport(LOG,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg("could not generate random cancel key")));
@@ -4143,8 +4143,6 @@ BackendStartup(Port *port)
pid = fork_process();
if (pid == 0) /* child */
{
- free(bn);
-
/* Detangle from postmaster */
InitPostmasterChild();
@@ -4175,7 +4173,7 @@ BackendStartup(Port *port)
if (!bn->dead_end)
(void) ReleasePostmasterChildSlot(bn->child_slot);
- free(bn);
+ pfree(bn);
errno = save_errno;
ereport(LOG,
(errmsg("could not fork new process for connection: %m")));
@@ -5438,7 +5436,7 @@ StartAutovacuumWorker(void)
return;
}
- bn = (Backend *) malloc(sizeof(Backend));
+ bn = (Backend *) palloc_extended(sizeof(Backend), MCXT_ALLOC_NO_OOM);
if (bn)
{
bn->cancel_key = MyCancelKey;
@@ -5465,7 +5463,7 @@ StartAutovacuumWorker(void)
* logged by StartAutoVacWorker
*/
(void) ReleasePostmasterChildSlot(bn->child_slot);
- free(bn);
+ pfree(bn);
}
else
ereport(LOG,
@@ -5710,7 +5708,7 @@ do_start_bgworker(RegisteredBgWorker *rw)
/* undo what assign_backendlist_entry did */
ReleasePostmasterChildSlot(rw->rw_child_slot);
rw->rw_child_slot = 0;
- free(rw->rw_backend);
+ pfree(rw->rw_backend);
rw->rw_backend = NULL;
/* mark entry as crashed, so we'll try again later */
rw->rw_crashed_at = GetCurrentTimestamp();
@@ -5836,7 +5834,7 @@ assign_backendlist_entry(RegisteredBgWorker *rw)
return false;
}
- bn = malloc(sizeof(Backend));
+ bn = palloc_extended(sizeof(Backend), MCXT_ALLOC_NO_OOM);
if (bn == NULL)
{
ereport(LOG,
--
2.39.2
From 0633b2ce261d43b60343c11000582b4ad444a0a5 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Thu, 24 Aug 2023 18:11:32 +0300
Subject: [PATCH v2 3/3] Fix misleading comment on StartBackgroundWorker().
StartBackgroundWorker() is in fact called in the child process, pretty
early in the child process initialization. I guess you could interpret
"called from postmaster" to mean that, but it seems wrong to me.
---
src/backend/postmaster/bgworker.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index cc66c61dee7..ece5ae0daac 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -743,8 +743,7 @@ bgworker_die(SIGNAL_ARGS)
/*
* Start a new background worker
*
- * This is the main entry point for background worker, to be called from
- * postmaster.
+ * This is the main entry point for a background worker process
*/
void
StartBackgroundWorker(void)
--
2.39.2
