From a07e45853abd231b4ec54fc1e82b7c4a669ce593 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 5 Aug 2021 19:33:00 -0700
Subject: [PATCH] Remove support for background workers without
 BGWORKER_SHMEM_ACCESS.

Background workers without shared memory access have been broken on
EXEC_BACKEND / windows builds since shortly after background workers have been
introduced without that being reported. Clearly they are not commonly used.

The problem is that bgworker startup requires to be attached to shared memory
in EXEC_BACKEND child processes. StartBackgroundWorker() detaches from shared
memory for unconnected workers, but at that point we already have initialized
subsystems referencing shared memory.

Fixing this problem is not entirely trivial, so removing the option seems the
best way forward. In most use cases the advantages of being connected to
shared memory far outweigh the disadvantages.

As there have been no reports about this issue so far, we have decided that it
is not worth trying to address the problem in the back branches.

Per discussion with Alvaro Herrera, Robert Haas and Tom Lane.

Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20210802065116.j763tz3vz4egqy3w@alap3.anarazel.de
---
 src/include/postmaster/bgworker.h   |  1 +
 src/backend/postmaster/bgworker.c   | 67 +++++++++++------------------
 src/backend/postmaster/postmaster.c | 23 ++++------
 doc/src/sgml/bgworker.sgml          | 10 ++---
 4 files changed, 38 insertions(+), 63 deletions(-)

diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index 8e9ef7c7bfa..6d4122b4c7e 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -48,6 +48,7 @@
 
 /*
  * Pass this flag to have your worker be able to connect to shared memory.
+ * This flag is required.
  */
 #define BGWORKER_SHMEM_ACCESS						0x0001
 
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 11c4ceddbf6..c05f5006393 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -652,17 +652,24 @@ static bool
 SanityCheckBackgroundWorker(BackgroundWorker *worker, int elevel)
 {
 	/* sanity check for flags */
+
+	/*
+	 * We used to support workers not connected to shared memory, but don't
+	 * anymore. Thus this is a required flag now. We're not removing the flag
+	 * for compatibility reasons and because the flag still provides some
+	 * signal when reading code.
+	 */
+	if (!(worker->bgw_flags & BGWORKER_SHMEM_ACCESS))
+	{
+		ereport(elevel,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("background worker \"%s\": background worker without shared memory access are not supported",
+						worker->bgw_name)));
+		return false;
+	}
+
 	if (worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION)
 	{
-		if (!(worker->bgw_flags & BGWORKER_SHMEM_ACCESS))
-		{
-			ereport(elevel,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("background worker \"%s\": must attach to shared memory in order to request a database connection",
-							worker->bgw_name)));
-			return false;
-		}
-
 		if (worker->bgw_start_time == BgWorkerStart_PostmasterStart)
 		{
 			ereport(elevel,
@@ -745,20 +752,6 @@ StartBackgroundWorker(void)
 	MyBackendType = B_BG_WORKER;
 	init_ps_display(worker->bgw_name);
 
-	/*
-	 * 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)
-	{
-		ShutdownLatchSupport();
-		dsm_detach_all();
-		PGSharedMemoryDetach();
-	}
-
 	SetProcessingMode(InitProcessing);
 
 	/* Apply PostAuthDelay */
@@ -832,29 +825,19 @@ StartBackgroundWorker(void)
 	PG_exception_stack = &local_sigjmp_buf;
 
 	/*
-	 * If the background worker request shared memory access, set that up now;
-	 * else, detach all shared memory segments.
+	 * 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 (worker->bgw_flags & BGWORKER_SHMEM_ACCESS)
-	{
-		/*
-		 * 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();
+	InitProcess();
 #endif
 
-		/*
-		 * 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();
-	}
+	/*
+	 * Early initialization.
+	 */
+	BaseInit();
 
 	/*
 	 * Look up the entry point function, loading its library if necessary.
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index fc0bc8d99ee..9c2c98614aa 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -3302,26 +3302,21 @@ CleanupBackgroundWorker(int pid,
 		}
 
 		/*
-		 * Additionally, for shared-memory-connected workers, just like a
-		 * backend, any exit status other than 0 or 1 is considered a crash
-		 * and causes a system-wide restart.
+		 * Additionally, just like a backend, any exit status other than 0 or
+		 * 1 is considered a crash and causes a system-wide restart.
 		 */
-		if ((rw->rw_worker.bgw_flags & BGWORKER_SHMEM_ACCESS) != 0)
+		if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
 		{
-			if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
-			{
-				HandleChildCrash(pid, exitstatus, namebuf);
-				return true;
-			}
+			HandleChildCrash(pid, exitstatus, namebuf);
+			return true;
 		}
 
 		/*
-		 * We must release the postmaster child slot whether this worker is
-		 * connected to shared memory or not, but we only treat it as a crash
-		 * if it is in fact connected.
+		 * We must release the postmaster child slot. If the worker failed to
+		 * do so, it did not clean up after itself, requiring a crash-restart
+		 * cycle.
 		 */
-		if (!ReleasePostmasterChildSlot(rw->rw_child_slot) &&
-			(rw->rw_worker.bgw_flags & BGWORKER_SHMEM_ACCESS) != 0)
+		if (!ReleasePostmasterChildSlot(rw->rw_child_slot))
 		{
 			HandleChildCrash(pid, exitstatus, namebuf);
 			return true;
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 7fd673ab54e..d34acfc220d 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -11,8 +11,8 @@
   PostgreSQL can be extended to run user-supplied code in separate processes.
   Such processes are started, stopped and monitored by <command>postgres</command>,
   which permits them to have a lifetime closely linked to the server's status.
-  These processes have the option to attach to <productname>PostgreSQL</productname>'s
-  shared memory area and to connect to databases internally; they can also run
+  These processes are attached to <productname>PostgreSQL</productname>'s
+  shared memory area and have the option to connect to databases internally; they can also run
   multiple transactions serially, just like a regular client-connected server
   process.  Also, by linking to <application>libpq</application> they can connect to the
   server and behave like a regular client application.
@@ -89,11 +89,7 @@ typedef struct BackgroundWorker
      <listitem>
       <para>
        <indexterm><primary>BGWORKER_SHMEM_ACCESS</primary></indexterm>
-       Requests shared memory access.  Workers without shared memory access
-       cannot access any of <productname>PostgreSQL's</productname> shared
-       data structures, such as heavyweight or lightweight locks, shared
-       buffers, or any custom data structures which the worker itself may
-       wish to create and use.
+       Requests shared memory access.  This flag is required.
       </para>
      </listitem>
     </varlistentry>
-- 
2.32.0.rc2

