Horton

Thread

Commits

  1. Add an explicit test to catch changes in checksumming calculations.

  1. More tests to stress directly checksum_impl.h

    Michael Paquier <michael@paquier.xyz> — 2020-03-06T07:52:30Z

    Hi all,

As of the thread which led to addd034 (please see
https://www.postgresql.org/message-id/E1j9ioh-0005Kn-4O@gemulon.postgresql.org,
and sorry about that), it happens that we don't have any tests which
validate the internal data checksum implementation present in core as
of checksum_impl.h.  pageinspect includes a SQL-callable function to
calculate the checksum of a page, mentioned by David in CC, and only
one test exists to make sure that a checksum is not NULL, but it does
not really help if the formula is touched.

Attached is a patch to close the gap by adding new tests to
pageinspect aimed at detecting any formula change.  The trick is to
make the page data representative enough so as it is possible to
detect problems if any part of the formulas are changed, like updates
of pg_checksum_block or checksumBaseOffsets.

Any thoughts or other ideas?
Thanks,
--
Michael

  2. Re: More tests to stress directly checksum_impl.h

    David Steele <david@pgmasters.net> — 2020-03-06T14:48:50Z

    On 3/6/20 2:52 AM, Michael Paquier wrote:
> 
> As of the thread which led to addd034 (please see
> https://www.postgresql.org/message-id/E1j9ioh-0005Kn-4O@gemulon.postgresql.org,
> and sorry about that), it happens that we don't have any tests which
> validate the internal data checksum implementation present in core as
> of checksum_impl.h.  pageinspect includes a SQL-callable function to
> calculate the checksum of a page, mentioned by David in CC, and only
> one test exists to make sure that a checksum is not NULL, but it does
> not really help if the formula is touched.
> 
> Attached is a patch to close the gap by adding new tests to
> pageinspect aimed at detecting any formula change.  The trick is to
> make the page data representative enough so as it is possible to
> detect problems if any part of the formulas are changed, like updates
> of pg_checksum_block or checksumBaseOffsets.
> 
> Any thoughts or other ideas?

This looks sensible to me.  The only downside is that it needs to be in 
a contrib test rather than in the core tests, but it is far better than 
nothing.

I'll be interested to see what the build farm thinks of it.  Since we 
treat the page as an array of uint32_t while checksumming it seems that 
endianness will be a factor in the checksum.  My guess is that the first 
three tests (01, 04, FF) will work on any endianness and the last three 
tests will not.

regards,
-- 
-David
david@pgmasters.net

  3. Re: More tests to stress directly checksum_impl.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-06T20:04:27Z

    Michael Paquier <michael@paquier.xyz> writes:
> Attached is a patch to close the gap by adding new tests to
> pageinspect aimed at detecting any formula change.  The trick is to
> make the page data representative enough so as it is possible to
> detect problems if any part of the formulas are changed, like updates
> of pg_checksum_block or checksumBaseOffsets.
> Any thoughts or other ideas?

I wonder whether big-endian machines will compute the same values.
A quick look at our checksum implementation makes it look like the
results will depend on the endianness.

Between that and the BLCKSZ dependency, it's not clear that we can
test this with just a plain old expected-file test case.  Might
need to fall back to a TAP test.

Another way would be variant output files, which could be a sane
solution if we put this in its own test script.

			regards, tom lane

  4. Re: More tests to stress directly checksum_impl.h

    Michael Paquier <michael@paquier.xyz> — 2020-03-07T05:06:30Z

    On Fri, Mar 06, 2020 at 03:04:27PM -0500, Tom Lane wrote:
> Between that and the BLCKSZ dependency, it's not clear that we can
> test this with just a plain old expected-file test case.  Might
> need to fall back to a TAP test.

Perhaps the dependency of page.sql on 8kB pages could be improved,
still I am not sure either that testing checksums is worth the
complexity of a new TAP test dependent on pageinspect (5a9323e has
removed such a dependency recently for example).

> Another way would be variant output files, which could be a sane
> solution if we put this in its own test script.

An extra option would be to just choose values which have the same
ordering as long as these are enough to break with changes in the
formula, as mentioned by David, and add a comment about this
assumption in the tests.  I am not sure either if this option has more
advantages than the others, but it has at least the merit to be the
simplest one.

