Use INTERRUPT_GENERAL for bgworker state change notification

As part of a project to remove the reliance on PIDs to identify
backends, backends that register dynamic background workers will now
receive INTERRUPT_GENERAL sent directly by the postmaster when the
worker state changes.  Previously, the postmaster would send a SIGUSR1
signal, which relied on the ProcSignal system's handler setting the
interrupt flag, with the same end effect.  Now that intermediate step
is skipped.

The field bgw_notify_pid still exists for backwards compatibility, but
has no effect and may be removed in a later release.
RegisterBackgroundWorker() now automatically assumes that the caller
would like state change notifications delivered to its proc number,
unless BGW_NO_NOTIFY is set in bgw_flags.

Removing other cases of PIDs from the bgworker API is left for later
work; this patch is concerned only with removing a dependency on
ProcSignal infrastructure that is due to be removed by a later commit.

This represents the first use of SendInterrupt() in the postmaster.
It might seem more natural to use condition variables in the
wait-for-state-change functions, so that anyone with a handle can
wait, but condition variables as currently implemented would be a lot
less robust than simple interrupts.

Author: Thomas Munro <thomas.munro@gmail.com>
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/CA%2BhUKG%2B3MkS21yK4jL4cgZywdnnGKiBg0jatoV6kzaniBmcqbQ%40mail.gmail.com

Author: hlinnaka

contrib/pg_prewarm/autoprewarm.c

@@ -817,9 +817,6 @@ apw_start_leader_worker(void)
 		return;
 	}
 
-	/* must set notify PID to wait for startup */
-	worker.bgw_notify_pid = MyProcPid;
-
 	if (!RegisterDynamicBackgroundWorker(&worker, &handle))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_RESOURCES),
@@ -853,9 +850,6 @@ apw_start_database_worker(void)
 	strcpy(worker.bgw_name, "autoprewarm worker");
 	strcpy(worker.bgw_type, "autoprewarm worker");
 
-	/* must set notify PID to wait for shutdown */
-	worker.bgw_notify_pid = MyProcPid;
-
 	if (!RegisterDynamicBackgroundWorker(&worker, &handle))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_RESOURCES),

doc/src/sgml/bgworker.sgml

@@ -63,7 +63,7 @@ typedef struct BackgroundWorker
     char        bgw_function_name[BGW_MAXLEN];
     Datum       bgw_main_arg;
     char        bgw_extra[BGW_EXTRALEN];
-    pid_t       bgw_notify_pid;
+    pid_t       bgw_notify_pid;			/* not used */
 } BackgroundWorker;
 </programlisting>
   </para>
@@ -108,6 +108,18 @@ typedef struct BackgroundWorker
      </listitem>
     </varlistentry>
 
+    <varlistentry>
+     <term><literal>BGWORKER_NO_NOTIFY</literal></term>
+     <listitem>
+      <para>
+       <indexterm><primary>BGWORKER_NO_NOTIFY</primary></indexterm>
+       Normally, the backend that registers a dynamic worker will be notified
+       with <literal>INTERRUPT_GENERAL</literal> when the workers state changes, which allows the
+       caller to wait for the worker to start and shut down.  That can be
+       suppressed by setting this flag.
+      </para>
+     </listitem>
+    </varlistentry>
    </variablelist>
 
   </para>
@@ -181,12 +193,8 @@ typedef struct BackgroundWorker
   </para>
 
   <para>
-   <structfield>bgw_notify_pid</structfield> is the PID of a PostgreSQL
-   backend process to which the postmaster should send <literal>SIGUSR1</literal>
-   when the process is started or exits.  It should be 0 for workers registered
-   at postmaster startup time, or when the backend registering the worker does
-   not wish to wait for the worker to start up.  Otherwise, it should be
-   initialized to <literal>MyProcPid</literal>.
+   <structfield>bgw_notify_pid</structfield> is not used and may be removed
+   in a future release.
   </para>
 
   <para>Once running, the process can connect to a database by calling
@@ -258,10 +266,7 @@ typedef struct BackgroundWorker
 
   <para>
    In some cases, a process which registers a background worker may wish to
-   wait for the worker to start up.  This can be accomplished by initializing
-   <structfield>bgw_notify_pid</structfield> to <literal>MyProcPid</literal> and
-   then passing the <type>BackgroundWorkerHandle *</type> obtained at
-   registration time to
+   wait for the worker to start up.  This can be accomplished with the
    <function>WaitForBackgroundWorkerStartup(<parameter>BackgroundWorkerHandle
    *handle</parameter>, <parameter>pid_t *</parameter>)</function> function.
    This function will block until the postmaster has attempted to start the

