From: Tristan Partin <tristan@neon.tech>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: reid.thompson@crunchydata.com
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Refactoring backend fork+exec code
Date: Tue, 05 Mar 2024 17:02:55 -0600
Message-ID: <CZM6WDX5H4QI.NZG1YUCKWLA@neon.tech> (raw)
In-Reply-To: <861f42c1-01c4-42e6-ab2f-d5743ff69607@iki.fi>
References: <59527c9b-c3f7-4d0c-a3f1-00d0ed5cc186@iki.fi>
<f3ecd4cb-85ee-4e54-8278-5fabfb3a4ed0@iki.fi>
<20240122210740.7vq5fd4woixpwx3f@awork3.anarazel.de>
<8171f1aa-496f-46a6-afc3-c46fe7a9b407@iki.fi>
<20240123195009.44wtutga24p5sh3s@awork3.anarazel.de>
<08eff756-beeb-4a11-9eb5-d45b6af38998@iki.fi>
<5997ff317a5827496537580078bcd19f4441f8d2.camel@crunchydata.com>
<606a62d6-321c-4967-92c1-2d4e20a2c136@iki.fi>
<20240207182521.wyt5dmd75l4v27zk@awork3.anarazel.de>
<2136fb4f-1630-43e9-9cf6-b32772b4b0a0@iki.fi>
<20240214213711.2gjlezqll37mg7lg@awork3.anarazel.de>
<CA+Tgmoa=m3qogg1QH8kbicTgCtipgK1hTUDWpHd5SBra7kV6Ew@mail.gmail.com>
<8f695109-8341-4f7c-8b0d-3cadc7eddc9e@iki.fi>
<0c247a78-9dac-4584-8e87-39f2b45bec68@iki.fi>
<861f42c1-01c4-42e6-ab2f-d5743ff69607@iki.fi>
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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tristan@neon.tech, hlinnaka@iki.fi, reid.thompson@crunchydata.com, thomas.munro@gmail.com, robertmhaas@gmail.com, andres@anarazel.de
Subject: Re: Refactoring backend fork+exec code
In-Reply-To: <CZM6WDX5H4QI.NZG1YUCKWLA@neon.tech>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox