On 30/11/2023 20:44, Tristan Partin wrote:
Patches 1-3 seem committable as-is.
Thanks for the review! I'm focusing on patches 1-3 now, and will come
back to the rest after committing 1-3.
There was one test failure with EXEC_BACKEND from patch 2, in
'test_shm_mq'. In restore_backend_variables() I checked if 'bgw_name' is
empty to decide if the BackgroundWorker struct is filled in or not, but
it turns out that 'test_shm_mq' doesn't fill in bgw_name. It probably
should, I think that's an oversight in 'test_shm_mq', but that's a
separate issue.
I did some more refactoring of patch 2, to fix that and to improve it in
general. The BackgroundWorker struct is now passed through the
fork-related functions similarly to the Port struct. That seems more
consistent.
Attached is new version of these patches. For easier review, I made the
new refactorings compared in a new commit 0003. I will squash that
before pushing, but this makes it easier to see what changed.
Barring any new feedback or issues, I will commit these.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 6dd44d7057e535eb441c9ab8fda1bbdd8079f2fb Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Thu, 30 Nov 2023 00:01:25 +0200
Subject: [PATCH v4 1/4] Refactor CreateSharedMemoryAndSemaphores
For clarity, have separate functions for *creating* the shared memory
and semaphores at postmaster or single-user backend startup, and
for *attaching* to existing shared memory structures in EXEC_BACKEND
case. CreateSharedMemoryAndSemaphores() is now called only at
postmaster startup, and a new AttachSharedMemoryStructs() function is
called at backend startup in EXEC_BACKEND mode.
Reviewed-by: Tristan Partin, Andres Freund
Discussion: https://www.postgresql.org/message-id/7a59b073-5b5b-151e-7ed3-8b01ff7ce...@iki.fi
---
src/backend/postmaster/postmaster.c | 20 +--
src/backend/replication/walreceiver.c | 3 +-
src/backend/storage/ipc/ipci.c | 178 +++++++++++++++-----------
src/backend/storage/lmgr/proc.c | 2 +-
src/include/storage/ipc.h | 3 +
5 files changed, 117 insertions(+), 89 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7a5cd06c5c9..b03e88e6702 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4909,11 +4909,11 @@ SubPostmasterMain(int argc, char *argv[])
/* Restore basic shared memory pointers */
InitShmemAccess(UsedShmemSegAddr);
- /* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
+ /* Need a PGPROC to run AttachSharedMemoryStructs */
InitProcess();
/* Attach process to shared data structures */
- CreateSharedMemoryAndSemaphores();
+ AttachSharedMemoryStructs();
/* And run the backend */
BackendRun(&port); /* does not return */
@@ -4927,11 +4927,11 @@ SubPostmasterMain(int argc, char *argv[])
/* Restore basic shared memory pointers */
InitShmemAccess(UsedShmemSegAddr);
- /* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
+ /* Need a PGPROC to run AttachSharedMemoryStructs */
InitAuxiliaryProcess();
/* Attach process to shared data structures */
- CreateSharedMemoryAndSemaphores();
+ AttachSharedMemoryStructs();
auxtype = atoi(argv[3]);
AuxiliaryProcessMain(auxtype); /* does not return */
@@ -4941,11 +4941,11 @@ SubPostmasterMain(int argc, char *argv[])
/* Restore basic shared memory pointers */
InitShmemAccess(UsedShmemSegAddr);
- /* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
+ /* Need a PGPROC to run AttachSharedMemoryStructs */
InitProcess();
/* Attach process to shared data structures */
- CreateSharedMemoryAndSemaphores();
+ AttachSharedMemoryStructs();
AutoVacLauncherMain(argc - 2, argv + 2); /* does not return */
}
@@ -4954,11 +4954,11 @@ SubPostmasterMain(int argc, char *argv[])
/* Restore basic shared memory pointers */
InitShmemAccess(UsedShmemSegAddr);
- /* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
+ /* Need a PGPROC to run AttachSharedMemoryStructs */
InitProcess();
/* Attach process to shared data structures */
- CreateSharedMemoryAndSemaphores();
+ AttachSharedMemoryStructs();
AutoVacWorkerMain(argc - 2, argv + 2); /* does not return */
}
@@ -4972,11 +4972,11 @@ SubPostmasterMain(int argc, char *argv[])
/* Restore basic shared memory pointers */
InitShmemAccess(UsedShmemSegAddr);
- /* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
+ /* Need a PGPROC to run AttachSharedMemoryStructs */
InitProcess();
/* Attach process to shared data structures */
- CreateSharedMemoryAndSemaphores();
+ AttachSharedMemoryStructs();
/* Fetch MyBgworkerEntry from shared memory */
shmem_slot = atoi(argv[1] + 15);
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 2398167f495..26ded928a71 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -193,7 +193,7 @@ WalReceiverMain(void)
TimeLineID startpointTLI;
TimeLineID primaryTLI;
bool first_stream;
- WalRcvData *walrcv = WalRcv;
+ WalRcvData *walrcv;
TimestampTz now;
char *err;
char *sender_host = NULL;
@@ -203,6 +203,7 @@ WalReceiverMain(void)
* WalRcv should be set up already (if we are a backend, we inherit this
* by fork() or EXEC_BACKEND mechanism from the postmaster).
*/
+ walrcv = WalRcv;
Assert(walrcv != NULL);
/*
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index a3d8eacb8dc..2225a4a6e60 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -58,6 +58,8 @@ shmem_startup_hook_type shmem_startup_hook = NULL;
static Size total_addin_request = 0;
+static void CreateOrAttachShmemStructs(void);
+
/*
* RequestAddinShmemSpace
* Request that extra shmem space be allocated for use by
@@ -156,9 +158,106 @@ CalculateShmemSize(int *num_semaphores)
return size;
}
+#ifdef EXEC_BACKEND
+/*
+ * AttachSharedMemoryStructs
+ * Initialize a postmaster child process's access to shared memory
+ * structures.
+ *
+ * In !EXEC_BACKEND mode, we inherit everything through the fork, and this
+ * isn't needed.
+ */
+void
+AttachSharedMemoryStructs(void)
+{
+ /* InitProcess must've been called already */
+ Assert(MyProc != NULL);
+ Assert(IsUnderPostmaster);
+
+ CreateOrAttachShmemStructs();
+
+ /*
+ * Now give loadable modules a chance to set up their shmem allocations
+ */
+ if (shmem_startup_hook)
+ shmem_startup_hook();
+}
+#endif
+
/*
* CreateSharedMemoryAndSemaphores
* Creates and initializes shared memory and semaphores.
+ */
+void
+CreateSharedMemoryAndSemaphores(void)
+{
+ PGShmemHeader *shim;
+ PGShmemHeader *seghdr;
+ Size size;
+ int numSemas;
+
+ Assert(!IsUnderPostmaster);
+
+ /* Compute the size of the shared-memory block */
+ size = CalculateShmemSize(&numSemas);
+ elog(DEBUG3, "invoking IpcMemoryCreate(size=%zu)", size);
+
+ /*
+ * Create the shmem segment
+ */
+ seghdr = PGSharedMemoryCreate(size, &shim);
+
+ /*
+ * Make sure that huge pages are never reported as "unknown" while the
+ * server is running.
+ */
+ Assert(strcmp("unknown",
+ GetConfigOption("huge_pages_status", false, false)) != 0);
+
+ InitShmemAccess(seghdr);
+
+ /*
+ * Create semaphores
+ */
+ PGReserveSemaphores(numSemas);
+
+ /*
+ * If spinlocks are disabled, initialize emulation layer (which depends on
+ * semaphores, so the order is important here).
+ */
+#ifndef HAVE_SPINLOCKS
+ SpinlockSemaInit();
+#endif
+
+ /*
+ * Set up shared memory allocation mechanism
+ */
+ InitShmemAllocation();
+
+ /* Initialize subsystems */
+ CreateOrAttachShmemStructs();
+
+#ifdef EXEC_BACKEND
+
+ /*
+ * Alloc the win32 shared backend array
+ */
+ ShmemBackendArrayAllocation();
+#endif
+
+ /* Initialize dynamic shared memory facilities. */
+ dsm_postmaster_startup(shim);
+
+ /*
+ * Now give loadable modules a chance to set up their shmem allocations
+ */
+ if (shmem_startup_hook)
+ shmem_startup_hook();
+}
+
+/*
+ * Initialize various subsystems, setting up their data structures in
+ * shared memory.
*
* This is called by the postmaster or by a standalone backend.
* It is also called by a backend forked from the postmaster in the
@@ -171,65 +270,9 @@ CalculateShmemSize(int *num_semaphores)
* check IsUnderPostmaster, rather than EXEC_BACKEND, to detect this case.
* This is a bit code-wasteful and could be cleaned up.)
*/
-void
-CreateSharedMemoryAndSemaphores(void)
+static void
+CreateOrAttachShmemStructs(void)
{
- PGShmemHeader *shim = NULL;
-
- if (!IsUnderPostmaster)
- {
- PGShmemHeader *seghdr;
- Size size;
- int numSemas;
-
- /* Compute the size of the shared-memory block */
- size = CalculateShmemSize(&numSemas);
- elog(DEBUG3, "invoking IpcMemoryCreate(size=%zu)", size);
-
- /*
- * Create the shmem segment
- */
- seghdr = PGSharedMemoryCreate(size, &shim);
-
- /*
- * Make sure that huge pages are never reported as "unknown" while the
- * server is running.
- */
- Assert(strcmp("unknown",
- GetConfigOption("huge_pages_status", false, false)) != 0);
-
- InitShmemAccess(seghdr);
-
- /*
- * Create semaphores
- */
- PGReserveSemaphores(numSemas);
-
- /*
- * If spinlocks are disabled, initialize emulation layer (which
- * depends on semaphores, so the order is important here).
- */
-#ifndef HAVE_SPINLOCKS
- SpinlockSemaInit();
-#endif
- }
- else
- {
- /*
- * We are reattaching to an existing shared memory segment. This
- * should only be reached in the EXEC_BACKEND case.
- */
-#ifndef EXEC_BACKEND
- elog(PANIC, "should be attached to shared memory already");
-#endif
- }
-
- /*
- * Set up shared memory allocation mechanism
- */
- if (!IsUnderPostmaster)
- InitShmemAllocation();
-
/*
* Now initialize LWLocks, which do shared memory allocation and are
* needed for InitShmemIndex.
@@ -302,25 +345,6 @@ CreateSharedMemoryAndSemaphores(void)
AsyncShmemInit();
StatsShmemInit();
WaitEventExtensionShmemInit();
-
-#ifdef EXEC_BACKEND
-
- /*
- * Alloc the win32 shared backend array
- */
- if (!IsUnderPostmaster)
- ShmemBackendArrayAllocation();
-#endif
-
- /* Initialize dynamic shared memory facilities. */
- if (!IsUnderPostmaster)
- dsm_postmaster_startup(shim);
-
- /*
- * Now give loadable modules a chance to set up their shmem allocations
- */
- if (shmem_startup_hook)
- shmem_startup_hook();
}
/*
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 01f7019b10c..6648c6e5e7e 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -468,7 +468,7 @@ InitProcess(void)
*
* This is separate from InitProcess because we can't acquire LWLocks until
* we've created a PGPROC, but in the EXEC_BACKEND case ProcArrayAdd won't
- * work until after we've done CreateSharedMemoryAndSemaphores.
+ * work until after we've done AttachSharedMemoryStructs.
*/
void
InitProcessPhase2(void)
diff --git a/src/include/storage/ipc.h b/src/include/storage/ipc.h
index 888c08b3067..e282dcad962 100644
--- a/src/include/storage/ipc.h
+++ b/src/include/storage/ipc.h
@@ -79,6 +79,9 @@ extern PGDLLIMPORT shmem_startup_hook_type shmem_startup_hook;
extern Size CalculateShmemSize(int *num_semaphores);
extern void CreateSharedMemoryAndSemaphores(void);
+#ifdef EXEC_BACKEND
+extern void AttachSharedMemoryStructs(void);
+#endif
extern void InitializeShmemGUCs(void);
#endif /* IPC_H */
--
2.39.2
From f4c1efd7a565d0f5856dd0e4fb7bcfb583190185 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 1 Dec 2023 11:04:01 +0200
Subject: [PATCH v4 2/4] Pass BackgroundWorker entry in the parameter file in
EXEC_BACKEND mode
This makes it possible to move InitProcess later in SubPostmasterMain
(in next commit), as we no longer need to access shared memory to read
background worker entry.
Reviewed-by: Tristan Partin, Andres Freund
Discussion: https://www.postgresql.org/message-id/7a59b073-5b5b-151e-7ed3-8b01ff7ce...@iki.fi
---
src/backend/postmaster/bgworker.c | 21 -----------
src/backend/postmaster/postmaster.c | 40 +++++++++++++--------
src/include/postmaster/bgworker_internals.h | 4 ---
3 files changed, 25 insertions(+), 40 deletions(-)
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 911bf24a7cb..c4e8cdb45d2 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -631,27 +631,6 @@ ResetBackgroundWorkerCrashTimes(void)
}
}
-#ifdef EXEC_BACKEND
-/*
- * In EXEC_BACKEND mode, workers use this to retrieve their details from
- * shared memory.
- */
-BackgroundWorker *
-BackgroundWorkerEntry(int slotno)
-{
- static BackgroundWorker myEntry;
- BackgroundWorkerSlot *slot;
-
- Assert(slotno < BackgroundWorkerData->total_slots);
- slot = &BackgroundWorkerData->slot[slotno];
- Assert(slot->in_use);
-
- /* must copy this in case we don't intend to retain shmem access */
- memcpy(&myEntry, &slot->worker, sizeof myEntry);
- return &myEntry;
-}
-#endif
-
/*
* Complain about the BackgroundWorker definition using error level elevel.
* Return true if it looks ok, false if not (unless elevel >= ERROR, in
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index b03e88e6702..135dc5a5f49 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -540,6 +540,8 @@ typedef struct
#endif
char my_exec_path[MAXPGPATH];
char pkglib_path[MAXPGPATH];
+
+ BackgroundWorker MyBgworkerEntry;
} BackendParameters;
static void read_backend_variables(char *id, Port *port);
@@ -4831,7 +4833,7 @@ SubPostmasterMain(int argc, char *argv[])
strcmp(argv[1], "--forkavlauncher") == 0 ||
strcmp(argv[1], "--forkavworker") == 0 ||
strcmp(argv[1], "--forkaux") == 0 ||
- strncmp(argv[1], "--forkbgworker=", 15) == 0)
+ strcmp(argv[1], "--forkbgworker") == 0)
PGSharedMemoryReAttach();
else
PGSharedMemoryNoReAttach();
@@ -4962,10 +4964,8 @@ SubPostmasterMain(int argc, char *argv[])
AutoVacWorkerMain(argc - 2, argv + 2); /* does not return */
}
- if (strncmp(argv[1], "--forkbgworker=", 15) == 0)
+ if (strcmp(argv[1], "--forkbgworker") == 0)
{
- int shmem_slot;
-
/* do this as early as possible; in particular, before InitProcess() */
IsBackgroundWorker = true;
@@ -4978,10 +4978,6 @@ SubPostmasterMain(int argc, char *argv[])
/* Attach process to shared data structures */
AttachSharedMemoryStructs();
- /* Fetch MyBgworkerEntry from shared memory */
- shmem_slot = atoi(argv[1] + 15);
- MyBgworkerEntry = BackgroundWorkerEntry(shmem_slot);
-
BackgroundWorkerMain();
}
if (strcmp(argv[1], "--forklog") == 0)
@@ -5640,22 +5636,24 @@ BackgroundWorkerUnblockSignals(void)
#ifdef EXEC_BACKEND
static pid_t
-bgworker_forkexec(int shmem_slot)
+bgworker_forkexec(BackgroundWorker *worker)
{
char *av[10];
int ac = 0;
- char forkav[MAXPGPATH];
-
- snprintf(forkav, MAXPGPATH, "--forkbgworker=%d", shmem_slot);
+ pid_t result;
av[ac++] = "postgres";
- av[ac++] = forkav;
+ av[ac++] = "--forkbgworker";
av[ac++] = NULL; /* filled in by postmaster_forkexec */
av[ac] = NULL;
Assert(ac < lengthof(av));
- return postmaster_forkexec(ac, av);
+ MyBgworkerEntry = worker;
+ result = postmaster_forkexec(ac, av);
+ MyBgworkerEntry = NULL;
+
+ return result;
}
#endif
@@ -5696,7 +5694,7 @@ do_start_bgworker(RegisteredBgWorker *rw)
rw->rw_worker.bgw_name)));
#ifdef EXEC_BACKEND
- switch ((worker_pid = bgworker_forkexec(rw->rw_shmem_slot)))
+ switch ((worker_pid = bgworker_forkexec(&rw->rw_worker)))
#else
switch ((worker_pid = fork_process()))
#endif
@@ -6096,6 +6094,11 @@ save_backend_variables(BackendParameters *param, Port *port,
strlcpy(param->pkglib_path, pkglib_path, MAXPGPATH);
+ if (MyBgworkerEntry)
+ memcpy(¶m->MyBgworkerEntry, MyBgworkerEntry, sizeof(BackgroundWorker));
+ else
+ memset(¶m->MyBgworkerEntry, 0, sizeof(BackgroundWorker));
+
return true;
}
@@ -6324,6 +6327,13 @@ 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, ¶m->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:
diff --git a/src/include/postmaster/bgworker_internals.h b/src/include/postmaster/bgworker_internals.h
index 09df054fcce..323f1e07291 100644
--- a/src/include/postmaster/bgworker_internals.h
+++ b/src/include/postmaster/bgworker_internals.h
@@ -57,8 +57,4 @@ extern void ResetBackgroundWorkerCrashTimes(void);
/* Entry point for background worker processes */
extern void BackgroundWorkerMain(void) pg_attribute_noreturn();
-#ifdef EXEC_BACKEND
-extern BackgroundWorker *BackgroundWorkerEntry(int slotno);
-#endif
-
#endif /* BGWORKER_INTERNALS_H */
--
2.39.2
From bb079f9c87dacddb6ee896ee62c0bc4ce19c7ccb Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@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(¶m, port))
+ if (!save_backend_variables(¶m, 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(¶m->port, port, sizeof(Port));
- if (!write_inheritable_socket(¶m->portsocket, port->sock, childPid))
- return false;
+ if (port)
+ {
+ memcpy(¶m->port, port, sizeof(Port));
+ if (!write_inheritable_socket(¶m->portsocket, port->sock, childPid))
+ return false;
+ param->has_port = true;
+ }
+ else
+ {
+ memset(¶m->port, 0, sizeof(Port));
+ param->has_port = false;
+ }
+
+ if (worker)
+ {
+ memcpy(¶m->MyBgworkerEntry, worker, sizeof(BackgroundWorker));
+ param->has_worker = true;
+ }
+ else
+ {
+ memset(¶m->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(¶m->MyBgworkerEntry, MyBgworkerEntry, sizeof(BackgroundWorker));
- else
- memset(¶m->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(¶m, port);
+ restore_backend_variables(¶m, 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, ¶m->port, sizeof(Port));
- read_inheritable_socket(&port->sock, ¶m->portsocket);
+ if (param->has_port)
+ {
+ *port = (Port *) MemoryContextAlloc(TopMemoryContext, sizeof(Port));
+ memcpy(*port, ¶m->port, sizeof(Port));
+ read_inheritable_socket(&(*port)->sock, ¶m->portsocket);
+ }
+ else
+ *port = NULL;
+
+ if (param->has_worker)
+ {
+ *worker = (BackgroundWorker *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(BackgroundWorker));
+ memcpy(*worker, ¶m->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, ¶m->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
From e4d30ce1366e6d6a42089d0f394195b824bb1560 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 1 Dec 2023 11:28:24 +0200
Subject: [PATCH v4 4/4] Refactor how InitProcess is called
The order of process initialization steps is now more consistent
between !EXEC_BACKEND and EXEC_BACKEND modes. InitProcess() is called
at the same place in either mode. We can now also move the
AttachSharedMemoryStructs() call into InitProcess() itself. This
reduces the number of "#ifdef EXEC_BACKEND" blocks.
Reviewed-by: Tristan Partin, Andres Freund
Discussion: https://www.postgresql.org/message-id/7a59b073-5b5b-151e-7ed3-8b01ff7ce...@iki.fi
---
src/backend/postmaster/autovacuum.c | 16 +++-------
src/backend/postmaster/auxprocess.c | 5 +---
src/backend/postmaster/bgworker.c | 8 ++---
src/backend/postmaster/postmaster.c | 45 ++++-------------------------
src/backend/storage/lmgr/proc.c | 24 +++++++++++++--
5 files changed, 35 insertions(+), 63 deletions(-)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 86a3b3d8be2..2f54485c217 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -476,14 +476,10 @@ AutoVacLauncherMain(int argc, char *argv[])
pqsignal(SIGCHLD, SIG_DFL);
/*
- * 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).
+ * Create a per-backend PGPROC struct in shared memory. We must do this
+ * before we can use LWLocks or access any shared memory.
*/
-#ifndef EXEC_BACKEND
InitProcess();
-#endif
/* Early initialization */
BaseInit();
@@ -1548,14 +1544,10 @@ AutoVacWorkerMain(int argc, char *argv[])
pqsignal(SIGCHLD, SIG_DFL);
/*
- * 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).
+ * Create a per-backend PGPROC struct in shared memory. We must do this
+ * before we can use LWLocks or access any shared memory.
*/
-#ifndef EXEC_BACKEND
InitProcess();
-#endif
/* Early initialization */
BaseInit();
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index cae6feb3562..bae6f68c402 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -97,12 +97,9 @@ AuxiliaryProcessMain(AuxProcType auxtype)
*/
/*
- * Create a PGPROC so we can use LWLocks. In the EXEC_BACKEND case, this
- * was already done by SubPostmasterMain().
+ * Create a PGPROC so we can use LWLocks and access shared memory.
*/
-#ifndef EXEC_BACKEND
InitAuxiliaryProcess();
-#endif
BaseInit();
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index c4e8cdb45d2..00656fcc19e 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -810,14 +810,10 @@ BackgroundWorkerMain(void)
PG_exception_stack = &local_sigjmp_buf;
/*
- * 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).
+ * Create a per-backend PGPROC struct in shared memory. We must do this
+ * before we can use LWLocks or access any shared memory.
*/
-#ifndef EXEC_BACKEND
InitProcess();
-#endif
/*
* Early initialization.
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 444dd8c6892..c127a748d71 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4098,15 +4098,6 @@ BackendStartup(Port *port)
/* Perform additional initialization and collect startup packet */
BackendInitialize(port);
- /*
- * Create a per-backend PGPROC struct in shared memory. We must do
- * this before we can use LWLocks. In the !EXEC_BACKEND case (here)
- * this could be delayed a bit further, but EXEC_BACKEND needs to do
- * stuff with LWLocks before PostgresMain(), so we do it here as well
- * for symmetry.
- */
- InitProcess();
-
/* And run the backend */
BackendRun(port);
}
@@ -4417,6 +4408,12 @@ BackendInitialize(Port *port)
static void
BackendRun(Port *port)
{
+ /*
+ * Create a per-backend PGPROC struct in shared memory. We must do this
+ * before we can use LWLocks or access any shared memory.
+ */
+ InitProcess();
+
/*
* Make sure we aren't in PostmasterContext anymore. (We can't delete it
* just yet, though, because InitPostgres will need the HBA data.)
@@ -4908,12 +4905,6 @@ SubPostmasterMain(int argc, char *argv[])
/* Restore basic shared memory pointers */
InitShmemAccess(UsedShmemSegAddr);
- /* Need a PGPROC to run AttachSharedMemoryStructs */
- InitProcess();
-
- /* Attach process to shared data structures */
- AttachSharedMemoryStructs();
-
/* And run the backend */
BackendRun(port); /* does not return */
}
@@ -4926,12 +4917,6 @@ SubPostmasterMain(int argc, char *argv[])
/* Restore basic shared memory pointers */
InitShmemAccess(UsedShmemSegAddr);
- /* Need a PGPROC to run AttachSharedMemoryStructs */
- InitAuxiliaryProcess();
-
- /* Attach process to shared data structures */
- AttachSharedMemoryStructs();
-
auxtype = atoi(argv[3]);
AuxiliaryProcessMain(auxtype); /* does not return */
}
@@ -4940,12 +4925,6 @@ SubPostmasterMain(int argc, char *argv[])
/* Restore basic shared memory pointers */
InitShmemAccess(UsedShmemSegAddr);
- /* Need a PGPROC to run AttachSharedMemoryStructs */
- InitProcess();
-
- /* Attach process to shared data structures */
- AttachSharedMemoryStructs();
-
AutoVacLauncherMain(argc - 2, argv + 2); /* does not return */
}
if (strcmp(argv[1], "--forkavworker") == 0)
@@ -4953,12 +4932,6 @@ SubPostmasterMain(int argc, char *argv[])
/* Restore basic shared memory pointers */
InitShmemAccess(UsedShmemSegAddr);
- /* Need a PGPROC to run AttachSharedMemoryStructs */
- InitProcess();
-
- /* Attach process to shared data structures */
- AttachSharedMemoryStructs();
-
AutoVacWorkerMain(argc - 2, argv + 2); /* does not return */
}
if (strcmp(argv[1], "--forkbgworker") == 0)
@@ -4969,12 +4942,6 @@ SubPostmasterMain(int argc, char *argv[])
/* Restore basic shared memory pointers */
InitShmemAccess(UsedShmemSegAddr);
- /* Need a PGPROC to run AttachSharedMemoryStructs */
- InitProcess();
-
- /* Attach process to shared data structures */
- AttachSharedMemoryStructs();
-
MyBgworkerEntry = worker;
BackgroundWorkerMain();
}
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 6648c6e5e7e..b6451d9d083 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -291,7 +291,7 @@ InitProcGlobal(void)
}
/*
- * InitProcess -- initialize a per-process data structure for this backend
+ * InitProcess -- initialize a per-process PGPROC entry for this backend
*/
void
InitProcess(void)
@@ -461,6 +461,16 @@ InitProcess(void)
*/
InitLWLockAccess();
InitDeadLockChecking();
+
+#ifdef EXEC_BACKEND
+
+ /*
+ * Initialize backend-local pointers to all the shared data structures.
+ * (We couldn't do this until now because it needs LWLocks.)
+ */
+ if (IsUnderPostmaster)
+ AttachSharedMemoryStructs();
+#endif
}
/*
@@ -487,7 +497,7 @@ InitProcessPhase2(void)
}
/*
- * InitAuxiliaryProcess -- create a per-auxiliary-process data structure
+ * InitAuxiliaryProcess -- create a PGPROC entry for an auxiliary process
*
* This is called by bgwriter and similar processes so that they will have a
* MyProc value that's real enough to let them wait for LWLocks. The PGPROC
@@ -621,6 +631,16 @@ InitAuxiliaryProcess(void)
* acquired in aux processes.)
*/
InitLWLockAccess();
+
+#ifdef EXEC_BACKEND
+
+ /*
+ * Initialize backend-local pointers to all the shared data structures.
+ * (We couldn't do this until now because it needs LWLocks.)
+ */
+ if (IsUnderPostmaster)
+ AttachSharedMemoryStructs();
+#endif
}
/*
--
2.39.2
