From dfb5814036358afa44670f377ef9768f782753a1 Mon Sep 17 00:00:00 2001
From: Mike Palmiotto <mike.palmiotto@crunchydata.com>
Date: Wed, 11 Mar 2020 00:24:42 +0000
Subject: [PATCH v2 05/12] Add PgstatCollectorType to subprocess struct

Introduce two new fields to handle subprocess control:

1) needs_shmem controls whether or not the subprocess needs to attach
to shmem. Up until now, all subprocesses needed to attach. This field
paves the way for easier handling of subprocesses which do not.

2) fork_prep allows StartSubprocess to do some preparation for
fork/exec. This is necessary to split out any functionality that
previously happened in Start functions prior to fork/exec.
---
 src/backend/postmaster/pgstat.c     | 86 +++--------------------------
 src/backend/postmaster/postmaster.c | 39 +++++++------
 src/backend/postmaster/subprocess.c | 28 ++++++++++
 src/include/pgstat.h                |  4 +-
 src/include/postmaster/subprocess.h |  8 ++-
 5 files changed, 65 insertions(+), 100 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index f9287b7942..6d4f2ace56 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -48,9 +48,9 @@
 #include "pg_trace.h"
 #include "pgstat.h"
 #include "postmaster/autovacuum.h"
-#include "postmaster/fork_process.h"
 #include "postmaster/interrupt.h"
 #include "postmaster/postmaster.h"
+#include "postmaster/subprocess.h"
 #include "replication/walsender.h"
 #include "storage/backendid.h"
 #include "storage/dsm.h"
@@ -275,11 +275,6 @@ static instr_time total_func_time;
  * Local function forward declarations
  * ----------
  */
-#ifdef EXEC_BACKEND
-static pid_t pgstat_forkexec(void);
-#endif
-
-NON_EXEC_STATIC void PgstatCollectorMain(int argc, char *argv[]) pg_attribute_noreturn();
 static void pgstat_beshutdown_hook(int code, Datum arg);
 
 static PgStat_StatDBEntry *pgstat_get_db_entry(Oid databaseid, bool create);
@@ -685,46 +680,15 @@ pgstat_reset_all(void)
 	pgstat_reset_remove_files(PGSTAT_STAT_PERMANENT_DIRECTORY);
 }
 
-#ifdef EXEC_BACKEND
-
 /*
- * pgstat_forkexec() -
+ * PgstatCollectorPrep
  *
- * Format up the arglist for, then fork and exec, statistics collector process
- */
-static pid_t
-pgstat_forkexec(void)
-{
-	char	   *av[10];
-	int			ac = 0;
-
-	av[ac++] = "postgres";
-	av[ac++] = "--forkcol";
-	av[ac++] = NULL;			/* filled in by postmaster_forkexec */
-
-	av[ac] = NULL;
-	Assert(ac < lengthof(av));
-
-	return postmaster_forkexec(ac, av);
-}
-#endif							/* EXEC_BACKEND */
-
-
-/*
- * pgstat_start() -
- *
- *	Called from postmaster at startup or after an existing collector
- *	died.  Attempt to fire up a fresh statistics collector.
- *
- *	Returns PID of child process, or 0 if fail.
- *
- *	Note: if fail, we will be called again from the postmaster main loop.
+ *	Called from StartSubprocess to prepare a new collector fork
  */
 int
