Horton

Re: Refactoring backend fork+exec code

Tristan Partin <tristan@neon.tech>

View thread
From: "Tristan Partin" <tristan@neon.tech>
To: "Heikki Linnakangas" <hlinnaka@iki.fi>
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-05T23:02:55Z
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.

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)