From 7592f9d2dbf7150a66c358952785cbb5e784486b Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Fri, 13 Dec 2024 14:14:33 +0100 Subject: [PATCH 1/5] Fix crash in StorageObjectStorageQueue --- src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp b/src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp index 3b1cfd9de09..7198ffa537b 100644 --- a/src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp +++ b/src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp @@ -204,7 +204,8 @@ StorageObjectStorageQueue::StorageObjectStorageQueue( storage_metadata.setColumns(columns); storage_metadata.setConstraints(constraints_); storage_metadata.setComment(comment); - storage_metadata.settings_changes = engine_args->settings->ptr(); + if (engine_args->settings) + storage_metadata.settings_changes = engine_args->settings->ptr(); setVirtuals(VirtualColumnUtils::getVirtualsForFileLikeStorage(storage_metadata.columns, context_)); setInMemoryMetadata(storage_metadata); From 27e309db6c7be563318fb5485202784464c18cf6 Mon Sep 17 00:00:00 2001 From: kssenii Date: Fri, 13 Dec 2024 15:32:52 +0100 Subject: [PATCH 2/5] Add a test --- .../integration/test_storage_s3_queue/test.py | 61 +++++++++++++++++-- 1 file changed, 56 insertions(+), 5 deletions(-) diff --git a/tests/integration/test_storage_s3_queue/test.py b/tests/integration/test_storage_s3_queue/test.py index bbc48af2978..9581789d19d 100644 --- a/tests/integration/test_storage_s3_queue/test.py +++ b/tests/integration/test_storage_s3_queue/test.py @@ -280,6 +280,7 @@ def create_table( bucket=None, expect_error=False, database_name="default", + no_settings=False ): auth_params = ",".join(auth) bucket = started_cluster.minio_bucket if bucket is None else bucket @@ -300,11 +301,17 @@ def create_table( engine_def = f"{engine_name}('{started_cluster.env_variables['AZURITE_CONNECTION_STRING']}', '{started_cluster.azurite_container}', '{files_path}/', 'CSV')" node.query(f"DROP TABLE IF EXISTS {table_name}") - create_query = f""" - CREATE TABLE {database_name}.{table_name} ({format}) - ENGINE = {engine_def} - SETTINGS {",".join((k+"="+repr(v) for k, v in settings.items()))} - """ + if no_settings: + create_query = f""" + CREATE TABLE {database_name}.{table_name} ({format}) + ENGINE = {engine_def} + """ + else: + create_query = f""" + CREATE TABLE {database_name}.{table_name} ({format}) + ENGINE = {engine_def} + SETTINGS {",".join((k+"="+repr(v) for k, v in settings.items()))} + """ if expect_error: return node.query_and_get_error(create_query) @@ -2527,3 +2534,47 @@ def test_registry(started_cluster): node1.query(f"DROP TABLE {db_name}.{table_name} SYNC") assert zk.exists(keeper_path) is None + + +def test_upgrade_3(started_cluster): + node = started_cluster.instances["instance_24.5"] + + table_name = f"test_upgrade_3_{uuid.uuid4().hex[:8]}" + dst_table_name = f"{table_name}_dst" + keeper_path = f"/clickhouse/test_{table_name}" + files_path = f"{table_name}_data" + files_to_generate = 10 + + create_table( + started_cluster, + node, + table_name, + "ordered", + files_path, + no_settings=True + ) + total_values = generate_random_files( + started_cluster, files_path, files_to_generate, start_ind=0, row_num=1 + ) + + create_mv(node, table_name, dst_table_name) + + def get_count(): + return int(node.query(f"SELECT count() FROM {dst_table_name}")) + + expected_rows = 10 + for _ in range(20): + if expected_rows == get_count(): + break + time.sleep(1) + + assert expected_rows == get_count() + + node.restart_with_latest_version() + assert table_name in node.query("SHOW TABLES") + + assert "Cannot alter settings, because table engine doesn't support settings changes" in node.query_and_get_error( + f""" + ALTER TABLE {table_name} MODIFY SETTING processing_threads_num=5 + """ + ) From 75f3e7a0b13d6c4605674a832339a6f88d58f8d4 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Fri, 13 Dec 2024 14:39:57 +0000 Subject: [PATCH 3/5] Automatic style fix --- tests/integration/test_storage_s3_queue/test.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/integration/test_storage_s3_queue/test.py b/tests/integration/test_storage_s3_queue/test.py index 9581789d19d..8cca93cc564 100644 --- a/tests/integration/test_storage_s3_queue/test.py +++ b/tests/integration/test_storage_s3_queue/test.py @@ -280,7 +280,7 @@ def create_table( bucket=None, expect_error=False, database_name="default", - no_settings=False + no_settings=False, ): auth_params = ",".join(auth) bucket = started_cluster.minio_bucket if bucket is None else bucket @@ -2546,12 +2546,7 @@ def test_upgrade_3(started_cluster): files_to_generate = 10 create_table( - started_cluster, - node, - table_name, - "ordered", - files_path, - no_settings=True + started_cluster, node, table_name, "ordered", files_path, no_settings=True ) total_values = generate_random_files( started_cluster, files_path, files_to_generate, start_ind=0, row_num=1 @@ -2573,8 +2568,11 @@ def test_upgrade_3(started_cluster): node.restart_with_latest_version() assert table_name in node.query("SHOW TABLES") - assert "Cannot alter settings, because table engine doesn't support settings changes" in node.query_and_get_error( - f""" + assert ( + "Cannot alter settings, because table engine doesn't support settings changes" + in node.query_and_get_error( + f""" ALTER TABLE {table_name} MODIFY SETTING processing_threads_num=5 """ + ) ) From 77b2e80336b575e8a72e71619fba4912817e2872 Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Fri, 13 Dec 2024 15:41:14 +0100 Subject: [PATCH 4/5] Add a comment --- src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp b/src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp index 7198ffa537b..15f778894e2 100644 --- a/src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp +++ b/src/Storages/ObjectStorageQueue/StorageObjectStorageQueue.cpp @@ -655,6 +655,8 @@ void StorageObjectStorageQueue::alter( auto table_id = getStorageID(); StorageInMemoryMetadata old_metadata = getInMemoryMetadata(); + /// At the moment we cannot do ALTER MODIFY/RESET SETTING if there are no settings changes (exception will be thrown), + /// so we do not need to check if old_metadata.settings_changes == nullptr. const auto & old_settings = old_metadata.settings_changes->as().changes; StorageInMemoryMetadata new_metadata = getInMemoryMetadata(); From 8e36067c2a9b0be9441af1be9b5121d1c8022867 Mon Sep 17 00:00:00 2001 From: kssenii Date: Mon, 16 Dec 2024 14:18:14 +0100 Subject: [PATCH 5/5] Fix test --- tests/integration/test_storage_s3_queue/test.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/integration/test_storage_s3_queue/test.py b/tests/integration/test_storage_s3_queue/test.py index 8cca93cc564..37165a2a611 100644 --- a/tests/integration/test_storage_s3_queue/test.py +++ b/tests/integration/test_storage_s3_queue/test.py @@ -1982,6 +1982,8 @@ def test_commit_on_limit(started_cluster): def test_upgrade_2(started_cluster): node = started_cluster.instances["instance_24.5"] + if "24.5" not in node.query("select version()").strip(): + node.restart_with_original_version() table_name = f"test_upgrade_2_{uuid.uuid4().hex[:8]}" dst_table_name = f"{table_name}_dst" @@ -2538,6 +2540,8 @@ def test_registry(started_cluster): def test_upgrade_3(started_cluster): node = started_cluster.instances["instance_24.5"] + if "24.5" not in node.query("select version()").strip(): + node.restart_with_original_version() table_name = f"test_upgrade_3_{uuid.uuid4().hex[:8]}" dst_table_name = f"{table_name}_dst"