From 21bd47f09e1823b8812b71b34dd29b93ab871e63 Mon Sep 17 00:00:00 2001 From: marco-vb Date: Wed, 11 Sep 2024 17:17:15 +0000 Subject: [PATCH 1/8] Add settings min_free_disk_bytes_to_throw_insert and min_free_disk_ratio_to_throw_insert and update documentation. --- .../settings/merge-tree-settings.md | 20 +++++++++++++++++++ src/Core/Settings.h | 2 ++ src/Storages/MergeTree/MergeTreeSettings.h | 2 ++ 3 files changed, 24 insertions(+) diff --git a/docs/en/operations/settings/merge-tree-settings.md b/docs/en/operations/settings/merge-tree-settings.md index a13aacc76e6..376c1c66ad5 100644 --- a/docs/en/operations/settings/merge-tree-settings.md +++ b/docs/en/operations/settings/merge-tree-settings.md @@ -156,6 +156,26 @@ Default value: 1000. ClickHouse artificially executes `INSERT` longer (adds ‘sleep’) so that the background merge process can merge parts faster than they are added. +## min_free_disk_bytes_to_throw_insert {#min_free_disk_bytes_to_throw_insert} + +The minimum number of bytes that should be free in disk space in order to insert data. If the number of available free bytes - `keep_free_space_bytes` is less than `min_free_disk_bytes_to_throw_insert` then an exception is thrown and the insert is not executed. Note that this setting does not take into account the amount of data that will be written by the `INSERT` operation. + +Possible values: + +- Any positive integer. + +Default value: 0 bytes. + +## min_free_disk_ratio_to_throw_insert {#min_free_disk_ratio_to_throw_insert} + +The minimum free to total disk space ratio to perform an `INSERT`. The free space is calculated by subtracting `keep_free_space_bytes` from the total available space in disk. + +Possible values: + +- Float, 0.0 - 1.0 + +Default value: 0.0 + ## inactive_parts_to_throw_insert {#inactive-parts-to-throw-insert} If the number of inactive parts in a single partition more than the `inactive_parts_to_throw_insert` value, `INSERT` is interrupted with the "Too many inactive parts (N). Parts cleaning are processing significantly slower than inserts" exception. diff --git a/src/Core/Settings.h b/src/Core/Settings.h index 493752fc3fe..86522e6c378 100644 --- a/src/Core/Settings.h +++ b/src/Core/Settings.h @@ -343,6 +343,8 @@ class IColumn; M(Int64, distributed_ddl_task_timeout, 180, "Timeout for DDL query responses from all hosts in cluster. If a ddl request has not been performed on all hosts, a response will contain a timeout error and a request will be executed in an async mode. Negative value means infinite. Zero means async mode.", 0) \ M(Milliseconds, stream_flush_interval_ms, 7500, "Timeout for flushing data from streaming storages.", 0) \ M(Milliseconds, stream_poll_timeout_ms, 500, "Timeout for polling data from/to streaming storages.", 0) \ + M(UInt64, min_free_disk_bytes_to_throw_insert, 0, "Minimum free disk space bytes to throw an insert.", 0) \ + M(Double, min_free_disk_ratio_to_throw_insert, 0.0, "Minimum free disk space ratio to throw an insert.", 0) \ \ M(Bool, final, false, "Query with the FINAL modifier by default. If the engine does not support final, it does not have any effect. On queries with multiple tables final is applied only on those that support it. It also works on distributed tables", 0) \ \ diff --git a/src/Storages/MergeTree/MergeTreeSettings.h b/src/Storages/MergeTree/MergeTreeSettings.h index 0769b60dc6b..02ba56f6e9a 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.h +++ b/src/Storages/MergeTree/MergeTreeSettings.h @@ -99,6 +99,8 @@ struct Settings; M(Bool, add_implicit_sign_column_constraint_for_collapsing_engine, false, "If true, add implicit constraint for sign column for CollapsingMergeTree engine.", 0) \ M(Milliseconds, sleep_before_commit_local_part_in_replicated_table_ms, 0, "For testing. Do not change it.", 0) \ M(Bool, optimize_row_order, false, "Allow reshuffling of rows during part inserts and merges to improve the compressibility of the new part", 0) \ + M(UInt64, min_free_disk_bytes_to_throw_insert, 0, "Minimum free disk space bytes to throw an insert.", 0) \ + M(Double, min_free_disk_ratio_to_throw_insert, 0.0, "Minimum free disk space ratio to throw an insert.", 0) \ \ /* Part removal settings. */ \ M(UInt64, simultaneous_parts_removal_limit, 0, "Maximum number of parts to remove during one CleanupThread iteration (0 means unlimited).", 0) \ From 7d36f3b7646b56b5e7abc54df6e3a2c305023db0 Mon Sep 17 00:00:00 2001 From: marco-vb Date: Thu, 12 Sep 2024 09:53:07 +0000 Subject: [PATCH 2/8] Implemented checks for new settings. --- .../MergeTree/MergeTreeDataWriter.cpp | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/MergeTreeDataWriter.cpp b/src/Storages/MergeTree/MergeTreeDataWriter.cpp index f29d715e791..fa280e6080a 100644 --- a/src/Storages/MergeTree/MergeTreeDataWriter.cpp +++ b/src/Storages/MergeTree/MergeTreeDataWriter.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -688,8 +689,25 @@ MergeTreeDataWriter::TemporaryPart MergeTreeDataWriter::writeProjectionPartImpl( MergeTreeDataPartType part_type; /// Size of part would not be greater than block.bytes() + epsilon size_t expected_size = block.bytes(); + + // If not temporary insert try to reserve respecting min free disk bytes + size_t reserve_extra = 0; + + if (!is_temp) + { + const auto context = CurrentThread::getQueryContext(); + const auto * settings = context ? &context->getSettingsRef() : nullptr; + + const UInt64 min_bytes = settings->min_free_disk_bytes_to_throw_insert; + const Float64 min_ratio = settings->min_free_disk_ratio_to_throw_insert; + + const auto total_disk_space = parent_part->getDataPartStorage().calculateTotalSizeOnDisk(); + const UInt64 min_bytes_from_ratio = static_cast(min_ratio * total_disk_space); + reserve_extra = std::min(min_bytes, min_bytes_from_ratio); + } + // just check if there is enough space on parent volume - MergeTreeData::reserveSpace(expected_size, parent_part->getDataPartStorage()); + MergeTreeData::reserveSpace(expected_size + reserve_extra, parent_part->getDataPartStorage()); part_type = data.choosePartFormatOnDisk(expected_size, block.rows()).part_type; auto new_data_part = parent_part->getProjectionPartBuilder(part_name, is_temp).withPartType(part_type).build(); From 562c23eac6b73199529aad5d85a7b81aca853376 Mon Sep 17 00:00:00 2001 From: marco-vb Date: Thu, 12 Sep 2024 13:28:49 +0000 Subject: [PATCH 3/8] Add new settings to settings change history. --- src/Core/SettingsChangesHistory.cpp | 2 ++ src/Storages/MergeTree/MergeTreeDataWriter.cpp | 1 + 2 files changed, 3 insertions(+) diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index fae8c25f5ed..ed36d69fba1 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -80,6 +80,8 @@ static std::initializer_listgetDataPartStorage()); part_type = data.choosePartFormatOnDisk(expected_size, block.rows()).part_type; From ddc506a677253b206922b1faa9b72f36a866d6f2 Mon Sep 17 00:00:00 2001 From: marco-vb Date: Fri, 13 Sep 2024 13:21:35 +0000 Subject: [PATCH 4/8] Corrected implementation for check of new settings and fix lint of settings change history. --- src/Core/SettingsChangesHistory.cpp | 4 +- .../MergeTree/MergeTreeDataWriter.cpp | 48 +++++++++++-------- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index 03d0ef8ea76..490198c2376 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -82,9 +82,9 @@ static std::initializer_list #include #include -#include #include #include #include @@ -61,6 +60,7 @@ namespace ErrorCodes extern const int ABORTED; extern const int LOGICAL_ERROR; extern const int TOO_MANY_PARTS; + extern const int NOT_ENOUGH_SPACE; } namespace @@ -554,6 +554,31 @@ MergeTreeDataWriter::TemporaryPart MergeTreeDataWriter::writeTempPartImpl( VolumePtr volume = data.getStoragePolicy()->getVolume(0); VolumePtr data_part_volume = createVolumeFromReservation(reservation, volume); + const auto & data_settings = data.getSettings(); + + { + const UInt64 min_bytes = data_settings->min_free_disk_bytes_to_throw_insert; + const Float64 min_ratio = data_settings->min_free_disk_ratio_to_throw_insert; + + const auto disk = data_part_volume->getDisk(); + const UInt64 total_disk_bytes = *disk->getTotalSpace(); + const UInt64 free_disk_bytes = *disk->getUnreservedSpace(); + + const UInt64 min_bytes_from_ratio = static_cast(min_ratio * total_disk_bytes); + const UInt64 needed_free_bytes = std::max(min_bytes, min_bytes_from_ratio); + + if (needed_free_bytes > free_disk_bytes) + { + throw Exception( + ErrorCodes::NOT_ENOUGH_SPACE, + "Could not perform insert: less than {} free bytes in disk space. " + "Configure this limit with user settings {} or {}", + needed_free_bytes, + "min_free_disk_bytes_to_throw_insert", + "min_free_disk_ratio_to_throw_insert"); + } + } + auto new_data_part = data.getDataPartBuilder(part_name, data_part_volume, part_dir) .withPartFormat(data.choosePartFormat(expected_size, block.rows())) .withPartInfo(new_part_info) @@ -565,8 +590,6 @@ MergeTreeDataWriter::TemporaryPart MergeTreeDataWriter::writeTempPartImpl( if (data.storage_settings.get()->assign_part_uuids) new_data_part->uuid = UUIDHelpers::generateV4(); - const auto & data_settings = data.getSettings(); - SerializationInfo::Settings settings{data_settings->ratio_of_defaults_for_sparse_serialization, true}; SerializationInfoByName infos(columns, settings); infos.add(block); @@ -690,25 +713,8 @@ MergeTreeDataWriter::TemporaryPart MergeTreeDataWriter::writeProjectionPartImpl( /// Size of part would not be greater than block.bytes() + epsilon size_t expected_size = block.bytes(); - // If not temporary insert try to reserve respecting min free disk bytes - size_t reserve_extra = 0; - - if (!is_temp) - { - const auto context = CurrentThread::getQueryContext(); - const auto * settings = context ? &context->getSettingsRef() : nullptr; - - const UInt64 min_bytes = settings->min_free_disk_bytes_to_throw_insert; - const Float64 min_ratio = settings->min_free_disk_ratio_to_throw_insert; - - const auto total_disk_space = parent_part->getDataPartStorage().calculateTotalSizeOnDisk(); - const UInt64 min_bytes_from_ratio = static_cast(min_ratio * total_disk_space); - reserve_extra = std::min(min_bytes, min_bytes_from_ratio); - } - // just check if there is enough space on parent volume - // down the line in reserving space there is concurrency safety so no need to worry about 'over-reserving' - MergeTreeData::reserveSpace(expected_size + reserve_extra, parent_part->getDataPartStorage()); + MergeTreeData::reserveSpace(expected_size, parent_part->getDataPartStorage()); part_type = data.choosePartFormatOnDisk(expected_size, block.rows()).part_type; auto new_data_part = parent_part->getProjectionPartBuilder(part_name, is_temp).withPartType(part_type).build(); From 5cc12ca9eed1eca78a89bacdf8b824105a089aa9 Mon Sep 17 00:00:00 2001 From: marco-vb Date: Fri, 13 Sep 2024 17:16:16 +0000 Subject: [PATCH 5/8] Added integration testing for newly implemented settings. --- .../MergeTree/MergeTreeDataWriter.cpp | 5 +- .../__init__.py | 0 .../config.d/storage_configuration.xml | 19 +++++++ .../test.py | 51 +++++++++++++++++++ 4 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 tests/integration/test_stop_insert_when_disk_close_to_full/__init__.py create mode 100644 tests/integration/test_stop_insert_when_disk_close_to_full/configs/config.d/storage_configuration.xml create mode 100644 tests/integration/test_stop_insert_when_disk_close_to_full/test.py diff --git a/src/Storages/MergeTree/MergeTreeDataWriter.cpp b/src/Storages/MergeTree/MergeTreeDataWriter.cpp index 66422dd621e..b606bff7faa 100644 --- a/src/Storages/MergeTree/MergeTreeDataWriter.cpp +++ b/src/Storages/MergeTree/MergeTreeDataWriter.cpp @@ -562,7 +562,7 @@ MergeTreeDataWriter::TemporaryPart MergeTreeDataWriter::writeTempPartImpl( const auto disk = data_part_volume->getDisk(); const UInt64 total_disk_bytes = *disk->getTotalSpace(); - const UInt64 free_disk_bytes = *disk->getUnreservedSpace(); + const UInt64 free_disk_bytes = *disk->getAvailableSpace(); const UInt64 min_bytes_from_ratio = static_cast(min_ratio * total_disk_bytes); const UInt64 needed_free_bytes = std::max(min_bytes, min_bytes_from_ratio); @@ -571,9 +571,10 @@ MergeTreeDataWriter::TemporaryPart MergeTreeDataWriter::writeTempPartImpl( { throw Exception( ErrorCodes::NOT_ENOUGH_SPACE, - "Could not perform insert: less than {} free bytes in disk space. " + "Could not perform insert: less than {} free bytes in disk space ({}). " "Configure this limit with user settings {} or {}", needed_free_bytes, + free_disk_bytes, "min_free_disk_bytes_to_throw_insert", "min_free_disk_ratio_to_throw_insert"); } diff --git a/tests/integration/test_stop_insert_when_disk_close_to_full/__init__.py b/tests/integration/test_stop_insert_when_disk_close_to_full/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/integration/test_stop_insert_when_disk_close_to_full/configs/config.d/storage_configuration.xml b/tests/integration/test_stop_insert_when_disk_close_to_full/configs/config.d/storage_configuration.xml new file mode 100644 index 00000000000..d4031ff656c --- /dev/null +++ b/tests/integration/test_stop_insert_when_disk_close_to_full/configs/config.d/storage_configuration.xml @@ -0,0 +1,19 @@ + + + + + local + /disk1/ + + + + + +
+ disk1 +
+ + + + + diff --git a/tests/integration/test_stop_insert_when_disk_close_to_full/test.py b/tests/integration/test_stop_insert_when_disk_close_to_full/test.py new file mode 100644 index 00000000000..d8533ba90bc --- /dev/null +++ b/tests/integration/test_stop_insert_when_disk_close_to_full/test.py @@ -0,0 +1,51 @@ +import pytest +from helpers.cluster import ClickHouseCluster, ClickHouseInstance +from helpers.client import QueryRuntimeException + +cluster = ClickHouseCluster(__file__) + +node = cluster.add_instance( + "node", + main_configs=["configs/config.d/storage_configuration.xml"], + tmpfs=["/disk1:size=100M"], + macros={"shard": 0, "replica": 1}, +) + +@pytest.fixture(scope="module") +def start_cluster(): + try: + cluster.start() + yield cluster + finally: + cluster.shutdown() + +def test_insert_stops_when_disk_full(start_cluster): + min_free_bytes = 3 * 1024 * 1024 # 3 MiB + + node.query(f""" + CREATE TABLE test_table ( + id UInt32, + data String + ) ENGINE = MergeTree() + ORDER BY id + SETTINGS storage_policy = 'only_disk1', min_free_disk_bytes_to_throw_insert = {min_free_bytes} + """) + + count = 0 + + # Insert data to fill up disk + try: + for _ in range(100000): + node.query("INSERT INTO test_table SELECT number, repeat('a', 1000 * 1000) FROM numbers(1)") + count += 1 + except QueryRuntimeException as e: + assert "Could not perform insert" in str(e) + assert "free bytes in disk space" in str(e) + + free_space = int(node.query("SELECT free_space FROM system.disks WHERE name = 'disk1'").strip()) + assert free_space <= min_free_bytes, f"Free space ({free_space}) is less than min_free_bytes ({min_free_bytes})" + + rows = int(node.query("SELECT count() from test_table").strip()) + assert rows == count + + node.query("DROP TABLE test_table") \ No newline at end of file From 56f3030b1795a4c3afddfec600cb22afda5a204f Mon Sep 17 00:00:00 2001 From: marco-vb Date: Fri, 13 Sep 2024 17:32:33 +0000 Subject: [PATCH 6/8] Black formatting python test. --- .../test.py | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tests/integration/test_stop_insert_when_disk_close_to_full/test.py b/tests/integration/test_stop_insert_when_disk_close_to_full/test.py index d8533ba90bc..9b8943705fd 100644 --- a/tests/integration/test_stop_insert_when_disk_close_to_full/test.py +++ b/tests/integration/test_stop_insert_when_disk_close_to_full/test.py @@ -11,6 +11,7 @@ node = cluster.add_instance( macros={"shard": 0, "replica": 1}, ) + @pytest.fixture(scope="module") def start_cluster(): try: @@ -19,33 +20,42 @@ def start_cluster(): finally: cluster.shutdown() + def test_insert_stops_when_disk_full(start_cluster): min_free_bytes = 3 * 1024 * 1024 # 3 MiB - node.query(f""" + node.query( + f""" CREATE TABLE test_table ( id UInt32, data String ) ENGINE = MergeTree() ORDER BY id SETTINGS storage_policy = 'only_disk1', min_free_disk_bytes_to_throw_insert = {min_free_bytes} - """) + """ + ) count = 0 # Insert data to fill up disk try: for _ in range(100000): - node.query("INSERT INTO test_table SELECT number, repeat('a', 1000 * 1000) FROM numbers(1)") + node.query( + "INSERT INTO test_table SELECT number, repeat('a', 1000 * 1000) FROM numbers(1)" + ) count += 1 except QueryRuntimeException as e: assert "Could not perform insert" in str(e) assert "free bytes in disk space" in str(e) - free_space = int(node.query("SELECT free_space FROM system.disks WHERE name = 'disk1'").strip()) - assert free_space <= min_free_bytes, f"Free space ({free_space}) is less than min_free_bytes ({min_free_bytes})" + free_space = int( + node.query("SELECT free_space FROM system.disks WHERE name = 'disk1'").strip() + ) + assert ( + free_space <= min_free_bytes + ), f"Free space ({free_space}) is less than min_free_bytes ({min_free_bytes})" rows = int(node.query("SELECT count() from test_table").strip()) assert rows == count - node.query("DROP TABLE test_table") \ No newline at end of file + node.query("DROP TABLE test_table") From 038f56cb5e4fec023cc56a9cc1688e6985857230 Mon Sep 17 00:00:00 2001 From: marco-vb Date: Sat, 14 Sep 2024 21:04:12 +0000 Subject: [PATCH 7/8] Only make checks to stop inserts if settings are being used. --- src/Storages/MergeTree/MergeTreeDataWriter.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Storages/MergeTree/MergeTreeDataWriter.cpp b/src/Storages/MergeTree/MergeTreeDataWriter.cpp index b606bff7faa..e766ae01dfc 100644 --- a/src/Storages/MergeTree/MergeTreeDataWriter.cpp +++ b/src/Storages/MergeTree/MergeTreeDataWriter.cpp @@ -555,11 +555,11 @@ MergeTreeDataWriter::TemporaryPart MergeTreeDataWriter::writeTempPartImpl( VolumePtr data_part_volume = createVolumeFromReservation(reservation, volume); const auto & data_settings = data.getSettings(); + const UInt64 min_bytes = data_settings->min_free_disk_bytes_to_throw_insert; + const Float64 min_ratio = data_settings->min_free_disk_ratio_to_throw_insert; + if (min_bytes > 0 || min_ratio > 0.0) { - const UInt64 min_bytes = data_settings->min_free_disk_bytes_to_throw_insert; - const Float64 min_ratio = data_settings->min_free_disk_ratio_to_throw_insert; - const auto disk = data_part_volume->getDisk(); const UInt64 total_disk_bytes = *disk->getTotalSpace(); const UInt64 free_disk_bytes = *disk->getAvailableSpace(); From 03737ddcab8e2355e6f6dd17348e0123272466da Mon Sep 17 00:00:00 2001 From: marco-vb Date: Sat, 14 Sep 2024 22:24:17 +0000 Subject: [PATCH 8/8] Reduced disk size on test for faster execution. --- .../test_stop_insert_when_disk_close_to_full/test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_stop_insert_when_disk_close_to_full/test.py b/tests/integration/test_stop_insert_when_disk_close_to_full/test.py index 9b8943705fd..328de674de1 100644 --- a/tests/integration/test_stop_insert_when_disk_close_to_full/test.py +++ b/tests/integration/test_stop_insert_when_disk_close_to_full/test.py @@ -7,7 +7,7 @@ cluster = ClickHouseCluster(__file__) node = cluster.add_instance( "node", main_configs=["configs/config.d/storage_configuration.xml"], - tmpfs=["/disk1:size=100M"], + tmpfs=["/disk1:size=7M"], macros={"shard": 0, "replica": 1}, )