Re: new heapcheck contrib module

> On Feb 4, 2021, at 1:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Feb 4, 2021 at 11:10 AM Mark Dilger
> <mark(dot)dilger(at)enterprisedb(dot)com> wrote:
>> I also made changes to clean up 0003 (formerly numbered 0004)
>
> "deduplice" is a typo.

Fixed.

> I'm not sure that I agree with check_each_database()'s commentary
> about why it doesn't make sense to optimize the resolve-the-databases
> step. Like, suppose I type 'pg_amcheck sasquatch'. I think the way you
> have it coded it's going to tell me that there are no databases to
> check, which might make me think I used the wrong syntax or something.
> I want it to tell me that sasquatch does not exist. If I happen to be
> a cryptid believer, I may reject that explanation as inaccurate, but
> at least there's no question about what pg_amcheck thinks the problem
> is.

The way v38 is coded, 'pg_amcheck sasquatch" will return a non-zero error code with an error message, database "sasquatch" does not exist.

The problem only comes up if you run it like one of the following:

pg_amcheck --maintenance-db postgres sasquatch
pg_amcheck postgres sasquatch
pg_amcheck "sasquatch.myschema.mytable"

In each of those, pg_amcheck first connects to the initial database ("postgres" or whatever) and tries to resolve all databases to check matching patterns like '^(postgres)$' and '^(sasquatch)$' and doesn't find any sasquatch matches, but also doesn't complain.

In v39, this is changed to complain when patterns do not match. This can be turned off with --no-strict-names.

> Why does check_each_database() go out of its way to run the main query
> without the always-secure search path? If there's a good reason, I
> think it deserves a comment saying what the reason is. If there's not
> a good reason, then I think it should use the always-secure search
> path for 100% of everything. Same question applies to
> check_one_database().

That bit of code survived some refactoring, but it doesn't make sense to keep it, assuming it ever made sense at all. Removed in v39. The calls to connectDatabase will always secure the search_path, so pg_amcheck need not touch that directly.

> ParallelSlotSetHandler(free_slot, VerifyHeapamSlotHandler, sql.data)
> could stand to be split over two lines, like you do for the nearly
> run_command() call, so that it doesn't go past 80 columns.

Fair enough. The code has been treated to a pass through pgindent as well.

> I suggest having two variables instead of one for amcheck_schema.
> Using the same variable to store the unescaped value and then later
> the escaped value is, IMHO, confusing. Whatever you call the escaped
> version, I'd rename the function parameters elsewhere to match.

The escaped version is now part of a struct, so there shouldn't be any confusion about this.

> "status = PQsendQuery(conn, sql) == 1" seems a bit uptight to me. Why
> not just make status an int and then just "status = PQsendQuery(conn,
> sql)" and then test for status != 0? I don't really care if you don't
> change this, it's not actually important. But personally I'd rather
> code it as if any non-zero value meant success.

I couldn't remember why I coded it like that, since it doesn't look like my style, then noticed I copied that from reindexdb.c, upon which this code is patterned. I agree it looks strange, and I've changed it in v39. Unlike the call site in reindexdb, there isn't any reason for pg_amcheck to store the returned value in a variable, so in v39 it doesn't.

> I think the pg_log_error() in run_command() could be worded a bit
> better. I don't think it's a good idea to try to include the type of
> object in there like this, because of the translatability guidelines
> around assembling messages from fragments. And I don't think it's good
> to say that the check failed because the reality is that we weren't
> able to ask for the check to be run in the first place. I would rather
> log this as something like "unable to send query: %s". I would also
> assume we need to bail out entirely if that happens. I'm not totally
> sure what sorts of things can make PQsendQuery() fail but I bet it
> boils down to having lost the server connection. Should that occur,
> trying to send queries for all of the remaining objects is going to
> result in repeating the same error many times, which isn't going to be
> what anybody wants. It's unclear to me whether we should give up on
> the whole operation but I think we have to at least give up on that
> connection... unless I'm confused about what the failure mode is
> likely to be here.

Changed in v39 to report the error as you suggest.

It will reconnect and retry a command one time on error. That should cover the case that the connection to the database was merely lost. If the second attempt also fails, no further retry of the same command is attempted, though commands for remaining relation targets will still be attempted, both for the database that had the error and for other remaining databases in the list.

