Re: Refactoring backend fork+exec code

On Mon Mar 4, 2024 at 3:05 AM CST, Heikki Linnakangas wrote:
> I've now completed many of the side-quests, here are the patches that
> remain.
>
> The first three patches form a logical unit. They move the
> initialization of the Port struct from postmaster to the backend
> process. Currently, that work is split between the postmaster and the
> backend process so that postmaster fills in the socket and some other
> fields, and the backend process fills the rest after reading the startup
> packet. With these patches, there is a new much smaller ClientSocket
> struct that is passed from the postmaster to the child process, which
> contains just the fields that postmaster initializes. The Port struct is
> allocated in the child process. That makes the backend startup easier to
> understand. I plan to commit those three patches next if there are no
> objections.
>
> That leaves the rest of the patches. I think they're in pretty good
> shape too, and I've gotten some review on those earlier and have
> addressed the comments I got so far, but would still appreciate another
> round of review.

> - * *MyProcPort, because ConnCreate() allocated that space with malloc()
> - * ... else we'd need to copy the Port data first. Also, subsidiary data
> - * such as the username isn't lost either; see ProcessStartupPacket().
> + * *MyProcPort, because that space is allocated in stack ... else we'd
> + * need to copy the Port data first. Also, subsidiary data such as the
> + * username isn't lost either; see ProcessStartupPacket().

s/allocated in/allocated on the

The first 3 patches seem good to go, in my opinion.

> @@ -225,14 +331,13 @@ internal_forkexec(int argc, char *argv[], ClientSocket *client_sock, BackgroundW
> return -1;
> }
>
> - /* Make sure caller set up argv properly */
> - Assert(argc >= 3);
> - Assert(argv[argc] == NULL);
> - Assert(strncmp(argv[1], "--fork", 6) == 0);
> - Assert(argv[2] == NULL);
> -
> - /* Insert temp file name after --fork argument */
> + /* set up argv properly */
> + argv[0] = "postgres";
> + snprintf(forkav, MAXPGPATH, "--forkchild=%s", child_kind);
> + argv[1] = forkav;
> + /* Insert temp file name after --forkchild argument */
> argv[2] = tmpfilename;
> + argv[3] = NULL;

Should we use postgres_exec_path instead of the naked "postgres" here?

> + /* in postmaster, fork failed ... */
> + ereport(LOG,
> + (errmsg("could not fork worker process: %m")));
> + /* undo what assign_backendlist_entry did */
> + ReleasePostmasterChildSlot(rw->rw_child_slot);
> + rw->rw_child_slot = 0;
> + pfree(rw->rw_backend);
> + rw->rw_backend = NULL;
> + /* mark entry as crashed, so we'll try again later */
> + rw->rw_crashed_at = GetCurrentTimestamp();
> + return false;

I think the error message should include the word "background." It would
be more consistent with the log message above it.

> +typedef struct
> +{
> + int syslogFile;
> + int csvlogFile;
> + int jsonlogFile;
> +} syslogger_startup_data;

It would be nice if all of these startup data structs were named
similarly. For instance, a previous one was BackendStartupInfo. It would
help with greppability.

I noticed there were a few XXX comments left that you created. I'll
highlight them here for more visibility.

> +/* XXX: where does this belong? */
> +extern bool LoadedSSL;

Perhaps near the My* variables or maybe in the Port struct?

> +#ifdef EXEC_BACKEND
> +
> + /*
> + * Need to reinitialize the SSL library in the backend, since the context
> + * structures contain function pointers and cannot be passed through the
> + * parameter file.
> + *
> + * If for some reason reload fails (maybe the user installed broken key
> + * files), soldier on without SSL; that's better than all connections
> + * becoming impossible.
> + *
> + * XXX should we do this in all child processes? For the moment it's
> + * enough to do it in backend children. XXX good question indeed
> + */
> +#ifdef USE_SSL
> + if (EnableSSL)
> + {
> + if (secure_initialize(false) == 0)
> + LoadedSSL = true;
> + else
> + ereport(LOG,
> + (errmsg("SSL configuration could not be loaded in child process")));
> + }
> +#endif
> +#endif

Here you added the "good question indeed." I am not sure what the best
answer is either! :)

> + /* XXX: translation? */
> + ereport(LOG,
> + (errmsg("could not fork %s process: %m", PostmasterChildName(type))));

I assume you are referring to the child name here?

> XXX: We now have functions called AuxiliaryProcessInit() and
> InitAuxiliaryProcess(). Confusing.

Based on my analysis, the *Init() is called in the Main functions, while
Init*() is called before the Main functions. Maybe
AuxiliaryProcessInit() could be renamed to AuxiliaryProcessStartup()?
Rename the other to AuxiliaryProcessInit().

--
Tristan Partin
Neon (https://neon.tech)