Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8; format=Flowed
Date: Fri, 01 Dec 2023 15:11:49 -0600
Message-Id: <CXDB1KDXT43W.3JWSW74DO6JT3@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>, "Alexander Lakhin"
 <exclusion@gmail.com>, "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>
 <CXCDA9YFDZS1.Y5JUZBHLLXYR@neon.tech>
 <3b02a5a1-81ea-44e4-a09e-c0a594a4cef8@iki.fi>
 <014768ed-8b39-c44f-b07c-098c87b1644c@gmail.com>
 <a1b69a02-544a-471e-8b95-81256e992248@iki.fi>
In-Reply-To: <a1b69a02-544a-471e-8b95-81256e992248@iki.fi>
Archived-At: 
 <https://www.postgresql.org/message-id/CXDB1KDXT43W.3JWSW74DO6JT3%40neon.tech>
Precedence: bulk

On Fri Dec 1, 2023 at 2:44 PM CST, Heikki Linnakangas wrote:
> On 01/12/2023 16:00, Alexander Lakhin wrote:
> > Maybe you could look at issues with running `make check` under Valgrind
> > when server built with CPPFLAGS=3D"-DUSE_VALGRIND -DEXEC_BACKEND":
> > # +++ regress check in src/test/regress +++
> > # initializing database system by copying initdb template
> > # postmaster failed, examine ".../src/test/regress/log/postmaster.log" =
for the reason
> > Bail out!make[1]: ***
> >=20
> > ...
> > 2023-12-01 16:48:39.136 MSK postmaster[1307988] LOG:=C2=A0 listening on=
 Unix socket "/tmp/pg_regress-pPFNk0/.s.PGSQL.55312"
> > =3D=3D00:00:00:01.692 1259396=3D=3D Syscall param write(buf) points to =
uninitialised byte(s)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 at 0x5245A37: wri=
te (write.c:26)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 by 0x51BBF6C: _IO=
_file_write@@GLIBC_2.2.5 (fileops.c:1180)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 by 0x51BC84F: new=
_do_write (fileops.c:448)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 by 0x51BC84F: _IO=
_new_file_xsputn (fileops.c:1254)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 by 0x51BC84F: _IO=
_file_xsputn@@GLIBC_2.2.5 (fileops.c:1196)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 by 0x51B1056: fwr=
ite (iofwrite.c:39)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 by 0x552E21: inte=
rnal_forkexec (postmaster.c:4518)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 by 0x5546A1: post=
master_forkexec (postmaster.c:4444)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 by 0x55471C: Star=
tChildProcess (postmaster.c:5275)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 by 0x557B61: Post=
masterMain (postmaster.c:1454)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0=C2=A0=C2=A0 by 0x472136: main=
 (main.c:198)
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0 Address 0x1ffeffdc11 is on th=
read 1's stack
> > =3D=3D00:00:00:01.692 1259396=3D=3D=C2=A0 in frame #4, created by inter=
nal_forkexec (postmaster.c:4482)
> > =3D=3D00:00:00:01.692 1259396=3D=3D
> >=20
> > With memset(param, 0, sizeof(*param)); added at the beginning of
> > save_backend_variables(), server starts successfully, but then
> > `make check` fails with other Valgrind error.
>
> That's actually a pre-existing issue, I'm seeing that even on unpatched=
=20
> 'master'.
>
> In a nutshell, the problem is that the uninitialized padding bytes in=20
> BackendParameters are written to the file. When we read the file back,=20
> we don't access the padding bytes, so that's harmless. But Valgrind=20
> doesn't know that.
>
> On Windows, the file is created with=20
> CreateFileMapping(INVALID_HANDLE_VALUE, ...) and we write the variables=
=20
> directly to the mapping. If I understand the Windows API docs correctly,=
=20
> it is guaranteed to be initialized to zeros, so we don't have this=20
> problem on Windows, only on other platforms with EXEC_BACKEND. I think=20
> it makes sense to clear the memory on other platforms too, since that's=
=20
> what we do on Windows.
>
> Committed a fix with memset(). I'm not sure what our policy with=20
> backpatching this kind of issues is. This goes back to all supported=20
> versions, but given the lack of complaints, I chose to not backpatch.

Seems like a harmless think to backpatch. It is conceivable that someone=20
might want to run Valgrind on something other than HEAD.

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