Re: Fix uninitialized xl_running_xacts padding
Heikki Linnakangas <hlinnaka@iki.fi>
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Alexander Kuzmenkov <akuzmenkov@tigerdata.com>
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>
Date: 2026-03-12T18:49:46Z
Lists: pgsql-hackers
On 12/03/2026 20:23, Alexander Kuzmenkov wrote:
> 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.
Cool. It would be nice to have test coverage for every WAL record type.
Could you add tests to the test suite to hit those cases?
> 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));
Yep.
> 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.
+1. I think it's a good goal that no uninitialized bytes reach the WAL.
It's not a security issue or anything, but just seems like good hygiene.
> 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.
Sounds good to me.
- Heikki
Commits
-
Add test for single-page VACUUM of hash index on INSERT
- 1f7947a48d0c 19 (unreleased) landed