Re: Refactoring backend fork+exec code

On 06/03/2024 01:02, Tristan Partin wrote:
> The first 3 patches seem good to go, in my opinion.

Committed these first patches, with a few more changes. Notably, I 
realized that we should move the logic that I originally put in the new 
InitClientConnection function to the existing pq_init() function. It 
servers the same purpose, initialization of the socket in the child 
process. Thanks for the review!

>> @@ -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?

I don't know, but it's the same as on 'master' currently. The code just 
got moved around.

>> +                /* 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.

This is also a pre-existing message I just moved around. But yeah, I 
agree, so changed.

>> +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.

Renamed them to SysloggerStartupData and BackendStartupData. Background 
worker startup still passes a struct called BackgroundWorker, however. I 
left that as it is, because the struct is used for other purposes too.

> 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?

It is valid in the postmaster, too, though. The My* variables and Port 
struct only make sense in the child process.

I think this is the best place after all, so I just removed the XXX comment.

>> +#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! :)

I just removed the extra XXX comment. It's still a valid question, but 
this patch just moves it around, we don't need to answer it here.

>> +                /* XXX: translation? */
>> +                ereport(LOG,
>> +                                (errmsg("could not fork %s process: %m", PostmasterChildName(type))));
> 
> I assume you are referring to the child name here?

Correct. Does the process name need to be translated? And this way of 
constructing sentences is not translation-friendly anyway. In some 
languages, the word 'process' might need to be inflected differently 
depending on the child name, for example.

I put the process name in quotes, and didn't mark the process name for 
translation.

>> 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().

Hmm. There's also BackendStartup() function in postmaster.c, which is 
very different: it runs in the postmaster process and launches the 
backend process. So the Startup suffix is not great either.

I renamed AuxiliaryProcessInit() to AuxiliaryProcessMainCommon(). As in 
"the common parts of the main functions of all the aux processes".

(We should perhaps merge InitProcess() and InitAuxiliaryProcess() into 
one function. There's a lot of duplicated code between them. And the 
parts that differ should perhaps be refactored to be more similar 
anyway. I don't want to take on that refactoring right now though.)

Attached is a new version of the remaining patches.

-- 
Heikki Linnakangas
Neon (https://neon.tech)