Horton

Thread

Commits

  1. Avoid using potentially-under-aligned page buffers.

  2. Code review for pg_verify_checksums.c.

  3. Make checksum_impl.h safe to compile with -fstrict-aliasing.

  1. pg_verify_checksums and -fno-strict-aliasing

    Michael Banck <michael.banck@credativ.de> — 2018-08-30T08:35:00Z

    Hi,

I noticed that pg_verify_checksums computes bogus checksums if I compile
it with '-O2 -Wall' but without -fno-strict-aliasing. Also I am getting
a compile warning then:

|src/bin/pg_verify_checksums$ make CFLAGS='-g -O2 -Wall'
[...]
|gcc -g -O2 -Wall -I../../../src/include 
|  -I/home/mba/Projekte/OSS/PostgreSQL/git/postgresql/build/../src/include
|  -D_GNU_SOURCE   -c -o pg_verify_checksums.o
|  /home/mba/Projekte/OSS/PostgreSQL/git/postgresql/build/../src/bin/pg_verify_checksums/pg_verify_checksums.c
|  -MMD -MP -MF .deps/pg_verify_checksums.Po
|/home/mba/Projekte/OSS/PostgreSQL/git/postgresql/build/../src/bin/pg_verify_checksums/pg_verify_checksums.c: 
|  In function ‘scan_file’:
|/home/mba/Projekte/OSS/PostgreSQL/git/postgresql/build/../src/bin/pg_verify_checksums/pg_verify_checksums.c:112:3: 
|  warning: dereferencing type-punned pointer will break strict-aliasing
|  rules [-Wstrict-aliasing]
|   if (PageIsNew(buf))
|   ^~
[...]
|src/bin/pg_verify_checksums$ ./pg_verify_checksums -D ../../test/regress/tmp_check/data
|pg_verify_checksums: checksum verification failed in file "../../test/regress/tmp_check/data/global/1233", block 0: calculated checksum FC8A but expected F857
|pg_verify_checksums: checksum verification failed in file "../../test/regress/tmp_check/data/global/1233", block 1: calculated checksum F340 but expected A68D
[...]
|pg_verify_checksums: checksum verification failed in file "../../test/regress/tmp_check/data/base/12368/2659", block 5: calculated checksum 954D but expected D2ED
|pg_verify_checksums: checksum verification failed in file "../../test/regress/tmp_check/data/base/12368/2659", block 6: calculated checksum 361 but expected E7F7
|pg_verify_checksums: checksum verification failed in file "../../test/regress/tmp_check/data/base/12368/2659", block 7: calculated checksum 4C0D but expected 4B10
|pg_verify_checksums: checksum verification failed in file "../../test/regress/tmp_check/data/base/12368/2659", block 8: calculated checksum 2B9A but expected 71E3
|pg_verify_checksums: checksum verification failed in file "../../test/regress/tmp_check/data/base/12368/2659", block 9: calculated checksum 19CD but expected 541C
|Checksum scan completed
|Data checksum version: 1
|Files scanned:  932
|Blocks scanned: 2789
|Bad checksums:  2789

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.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

  2. Re: pg_verify_checksums and -fno-strict-aliasing

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-08-30T11:25:14Z

    > I noticed that pg_verify_checksums computes bogus checksums if I compile
> it with '-O2 -Wall' but without -fno-strict-aliasing. Also I am getting
> a compile warning then:
>
> [...]
>
> 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...

As PostgreSQL source is expected to conform to some C standard (unsure 
which one right now, possibly c89 but maybe it is beginning to switch to 
c99, a young 19 years old standard), I'd suggest that the right fix is 
rather to actually remove the aliasing issue.

-- 
Fabien.

  3. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-30T14:39:26Z

    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.

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

			regards, tom lane

  4. Re: pg_verify_checksums and -fno-strict-aliasing

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-08-30T14:45:47Z

    On 2018-Aug-30, Fabien COELHO wrote:

> As PostgreSQL source is expected to conform to some C standard (unsure which
> one right now, possibly c89 but maybe it is beginning to switch to c99, a
> young 19 years old standard), I'd suggest that the right fix is rather to
> actually remove the aliasing issue.

