Re: Refactoring backend fork+exec code

Hi,

On 2023-11-30 01:36:25 +0200, Heikki Linnakangas wrote:
> - patch 1 splits CreateSharedMemoryAndSemaphores into two functions:
> CreateSharedMemoryAndSemaphores is now only called at postmaster startup,
> and a new function called AttachSharedMemoryStructs() is called in backends
> in EXEC_BACKEND mode. I extracted the common parts of those functions to a
> new static function. (Some of this refactoring used to be part of the 3rd
> patch in the series, but it seems useful on its own, so I split it out.)

I like that idea.



> From a96b6e92fdeaa947bf32774c425419b8f987b8e2 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
> Date: Thu, 30 Nov 2023 00:01:25 +0200
> Subject: [PATCH v3 1/7] 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.

I assume CreateSharedMemoryAndSemaphores() is still called during crash
restart?  I wonder if it shouldn't split three ways:
1) create
2) initialize
3) attach


> From 3478cafcf74a5c8d649e0287e6c72669a29c0e70 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
> Date: Thu, 30 Nov 2023 00:02:03 +0200
> Subject: [PATCH v3 2/7] 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.

>  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)
> +		strncmp(argv[1], "--forkbgworker", 14) == 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 (strncmp(argv[1], "--forkbgworker", 14) == 0)


Now that we don't need to look at parameters anymore, these should probably be
just a strcmp(), like the other cases?


> From 0d071474e12a70ff8113c7b0731c5b97fec45007 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
> Date: Wed, 29 Nov 2023 23:47:25 +0200
> Subject: [PATCH v3 3/7] 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.

Yay.


> diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
> index cdfdd6fbe1d..6c708777dde 100644
> --- a/src/backend/storage/lmgr/proc.c
> +++ b/src/backend/storage/lmgr/proc.c
> @@ -461,6 +461,12 @@ InitProcess(void)
>  	 */
>  	InitLWLockAccess();
>  	InitDeadLockChecking();
> +
> +#ifdef EXEC_BACKEND
> +	/* Attach process to shared data structures */
> +	if (IsUnderPostmaster)
> +		AttachSharedMemoryStructs();
> +#endif
>  }
>
>  /*
> @@ -614,6 +620,12 @@ InitAuxiliaryProcess(void)
>  	 * Arrange to clean up at process exit.
>  	 */
>  	on_shmem_exit(AuxiliaryProcKill, Int32GetDatum(proctype));
> +
> +#ifdef EXEC_BACKEND
> +	/* Attach process to shared data structures */
> +	if (IsUnderPostmaster)
> +		AttachSharedMemoryStructs();
> +#endif
>  }

Aside: Somewhat odd that InitAuxiliaryProcess() doesn't call
InitLWLockAccess().


I think a short comment explaining why we can attach to shmem structs after
already accessing shared memory earlier in the function would be worthwhile.


> From ce51876f87f1e4317e25baf64184749448fcd033 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
> Date: Thu, 30 Nov 2023 00:07:34 +0200
> Subject: [PATCH v3 7/7] Refactor postmaster child process launching
>
> - Move code related to launching backend processes to new source file,
>   launch_backend.c
>
> - Introduce new postmaster_child_launch() function that deals with the
>   differences between EXEC_BACKEND and fork mode.
>
> - Refactor the mechanism of passing information from the parent to
>   child process. Instead of using different command-line arguments when
>   launching the child process in EXEC_BACKEND mode, pass a
>   variable-length blob of data along with all the global variables. The
>   contents of that blob depend on the kind of child process being
>   launched. In !EXEC_BACKEND mode, we use the same blob, but it's simply
>   inherited from the parent to child process.
>
>  [...]
>  33 files changed, 1787 insertions(+), 2002 deletions(-)

Well, that's not small...

I think it may be worth splitting some of the file renaming out into a
separate commit, makes it harder to see what changed here.


> +AutoVacLauncherMain(char *startup_data, size_t startup_data_len)
>  {
> -	pid_t		AutoVacPID;
> +	sigjmp_buf	local_sigjmp_buf;
>
> -#ifdef EXEC_BACKEND
> -	switch ((AutoVacPID = avlauncher_forkexec()))
> -#else
> -	switch ((AutoVacPID = fork_process()))
> -#endif
> +	/* Release postmaster's working memory context */
> +	if (PostmasterContext)
>  	{
> -		case -1:
> -			ereport(LOG,
> -					(errmsg("could not fork autovacuum launcher process: %m")));
> -			return 0;
> -
> -#ifndef EXEC_BACKEND
> -		case 0:
> -			/* in postmaster child ... */
> -			InitPostmasterChild();
> -
> -			/* Close the postmaster's sockets */
> -			ClosePostmasterPorts(false);
> -
> -			AutoVacLauncherMain(0, NULL);
> -			break;
> -#endif
> -		default:
> -			return (int) AutoVacPID;
> +		MemoryContextDelete(PostmasterContext);
> +		PostmasterContext = NULL;
>  	}
>
> -	/* shouldn't get here */
> -	return 0;
> -}

This if (PostmasterContext) ... else "shouldn't get here" business seems
pretty silly, more likely to hide problems than to help.


