On Wed, Sep 2, 2015 at 2:02 AM, Robert Haas <[email protected]> wrote:
> On Mon, Aug 31, 2015 at 8:01 AM, Ashutosh Bapat
> <[email protected]> wrote:
> >> Thanks. It needs testing though to see if it really works as
> >> intended. Can you look into that?
> >
> > PFA the patch containing your code changes + test module. See if that
> meets
> > your expectations.
>
>
PFA patch with improved test module and fix for a bug.
bgworker_sigusr1_handler() should set the latch when set_latch_on_sigusr1
is true, similar to procsignal_sigusr1_handler(). Without this fix, if a
background worker without DATABASE_CONNECTION flag calls
WaitForBackgroundWorker*() functions, those functions wait indefinitely as
the latch is never set upon receipt of SIGUSR1.
> Thanks. I don't think this test module is suitable for commit for a
> number of reasons, including the somewhat hackish use of exit(0)
> instead of proper error reporting
I have changed this part of code.
> , the fact that you didn't integrate
> it into the Makefile structure properly
What was missing? I am able to make {check,clean,install) from the
directory. I can also make -C <dirpath> check from repository's root.
> and the fact that without the
> postmaster.c changes it hangs forever instead of causing a test
> failure.
Changed this too. The SQL level function test_bgwnotify() now errors out if
it doesn't receive notification in specific time.
> But it's sufficient to show that the code changes have the
> intended effect.
>
Looking at the kind of bugs I am getting, we should commit this test
module. Let me know your comments, I will fix those.
> I've committed this and back-patched it to 9.5, but not further. It's
> a bug fix, but it's also a refactoring exercise, so I'd rather not
> push it into released branches.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 68c9505..fbbf97e 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -556,20 +556,23 @@ bgworker_die(SIGNAL_ARGS)
* Standard SIGUSR1 handler for unconnected workers
*
* Here, we want to make sure an unconnected worker will at least heed
* latch activity.
*/
static void
bgworker_sigusr1_handler(SIGNAL_ARGS)
{
int save_errno = errno;
+ if (set_latch_on_sigusr1)
+ SetLatch(MyLatch);
+
latch_sigusr1_handler();
errno = save_errno;
}
/*
* Start a new background worker
*
* This is the main entry point for background worker, to be called from
* postmaster.
diff --git a/src/test/modules/test_bgwnotify/Makefile b/src/test/modules/test_bgwnotify/Makefile
new file mode 100644
index 0000000..6931c09
--- /dev/null
+++ b/src/test/modules/test_bgwnotify/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_bgwnotify/Makefile
+
+MODULE_big = test_bgwnotify
+OBJS = test_bgwnotify.o $(WIN32RES)
+PGFILEDESC = "test_bgwnotify - example use of background worker notification infrastructure"
+
+EXTENSION = test_bgwnotify
+DATA = test_bgwnotify--1.0.sql
+
+REGRESS = test_bgwnotify
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_bgwnotify
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_bgwnotify/expected/test_bgwnotify.out b/src/test/modules/test_bgwnotify/expected/test_bgwnotify.out
new file mode 100644
index 0000000..b7e1b65
--- /dev/null
+++ b/src/test/modules/test_bgwnotify/expected/test_bgwnotify.out
@@ -0,0 +1,11 @@
+CREATE EXTENSION test_bgwnotify;
+--
+-- We're checking that the operations complete without crashing or hanging and
+-- that none of their internal sanity tests fail.
+--
+SELECT test_bgwnotify();
+ test_bgwnotify
+----------------
+ t
+(1 row)
+
diff --git a/src/test/modules/test_bgwnotify/sql/test_bgwnotify.sql b/src/test/modules/test_bgwnotify/sql/test_bgwnotify.sql
new file mode 100644
index 0000000..e448ef0
--- /dev/null
+++ b/src/test/modules/test_bgwnotify/sql/test_bgwnotify.sql
@@ -0,0 +1,7 @@
+CREATE EXTENSION test_bgwnotify;
+
+--
+-- We're checking that the operations complete without crashing or hanging and
+-- that none of their internal sanity tests fail.
+--
+SELECT test_bgwnotify();
diff --git a/src/test/modules/test_bgwnotify/test_bgwnotify--1.0.sql b/src/test/modules/test_bgwnotify/test_bgwnotify--1.0.sql
new file mode 100644
index 0000000..f3423bc
--- /dev/null
+++ b/src/test/modules/test_bgwnotify/test_bgwnotify--1.0.sql
@@ -0,0 +1,7 @@
+/* src/test/modules/test_bgwnotify/test_bgwnotify--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_bgwnotify" to load this file. \quit
+
+CREATE FUNCTION test_bgwnotify() RETURNS pg_catalog.bool STRICT
+ AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_bgwnotify/test_bgwnotify.c b/src/test/modules/test_bgwnotify/test_bgwnotify.c
new file mode 100644
index 0000000..8966b0e
--- /dev/null
+++ b/src/test/modules/test_bgwnotify/test_bgwnotify.c
@@ -0,0 +1,239 @@
+/* -------------------------------------------------------------------------
+ *
+ * test_bgwnotify.c
+ * Test for background worker notify feature. The SQL script for the test calls
+ * test_bgwnotify() function. This function in turn runs launcher background
+ * workers in different configurations. The launcher in turn runs one
+ * background workers, which does nothing but exit after sleeping for a while.
+ * The launcher waits for the workers to finish. The function test_bgwnotify()
+ * waits for launcher to quit. If launcher doesn't quit within definite time,
+ * test_bgwnotify() throws error and kills the launcher.
+ *
+ * Copyright (C) 2015, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/test/modules/test_bgwnotify/test_bgwnotify.c
+ *
+ * -------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+/* These are always necessary for a bgworker */
+#include "miscadmin.h"
+#include "postmaster/bgworker.h"
+#include "storage/ipc.h"
+#include "storage/latch.h"
+#include "storage/lwlock.h"
+#include "storage/proc.h"
+#include "storage/shmem.h"
+
+/* these headers are used by this particular worker's code */
+#include "fmgr.h"
+
+PG_MODULE_MAGIC;
+
+PG_FUNCTION_INFO_V1(test_bgwnotify);
+void test_bgwnotify_launcher_main(Datum datum);
+void test_bgwnotify_worker_main(Datum datum);
+
+/* flags set by signal handlers */
+static volatile sig_atomic_t got_sigusr1 = false;
+
+/* Worker sleep time in seconds */
+static int worker_sleeptime = 5;
+
+/*
+ * Signal handler for SIGUSR1
+ * Set a flag to tell the main loop to check status of worker processes,
+ * and set our latch to wake it up.
+ */
+static void
+test_bgwnotify_sigusr1(SIGNAL_ARGS)
+{
+ int save_errno = errno;
+
+ got_sigusr1 = true;
+ SetLatch(MyLatch);
+
+ errno = save_errno;
+}
+
+static void
+wait_for_launcher_shutdown(BackgroundWorkerHandle *handle)
+{
+ int pid;
+ BgwHandleStatus status;
+ BgwHandleStatus expected_status;
+
+ expected_status = BGWH_STARTED;
+ while (true)
+ {
+ int rc;
+
+ CHECK_FOR_INTERRUPTS();
+
+ /* Be on the safe side and wait for twice as much time as a worker */
+ rc = WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH,
+ worker_sleeptime * 2 * 1000L);
+ ResetLatch(MyLatch);
+
+ if (got_sigusr1)
+ {
+ got_sigusr1 = false;
+ status = GetBackgroundWorkerPid(handle, &pid);
+ if (status != expected_status)
+ elog(ERROR, "found launcher in unexpected state (expected status %d, got %d), exiting.",
+ expected_status, status);
+
+ if (status == BGWH_STARTED)
+ {
+ expected_status = BGWH_STOPPED;
+ continue;
+ }
+ else if (status == BGWH_STOPPED)
+ {
+ /* wait is over, return */
+ return;
+ }
+ }
+
+ if (rc & WL_TIMEOUT)
+ {
+ TerminateBackgroundWorker(handle);
+ elog(ERROR, "did not receive notification from the background worker within expected time");
+ }
+ }
+}
+
+/*
+ * This function runs the launcher background worker in different
+ * configurations. The function raises an error if the launcher background
+ * worker doesn't exit within stipulated time.
+ */
+void
+test_bgwnotify_launcher_main(Datum main_arg)
+{
+ BackgroundWorker worker;
+ BackgroundWorkerHandle *handle;
+ int pid;
+
+ /* We're now ready to receive signals */
+ BackgroundWorkerUnblockSignals();
+
+ /* Register one background worker */
+ worker.bgw_flags = BGWORKER_SHMEM_ACCESS;
+ 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, "test_bgwnotify");
+ sprintf(worker.bgw_function_name, "test_bgwnotify_worker_main");
+ worker.bgw_notify_pid = MyProcPid;
+ snprintf(worker.bgw_name, BGW_MAXLEN, "test_bgwnotify_worker");
+ worker.bgw_main_arg = 0;
+
+ /* Launch the worker and wait for it to quit */
+ RegisterDynamicBackgroundWorker(&worker, &handle);
+
+ /*
+ * First wait for the background worker to start and then for it to quit.
+ * In case, the notifications are not received in time, the launcher thread
+ * will wait indefinitely. The backend which started the launcher, however
+ * waits for definite time and errors out when it doesn't receive the
+ * notification in time. At that time, it will also terminate us.
+ */
+ WaitForBackgroundWorkerStartup(handle, &pid);
+
+ WaitForBackgroundWorkerShutdown(handle);
+
+ proc_exit(0);
+}
+
+void
+test_bgwnotify_worker_main(Datum main_arg)
+{
+ int rc;
+
+ /* We're now ready to receive signals */
+ BackgroundWorkerUnblockSignals();
+
+ ResetLatch(MyLatch);
+ /*
+ * The worker registered will wait for worker_sleeptime and is expected to
+ * quit then. The launcher is expected to get a notification when the worker
+ * quits. Let the launcher wait for twice the time the worker waits. Raise
+ * error if the launcher doesn't receive the noitification.
+ */
+ rc = WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH,
+ worker_sleeptime * 1000L);
+
+ /*
+ * Only timeout should wake up this worker. Everything else is error
+ * condition.
+ */
+ if (rc & WL_TIMEOUT)
+ proc_exit(0);
+
+ /* Anything else is unexpected. Throw error and exit. */
+ elog(WARNING, "unexpected event (rc = %d) occured, exiting", rc);
+ proc_exit(1);
+}
+
+/*
+ * Dynamically launch an SPI worker.
+ */
+Datum
+test_bgwnotify(PG_FUNCTION_ARGS)
+{
+ BackgroundWorker worker;
+ BackgroundWorkerHandle *handle;
+
+ /* Establish signal handlers before unblocking signals. */
+ pqsignal(SIGUSR1, test_bgwnotify_sigusr1);
+
+ /* test1: shared memory and database access */
+ worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
+ BGWORKER_BACKEND_DATABASE_CONNECTION;
+ 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, "test_bgwnotify");
+ sprintf(worker.bgw_function_name, "test_bgwnotify_launcher_main");
+ snprintf(worker.bgw_name, BGW_MAXLEN, "test_bgwnotify_launcher");
+ worker.bgw_main_arg = 0;
+ worker.bgw_notify_pid = MyProcPid;
+ got_sigusr1 = false;
+
+ if (!RegisterDynamicBackgroundWorker(&worker, &handle))
+ PG_RETURN_BOOL(false);
+
+ /*
+ * Wait for the launcher to quit. The function makes sure that the launcher
+ * has been terminated before quitting from the function.
+ */
+ wait_for_launcher_shutdown(handle);
+
+ /* test2: shared memory access */
+ worker.bgw_flags = BGWORKER_SHMEM_ACCESS;
+ 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, "test_bgwnotify");
+ sprintf(worker.bgw_function_name, "test_bgwnotify_launcher_main");
+ snprintf(worker.bgw_name, BGW_MAXLEN, "test_bgwnotify_launcher");
+ worker.bgw_main_arg = 0;
+ worker.bgw_notify_pid = MyProcPid;
+ got_sigusr1 = false;
+
+ if (!RegisterDynamicBackgroundWorker(&worker, &handle))
+ PG_RETURN_BOOL(false);
+
+ /*
+ * Wait for the launcher to quit. The function makes sure that the launcher
+ * has been terminated before quitting from the function.
+ */
+ wait_for_launcher_shutdown(handle);
+
+ PG_RETURN_BOOL(true);
+}
diff --git a/src/test/modules/test_bgwnotify/test_bgwnotify.control b/src/test/modules/test_bgwnotify/test_bgwnotify.control
new file mode 100644
index 0000000..c2881fc
--- /dev/null
+++ b/src/test/modules/test_bgwnotify/test_bgwnotify.control
@@ -0,0 +1,4 @@
+comment = 'Test code for background worker notification'
+default_version = '1.0'
+module_pathname = '$libdir/test_bgwnotify'
+relocatable = true
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