src/backend/access/transam/parallel.c

@@ -600,7 +600,6 @@ LaunchParallelWorkers(ParallelContext *pcxt)
 	sprintf(worker.bgw_library_name, "postgres");
 	sprintf(worker.bgw_function_name, "ParallelWorkerMain");
 	worker.bgw_main_arg = UInt32GetDatum(dsm_segment_handle(pcxt->seg));
-	worker.bgw_notify_pid = MyProcPid;
 
 	/*
 	 * Start workers.

src/backend/postmaster/bgworker.c

@@ -78,6 +78,8 @@ typedef struct BackgroundWorkerSlot
 	pid_t		pid;			/* InvalidPid = not started yet; 0 = dead */
 	uint64		generation;		/* incremented when slot is recycled */
 	BackgroundWorker worker;
+	int			notify_pmchild;
+	ProcNumber	notify_proc_number;
 } BackgroundWorkerSlot;
 
 /*
@@ -192,8 +194,9 @@ BackgroundWorkerShmemInit(void)
 			slot->terminate = false;
 			slot->pid = InvalidPid;
 			slot->generation = 0;
+			slot->notify_pmchild = 0;
+			slot->notify_proc_number = INVALID_PROC_NUMBER;
 			rw->rw_shmem_slot = slotno;
-			rw->rw_worker.bgw_notify_pid = 0;	/* might be reinit after crash */
 			memcpy(&slot->worker, &rw->rw_worker, sizeof(BackgroundWorker));
 			++slotno;
 		}
@@ -234,6 +237,14 @@ FindRegisteredWorkerBySlotNumber(int slotno)
 	return NULL;
 }
 
+ProcNumber
+GetNotifyProcNumberForRegisteredWorker(RegisteredBgWorker *rw)
+{
+	BackgroundWorkerSlot *slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
+
+	return slot->notify_proc_number;
+}
+
 /*
  * Notice changes to shared memory made by other backends.
  * Accept new worker requests only if allow_new_workers is true.
@@ -315,29 +326,29 @@ BackgroundWorkerStateChange(bool allow_new_workers)
 		/*
 		 * 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
-		 * need to know that we've processed the terminate request, so be sure
-		 * to signal it.
+		 * bgw_notify_proc_number is set, the process that registered the
+		 * worker may need to know that we've processed the terminate request,
+		 * so be sure to signal it.
 		 */
 		if (slot->terminate)
 		{
-			int			notify_pid;
+			int			notify_proc_number;
 
 			/*
 			 * We need a memory barrier here to make sure that the load of
-			 * bgw_notify_pid and the update of parallel_terminate_count
-			 * complete before the store to in_use.
+			 * bgw_notify_proc_number and the update of
+			 * parallel_terminate_count complete before the store to in_use.
 			 */
-			notify_pid = slot->worker.bgw_notify_pid;
+			notify_proc_number = slot->notify_proc_number;
 			if ((slot->worker.bgw_flags & BGWORKER_CLASS_PARALLEL) != 0)
 				BackgroundWorkerData->parallel_terminate_count++;
 			slot->pid = 0;
 
 			pg_memory_barrier();
 			slot->in_use = false;
 
-			if (notify_pid != 0)
-				kill(notify_pid, SIGUSR1);
+			if (notify_proc_number != INVALID_PROC_NUMBER)
+				SendInterrupt(INTERRUPT_GENERAL, notify_proc_number);
 
 			continue;
 		}
@@ -383,23 +394,6 @@ BackgroundWorkerStateChange(bool allow_new_workers)
 		rw->rw_worker.bgw_main_arg = slot->worker.bgw_main_arg;
 		memcpy(rw->rw_worker.bgw_extra, slot->worker.bgw_extra, BGW_EXTRALEN);
 
-		/*
-		 * Copy the PID to be notified about state changes, but only if the
-		 * postmaster knows about a backend with that PID.  It isn't an error
-		 * if the postmaster doesn't know about the PID, because the backend
-		 * that requested the worker could have died (or been killed) just
-		 * after doing so.  Nonetheless, at least until we get some experience
-		 * with how this plays out in the wild, log a message at a relative
-		 * high debug level.
-		 */
-		rw->rw_worker.bgw_notify_pid = slot->worker.bgw_notify_pid;
-		if (!PostmasterMarkPIDForWorkerNotify(rw->rw_worker.bgw_notify_pid))
-		{
-			elog(DEBUG1, "worker notification PID %d is not valid",
-				 (int) rw->rw_worker.bgw_notify_pid);
-			rw->rw_worker.bgw_notify_pid = 0;
-		}
-
 		/* Initialize postmaster bookkeeping. */
 		rw->rw_pid = 0;
 		rw->rw_crashed_at = 0;