Yeah, type aliasing is quite a rampant issue in our code and I don't
think there's an easy fix.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  5. Re: pg_verify_checksums and -fno-strict-aliasing

    Michael Paquier <michael@paquier.xyz> — 2018-08-30T17:56:52Z

    On Thu, Aug 30, 2018 at 10:39:26AM -0400, Tom Lane wrote:
> 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.
> 
> (The right fix, of course, is to malloc the work buffer rather than
> put it on the stack.)

pg_upgrade/file.c is careful about that (5afcd2a), and has a comment on
the matter, as does pg_standby.c.

Now, grepping around for "BLCKSZ]", some garbage may have accumulated?
- entrySplitPage in ginentrypage.c
- rewind_copy_file_range in file.c
- _hash_alloc_buckets in hashpage.c
- pg_prewarm.c
- blinsert.c
- pg_waldump.c
- logtape.c
--
Michael

  6. Re: pg_verify_checksums and -fno-strict-aliasing

    Magnus Hagander <magnus@hagander.net> — 2018-08-30T19:35:33Z

    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?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  7. Re: pg_verify_checksums and -fno-strict-aliasing

    Magnus Hagander <magnus@hagander.net> — 2018-08-30T20:02:13Z

    On Thu, Aug 30, 2018 at 9:35 PM, Magnus Hagander <magnus@hagander.net>
wrote:

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

PFA some *very* basic tests for pg_verify_checksums, which should at least
be enough to catch the kind of errors we had now in the tool itself.

Does not at this point try to solve the fact that the backend checksum code
is not tested.


(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?
>
>
-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  8. Re: pg_verify_checksums and -fno-strict-aliasing

    Michael Paquier <michael@paquier.xyz> — 2018-08-30T20:02:58Z

    On Thu, Aug 30, 2018 at 09:35:33PM +0200, Magnus Hagander wrote:
> 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)

This should be a separate suite.  And FWIW, we only use pg_regress to
make sure that an already-initialized data folder has the correct,
secure authentication set.  So creating a node with checksums enabled is
just that:
$node->init(extra => ['--data-checksums']);

[... 20 minutes later ...]
Attached is a basic test suite ;)
--
Michael

  9. Re: pg_verify_checksums and -fno-strict-aliasing

    Magnus Hagander <magnus@hagander.net> — 2018-08-30T20:07:38Z

    On Thu, Aug 30, 2018 at 10:02 PM, Michael Paquier <michael@paquier.xyz>
wrote:

> On Thu, Aug 30, 2018 at 09:35:33PM +0200, Magnus Hagander wrote:
> > 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)
>
> This should be a separate suite.  And FWIW, we only use pg_regress to
> make sure that an already-initialized data folder has the correct,
> secure authentication set.  So creating a node with checksums enabled is
> just that:
> $node->init(extra => ['--data-checksums']);
>
> [... 20 minutes later ...]
> Attached is a basic test suite ;)
>

Haha, nice timing :)

I wonder if your tests that pg_control has picked things up belong more in
the tests of initdb itself?

Do you think there is value in testing against a non-checksum cluster? I
guess there's some point to it. I think testing actual corruption (like my
version of the tests) is more valuable, but perhaps we should just do both?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  10. Re: pg_verify_checksums and -fno-strict-aliasing

    Andres Freund <andres@anarazel.de> — 2018-08-30T20:11:45Z

    On 2018-08-30 10:39:26 -0400, Tom Lane wrote:
> 	char		buf[BLCKSZ];
> 	PageHeader	header = (PageHeader) buf;

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

Or alternatively, for places where such allocations could be a problem
for performance, using a union can be used to force the required
alignment.  I'm fairly certain that just allocating is more than OK
here, to be clear.

Greetings,

Andres Freund

  11. Re: pg_verify_checksums and -fno-strict-aliasing

    Michael Banck <michael.banck@credativ.de> — 2018-08-30T20:22:24Z

    Hi,

Am Donnerstag, den 30.08.2018, 22:02 +0200 schrieb Magnus Hagander:
> PFA some *very* basic tests for pg_verify_checksums, which should at
> least be enough to catch the kind of errors we had now in the tool
> itself.

I proposed something similar for pg_basebackup back then and IIRC Peter
(rightfully) complained that using a system catalog for that wasn't so
great - also, are you sure the relfilenode will always be stable? Then
again, this is obviously a throw-away data directory so it should be no
general issue, just a matter of taste.

