Allow all possible experimental indexes on restore #245
Conversation
Presently we don't know which experimental features/indexes were enabled during table creation, because those could have been enabled on a per-session/per-query basis and not captured in the create_table_query. To avoid failures on restore, just enable all experimental features when creating the tables. Should be relatively harmless, since it affects only the restoration session; also most of those flags do not affect tables that don't have experimental indexes.
| b"SET allow_suspicious_codecs=true", | ||
| b"SET allow_hyperscan=true", | ||
| b"SET allow_simdjson=true", | ||
| b"SET allow_deprecated_syntax_for_merge_tree=true", |
There was a problem hiding this comment.
Good work. The only thing I am concerned about is compatibility.
Previously the list was conservative and now it is quite wide.
It is possible that some very new option is not available on a running clickhouse, and will cause error like this
clickhouse01 :) set test=1
Received exception from server (version 24.8.4):
Code: 115. DB::Exception: Received from localhost:9000. DB::Exception: Unknown setting 'test'. (UNKNOWN_SETTING)
Which is extremely unpleasant during restoration.
I think it makes sense to just ignore those errors or check if the setting is available using system.settings table.
There was a problem hiding this comment.
It is possible that some very new option is not available on a running clickhouse
I've taken the list from the major we're running presently
dmitry.potepalov@dpotepalov-test:~/ClickHouse$ git describe
v23.8.8.20-lts
unless we manually call /restore on a running node, the node performing the restore should run CH no older than this LTS -> all the setting should be there. To me manually doing /restore looks like a rather obscure edge case, I'd keep the list synced with the LTS we want the new nodes to run
There was a problem hiding this comment.
unless we manually call /restore on a running node
and that's actually also an edge case for a future update of that list, since the old nodes don't have the extended list and the new nodes already have 23.8 LTS
There was a problem hiding this comment.
You are right, but I think it is easier to just add the safety guard and forget about possible problems:
- may be someone is using this open source project)
- it is actually better to add all the settings from the latest clickhouse, rather then currently used lts
- situation may change in the future it is better to just be prepared
I really do not see a reason to not protect this place.
There was a problem hiding this comment.
dynamic check against available settings is IMHO overkill, I've added a handler for the UNKNOWN_SETTING error, there's indeed some value in that.
Just in case we're running against an older CH version, ignore an experimental setting we cannot enable.
Khatskevich
deleted the
dmitry-potepalov/clickhouse/allow-experimental-indexes-on-restore
branch
Presently we don't know which experimental features/indexes were enabled during table creation, because those could have been enabled on a per-session/per-query basis and not captured in the create_table_query. To avoid failures on restore, just enable all experimental features when creating the tables. Should be relatively harmless, since it affects only the restoration session; also most of those flags do not affect tables that don't have experimental indexes.