Fix race condition between shutdown and unstarted background workers.

If a database shutdown (smart or fast) is commanded between the time
some process decides to request a new background worker and the time
that the postmaster can launch that worker, then nothing happens
because the postmaster won't launch any bgworkers once it's exited
PM_RUN state.  This is fine ... unless the requesting process is
waiting for that worker to finish (or even for it to start); in that
case the requestor is stuck, and only manual intervention will get us
to the point of being able to shut down.

To fix, cancel pending requests for workers when the postmaster sends
shutdown (SIGTERM) signals, and similarly cancel any new requests that
arrive after that point.  (We can optimize things slightly by only
doing the cancellation for workers that have waiters.)  To fit within
the existing bgworker APIs, the "cancel" is made to look like the
worker was started and immediately stopped, causing deregistration of
the bgworker entry.  Waiting processes would have to deal with
premature worker exit anyway, so this should introduce no bugs that
weren't there before.  We do have a side effect that registration
records for restartable bgworkers might disappear when theoretically
they should have remained in place; but since we're shutting down,
that shouldn't matter.

Back-patch to v10.  There might be value in putting this into 9.6
as well, but the management of bgworkers is a bit different there
(notably see 8ff518699) and I'm not convinced it's worth the effort
to validate the patch for that branch.

Discussion: https://postgr.es/m/661570.1608673226@sss.pgh.pa.us

2 files changed, 87 insertions, 13 deletions

diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 2db83d666f..850b652f9f 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -232,13 +232,15 @@ FindRegisteredWorkerBySlotNumber(int slotno)
 }
 
 /*
- * Notice changes to shared memory made by other backends.  This code
- * runs in the postmaster, so we must be very careful not to assume that
- * shared memory contents are sane.  Otherwise, a rogue backend could take
- * out the postmaster.
+ * Notice changes to shared memory made by other backends.
+ * Accept new worker requests only if allow_new_workers is true.
+ *
+ * This code runs in the postmaster, so we must be very careful not to assume
+ * that shared memory contents are sane.  Otherwise, a rogue backend could
+ * take out the postmaster.
  */
 void
-BackgroundWorkerStateChange(void)
+BackgroundWorkerStateChange(bool allow_new_workers)
 {
 	int			slotno;
 
@@ -299,6 +301,15 @@ BackgroundWorkerStateChange(void)
 		}
 
 		/*
+		 * If we aren't allowing new workers, then immediately mark it for
+		 * termination; the next stanza will take care of cleaning it up.
+		 * Doing this ensures that any process waiting for the worker will get
+		 * awoken, even though the worker will never be allowed to run.
+		 */
+		if (!allow_new_workers)
+			slot->terminate = true;
+
+		/*
 		 * If the worker is marked for termination, we don't need to add it to
 		 * the registered workers list; we can just free the slot. However, if
 		 * bgw_notify_pid is set, the process that registered the worker may
@@ -505,11 +516,54 @@ BackgroundWorkerStopNotifications(pid_t pid)
 }
 
 /*
+ * Cancel any not-yet-started worker requests that have waiting processes.
+ *
+ * This is called during a normal ("smart" or "fast") database shutdown.
+ * After this point, no new background workers will be started, so anything
+ * that might be waiting for them needs to be kicked off its wait.  We do
+ * that by cancelling the bgworker registration entirely, which is perhaps
+ * overkill, but since we're shutting down it does not matter whether the
+ * registration record sticks around.
+ *
+ * This function should only be called from the postmaster.
+ */
+void
+ForgetUnstartedBackgroundWorkers(void)
+{
+	slist_mutable_iter iter;
+
+	slist_foreach_modify(iter, &BackgroundWorkerList)
+	{
+		RegisteredBgWorker *rw;
+		BackgroundWorkerSlot *slot;
+
+		rw = slist_container(RegisteredBgWorker, rw_lnode, iter.cur);
+		Assert(rw->rw_shmem_slot < max_worker_processes);
+		slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
+
+		/* If it's not yet started, and there's someone waiting ... */
+		if (slot->pid == InvalidPid &&
+			rw->rw_worker.bgw_notify_pid != 0)
+		{
+			/* ... then zap it, and notify the waiter */
+			int			notify_pid = rw->rw_worker.bgw_notify_pid;
+
+			ForgetBackgroundWorker(&iter);
+			if (notify_pid != 0)
+				kill(notify_pid, SIGUSR1);
+		}
+	}
+}
+
+/*
  * Reset background worker crash state.
  *
  * We assume that, after a crash-and-restart cycle, background workers without
  * the never-restart flag should be restarted immediately, instead of waiting
- * for bgw_restart_time to elapse.
+ * for bgw_restart_time to elapse.  On the other hand, workers with that flag
+ * should be forgotten immediately, since we won't ever restart them.
+ *
+ * This function should only be called from the postmaster.
  */
 void
 ResetBackgroundWorkerCrashTimes(void)
