Use C11 alignas instead of palloc/malloc for alignment

On 08/09/2026 13:08, Peter Eisentraut wrote:

There are a number of places where palloc()/malloc()/etc. was used
solely to obtain an aligned buffer.  We can do these much simpler by
using alignas with a local variable instead.  See attached patch.

In XLogRecordAssemble, you could use a local variable in XLogInsert()
for the scratch buffer, and pass a pointer to that to
XLogRecordAssemble(). That would avoid the a global variable, which is
nice for multi-threading.

Not new with this patch, but the calculations for HEADER_SCRATCH_SIZE
are a little scary. I'd also suggest adding an assertion somewhere that
you don't overrun the HEADER_SCRATCH_SIZE buffer, and maybe some extra
comments. I don't see any bugs there, but it's very subtle that if you
e.g. add a new "special" block ID similar to XLR_BLOCK_ID_TOPLEVEL_XID,
you need to remember to adjust HEADER_SCRATCH_SIZE accordingly.

For the BLCKSZ-sized buffers, how about using PGAlignedBlock?

- Heikki

On 2026-09-08 Tu 6:08 AM, Peter Eisentraut wrote:

There are a number of places where palloc()/malloc()/etc. was used
solely to obtain an aligned buffer.  We can do these much simpler by
using alignas with a local variable instead.  See attached patch.

One of these revealed a small problem with how pgindent handles
alignas (it doesn't know about it and it might or might not work well
depending on context), so I added a workaround into pgindent to fix
that.  (Or we could try to reshuffle that code to avoid the problem.)

The pgindent recipe should possibly be hardened for things like nested
parens. Here's a reworked patch 2.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 2026-09-08 12:08:52 +0200, Peter Eisentraut wrote:

From 9e5c5d6a660791aaab06bab10272260da109dfb6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 8 Sep 2026 11:42:24 +0200
Subject: [PATCH 1/2] Use C11 alignas instead of palloc/malloc for alignment

Replace several cases where palloc()/malloc()/etc. was used solely to
obtain an aligned buffer. Use alignas with a local variable instead.

The previous alignment guarantees are carried over. palloc-based
allocations are replaced by alignas(MAXIMUM_ALIGNOF). Theoretically,
malloc-based allocations should be replaced by alignas(max_align_t),
but MSVC doesn't provide max_align_t, and so we use MAXIMUM_ALIGNOF
here as well. They should be the same in practice.

The allocations in InitWalRecovery() are not converted, because the
comment says it is also this way to avoid wasting storage. The
comment in XLogReaderAllocate(), on the other hand, was probably
copied from InitWalRecovery(), but the part of the comment about
wasting storage does not make sense in that context, so it is
converted.

FIXME: indent in xlogreader.h
---
src/backend/access/transam/xloginsert.c | 28 ++++++++---------------
src/backend/access/transam/xlogreader.c | 17 --------------
src/backend/access/transam/xlogrecovery.c | 3 ++-
src/backend/commands/sequence_xlog.c | 9 +++-----
src/backend/storage/file/copydir.c | 12 ++--------
src/backend/storage/ipc/dsm_impl.c | 10 ++++----
src/backend/storage/smgr/md.c | 4 +---
src/bin/pg_resetwal/pg_resetwal.c | 7 ++----
src/include/access/xlogreader.h | 5 ++--
9 files changed, 26 insertions(+), 69 deletions(-)

diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index c9aff944a2e..70cbe9d709b 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -105,17 +105,6 @@ static uint64 mainrdata_len;	/* total # of bytes in chain */
/* flags for the in-progress insertion */
static uint8 curinsert_flags = 0;

-/*
- * These are used to hold the record header while constructing a record.
- * 'hdr_scratch' is not a plain variable, but is palloc'd at initialization,
- * because we want it to be MAXALIGNed and padding bytes zeroed.
- *
- * For simplicity, it's allocated large enough to hold the headers for any
- * WAL record.
- */
-static XLogRecData hdr_rdt;
-static char *hdr_scratch = NULL;
-
#define SizeOfXlogOrigin (sizeof(ReplOriginId) + sizeof(char))
#define SizeOfXLogTransactionId (sizeof(TransactionId) + sizeof(char))

