clickhouse: updates to system.query_log ttl don't survive restart

[jmcarp]

Our static clickhouse config sets a seven-day ttl on system.query_log:

    <query_log>
        <database>system</database>
        <table>query_log</table>
        <engine>Engine = MergeTree ORDER BY event_time TTL event_date + INTERVAL 7 DAY</engine>
        <flush_interval_milliseconds>10000</flush_interval_milliseconds>
    </query_log>

In #10366, we added support for setting table ttls via clickhouse-admin/omdb. This path uses ALTER TABLE...MODIFY TTL to update ttls for both oximeter tables and system.query_log. This works as intended for oximeter tables, and works initially for system tables. However, on clickhouse restart, clickhouse observes that the structure of system.query_log doesn't match the config.xml, aliases the existing system.query_log to system.query_log_1, and creates a new system.query_log with a seven-day ttl. So the ttl set by the user (specifically, support) only lasts until the next clickhouse restart, and we have potentially many log tables.

I think the "correct" fix would involve persisting retention policies in crdb, then propagating that setting to the clickhouse config.xml. That also sounds like a non-trivial change for a small benefit. In the interim, I wonder if it would make more sense to (1) pick a shorter default ttl for system.query_log, given we probably rarely/never look at it, and (2) exclude system.query_log from the retention policy logic in clickhouse-admin, given it's not currently persistent.

Note: I'm proposing a related change in #10443 that makes the query log table much smaller for read-heavy workloads, such as scraping oximeter for current metrics. That might make this issue less urgent, although we still have a bug in that setting query log ttl isn't persistent.

Another note: thinking about it more, if we do decide to properly expose the query log ttl to omdb/users, I think we should allow setting different ttls for oximeter tables and system tables. By default, we default to 30d for measurements and 7d for query logs, and I think it's not ideal to force users to couple those ttls if they want to update either of them.

cc @bnaecker

[jmcarp]

@JustinAzoff points out that we can also set log_queries_min_query_duration_ms and skip a large number of uninteresting query logs. Most queries logged on dogfood are inserts from oximeter, complete in <10ms, and probably aren't operationally interesting. If we combine that approach with the query masking change in #10443 (honestly either change would probably be enough, but I like them both), then the query log table should become small enough that we arguably don't have to bother offering a configurable retention policy for it at all.

There are questions we wouldn't be able to answer if we dropped logs of fast queries, like counts or latency distributions. But clickhouse exposes some metrics about itself that might be enough, and we can look at application-level metrics as well.

[bnaecker]

Both of these approaches sound good to me. The TTL for the query log table is also part of the configuration XML file we pass to CH on startup, meaning we don't need a schema migration to change it. I'd be fine shrinking that statically, along with any or all of the other fixes too.