Assuming something is wrong with "db2", the command `pg_amcheck db1 db2 db3` could result in two failures per relation in db2 before finally moving on to db3. That seems pretty awful considering how many relations that could be, but failing to soldier on in the face of errors seems a strange design for a corruption checking tool.

> It looks to me like the user won't be able to tell by the exit code
> what happened. What I did with pg_verifybackup, and what I suggest we
> do here, is exit(1) if anything went wrong, either in terms of failing
> to execute queries or in terms of those queries returning problem
> reports. With pg_verifybackup, I thought about trying to make it like
> 0 => backup OK, 2 => backup not OK, 2 => trouble, but I found it too
> hard to distinguish what should be exit(1) and what should be exit(2)
> and the coding wasn't trivial either, so I went with the simpler
> scheme.

In v39, exit(1) is used for all errors which are intended to stop the program. It is important to recognize that finding corruption is not an error in this sense. A query to verify_heapam() can fail if the relation's checksums are bad, and that happens beyond verify_heapam()'s control when the page is not allowed into the buffers. There can be errors if the file backing a relation is missing. There may be other corruption error cases that I have not yet thought about. The connections' errors get reported to the user, but pg_amcheck does not exit as a consequence of them. As discussed above, failing to send the query to the server is not viewed as a reason to exit, either. It would be hard to quantify all the failure modes, but presumably the catalogs for a database could be messed up enough to cause such failures, and I'm not sure that pg_amcheck should just abort.

>
> The opening line of appendDatabaseSelect() could be adjusted to put
> the regexps parameter on the next line, avoiding awkward wrapping.
>
> If they are being run with a safe search path, the queries in
> appendDatabaseSelect(), appendSchemaSelect(), etc. could be run
> without all the paranoia. If not, maybe they should be. The casts to
> text don't include the paranoia: with an unsafe search path, we need
> pg_catalog.text here. Or no cast at all, which seems like it ought to
> be fine too. Not quite sure why you are doing all that casting to
> text; the datatype is presumably 'name' and ought to collate like
> collate "C" which is probably fine.

In v39, everything is being run with a safe search path, and the paranoia and casts are largely gone.

> It would probably be a better idea for appendSchemaSelect to declare a
> PQExpBuffer and call initPQExpBuffer just once, and then
> resetPQExpBuffer after each use, and finally termPQExpBuffer just
> once. The way you have it is not expensive enough to really matter,
> but avoiding repeated allocate/free cycles is probably best.

I'm not sure what this comment refers to, but this function doesn't exist in v39.

> I wonder if a pattern like .foo.bar ends up meaning the same thing as
> a pattern like foo.bar, with the empty database name being treated the
> same as if nothing were specified.

That's really a question of how patternToSQLRegex parses that string. In general, "a.b.c" => ("^(a)$", "^(b)$", "^(c)$"), so I would expect your example to have a database pattern "^()$" which should only match databases with zero length names, presumably none. I've added a regression test for this, and indeed that's what it does.

> From the way appendTableCTE() is coded, it seems to me that if I ask
> for tables named j* excluding tables named jam* I still might get
> toast tables for my jam, which seems wrong.

In v39, the query is entirely reworked, so I can't respond directly to this, though I agree that excluding a table should mean the toast table does not automatically get included. There is an interaction, though, if you select both "j*' and "pg_toast.*" and then exclude "jam".

> There does not seem to be any clear benefit to defining CT_TABLE = 0
> in this case, so I would let the compiler deal with it. We should not
> be depending on that to have any particular numeric value.

The enum is removed in v39.

> Why does pg_amcheck.c have a header file pg_amcheck.h if there's only
> one source file? If you had multiple source files then the header
> would be a reasonable place to put stuff they all need, but you don't.

Everything is in pg_amcheck.c now.

> Copying the definitions of HEAP_TABLE_AM_OID and BTREE_AM_OID into
> pg_amcheck.h or anywhere else seems bad. I think you just be doing
> #include "catalog/pg_am_d.h".

Good point. Done.

> I think I'm out of steam for today but I'll try to look at this more
> soon. In general I think this patch and the whole series are pretty
> close to being ready to commit, even though there are still things I
> think need fixing here and there.

Reworking the code took a while. Version 39 patches attached.