> +/*
> + * Information needed to launch different kinds of child processes.
> + */
> +static const struct
> +{
> +	const char *name;
> +	void		(*main_fn) (char *startup_data, size_t startup_data_len);
> +	bool		shmem_attach;
> +}			entry_kinds[] = {
> +	[PMC_BACKEND] = {"backend", BackendMain, true},

Personally I'd give the struct an actual name - makes the debugging experience
a bit nicer than anonymous structs that you can't even reference by a typedef.


> +	[PMC_AV_LAUNCHER] = {"autovacuum launcher", AutoVacLauncherMain, true},
> +	[PMC_AV_WORKER] = {"autovacuum worker", AutoVacWorkerMain, true},
> +	[PMC_BGWORKER] = {"bgworker", BackgroundWorkerMain, true},
> +	[PMC_SYSLOGGER] = {"syslogger", SysLoggerMain, false},
> +
> +	[PMC_STARTUP] = {"startup", StartupProcessMain, true},
> +	[PMC_BGWRITER] = {"bgwriter", BackgroundWriterMain, true},
> +	[PMC_ARCHIVER] = {"archiver", PgArchiverMain, true},
> +	[PMC_CHECKPOINTER] = {"checkpointer", CheckpointerMain, true},
> +	[PMC_WAL_WRITER] = {"wal_writer", WalWriterMain, true},
> +	[PMC_WAL_RECEIVER] = {"wal_receiver", WalReceiverMain, true},
> +};


It feels like we have too many different ways of documenting the type of a
process. This new PMC_ stuff, enum AuxProcType, enum BackendType. Which then
leads to code like this:


> -CheckpointerMain(void)
> +CheckpointerMain(char *startup_data, size_t startup_data_len)
>  {
>  	sigjmp_buf	local_sigjmp_buf;
>  	MemoryContext checkpointer_context;
>
> +	Assert(startup_data_len == 0);
> +
> +	MyAuxProcType = CheckpointerProcess;
> +	MyBackendType = B_CHECKPOINTER;
> +	AuxiliaryProcessInit();
> +

For each type of child process. That seems a bit too redundant.  Can't we
unify this at least somewhat? Can't we just reuse BackendType here? Sure,
there'd be pointless entry for B_INVALID, but that doesn't seem like a
problem, could even be useful, by pointing it to a function raising an error.

At the very least this shouldn't deviate from the naming pattern of
BackendType.


> +/*
> + * SubPostmasterMain -- Get the fork/exec'd process into a state equivalent
> + *			to what it would be if we'd simply forked on Unix, and then
> + *			dispatch to the appropriate place.
> + *
> + * The first two command line arguments are expected to be "--forkchild=<name>",
> + * where <name> indicates which postmaster child we are to become, and
> + * the name of a variables file that we can read to load data that would
> + * have been inherited by fork() on Unix.
> + */
> +void
> +SubPostmasterMain(int argc, char *argv[])
> +{
> +	PostmasterChildType child_type;
> +	char	   *startup_data;
> +	size_t		startup_data_len;
> +	char	   *entry_name;
> +	bool		found = false;
> +
> +	/* In EXEC_BACKEND case we will not have inherited these settings */
> +	IsPostmasterEnvironment = true;
> +	whereToSendOutput = DestNone;
> +
> +	/* Setup essential subsystems (to ensure elog() behaves sanely) */
> +	InitializeGUCOptions();
> +
> +	/* Check we got appropriate args */
> +	if (argc != 3)
> +		elog(FATAL, "invalid subpostmaster invocation");
> +
> +	if (strncmp(argv[1], "--forkchild=", 12) != 0)
> +		elog(FATAL, "invalid subpostmaster invocation (--forkchild argument missing)");
> +	entry_name = argv[1] + 12;
> +	found = false;
> +	for (int idx = 0; idx < lengthof(entry_kinds); idx++)
> +	{
> +		if (strcmp(entry_kinds[idx].name, entry_name) == 0)
> +		{
> +			child_type = idx;
> +			found = true;
> +			break;
> +		}
> +	}
> +	if (!found)
> +		elog(ERROR, "unknown child kind %s", entry_name);

If we then have to search linearly, why don't we just pass the index into the
array?

>
> -#define StartupDataBase()		StartChildProcess(StartupProcess)
> -#define StartArchiver()			StartChildProcess(ArchiverProcess)
> -#define StartBackgroundWriter() StartChildProcess(BgWriterProcess)
> -#define StartCheckpointer()		StartChildProcess(CheckpointerProcess)
> -#define StartWalWriter()		StartChildProcess(WalWriterProcess)
> -#define StartWalReceiver()		StartChildProcess(WalReceiverProcess)
> +#define StartupDataBase()		StartChildProcess(PMC_STARTUP)
> +#define StartArchiver()			StartChildProcess(PMC_ARCHIVER)
> +#define StartBackgroundWriter() StartChildProcess(PMC_BGWRITER)
> +#define StartCheckpointer()		StartChildProcess(PMC_CHECKPOINTER)
> +#define StartWalWriter()		StartChildProcess(PMC_WAL_WRITER)
> +#define StartWalReceiver()		StartChildProcess(PMC_WAL_RECEIVER)
> +
> +#define StartAutoVacLauncher()	StartChildProcess(PMC_AV_LAUNCHER);
> +#define StartAutoVacWorker()	StartChildProcess(PMC_AV_WORKER);

Obviously not your fault, but these macros are so pointless... Making it
harder to find where we start child processes, all to save a a few characters
in one place, while adding considerably more in others.


> +void
> +BackendMain(char *startup_data, size_t startup_data_len)
> +{

Is there any remaining reason for this to live in postmaster.c? Given that
other backend types don't, that seems oddly assymmetrical.

Greetings,

Andres Freund