Re: pg_amcheck contrib application

> On Mar 3, 2021, at 9:15 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Wed, Mar 3, 2021 at 10:22 AM Mark Dilger
> <mark(dot)dilger(at)enterprisedb(dot)com> wrote:
>>> Your four messages about there being nothing to check seem like they
>>> could be consolidated down to one: "nothing to check for pattern
>>> \"%s\"".
>>
>> I anticipated your review comment, but I'm worried about the case that somebody runs
>>
>> pg_amcheck -t "foo" -i "foo"
>>
>> and one of those matches and the other does not. The message 'nothing to check for pattern "foo"' will be wrong (because there was something to check for it) and unhelpful (because it doesn't say which failed to match.)
>
> Fair point.
>
>> Changed, though I assumed your parens for corruption() were not intended.
>
> Uh, yeah.
>
>> Thanks for the review!
>
> + fprintf(stderr, "%s: no relations to check", progname);
>
> Missing newline.
>
> Generally, I would favor using pg_log_whatever as a way of reporting
> messages starting when option parsing is complete. In other words,
> starting here:
>
> + fprintf(stderr, "%s: no databases to check\n", progname);
>
> I see no real advantage in having a bunch of these using
> fprintf(stderr, ...), which to me seems most appropriate only for very
> early failures.

Ok, the newline issues should be fixed, and the use of pg_log_{error,warning,info} is now used more consistently.

> Perhaps amcheck_sql could be spread across fewer lines, now that it
> doesn't have so many decorations?

Done.

> pg_basebackup uses -P as a short form for --progress, so maybe we
> should match that here.

Done.

> When I do "pg_amcheck --progress", it just says "259/259 (100%)" which
> I don't find too clear. The corresponding pg_basebackup output is
> "32332/32332 kB (100%), 1/1 tablespace" which has the advantage of
> including units. I think if you just add the word "relations" to your
> message it will be nicer.

Done. It now shows:

% pg_amcheck -P
259/259 relations (100%) 870/870 pages (100%)

As you go along, the percent of relations processed may not be equal to the percent of pages, though at the end they are both 100%. The value of printing both can only be seen while things are underway.

> When I do "pg_amcheck -s public" it tells me that there are no
> relations to check in schemas for "public". I think "schemas matching"
> would read better than "schemas for." Similar with the other messages.
> When I try "pg_amcheck -t nessie" it tells me that there are no tables
> to check for "nessie" but saying that there are no tables to check
> matching "nessie" to me sounds more natural.

Done.

% pg_amcheck -s public
pg_amcheck: error: no relations to check in schemas matching "public"

> The code doesn't seem real clear on the difference between a database
> name and a pattern. Consider:
>
> createdb rhaas
> createdb 'rh*s'
> PGDATABASE='rh*s' pg_amcheck
>
> It checks the rhaas database, which I venture to say is just plain wrong.

This next version treats any arguments supplied with -d and -D as database patterns, and all others as database names. Exclusion patterns (-D) only override inclusion patterns, not names.

> The error message when I exclude the only checkable database is not
> very clear. "pg_amcheck -D rhaas" says pg_amcheck: no checkable
> database: "rhaas". Well, I get that there's no checkable database. But
> as a user I have no idea what "rhaas" is. I can even get it to issue
> this complaint more than once:
>
> createdb q
> createdb qq
> pg_amcheck -D 'q*' q qq
>
> Now it issues the "no checkable database" complaint twice, once for q
> and once for qq. But if there's no checkable database, I only need to
> know that once. Either the message is wrongly-worded, or it should
> only be issued once and doesn't need to include the pattern. I think
> it's the second one, but I could be wrong.

I think this whole problem goes away with the change to how -D/-d work and don't interact with database names. At least, I don't get any problems like the one you mention:

% PGDATABASE=postgres pg_amcheck -D postgres
pg_amcheck: warning: skipping database "postgres": amcheck is not installed
pg_amcheck: error: no relations to check

% PGDATABASE=mark.dilger pg_amcheck -D mark.dilger --progress
259/259 relations (100%) 870/870 pages (100%)

> Using a pattern as the only or first argument doesn't work; i.e.
> "pg_amcheck rhaas" works but "pg_amcheck rhaa?" fails because there is
> no database with that exact literal name. This seems like another
> instance of confusion between a literal database name and a database
> name pattern. I'm not quite sure what the right solution is here. We
> could give up on having database patterns altogether -- the comparable
> issue does not arise for database and schema name patterns -- or the
> maintenance database could default to something that's not going to be
> a pattern, like "postgres," rather than being taken from a
> command-line argument that is intended to be a pattern. Or some hybrid
> approach e.g. -d options are patterns, but don't set the maintenance
> database, while extra command line arguments are literal database
> names, and thus are presumably OK to use as the maintenance DB. But
> it's too weird IMHO to support patterns here and then have supplying
> one inevitably fail unless you also specify --maintenance-db.

Right. I think the changes in this next version address all your concerns as stated, but here are some examples:

% pg_amcheck "mark.d*" --progress
pg_amcheck: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: database "mark.d*" does not exist

% PGDATABASE=postgres pg_amcheck "mark.d*" --progress
pg_amcheck: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: database "mark.d*" does not exist

% PGDATABASE=postgres pg_amcheck -d "mark.d*" --progress
520/520 relations (100%) 1815/1815 pages (100%)

% pg_amcheck --all --maintenance-db="mark.d*" --progress
pg_amcheck: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: database "mark.d*" does not exist

% pg_amcheck --all -D="mark.d*" --progress
pg_amcheck: warning: skipping database "template1": amcheck is not installed
520/520 relations (100%) 1815/1815 pages (100%)

> It's sorta annoying that there doesn't seem to be an easy way to find
> out exactly what relations got checked as a result of whatever I did.
> Perhaps pg_amcheck -v should print a line for each relation saying
> that it's checking that relation; it's not actually that verbose as
> things stand. If we thought that was overdoing it, we could set things
> up so that multiple -v options keep increasing the verbosity level, so
> that you can get this via pg_amcheck -vv. I submit that pg_amcheck -e
> is not useful for this purpose because the queries, besides being
> long, use the relation OIDs rather than the names, so it's not easy to
> see what happened.

I added that, as shown here:

% pg_amcheck mark.dilger --table=pg_subscription --table=pg_publication -v
pg_amcheck: in database "mark.dilger": using amcheck version "1.3" in schema "public"
pg_amcheck: checking btree index "mark.dilger"."pg_toast"."pg_toast_6100_index" (oid 4184) (1/1 page)
pg_amcheck: checking btree index "mark.dilger"."pg_catalog"."pg_publication_oid_index" (oid 6110) (1/1 page)
pg_amcheck: checking btree index "mark.dilger"."pg_catalog"."pg_publication_pubname_index" (oid 6111) (1/1 page)
pg_amcheck: checking btree index "mark.dilger"."pg_catalog"."pg_subscription_oid_index" (oid 6114) (1/1 page)
pg_amcheck: checking btree index "mark.dilger"."pg_catalog"."pg_subscription_subname_index" (oid 6115) (1/1 page)
pg_amcheck: checking table "mark.dilger"."pg_toast"."pg_toast_6100" (oid 4183) (0/0 pages)
pg_amcheck: checking table "mark.dilger"."pg_catalog"."pg_subscription" (oid 6100) (0/0 pages)
pg_amcheck: checking table "mark.dilger"."pg_catalog"."pg_publication" (oid 6104) (0/0 pages)

> I think that something's not working in terms of schema exclusion. If
> I create a brand-new database and then run "pg_amcheck -S pg_catalog
> -S information_schema -S pg_toast" it still checks stuff. In fact it
> seems to check the exact same amount of stuff that it checks if I run
> it with no command-line options at all. In fact, if I run "pg_amcheck
> -S '*'" that still checks everything. Unless I'm misunderstanding what
> this option is supposed to do, the fact that a version of this patch
> where this seemingly doesn't work at all escaped to the list suggests
> that your testing has got some gaps.

Good catch. That works now, but beware that -S doesn't apply to excluding things brought in by toast or index expansion, so:

% pg_amcheck mark.dilger -S pg_catalog -S pg_toast --progress -v
pg_amcheck: in database "mark.dilger": using amcheck version "1.3" in schema "public"
0/14 relations (0%) 0/90 pages (0%)
pg_amcheck: checking table "mark.dilger"."public"."foo" (oid 16385) (45/45 pages)
pg_amcheck: checking btree index "mark.dilger"."public"."foo_idx" (oid 16388) (30/30 pages)
pg_amcheck: checking table "mark.dilger"."information_schema"."sql_features" (oid 13051) (8/8 pages)
pg_amcheck: checking btree index "mark.dilger"."pg_toast"."pg_toast_13051_index" (oid 13055) (1/1 page)
pg_amcheck: checking table "mark.dilger"."information_schema"."sql_implementation_info" (oid 13056) (1/1 page)
pg_amcheck: checking btree index "mark.dilger"."pg_toast"."pg_toast_13056_index" (oid 13060) (1/1 page)
pg_amcheck: checking table "mark.dilger"."information_schema"."sql_parts" (oid 13061) (1/1 page)
pg_amcheck: checking btree index "mark.dilger"."pg_toast"."pg_toast_13061_index" (oid 13065) (1/1 page)
pg_amcheck: checking table "mark.dilger"."information_schema"."sql_sizing" (oid 13066) (1/1 page)
pg_amcheck: checking btree index "mark.dilger"."pg_toast"."pg_toast_13066_index" (oid 13070) (1/1 page)
pg_amcheck: checking table "mark.dilger"."pg_toast"."pg_toast_13051" (oid 13054) (0/0 pages)
pg_amcheck: checking table "mark.dilger"."pg_toast"."pg_toast_13056" (oid 13059) (0/0 pages)
pg_amcheck: checking table "mark.dilger"."pg_toast"."pg_toast_13061" (oid 13064) (0/0 pages)
pg_amcheck: checking table "mark.dilger"."pg_toast"."pg_toast_13066" (oid 13069) (0/0 pages)
14/14 relations (100%) 90/90 pages (100%)

but

% pg_amcheck mark.dilger -S pg_catalog -S pg_toast -S information_schema --progress -v
pg_amcheck: in database "mark.dilger": using amcheck version "1.3" in schema "public"
0/2 relations (0%) 0/75 pages (0%)
pg_amcheck: checking table "mark.dilger"."public"."foo" (oid 16385) (45/45 pages)
pg_amcheck: checking btree index "mark.dilger"."public"."foo_idx" (oid 16388) (30/30 pages)
2/2 relations (100%) 75/75 pages (100%)

The first one checks so much because the toast and indexes for tables in the "information_schema" are not excluded by -S, but:

% pg_amcheck mark.dilger -S pg_catalog -S pg_toast --progress --no-dependent-indexes --no-dependent-toast -v
pg_amcheck: in database "mark.dilger": using amcheck version "1.3" in schema "public"
0/5 relations (0%) 0/56 pages (0%)
pg_amcheck: checking table "mark.dilger"."public"."foo" (oid 16385) (45/45 pages)
pg_amcheck: checking table "mark.dilger"."information_schema"."sql_features" (oid 13051) (8/8 pages)
pg_amcheck: checking table "mark.dilger"."information_schema"."sql_implementation_info" (oid 13056) (1/1 page)
pg_amcheck: checking table "mark.dilger"."information_schema"."sql_parts" (oid 13061) (1/1 page)
pg_amcheck: checking table "mark.dilger"."information_schema"."sql_sizing" (oid 13066) (1/1 page)
5/5 relations (100%) 56/56 pages (100%)

works as you might expect.

> I like the semantics of --no-toast-expansion and --no-index-expansion
> as you now have them, but I find I don't really like the names. Could
> I suggest --no-dependent-indexes and --no-dependent-toast?

Changed.

> I tried pg_amcheck --startblock=tsgsdg and got an error message
> without a trailing newline.

Fixed.

> I tried --startblock=-525523 and got no
> error.

Fixed.

> I tried --startblock=99999999999999999999999999 and got a
> complaint that the value was out of bounds, but without a trailing
> newline.

Fixed.

> Maybe there's an argument that the bounds don't need to be
> checked, but surely there's no argument for checking one and not the
> other.

It checks both now, and also for --endblock

> I haven't tried the corresponding cases with --endblock but you
> should. I tried --startblock=2 --endblock=1 and got a complaint that
> the ending block precedes the starting block, which is totally
> reasonable (though I might say "start block" and "end block" rather
> than using the -ing forms)

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?

> but this message is prefixed with
> "pg_amcheck: " whereas the messages about an altogether invalid
> starting block where not so prefixed. Is there a reason not to make
> this consistent?

That was a stray usage of pg_log_error where fprintf should have been used. Fixed.

> I also tried using a random positive integer for startblock, and for
> every relation I am told "ERROR: starting block number must be
> between 0 and <whatever>". That makes sense, because I used a big
> number for the start block and I don't have any big relations, but it
> makes for an absolute ton of output, because every verify_heapam query
> is 11 lines long.

This happens because the range was being passed down to verify_heapam. It won't do that now.

> This suggests a couple of possible improvements.
> First, maybe we should only display the query that produced the error
> in verbose mode.

No longer relevant.

> Second, maybe the verify_heapam() query should be
> tightened up so that it doesn't stretch across quite so many lines.

Not a bad idea, but no longer relevant to the startblock/endblock issues. Done.

> I
> think the call to verify_heapam() could be spread across like 2 lines
> rather than 7, which would improve readability.

Done.

> On a related note, I
> wonder why we need every verify_heapam() call to join to pg_class and
> pg_namespace just to fetch the schema and table name which,
> presumably, we should or at least could already have.

We didn't have it, but we do now, so that join is removed.

> This kinda
> relates to my comment earlier about making -v print a message per
> relation so that we can see, in human-readable format, which relations
> are getting checked.

Done.

> Right now, if you got an error checking just one
> relation, how would you know which relation you got it from? Unless
> the server happens to report that information in the message, you're
> just in the dark, because pg_amcheck won't tell you.

That information is now included in the query text, so you can see it in the error message along with the oid.

>
> The line "Read the description of the amcheck contrib module for
> details" seems like it could be omitted. Perhaps the first line of the
> help message could be changed to read "pg_amcheck uses amcheck to find
> corruption in a PostgreSQL database." or something like that, instead.

Done.