Re: pg_verify_checksums and -fno-strict-aliasing
Michael Paquier <michael@paquier.xyz>
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Fabien COELHO <coelho@cri.ensmp.fr>, Michael Banck <michael.banck@credativ.de>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2018-08-31T22:21:19Z
Lists: pgsql-hackers
On Fri, Aug 31, 2018 at 03:55:51PM -0400, Tom Lane wrote:
> I wrote:
>> Some of these places might be performance-critical enough that adding
>> a palloc/pfree cycle would not be nice. What I was considering doing
>> was inventing something like
>>
>> typedef union PGAlignedBuffer
>> {
>> char data[BLCKSZ];
>> double force_align;
>> } PGAlignedBuffer;
>
> Here's a proposed patch using that approach.
This solution is smarter than using malloc/palloc to enforce alignment.
I was digging into the GIN and bloom code with this pattern, but except
if I used TopTransactionContext for a simple approach, which is not
acceptable by the way, I was finishing with ugly memory contexts
used... I am still not sure if that was completely correct either, this
needed more time ;)
> Although some of the places that were using "char buf[BLCKSZ]" variables
> weren't doing anything that really requires better alignment, I made
> almost all of them use PGAlignedBuffer variables anyway, on the grounds
> that you typically get better memcpy speed for aligned than unaligned
> transfers.
Makes sense.
> I also fixed a few places that were using the palloc solution, and
> one that was actually doing hand-alignment of the pointer (ugh).
> I didn't try to be thorough about getting rid of such pallocs,
> just fix places that seemed likely to be performance-critical.
Okay, for the memo replay_image_masked and master_image_masked
in xlog.c could make use of the new structure. SetWALSegSize in
pg_standby.c and WriteEmptyXLOG in pg_resetwal.c, and pg_upgrade's
file.c could also be patched.
walmethods.c could also use some static buffers, not worth the
complication perhaps..
> +typedef union PGAlignedBuffer
> +{
> + char data[BLCKSZ];
> + double force_align_d;
> + int64 force_align_i64;
> +} PGAlignedBuffer;
> +
> +/* Same, but for an XLOG_BLCKSZ-sized buffer */
> +typedef union PGAlignedXLogBuffer
> +{
> + char data[XLOG_BLCKSZ];
> + double force_align_d;
> + int64 force_align_i64;
> +} PGAlignedXLogBuffer;
One complain I have is about the name of those structures. Perhaps
PGAlignedBlock and PGAlignedXlogBlock make more sense?
--
Michael
Commits
-
Avoid using potentially-under-aligned page buffers.
- 44cac9346479 12.0 landed
- 5af055ed7446 9.3.25 landed
- f5c93cf92223 11.0 landed
- 826980424538 9.6.11 landed
- 10b9af3ebbed 10.6 landed
- 083d9ced14e4 9.4.20 landed
- 03ffe5553c1d 9.5.15 landed
-
Code review for pg_verify_checksums.c.
- d787af7badfe 11.0 landed
- d9c366f9e801 12.0 landed
-
Make checksum_impl.h safe to compile with -fstrict-aliasing.
- db87d3b5253f 9.3.25 landed
- 20f9cd55dd53 9.4.20 landed
- 853af991e35a 9.5.15 landed
- d6ef17ed7bba 9.6.11 landed
- c2dfbd18ce25 10.6 landed
- 9daff2fe69dc 11.0 landed
- 8c62d9d16f01 12.0 landed