Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8; format=Flowed
Date: Tue, 05 Mar 2024 17:02:55 -0600
Message-Id: <CZM6WDX5H4QI.NZG1YUCKWLA@neon.tech>
To: "Heikki Linnakangas" <hlinnaka@iki.fi>
From: "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>
Subject: Re: Refactoring backend fork+exec code
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>
In-Reply-To: <861f42c1-01c4-42e6-ab2f-d5743ff69607@iki.fi>
Archived-At: 
 <https://www.postgresql.org/message-id/CZM6WDX5H4QI.NZG1YUCKWLA%40neon.tech>
Precedence: bulk

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=20
> remain.
>
> The first three patches form a logical unit. They move the=20
> initialization of the Port struct from postmaster to the backend=20
> process. Currently, that work is split between the postmaster and the=20
> backend process so that postmaster fills in the socket and some other=20
> fields, and the backend process fills the rest after reading the startup=
=20
> packet. With these patches, there is a new much smaller ClientSocket=20
> struct that is passed from the postmaster to the child process, which=20
> contains just the fields that postmaster initializes. The Port struct is=
=20
> allocated in the child process. That makes the backend startup easier to=
=20
> understand. I plan to commit those three patches next if there are no=20
> objections.
>
> That leaves the rest of the patches. I think they're in pretty good=20
> shape too, and I've gotten some review on those earlier and have=20
> addressed the comments I got so far, but would still appreciate another=
=20
> round of review.

> -         * *MyProcPort, because ConnCreate() allocated that space with m=
alloc()
> -         * ... else we'd need to copy the Port data first.  Also, subsid=
iary data
> -         * such as the username isn't lost either; see ProcessStartupPac=
ket().
> +         * *MyProcPort, because that space is allocated in stack ... els=
e 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[], ClientSoc=
ket *client_sock, BackgroundW
>                  return -1;
>          }
>=20
> -        /* Make sure caller set up argv properly */
> -        Assert(argc >=3D 3);
> -        Assert(argv[argc] =3D=3D NULL);
> -        Assert(strncmp(argv[1], "--fork", 6) =3D=3D 0);
> -        Assert(argv[2] =3D=3D NULL);
> -
> -        /* Insert temp file name after --fork argument */
> +        /* set up argv properly */
> +        argv[0] =3D "postgres";
> +        snprintf(forkav, MAXPGPATH, "--forkchild=3D%s", child_kind);
> +        argv[1] =3D forkav;
> +        /* Insert temp file name after --forkchild argument */
>          argv[2] =3D tmpfilename;
> +        argv[3] =3D 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 =3D 0;
> +                pfree(rw->rw_backend);
> +                rw->rw_backend =3D NULL;
> +                /* mark entry as crashed, so we'll try again later */
> +                rw->rw_crashed_at =3D GetCurrentTimestamp();
> +                return false;

I think the error message should include the word "background." It would=20
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=20
similarly. For instance, a previous one was BackendStartupInfo. It would=20
help with greppability.

I noticed there were a few XXX comments left that you created. I'll=20
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 th=
e context
> +         * structures contain function pointers and cannot be passed thr=
ough the
> +         * parameter file.
> +         *
> +         * If for some reason reload fails (maybe the user installed bro=
ken key
> +         * files), soldier on without SSL; that's better than all connec=
tions
> +         * 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) =3D=3D 0)
> +                        LoadedSSL =3D 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=20
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=20
Init*() is called before the Main functions. Maybe=20
AuxiliaryProcessInit() could be renamed to AuxiliaryProcessStartup()?=20
Rename the other to AuxiliaryProcessInit().

--=20
Tristan Partin
Neon (https://neon.tech)