On 2018-08-30 14:46:06 -0700, Andres Freund wrote: > Hi, > > On 2018-08-30 17:19:28 -0400, Tom Lane wrote: > > So, I've been fooling around trying to get it to work without > > -fno-strict-aliasing, but with little luck so far. > > The problem presumably is that pg_checksum_block() accesses the relevant > fields as an uint32, whereas pg_checksum_page() accesses it as a > PageHeader. That's an aliasing violation. *One* cast from char* to > either type is fine, it's accessing under both those types that's > problematic. > > One way to fix it would be to memcpy in/out the modified PageHeader, or > just do offset math and memcpy to that offset.
It took me a bit to reproduce the issue (due to sheer stupidity on my part: no, changing the flags passed to gcc to link pg_verify_checksums doesn't do the trick), but the above indeed fixes the issue for me. The attached is just for demonstration that the approach works. Greetings, Andres Freund
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c index 7cf3cf35c7a..c23b0dd5c37 100644 --- a/src/bin/pg_verify_checksums/pg_verify_checksums.c +++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c @@ -78,7 +78,7 @@ skipfile(char *fn) static void scan_file(char *fn, int segmentno) { - char buf[BLCKSZ]; + char *buf = palloc(BLCKSZ); PageHeader header = (PageHeader) buf; int f; int blockno; @@ -126,6 +126,8 @@ scan_file(char *fn, int segmentno) } close(f); + + pfree(buf); } static void diff --git a/src/include/storage/checksum_impl.h b/src/include/storage/checksum_impl.h index 64d76229add..a258ea8d117 100644 --- a/src/include/storage/checksum_impl.h +++ b/src/include/storage/checksum_impl.h @@ -178,12 +178,14 @@ pg_checksum_block(char *data, uint32 size) uint16 pg_checksum_page(char *page, BlockNumber blkno) { - PageHeader phdr = (PageHeader) page; + PageHeaderData phdr; uint16 save_checksum; uint32 checksum; + memcpy(&phdr, page, sizeof(PageHeaderData)); + /* We only calculate the checksum for properly-initialized pages */ - Assert(!PageIsNew(page)); + Assert(!PageIsNew(&page)); /* * Save pd_checksum and temporarily set it to zero, so that the checksum @@ -191,10 +193,14 @@ pg_checksum_page(char *page, BlockNumber blkno) * Restore it after, because actually updating the checksum is NOT part of * the API of this function. */ - save_checksum = phdr->pd_checksum; - phdr->pd_checksum = 0; + save_checksum = phdr.pd_checksum; + phdr.pd_checksum = 0; + memcpy(page, &phdr, sizeof(PageHeaderData)); + checksum = pg_checksum_block(page, BLCKSZ); - phdr->pd_checksum = save_checksum; + + phdr.pd_checksum = save_checksum; + memcpy(page, &phdr, sizeof(PageHeaderData)); /* Mix in the block number to detect transposed pages */ checksum ^= blkno;