I've complained about this problem a few times before: there's nothing
to prevent a background worker which doesn't request shared memory
access from calling InitProcess() and then accessing shared memory
anyway. The attached patch is a first crack at fixing it.
Unfortunately, there's still a window between when we fork() and when
we detach shared memory, but that's also true for processes like
syslogger, whose death is nevertheless does not cause a
crash-and-restart. Also unfortunately, in the EXEC_BACKEND case, we
actually have to attach shared memory first, to get our background
worker entry details. This is undesirable, but I'm not sure there's
any good way to fix it at all, and certainly not in time for 9.4.
Hopefully, the window when shared memory is attached with this patch
is short enough to make things relatively safe.
At a minimum, it's got to be better than the status quo, where shared
memory is accessible throughout the entire lifetime of
non-shmem-access background workers.
Attached is also a new background worker called say_hello which I used
for testing this patch. That's obviously not for commit.
Comments?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index a6b25d8..8b8ae89 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -20,9 +20,11 @@
#include "postmaster/bgworker_internals.h"
#include "postmaster/postmaster.h"
#include "storage/barrier.h"
+#include "storage/dsm.h"
#include "storage/ipc.h"
#include "storage/latch.h"
#include "storage/lwlock.h"
+#include "storage/pg_shmem.h"
#include "storage/pmsignal.h"
#include "storage/proc.h"
#include "storage/procsignal.h"
@@ -400,12 +402,16 @@ BackgroundWorkerStopNotifications(pid_t pid)
BackgroundWorker *
BackgroundWorkerEntry(int slotno)
{
+ static BackgroundWorker myEntry;
BackgroundWorkerSlot *slot;
Assert(slotno < BackgroundWorkerData->total_slots);
slot = &BackgroundWorkerData->slot[slotno];
Assert(slot->in_use);
- return &slot->worker; /* can't become free while we're still here */
+
+ /* must copy this in case we don't intend to retain shmem access */
+ memcpy(&myEntry, &slot->worker, sizeof myEntry);
+ return &myEntry;
}
#endif
@@ -542,6 +548,19 @@ StartBackgroundWorker(void)
snprintf(buf, MAXPGPATH, "bgworker: %s", worker->bgw_name);
init_ps_display(buf, "", "", "");
+ /*
+ * If we're not supposed to have shared memory access, then detach from
+ * shared memory. If we didn't request shared memory access, the
+ * postmaster won't force a cluster-wide restart if we exit unexpectedly,
+ * so we'd better make sure that we don't mess anything up that would
+ * require that sort of cleanup.
+ */
+ if ((worker->bgw_flags & BGWORKER_SHMEM_ACCESS) == 0)
+ {
+ dsm_detach_all();
+ PGSharedMemoryDetach();
+ }
+
SetProcessingMode(InitProcessing);
/* Apply PostAuthDelay */
@@ -616,19 +635,29 @@ StartBackgroundWorker(void)
/* We can now handle ereport(ERROR) */
PG_exception_stack = &local_sigjmp_buf;
- /* Early initialization */
- BaseInit();
-
/*
- * If necessary, create a per-backend PGPROC struct in shared memory,
- * except in the EXEC_BACKEND case where this was done in
- * SubPostmasterMain. We must do this before we can use LWLocks (and in
- * the EXEC_BACKEND case we already had to do some stuff with LWLocks).
+ * If the background worker request shared memory access, set that up now;
+ * else, detach all shared memory segments.
*/
-#ifndef EXEC_BACKEND
if (worker->bgw_flags & BGWORKER_SHMEM_ACCESS)
+ {
+ /*
+ * Early initialization. Some of this could be useful even for
+ * background workers that aren't using shared memory, but they can
+ * call the individual startup routines for those subsystems if needed.
+ */
+ BaseInit();
+
+ /*
+ * Create a per-backend PGPROC struct in shared memory, except in the
+ * EXEC_BACKEND case where this was done in SubPostmasterMain. We must
+ * do this before we can use LWLocks (and in the EXEC_BACKEND case we
+ * already had to do some stuff with LWLocks).
+ */
+#ifndef EXEC_BACKEND
InitProcess();
#endif
+ }
/*
* If bgw_main is set, we use that value as the initial entrypoint.
diff --git a/contrib/say_hello/Makefile b/contrib/say_hello/Makefile
new file mode 100644
index 0000000..2da89d7
--- /dev/null
+++ b/contrib/say_hello/Makefile
@@ -0,0 +1,17 @@
+# contrib/say_hello/Makefile
+
+MODULES = say_hello
+
+EXTENSION = say_hello
+DATA = say_hello--1.0.sql
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/say_hello
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/say_hello/say_hello--1.0.sql b/contrib/say_hello/say_hello--1.0.sql
new file mode 100644
index 0000000..c82b7df
--- /dev/null
+++ b/contrib/say_hello/say_hello--1.0.sql
@@ -0,0 +1,9 @@
+/* contrib/say_hello/say_hello--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION say_hello" to load this file. \quit
+
+CREATE FUNCTION say_hello_launch()
+RETURNS pg_catalog.int4 STRICT
+AS 'MODULE_PATHNAME'
+LANGUAGE C;
diff --git a/contrib/say_hello/say_hello.c b/contrib/say_hello/say_hello.c
new file mode 100644
index 0000000..15b1daf
--- /dev/null
+++ b/contrib/say_hello/say_hello.c
@@ -0,0 +1,110 @@
+/* -------------------------------------------------------------------------
+ *
+ * say_hello.c
+ *
+ * A background worker that just says hello.
+ *
+ * Copyright (C) 2013, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/say_hello/say_hello.c
+ *
+ * -------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "postmaster/bgworker.h"
+#include "storage/ipc.h"
+#include "utils/builtins.h"
+#include "utils/guc.h"
+
+PG_MODULE_MAGIC;
+
+PG_FUNCTION_INFO_V1(say_hello_launch);
+
+void _PG_init(void);
+void say_hello_main(Datum);
+
+static volatile sig_atomic_t got_sighup = false;
+
+static void
+say_hello_sighup(SIGNAL_ARGS)
+{
+ got_sighup = true;
+}
+
+static void
+say_hello_sigterm(SIGNAL_ARGS)
+{
+ exit(1);
+}
+
+void
+say_hello_main(Datum main_arg)
+{
+ pqsignal(SIGHUP, say_hello_sighup);
+ pqsignal(SIGTERM, say_hello_sigterm);
+ BackgroundWorkerUnblockSignals();
+
+ while (1)
+ {
+ CHECK_FOR_INTERRUPTS();
+
+ elog(LOG, "hello!");
+
+ pg_usleep(5 * 1000000L);
+
+ if (got_sighup)
+ {
+ got_sighup = false;
+ ProcessConfigFile(PGC_SIGHUP);
+ }
+ }
+
+ proc_exit(1);
+}
+
+/*
+ * Dynamically launch a worker.
+ */
+Datum
+say_hello_launch(PG_FUNCTION_ARGS)
+{
+ int32 i = PG_GETARG_INT32(0);
+ BackgroundWorker worker;
+ BackgroundWorkerHandle *handle;
+ BgwHandleStatus status;
+ pid_t pid;
+
+ worker.bgw_flags = 0;
+ worker.bgw_start_time = BgWorkerStart_RecoveryFinished;
+ worker.bgw_restart_time = BGW_NEVER_RESTART;
+ worker.bgw_main = NULL; /* new worker might not have library loaded */
+ sprintf(worker.bgw_library_name, "say_hello");
+ sprintf(worker.bgw_function_name, "say_hello_main");
+ sprintf(worker.bgw_name, "say_hello");
+ worker.bgw_main_arg = Int32GetDatum(i);
+ /* set bgw_notify_pid so that we can use WaitForBackgroundWorkerStartup */
+ worker.bgw_notify_pid = MyProcPid;
+
+ if (!RegisterDynamicBackgroundWorker(&worker, &handle))
+ PG_RETURN_NULL();
+
+ status = WaitForBackgroundWorkerStartup(handle, &pid);
+
+ if (status == BGWH_STOPPED)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_RESOURCES),
+ errmsg("could not start background process"),
+ errhint("More details may be available in the server log.")));
+ if (status == BGWH_POSTMASTER_DIED)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_RESOURCES),
+ errmsg("cannot start background processes without postmaster"),
+ errhint("Kill all remaining database processes and restart the database.")));
+ Assert(status == BGWH_STARTED);
+
+ PG_RETURN_INT32(pid);
+}
diff --git a/contrib/say_hello/say_hello.control b/contrib/say_hello/say_hello.control
new file mode 100644
index 0000000..85044c1
--- /dev/null
+++ b/contrib/say_hello/say_hello.control
@@ -0,0 +1,5 @@
+# say_hello extension
+comment = 'Background worker that just says hello'
+default_version = '1.0'
+module_pathname = '$libdir/say_hello'
+relocatable = true
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
