Horton

Re: pg_amcheck contrib application

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Peter Geoghegan <pg@bowt.ie>
Cc: Mark Dilger <mark.dilger@enterprisedb.com>, Thomas Munro <thomas.munro@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, "Andrey M. Borodin" <x4mmm@yandex-team.ru>, Stephen Frost <sfrost@snowman.net>, Michael Paquier <michael@paquier.xyz>, Amul Sul <sulamul@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Andres Freund <andres@anarazel.de>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2021-03-12T21:43:05Z
Lists: pgsql-hackers 
On Fri, Mar 12, 2021 at 2:31 PM Robert Haas <robertmhaas@gmail.com> wrote:
> I'll commit something shortly to address these.

There are some interesting failures in the test cases on the
buildfarm. One of the tests ($offnum == 13) corrupts the TOAST pointer
with a garbage value, expecting to get the message "final toast chunk
number 0 differs from expected value 6". But on florican and maybe
other systems we instead get "final toast chunk number 0 differs from
expected value 5". That's because the value of TOAST_MAX_CHUNK_SIZE
depends on MAXIMUM_ALIGNOF. I think that on 4-byte alignment systems
it works out to 2000 and on 8-byte alignment systems it works out to
1996, and the value being stored is 10000 bytes, hence the problem.
The place where the calculation goes different seems to be in
MaximumBytesPerTuple(), where it uses MAXALIGN_DOWN() on a value that,
according to my calculations, will be 2038 on all platforms, but the
output of MAXALIGN_DOWN() will be 2032 or 2036 depending on the
platform. I think the solution to this is just to change the message
to match \d+ chunks instead of exactly 6. We should do that right away
to avoid having the buildfarm barf.

But, I also notice a couple of other things I think could be improved here:

1. amcheck is really reporting the complete absence of any TOAST rows
here due to a corrupted va_valueid. It could pick a better phrasing of
that message than "final toast chunk number 0 differs from expected
value XXX". I mean, there is no chunk 0. There are no chunks at all.

2. Using SSSSSSSSS as the perl unpack code for the varlena header is
not ideal, because it's really 2 1-byte fields followed by 4 4-byte
fields. So I think you should be using CCllLL, for two unsigned bytes
and then two signed 4-byte quantities and then two unsigned 4-byte
quantities. I think if you did that you'd be overwriting the
va_valueid with the *same* garbage value on every platform, which
would be better than different ones. Perhaps when we improve the
message as suggested in (1) this will become a live issue, since we
might choose to say something like "no TOAST entries for value %u".

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Commits

  1. amcheck: Improve some confusing reports about TOAST problems.

  2. amcheck: Reword some messages and fix an alignment problem.

  3. amcheck: fix multiple problems with TOAST pointer validation

  4. amcheck: Remove duplicate XID/MXID bounds checks.

  5. amcheck: Fix verify_heapam's tuple visibility checking rules.

  6. nbtree VACUUM: Cope with buggy opclasses.

  7. Improve pg_amcheck's TAP test 003_check.pl.

  8. Fix a confusing amcheck corruption message.

  9. Doc: add note about how to run the pg_amcheck regression tests.

  10. In pg_amcheck tests, don't depend on perl's Q/q pack code.

  11. pg_amcheck: Keep trying to fix the tests.

  12. pg_amcheck: Try to fix still more test failures.

  13. Try to avoid apparent platform-dependency in IPC::Run

  14. Fix portability issues in pg_amcheck's 004_verify_heapam.pl.

  15. Try to fix compiler warnings.

  16. Add pg_amcheck, a CLI for contrib/amcheck.

  17. Refactor and generalize the ParallelSlot machinery.

  18. Remove old-style VACUUM FULL (which was known for a little while as