Regarding BGworkers

Started by Amit kapilaover 12 years ago12 messages
#1Amit kapila
amit.kapila@huawei.com

While going through below commit, few doubts/observations:
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=7f7485a0cde92aa4ba235a1ffe4dda0ca0b6cc9a

1. Bgworker.c -
FindRegisteredWorkerBySlotNumber()
{
..
/*
* Copy contents of worker list into shared memory. Record the
* shared memory slot assigned to each worker. This ensures
* a 1-to-1 correspondence betwen the postmaster's private list and
* the array in shared memory.
*/
..
}
a. Comment in function doesn't seem to be appropriate. It seems copy-pasted from function
BackgroundWorkerShmemInit
b. all function's except this have function header to explain a bit about function, though
it might not be required here, but not sure so pointed.

2. Shouldn't function
do_start_bgworker()/StartOneBackgroundWorker(void) be moved to bgworker.c
as similar functions AutoVacWorkerMain()/PgArchiverMain() are in their respective files.

3. bgworker.h - file header still contains explanation only as per old functionality.
Not sure, if it needs to be updated for new functionality of dynamic workers.

With Regards,
Amit Kapila.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#2Robert Haas
robertmhaas@gmail.com
In reply to: Amit kapila (#1)
Re: Regarding BGworkers

On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit.kapila@huawei.com> wrote:

1. Bgworker.c -
FindRegisteredWorkerBySlotNumber()
{
..
/*
* Copy contents of worker list into shared memory. Record the
* shared memory slot assigned to each worker. This ensures
* a 1-to-1 correspondence betwen the postmaster's private list and
* the array in shared memory.
*/
..
}
a. Comment in function doesn't seem to be appropriate. It seems copy-pasted from function
BackgroundWorkerShmemInit
b. all function's except this have function header to explain a bit about function, though
it might not be required here, but not sure so pointed.

Fixed.

2. Shouldn't function
do_start_bgworker()/StartOneBackgroundWorker(void) be moved to bgworker.c
as similar functions AutoVacWorkerMain()/PgArchiverMain() are in their respective files.

Yes, perhaps so. Other votes?

3. bgworker.h - file header still contains explanation only as per old functionality.
Not sure, if it needs to be updated for new functionality of dynamic workers.

Fixed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#2)
Re: Regarding BGworkers

On Fri, Aug 2, 2013 at 1:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit.kapila@huawei.com>
wrote:

2. Shouldn't function
do_start_bgworker()/StartOneBackgroundWorker(void) be moved to bgworker.c
as similar functions AutoVacWorkerMain()/PgArchiverMain() are in

their respective files.

Yes, perhaps so. Other votes?

StartOneBackgroundWorker uses StartWorkerNeeded and HaveCrashedWorker, and
IMO, we should not expose that outside the postmaster. On the contrary,
moving do_start_bgworker would be fine, as it uses nothing exclusive to the
postmaster as far as I saw, and it would also make it more consistent with
the other features.

Regards,
--
Michael

#4Amit Kapila
amit.kapila@huawei.com
In reply to: Michael Paquier (#3)
Re: Regarding BGworkers

On Friday, August 02, 2013 4:19 AM Michael Paquier wrote:
On Fri, Aug 2, 2013 at 1:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit.kapila@huawei.com> wrote:

2. Shouldn't function
do_start_bgworker()/StartOneBackgroundWorker(void) be moved to

bgworker.c

   as similar functions AutoVacWorkerMain()/PgArchiverMain() are in

their respective files.

Yes, perhaps so.  Other votes?

StartOneBackgroundWorker uses StartWorkerNeeded and HaveCrashedWorker, and

IMO, we should not expose that outside the postmaster.

How about exposing Set/Get for these from bgworker?

On the contrary,
moving do_start_bgworker would be fine, as it uses nothing exclusive to

the postmaster as far as I saw, and it would also make it more consistent
with > the other features.

With Regards,
Amit Kapila.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Amit Kapila (#4)
Re: Regarding BGworkers

Amit Kapila escribi�:

On Friday, August 02, 2013 4:19 AM Michael Paquier wrote:

On Fri, Aug 2, 2013 at 1:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Jul 28, 2013 at 1:26 AM, Amit kapila <amit.kapila@huawei.com> wrote:

2. Shouldn't function
do_start_bgworker()/StartOneBackgroundWorker(void) be moved to
bgworker.c
� �as similar functions AutoVacWorkerMain()/PgArchiverMain() are in
their respective files.

Yes, perhaps so. �Other votes?

StartOneBackgroundWorker uses StartWorkerNeeded and HaveCrashedWorker, and
IMO, we should not expose that outside the postmaster.

How about exposing Set/Get for these from bgworker?

That seems more mess than just keeping that function in postmaster.c.
I agree with moving the other one.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Michael Paquier
michael.paquier@gmail.com
In reply to: Alvaro Herrera (#5)
1 attachment(s)
Re: Regarding BGworkers

On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

That seems more mess than just keeping that function in postmaster.c.
I agree with moving the other one.

Please find attached a patch for that can be applied on master branch.
do_start_bgworker is renamed to StartBackgroundWorker and moved to
bgworker.c. At the same time, bgworker_quickdie, bgworker_die and
bgworker_sigusr1_handler are moved to bgworker.c as they are used in
do_start_bgworker.

Regards,
--
Michael

Attachments:

20130806_bgworker_refactor_master.patchapplication/octet-stream; name=20130806_bgworker_refactor_master.patchDownload
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 5ed4229..bf8bbbf 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -12,13 +12,24 @@
 
 #include "postgres.h"
 
+#include <unistd.h>
+#include <time.h>
+
 #include "miscadmin.h"
+#include "libpq/pqsignal.h"
 #include "postmaster/bgworker_internals.h"
 #include "storage/barrier.h"
+#include "storage/ipc.h"
+#include "storage/latch.h"
 #include "storage/lwlock.h"
 #include "storage/pmsignal.h"
+#include "storage/proc.h"
+#include "storage/procsignal.h"
 #include "storage/shmem.h"
+#include "tcop/tcopprot.h"
 #include "utils/ascii.h"
+#include "utils/ps_status.h"
+#include "utils/timeout.h"
 
 /*
  * The postmaster's list of registered background workers, in private memory.
@@ -354,6 +365,214 @@ SanityCheckBackgroundWorker(BackgroundWorker *worker, int elevel)
 	return true;
 }
 
+static void
+bgworker_quickdie(SIGNAL_ARGS)
+{
+	sigaddset(&BlockSig, SIGQUIT);      /* prevent nested calls */
+	PG_SETMASK(&BlockSig);
+
+	/*
+	 * We DO NOT want to run proc_exit() callbacks -- we're here because
+	 * shared memory may be corrupted, so we don't want to try to clean up our
+	 * transaction.  Just nail the windows shut and get out of town.  Now that
+	 * there's an atexit callback to prevent third-party code from breaking
+	 * things by calling exit() directly, we have to reset the callbacks
+	 * explicitly to make this work as intended.
+	 */
+	on_exit_reset();
+
+	/*
+	 * Note we do exit(0) here, not exit(2) like quickdie.  The reason is that
+	 * we don't want to be seen this worker as independently crashed, because
+	 * then postmaster would delay restarting it again afterwards.  If some
+	 * idiot DBA manually sends SIGQUIT to a random bgworker, the "dead man
+	 * switch" will ensure that postmaster sees this as a crash.
+	 */
+	exit(0);
+}
+
+/*
+ * Standard SIGTERM handler for background workers
+ */
+static void
+bgworker_die(SIGNAL_ARGS)
+{
+	PG_SETMASK(&BlockSig);
+
+	ereport(FATAL,
+			(errcode(ERRCODE_ADMIN_SHUTDOWN),
+			 errmsg("terminating background worker \"%s\" due to administrator command",
+					MyBgworkerEntry->bgw_name)));
+}
+
+/*
+ * 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;
+
+	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.
+ */
+void
+StartBackgroundWorker(void)
+{
+	sigjmp_buf	local_sigjmp_buf;
+	char		buf[MAXPGPATH];
+	BackgroundWorker *worker = MyBgworkerEntry;
+	bgworker_main_type entrypt;
+
+	if (worker == NULL)
+		elog(FATAL, "unable to find bgworker entry");
+
+	/* we are a postmaster subprocess now */
+	IsUnderPostmaster = true;
+	IsBackgroundWorker = true;
+
+	/* reset MyProcPid */
+	MyProcPid = getpid();
+
+	/* record Start Time for logging */
+	MyStartTime = time(NULL);
+
+	/* Identify myself via ps */
+	snprintf(buf, MAXPGPATH, "bgworker: %s", worker->bgw_name);
+	init_ps_display(buf, "", "", "");
+
+	SetProcessingMode(InitProcessing);
+
+	/* Apply PostAuthDelay */
+	if (PostAuthDelay > 0)
+		pg_usleep(PostAuthDelay * 1000000L);
+
+	/*
+	 * If possible, make this process a group leader, so that the postmaster
+	 * can signal any child processes too.
+	 */
+#ifdef HAVE_SETSID
+	if (setsid() < 0)
+		elog(FATAL, "setsid() failed: %m");
+#endif
+
+	/*
+	 * Set up signal handlers.
+	 */
+	if (worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION)
+	{
+		/*
+		 * SIGINT is used to signal canceling the current action
+		 */
+		pqsignal(SIGINT, StatementCancelHandler);
+		pqsignal(SIGUSR1, procsignal_sigusr1_handler);
+		pqsignal(SIGFPE, FloatExceptionHandler);
+
+		/* XXX Any other handlers needed here? */
+	}
+	else
+	{
+		pqsignal(SIGINT, SIG_IGN);
+		pqsignal(SIGUSR1, bgworker_sigusr1_handler);
+		pqsignal(SIGFPE, SIG_IGN);
+	}
+	pqsignal(SIGTERM, bgworker_die);
+	pqsignal(SIGHUP, SIG_IGN);
+
+	pqsignal(SIGQUIT, bgworker_quickdie);
+	InitializeTimeouts();       /* establishes SIGALRM handler */
+
+	pqsignal(SIGPIPE, SIG_IGN);
+	pqsignal(SIGUSR2, SIG_IGN);
+	pqsignal(SIGCHLD, SIG_DFL);
+
+	/*
+	 * If an exception is encountered, processing resumes here.
+	 *
+	 * See notes in postgres.c about the design of this coding.
+	 */
+	if (sigsetjmp(local_sigjmp_buf, 1) != 0)
+	{
+		/* Since not using PG_TRY, must reset error stack by hand */
+		error_context_stack = NULL;
+
+		/* Prevent interrupts while cleaning up */
+		HOLD_INTERRUPTS();
+
+		/* Report the error to the server log */
+		EmitErrorReport();
+
+		/*
+		 * Do we need more cleanup here?  For shmem-connected bgworkers, we
+		 * will call InitProcess below, which will install ProcKill as exit
+		 * callback.  That will take care of releasing locks, etc.
+		 */
+
+		/* and go away */
+		proc_exit(1);
+	}
+
+	/* 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).
+	 */
+#ifndef EXEC_BACKEND
+	if (worker->bgw_flags & BGWORKER_SHMEM_ACCESS)
+		InitProcess();
+#endif
+
+	/*
+	 * If bgw_main is set, we use that value as the initial entrypoint.
+	 * However, if the library containing the entrypoint wasn't loaded at
+	 * postmaster startup time, passing it as a direct function pointer is
+	 * not possible.  To work around that, we allow callers for whom a
+	 * function pointer is not available to pass a library name (which will
+	 * be loaded, if necessary) and a function name (which will be looked up
+	 * in the named library).
+	 */
+	if (worker->bgw_main != NULL)
+		entrypt = worker->bgw_main;
+	else
+		entrypt = (bgworker_main_type)
+			load_external_function(worker->bgw_library_name,
+								   worker->bgw_function_name,
+								   true, NULL);
+
+	/*
+	 * Note that in normal processes, we would call InitPostgres here.  For a
+	 * worker, however, we don't know what database to connect to, yet; so we
+	 * need to wait until the user code does it via
+	 * BackgroundWorkerInitializeConnection().
+	 */
+
+	/*
+	 * Now invoke the user-defined worker code
+	 */
+	entrypt(worker->bgw_main_arg);
+
+	/* ... and if it returns, we're done */
+	proc_exit(0);
+}
+
 /*
  * Register a new background worker while processing shared_preload_libraries.
  *
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e6d750d..d00319b 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -383,7 +383,6 @@ static void dummy_handler(SIGNAL_ARGS);
 static void StartupPacketTimeoutHandler(void);
 static void CleanupBackend(int pid, int exitstatus);
 static bool CleanupBackgroundWorker(int pid, int exitstatus);
-static void do_start_bgworker(void);
 static void HandleChildCrash(int pid, int exitstatus, const char *procname);
 static void LogChildExit(int lev, const char *procname,
 			 int pid, int exitstatus);
@@ -4614,7 +4613,7 @@ SubPostmasterMain(int argc, char *argv[])
 
 		shmem_slot = atoi(argv[1] + 15);
 		MyBgworkerEntry = BackgroundWorkerEntry(shmem_slot);
-		do_start_bgworker();
+		StartBackgroundWorker();
 	}
 	if (strcmp(argv[1], "--forkarch") == 0)
 	{
@@ -5241,208 +5240,6 @@ BackgroundWorkerUnblockSignals(void)
 	PG_SETMASK(&UnBlockSig);
 }
 
-static void
-bgworker_quickdie(SIGNAL_ARGS)
-{
-	sigaddset(&BlockSig, SIGQUIT);		/* prevent nested calls */
-	PG_SETMASK(&BlockSig);
-
-	/*
-	 * We DO NOT want to run proc_exit() callbacks -- we're here because
-	 * shared memory may be corrupted, so we don't want to try to clean up our
-	 * transaction.  Just nail the windows shut and get out of town.  Now that
-	 * there's an atexit callback to prevent third-party code from breaking
-	 * things by calling exit() directly, we have to reset the callbacks
-	 * explicitly to make this work as intended.
-	 */
-	on_exit_reset();
-
-	/*
-	 * Note we do exit(0) here, not exit(2) like quickdie.	The reason is that
-	 * we don't want to be seen this worker as independently crashed, because
-	 * then postmaster would delay restarting it again afterwards.	If some
-	 * idiot DBA manually sends SIGQUIT to a random bgworker, the "dead man
-	 * switch" will ensure that postmaster sees this as a crash.
-	 */
-	exit(0);
-}
-
-/*
- * Standard SIGTERM handler for background workers
- */
-static void
-bgworker_die(SIGNAL_ARGS)
-{
-	PG_SETMASK(&BlockSig);
-
-	ereport(FATAL,
-			(errcode(ERRCODE_ADMIN_SHUTDOWN),
-			 errmsg("terminating background worker \"%s\" due to administrator command",
-					MyBgworkerEntry->bgw_name)));
-}
-
-/*
- * 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;
-
-	latch_sigusr1_handler();
-
-	errno = save_errno;
-}
-
-static void
-do_start_bgworker(void)
-{
-	sigjmp_buf	local_sigjmp_buf;
-	char		buf[MAXPGPATH];
-	BackgroundWorker *worker = MyBgworkerEntry;
-	bgworker_main_type entrypt;
-
-	if (worker == NULL)
-		elog(FATAL, "unable to find bgworker entry");
-
-	/* we are a postmaster subprocess now */
-	IsUnderPostmaster = true;
-	IsBackgroundWorker = true;
-
-	/* reset MyProcPid */
-	MyProcPid = getpid();
-
-	/* record Start Time for logging */
-	MyStartTime = time(NULL);
-
-	/* Identify myself via ps */
-	snprintf(buf, MAXPGPATH, "bgworker: %s", worker->bgw_name);
-	init_ps_display(buf, "", "", "");
-
-	SetProcessingMode(InitProcessing);
-
-	/* Apply PostAuthDelay */
-	if (PostAuthDelay > 0)
-		pg_usleep(PostAuthDelay * 1000000L);
-
-	/*
-	 * If possible, make this process a group leader, so that the postmaster
-	 * can signal any child processes too.
-	 */
-#ifdef HAVE_SETSID
-	if (setsid() < 0)
-		elog(FATAL, "setsid() failed: %m");
-#endif
-
-	/*
-	 * Set up signal handlers.
-	 */
-	if (worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION)
-	{
-		/*
-		 * SIGINT is used to signal canceling the current action
-		 */
-		pqsignal(SIGINT, StatementCancelHandler);
-		pqsignal(SIGUSR1, procsignal_sigusr1_handler);
-		pqsignal(SIGFPE, FloatExceptionHandler);
-
-		/* XXX Any other handlers needed here? */
-	}
-	else
-	{
-		pqsignal(SIGINT, SIG_IGN);
-		pqsignal(SIGUSR1, bgworker_sigusr1_handler);
-		pqsignal(SIGFPE, SIG_IGN);
-	}
-	pqsignal(SIGTERM, bgworker_die);
-	pqsignal(SIGHUP, SIG_IGN);
-
-	pqsignal(SIGQUIT, bgworker_quickdie);
-	InitializeTimeouts();		/* establishes SIGALRM handler */
-
-	pqsignal(SIGPIPE, SIG_IGN);
-	pqsignal(SIGUSR2, SIG_IGN);
-	pqsignal(SIGCHLD, SIG_DFL);
-
-	/*
-	 * If an exception is encountered, processing resumes here.
-	 *
-	 * See notes in postgres.c about the design of this coding.
-	 */
-	if (sigsetjmp(local_sigjmp_buf, 1) != 0)
-	{
-		/* Since not using PG_TRY, must reset error stack by hand */
-		error_context_stack = NULL;
-
-		/* Prevent interrupts while cleaning up */
-		HOLD_INTERRUPTS();
-
-		/* Report the error to the server log */
-		EmitErrorReport();
-
-		/*
-		 * Do we need more cleanup here?  For shmem-connected bgworkers, we
-		 * will call InitProcess below, which will install ProcKill as exit
-		 * callback.  That will take care of releasing locks, etc.
-		 */
-
-		/* and go away */
-		proc_exit(1);
-	}
-
-	/* 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).
-	 */
-#ifndef EXEC_BACKEND
-	if (worker->bgw_flags & BGWORKER_SHMEM_ACCESS)
-		InitProcess();
-#endif
-
-	/*
-	 * If bgw_main is set, we use that value as the initial entrypoint.
-	 * However, if the library containing the entrypoint wasn't loaded at
-	 * postmaster startup time, passing it as a direct function pointer is
-	 * not possible.  To work around that, we allow callers for whom a
-	 * function pointer is not available to pass a library name (which will
-	 * be loaded, if necessary) and a function name (which will be looked up
-	 * in the named library).
-	 */
-	if (worker->bgw_main != NULL)
-		entrypt = worker->bgw_main;
-	else
-		entrypt = (bgworker_main_type)
-			load_external_function(worker->bgw_library_name,
-								   worker->bgw_function_name,
-								   true, NULL);
-
-	/*
-	 * Note that in normal processes, we would call InitPostgres here.	For a
-	 * worker, however, we don't know what database to connect to, yet; so we
-	 * need to wait until the user code does it via
-	 * BackgroundWorkerInitializeConnection().
-	 */
-
-	/*
-	 * Now invoke the user-defined worker code
-	 */
-	entrypt(worker->bgw_main_arg);
-
-	/* ... and if it returns, we're done */
-	proc_exit(0);
-}
-
 #ifdef EXEC_BACKEND
 static pid_t
 bgworker_forkexec(int shmem_slot)
@@ -5502,7 +5299,7 @@ start_bgworker(RegisteredBgWorker *rw)
 			/* Do NOT release postmaster's working memory context */
 
 			MyBgworkerEntry = &rw->rw_worker;
-			do_start_bgworker();
+			StartBackgroundWorker();
 			break;
 #endif
 		default:
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 0bb897b..b3a79bf 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -82,6 +82,9 @@ typedef struct BackgroundWorker
 	Datum		bgw_main_arg;
 } BackgroundWorker;
 