@@ -549,6 +603,11 @@ ResetBackgroundWorkerCrashTimes(void)
 			 * resetting.
 			 */
 			rw->rw_crashed_at = 0;
+
+			/*
+			 * If there was anyone waiting for it, they're history.
+			 */
+			rw->rw_worker.bgw_notify_pid = 0;
 		}
 	}
 }
@@ -1098,6 +1157,9 @@ GetBackgroundWorkerPid(BackgroundWorkerHandle *handle, pid_t *pidp)
  * returned.  However, if the postmaster has died, we give up and return
  * BGWH_POSTMASTER_DIED, since it that case we know that startup will not
  * take place.
+ *
+ * The caller *must* have set our PID as the worker's bgw_notify_pid,
+ * else we will not be awoken promptly when the worker's state changes.
  */
 BgwHandleStatus
 WaitForBackgroundWorkerStartup(BackgroundWorkerHandle *handle, pid_t *pidp)
@@ -1140,6 +1202,9 @@ WaitForBackgroundWorkerStartup(BackgroundWorkerHandle *handle, pid_t *pidp)
  * and then return BGWH_STOPPED.  However, if the postmaster has died, we give
  * up and return BGWH_POSTMASTER_DIED, because it's the postmaster that
  * notifies us when a worker's state changes.
+ *
+ * The caller *must* have set our PID as the worker's bgw_notify_pid,
+ * else we will not be awoken promptly when the worker's state changes.
  */
 BgwHandleStatus
 WaitForBackgroundWorkerShutdown(BackgroundWorkerHandle *handle)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e1822c7923..257301ed8a 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -3761,6 +3761,13 @@ PostmasterStateMachine(void)
 	 */
 	if (pmState == PM_STOP_BACKENDS)
 	{
+		/*
+		 * Forget any pending requests for background workers, since we're no
+		 * longer willing to launch any new workers.  (If additional requests
+		 * arrive, BackgroundWorkerStateChange will reject them.)
+		 */
+		ForgetUnstartedBackgroundWorkers();
+
 		/* Signal all backend children except walsenders */
 		SignalSomeChildren(SIGTERM,
 						   BACKEND_TYPE_ALL - BACKEND_TYPE_WALSND);
@@ -5151,13 +5158,6 @@ sigusr1_handler(SIGNAL_ARGS)
 	PG_SETMASK(&BlockSig);
 #endif
 
-	/* Process background worker state change. */
-	if (CheckPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE))
-	{
-		BackgroundWorkerStateChange();
-		StartWorkerNeeded = true;
-	}
-
 	/*
 	 * RECOVERY_STARTED and BEGIN_HOT_STANDBY signals are ignored in
 	 * unexpected states. If the startup process quickly starts up, completes
@@ -5203,6 +5203,7 @@ sigusr1_handler(SIGNAL_ARGS)
 
 		pmState = PM_RECOVERY;
 	}
+
 	if (CheckPostmasterSignal(PMSIGNAL_BEGIN_HOT_STANDBY) &&
 		pmState == PM_RECOVERY && Shutdown == NoShutdown)
 	{
@@ -5228,6 +5229,14 @@ sigusr1_handler(SIGNAL_ARGS)
 		StartWorkerNeeded = true;
 	}
 
+	/* Process background worker state changes. */
+	if (CheckPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE))
+	{
+		/* Accept new worker requests only if not stopping. */
+		BackgroundWorkerStateChange(pmState < PM_STOP_BACKENDS);
+		StartWorkerNeeded = true;
+	}
+
 	if (StartWorkerNeeded || HaveCrashedWorker)
 		maybe_start_bgworkers();