https://github.com/credativ/pg_checksums/blob/master/t/001_checksums.pl
has some similar tests at the end but creates a table for corruption,
adds some safeguards for block size and also uses  command_checks_all()
to parse the output. It's PGDG licensed so you're welcome to take some
or all of that.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

  12. Re: pg_verify_checksums and -fno-strict-aliasing

    Michael Banck <michael.banck@credativ.de> — 2018-08-30T20:28:21Z

    Hi,

Am Donnerstag, den 30.08.2018, 21:35 +0200 schrieb Magnus Hagander:
> So if I get you  right, you're saying the attached patch should be all
> that's needed? 

I tried to do some similar changes but neither what you proposed nor
what I came up with actually fixes the checksum failures, though they
silence the warning at -Wall level.

Could well be I'm doing something wrong, so it would be cool if somebody
could reproduce this first. In principle, it should be enough to run
'make clean && make CLFAGS=-O2' in the src/bin/pg_verify_checksums
subdirectory in order to get a broken executable.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

  13. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-30T21:19:28Z

    Michael Banck <michael.banck@credativ.de> writes:
> Could well be I'm doing something wrong, so it would be cool if somebody
> could reproduce this first. In principle, it should be enough to run
> 'make clean && make CLFAGS=-O2' in the src/bin/pg_verify_checksums
> subdirectory in order to get a broken executable.

I can reproduce it on my Fedora 28 machine (gcc 8.1.1) by compiling
pg_verify_checksums with all the normal flags except -fno-strict-aliasing.
I don't see any warning, not even with -Wstrict-aliasing=2 :-(, but the
generated code malfunctions as Michael describes.  As best I can tell,
the problem is in the inlined code from checksum_impl.h rather than
in pg_verify_checksums.c itself.  I think what is happening is gcc
is concluding that it can optimize away the code that temporarily
sets the page's checksum field to zero.

Although, as Alvaro mentioned upthread, there's basically no hope of
getting away from -fno-strict-aliasing for Postgres-at-large, I think it'd
be a good thing if we could get checksum_impl.h to not depend on it.  The
whole point of that header, according to its own self-description, is to
export the checksum calculation for use by external programs --- and we
can't really control what compiler flags will be used by those.  The fact
that Michael even ran into this problem seems to be an instance of that.

So, I've been fooling around trying to get it to work without
-fno-strict-aliasing, but with little luck so far.

			regards, tom lane

  14. Re: pg_verify_checksums and -fno-strict-aliasing

    Andres Freund <andres@anarazel.de> — 2018-08-30T21:46:06Z

    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.

Greetings,

