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-23T17:28:50Z
Lists: pgsql-hackers

Attachments

On Thu, Apr 22, 2021 at 7:28 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> I have refactored the patch to address your other concerns.  Breaking the patch into multiple pieces didn't add any clarity, but refactoring portions of it made things simpler to read, I think, so here it is as one patch file.

I was hoping that this version was going to be smaller than the last
version, but instead it went from 300+ lines to 500+ lines.

The main thing I'm unhappy about in the status quo is the use of
chunkno in error messages. I have suggested several times making that
concept go away, because I think users will be confused. Here's a
minimal patch that does just that. It's 32 lines and results in a net
removal of 4 lines. It differs somewhat from my earlier suggestions,
because my priority here is to get reasonably understandable output
without needing a ton of code, and as I was working on this I found
that some of my earlier suggestions would have needed more code to
implement and I didn't think it bought enough to be worth it. It's
possible this is too simple, or that it's buggy, so let me know what
you think. But basically, I think what got committed before is
actually mostly fine and doesn't need major revision. It just needs
tidying up to avoid the confusing chunkno concept.

Now, the other thing we've talked about is adding a few more checks,
to verify for example that the toastrelid is what we expect, and I see
in your v22 you thought of a few other things. I think we can consider
those, possibly as things where we consider it tidying up loose ends
for v14, or else as improvements for v15. But I don't think that the
fairly large size of your patch comes primarily from additional
checks. I think it mostly comes from the code to produce error reports
getting a lot more complicated. I apologize if my comments have driven
that complexity, but they weren't intended to.

One tiny problem with the attached patch is that it does not make any
regression tests fail, which also makes it hard for me to tell if it
breaks anything, or if the existing code works. I don't know how
practical it is to do anything about that. Do you have a patch handy
that allows manual updates and deletes on TOAST tables, for manual
testing purposes?

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