@@ -421,7 +415,7 @@ BackgroundWorkerStateChange(bool allow_new_workers)
  * NOTE: The entry is unlinked from BackgroundWorkerList.  If the caller is
  * iterating through it, better use a mutable iterator!
  *
- * Caller is responsible for notifying bgw_notify_pid, if appropriate.
+ * Caller is responsible for notifying bgw_notify_proc_number, if appropriate.
  *
  * This function must be invoked only in the postmaster.
  */
@@ -466,8 +460,8 @@ ReportBackgroundWorkerPID(RegisteredBgWorker *rw)
 	slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
 	slot->pid = rw->rw_pid;
 
-	if (rw->rw_worker.bgw_notify_pid != 0)
-		kill(rw->rw_worker.bgw_notify_pid, SIGUSR1);
+	if (slot->notify_proc_number != INVALID_PROC_NUMBER)
+		SendInterrupt(INTERRUPT_GENERAL, slot->notify_proc_number);
 }
 
 /*
@@ -483,12 +477,12 @@ void
 ReportBackgroundWorkerExit(RegisteredBgWorker *rw)
 {
 	BackgroundWorkerSlot *slot;
-	int			notify_pid;
+	ProcNumber	notify_proc_number;
 
 	Assert(rw->rw_shmem_slot < max_worker_processes);
 	slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
 	slot->pid = rw->rw_pid;
-	notify_pid = rw->rw_worker.bgw_notify_pid;
+	notify_proc_number = slot->notify_proc_number;
 
 	/*
 	 * If this worker is slated for deregistration, do that before notifying
@@ -501,27 +495,34 @@ ReportBackgroundWorkerExit(RegisteredBgWorker *rw)
 		rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART)
 		ForgetBackgroundWorker(rw);
 
-	if (notify_pid != 0)
-		kill(notify_pid, SIGUSR1);
+	if (notify_proc_number != INVALID_PROC_NUMBER)
+		SendInterrupt(INTERRUPT_GENERAL, notify_proc_number);
 }
 
 /*
- * Cancel SIGUSR1 notifications for a PID belonging to an exiting backend.
+ * Cancel notifications for a PID belonging to an exiting backend.
  *
  * This function should only be called from the postmaster.
  */
 void
-BackgroundWorkerStopNotifications(pid_t pid)
+BackgroundWorkerStopNotifications(int pmchild)
 {
 	dlist_iter	iter;
 
 	dlist_foreach(iter, &BackgroundWorkerList)
 	{
 		RegisteredBgWorker *rw;
+		BackgroundWorkerSlot *slot;
 
 		rw = dlist_container(RegisteredBgWorker, rw_lnode, iter.cur);
-		if (rw->rw_worker.bgw_notify_pid == pid)
-			rw->rw_worker.bgw_notify_pid = 0;
+		Assert(rw->rw_shmem_slot < max_worker_processes);
+		slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
+
+		if (slot->notify_pmchild == pmchild)
+		{
+			slot->notify_pmchild = 0;
+			slot->notify_proc_number = INVALID_PROC_NUMBER;
+		}
 	}
 }
 
@@ -553,14 +554,14 @@ ForgetUnstartedBackgroundWorkers(void)
 
 		/* If it's not yet started, and there's someone waiting ... */
 		if (slot->pid == InvalidPid &&
-			rw->rw_worker.bgw_notify_pid != 0)
+			slot->notify_proc_number != INVALID_PROC_NUMBER)
 		{
 			/* ... then zap it, and notify the waiter */
-			int			notify_pid = rw->rw_worker.bgw_notify_pid;
+			int			notify_proc_number = slot->notify_proc_number;
 
 			ForgetBackgroundWorker(rw);
-			if (notify_pid != 0)
-				kill(notify_pid, SIGUSR1);
+			if (notify_proc_number != INVALID_PROC_NUMBER)
+				SendInterrupt(INTERRUPT_GENERAL, notify_proc_number);
 		}
 	}
 }
@@ -613,11 +614,6 @@ 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;
 		}
 	}
 }
@@ -981,15 +977,6 @@ RegisterBackgroundWorker(BackgroundWorker *worker)
 	if (!SanityCheckBackgroundWorker(worker, LOG))
 		return;
 