-pgstat_start(void)
+PgstatCollectorPrep(int argc, char *argv[])
 {
 	time_t		curtime;
-	pid_t		pgStatPid;
 
 	/*
 	 * Check that the socket is there, else pgstat_init failed and we can do
@@ -740,46 +704,10 @@ pgstat_start(void)
 	 * from the postmaster main loop, we will get another chance later.
 	 */
 	curtime = time(NULL);
-	if ((unsigned int) (curtime - last_pgstat_start_time) <
+	if ((unsigned int) (curtime - last_pgstat_start_time) >=
 		(unsigned int) PGSTAT_RESTART_INTERVAL)
-		return 0;
-	last_pgstat_start_time = curtime;
+		last_pgstat_start_time = curtime;
 
-	/*
-	 * Okay, fork off the collector.
-	 */
-#ifdef EXEC_BACKEND
-	switch ((pgStatPid = pgstat_forkexec()))
-#else
-	switch ((pgStatPid = fork_process()))
-#endif
-	{
-		case -1:
-			ereport(LOG,
-					(errmsg("could not fork statistics collector: %m")));
-			return 0;
-
-#ifndef EXEC_BACKEND
-		case 0:
-			/* in postmaster child ... */
-			InitPostmasterChild();
-
-			/* Close the postmaster's sockets */
-			ClosePostmasterPorts(false);
-
-			/* Drop our connection to postmaster's shared memory, as well */
-			dsm_detach_all();
-			PGSharedMemoryDetach();
-
-			PgstatCollectorMain(0, NULL);
-			break;
-#endif
-
-		default:
-			return (int) pgStatPid;
-	}
-
-	/* shouldn't get here */
 	return 0;
 }
 
@@ -4326,7 +4254,7 @@ pgstat_send_bgwriter(void)
  *	The argc/argv parameters are valid only in EXEC_BACKEND case.
  * ----------
  */
-NON_EXEC_STATIC void
+void
 PgstatCollectorMain(int argc, char *argv[])
 {
 	int			len;
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 21bbbf8194..e2ad879452 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1781,7 +1781,7 @@ ServerLoop(void)
 		/* If we have lost the stats collector, try to start a new one */
 		if (PgStatPID == 0 &&
 			(pmState == PM_RUN || pmState == PM_HOT_STANDBY))
-			PgStatPID = pgstat_start();
+			PgStatPID = StartSubprocess(PgstatCollectorType);
 
 		/* If we have lost the archiver, try to start a new one. */
 		if (PgArchPID == 0 && PgArchStartupAllowed())
@@ -3057,7 +3057,7 @@ reaper(SIGNAL_ARGS)
 			if (PgArchStartupAllowed() && PgArchPID == 0)
 				PgArchPID = pgarch_start();
 			if (PgStatPID == 0)
-				PgStatPID = pgstat_start();
+				PgStatPID = StartSubprocess(PgstatCollectorType);
 
 			/* workers may be scheduled to start now */
 			maybe_start_bgworkers();
@@ -3219,7 +3219,7 @@ reaper(SIGNAL_ARGS)
 				LogChildExit(LOG, _("statistics collector process"),
 							 pid, exitstatus);
 			if (pmState == PM_RUN || pmState == PM_HOT_STANDBY)
-				PgStatPID = pgstat_start();
+				PgStatPID = StartSubprocess(PgstatCollectorType);
 			continue;
 		}
 
@@ -5069,12 +5069,6 @@ SubPostmasterMain(int argc, char *argv[])
 
 		PgArchiverMain(argc, argv); /* does not return */
 	}
-	if (strcmp(argv[1], "--forkcol") == 0)
-	{
-		/* Do not want to attach to shared memory */
-
-		PgstatCollectorMain(argc, argv);	/* does not return */
-	}
 	if (strcmp(argv[1], "--forklog") == 0)
 	{
 		/* Do not want to attach to shared memory */
@@ -5097,14 +5091,17 @@ SubPostmasterMain(int argc, char *argv[])
 	}
 	else
 	{
-		/* Restore basic shared memory pointers */
-		InitShmemAccess(UsedShmemSegAddr);
+		if (MySubprocess->needs_shmem)
+		{
+			/* Restore basic shared memory pointers */
+			InitShmemAccess(UsedShmemSegAddr);
 
-		/* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
-		InitProcess();
+			/* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
+			InitProcess();
 
-		/* Attach process to shared data structures */
-		CreateSharedMemoryAndSemaphores();
+			/* Attach process to shared data structures */
+			CreateSharedMemoryAndSemaphores();
+		}
 
 		MySubprocess->entrypoint(argc - 2, argv + 2);
 	}
@@ -5224,7 +5221,7 @@ sigusr1_handler(SIGNAL_ARGS)
 		 * Likewise, start other special children as needed.
 		 */
 		Assert(PgStatPID == 0);
-		PgStatPID = pgstat_start();
+		PgStatPID = StartSubprocess(PgstatCollectorType);
 
 		ereport(LOG,
 				(errmsg("database system is ready to accept read only connections")));
@@ -5475,6 +5472,16 @@ StartSubprocess(SubprocessType type)
 		MemoryContextDelete(PostmasterContext);
 		PostmasterContext = NULL;
 
+		/*
+		 * Drop connection to postmaster's shared memory if the subprocess
+		 * doesn't need it.
+		 */
+		if (!MySubprocess->needs_shmem)
+		{
+			dsm_detach_all();
+			PGSharedMemoryDetach();
+		}
+
 		if (MySubprocess->needs_aux_proc)
 			AuxiliaryProcessMain(argc, argv);
 		else
diff --git a/src/backend/postmaster/subprocess.c b/src/backend/postmaster/subprocess.c
index 0fbd067310..ee44ef0789 100644
--- a/src/backend/postmaster/subprocess.c
+++ b/src/backend/postmaster/subprocess.c
@@ -12,6 +12,7 @@
  */
 #include "postgres.h"
 #include "bootstrap/bootstrap.h"
+#include "pgstat.h"
 #include "postmaster/autovacuum.h"
 #include "postmaster/bgwriter.h"
 #include "postmaster/postmaster.h"
@@ -28,6 +29,8 @@ static PgSubprocess process_types[] = {
 		.name = "boot",
 		.desc = "checker",
 		.needs_aux_proc = true,
+		.needs_shmem = true,
+		.fork_prep = NULL,
 		.entrypoint = CheckerModeMain,
 		.fork_failure = NULL
 	},
