Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow all possible experimental indexes on restore #245

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Allow all possible experimental indexes on restore #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
982de56
Allow all possible experimental indexes on restore
dmitry-potepalov Oct 9, 2024
7b29af8
Ignore unknown experimental settings on restore
dmitry-potepalov Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions astacus/coordinator/plugins/clickhouse/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ async def execute(self, query: bytes, timeout: float | None = None, session_id:
class ClickHouseClientQueryError(Exception):
# If we have to handle more error types, we might consider adding proper
# rich error types and not display numeric values to the end user.
UNKNOWN_SETTING = 115
SETTING_CONSTRAINT_VIOLATION = 452

def __init__(
Expand Down
27 changes: 23 additions & 4 deletions astacus/coordinator/plugins/clickhouse/steps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,11 +631,30 @@ def _create_dbs(client: ClickHouseClient) -> Iterator[Awaitable[None]]:
# we need to re-enable these custom global settings when creating the table again.
# We can enable these settings unconditionally because they are harmless
# for tables not needing them.
# Upstream has introduced a similar list in the replica recovery context:
# https://github.com/ClickHouse/ClickHouse/commit/48ed54e822f6ed6f6bdd67db3df7a4a58e550bbb
# the two should be kept in sync until system.tables captures the query context.
b"SET allow_experimental_inverted_index=true",
b"SET allow_experimental_codecs=true",
b"SET allow_experimental_live_view=true",
b"SET allow_experimental_window_view=true",
b"SET allow_experimental_funnel_functions=true",
b"SET allow_experimental_nlp_functions=true",
b"SET allow_experimental_hash_functions=true",
b"SET allow_experimental_object_type=true",
b"SET allow_experimental_annoy_index=true",
b"SET allow_experimental_usearch_index=true",
b"SET allow_experimental_bigint_types=true",
b"SET allow_experimental_window_functions=true",
b"SET allow_experimental_geo_types=true",
b"SET allow_experimental_object_type=true",
b"SET allow_suspicious_codecs=true",
b"SET allow_experimental_map_type=true",
b"SET allow_suspicious_low_cardinality_types=true",
b"SET allow_suspicious_fixed_string_types=true",
b"SET allow_suspicious_indices=true",
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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, but I think it is easier to just add the safety guard and forget about possible problems:

  1. may be someone is using this open source project)
  2. it is actually better to add all the settings from the latest clickhouse, rather then currently used lts
  3. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# If a table was created with flatten_nested=0, we must be careful to not re-create the
# table with flatten_nested=1, since this would recreate the table with a different schema.
# If a table was created with flatten_nested=1, the query in system.tables.create_table_query
Expand All @@ -647,9 +666,9 @@ def _create_dbs(client: ClickHouseClient) -> Iterator[Awaitable[None]]:
try:
await self.clients[0].execute(query, session_id=session_id)
except ClickHouseClientQueryError as error:
if error.exception_code == error.SETTING_CONSTRAINT_VIOLATION:
if error.exception_code in (error.SETTING_CONSTRAINT_VIOLATION, error.UNKNOWN_SETTING):
# If we can't set the option, that's fine, either it's not needed or it will fail later anyway
pass
logger.info("Could not enable experimental setting, skipped; full error: %s", error)
else:
raise

Expand Down
60 changes: 54 additions & 6 deletions tests/unit/coordinator/plugins/clickhouse/test_steps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,11 +859,27 @@ async def test_creates_all_replicated_databases_and_tables_in_manifest() -> None
b"SET receive_timeout=10.0",
b"CREATE DATABASE `db-two` UUID '00000000-0000-0000-0000-000000000012'"
b" ENGINE = Replicated('/clickhouse/databases/db%2Dtwo', '{my_shard}', '{my_replica}')",
b"SET allow_experimental_inverted_index=true",
b"SET allow_experimental_codecs=true",
b"SET allow_experimental_live_view=true",
b"SET allow_experimental_window_view=true",
b"SET allow_experimental_funnel_functions=true",
b"SET allow_experimental_nlp_functions=true",
b"SET allow_experimental_hash_functions=true",
b"SET allow_experimental_object_type=true",
b"SET allow_experimental_annoy_index=true",
b"SET allow_experimental_usearch_index=true",
b"SET allow_experimental_bigint_types=true",
b"SET allow_experimental_window_functions=true",
b"SET allow_experimental_geo_types=true",
b"SET allow_experimental_object_type=true",
b"SET allow_suspicious_codecs=true",
b"SET allow_experimental_map_type=true",
b"SET allow_suspicious_low_cardinality_types=true",
b"SET allow_suspicious_fixed_string_types=true",
b"SET allow_suspicious_indices=true",
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",
b"SET flatten_nested=0",
b"CREATE TABLE db-one.table-uno ...",
b"CREATE TABLE db-one.table-dos ...",
Expand Down Expand Up @@ -917,11 +933,27 @@ async def test_creates_all_replicated_databases_and_tables_in_manifest_with_cust
b"CREATE DATABASE `db-two` UUID '00000000-0000-0000-0000-000000000012'"
b" ENGINE = Replicated('/clickhouse/databases/db%2Dtwo', '{my_shard}', '{my_replica}') "
b"SETTINGS cluster_username='alice', cluster_password='alice_secret'",
b"SET allow_experimental_inverted_index=true",
b"SET allow_experimental_codecs=true",
b"SET allow_experimental_live_view=true",
b"SET allow_experimental_window_view=true",
b"SET allow_experimental_funnel_functions=true",
b"SET allow_experimental_nlp_functions=true",
b"SET allow_experimental_hash_functions=true",
b"SET allow_experimental_object_type=true",
b"SET allow_experimental_annoy_index=true",
b"SET allow_experimental_usearch_index=true",
b"SET allow_experimental_bigint_types=true",
b"SET allow_experimental_window_functions=true",
b"SET allow_experimental_geo_types=true",
b"SET allow_experimental_object_type=true",
b"SET allow_suspicious_codecs=true",
b"SET allow_experimental_map_type=true",
b"SET allow_suspicious_low_cardinality_types=true",
b"SET allow_suspicious_fixed_string_types=true",
b"SET allow_suspicious_indices=true",
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",
b"SET flatten_nested=0",
b"CREATE TABLE db-one.table-uno ...",
b"CREATE TABLE db-one.table-dos ...",
Expand Down Expand Up @@ -960,11 +992,27 @@ async def test_drops_each_database_on_all_servers_before_recreating_it() -> None
b" ENGINE = Replicated('/clickhouse/databases/db%2Dtwo', '{my_shard}', '{my_replica}')",
]
queries_expected_on_a_single_node = [
b"SET allow_experimental_inverted_index=true",
b"SET allow_experimental_codecs=true",
b"SET allow_experimental_live_view=true",
b"SET allow_experimental_window_view=true",
b"SET allow_experimental_funnel_functions=true",
b"SET allow_experimental_nlp_functions=true",
b"SET allow_experimental_hash_functions=true",
b"SET allow_experimental_object_type=true",
b"SET allow_experimental_annoy_index=true",
b"SET allow_experimental_usearch_index=true",
b"SET allow_experimental_bigint_types=true",
b"SET allow_experimental_window_functions=true",
b"SET allow_experimental_geo_types=true",
b"SET allow_experimental_object_type=true",
b"SET allow_suspicious_codecs=true",
b"SET allow_experimental_map_type=true",
b"SET allow_suspicious_low_cardinality_types=true",
b"SET allow_suspicious_fixed_string_types=true",
b"SET allow_suspicious_indices=true",
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",
b"SET flatten_nested=0",
b"CREATE TABLE db-one.table-uno ...",
b"CREATE TABLE db-one.table-dos ...",
Expand Down
Loading