Re: pg_amcheck contrib application

On Thu, Mar 4, 2021 at 10:29 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 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.

Just a thought - I don't feel strongly about this - but you want want
to consider storing your list of patterns in an array that gets
resized as necessary rather than a list. Then the pattern ID would
just be pattern_ptr - pattern_array, and finding the pattern by ID
would just be pattern_ptr = &pattern_array[pattern_id]. I don't think
there's a real efficiency issue here because the list of patterns is
almost always going to be short, and even if somebody decides to
provide a very long list of patterns (e.g. by using xargs) it's
probably still not that big a deal. A sufficiently obstinate user
running an operating system where argument lists can be extremely long
could probably make this the dominant cost by providing a gigantic
number of patterns that don't match anything, but such a person is
trying to prove a point, rather than accomplish anything useful, so I
don't care. But, the code might be more elegant the other way.

This patch increases the number of cases where we use ^ to assert that
exactly one of two things is true from 4 to 5. I think it might be
better to just write out (a && !b) || (b && !a), but there is some
precedent for the way you did it so perhaps it's fine.

The name prepare_table_commmand() is oddly non-parallel with
verify_heapam_slot_handler(). Seems better to call it either a table
throughout, or a heapam throughout. Actually I think I would prefer
"heap" to either of those, but I definitely think we shouldn't switch
terminology. Note that prepare_btree_command() doesn't have this
issue, since it matches verify_btree_slot_handler(). On a related
note, "c.relam = 2" is really a test for is_heap, not is_table. We
might have other table AMs in the future, but only one of those AMs
will be called heap, and only one will have OID 2.

You've got some weird round-tripping stuff where you sent literal
values to the server so that you can turn around and get them back
from the server. For example, you've got prepare_table_command()
select rel->nspname and rel->relname back from the server as literals,
which seems silly because we have to already have that information or
we couldn't ask the server to give it to us ... and if we already have
it, then why do we need to get it again? The reason it's like this
seems to be that after calling prepare_table_command(), we use
ParallelSlotSetHandler() to set verify_heapam_slot_handler() as the
callback, and we set sql.data as the callback, so we don't have access
to the RelationInfo object when we're handling the slot result. But
that's easy to fix: just store the sql as a field inside the
RelationInfo, and then pass a pointer to the whole RelationInfo to the
slot handler. Then you don't need to round-trip the table and schema
names; and you have the values available even if an error happens.

On a somewhat related note, I think it might make sense to have the
slot handlers try to free memory. It seems hard to make pg_amcheck
leak enough memory to matter, but I guess it's not entirely
implausible that someone could be checking let's say 10 million
relations. Freeing the query strings could probably prevent a half a
GB or so of accumulated memory usage under those circumstances. I
suppose freeing nspname and relname would save a bit more, but it's
hardly worth doing since they are a lot shorter and you've got to have
all that information in memory at once at some point anyway; similarly
with the RelationInfo structures, which have the further complexity of
being part of a linked list you might not want to corrupt. But you
don't need to have every query string in memory at the same time, just
as many as are running at one in time.

Also, maybe compile_relation_list_one_db() should keep the result set
around so that you don't need to pstrdup() the nspname and relname in
the first place. Right now, just before compile_relation_list_one_db()
calls PQclear() you have two copies of every nspname and relname
allocated. If you just kept the result sets around forever, the peak
memory usage would be lower than it is currently. If you really wanted
to get fancy you could arrange to free each result set when you've
finished that database, but that seems annoying to code and I'm pretty
sure it doesn't matter.

The CTEs called "include_raw" and "exclude_raw" which are used as part
of the query to construct a list of tables. The regexes are fished
through there, and the pattern IDs, which makes sense, but the raw
patterns are also fished through, and I don't see a reason for that.
We don't seem to need that for anything. The same seems to apply to
the query used to resolve database patterns.

I see that most of the queries have now been adjusted to be spread
across fewer lines, which is good, but please make sure to do that
everywhere. In particular, I notice that the bt_index_check calls are
still too spread out.

More in a bit, need to grab some lunch.

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