Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8; format=Flowed
Date: Thu, 30 Nov 2023 12:44:33 -0600
Message-Id: <CXCDA9YFDZS1.Y5JUZBHLLXYR@neon.tech>
Cc: "pgsql-hackers" <pgsql-hackers@postgresql.org>, "Thomas Munro"
 <thomas.munro@gmail.com>
Subject: Re: Refactoring backend fork+exec code
To: "Heikki Linnakangas" <hlinnaka@iki.fi>, "Andres Freund"
 <andres@anarazel.de>
From: "Tristan Partin" <tristan@neon.tech>
References: <7a59b073-5b5b-151e-7ed3-8b01ff7ce9ef@iki.fi>
 <20230710225043.svl7fqxecwshwc7a@awork3.anarazel.de>
 <119551e6-60f6-49b2-ae04-a382a89f900c@iki.fi>
 <e0dcfabb-f59a-45e8-bd92-f8d706c92128@iki.fi>
In-Reply-To: <e0dcfabb-f59a-45e8-bd92-f8d706c92128@iki.fi>
Archived-At: 
 <https://www.postgresql.org/message-id/CXCDA9YFDZS1.Y5JUZBHLLXYR%40neon.tech>
Precedence: bulk

On Wed Nov 29, 2023 at 5:36 PM CST, Heikki Linnakangas wrote:
> On 11/10/2023 14:12, Heikki Linnakangas wrote:
> Here's another rebased patch set. Compared to previous version, I did a=
=20
> little more refactoring around CreateSharedMemoryAndSemaphores and=20
> InitProcess:
>
> - patch 1 splits CreateSharedMemoryAndSemaphores into two functions:=20
> CreateSharedMemoryAndSemaphores is now only called at postmaster=20
> startup, and a new function called AttachSharedMemoryStructs() is called=
=20
> in backends in EXEC_BACKEND mode. I extracted the common parts of those=
=20
> functions to a new static function. (Some of this refactoring used to be=
=20
> part of the 3rd patch in the series, but it seems useful on its own, so=
=20
> I split it out.)
>
> - patch 3 moves the call to AttachSharedMemoryStructs() to=20
> InitProcess(), reducing the boilerplate code a little.
>
>
> The patches are incrementally useful, so if you don't have time to=20
> review all of them, a review on a subset would be useful too.

>  From 8886db1ed6bae21bf6d77c9bb1230edbb55e24f9 Mon Sep 17 00:00:00 2001
>  From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
>  Date: Thu, 30 Nov 2023 00:04:22 +0200
>  Subject: [PATCH v3 4/7] Pass CAC as argument to backend process

For me, being new to the code, it would be nice to have more of an=20
explanation as to why this is "better." I don't doubt it; it would just=20
help me and future readers of this commit in the future. More of an=20
explanation in the commit message would suffice.

My other comment on this commit is that we now seem to have lost the=20
context on what CAC stands for. Before we had the member variable to=20
explain it. A comment on the enum would be great or changing cac named=20
variables to canAcceptConnections. I did notice in patch 7 that there=20
are still some variables named canAcceptConnections around, so I'll=20
leave this comment up to you.

>   From 98f8397b32a0b36e221475b32697c9c5bbca86a0 Mon Sep 17 00:00:00 2001
>   From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
>   Date: Wed, 11 Oct 2023 13:38:06 +0300
>   Subject: [PATCH v3 5/7] Remove ConnCreate and ConnFree, and allocate Po=
rt in
>    stack

I like it separate.

>  From 79aab42705a8cb0e16e61c33052fc56fdd4fca76 Mon Sep 17 00:00:00 2001
>  From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
>  Date: Wed, 11 Oct 2023 13:38:10 +0300
>  Subject: [PATCH v3 6/7] Introduce ClientSocket, rename some funcs

>  +static int        BackendStartup(ClientSocket *port);

s/port/client_sock

>  -                port->remote_hostname =3D strdup(remote_host);
>  +                port->remote_hostname =3D pstrdup(remote_host);
>  +        MemoryContextSwitchTo(oldcontext);

Something funky with the whitespace here, but my eyes might also be=20
playing tricks on me. Mixing spaces in tabs like what do in this=20
codebase makes it difficult to review :).

>  From ce51876f87f1e4317e25baf64184749448fcd033 Mon Sep 17 00:00:00 2001
>  From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
>  Date: Thu, 30 Nov 2023 00:07:34 +0200
>  Subject: [PATCH v3 7/7] Refactor postmaster child process launching

>  +                entry_kinds[child_type].main_fn(startup_data, startup_d=
ata_len);
>  +                Assert(false);

