Horton

Re: pg_amcheck contrib application

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Mark Dilger <mark.dilger@enterprisedb.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Noah Misch <noah@leadboat.com>, Peter Geoghegan <pg@bowt.ie>, Thomas Munro <thomas.munro@gmail.com>, "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-04-30T18:56:45Z
Lists: pgsql-hackers

Attachments

On Fri, Apr 30, 2021 at 2:31 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> Just to be clear, I did not use your patch v1 as the starting point.

I thought that might be the case, but I was trying to understand what
you didn't like about my version, and comparing them seemed like a way
to figure that out.

> I took the code as committed to master as the starting point, used your corruption report verbiage changes and at least some of your variable naming choices, but did not use the rest, in large part because it didn't work.  It caused corruption messages to be reported against tables that have no corruption.  For that matter, your v2 patch doesn't work either, and in the same way.  To wit:
>
>   heap table "postgres"."pg_catalog"."pg_rewrite", block 6, offset 4, attribute 7:
>      toast value 13461 chunk 0 has size 1995, but expected size 1996
>
> I think there is something wrong with the way you are trying to calculate and use extsize, because I'm not corrupting pg_catalog.pg_rewrite.  You can get these same results by applying your patch to master, building, and running 'make check' from src/bin/pg_amcheck/

Argh, OK, I didn't realize. Should be fixed in this version.

> > 4. You don't return if chunk_seq > last_chunk_seq. That seems wrong,
> > because we cannot compute a sensible expected size in that case. I
> > think your code will subtract a larger value from a smaller one and,
> > this being unsigned arithmetic, say that the expected chunk size is
> > something gigantic.
>
> Your conclusion is probably right, but I think your analysis is based on a misreading of what "last_chunk_seq" means.  It's not the last one seen, but the last one expected.  (Should we rename the variable to avoid confusion?)  It won't compute a gigantic size.  Rather, it will expect *every* chunk with chunk_seq >= last_chunk_seq to have whatever size is appropriate for the last chunk.

I realize it's the last one expected. That's the point: we don't have
any expectation for the sizes of chunks higher than the last one we
expected to see. If the value is 2000 bytes and the chunk size is 1996
bytes, we expect chunk 0 to be 1996 bytes and chunk 1 to be 4 bytes.
If not, we can complain. But it makes no sense to complain about chunk
2 being of a size we don't expect. We don't expect it to exist in the
first place, so we have no notion of what size it ought to be.

> If we have seen any chunks, the variable is holding the expected next chunk seq, which is one greater than the last chunk seq we saw.
>
> If we expect chunks 0..3 and see chunk 0 but not chunk 1, it will complain ..."expected to end at chunk 4, but ended at chunk 1".  This is clearly by design and not merely a bug, though I tend to agree with you that this is a strange wording choice.  I can't remember exactly when and how we decided to word the message this way, but it has annoyed me for a while, and I assumed it was something you suggested a while back, because I don't recall doing it.  Either way, since you seem to also be bothered by this, I agree we should change it.

Can you review this version?

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