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)