v4-0003-Refactor-how-MyBgWorkerEntry-is-passed-through-th.patch

From bb079f9c87dacddb6ee896ee62c0bc4ce19c7ccb Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 1 Dec 2023 12:51:50 +0200
Subject: [PATCH v4 3/4] Refactor how 'MyBgWorkerEntry' is passed through the
 fork functions.

The BackgroundWorker struct is now passed the same way as the Port
struct. Seems more consistent.

Also add explicit 'has_port' and 'has_worker' flags to
BackendParameters. Previously, I looked at 'bgw_name[0]' to determine
if BackgroundWorker the struct is valid, but that caused regression
failures in test_shm_mq. Turns out that test_shm_mq leaves bgw_name
empty. That should probably be fixed, and perhaps we should even
assert that it's not empty, because I think it's bad form to not set
it and is almost certainly an oversight. bgw_name is only used in log
messages and such, so it cause any serious consequences
though. But having explicit 'has_port' and 'has_worker' flags is
cheap, so I'm inclined to add them anyway.

XXX: to be squashed with previous commit. I'm having this as a
separate commit here to show the diff from v3 of this patch set, for
easier review.
---
 src/backend/postmaster/postmaster.c | 118 ++++++++++++++++------------
 1 file changed, 67 insertions(+), 51 deletions(-)

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 135dc5a5f49..444dd8c6892 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -476,7 +476,7 @@ typedef struct
 #endif							/* WIN32 */
 
 static pid_t backend_forkexec(Port *port);
-static pid_t internal_forkexec(int argc, char *argv[], Port *port);
+static pid_t internal_forkexec(int argc, char *argv[], Port *port, BackgroundWorker *worker);
 
 /* Type for a socket that can be inherited to a client process */
 #ifdef WIN32
@@ -495,8 +495,11 @@ typedef int InheritableSocket;
  */
 typedef struct
 {
+	bool		has_port;
 	Port		port;
 	InheritableSocket portsocket;
+	bool		has_worker;
+	BackgroundWorker MyBgworkerEntry;
 	char		DataDir[MAXPGPATH];
 	int32		MyCancelKey;
 	int			MyPMChildSlot;
@@ -540,17 +543,15 @@ typedef struct
 #endif
 	char		my_exec_path[MAXPGPATH];
 	char		pkglib_path[MAXPGPATH];
-
-	BackgroundWorker MyBgworkerEntry;
 } BackendParameters;
 
-static void read_backend_variables(char *id, Port *port);
-static void restore_backend_variables(BackendParameters *param, Port *port);
+static void read_backend_variables(char *id, Port **port, BackgroundWorker **worker);
+static void restore_backend_variables(BackendParameters *param, Port **port, BackgroundWorker **worker);
 
 #ifndef WIN32
-static bool save_backend_variables(BackendParameters *param, Port *port);
+static bool save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *worker);
 #else
-static bool save_backend_variables(BackendParameters *param, Port *port,
+static bool save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *worker,
 								   HANDLE childProcess, pid_t childPid);
 #endif
 
@@ -4443,11 +4444,7 @@ BackendRun(Port *port)
 pid_t
 postmaster_forkexec(int argc, char *argv[])
 {
-	Port		port;
-
-	/* This entry point passes dummy values for the Port variables */
-	memset(&port, 0, sizeof(port));
-	return internal_forkexec(argc, argv, &port);
+	return internal_forkexec(argc, argv, NULL, NULL);
 }
 
 /*
@@ -4472,7 +4469,7 @@ backend_forkexec(Port *port)
 	av[ac] = NULL;
 	Assert(ac < lengthof(av));
 
-	return internal_forkexec(ac, av, port);
+	return internal_forkexec(ac, av, port, NULL);
 }
 
 #ifndef WIN32
@@ -4484,7 +4481,7 @@ backend_forkexec(Port *port)
  * - fork():s, and then exec():s the child process
  */
 static pid_t