Seems like you want the pg_unreachable() macro here instead of=20
Assert(false). Similar comment at the end of SubPostmasterMain().

>  +        if (fwrite(param, paramsz, 1, fp) !=3D 1)
>  +        {
>  +                ereport(LOG,
>  +                                (errcode_for_file_access(),
>  +                                 errmsg("could not write to file \"%s\"=
: %m", tmpfilename)));
>  +                FreeFile(fp);
>  +                return -1;
>  +        }
>  +
>  +        /* Release file */
>  +        if (FreeFile(fp))
>  +        {
>  +                ereport(LOG,
>  +                                (errcode_for_file_access(),
>  +                                 errmsg("could not write to file \"%s\"=
: %m", tmpfilename)));
>  +                return -1;
>  +        }

Two pieces of feedback here. I generally find write(2) more useful than=20
fwrite(3) because write(2) will report a useful errno, whereas fwrite(2)=20
just uses ferror(3). The additional errno information might be valuable=20
context in the log message. Up to you if you think it is also valuable.

The log message if FreeFile() fails doesn't seem to make sense to me.=20
I didn't see any file writing in that code path, but it is possible that=20
I missed something.

>  +        /*
>  +         * Set reference point for stack-depth checking.  This might se=
em
>  +         * redundant in !EXEC_BACKEND builds; but it's not because the =
postmaster
>  +         * launches its children from signal handlers, so we might be r=
unning on
>  +         * an alternative stack. XXX still true?
>  +         */
>  +        (void) set_stack_base();

Looks like there is still this XXX left. Can't say I completely=20
understand the second sentence either.

>  +        /*
>  +         * make sure stderr is in binary mode before anything can possi=
bly be
>  +         * written to it, in case it's actually the syslogger pipe, so =
the pipe
>  +         * chunking protocol isn't disturbed. Non-logpipe data gets tra=
nslated on
>  +         * redirection (e.g. via pg_ctl -l) anyway.
>  +         */

Nit: The 'm' in the first "make" should be capitalized.

>  +        if (fread(&param, sizeof(param), 1, fp) !=3D 1)
>  +        {
>  +                write_stderr("could not read from backend variables fil=
e \"%s\": %s\n",
>  +                                         id, strerror(errno));
>  +                exit(1);
>  +        }
>  +
>  +        /* read startup data */
>  +        *startup_data_len =3D param.startup_data_len;
>  +        if (param.startup_data_len > 0)
>  +        {
>  +                *startup_data =3D palloc(*startup_data_len);
>  +                if (fread(*startup_data, *startup_data_len, 1, fp) !=3D=
 1)
>  +                {
>  +                        write_stderr("could not read startup data from =
backend variables file \"%s\": %s\n",
>  +                                                 id, strerror(errno));
>  +                        exit(1);
>  +                }
>  +        }

fread(3) doesn't set errno. I would probably switch these to read(2) for=20
the reason I wrote in a previous comment.

>  +        /*
>  +         * Need to reinitialize the SSL library in the backend, since t=
he context
>  +         * structures contain function pointers and cannot be passed th=
rough the
>  +         * parameter file.
>  +         *
>  +         * If for some reason reload fails (maybe the user installed br=
oken key
>  +         * files), soldier on without SSL; that's better than all conne=
ctions
>  +         * becoming impossible.
>  +         *
>  +         * XXX should we do this in all child processes?  For the momen=
t it's
>  +         * enough to do it in backend children.
>  +         */
>  +#ifdef USE_SSL
>  +        if (EnableSSL)
>  +        {
>  +                if (secure_initialize(false) =3D=3D 0)
>  +                        LoadedSSL =3D true;
>  +                else
>  +                        ereport(LOG,
>  +                                        (errmsg("SSL configuration coul=
d not be loaded in child process")));
>  +        }
>  +#endif

Do other child process types do any non-local communication?

>  -typedef struct ClientSocket {
>  +struct ClientSocket
>  +{
>           pgsocket        sock;                        /* File descriptor=
 */
>           SockAddr        laddr;                        /* local addr (po=
stmaster) */
>           SockAddr        raddr;                        /* remote addr (c=
lient) */
>  -} ClientSocket;
>  +};
>  +typedef struct ClientSocket ClientSocket;

Can't say I completely understand the reason for this change given it=20
was added in your series.

I didn't look too hard at the Windows-specific code, so maybe someone=20
who knows Windows will have something to say, but it also might've just=20
been copy-paste that I didn't realize.

There were a few more XXXs that probably should be figured out before=20
committing. Though perhaps some of them were already there.

Patches 1-3 seem committable as-is. I only had minor comments on=20
everything but 7, so after taking a look at those, they could be=20
committed.

Overall, this seems liked a marked improvement :).

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