+/* Function to start a background worker, called from postmaster.c */
+extern void StartBackgroundWorker(void);
+
 /* Register a new bgworker during shared_preload_libraries */
 extern void RegisterBackgroundWorker(BackgroundWorker *worker);
 
#7Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#6)
Re: Regarding BGworkers

On Mon, Aug 5, 2013 at 9:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

That seems more mess than just keeping that function in postmaster.c.
I agree with moving the other one.

Please find attached a patch for that can be applied on master branch.
do_start_bgworker is renamed to StartBackgroundWorker and moved to
bgworker.c. At the same time, bgworker_quickdie, bgworker_die and
bgworker_sigusr1_handler are moved to bgworker.c as they are used in
do_start_bgworker.

This particular formulation doesn't seem quite good to me, because
we'd end up with a function called StartBackgroundWorker() and another
called StartOneBackgroundWorker() doing related but different things.
Maybe we can name things a bit better?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Robert Haas (#7)
Re: Regarding BGworkers

Robert Haas escribi�:

On Mon, Aug 5, 2013 at 9:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Aug 2, 2013 at 1:40 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

That seems more mess than just keeping that function in postmaster.c.
I agree with moving the other one.

Please find attached a patch for that can be applied on master branch.
do_start_bgworker is renamed to StartBackgroundWorker and moved to
bgworker.c. At the same time, bgworker_quickdie, bgworker_die and
bgworker_sigusr1_handler are moved to bgworker.c as they are used in
do_start_bgworker.

This particular formulation doesn't seem quite good to me, because
we'd end up with a function called StartBackgroundWorker() and another
called StartOneBackgroundWorker() doing related but different things.
Maybe we can name things a bit better?

Yeah, we also have start_bgworker(). I agree that we should rename
things so that they make as much sense as possible.

In the current code, we have this:

StartOneBackgroundWorker() in postmaster.c
start_bgworker() in postmaster.c
do_start_bgworker() in postmaster.c

With this patch we would have
StartOneBackgroundWorker() in postmaster.c
start_bgworker() in postmaster.c
StartBackgroundWorker() in bgworker.c

I think we should rename to something like this:

maybe_start_bgworker() in postmaster.c
do_start_bgworker() in postmaster.c
StartBackgroundWorker() in bgworker.c

(I would also rename the functions in 9.3 to avoid inconsistency). Not
wedded to those particular names, but (1) I would add the "maybe" prefix
because that's what that function does; and (2) it seems to me that
stuff in bgworker.c tend to use CamelCaseNaming and postmaster.c uses
names_with_stuffed_underscores.

(My convention tends to be that "internal" stuff uses underscores while
exposed APIs use CamelCase. I probably fail to do it really
consistently.)

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#9Michael Paquier
michael.paquier@gmail.com
In reply to: Alvaro Herrera (#8)
Re: Regarding BGworkers

On Tue, Aug 13, 2013 at 11:59 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

maybe_start_bgworker() in postmaster.c
do_start_bgworker() in postmaster.c
StartBackgroundWorker() in bgworker.c

This formulation is fine, thanks. Instead of maybe_start_bgworker,
what about start_bgworker_if_necessary?
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#9)
Re: Regarding BGworkers

On Tue, Aug 13, 2013 at 8:07 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Tue, Aug 13, 2013 at 11:59 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

maybe_start_bgworker() in postmaster.c
do_start_bgworker() in postmaster.c
StartBackgroundWorker() in bgworker.c

This formulation is fine, thanks. Instead of maybe_start_bgworker,
what about start_bgworker_if_necessary?

I think Alvaro's suggestion is better. It's shorter, and makes clear
that at most one will be started.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#10)
2 attachment(s)
Re: Regarding BGworkers

On Wed, Aug 14, 2013 at 10:10 AM, Robert Haas <robertmhaas@gmail.com> wrote:

I think Alvaro's suggestion is better. It's shorter, and makes clear
that at most one will be started.

OK cool. Here are patches for 9.3 and master respecting those comments.

Regards,
--
Michael

Attachments:

20130814_bgworker_refactor_93_v2.patchapplication/octet-stream; name=20130814_bgworker_refactor_93_v2.patchDownload
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 75f2f82..b94851a 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -401,7 +401,7 @@ static int	GetNumRegisteredBackgroundWorkers(int flags);
 static void StartupPacketTimeoutHandler(void);
 static void CleanupBackend(int pid, int exitstatus);
 static bool CleanupBackgroundWorker(int pid, int exitstatus);
-static void do_start_bgworker(void);
+static void StartBackgroundWorker(void);
 static void HandleChildCrash(int pid, int exitstatus, const char *procname);
 static void LogChildExit(int lev, const char *procname,
 			 int pid, int exitstatus);
@@ -426,7 +426,7 @@ static bool SignalUnconnectedWorkers(int signal);
 
 static int	CountChildren(int target);
 static int	CountUnconnectedWorkers(void);
-static void StartOneBackgroundWorker(void);
+static void maybe_start_bgworker(void);
 static bool CreateOptsFile(int argc, char *argv[], char *fullprogname);
 static pid_t StartChildProcess(AuxProcType type);
 static void StartAutovacuumWorker(void);
@@ -1251,7 +1251,7 @@ PostmasterMain(int argc, char *argv[])
 	pmState = PM_STARTUP;
 
 	/* Some workers may be scheduled to start now */
-	StartOneBackgroundWorker();
+	maybe_start_bgworker();
 
 	status = ServerLoop();
 
@@ -1656,7 +1656,7 @@ ServerLoop(void)
 
 		/* Get other worker processes running, if needed */
 		if (StartWorkerNeeded || HaveCrashedWorker)
-			StartOneBackgroundWorker();
+			maybe_start_bgworker();
 
 		/*
 		 * Touch Unix socket and lock files every 58 minutes, to ensure that
@@ -2596,7 +2596,7 @@ reaper(SIGNAL_ARGS)
 				PgStatPID = pgstat_start();
 
 			/* some workers may be scheduled to start now */
-			StartOneBackgroundWorker();
+			maybe_start_bgworker();
 
 			/* at this point we are really open for business */
 			ereport(LOG,
@@ -4567,7 +4567,7 @@ SubPostmasterMain(int argc, char *argv[])
 
 		cookie = atoi(argv[1] + 15);
 		MyBgworkerEntry = find_bgworker_entry(cookie);
-		do_start_bgworker();
+		StartBackgroundWorker();
 	}
 	if (strcmp(argv[1], "--forkarch") == 0)
 	{
@@ -4670,7 +4670,7 @@ sigusr1_handler(SIGNAL_ARGS)
 		pmState = PM_HOT_STANDBY;
 
 		/* Some workers may be scheduled to start now */
-		StartOneBackgroundWorker();
+		maybe_start_bgworker();
 	}
 
 	if (CheckPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER) &&
@@ -5382,7 +5382,7 @@ bgworker_sigusr1_handler(SIGNAL_ARGS)
 }
 
 static void
-do_start_bgworker(void)
+StartBackgroundWorker(void)
 {
 	sigjmp_buf	local_sigjmp_buf;
 	char		buf[MAXPGPATH];
@@ -5573,7 +5573,7 @@ bgworker_forkexec(int cookie)
  * This code is heavily based on autovacuum.c, q.v.
  */
 static void
-start_bgworker(RegisteredBgWorker *rw)
+do_start_bgworker(RegisteredBgWorker *rw)
 {
 	pid_t		worker_pid;
 
@@ -5604,7 +5604,7 @@ start_bgworker(RegisteredBgWorker *rw)
 			/* Do NOT release postmaster's working memory context */
 
 			MyBgworkerEntry = &rw->rw_worker;
-			do_start_bgworker();
+			StartBackgroundWorker();
 			break;
 #endif
 		default:
@@ -5708,7 +5708,7 @@ assign_backendlist_entry(RegisteredBgWorker *rw)
  * system state requires it.
  */
 static void
-StartOneBackgroundWorker(void)
+maybe_start_bgworker(void)
 {
 	slist_iter	iter;
 	TimestampTz now = 0;
@@ -5776,7 +5776,7 @@ StartOneBackgroundWorker(void)
 			else
 				rw->rw_child_slot = MyPMChildSlot = AssignPostmasterChildSlot();
 
-			start_bgworker(rw); /* sets rw->rw_pid */
+			do_start_bgworker(rw); /* sets rw->rw_pid */
 
 			if (rw->rw_backend)
 			{
20130814_bgworker_refactor_master_v2.patchapplication/octet-stream; name=20130814_bgworker_refactor_master_v2.patchDownload
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 19a1398..fc73030 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -12,13 +12,24 @@
 
 #include "postgres.h"
 
+#include <unistd.h>
+#include <time.h>
+
 #include "miscadmin.h"
+#include "libpq/pqsignal.h"
 #include "postmaster/bgworker_internals.h"
 #include "storage/barrier.h"
+#include "storage/ipc.h"
+#include "storage/latch.h"
 #include "storage/lwlock.h"
 #include "storage/pmsignal.h"
+#include "storage/proc.h"
+#include "storage/procsignal.h"
 #include "storage/shmem.h"
+#include "tcop/tcopprot.h"
 #include "utils/ascii.h"
+#include "utils/ps_status.h"
+#include "utils/timeout.h"
 
 /*
  * The postmaster's list of registered background workers, in private memory.
@@ -354,6 +365,214 @@ SanityCheckBackgroundWorker(BackgroundWorker *worker, int elevel)
 	return true;
 }
 
+static void
+bgworker_quickdie(SIGNAL_ARGS)
+{
+	sigaddset(&BlockSig, SIGQUIT);      /* prevent nested calls */
+	PG_SETMASK(&BlockSig);
+
+	/*
+	 * We DO NOT want to run proc_exit() callbacks -- we're here because
+	 * shared memory may be corrupted, so we don't want to try to clean up our
+	 * transaction.  Just nail the windows shut and get out of town.  Now that
+	 * there's an atexit callback to prevent third-party code from breaking
+	 * things by calling exit() directly, we have to reset the callbacks
+	 * explicitly to make this work as intended.
+	 */
+	on_exit_reset();
+
+	/*
+	 * Note we do exit(0) here, not exit(2) like quickdie.  The reason is that
+	 * we don't want to be seen this worker as independently crashed, because
+	 * then postmaster would delay restarting it again afterwards.  If some
+	 * idiot DBA manually sends SIGQUIT to a random bgworker, the "dead man
+	 * switch" will ensure that postmaster sees this as a crash.
+	 */
+	exit(0);
+}
+
+/*
+ * Standard SIGTERM handler for background workers
+ */
+static void
+bgworker_die(SIGNAL_ARGS)
+{
+	PG_SETMASK(&BlockSig);
+
+	ereport(FATAL,
+			(errcode(ERRCODE_ADMIN_SHUTDOWN),
+			 errmsg("terminating background worker \"%s\" due to administrator command",
+					MyBgworkerEntry->bgw_name)));
+}
+
+/*
+ * 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;
+
+	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.
+ */
+void
+StartBackgroundWorker(void)
+{
+	sigjmp_buf	local_sigjmp_buf;
+	char		buf[MAXPGPATH];
+	BackgroundWorker *worker = MyBgworkerEntry;
+	bgworker_main_type entrypt;
+
+	if (worker == NULL)
+		elog(FATAL, "unable to find bgworker entry");
+
+	/* we are a postmaster subprocess now */
+	IsUnderPostmaster = true;
+	IsBackgroundWorker = true;
+
+	/* reset MyProcPid */
+	MyProcPid = getpid();
+
+	/* record Start Time for logging */
+	MyStartTime = time(NULL);
+
+	/* Identify myself via ps */
+	snprintf(buf, MAXPGPATH, "bgworker: %s", worker->bgw_name);
+	init_ps_display(buf, "", "", "");
+
+	SetProcessingMode(InitProcessing);
+
+	/* Apply PostAuthDelay */
+	if (PostAuthDelay > 0)
+		pg_usleep(PostAuthDelay * 1000000L);
+
+	/*
+	 * If possible, make this process a group leader, so that the postmaster
+	 * can signal any child processes too.
+	 */
+#ifdef HAVE_SETSID
+	if (setsid() < 0)
+		elog(FATAL, "setsid() failed: %m");
+#endif
+
+	/*
+	 * Set up signal handlers.
+	 */
+	if (worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION)
+	{
+		/*
+		 * SIGINT is used to signal canceling the current action
+		 */
+		pqsignal(SIGINT, StatementCancelHandler);
+		pqsignal(SIGUSR1, procsignal_sigusr1_handler);
+		pqsignal(SIGFPE, FloatExceptionHandler);
+
+		/* XXX Any other handlers needed here? */
+	}
+	else
+	{
+		pqsignal(SIGINT, SIG_IGN);
+		pqsignal(SIGUSR1, bgworker_sigusr1_handler);
+		pqsignal(SIGFPE, SIG_IGN);
+	}
+	pqsignal(SIGTERM, bgworker_die);
+	pqsignal(SIGHUP, SIG_IGN);
+
+	pqsignal(SIGQUIT, bgworker_quickdie);
+	InitializeTimeouts();       /* establishes SIGALRM handler */
+
+	pqsignal(SIGPIPE, SIG_IGN);
+	pqsignal(SIGUSR2, SIG_IGN);
+	pqsignal(SIGCHLD, SIG_DFL);
+
+	/*
+	 * If an exception is encountered, processing resumes here.
+	 *
+	 * See notes in postgres.c about the design of this coding.
+	 */
+	if (sigsetjmp(local_sigjmp_buf, 1) != 0)
+	{
+		/* Since not using PG_TRY, must reset error stack by hand */
+		error_context_stack = NULL;
+
+		/* Prevent interrupts while cleaning up */
+		HOLD_INTERRUPTS();
+
+		/* Report the error to the server log */
+		EmitErrorReport();
+
+		/*
+		 * Do we need more cleanup here?  For shmem-connected bgworkers, we
+		 * will call InitProcess below, which will install ProcKill as exit
+		 * callback.  That will take care of releasing locks, etc.
+		 */
+
+		/* and go away */
+		proc_exit(1);
+	}
+
+	/* 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).
+	 */
+#ifndef EXEC_BACKEND
+	if (worker->bgw_flags & BGWORKER_SHMEM_ACCESS)
+		InitProcess();
+#endif
+
+	/*
+	 * If bgw_main is set, we use that value as the initial entrypoint.
+	 * However, if the library containing the entrypoint wasn't loaded at
+	 * postmaster startup time, passing it as a direct function pointer is
+	 * not possible.  To work around that, we allow callers for whom a
+	 * function pointer is not available to pass a library name (which will
+	 * be loaded, if necessary) and a function name (which will be looked up
+	 * in the named library).
+	 */
+	if (worker->bgw_main != NULL)
+		entrypt = worker->bgw_main;
+	else
+		entrypt = (bgworker_main_type)
+			load_external_function(worker->bgw_library_name,
+								   worker->bgw_function_name,
+								   true, NULL);
+
+	/*
+	 * Note that in normal processes, we would call InitPostgres here.  For a
+	 * worker, however, we don't know what database to connect to, yet; so we
+	 * need to wait until the user code does it via
+	 * BackgroundWorkerInitializeConnection().
+	 */
+
+	/*
+	 * Now invoke the user-defined worker code
+	 */
+	entrypt(worker->bgw_main_arg);
+
+	/* ... and if it returns, we're done */
+	proc_exit(0);
+}
+
 /*
  * Register a new background worker while processing shared_preload_libraries.
  *
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 841b5ec..13f4147 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -383,7 +383,6 @@ static void dummy_handler(SIGNAL_ARGS);
 static void StartupPacketTimeoutHandler(void);
 static void CleanupBackend(int pid, int exitstatus);
 static bool CleanupBackgroundWorker(int pid, int exitstatus);
-static void do_start_bgworker(void);
 static void HandleChildCrash(int pid, int exitstatus, const char *procname);
 static void LogChildExit(int lev, const char *procname,
 			 int pid, int exitstatus);
@@ -409,7 +408,7 @@ static void TerminateChildren(int signal);
 
 static int	CountChildren(int target);
 static int	CountUnconnectedWorkers(void);
-static void StartOneBackgroundWorker(void);
+static void maybe_start_bgworker(void);
 static bool CreateOptsFile(int argc, char *argv[], char *fullprogname);
 static pid_t StartChildProcess(AuxProcType type);
 static void StartAutovacuumWorker(void);
@@ -1232,7 +1231,7 @@ PostmasterMain(int argc, char *argv[])
 	pmState = PM_STARTUP;
 
 	/* Some workers may be scheduled to start now */
-	StartOneBackgroundWorker();
+	maybe_start_bgworker();
 
 	status = ServerLoop();
 
@@ -1650,7 +1649,7 @@ ServerLoop(void)
 
 		/* Get other worker processes running, if needed */
 		if (StartWorkerNeeded || HaveCrashedWorker)
-			StartOneBackgroundWorker();
+			maybe_start_bgworker();
 
 		/*
 		 * Touch Unix socket and lock files every 58 minutes, to ensure that
@@ -2610,7 +2609,7 @@ reaper(SIGNAL_ARGS)
 				PgStatPID = pgstat_start();
 
 			/* some workers may be scheduled to start now */
-			StartOneBackgroundWorker();
+			maybe_start_bgworker();
 
 			/* at this point we are really open for business */
 			ereport(LOG,
@@ -4626,7 +4625,7 @@ SubPostmasterMain(int argc, char *argv[])
 
 		shmem_slot = atoi(argv[1] + 15);
 		MyBgworkerEntry = BackgroundWorkerEntry(shmem_slot);
-		do_start_bgworker();
+		StartBackgroundWorker();
 	}
 	if (strcmp(argv[1], "--forkarch") == 0)
 	{
@@ -4741,7 +4740,7 @@ sigusr1_handler(SIGNAL_ARGS)
 	}
 
 	if (start_bgworker)
-		StartOneBackgroundWorker();
+		maybe_start_bgworker();
 
 	if (CheckPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER) &&
 		PgArchPID != 0)
@@ -5253,208 +5252,6 @@ BackgroundWorkerUnblockSignals(void)
 	PG_SETMASK(&UnBlockSig);
 }
 
-static void
-bgworker_quickdie(SIGNAL_ARGS)
-{
-	sigaddset(&BlockSig, SIGQUIT);		/* prevent nested calls */
-	PG_SETMASK(&BlockSig);
-
-	/*
-	 * We DO NOT want to run proc_exit() callbacks -- we're here because
-	 * shared memory may be corrupted, so we don't want to try to clean up our
-	 * transaction.  Just nail the windows shut and get out of town.  Now that
-	 * there's an atexit callback to prevent third-party code from breaking
-	 * things by calling exit() directly, we have to reset the callbacks
-	 * explicitly to make this work as intended.
-	 */
-	on_exit_reset();
-
-	/*
-	 * Note we do exit(0) here, not exit(2) like quickdie.	The reason is that
-	 * we don't want to be seen this worker as independently crashed, because
-	 * then postmaster would delay restarting it again afterwards.	If some
-	 * idiot DBA manually sends SIGQUIT to a random bgworker, the "dead man
-	 * switch" will ensure that postmaster sees this as a crash.
-	 */
-	exit(0);
-}
-
-/*
- * Standard SIGTERM handler for background workers
- */
-static void
-bgworker_die(SIGNAL_ARGS)
-{
-	PG_SETMASK(&BlockSig);
-
-	ereport(FATAL,
-			(errcode(ERRCODE_ADMIN_SHUTDOWN),
-			 errmsg("terminating background worker \"%s\" due to administrator command",
-					MyBgworkerEntry->bgw_name)));
-}
-
-/*
- * 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;
-
-	latch_sigusr1_handler();
-
-	errno = save_errno;
-}
-
-static void
-do_start_bgworker(void)
-{
-	sigjmp_buf	local_sigjmp_buf;
-	char		buf[MAXPGPATH];
-	BackgroundWorker *worker = MyBgworkerEntry;
-	bgworker_main_type entrypt;
-
-	if (worker == NULL)
-		elog(FATAL, "unable to find bgworker entry");
-
-	/* we are a postmaster subprocess now */
-	IsUnderPostmaster = true;
-	IsBackgroundWorker = true;
-
-	/* reset MyProcPid */
-	MyProcPid = getpid();
-
-	/* record Start Time for logging */
-	MyStartTime = time(NULL);
-
-	/* Identify myself via ps */
-	snprintf(buf, MAXPGPATH, "bgworker: %s", worker->bgw_name);
-	init_ps_display(buf, "", "", "");
-
-	SetProcessingMode(InitProcessing);
-
-	/* Apply PostAuthDelay */
-	if (PostAuthDelay > 0)
-		pg_usleep(PostAuthDelay * 1000000L);
-
-	/*
-	 * If possible, make this process a group leader, so that the postmaster
-	 * can signal any child processes too.
-	 */
-#ifdef HAVE_SETSID
-	if (setsid() < 0)
-		elog(FATAL, "setsid() failed: %m");
-#endif
-
-	/*
-	 * Set up signal handlers.
-	 */
-	if (worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION)
-	{
-		/*
-		 * SIGINT is used to signal canceling the current action
-		 */
-		pqsignal(SIGINT, StatementCancelHandler);
-		pqsignal(SIGUSR1, procsignal_sigusr1_handler);
-		pqsignal(SIGFPE, FloatExceptionHandler);
-
-		/* XXX Any other handlers needed here? */
-	}
-	else
-	{
-		pqsignal(SIGINT, SIG_IGN);
-		pqsignal(SIGUSR1, bgworker_sigusr1_handler);
-		pqsignal(SIGFPE, SIG_IGN);
-	}
-	pqsignal(SIGTERM, bgworker_die);
-	pqsignal(SIGHUP, SIG_IGN);
-
-	pqsignal(SIGQUIT, bgworker_quickdie);
-	InitializeTimeouts();		/* establishes SIGALRM handler */
-
-	pqsignal(SIGPIPE, SIG_IGN);
-	pqsignal(SIGUSR2, SIG_IGN);
-	pqsignal(SIGCHLD, SIG_DFL);
-
-	/*
-	 * If an exception is encountered, processing resumes here.
-	 *
-	 * See notes in postgres.c about the design of this coding.
-	 */
-	if (sigsetjmp(local_sigjmp_buf, 1) != 0)
-	{
-		/* Since not using PG_TRY, must reset error stack by hand */
-		error_context_stack = NULL;
-
-		/* Prevent interrupts while cleaning up */
-		HOLD_INTERRUPTS();
-
-		/* Report the error to the server log */
-		EmitErrorReport();
-
-		/*
-		 * Do we need more cleanup here?  For shmem-connected bgworkers, we
-		 * will call InitProcess below, which will install ProcKill as exit
-		 * callback.  That will take care of releasing locks, etc.
-		 */
-
-		/* and go away */
-		proc_exit(1);
-	}
-
-	/* 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).
-	 */
-#ifndef EXEC_BACKEND
-	if (worker->bgw_flags & BGWORKER_SHMEM_ACCESS)
-		InitProcess();
-#endif
-
-	/*
-	 * If bgw_main is set, we use that value as the initial entrypoint.
-	 * However, if the library containing the entrypoint wasn't loaded at
-	 * postmaster startup time, passing it as a direct function pointer is
-	 * not possible.  To work around that, we allow callers for whom a
-	 * function pointer is not available to pass a library name (which will
-	 * be loaded, if necessary) and a function name (which will be looked up
-	 * in the named library).
-	 */
-	if (worker->bgw_main != NULL)
-		entrypt = worker->bgw_main;
-	else
-		entrypt = (bgworker_main_type)
-			load_external_function(worker->bgw_library_name,
-								   worker->bgw_function_name,
-								   true, NULL);
-
-	/*
-	 * Note that in normal processes, we would call InitPostgres here.	For a
-	 * worker, however, we don't know what database to connect to, yet; so we
-	 * need to wait until the user code does it via
-	 * BackgroundWorkerInitializeConnection().
-	 */
-
-	/*
-	 * Now invoke the user-defined worker code
-	 */
-	entrypt(worker->bgw_main_arg);
-
-	/* ... and if it returns, we're done */
-	proc_exit(0);
-}
-
 #ifdef EXEC_BACKEND
 static pid_t
 bgworker_forkexec(int shmem_slot)
@@ -5483,7 +5280,7 @@ bgworker_forkexec(int shmem_slot)
  * This code is heavily based on autovacuum.c, q.v.
  */
 static void
-start_bgworker(RegisteredBgWorker *rw)
+do_start_bgworker(RegisteredBgWorker *rw)
 {
 	pid_t		worker_pid;
 
@@ -5514,7 +5311,7 @@ start_bgworker(RegisteredBgWorker *rw)
 			/* Do NOT release postmaster's working memory context */
 
 			MyBgworkerEntry = &rw->rw_worker;
-			do_start_bgworker();
+			StartBackgroundWorker();
 			break;
 #endif
 		default:
@@ -5618,7 +5415,7 @@ assign_backendlist_entry(RegisteredBgWorker *rw)
  * system state requires it.
  */
 static void
-StartOneBackgroundWorker(void)
+maybe_start_bgworker(void)
 {
 	slist_mutable_iter	iter;
 	TimestampTz now = 0;
@@ -5689,7 +5486,7 @@ StartOneBackgroundWorker(void)
 			else
 				rw->rw_child_slot = MyPMChildSlot = AssignPostmasterChildSlot();
 
-			start_bgworker(rw); /* sets rw->rw_pid */
+			do_start_bgworker(rw); /* sets rw->rw_pid */
 
 			if (rw->rw_backend)
 			{
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 0bb897b..b3a79bf 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -82,6 +82,9 @@ typedef struct BackgroundWorker
 	Datum		bgw_main_arg;
 } BackgroundWorker;
 
+/* Function to start a background worker, called from postmaster.c */
+extern void StartBackgroundWorker(void);
+
 /* Register a new bgworker during shared_preload_libraries */
 extern void RegisterBackgroundWorker(BackgroundWorker *worker);
 
#12Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#11)
Re: Regarding BGworkers

On Wed, Aug 14, 2013 at 8:04 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Wed, Aug 14, 2013 at 10:10 AM, Robert Haas <robertmhaas@gmail.com> wrote:

I think Alvaro's suggestion is better. It's shorter, and makes clear
that at most one will be started.

OK cool. Here are patches for 9.3 and master respecting those comments.

Thanks, committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers