Re: pg_verify_checksums and -fno-strict-aliasing

On Thu, Aug 30, 2018 at 4:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> If I add -fno-strict-aliasing to $CFLAGS, the problem goes away.
>> Is this something to worry about, or just pilot error cause I am not
>> using the same $CFLAGS as for the rest of the build? I originally
>> noticed this problem with my external fork of pg_verify_checksums.

> It looks like the code is using aliasing where the standard says it should
> not, which breaks compiler optimization, and the added options tells the
> compiler to not assume that the code conforms to the standard...

Actually, this code is just plain broken:

        char            buf[BLCKSZ];
        PageHeader      header = (PageHeader) buf;

There is no guarantee that a local char[] array is aligned on anything
stronger than a byte boundary, and if it isn't, word-sized accesses to
*header are going to fail on machines with strict alignment rules.
I suppose that that is really what Michael's compiler is moaning about.

I rather suspect that this hasn't been tested on anything but Intel
hardware, which is famously misalignment-tolerant.  The lack of any
apparent regression test infrastructure for it isn't leaving a warm
feeling about how much the buildfarm is testing it.

I know I certainly didn't test it on non-intel.

We did have that in the online verify checksum patch, but it came out as part of the revert of that part.

Given that we also recently found bugs in the actual hash index implementation that would've gotten caught by this, perhaps we should add a TAP test for this.

Should we make it a separate test in pg_verify_checksums, or should we piggyback on the pg_basebackup tests (which AFAICT is the only ones that create a cluster with checksums enabled at all, and thus is the only codepath that actually uses the backend checksum code at all, which I think is an even worse thing to have no tests of)

(The right fix, of course, is to malloc the work buffer rather than
put it on the stack.)

So if I get you  right, you're saying the attached patch should be all that's needed?