Horton

Re: pg_amcheck contrib application

Mark Dilger <mark.dilger@enterprisedb.com>

View thread View on postgr.es
From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Robert Haas <robertmhaas@gmail.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-03-29T17:45:04Z
Lists: pgsql-hackers

Attachments


> On Mar 24, 2021, at 1:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> Mark,
> 
> Here's a quick and very dirty sketch of what I think perhaps this
> logic could look like. This is pretty much untested and it might be
> buggy, but at least you can see whether we're thinking at all in the
> same direction.

Thanks!  The attached patch addresses your comments here and in your prior email.  In particular, this patch changes the tuple visibility logic to not check tuples for which the inserting transaction aborted or is still in progress, and to not check toast for tuples deleted in transactions older than our transaction snapshot's xmin.  A list of toasted attributes which are safe to check is compiled per main table page during the scan of the page, then checked after the buffer lock on the main page is released.

In the perhaps unusual case where verify_heapam() is called in a transaction which has also added tuples to the table being checked, this patch's visibility logic chooses not to check such tuples.  I'm on the fence about this choice, and am mostly following your lead.  I like that this decision maintains the invariant that we never check tuples which have not yet been committed.

The patch includes a bit of refactoring.  In the old code, heap_check() performed clog bounds checking on xmin and xmax prior to calling check_tuple_header_and_visibilty(), but I think that's not such a great choice.  If the tuple header is garbled to have random bytes in the xmin and xmax fields, and we can detect that situation because other tuple header fields are garbled in detectable ways, I'd rather get a report about the header being garbled than a report about the xmin or xmax being out of bounds.  In the new code, the tuple header is checked first, then the visibility is checked, then the tuple is checked against the current relation description, then the tuple attributes are checked.  I think the layout is easier to follow, too.

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