On Thu, Mar 4, 2021 at 10:29 AM Robert Haas <robertmh...@gmail.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
