Date: Fri, 31 Aug 2018 15:21:19 -0700
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>
Subject: Re: pg_verify_checksums and -fno-strict-aliasing
Message-ID: <20180831222119.GD5305@paquier.xyz>
References: <1535618100.1286.3.camel@credativ.de>
 <alpine.DEB.2.21.1808301325070.32764@lancre>
 <7321.1535639966@sss.pgh.pa.us>
 <20180830175652.GC15446@paquier.xyz>
 <12123.1535672257@sss.pgh.pa.us>
 <20180831000600.GI15446@paquier.xyz>
 <28226.1535727306@sss.pgh.pa.us>
 <21037.1535745351@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="7CZp05NP8/gJM8Cl"
Content-Disposition: inline
In-Reply-To: <21037.1535745351@sss.pgh.pa.us>
Precedence: bulk


--7CZp05NP8/gJM8Cl
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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;
>=20
> 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.. =20

> +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

--7CZp05NP8/gJM8Cl
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAluJv14ACgkQnvQgOdby
QH2aTQ//b92DIpNu7ljDTm8RwrlUuBbG7n5xACWIrXN3PgKpfzCMTLlKxeadAf5U
PWG2VaXG7vQpTW3SV2ODaix3kg2kFdkuTXD4yhvPNq2V7Tb05juWLJ5JLk+D5+fh
7iIzPwI6sWAVxmKYc8NwKZ8dvRODxYcjK5+xWHflQMQWEdb8c6vP/RRPbY+Vzlp1
HGzVDXiG11xJWUel1qLJLJWwFpAvnhffgtRzLuOxJdbWIogxpP0ZSe/GBm3N9VNh
l3I5N/j3VQXWp4yn++PPRX+P4T4xoCdnC+eJI9tfQENsZN0gAUYIIevkEYUfslUV
m7W8CNuxQ3myNtLSofo3ldOYbM0NXfGZ6dYD0KlEzgnRZZFSqTlqEP5waRK6W0b9
lbO2KljT47o9UIMx+b/63M/JDq2HARssOKm/mpGgZxcz/BxA5+UYZ/Gucx6ElF4o
eFm6wKvfAm7+TEoUB3usiv8742fzzSifYMoxk1Crch8p/eEg5Bqr+Wpp/fLVXPPA
G/ct8/3newBWOjmdjakhfN6jP1oztFUP19kuUfLagVlumYHBZHRAgFOQCWkn0o+d
xHEhtd5AMT3E76+ksKT3Td/rcMXGL/QItlEdGrTIEar7f7pwGmBOnZ6DymGTgxlK
qWZIuZxrjVanRA01sDo9BQoSGiXw9viAcdri93aYgAMtcxL2SEQ=
=MDN1
-----END PGP SIGNATURE-----

--7CZp05NP8/gJM8Cl--