Andres Freund

  15. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-30T21:49:04Z

    Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Aug 30, 2018 at 4:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> (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?

Well, that's some of what's needed.  I notice this code is also being
sloppy about whether block numbers are signed or unsigned, which means
it'd probably misbehave on relations exceeding 2^31 blocks.  I have
a patch in progress to clean all that up, though I'm still working
on the strict-aliasing part.

			regards, tom lane

  16. Re: pg_verify_checksums and -fno-strict-aliasing

    Andres Freund <andres@anarazel.de> — 2018-08-30T22:04:09Z

    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

  17. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-30T22:11:40Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-08-30 14:46:06 -0700, Andres Freund wrote:
>> 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.

I suspect people will complain about the added cost of doing that.

I've been AFK all afternoon, but what I was intending to try next was
the union approach, specifically union'ing PageHeaderData with the uint32
array representation needed by pg_checksum_block().  That might also
end up giving us code less unreadable than this:

	uint32		(*dataArr)[N_SUMS] = (uint32 (*)[N_SUMS]) data;


BTW, not to mention the elephant in the room, but: is it *really* OK
that pg_checksum_page scribbles on the page buffer, even temporarily?
It's certainly quite horrid that there aren't large warnings about
that in the function's API comment.

			regards, tom lane

  18. Re: pg_verify_checksums and -fno-strict-aliasing

    Andres Freund <andres@anarazel.de> — 2018-08-30T22:33:15Z

    On 2018-08-30 18:11:40 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2018-08-30 14:46:06 -0700, Andres Freund wrote:
> >> 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.
> 
> I suspect people will complain about the added cost of doing that.

I think the compiler will just optimize it away.  But if we're concerned
we could write it as

	memcpy(&save_checksum, page + offsetof(PageHeaderData, pd_checksum), sizeof(save_checksum));
	memset(page + offsetof(PageHeaderData, pd_checksum), 0, sizeof(save_checksum));

	checksum = pg_checksum_block(page, BLCKSZ);

	memcpy(page + offsetof(PageHeaderData, pd_checksum), &save_checksum, sizeof(save_checksum));

works, but still not exceedingly pretty :/.  The code generated looks
reasonable:

194		memcpy(&save_checksum, page + offsetof(PageHeaderData, pd_checksum), sizeof(save_checksum));
   0x00000000000035d0 <+0>:	push   %r12
   0x00000000000035d2 <+2>:	xor    %eax,%eax
   0x00000000000035d4 <+4>:	movzwl 0x8(%rdi),%r12d

195		memset(page + offsetof(PageHeaderData, pd_checksum), 0, sizeof(save_checksum));
   0x00000000000035d9 <+9>:	push   %rbp
   0x00000000000035da <+10>:	mov    %ax,0x8(%rdi)

(the pushes are just interspersed stuff, yay latency aware instruction
scheduling)


> I've been AFK all afternoon, but what I was intending to try next was
> the union approach, specifically union'ing PageHeaderData with the uint32
> array representation needed by pg_checksum_block().  That might also
> end up giving us code less unreadable than this:
> 
> 	uint32		(*dataArr)[N_SUMS] = (uint32 (*)[N_SUMS]) data;

Hm.


> BTW, not to mention the elephant in the room, but: is it *really* OK
> that pg_checksum_page scribbles on the page buffer, even temporarily?
> It's certainly quite horrid that there aren't large warnings about
> that in the function's API comment.

It certainly should be warned about.  Practically I don't think it's a
problem, because we pretty much always operate on a copy of the page
when writing out, as otherwise concurrently set hint bits would be
troublesome.

Greetings,

Andres Freund

  19. Re: pg_verify_checksums and -fno-strict-aliasing

    Michael Paquier <michael@paquier.xyz> — 2018-08-30T22:44:58Z

    On Thu, Aug 30, 2018 at 10:07:38PM +0200, Magnus Hagander wrote:
> I wonder if your tests that pg_control has picked things up belong more in
> the tests of initdb itself?

For the case where checksums are disabled, moving there the check on
control data makes sense.

> Do you think there is value in testing against a non-checksum cluster? I
> guess there's some point to it. I think testing actual corruption (like my
> version of the tests) is more valuable, but perhaps we should just do both?

Yeah, let's do stuff on a single cluster which has them only enabled,
as initializing a node is one of the most costly operations in TAP
tests.  Checking that the server is stopped is definitely a must in my
opinion, and your addition about emulating corrupted blocks is a good
idea.  I would personally vote for keeping a control file check within
the tests of pg_verify_checksums as that's cheap.
--
Michael

  20. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-30T23:02:15Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-08-30 18:11:40 -0400, Tom Lane wrote:
>> I suspect people will complain about the added cost of doing that.

> I think the compiler will just optimize it away.

Maybe.  In any case, the attached version avoids any need for memcpy
and is, I think, cleaner code anyhow.  It fixes the problem for me
with Fedora's gcc, though I'm not sure that it'd be enough to get rid
of the warning Michael sees (I wonder what compiler he's using).

>> BTW, not to mention the elephant in the room, but: is it *really* OK
>> that pg_checksum_page scribbles on the page buffer, even temporarily?
>> It's certainly quite horrid that there aren't large warnings about
>> that in the function's API comment.

> It certainly should be warned about.  Practically I don't think it's a
> problem, because we pretty much always operate on a copy of the page
> when writing out, as otherwise concurrently set hint bits would be
> troublesome.

The write end of things is not a problem IMO, since presumably the caller
would be about to overwrite the checksum field anyway.  The issue is for
reading and/or verifying, where it's much less obvious that clobbering
the page image is safe.

			regards, tom lane

  21. Re: pg_verify_checksums and -fno-strict-aliasing

    Andres Freund <andres@anarazel.de> — 2018-08-30T23:12:14Z

    On 2018-08-30 19:02:15 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > It certainly should be warned about.  Practically I don't think it's a
> > problem, because we pretty much always operate on a copy of the page
> > when writing out, as otherwise concurrently set hint bits would be
> > troublesome.
> 
> The write end of things is not a problem IMO, since presumably the caller
> would be about to overwrite the checksum field anyway.  The issue is for
> reading and/or verifying, where it's much less obvious that clobbering
> the page image is safe.

With "reading" you mean putting the page into the buffercache? If so,
that should be ok, the page isn't yet accessible (BM_VALID isn't set).

