Re: new heapcheck contrib module - Mailing list pgsql-hackers

On Thu, Nov 19, 2020 at 12:06 PM Robert Haas <robertmhaas@gmail.com> wrote:
> I originally intended to review the docs and regression tests in the
> same email as the patch itself, but this email has gotten rather long
> and taken rather longer to get together than I had hoped, so I'm going
> to stop here for now and come back to that stuff.

Broad question: Does pg_amcheck belong in src/bin, or in contrib? You
have it in the latter place, but I'm not sure if that's the right
idea. I'm not saying it *isn't* the right idea, but I'm just wondering
what other people think.

Now, on to the docs:

+  Currently, this requires execute privileges on <xref linkend="amcheck"/>'s
+  <function>bt_index_parent_check</function> and
<function>verify_heapam</function>

This makes me wonder why there isn't an option to call
bt_index_check() rather than bt_index_parent_check().

It doesn't seem to be standard practice to include the entire output
of the command's --help option in the documentation. That means as
soon as anybody changes anything they've got to change the
documentation too. I don't see anything like that in the pages for
psql or vacuumlo or pg_verifybackup. It also doesn't seem like a
useful thing to do. Anyone who is reading the documentation probably
is in a position to try --help if they wish; they don't need that
duplicated here.

Looking at those other pages, what seems to be typical for an SGML is
to list all the options and give a short paragraph on what each one
does. What you have instead is a narrative description. I recommend
looking over the reference page for one of those other command-line
utilities and adapting it to this case.

Back to the the code:

+static const char *
+get_index_relkind_quals(void)
+{
+       if (!index_relkind_quals)
+               index_relkind_quals = psprintf("'%c'", RELKIND_INDEX);
+       return index_relkind_quals;
+}

I feel like there ought to be a way to work this out at compile time
rather than leaving it to runtime. I think that replacing the function
body with "return CppAsString2(RELKIND_INDEX);" would have the same
result, and once you do that you don't really need the function any
more. This is arguably cheating a bit: RELKIND_INDEX is defined as 'i'
and CppAsString2() turns that into a string containing those three
characters. That happens to work because what we want to do is quote
this for use in SQL, and SQL happens to use single quotes for literals
just like C does for individual characters. It would be mor elegant to
figure out a way to interpolate just the character into C string, but
I don't know of a macro trick that will do that. I think one could
write char *something = { '\'', RELKIND_INDEX, '\'', '\0' } but that
would be pretty darn awkward for the table case where you want an ANY
with three relkinds in there.

But maybe you could get around that by changing the query slightly.
Suppose instead of relkind = BLAH, you write POSITION(relkind IN '%s')
> 0. Then you could just have the caller pass either:

char *index_relkinds = { RELKIND_INDEX, '\0' };
-or-
char *table_relkinds = { RELKIND_RELATION, RELKIND_MATVIEW,
RELKIND_TOASTVALUE, '\0' };

The patch actually has RELKIND_PARTITIONED_TABLE there rather than
RELKIND_RELATION, but that seems wrong to me, because partitioned
tables don't have storage, and toast tables do. And if we're going to
include RELKIND_PARTITIONED_TABLE for some reason, then why not
RELKIND_PARTITIONED_INDEX for the index case?

On the tests:

I think 003_check.pl needs to stop and restart the table between
populating the tables and corrupting them. Otherwise, how do we know
that the subsequent checks are going to actually see the corruption
rather than something already cached in memory?

There are some philosophical questions to consider too, about how
these tests are written and what our philosophy ought to be here, but
I am again going to push that off to a future email.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company