(It is kind of hard to find a qemu image with big endian lately?)
--
Michael

  5. Re: More tests to stress directly checksum_impl.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-07T18:22:52Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Mar 06, 2020 at 03:04:27PM -0500, Tom Lane wrote:
>> Between that and the BLCKSZ dependency, it's not clear that we can
>> test this with just a plain old expected-file test case.  Might
>> need to fall back to a TAP test.

> Perhaps the dependency of page.sql on 8kB pages could be improved,
> still I am not sure either that testing checksums is worth the
> complexity of a new TAP test dependent on pageinspect (5a9323e has
> removed such a dependency recently for example).

Yeah, a TAP test is a mighty expensive solution.

>> Another way would be variant output files, which could be a sane
>> solution if we put this in its own test script.

I think this way could work; see attached.

I'm not sure if it's actually worth providing the variants for non-8K
block sizes.  While running the tests to construct those, I was reminded
that not only do several of the other pageinspect tests "fail" at
nondefault block sizes, but so do the core regression tests and some
other tests as well.  We are a long way from having check-world pass
with nondefault block sizes, so maybe this test doesn't need to either.
However, there's something to be said for memorializing the behavior
we expect.

> (It is kind of hard to find a qemu image with big endian lately?)

The boneyard over on my other desk has actual hardware ;-)

			regards, tom lane

  6. Re: More tests to stress directly checksum_impl.h

    David Steele <david@pgmasters.net> — 2020-03-07T18:46:43Z

    On 3/7/20 1:22 PM, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> 
>>> Another way would be variant output files, which could be a sane
>>> solution if we put this in its own test script.
> 
> I think this way could work; see attached.
> 
> I'm not sure if it's actually worth providing the variants for non-8K
> block sizes.  While running the tests to construct those, I was reminded
> that not only do several of the other pageinspect tests "fail" at
> nondefault block sizes, but so do the core regression tests and some
> other tests as well.  We are a long way from having check-world pass
> with nondefault block sizes, so maybe this test doesn't need to either.
> However, there's something to be said for memorializing the behavior
> we expect.

Nice! Looks like I was wrong about the checksums being the same on le/be 
systems for repeated byte values. On closer inspection it looks like >> 
17 at least ensures this will not be true.

Good to know.

Thanks,
-- 
-David
david@pgmasters.net

  7. Re: More tests to stress directly checksum_impl.h

    Michael Paquier <michael@paquier.xyz> — 2020-03-08T03:15:14Z

    On Sat, Mar 07, 2020 at 01:46:43PM -0500, David Steele wrote:
> Nice! Looks like I was wrong about the checksums being the same on le/be
> systems for repeated byte values. On closer inspection it looks like >> 17
> at least ensures this will not be true.

Thanks for the computations with big-endian!  I would have just gone
down to the 8kB page for the expected results by seeing three other
tests blowing up, but no objection to what you have here either.  I
have checked the computations with little-endian from your patch and
these are correct.
--
Michael

  8. Re: More tests to stress directly checksum_impl.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-08T19:12:11Z

    Michael Paquier <michael@paquier.xyz> writes:
> Thanks for the computations with big-endian!  I would have just gone
> down to the 8kB page for the expected results by seeing three other
> tests blowing up, but no objection to what you have here either.  I
> have checked the computations with little-endian from your patch and
> these are correct.

After thinking more I concluded that the extra expected files would
just be a waste of tarball space, at least till such time as we make
a push to fix all the regression tests to be blocksize-independent.

Pushed it with just the 8K files.

			regards, tom lane

  9. Re: More tests to stress directly checksum_impl.h

    Michael Paquier <michael@paquier.xyz> — 2020-03-09T01:03:15Z

    On Sun, Mar 08, 2020 at 03:12:11PM -0400, Tom Lane wrote:
> After thinking more I concluded that the extra expected files would
> just be a waste of tarball space, at least till such time as we make
> a push to fix all the regression tests to be blocksize-independent.

Makes sense.

> Pushed it with just the 8K files.

Thanks!
--
Michael