I don't think it's possible to do verification from the page in s_b,
because we don't keep the checksum current, so there should never be
calls to do so.

But we probably should still add a comment making clear that the
function modifies the buffer temporarily.

Greetings,

Andres Freund

  22. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-30T23:37:37Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Aug 30, 2018 at 10:39:26AM -0400, Tom Lane wrote:
>> (The right fix, of course, is to malloc the work buffer rather than
>> put it on the stack.)

> pg_upgrade/file.c is careful about that (5afcd2a), and has a comment on
> the matter, as does pg_standby.c.

> Now, grepping around for "BLCKSZ]", some garbage may have accumulated?

Ah, that's a cute idea for finding trouble spots.

> - entrySplitPage in ginentrypage.c
> - rewind_copy_file_range in file.c
> - _hash_alloc_buckets in hashpage.c
> - pg_prewarm.c
> - blinsert.c
> - pg_waldump.c
> - logtape.c

Some of these are safe, I think, because the buffers are only used as
targets for read() and write().  But some are definitely broken.
My own list of files that seem to have issues is

blinsert.c
generic_xlog.c
ginentrypage.c
hashpage.c
pg_verify_checksums.c
pg_waldump.c
xloginsert.c

The fact that some of these are pretty old and we've not noticed is
not good.  It suggests that we don't currently have any compilers in the
buildfarm that under-align char[] arrays on the stack, which seems like
a gotcha waiting to bite us.  I wonder if there is any way to persuade
some compiler on a non-Intel box to do that.

Anyway, I'll work on a patch for that, unless you were on it already?

			regards, tom lane

  23. Re: pg_verify_checksums and -fno-strict-aliasing

    Michael Paquier <michael@paquier.xyz> — 2018-08-31T00:06:00Z

    On Thu, Aug 30, 2018 at 07:37:37PM -0400, Tom Lane wrote:
> Some of these are safe, I think, because the buffers are only used as
> targets for read() and write().  But some are definitely broken.

Yes, I have not spent more than a couple of minutes on this issue.  I
noticed some of them easily though.

> My own list of files that seem to have issues is
> 
> blinsert.c
> generic_xlog.c
> ginentrypage.c
> hashpage.c
> pg_verify_checksums.c
> pg_waldump.c
> xloginsert.c
> 
> The fact that some of these are pretty old and we've not noticed is
> not good.  It suggests that we don't currently have any compilers in the
> buildfarm that under-align char[] arrays on the stack, which seems like
> a gotcha waiting to bite us.  I wonder if there is any way to persuade
> some compiler on a non-Intel box to do that.

Agreed, that's annoying.

> Anyway, I'll work on a patch for that, unless you were on it already?

I have begun working on that and am less than halfway through it as I
needed a fresh problem, however I am not sure I would be able to finish
it today, perhaps tomorrow...  If you have time and want to press on as
11 would release soon, of course feel free to wrap up more quickly than
I can.
--
Michael

  24. Re: pg_verify_checksums and -fno-strict-aliasing

    Michael Banck <michael.banck@credativ.de> — 2018-08-31T08:11:07Z

    Hi,

Am Donnerstag, den 30.08.2018, 19:02 -0400 schrieb Tom Lane:
> Andres Freund <andres@anarazel.de> writes:
> > On 2018-08-30 18:11:40 -0400, Tom Lane wrote:
> > > I suspect people will complain about the added cost of doing that.
> > I think the compiler will just optimize it away.
> 
> Maybe.  In any case, the attached version avoids any need for memcpy
> and is, I think, cleaner code anyhow.  It fixes the problem for me
> with Fedora's gcc, though I'm not sure that it'd be enough to get rid
> of the warning Michael sees (I wonder what compiler he's using).

This fixes the bogus checksums, thanks!

I am on Debian stable, i.e. 'gcc version 6.3.0 20170516 (Debian 6.3.0-
18+deb9u1)'. 