@@ -35,6 +38,8 @@ static PgSubprocess process_types[] = {
 		.name = "boot",
 		.desc = "bootstrap",
 		.needs_aux_proc = true,
+		.needs_shmem = true,
+		.fork_prep = NULL,
 		.entrypoint = BootstrapModeMain,
 		.fork_failure = NULL
 	},
@@ -42,6 +47,8 @@ static PgSubprocess process_types[] = {
 		.name = "boot",
 		.desc = "startup",
 		.needs_aux_proc = true,
+		.needs_shmem = true,
+		.fork_prep = NULL,
 		.entrypoint = StartupProcessMain,
 		.fork_failure = StartupForkFailure
 	},
@@ -49,6 +56,8 @@ static PgSubprocess process_types[] = {
 		.name = "boot",
 		.desc = "background writer",
 		.needs_aux_proc = true,
+		.needs_shmem = true,
+		.fork_prep = NULL,
 		.entrypoint = BackgroundWriterMain,
 		.fork_failure = NULL
 	},
@@ -56,6 +65,8 @@ static PgSubprocess process_types[] = {
 		.name = "boot",
 		.desc = "checkpointer",
 		.needs_aux_proc = true,
+		.needs_shmem = true,
+		.fork_prep = NULL,
 		.entrypoint = CheckpointerMain,
 		.fork_failure = NULL
 	},
@@ -63,6 +74,8 @@ static PgSubprocess process_types[] = {
 		.name = "boot",
 		.desc = "wal writer",
 		.needs_aux_proc = true,
+		.needs_shmem = true,
+		.fork_prep = NULL,
 		.entrypoint = WalWriterMain,
 		.fork_failure = NULL
 	},
@@ -70,6 +83,8 @@ static PgSubprocess process_types[] = {
 		.name = "boot",
 		.desc = "wal receiver",
 		.needs_aux_proc = true,
+		.needs_shmem = true,
+		.fork_prep = NULL,
 		.entrypoint = WalReceiverMain,
 		.fork_failure = NULL
 	},
@@ -77,6 +92,8 @@ static PgSubprocess process_types[] = {
 		.name = "avlauncher",
 		.desc = "autovacuum launcher",
 		.needs_aux_proc = false,
+		.needs_shmem = true,
+		.fork_prep = NULL,
 		.entrypoint = AutoVacLauncherMain,
 		.fork_failure = NULL
 	},
@@ -84,8 +101,19 @@ static PgSubprocess process_types[] = {
 		.name = "avworker",
 		.desc = "autovacuum worker",
 		.needs_aux_proc = false,
+		.needs_shmem = true,
+		.fork_prep = NULL,
 		.entrypoint = AutoVacWorkerMain,
 		.fork_failure = NULL
+	},
+	{
+		.name = "col",
+		.desc = "statistics collector",
+		.needs_aux_proc = false,
+		.needs_shmem = false,
+		.fork_prep = PgstatCollectorPrep,
+		.entrypoint = PgstatCollectorMain,
+		.fork_failure = NULL
 	}
 };
 
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 1a19921f80..92943f5cff 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -1224,10 +1224,8 @@ extern int	pgstat_start(void);
 extern void pgstat_reset_all(void);
 extern void allow_immediate_pgstat_restart(void);
 
-#ifdef EXEC_BACKEND
+extern int PgstatCollectorPrep(int argc, char *argv[]);
 extern void PgstatCollectorMain(int argc, char *argv[]) pg_attribute_noreturn();
-#endif
-
 
 /* ----------
  * Functions called from backends
diff --git a/src/include/postmaster/subprocess.h b/src/include/postmaster/subprocess.h
index e11c24cc62..a7ab036b7b 100644
--- a/src/include/postmaster/subprocess.h
+++ b/src/include/postmaster/subprocess.h
@@ -25,12 +25,14 @@ typedef enum
 	WalReceiverType,	/* end of Auxiliary Process Forks */
 	AutoVacuumLauncherType,
 	AutoVacuumWorkerType,
+	PgstatCollectorType,
 
 	NUMSUBPROCESSTYPES			/* Must be last! */
 } SubprocessType;
 
-typedef void (*SubprocessEntryPoint) (int argc, char *argv[]);
-typedef bool (*SubprocessForkFailure) (int fork_errno);
+typedef int		(*SubprocessPrep) (int argc, char *argv[]);
+typedef void	(*SubprocessEntryPoint) (int argc, char *argv[]);
+typedef bool	(*SubprocessForkFailure) (int fork_errno);
 
 /* Current subprocess initializer */
 extern void InitMySubprocess(SubprocessType type);
@@ -40,6 +42,8 @@ typedef struct PgSubprocess
 	const char			   *name;
 	const char			   *desc;
 	bool					needs_aux_proc;
+	bool					needs_shmem;
+	SubprocessPrep			fork_prep;
 	SubprocessEntryPoint	entrypoint;
 	SubprocessForkFailure	fork_failure;
 } PgSubprocess;
-- 
2.17.0