@@ -622,6 +611,16 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
XLogRecPtr *fpw_lsn, int *num_fpi, uint64 *fpi_bytes,
bool *topxid_included)
{
+	/*
+	 * These are used to hold the record header while constructing a record.
+	 * 'hdr_scratch' must be MAXALIGNed and padding bytes zeroed.
+	 *
+	 * For simplicity, it's allocated large enough to hold the headers for any
+	 * WAL record.
+	 */
+	static XLogRecData hdr_rdt;
+	static alignas(MAXIMUM_ALIGNOF) char hdr_scratch[HEADER_SCRATCH_SIZE];

I think we really shouldn't add more function level statics at this point. At
least for file level static variables you can just slap a thread_local on and
it has a chance of working. But it won't with this.

@@ -133,16 +133,13 @@ copydir(const char *fromdir, const char *todir, bool recurse)
void
copy_file(const char *fromfile, const char *tofile)
{
-	char	   *buffer;
+	alignas(MAXIMUM_ALIGNOF) char buffer[8 * BLCKSZ];
int			srcfd;
int			dstfd;
ssize_t		nbytes;
off_t		offset;
off_t		flush_offset;

- /* Size of copy buffer (read and write requests) */
-#define COPY_BUF_SIZE (8 * BLCKSZ)
-

I don't think it's a great idea to allocate that much on the stack... And for
performance we really ought to make this a substantially *bigger* buffer. So
this one I would just replace the comment with something indicating that we
are leaving it a dynamically allocated buffer for size *and* alignment reason.

Greetings,

Andres Freund

On 08.09.26 17:03, Andres Freund wrote:

+	/*
+	 * These are used to hold the record header while constructing a record.
+	 * 'hdr_scratch' must be MAXALIGNed and padding bytes zeroed.
+	 *
+	 * For simplicity, it's allocated large enough to hold the headers for any
+	 * WAL record.
+	 */
+	static XLogRecData hdr_rdt;
+	static alignas(MAXIMUM_ALIGNOF) char hdr_scratch[HEADER_SCRATCH_SIZE];

I think we really shouldn't add more function level statics at this point. At
least for file level static variables you can just slap a thread_local on and
it has a chance of working. But it won't with this.

What would prevent us from adding thread_local to a function-scope
static variable? AFAICT, that should work.

(In this particular case, with Heikki's suggestion, it seems we can get
rid of it.)

On 08.09.26 16:05, Andrew Dunstan wrote:

On 2026-09-08 Tu 6:08 AM, Peter Eisentraut wrote:

There are a number of places where palloc()/malloc()/etc. was used
solely to obtain an aligned buffer.  We can do these much simpler by
using alignas with a local variable instead.  See attached patch.

One of these revealed a small problem with how pgindent handles
alignas (it doesn't know about it and it might or might not work well
depending on context), so I added a workaround into pgindent to fix
that.  (Or we could try to reshuffle that code to avoid the problem.)

The pgindent recipe should possibly be hardened for things like nested
parens. Here's a reworked patch 2.

Thanks, this is a clever solution. I have committed this separately.

On 08.09.26 12:08, Peter Eisentraut wrote:

There are a number of places where palloc()/malloc()/etc. was used
solely to obtain an aligned buffer.  We can do these much simpler by
using alignas with a local variable instead.  See attached patch.

Here is a new patch set that aims to address all the comments.

First of all, while changing this to make use of the existing
"AlignedBlock" types, I noticed that PGAlignedXLogBlock is misnamed: It
should be PGIOAlignedXLogBlock, to maintain the similarity with
PGAlignedBlock and PGIOAlignedBlock, respectively. So I'm proposing to
rename it in patch 0001.

We could then re-introduce the "correct" PGAlignedXLogBlock and make use
of it, which is patch 0003. But I'm hesitant to change the meaning of
PGAlignedXLogBlock without some gap in between, so I'm not sure about
this patch.

Patch 0002 is as before, but with the "AlignedBlock" types used, and the
copy_file() change backed out and a comment added.

Patch 0004 adds some comments and an assertion for HEADER_SCRATCH_SIZE,
and patch 0005 refactors things to convert the workspace from static
variable to a normal (non-static) local variable. (This could be
squashed into 0002, but it seems cleaner to review this way at least.)

(The pgindent changes were already committed separately.)

Hi,

I tested v3 against two questions: whether 0003 needs a gap in between,
and whether 0005 really leaves no garbage in the WAL. Everything below
can be rerun with the attached files.

0003: who uses PGAlignedXLogBlock out of tree
---------------------------------------------

A GitHub code search (default branches only, so a lower bound), leaving
out forks that carry their own c.h and copies of the in-core files,
finds it in percona/pg_tde (pg_tde_archive_decrypt.c,
pg_tde_restore_encrypt.c, fetools/pg16..pg19/pg_rewind/tde_ops.c),
commandprompt/open_pg_tde, bdrouvot/pg_wal_fp_extract,
wublabdubdub/pg_flashback, 3Davydov/WAL-DIFF,
ApsaraDB/PolarDB-BackupAgent and OpenTeleDB. All of them use it as a
page buffer for plain read/write on WAL segments; none of those
repositories uses O_DIRECT in its own code. For example, pg_tde:

PGAlignedXLogBlock buf;
...
r = read(tmpfd, buf.data, XLOG_BLCKSZ);

and WAL-DIFF opens its file with O_RDONLY | PG_BINARY.

To see what each option does to that code, I compiled
xlogblock_user.c (attached, the same pattern in 30 lines, with an
optional O_DIRECT read of the same buffer) against 19beta2, against
master plus 0001 only, and against master plus 0001-0005:

gcc -D_GNU_SOURCE -I$(pg_config --includedir-server) xlogblock_user.c \
-L$(pg_config --libdir) -lpgcommon -lpgport

19beta2:
alignof(PGAlignedXLogBlock) = 4096, buf % 4096 = 0
pread: 8192 bytes
pread with O_DIRECT: 8192 bytes

0001 only:
error: unknown type name 'PGAlignedXLogBlock'; did you mean
'PGIOAlignedXLogBlock'?

0001-0005, file on ext4:
alignof(PGAlignedXLogBlock) = 8, buf % 4096 = 2720
pread: 8192 bytes
alignof(PGAlignedXLogBlock) = 8, buf % 4096 = 336
pread with O_DIRECT: Invalid argument

0001-0005, same binary, file on btrfs:
pread with O_DIRECT: 8192 bytes

So both sides of your hesitation are real. 0003 keeps the code I found
compiling unchanged, and that code only does plain I/O, which works.
But code doing direct I/O through the type would also compile silently
and then fail with EINVAL, and only on some file systems: btrfs accepts
the misaligned buffer, ext4 rejects it. I found no such code, but a
failure that depends on the file system would be hard to trace back to
this change. With 0001 alone, every user above gets a compile error that
names the replacement.

0005: padding bytes in the WAL
------------------------------

XLogRecord has a two-byte hole at offset 18, between xl_rmid and xl_crc
(gdb "ptype /o struct XLogRecord" on the build). wal_padding.py
(attached) takes every record that pg_waldump lists, reads those two
bytes straight from the segment file, and counts the non-zero ones.
Workload: pgbench -i -s 5, pgbench -c 4 -t 2000, CREATE INDEX,
VACUUM ANALYZE, CHECKPOINT (wal_run.sh, attached).

19beta2: 0 of 67595 records non-zero
0001-0005: 0 of 67716 records non-zero
0001-0005 without the new
memset in 0005: 16033 of 67535 records non-zero
(e.g. lsn 0/0502C978 -> 8fc3)

The third run is there to show that the check does catch garbage: the
memset that 0005 adds is what keeps the WAL identical to before. The
new workspace is 952 bytes on the stack (sizeof(struct
XLogRecordHeaderScratch)).

The rest: v3 0001-0005 applies to master at dfb474ca6d8, builds with
--enable-cassert without warnings, and all 239 regression tests pass.

Regards,
Manu

El mié, 16 sept 2026 a las 6:16, Peter Eisentraut
(<peter@eisentraut.org>) escribió: