Re: Use C11 alignas instead of palloc/malloc for alignment

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(at)eisentraut(dot)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