The warning shows up only if I add -Wall *and* -O2 to CFLAGS, I think I
did not mention that explictly before.

As it is in pg_verify_checksums.c I need Magnus' patch as well to make
it go away. But the warning is independent of the runtime issue so less
of an issue (but probably worth fixing).


Michael 

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

  25. Re: pg_verify_checksums and -fno-strict-aliasing

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-08-31T11:10:08Z

    On 31/08/2018 01:37, Tom Lane wrote:
> The fact that some of these are pretty old and we've not noticed is
> not good.  It suggests that we don't currently have any compilers in the
> buildfarm that under-align char[] arrays on the stack, which seems like
> a gotcha waiting to bite us.  I wonder if there is any way to persuade
> some compiler on a non-Intel box to do that.

-Wcast-align=strict (requires gcc-8) warns about the issue in
pg_verify_checksums.c, but also about a handful^W^W15211 other things,
so it wouldn't be immediately useful.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  26. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-31T14:55:06Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Aug 30, 2018 at 07:37:37PM -0400, Tom Lane wrote:
>> Anyway, I'll work on a patch for that, unless you were on it already?

> I have begun working on that and am less than halfway through it as I
> needed a fresh problem, however I am not sure I would be able to finish
> it today, perhaps tomorrow...  If you have time and want to press on as
> 11 would release soon, of course feel free to wrap up more quickly than
> I can.

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;

and replacing plain char[] variables with that as appropriate.
We might need one for XLOG_BLCKSZ too.

Since this'd be used in frontend as well as backend, it'd likely
have to end up in c.h, which is slightly annoying but not really
a big deal as long as we pick a nonconflicting type name.

			regards, tom lane

  27. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-31T16:02:04Z

    Michael Banck <michael.banck@credativ.de> writes:
> Am Donnerstag, den 30.08.2018, 19:02 -0400 schrieb Tom Lane:
>> Maybe.  In any case, the attached version avoids any need for memcpy
>> and is, I think, cleaner code anyhow.  It fixes the problem for me
>> with Fedora's gcc, though I'm not sure that it'd be enough to get rid
>> of the warning Michael sees (I wonder what compiler he's using).

> This fixes the bogus checksums, thanks!
> I am on Debian stable, i.e. 'gcc version 6.3.0 20170516 (Debian 6.3.0-
> 18+deb9u1)'.

Ah-hah.  I still can't replicate any warning with gcc 8.1.1, but
I do have a Raspbian installation at hand, with
gcc version 6.3.0 20170516 (Raspbian 6.3.0-18+rpi1+deb9u1) 
and on that I get

$ gcc  -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fwrapv -fexcess-precision=standard -g -O2 -I../../../src/include  -D_GNU_SOURCE   -c -o pg_verify_checksums.o pg_verify_checksums.c
pg_verify_checksums.c: In function 'scan_file':
pg_verify_checksums.c:112:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
   if (PageIsNew(buf))
   ^~

Adding -Wstrict-aliasing=2 makes it slightly more verbose:

$ gcc -Wstrict-aliasing=2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fwrapv -fexcess-precision=standard -g -O2 -I../../../src/include  -D_GNU_SOURCE   -c -o pg_verify_checksums.o pg_verify_checksums.c
pg_verify_checksums.c: In function 'scan_file':
pg_verify_checksums.c:82:2: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  PageHeader header = (PageHeader) buf;
  ^~~~~~~~~~
pg_verify_checksums.c:112:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
   if (PageIsNew(buf))
   ^~

