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