-	if (worker->bgw_notify_pid != 0)
-	{
-		ereport(LOG,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("background worker \"%s\": only dynamic background workers can request notification",
-						worker->bgw_name)));
-		return;
-	}
-
 	/*
 	 * Enforce maximum number of workers.  Note this is overly restrictive: we
 	 * could allow more non-shmem-connected workers, because these don't count
@@ -1105,6 +1092,25 @@ RegisterDynamicBackgroundWorker(BackgroundWorker *worker,
 			if (parallel)
 				BackgroundWorkerData->parallel_register_count++;
 
+			if ((slot->worker.bgw_flags & BGWORKER_SHMEM_ACCESS) != 0 &&
+				(slot->worker.bgw_flags & BGWORKER_NO_NOTIFY) == 0)
+			{
+				/*
+				 * Set notify_proc_number so that postmaster will send us an
+				 * interrupt. Also remember the pmchild slot number;
+				 * postmaster needs it to detect when we exit, to disarm the
+				 * notification.
+				 */
+				slot->notify_pmchild = MyPMChildSlot;
+				slot->notify_proc_number = MyProcNumber;
+			}
+			else
+			{
+				/* No notifications. */
+				slot->notify_pmchild = 0;
+				slot->notify_proc_number = INVALID_PROC_NUMBER;
+			}
+
 			/*
 			 * Make sure postmaster doesn't see the slot as in use before it
 			 * sees the new contents.
@@ -1205,8 +1211,9 @@ GetBackgroundWorkerPid(BackgroundWorkerHandle *handle, pid_t *pidp)
  * 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.
+ * This works only if the worker was registered with BGWORKER_SHMEM_ACCESS and
+ * without BGWORKER_NO_NOTIFY, else we will not be awoken promptly when the
+ * worker's state changes.
  */
 BgwHandleStatus
 WaitForBackgroundWorkerStartup(BackgroundWorkerHandle *handle, pid_t *pidp)
@@ -1250,8 +1257,9 @@ WaitForBackgroundWorkerStartup(BackgroundWorkerHandle *handle, pid_t *pidp)
  * 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.
+ * This works only if the worker was registered with BGWORKER_SHMEM_ACCESS and
+ * without BGWORKER_NO_NOTIFY, else we will not be awoken promptly when the
+ * worker's state changes.
  */
 BgwHandleStatus
 WaitForBackgroundWorkerShutdown(BackgroundWorkerHandle *handle)

src/backend/postmaster/postmaster.c

@@ -4008,15 +4008,15 @@ maybe_start_bgworkers(void)
 		{
 			if (rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART)
 			{
-				int			notify_pid;
+				ProcNumber	notify_proc_number;
 
-				notify_pid = rw->rw_worker.bgw_notify_pid;
+				notify_proc_number = GetNotifyProcNumberForRegisteredWorker(rw);
 
 				ForgetBackgroundWorker(rw);
 
 				/* Report worker is gone now. */
-				if (notify_pid != 0)
-					kill(notify_pid, SIGUSR1);
+				if (notify_proc_number != INVALID_PROC_NUMBER)
+					SendInterrupt(INTERRUPT_GENERAL, notify_proc_number);
 
 				continue;
 			}

src/backend/replication/logical/launcher.c

@@ -496,7 +496,6 @@ logicalrep_worker_launch(LogicalRepWorkerType wtype,
 	}
 
 	bgw.bgw_restart_time = BGW_NEVER_RESTART;
-	bgw.bgw_notify_pid = MyProcPid;
 	bgw.bgw_main_arg = Int32GetDatum(slot);
 
 	if (!RegisterDynamicBackgroundWorker(&bgw, &bgw_handle))
@@ -939,7 +938,6 @@ ApplyLauncherRegister(void)
 	snprintf(bgw.bgw_type, BGW_MAXLEN,
 			 "logical replication launcher");
 	bgw.bgw_restart_time = 5;
-	bgw.bgw_notify_pid = 0;
 	bgw.bgw_main_arg = (Datum) 0;
 
 	RegisterBackgroundWorker(&bgw);

src/backend/storage/ipc/interrupt.c

@@ -102,6 +102,9 @@ RaiseInterrupt(InterruptType reason)
 
 /*
  * Set an interrupt flag in another backend.
+ *
+ * Note: This can also be called from the postmaster, so be careful to not
+ * trust the contents of shared memory.
  */
 void
 SendInterrupt(InterruptType reason, ProcNumber pgprocno)