Re: pg_verify_checksums and -fno-strict-aliasing

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

Michael Paquier  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

Re: pg_verify_checksums and -fno-strict-aliasing

[Fabien COELHO]

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.

Re: pg_verify_checksums and -fno-strict-aliasing

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

Michael Paquier  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

Re: pg_verify_checksums and -fno-strict-aliasing

[Michael Paquier]

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
>> {
>>  chardata[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
> +{
> + chardata[BLCKSZ];
> + double  force_align_d;
> + int64   force_align_i64;
> +} PGAlignedBuffer;
> +
> +/* Same, but for an XLOG_BLCKSZ-sized buffer */
> +typedef union PGAlignedXLogBuffer
> +{
> + chardata[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


signature.asc
Description: PGP signature

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

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
> {
>   chardata[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

diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index 4afdea7..5ae1dda 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -36,7 +36,7 @@ typedef struct
 	int64		indtuples;		/* total number of tuples indexed */
 	MemoryContext tmpCtx;		/* temporary memory context reset after each
  * tuple */
-	char		data[BLCKSZ];	/* cached page */
+	PGAlignedBuffer data;		/* cached page */
 	int			count;			/* number of tuples in cached page */
 } BloomBuildState;
 
@@ -52,7 +52,7 @@ flushCachedPage(Relation index, BloomBuildState *buildstate)
 
 	state = GenericXLogStart(index);
 	page = GenericXLogRegisterBuffer(state, buffer, GENERIC_XLOG_FULL_IMAGE);
-	memcpy(page, buildstate->data, BLCKSZ);
+	memcpy(page, buildstate->data.data, BLCKSZ);
 	GenericXLogFinish(state);
 	UnlockReleaseBuffer(buffer);
 }
@@ -63,8 +63,8 @@ flushCachedPage(Relation index, BloomBuildState *buildstate)
 static void
 initCachedPage(BloomBuildState *buildstate)
 {
-	memset(buildstate->data, 0, BLCKSZ);
-	BloomInitPage(buildstate->data, 0);
+	memset(buildstate->data.data, 0, BLCKSZ);
+	BloomInitPage(buildstate->data.data, 0);
 	buildstate->count = 0;
 }
 
@@ -84,7 +84,7 @@ bloomBuildCallback(Relation index, HeapTuple htup, Datum *values,
 	itup = BloomFormTuple(>blstate, >t_self, values, isnull);
 
 	/* Try to add next item to cached page */
-	if (BloomPageAddItem(>blstate, buildstate->data, itup))
+	if (BloomPageAddItem(>blstate, buildstate->data.data, itup))
 	{
 		/* Next item was added successfully */
 		buildstate->count++;
@@ -98,7 +98,7 @@ bloomBuildCallback(Relation index, HeapTuple htup, Datum *values,
 
 		initCachedPage(buildstate);
 
-		if (!BloomPageAddItem(>blstate, buildstate->data, itup))
+		if (!BloomPageAddItem(>blstate, buildstate->data.data, itup))
 		{
 			/* We shouldn't be here since we're inserting to the empty page */
 			elog(ERROR, "could not add new bloom tuple to empty page");
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index 3cbb7c2..1a00c59 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -36,7 +36,7 @@ typedef enum
 	PREWARM_BUFFER
 } PrewarmType;
 
-static char blockbuffer[BLCKSZ];
+static PGAlignedBuffer blockbuffer;
 
 /*
  * pg_prewarm(regclass, mode text, fork text,
@@ -178,7 +178,7 @@ pg_prewarm(PG_FUNCTION_ARGS)
 		for (block = first_block; block <= last_block; ++block)
 		{
 			CHECK_FOR_INTERRUPTS();
-			smgrread(rel->rd_smgr, forkNumber, block, blockbuffer);
+			smgrread(rel->rd_smgr, forkNumber, block, blockbuffer.data);
 			++blocks_done;
 		}
 	}
diff --git a/src/backend/access/gin/ginentrypage.c b/src/backend/access/gin/ginentrypage.c
index 8107697..ef5c367 100644
--- a/src/backend/access/gin/ginentrypage.c
+++ b/src/backend/access/gin/ginentrypage.c
@@ -616,7 +616,7 @@ entrySplitPage(GinBtree btree, Buffer origbuf,
 	Page		lpage = PageGetTempPageCopy(BufferGetPage(origbuf));
 	Page		rpage = PageGetTempPageCopy(BufferGetPage(origbuf));
 	Size		pageSize = PageGetPageSize(lpage);
-	char		tupstore[2 * BLCKSZ];
+	PGAlignedBuffer tupstore[2];	/* could need 2 pages' worth of tuples */
 
 	entryPreparePage(btree, lpage, off, insertData, updateblkno);
 
@@ -625,7 +625,7 @@ entrySplitPage(GinBtree btree, Buffer origbuf,
 	 * one after another in a temporary workspace.
 	 */
 	maxoff = PageGetMaxOffsetNumber(lpage);
-	ptr = tupstore;
+	ptr = tupstore[0].data;
 	for (i = FirstOffsetNumber; i <= maxoff; i++)
 	{
 		if (i == off)
@@ -658,7 +658,7 @@ entrySplitPage(GinBtree btree, Buffer origbuf,
 	GinInitPage(rpage, GinPageGetOpaque(lpage)->flags, pageSize);
 	GinInitPage(lpage, GinPageGetOpaque(rpage)->flags, pageSize);
 
-	ptr = tupstore;
+	ptr = tupstore[0].data;
 	maxoff++;
 	lsize = 0;
 
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
index e32807e..915b9ca 100644
--- a/src/backend/access/gin/ginfast.c
+++ b/src/backend/access/gin/ginfast.c
@@ -64,18 +64,15 @@ 

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

Michael Banck  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

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

Michael Paquier  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
{
chardata[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

Re: pg_verify_checksums and -fno-strict-aliasing

[Peter Eisentraut]

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

Re: pg_verify_checksums and -fno-strict-aliasing

[Michael Banck]

Hi,

Am Donnerstag, den 30.08.2018, 19:02 -0400 schrieb Tom Lane:
> Andres Freund  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.ba...@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

Re: pg_verify_checksums and -fno-strict-aliasing

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

Michael Paquier  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

Re: pg_verify_checksums and -fno-strict-aliasing

[Andres Freund]

On 2018-08-30 19:02:15 -0400, Tom Lane wrote:
> Andres Freund  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

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

Andres Freund  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

diff --git a/src/include/storage/checksum_impl.h b/src/include/storage/checksum_impl.h
index 64d7622..a49d27f 100644
--- a/src/include/storage/checksum_impl.h
+++ b/src/include/storage/checksum_impl.h
@@ -107,6 +107,13 @@
 /* prime multiplier of FNV-1a hash */
 #define FNV_PRIME 16777619
 
+/* Use a union so that this code is valid under strict aliasing */
+typedef union
+{
+	PageHeaderData phdr;
+	uint32		data[BLCKSZ / (sizeof(uint32) * N_SUMS)][N_SUMS];
+} PGChecksummablePage;
+
 /*
  * Base offsets to initialize each of the parallel FNV hashes into a
  * different initial state.
@@ -132,28 +139,27 @@ do { \
 } while (0)
 
 /*
- * Block checksum algorithm.  The data argument must be aligned on a 4-byte
- * boundary.
+ * Block checksum algorithm.  The page must be adequately aligned
+ * (at least on 4-byte boundary).
  */
 static uint32
-pg_checksum_block(char *data, uint32 size)
+pg_checksum_block(const PGChecksummablePage *page)
 {
 	uint32		sums[N_SUMS];
-	uint32		(*dataArr)[N_SUMS] = (uint32 (*)[N_SUMS]) data;
 	uint32		result = 0;
 	uint32		i,
 j;
 
 	/* ensure that the size is compatible with the algorithm */
-	Assert((size % (sizeof(uint32) * N_SUMS)) == 0);
+	Assert(sizeof(PGChecksummablePage) == BLCKSZ);
 
 	/* initialize partial checksums to their corresponding offsets */
 	memcpy(sums, checksumBaseOffsets, sizeof(checksumBaseOffsets));
 
 	/* main checksum calculation */
-	for (i = 0; i < size / sizeof(uint32) / N_SUMS; i++)
+	for (i = 0; i < (uint32) (BLCKSZ / (sizeof(uint32) * N_SUMS)); i++)
 		for (j = 0; j < N_SUMS; j++)
-			CHECKSUM_COMP(sums[j], dataArr[i][j]);
+			CHECKSUM_COMP(sums[j], page->data[i][j]);
 
 	/* finally add in two rounds of zeroes for additional mixing */
 	for (i = 0; i < 2; i++)
@@ -168,8 +174,10 @@ pg_checksum_block(char *data, uint32 size)
 }
 
 /*
- * Compute the checksum for a Postgres page.  The page must be aligned on a
- * 4-byte boundary.
+ * Compute the checksum for a Postgres page.
+ *
+ * The page must be adequately aligned (at least on a 4-byte boundary).
+ * Beware also that the checksum field of the page is transiently zeroed.
  *
  * The checksum includes the block number (to detect the case where a page is
  * somehow moved to a different location), the page header (excluding the
@@ -178,12 +186,12 @@ pg_checksum_block(char *data, uint32 size)
 uint16
 pg_checksum_page(char *page, BlockNumber blkno)
 {
-	PageHeader	phdr = (PageHeader) page;
+	PGChecksummablePage *cpage = (PGChecksummablePage *) page;
 	uint16		save_checksum;
 	uint32		checksum;
 
 	/* We only calculate the checksum for properly-initialized pages */
-	Assert(!PageIsNew(page));
+	Assert(!PageIsNew(>phdr));
 
 	/*
 	 * Save pd_checksum and temporarily set it to zero, so that the checksum
@@ -191,10 +199,10 @@ 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;
-	checksum = pg_checksum_block(page, BLCKSZ);
-	phdr->pd_checksum = save_checksum;
+	save_checksum = cpage->phdr.pd_checksum;
+	cpage->phdr.pd_checksum = 0;
+	checksum = pg_checksum_block(cpage);
+	cpage->phdr.pd_checksum = save_checksum;
 
 	/* Mix in the block number to detect transposed pages */
 	checksum ^= blkno;

Re: pg_verify_checksums and -fno-strict-aliasing

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_verify_checksums and -fno-strict-aliasing

[Andres Freund]

On 2018-08-30 18:11:40 -0400, Tom Lane wrote:
> Andres Freund  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(_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), _checksum, 
sizeof(save_checksum));

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

194 memcpy(_checksum, page + offsetof(PageHeaderData, 
pd_checksum), sizeof(save_checksum));
   0x35d0 <+0>: push   %r12
   0x35d2 <+2>: xor%eax,%eax
   0x35d4 <+4>: movzwl 0x8(%rdi),%r12d

195 memset(page + offsetof(PageHeaderData, pd_checksum), 0, 
sizeof(save_checksum));
   0x35d9 <+9>: push   %rbp
   0x35da <+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

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

Andres Freund  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

Re: pg_verify_checksums and -fno-strict-aliasing

[Andres Freund]

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(, page, sizeof(PageHeaderData));
+
 	/* We only calculate the checksum for properly-initialized pages */
-	Assert(!PageIsNew(page));
+	Assert(!PageIsNew());
 
 	/*
 	 * 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, , sizeof(PageHeaderData));
+
 	checksum = pg_checksum_block(page, BLCKSZ);
-	phdr->pd_checksum = save_checksum;
+
+	phdr.pd_checksum = save_checksum;
+	memcpy(page, , sizeof(PageHeaderData));
 
 	/* Mix in the block number to detect transposed pages */
 	checksum ^= blkno;

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

Magnus Hagander  writes:
> On Thu, Aug 30, 2018 at 4:39 PM, Tom Lane  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

Re: pg_verify_checksums and -fno-strict-aliasing

[Andres Freund]

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

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

Michael Banck  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

Re: pg_verify_checksums and -fno-strict-aliasing

[Michael Banck]

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.ba...@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

Re: pg_verify_checksums and -fno-strict-aliasing

[Michael Banck]

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.ba...@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

Re: pg_verify_checksums and -fno-strict-aliasing

[Andres Freund]

On 2018-08-30 10:39:26 -0400, Tom Lane wrote:
>   charbuf[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

Re: pg_verify_checksums and -fno-strict-aliasing

[Magnus Hagander]

On Thu, Aug 30, 2018 at 10:02 PM, Michael Paquier 
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/ 
 Work: https://www.redpill-linpro.com/ 

Re: pg_verify_checksums and -fno-strict-aliasing

[Michael Paquier]

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
diff --git a/src/bin/pg_verify_checksums/.gitignore b/src/bin/pg_verify_checksums/.gitignore
index d1dcdaf0dd..0e5e569a54 100644
--- a/src/bin/pg_verify_checksums/.gitignore
+++ b/src/bin/pg_verify_checksums/.gitignore
@@ -1 +1,3 @@
 /pg_verify_checksums
+
+/tmp_check/
diff --git a/src/bin/pg_verify_checksums/Makefile b/src/bin/pg_verify_checksums/Makefile
index d16261571f..cfe4ab1b8b 100644
--- a/src/bin/pg_verify_checksums/Makefile
+++ b/src/bin/pg_verify_checksums/Makefile
@@ -34,3 +34,9 @@ uninstall:
 clean distclean maintainer-clean:
 	rm -f pg_verify_checksums$(X) $(OBJS)
 	rm -rf tmp_check
+
+check:
+	$(prove_check)
+
+installcheck:
+	$(prove_installcheck)
diff --git a/src/bin/pg_verify_checksums/t/001_basic.pl b/src/bin/pg_verify_checksums/t/001_basic.pl
new file mode 100644
index 00..1fa2e12db2
--- /dev/null
+++ b/src/bin/pg_verify_checksums/t/001_basic.pl
@@ -0,0 +1,8 @@
+use strict;
+use warnings;
+use TestLib;
+use Test::More tests => 8;
+
+program_help_ok('pg_verify_checksums');
+program_version_ok('pg_verify_checksums');
+program_options_handling_ok('pg_verify_checksums');
diff --git a/src/bin/pg_verify_checksums/t/002_actions.pl b/src/bin/pg_verify_checksums/t/002_actions.pl
new file mode 100644
index 00..20d19f0291
--- /dev/null
+++ b/src/bin/pg_verify_checksums/t/002_actions.pl
@@ -0,0 +1,42 @@
+# Do basic sanity checks supported by pg_verify_checksums using
+# an initialized cluster.
+
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 9;
+
+# Initialize node with checksums disabled.
+my $node_no_checksum = get_new_node('node_no_checksum');
+$node_no_checksum->init;
+$node_no_checksum->start;
+my $pgdata = $node_no_checksum->data_dir;
+
+# Error, server must be shut down cleanly.
+command_fails(['pg_verify_checksums', '-d', $pgdata],
+			  "pg_verify_checksums requires server to be cleanly stopped");
+$node_no_checksum->stop;
+
+# Control file should know that checksums are enabled.
+command_like(['pg_controldata', $pgdata],
+	 qr/Data page checksum version:.*0/,
+	 'checksums are disabled in control file');
+
+# Run verification, which should fail as checksums are disabled.
+command_fails(['pg_verify_checksums', '-d', $pgdata],
+			  "checksums disabled so no verification");
+
+# Initialize node with checksums enabled.
+my $node_checksum = get_new_node('node_checksum');
+$node_checksum->init(extra => ['--data-checksums']);
+$pgdata = $node_checksum->data_dir;
+
+# Control file should know that checksums are disabled.
+command_like(['pg_controldata', $pgdata],
+	 qr/Data page checksum version:.*1/,
+		 'checksums are enabled in control file');
+
+# This time verification is able to work.
+command_ok(['pg_verify_checksums',  '-d', $pgdata],
+	   "checksums enabled so verification happens");


signature.asc
Description: PGP signature

Re: pg_verify_checksums and -fno-strict-aliasing

[Magnus Hagander]

On Thu, Aug 30, 2018 at 9:35 PM, Magnus Hagander 
wrote:

> On Thu, Aug 30, 2018 at 4:39 PM, Tom Lane  wrote:
>
>> Fabien COELHO  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:
>>
>> charbuf[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/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/src/bin/pg_verify_checksums/Makefile b/src/bin/pg_verify_checksums/Makefile
index d16261571f..cfe4ab1b8b 100644
--- a/src/bin/pg_verify_checksums/Makefile
+++ b/src/bin/pg_verify_checksums/Makefile
@@ -34,3 +34,9 @@ uninstall:
 clean distclean maintainer-clean:
 	rm -f pg_verify_checksums$(X) $(OBJS)
 	rm -rf tmp_check
+
+check:
+	$(prove_check)
+
+installcheck:
+	$(prove_installcheck)
diff --git a/src/bin/pg_verify_checksums/t/010_pg_verify_checksums.pl b/src/bin/pg_verify_checksums/t/010_pg_verify_checksums.pl
new file mode 100644
index 00..b6d535d76b
--- /dev/null
+++ b/src/bin/pg_verify_checksums/t/010_pg_verify_checksums.pl
@@ -0,0 +1,33 @@
+use strict;
+use warnings;
+use Cwd;
+use Config;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 10;
+
+program_help_ok('pg_verify_checksums');
+program_version_ok('pg_verify_checksums');
+program_options_handling_ok('pg_verify_checksums');
+
+my $node = get_new_node('main');
+
+# Set umask so test directories and files are created with default permissions
+umask(0077);
+
+# Initialize node without replication settings
+$node->init(extra => ['--data-checksums']);
+
+# Verify checksums of fresh node
+$node->command_ok(['pg_verify_checksums', '-D', $node->data_dir()]);
+
+# Cause some corruption in pg_proc
+open my $file, '+<', $node->data_dir() . "/base/1/1255";
+seek($file, 131, 0);
+syswrite($file, '\0\0\0\0\0\0\0\0\0');
+seek($file, 15721, 0);
+syswrite($file, '\0\0\0\0\0\0\0\0\0');
+close $file;
+
+# Verify that checksums are now broken
+$node->command_fails(['pg_verify_checksums', '-D', $node->data_dir()]);

Re: pg_verify_checksums and -fno-strict-aliasing

[Magnus Hagander]

On Thu, Aug 30, 2018 at 4:39 PM, Tom Lane  wrote:

> Fabien COELHO  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:
>
> charbuf[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/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index a941236563..965ec70557 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;
@@ -127,6 +127,8 @@ scan_file(char *fn, int segmentno)
 _("%s: checksums verified in file \"%s\"\n"), progname, fn);
 
 	close(f);
+
+	pfree(buf);
 }
 
 static void

Re: pg_verify_checksums and -fno-strict-aliasing

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: pg_verify_checksums and -fno-strict-aliasing

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pg_verify_checksums and -fno-strict-aliasing

[Tom Lane]

Fabien COELHO  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:

charbuf[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

Re: pg_verify_checksums and -fno-strict-aliasing

[Fabien COELHO]

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.

pg_verify_checksums and -fno-strict-aliasing

[Michael Banck]

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.ba...@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