> On Dec 1, 2025, at 15:55, Peter Eisentraut <[email protected]> wrote:
>
> Commit faeedbcefd4 changed the alignment of WAL buffers from XLOG_BLCKSZ to
> PG_IO_ALIGN_SIZE.
>
> While looking around for places to apply alignas, I think I found at least
> two places that were forgotten, namely in BootStrapXLOG() and in
> pg_test_fsync.c. Patches attached for those.
>
> I also suspect that the TYPEALIGN call in XLOGShmemInit() should take
> PG_IO_ALIGN_SIZE into account, but it's not immediately obvious how, since
> the comment also mentions that it wants alignment on "a full xlog block size
> boundary". Maybe Max(XLOG_BLCKSZ, PG_IO_ALIGN_SIZE)?
Agreed, in order to keep both full xlog block and direct-I/O alignment the same.
>
> I also wonder whether the check in check_debug_io_direct() for #if
> XLOG_BLCKSZ < PG_IO_ALIGN_SIZE would be required if we fixed all those places?
Also agreed. Once every WAL buffer aligns by PG_IO_SIZE, the check will become
redundant.
>
> <0001-Use-PG_IO_ALIGN_SIZE-for-aligning-WAL-buffers.patch><0002-Use-PGAlignedXLogBlock-in-BootStrapXLOG.patch><0003-pg_test_fsync-Align-test-data-using-PGAlignedXLogBlo.patch>
Overall the patch looks good to me:
0001 fixes two overlooked alignment to PG_IO_ALIGN_SIZE
0002 switches BootStrapXLOG to use PGAlignedXLogBlock, which is aligned to
PG_IO_ALIGN_SIZE
0003 does the same for pg_test_fsync
My only nit comment is in 0002:
```
- memset(page, 0, XLOG_BLCKSZ);
+ memset(&buffer, 0, sizeof buffer);
```
I know “sizeof” is an operator instead of a function, “sizeof buffer” is
grammatically correct. However, most of places do “sizeof(buffer)”, so unless
we want to prompt the syntax of “sizeof buffer” (without the braces), it’s
better to keep a consistent syntax.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
