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: Peter Geoghegan <pg@bowt.ie>, 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-08T16:26:08Z
Lists: pgsql-hackers 
On Thu, Mar 4, 2021 at 5:39 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> I think Robert mistook why I was doing that.  I was thinking about a different usage pattern.  If somebody thinks a subset of relations have been badly corrupted, but doesn't know which relations those might be, they might try to find them with pg_amcheck, but wanting to just check the first few blocks per relation in order to sample the relations.  So,
>
>   pg_amcheck --startblock=0 --endblock=9 --no-dependent-indexes
>
> or something like that.  I don't think it's very fun to have it error out for each relation that doesn't have at least ten blocks, nor is it fun to have those relations skipped by error'ing out before checking any blocks, as they might be the corrupt relations you are looking for.  But using --startblock and --endblock for this is not a natural fit, as evidenced by how I was trying to "fix things up" for the user, so I'll punt on this usage until some future version, when I might add a sampling option.

I admit I hadn't thought of that use case. I guess somebody could want
to do that, but it doesn't seem all that useful. Checking the first
up-to-ten blocks of every relation is not a very representative
sample, and it's not clear to me that sampling is a good idea even if
it were representative. What good is it to know that 10% of my
database is probably not corrupted?

On the other hand, people want to do all kinds of things that seem
strange to me, and this might be another one. But, if that's so, then
I think the right place to implement it is in amcheck itself, not
pg_amcheck. I think pg_amcheck should be, now and in the future, a
thin wrapper around the functionality provided by amcheck, just
providing target selection and parallel execution. If you put
something into pg_amcheck that figures out how long the relation is
and runs it on some of the blocks, that functionality is only
accessible to people who are accessing amcheck via pg_amcheck. If you
put it in amcheck itself and just expose it through pg_amcheck, then
it's accessible either way. It's probably cleaner and more performant
to do it that way, too.

So if you did add a sampling option in the future, that's the way I
would recommend doing it, but I think it is probably best not to go
there right now.

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