Re: Refactoring backend fork+exec code

Hi,

On 2023-06-18 14:22:33 +0300, Heikki Linnakangas wrote:
> I started to look at the code in postmaster.c related to launching child
> processes. I tried to reduce the difference between EXEC_BACKEND and
> !EXEC_BACKEND code paths, and put the code that needs to differ behind a
> better abstraction. I started doing this to help with implementing
> multi-threading, but it doesn't introduce anything thread-related yet and I
> think this improves readability anyway.

Yes please! This code is absolutely awful.

> From 0cb6f8d665980d30a5d2a29013000744f16bf813 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Sun, 18 Jun 2023 11:00:21 +0300
> Subject: [PATCH 3/9] Refactor CreateSharedMemoryAndSemaphores.
>
> Moves InitProcess calls a little later in EXEC_BACKEND case.

What's the reason for this part? ISTM that we'd really want to get away from
plastering duplicated InitProcess() etc everywhere.

I think this might be easier to understand if you just changed did the
CreateSharedMemoryAndSemaphores() -> AttachSharedMemoryAndSemaphores() piece
in this commit, and the rest later.

> +void
> +AttachSharedMemoryAndSemaphores(void)
> +{
> + /* InitProcess must've been called already */

Perhaps worth an assertion to make it easier to see that the order is wrong?

> From 1d89eec53c7fefa7a4a8c011c9f19e3df64dc436 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Mon, 12 Jun 2023 16:33:20 +0300
> Subject: [PATCH 4/9] Use FD_CLOEXEC on ListenSockets
>
> We went through some effort to close them in the child process. Better to
> not hand them down to the child process in the first place.

I think Thomas has a larger version of this patch:
https://postgr.es/m/CA%2BhUKGKPNFcfBQduqof4-7C%3DavjcSfdkKBGvQoRuAvfocnvY0A%40mail.gmail.com

> From 65384b9a6cfb3b9b589041526216e0f64d64bea5 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Sun, 18 Jun 2023 13:56:44 +0300
> Subject: [PATCH 8/9] Introduce ClientSocket, rename some funcs
>
> - Move more of the work on a client socket to the child process.
>
> - Reduce the amount of data that needs to be passed from postmaster to
> child. (Used to pass a full Port struct, although most of the fields were
> empty. Now we pass the much slimmer ClientSocket.)

I think there might be extensions accessing Port. Not sure if it's worth
worrying about, but ...

> --- a/src/backend/postmaster/autovacuum.c
> +++ b/src/backend/postmaster/autovacuum.c
> @@ -476,8 +476,8 @@ AutoVacLauncherMain(int argc, char *argv[])
> pqsignal(SIGCHLD, SIG_DFL);
>
> /*
> - * Create a per-backend PGPROC struct in shared memory. We must do
> - * this before we can use LWLocks.
> + * Create a per-backend PGPROC struct in shared memory. We must do this
> + * before we can use LWLocks.
> */
> InitProcess();
>

Don't think this was intentional?

> From b33cfeb28a5419045acb659a01410b2b463bea3e Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Sun, 18 Jun 2023 13:59:48 +0300
> Subject: [PATCH 9/9] Refactor postmaster child process launching
>
> - Move code related to launching backend processes to new source file,
> process_start.c

I think you might have renamed this to 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 informaton 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 depends 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.

> +const PMChildEntry entry_kinds[] = {
> + {"backend", BackendMain, true},
> +
> + {"autovacuum launcher", AutoVacLauncherMain, true},
> + {"autovacuum worker", AutoVacWorkerMain, true},
> + {"bgworker", BackgroundWorkerMain, true},
> + {"syslogger", SysLoggerMain, false},
> +
> + {"startup", StartupProcessMain, true},
> + {"bgwriter", BackgroundWriterMain, true},
> + {"archiver", PgArchiverMain, true},
> + {"checkpointer", CheckpointerMain, true},
> + {"wal_writer", WalWriterMain, true},
> + {"wal_receiver", WalReceiverMain, true},
> +};

I'd assign them with the PostmasterChildType as index, so there's no danger of
getting out of order.

const PMChildEntry entry_kinds = {
[PMC_AV_LAUNCHER] = {"autovacuum launcher", AutoVacLauncherMain, true},
...
}

or such should work.

I'd also use designated initializers for the fields, it's otherwise hard to
know what true means etc.

I think it might be good to put more into array. If we e.g. knew whether a
particular child type is a backend-like, and aux process or syslogger, we
could avoid the duplicated InitAuxiliaryProcess(),
MemoryContextDelete(PostmasterContext) etc calls everywhere.

> +/*
> + * 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 "--forkFOO"
> + * (where FOO 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. Remaining arguments go to the
> + * subprocess FooMain() routine. XXX
> + */
> +void
> +SubPostmasterMain(int argc, char *argv[])
> +{
> + PostmasterChildType child_type;
> + char *startup_data;
> + size_t startup_data_len;
> +
> + /* 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)
> + {
> + char *entry_name = argv[1] + 12;
> + bool 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);
> + }

Hm, shouldn't we error out when called without --forkchild?

> +/* Save critical backend variables into the BackendParameters struct */
> +#ifndef WIN32
> +static bool
> +save_backend_variables(BackendParameters *param, ClientSocket *client_sock)
> +#else

There's so much of this kind of thing. Could we hide it in a struct or such
instead of needing ifdefs everywhere?

> --- a/src/backend/storage/ipc/shmem.c
> +++ b/src/backend/storage/ipc/shmem.c
> @@ -144,6 +144,8 @@ InitShmemAllocation(void)
> /*
> * Initialize ShmemVariableCache for transaction manager. (This doesn't
> * really belong here, but not worth moving.)
> + *
> + * XXX: we really should move this
> */
> ShmemVariableCache = (VariableCache)
> ShmemAlloc(sizeof(*ShmemVariableCache));

Heh. Indeed. And probably just rename it to something less insane.

Greetings,

Andres Freund