MIME-Version: 1.0
References: 
 <CAO6_Xqoxp7C+y0L==xZXH5V=9PjpBx4T9vJYs87EbxFp_9nwOA@mail.gmail.com>
 <CA+hUKG++LE6P6g4n+-QPHBwAnvcVRyG1tUnzscUriWAFHc6s6Q@mail.gmail.com>
 <aZJuoggMUaXjwoDO@paquier.xyz>
 <aZLHYtCsEldmm8Eu@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAO6_XqpGYneOe357s4QhOTYpU6UEgk3JJrBiyeuk-9n=f7p=Vw@mail.gmail.com>
 <aoaj45foewpjtu6r5cs67yrx4en3pkurs23e4azv6tbikpw6c3@h3pnaqaksoeg>
 <b607e42e-34ff-414a-b727-dd5ec70babe3@timescale.com>
 <ed73b90b-e153-41ba-a626-2e13da42d6c8@iki.fi>
 <CALzhyqzKTRVsQGj+qDDRVs3Oo0EvffuQvVO0v4rbpWU=SoXKug@mail.gmail.com>
 <CALzhyqzLh9iciLMvT9T-Z+o1ekOPpeP2p2r7tYXNZ8JMSea3Zg@mail.gmail.com>
 <f2875d6b-5ff6-4744-b151-f5ab54cbd2c5@iki.fi>
In-Reply-To: <f2875d6b-5ff6-4744-b151-f5ab54cbd2c5@iki.fi>
From: Alexander Kuzmenkov <akuzmenkov@tigerdata.com>
Date: Thu, 12 Mar 2026 19:23:31 +0100
Message-ID: 
 <CALzhyqy27==NkjnXYysF1zJj_K2rX5kncJZ9fEdTRVszTjJisg@mail.gmail.com>
Subject: Re: Fix uninitialized xl_running_xacts padding
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Andres Freund <andres@anarazel.de>,
 Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>,
	Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
 Michael Paquier <michael@paquier.xyz>,
	Thomas Munro <thomas.munro@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000003132b3064cd7dafd"
Archived-At: 
 <https://www.postgresql.org/message-id/CALzhyqy27%3D%3DNkjnXYysF1zJj_K2rX5kncJZ9fEdTRVszTjJisg%40mail.gmail.com>
Precedence: bulk

--0000000000003132b3064cd7dafd
Content-Type: text/plain; charset="UTF-8"

The functions in the "0003" patch haven't surfaced in my "make
installcheck-parallel" runs with Valgrind, or the "make check" with
MemorySanitizer. However, I could hit most of them with some fuzzing. The
only exception was `xl_hash_vacuum_one_page` but that's probably also
triggerable.

I noticed that we also use `sizeof` in some WAL functions, so probably the
tail padding can also be written to WAL? For example, consider this:
(gdb) ptype/o gistxlogPageSplit
type = struct gistxlogPageSplit {
/*      0      |       4 */    BlockNumber origrlink;
/* XXX  4-byte hole      */
/*      8      |       8 */    GistNSN orignsn;
/*     16      |       1 */    _Bool origleaf;
/* XXX  1-byte hole      */
/*     18      |       2 */    uint16 npage;
/*     20      |       1 */    _Bool markfollowright;
/* XXX  3-byte padding   */

                               /* total size (bytes):   24 */
                             }

And then we do  XLogRegisterData((char *) &xlrec,
sizeof(gistxlogPageSplit));


In general, I'm wondering what our approach to this should be. Several
potential improvements were mentioned, but I think for now we could focus
on removing the Valgrind suppression. This is a meaningful improvement that
uses the existing test tools. Do we want to defensively zero-initialize
every case that seems to be potentially affected, i.e. written to WAL and
has holes/tail padding? That sounds cheap and simple and probably even
backportable. In the "0001" patch, there are several cases where no padding
goes into WAL, I can remove these. For example, the use of
xl_brin_createidx in brinbuild() does not have this problem.

--0000000000003132b3064cd7dafd
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">The functions in the &quot;0003&quot; patch haven&#39;t su=
rfaced in my &quot;make installcheck-parallel&quot; runs with Valgrind, or =
the &quot;make check&quot; with MemorySanitizer. However, I could hit most =
of them with some fuzzing. The only exception was `xl_hash_vacuum_one_page`=
 but that&#39;s probably also triggerable.<br><br>I noticed that we also us=
e `sizeof` in some WAL functions, so probably the tail padding can also be =
written to WAL? For example, consider this:<br><span style=3D"font-family:m=
onospace">(gdb) ptype/o gistxlogPageSplit<br>type =3D struct gistxlogPageSp=
lit {<br>/* =C2=A0 =C2=A0 =C2=A00 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=
=A0 4 */ =C2=A0 =C2=A0BlockNumber origrlink;<br>/* XXX =C2=A04-byte hole =
=C2=A0 =C2=A0 =C2=A0*/<br>/* =C2=A0 =C2=A0 =C2=A08 =C2=A0 =C2=A0 =C2=A0| =
=C2=A0 =C2=A0 =C2=A0 8 */ =C2=A0 =C2=A0GistNSN orignsn;<br>/* =C2=A0 =C2=A0=
 16 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=A0 1 */ =C2=A0 =C2=A0_Bool orig=
leaf;<br>/* XXX =C2=A01-byte hole =C2=A0 =C2=A0 =C2=A0*/<br>/* =C2=A0 =C2=
=A0 18 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=A0 2 */ =C2=A0 =C2=A0uint16 =
npage;<br>/* =C2=A0 =C2=A0 20 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0 =C2=A0 1 =
*/ =C2=A0 =C2=A0_Bool markfollowright;<br>/* XXX =C2=A03-byte padding =C2=
=A0 */<br><br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* total size (bytes): =
=C2=A0 24 */<br></span><div><span style=3D"font-family:monospace">=C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0}</span></div><div><br></div>And then we do=C2=A0 X=
LogRegisterData((char *) &amp;xlrec, sizeof(gistxlogPageSplit));<br><br><br=
>In general, I&#39;m wondering what our approach to this should be. Several=
 potential improvements were mentioned, but I think for now we could focus =
on removing the Valgrind suppression. This is a meaningful improvement that=
 uses the existing test tools. Do we want to defensively zero-initialize ev=
ery case that seems to be potentially affected, i.e. written to WAL and has=
 holes/tail padding? That sounds cheap and simple and probably even backpor=
table. In the &quot;0001&quot; patch, there are several cases where no padd=
ing goes into WAL, I can remove these. For example, the use of xl_brin_crea=
teidx in brinbuild() does not have this problem.</div>

--0000000000003132b3064cd7dafd--