-internal_forkexec(int argc, char *argv[], Port *port)
+internal_forkexec(int argc, char *argv[], Port *port, BackgroundWorker *worker)
 {
 	static unsigned long tmpBackendFileNum = 0;
 	pid_t		pid;
@@ -4492,7 +4489,7 @@ internal_forkexec(int argc, char *argv[], Port *port)
 	BackendParameters param;
 	FILE	   *fp;
 
-	if (!save_backend_variables(&param, port))
+	if (!save_backend_variables(&param, port, worker))
 		return -1;				/* log made by save_backend_variables */
 
 	/* Calculate name for temp file */
@@ -4576,7 +4573,7 @@ internal_forkexec(int argc, char *argv[], Port *port)
  *	 file is complete.
  */
 static pid_t
-internal_forkexec(int argc, char *argv[], Port *port)
+internal_forkexec(int argc, char *argv[], Port *port, BackgroundWorker *worker)
 {
 	int			retry_count = 0;
 	STARTUPINFO si;
@@ -4673,7 +4670,7 @@ retry:
 		return -1;
 	}
 
-	if (!save_backend_variables(param, port, pi.hProcess, pi.dwProcessId))
+	if (!save_backend_variables(param, port, worker, pi.hProcess, pi.dwProcessId))
 	{
 		/*
 		 * log made by save_backend_variables, but we have to clean up the
@@ -4790,7 +4787,8 @@ retry:
 void
 SubPostmasterMain(int argc, char *argv[])
 {
-	Port		port;
+	Port		*port;
+	BackgroundWorker *worker;
 
 	/* In EXEC_BACKEND case we will not have inherited these settings */
 	IsPostmasterEnvironment = true;
@@ -4804,8 +4802,7 @@ SubPostmasterMain(int argc, char *argv[])
 		elog(FATAL, "invalid subpostmaster invocation");
 
 	/* Read in the variables file */
-	memset(&port, 0, sizeof(Port));
-	read_backend_variables(argv[2], &port);
+	read_backend_variables(argv[2], &port, &worker);
 
 	/* Close the postmaster's sockets (as soon as we know them) */
 	ClosePostmasterPorts(strcmp(argv[1], "--forklog") == 0);
@@ -4906,7 +4903,7 @@ SubPostmasterMain(int argc, char *argv[])
 		 * PGPROC slots, we have already initialized libpq and are able to
 		 * report the error to the client.
 		 */
-		BackendInitialize(&port);
+		BackendInitialize(port);
 
 		/* Restore basic shared memory pointers */
 		InitShmemAccess(UsedShmemSegAddr);
@@ -4918,7 +4915,7 @@ SubPostmasterMain(int argc, char *argv[])
 		AttachSharedMemoryStructs();
 
 		/* And run the backend */
-		BackendRun(&port);		/* does not return */
+		BackendRun(port);		/* does not return */
 	}
 	if (strcmp(argv[1], "--forkaux") == 0)
 	{
@@ -4978,6 +4975,7 @@ SubPostmasterMain(int argc, char *argv[])
 		/* Attach process to shared data structures */
 		AttachSharedMemoryStructs();
 
+		MyBgworkerEntry = worker;
 		BackgroundWorkerMain();
 	}
 	if (strcmp(argv[1], "--forklog") == 0)
@@ -5640,20 +5638,15 @@ bgworker_forkexec(BackgroundWorker *worker)
 {
 	char	   *av[10];
 	int			ac = 0;
-	pid_t		result;
 
 	av[ac++] = "postgres";
 	av[ac++] = "--forkbgworker";
-	av[ac++] = NULL;			/* filled in by postmaster_forkexec */
+	av[ac++] = NULL;			/* filled in by internal_forkexec */
 	av[ac] = NULL;
 
 	Assert(ac < lengthof(av));
 
-	MyBgworkerEntry = worker;
-	result = postmaster_forkexec(ac, av);
-	MyBgworkerEntry = NULL;
-
-	return result;
+	return internal_forkexec(ac, av, NULL, worker);
 }
 #endif
 
@@ -6027,16 +6020,36 @@ static void read_inheritable_socket(SOCKET *dest, InheritableSocket *src);
 /* Save critical backend variables into the BackendParameters struct */
 #ifndef WIN32
 static bool
-save_backend_variables(BackendParameters *param, Port *port)
+save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *worker)
 #else
 static bool
