Horton

Re: Refactoring backend fork+exec code

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Tristan Partin <tristan@neon.tech>
Cc: reid.thompson@crunchydata.com, Thomas Munro <thomas.munro@gmail.com>, Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2024-03-13T07:30:27Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Avoid 0-length memcpy to NULL with EXEC_BACKEND

  2. Tighten check for --forkchild argument when spawning child process

  3. Add missing PGDLLIMPORT markings

  4. Remove assertions that some compiler say are tautological

  5. Move code for backend startup to separate file

  6. Refactor postmaster child process launching

  7. Move some functions from postmaster.c to a new source file

  8. Split registration of Win32 deadchild callback to separate function

  9. Improve log messages referring to background worker processes

  10. Move initialization of the Port struct to the child process

  11. Pass CAC as an argument to the backend process

  12. Set socket options in child process after forking

  13. Fix references to renamed function in comments

  14. Use MyBackendType in more places to check what process this is

  15. Remove MyAuxProcType, use MyBackendType instead

  16. Replace BackendIds with 0-based ProcNumbers

  17. Redefine backend ID to be an index into the proc array

  18. Remove superfluous 'pgprocno' field from PGPROC

  19. Fix incorrect comment on how BackendStatusArray is indexed

  20. Refactor how InitProcess is called

  21. Pass BackgroundWorker entry in the parameter file in EXEC_BACKEND mode

  22. Refactor CreateSharedMemoryAndSemaphores

  23. Silence Valgrind complaint with EXEC_BACKEND

  24. Adjust obsolete comment explaining set_stack_base().

  25. Print lwlock stats also for aux processes, when built with LWLOCK_STATS

  26. Move canAcceptConnections check from ProcessStartupPacket to caller.

  27. Fix crash on syslogger startup

  28. Refactor ListenSocket array.

  29. Use FD_CLOEXEC on ListenSockets

  30. Don't leak descriptors into subprograms.

  31. Use WaitEventSet API for postmaster's event loop.

  32. Rearrange postmaster's startup sequence for better syslogger results.

  33. Fix and enhance the assertion of no palloc's in a critical section.

Attachments

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)