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)