Re: pg_amcheck contrib application

Most of these changes sound good. I'll go through the whole patch
again today, or as much of it as I can. But before I do that, I want
to comment on this point specifically.

On Thu, Mar 4, 2021 at 1:25 AM Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> I think this is fixed up now. There is an interaction with amcheck's verify_heapam(), where that function raises an error if the startblock or endblock arguments are out of bounds for the relation in question. Rather than aborting the entire pg_amcheck run, it avoids passing inappropriate block ranges to verify_heapam() and outputs a warning, so:
>
> % pg_amcheck mark.dilger -t foo -t pg_class --progress -v --startblock=35 --endblock=77
> pg_amcheck: in database "mark.dilger": using amcheck version "1.3" in schema "public"
> 0/6 relations (0%) 0/55 pages (0%)
> pg_amcheck: checking table "mark.dilger"."public"."foo" (oid 16385) (10/45 pages)
> pg_amcheck: warning: ignoring endblock option 77 beyond end of table "mark.dilger"."public"."foo"
> pg_amcheck: checking btree index "mark.dilger"."public"."foo_idx" (oid 16388) (30/30 pages)
> pg_amcheck: checking table "mark.dilger"."pg_catalog"."pg_class" (oid 1259) (0/13 pages)
> pg_amcheck: warning: ignoring startblock option 35 beyond end of table "mark.dilger"."pg_catalog"."pg_class"
> pg_amcheck: warning: ignoring endblock option 77 beyond end of table "mark.dilger"."pg_catalog"."pg_class"
> pg_amcheck: checking btree index "mark.dilger"."pg_catalog"."pg_class_relname_nsp_index" (oid 2663) (6/6 pages)
> pg_amcheck: checking btree index "mark.dilger"."pg_catalog"."pg_class_tblspc_relfilenode_index" (oid 3455) (5/5 pages)
> pg_amcheck: checking btree index "mark.dilger"."pg_catalog"."pg_class_oid_index" (oid 2662) (4/4 pages)
> 6/6 relations (100%) 55/55 pages (100%)
>
> The way the (x/y pages) is printed takes into account that the [startblock..endblock] range may reduce the number of pages to check (x) to something less than the number of pages in the relation (y), but the reporting is a bit of a lie when the startblock is beyond the end of the table, as it doesn't get passed to verify_heapam and so the number of blocks checked may be more than the zero blocks reported. I think I might need to fix this up tomorrow, but I want to get what I have in this patch set posted tonight, so it's not fixed here. Also, there are multiple ways of addressing this, and I'm having trouble deciding which way is best. I can exclude the relation from being checked at all, or realize earlier that I'm not going to honor the startblock argument and compute the blocks to check correctly. Thoughts?

I think this whole approach is pretty suspect because the number of
blocks in the relation can increase (by relation extension) or
decrease (by VACUUM or TRUNCATE) between the time when we query for
the list of target relations and the time we get around to executing
any queries against them. I think it's OK to use the number of
relation pages for progress reporting because progress reporting is
only approximate anyway, but I wouldn't print them out in the progress
messages, and I wouldn't try to fix up the startblock and endblock
arguments on the basis of how long you think that relation is going to
be. You seem to view the fact that the server reported the error as
the reason for the problem, but I don't agree. I think having the
server report the error here is right, and the problem is that the
error reporting sucked because it was long-winded and didn't
necessarily tell you which table had the problem.

There are a LOT of things that can go wrong when we go try to run
verify_heapam on a table. The table might have been dropped; in fact,
on a busy production system, such cases are likely to occur routinely
if DDL is common, which for many users it is. The system catalog
entries might be screwed up, so that the relation can't be opened.
There might be an unreadable page in the relation, either because the
OS reports an I/O error or something like that, or because checksum
verification fails. There are various other possibilities. We
shouldn't view such errors as low-level things that occur only in
fringe cases; this is a corruption-checking tool, and we should expect
that running it against messed-up databases will be common. We
shouldn't try to interpret the errors we get or make any big decisions
about them, but we should have a clear way of reporting them so that
the user can decide what to do.

Just as an experiment, I suggest creating a database with 100 tables
in it, each with 1 index, and then deleting a single pg_attribute
entry for 10 of the tables, and then running pg_amcheck. I think you
will get 20 errors - one for each messed-up table and one for the
corresponding index. Maybe you'll get errors for the TOAST tables
checks too, if the tables have TOAST tables, although that seems like
it should be avoidable. Now, now matter what you do, the tool is going
to produce a lot of output here, because you have a lot of problems,
and that's OK. But how understandable is that output, and how concise
is it? If it says something like:

pg_amcheck: could not check "SCHEMA_NAME"."TABLE_NAME": ERROR: some
attributes are missing or something

...and that line is repeated 20 times, maybe with a context or detail
line for each one or something like that, then you have got a good UI.
If it's not clear which tables have the problem, you have got a bad
UI. If it dumps out 300 lines of output instead of 20 or 40, you have
a UI that is so verbose that usability is going to be somewhat
impaired, which is why I suggested only showing the query in verbose
mode.

BTW, another thing that might be interesting is to call
PQsetErrorVerbosity(conn, PQERRORS_VERBOSE) in verbose mode. It's
probably possible to contrive a case where the server error message is
something generic like "cache lookup failed for relation %u" which
occurs in a whole bunch of places in the source code, and being able
get the file and line number information can be really useful when
trying to track such things down.

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