Compiling this way also causes pg_verify_checksums to compute wrong
checksums.  Installing the patched version of checksum_impl.h fixes
that, but doesn't change the warnings, meaning they have absolutely
nothing to do with the actual problem :-(

So, even aside from the difficulty of getting rid of aliasing-unsafe
code in Postgres, the state of the compiler warning technology is
still years short of where we could sanely consider dispensing with
-fno-strict-aliasing in general.  Still, as I said before, it'd be
a good idea for checksum_impl.h to not depend on that.  I'll go
ahead and push the fix.

			regards, tom lane

  28. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-31T19:55:51Z

    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.

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.

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.

This needs to be back-patched as relevant, of course.

			regards, tom lane

  29. Re: pg_verify_checksums and -fno-strict-aliasing

    Michael Paquier <michael@paquier.xyz> — 2018-08-31T22:21:19Z

    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

  30. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-31T23:59:58Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Aug 31, 2018 at 03:55:51PM -0400, Tom Lane wrote:
>> 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.

I intentionally didn't change replay_image_masked/master_image_masked
to use statically-allocated buffers.  Since, AFAICS, those aren't
needed in most backend processes, they'd just be eating 16KB of
per-process data space to no purpose.

The others you mention could be changed, probably, but I didn't
bother as they didn't seem performance-critical.

>> +typedef union PGAlignedBuffer

> One complain I have is about the name of those structures.  Perhaps
> PGAlignedBlock and PGAlignedXlogBlock make more sense?

Don't have a strong preference, anybody else have an opinion?

(I also wondered whether to use "WAL" instead of "XLog" in that
struct name, but it seems like we've mostly stuck with "xlog"
in internal C names.)

			regards, tom lane

  31. Re: pg_verify_checksums and -fno-strict-aliasing

    Michael Paquier <michael@paquier.xyz> — 2018-09-01T00:33:03Z

    On Fri, Aug 31, 2018 at 07:59:58PM -0400, Tom Lane wrote:
> The others you mention could be changed, probably, but I didn't
> bother as they didn't seem performance-critical.

It is not really critical indeed.  There is an argument to change them
so as other folks get used to it though.

> (I also wondered whether to use "WAL" instead of "XLog" in that
> struct name, but it seems like we've mostly stuck with "xlog"
> in internal C names.)

XLOG_BLCKSZ is used, which makes me think that XLog is better than WAL
here.  A matter of taste of course.
--
Michael

  32. Re: pg_verify_checksums and -fno-strict-aliasing

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-09-01T05:24:38Z

    Hello,

>> 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.
>
> I intentionally didn't change replay_image_masked/master_image_masked
> to use statically-allocated buffers.  Since, AFAICS, those aren't
> needed in most backend processes, they'd just be eating 16KB of
> per-process data space to no purpose.
>
> The others you mention could be changed, probably, but I didn't
> bother as they didn't seem performance-critical.

I'd go for having just one same approach everywhere, for code base 
homogeneity.

>>> +typedef union PGAlignedBuffer
>
>> One complain I have is about the name of those structures.  Perhaps
>> PGAlignedBlock and PGAlignedXlogBlock make more sense?
>
> Don't have a strong preference, anybody else have an opinion?

I like "Block" better, because it's more precise.

> (I also wondered whether to use "WAL" instead of "XLog" in that
> struct name, but it seems like we've mostly stuck with "xlog"
> in internal C names.)

Best to blend with the surrounding code in the header file?

-- 
Fabien.

  33. Re: pg_verify_checksums and -fno-strict-aliasing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-01T19:32:10Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Aug 31, 2018 at 07:59:58PM -0400, Tom Lane wrote:
>> The others you mention could be changed, probably, but I didn't
>> bother as they didn't seem performance-critical.

> It is not really critical indeed.  There is an argument to change them
> so as other folks get used to it though.

Fair enough.  I renamed the types as suggested, changed a few more
places for consistency's sake, and pushed.

There still remain some places where palloc(BLCKSZ) or equivalent is used,
but there's no matching pfree.  In a lot of them the buffer is returned
to the caller so there's no choice.  It's likely that some are just
leaking the storage transiently and we could convert them to using a
PGAlignedBlock local variable, but I didn't bother trying to do the
analysis.

			regards, tom lane

  34. Re: pg_verify_checksums and -fno-strict-aliasing

    Michael Paquier <michael@paquier.xyz> — 2018-09-01T23:19:00Z

    On Sat, Sep 01, 2018 at 03:32:10PM -0400, Tom Lane wrote:
> Fair enough.  I renamed the types as suggested, changed a few more
> places for consistency's sake, and pushed.

Thanks!

> There still remain some places where palloc(BLCKSZ) or equivalent is used,
> but there's no matching pfree.  In a lot of them the buffer is returned
> to the caller so there's no choice.  It's likely that some are just
> leaking the storage transiently and we could convert them to using a
> PGAlignedBlock local variable, but I didn't bother trying to do the
> analysis.

At quick glance, I am not seeing anything critical.  So the result looks
good to me.
--
Michael