-save_backend_variables(BackendParameters *param, Port *port,
+save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *worker,
 					   HANDLE childProcess, pid_t childPid)
 #endif
 {
-	memcpy(&param->port, port, sizeof(Port));
-	if (!write_inheritable_socket(&param->portsocket, port->sock, childPid))
-		return false;
+	if (port)
+	{
+		memcpy(&param->port, port, sizeof(Port));
+		if (!write_inheritable_socket(&param->portsocket, port->sock, childPid))
+			return false;
+		param->has_port = true;
+	}
+	else
+	{
+		memset(&param->port, 0, sizeof(Port));
+		param->has_port = false;
+	}
+
+	if (worker)
+	{
+		memcpy(&param->MyBgworkerEntry, worker, sizeof(BackgroundWorker));
+		param->has_worker = true;
+	}
+	else
+	{
+		memset(&param->MyBgworkerEntry, 0, sizeof(BackgroundWorker));
+		param->has_worker = false;
+	}
 
 	strlcpy(param->DataDir, DataDir, MAXPGPATH);
 
@@ -6094,11 +6107,6 @@ save_backend_variables(BackendParameters *param, Port *port,
 
 	strlcpy(param->pkglib_path, pkglib_path, MAXPGPATH);
 
-	if (MyBgworkerEntry)
-		memcpy(&param->MyBgworkerEntry, MyBgworkerEntry, sizeof(BackgroundWorker));
-	else
-		memset(&param->MyBgworkerEntry, 0, sizeof(BackgroundWorker));
-
 	return true;
 }
 
@@ -6197,7 +6205,7 @@ read_inheritable_socket(SOCKET *dest, InheritableSocket *src)
 #endif
 
 static void
-read_backend_variables(char *id, Port *port)
+read_backend_variables(char *id, Port **port, BackgroundWorker **worker)
 {
 	BackendParameters param;
 
@@ -6264,15 +6272,30 @@ read_backend_variables(char *id, Port *port)
 	}
 #endif
 
-	restore_backend_variables(&param, port);
+	restore_backend_variables(&param, port, worker);
 }
 
 /* Restore critical backend variables from the BackendParameters struct */
 static void
-restore_backend_variables(BackendParameters *param, Port *port)
+restore_backend_variables(BackendParameters *param, Port **port, BackgroundWorker **worker)
 {
-	memcpy(port, &param->port, sizeof(Port));
-	read_inheritable_socket(&port->sock, &param->portsocket);
+	if (param->has_port)
+	{
+		*port = (Port *) MemoryContextAlloc(TopMemoryContext, sizeof(Port));
+		memcpy(*port, &param->port, sizeof(Port));
+		read_inheritable_socket(&(*port)->sock, &param->portsocket);
+	}
+	else
+		*port = NULL;
+
+	if (param->has_worker)
+	{
+		*worker = (BackgroundWorker *)
+			MemoryContextAlloc(TopMemoryContext, sizeof(BackgroundWorker));
+		memcpy(*worker, &param->MyBgworkerEntry, sizeof(BackgroundWorker));
+	}
+	else
+		*worker = NULL;
 
 	SetDataDir(param->DataDir);
 
@@ -6327,13 +6350,6 @@ restore_backend_variables(BackendParameters *param, Port *port)
 
 	strlcpy(pkglib_path, param->pkglib_path, MAXPGPATH);
 
-	if (param->MyBgworkerEntry.bgw_name[0] != '\0')
-	{
-		MyBgworkerEntry = (BackgroundWorker *)
-			MemoryContextAlloc(TopMemoryContext, sizeof(BackgroundWorker));
-		memcpy(MyBgworkerEntry, &param->MyBgworkerEntry, sizeof(BackgroundWorker));
-	}
-
 	/*
 	 * We need to restore fd.c's counts of externally-opened FDs; to avoid
 	 * confusion, be sure to do this after restoring max_safe_fds.  (Note:
-- 
2.39.2