From 7bb8db4bb350db56aadd948410ec539b9b5c3438 Mon Sep 17 00:00:00 2001 From: Nikolay Degterinsky Date: Wed, 28 Dec 2022 00:54:50 +0000 Subject: [PATCH 01/57] Add generateULID function --- .gitmodules | 3 + contrib/CMakeLists.txt | 2 + contrib/ulid-c | 1 + .../sql-reference/functions/uuid-functions.md | 45 +++++++++++ src/CMakeLists.txt | 2 + src/Functions/generateULID.cpp | 76 +++++++++++++++++++ .../0_stateless/02515_generate_ulid.reference | 1 + .../0_stateless/02515_generate_ulid.sql | 1 + 8 files changed, 131 insertions(+) create mode 160000 contrib/ulid-c create mode 100644 src/Functions/generateULID.cpp create mode 100644 tests/queries/0_stateless/02515_generate_ulid.reference create mode 100644 tests/queries/0_stateless/02515_generate_ulid.sql diff --git a/.gitmodules b/.gitmodules index 0805b6d5492..6a35b497826 100644 --- a/.gitmodules +++ b/.gitmodules @@ -294,3 +294,6 @@ [submodule "contrib/libdivide"] path = contrib/libdivide url = https://github.com/ridiculousfish/libdivide.git +[submodule "contrib/ulid-c"] + path = contrib/ulid-c + url = https://github.com/evillique/ulid-c.git diff --git a/contrib/CMakeLists.txt b/contrib/CMakeLists.txt index 6f80059498e..9dc0663e58b 100644 --- a/contrib/CMakeLists.txt +++ b/contrib/CMakeLists.txt @@ -172,6 +172,8 @@ add_contrib (xxHash-cmake xxHash) add_contrib (google-benchmark-cmake google-benchmark) +add_contrib (ulid-c-cmake ulid-c) + # Put all targets defined here and in subdirectories under "contrib/" folders in GUI-based IDEs. # Some of third-party projects may override CMAKE_FOLDER or FOLDER property of their targets, so they would not appear # in "contrib/..." as originally planned, so we workaround this by fixing FOLDER properties of all targets manually, diff --git a/contrib/ulid-c b/contrib/ulid-c new file mode 160000 index 00000000000..8b8bc280bc3 --- /dev/null +++ b/contrib/ulid-c @@ -0,0 +1 @@ +Subproject commit 8b8bc280bc305f0d3dbdca49131677f2a208c48c diff --git a/docs/en/sql-reference/functions/uuid-functions.md b/docs/en/sql-reference/functions/uuid-functions.md index 43542367cd5..d1b3db09e6c 100644 --- a/docs/en/sql-reference/functions/uuid-functions.md +++ b/docs/en/sql-reference/functions/uuid-functions.md @@ -315,6 +315,51 @@ serverUUID() Type: [UUID](../data-types/uuid.md). + +# Functions for Working with ULID + +## generateULID + +Generates the [ULID](https://github.com/ulid/spec). + +**Syntax** + +``` sql +generateULID([x]) +``` + +**Arguments** + +- `x` — [Expression](../../sql-reference/syntax.md#syntax-expressions) resulting in any of the [supported data types](../../sql-reference/data-types/index.md#data_types). The resulting value is discarded, but the expression itself if used for bypassing [common subexpression elimination](../../sql-reference/functions/index.md#common-subexpression-elimination) if the function is called multiple times in one query. Optional parameter. + +**Returned value** + +The [FixedString](../data-types/fixedstring.md) type value. + +**Usage example** + +``` sql +SELECT generateULID() +``` + +``` text +┌─generateULID()─────────────┐ +│ 01GNB2S2FGN2P93QPXDNB4EN2R │ +└────────────────────────────┘ +``` + +**Usage example if it is needed to generate multiple values in one row** + +```sql +SELECT generateULID(1), generateULID(2) +``` + +``` text +┌─generateULID(1)────────────┬─generateULID(2)────────────┐ +│ 01GNB2SGG4RHKVNT9ZGA4FFMNP │ 01GNB2SGG4V0HMQVH4VBVPSSRB │ +└────────────────────────────┴────────────────────────────┘ +``` + ## See Also - [dictGetUUID](../../sql-reference/functions/ext-dict-functions.md#ext_dict_functions-other) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 7e3364dc92e..63c566df145 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -567,6 +567,8 @@ if (ENABLE_NLP) dbms_target_link_libraries (PUBLIC ch_contrib::nlp_data) endif() +dbms_target_link_libraries (PUBLIC ch_contrib::ulid) + if (TARGET ch_contrib::bzip2) target_link_libraries (clickhouse_common_io PRIVATE ch_contrib::bzip2) endif() diff --git a/src/Functions/generateULID.cpp b/src/Functions/generateULID.cpp new file mode 100644 index 00000000000..95f00b9dab9 --- /dev/null +++ b/src/Functions/generateULID.cpp @@ -0,0 +1,76 @@ +#include +#include +#include +#include +#include +#include + +#include + + +namespace DB +{ + +namespace ErrorCodes +{ + extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH; +} + +class FunctionGenerateULID : public IFunction +{ +public: + static constexpr size_t ULID_LENGTH = 26; + + static constexpr auto name = "generateULID"; + + static FunctionPtr create(ContextPtr /*context*/) + { + return std::make_shared(); + } + + String getName() const override { return name; } + + size_t getNumberOfArguments() const override { return 0; } + + bool isVariadic() const override { return true; } + bool isDeterministic() const override { return false; } + bool isDeterministicInScopeOfQuery() const override { return false; } + bool isSuitableForShortCircuitArgumentsExecution(const DataTypesWithConstInfo & /*arguments*/) const override { return false; } + + DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override + { + if (arguments.size() > 1) + throw Exception( + ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH, + "Number of arguments for function {} doesn't match: passed {}, should be 0 or 1.", + getName(), arguments.size()); + + return std::make_shared(ULID_LENGTH); + } + + bool useDefaultImplementationForConstants() const override { return true; } + + ColumnPtr executeImpl(const ColumnsWithTypeAndName & /*arguments*/, const DataTypePtr &, size_t input_rows_count) const override + { + auto col_res = ColumnFixedString::create(ULID_LENGTH); + auto & vec_res = col_res->getChars(); + + vec_res.resize(input_rows_count * ULID_LENGTH); + + ulid_generator generator; + ulid_generator_init(&generator, 0); + + for (size_t offset = 0, size = vec_res.size(); offset < size; offset += ULID_LENGTH) + ulid_generate(&generator, reinterpret_cast(&vec_res[offset])); + + return col_res; + } +}; + + +REGISTER_FUNCTION(GenerateULID) +{ + factory.registerFunction(); +} + +} diff --git a/tests/queries/0_stateless/02515_generate_ulid.reference b/tests/queries/0_stateless/02515_generate_ulid.reference new file mode 100644 index 00000000000..23aed8ce678 --- /dev/null +++ b/tests/queries/0_stateless/02515_generate_ulid.reference @@ -0,0 +1 @@ +1 FixedString(26) diff --git a/tests/queries/0_stateless/02515_generate_ulid.sql b/tests/queries/0_stateless/02515_generate_ulid.sql new file mode 100644 index 00000000000..3e179a45bac --- /dev/null +++ b/tests/queries/0_stateless/02515_generate_ulid.sql @@ -0,0 +1 @@ +SELECT generateULID(1) != generateULID(2), toTypeName(generateULID()); From 945d50af879803ee86fc232d7d7d185e2867b684 Mon Sep 17 00:00:00 2001 From: Nikolay Degterinsky Date: Thu, 29 Dec 2022 02:17:52 +0000 Subject: [PATCH 02/57] Fix build --- src/CMakeLists.txt | 4 +++- src/Common/config.h.in | 1 + src/Functions/generateULID.cpp | 6 ++++++ src/configure_config.cmake | 3 +++ tests/queries/0_stateless/02515_generate_ulid.sql | 2 ++ 5 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 63c566df145..208bc3ea2f7 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -567,7 +567,9 @@ if (ENABLE_NLP) dbms_target_link_libraries (PUBLIC ch_contrib::nlp_data) endif() -dbms_target_link_libraries (PUBLIC ch_contrib::ulid) +if (TARGET ch_contrib::ulid) + dbms_target_link_libraries (PUBLIC ch_contrib::ulid) +endif() if (TARGET ch_contrib::bzip2) target_link_libraries (clickhouse_common_io PRIVATE ch_contrib::bzip2) diff --git a/src/Common/config.h.in b/src/Common/config.h.in index baa480a6545..eb674865298 100644 --- a/src/Common/config.h.in +++ b/src/Common/config.h.in @@ -54,3 +54,4 @@ #cmakedefine01 USE_BLAKE3 #cmakedefine01 USE_SKIM #cmakedefine01 USE_OPENSSL_INTREE +#cmakedefine01 USE_ULID diff --git a/src/Functions/generateULID.cpp b/src/Functions/generateULID.cpp index 95f00b9dab9..46b8d190496 100644 --- a/src/Functions/generateULID.cpp +++ b/src/Functions/generateULID.cpp @@ -1,3 +1,7 @@ +#include "config.h" + +#if USE_ULID + #include #include #include @@ -74,3 +78,5 @@ REGISTER_FUNCTION(GenerateULID) } } + +#endif diff --git a/src/configure_config.cmake b/src/configure_config.cmake index 58cb34b7d67..ae9725dff63 100644 --- a/src/configure_config.cmake +++ b/src/configure_config.cmake @@ -94,6 +94,9 @@ endif() if (ENABLE_NLP) set(USE_NLP 1) endif() +if (TARGET ch_contrib::ulid) + set(USE_ULID 1) +endif() if (TARGET ch_contrib::llvm) set(USE_EMBEDDED_COMPILER 1) endif() diff --git a/tests/queries/0_stateless/02515_generate_ulid.sql b/tests/queries/0_stateless/02515_generate_ulid.sql index 3e179a45bac..4059090a72b 100644 --- a/tests/queries/0_stateless/02515_generate_ulid.sql +++ b/tests/queries/0_stateless/02515_generate_ulid.sql @@ -1 +1,3 @@ +-- Tags: no-fasttest + SELECT generateULID(1) != generateULID(2), toTypeName(generateULID()); From 57b3fbf206a160edb9e7cce0e9bdf45187fa3de8 Mon Sep 17 00:00:00 2001 From: Nikolay Degterinsky Date: Wed, 4 Jan 2023 02:24:14 +0000 Subject: [PATCH 03/57] Fix build --- contrib/ulid-c-cmake/CMakeLists.txt | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 contrib/ulid-c-cmake/CMakeLists.txt diff --git a/contrib/ulid-c-cmake/CMakeLists.txt b/contrib/ulid-c-cmake/CMakeLists.txt new file mode 100644 index 00000000000..9a68e09faa2 --- /dev/null +++ b/contrib/ulid-c-cmake/CMakeLists.txt @@ -0,0 +1,17 @@ +option (ENABLE_ULID "Enable ulid" ${ENABLE_LIBRARIES}) + +if (NOT ENABLE_ULID) + message(STATUS "Not using ulid") + return() +endif() + +set (LIBRARY_DIR "${ClickHouse_SOURCE_DIR}/contrib/ulid-c") + +set (SRCS + "${LIBRARY_DIR}/include/ulid.h" + "${LIBRARY_DIR}/include/ulid.c" +) + +add_library(_ulid ${SRCS}) +target_include_directories(_ulid SYSTEM PUBLIC "${LIBRARY_DIR}/include") +add_library(ch_contrib::ulid ALIAS _ulid) \ No newline at end of file From a10f020b2d61a2e35b97fab3fabaaedebb1ff929 Mon Sep 17 00:00:00 2001 From: flynn Date: Sat, 4 Feb 2023 14:28:31 +0000 Subject: [PATCH 04/57] Storage Log faminy support settings storage policy --- src/Storages/StorageLog.cpp | 2 +- src/Storages/StorageLogSettings.cpp | 12 +++++++++++- src/Storages/StorageLogSettings.h | 4 +++- src/Storages/StorageStripeLog.cpp | 2 +- ...54_log_faminy_support_storage_policy.reference | 2 ++ .../02554_log_faminy_support_storage_policy.sql | 15 +++++++++++++++ 6 files changed, 33 insertions(+), 4 deletions(-) create mode 100644 tests/queries/0_stateless/02554_log_faminy_support_storage_policy.reference create mode 100644 tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql diff --git a/src/Storages/StorageLog.cpp b/src/Storages/StorageLog.cpp index 7d445c0d7ec..f2eca42ed0b 100644 --- a/src/Storages/StorageLog.cpp +++ b/src/Storages/StorageLog.cpp @@ -1104,7 +1104,7 @@ void registerStorageLog(StorageFactory & factory) throw Exception(ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH, "Engine {} doesn't support any arguments ({} given)", args.engine_name, args.engine_args.size()); - String disk_name = getDiskName(*args.storage_def); + String disk_name = getDiskName(*args.storage_def, args.getContext()); DiskPtr disk = args.getContext()->getDisk(disk_name); return std::make_shared( diff --git a/src/Storages/StorageLogSettings.cpp b/src/Storages/StorageLogSettings.cpp index 900e1070eac..1c15f8ff392 100644 --- a/src/Storages/StorageLogSettings.cpp +++ b/src/Storages/StorageLogSettings.cpp @@ -1,17 +1,27 @@ #include "StorageLogSettings.h" +#include +#include #include #include namespace DB { -String getDiskName(ASTStorage & storage_def) +String getDiskName(ASTStorage & storage_def, ContextPtr context) { if (storage_def.settings) { SettingsChanges changes = storage_def.settings->changes; for (const auto & change : changes) + { + /// How about both disk and storage_policy are specified? if (change.name == "disk") return change.value.safeGet(); + if (change.name == "storage_policy") + { + auto policy = context->getStoragePolicy(change.value.safeGet()); + return policy->getAnyDisk()->getName(); + } + } } return "default"; } diff --git a/src/Storages/StorageLogSettings.h b/src/Storages/StorageLogSettings.h index 0903c034ec6..fa8bb282360 100644 --- a/src/Storages/StorageLogSettings.h +++ b/src/Storages/StorageLogSettings.h @@ -5,6 +5,8 @@ namespace DB { class ASTStorage; + class Context; + using ContextPtr = std::shared_ptr; - String getDiskName(ASTStorage & storage_def); + String getDiskName(ASTStorage & storage_def, ContextPtr context); } diff --git a/src/Storages/StorageStripeLog.cpp b/src/Storages/StorageStripeLog.cpp index be5045b884f..870f6b96ae6 100644 --- a/src/Storages/StorageStripeLog.cpp +++ b/src/Storages/StorageStripeLog.cpp @@ -678,7 +678,7 @@ void registerStorageStripeLog(StorageFactory & factory) throw Exception(ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH, "Engine {} doesn't support any arguments ({} given)", args.engine_name, args.engine_args.size()); - String disk_name = getDiskName(*args.storage_def); + String disk_name = getDiskName(*args.storage_def, args.getContext()); DiskPtr disk = args.getContext()->getDisk(disk_name); return std::make_shared( diff --git a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.reference b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.reference new file mode 100644 index 00000000000..6ed281c757a --- /dev/null +++ b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.reference @@ -0,0 +1,2 @@ +1 +1 diff --git a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql new file mode 100644 index 00000000000..32fb09c0b42 --- /dev/null +++ b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql @@ -0,0 +1,15 @@ +DROP TABLE IF EXISTS test_2554_log; +CREATE TABLE test_2554_log (n UInt32) ENGINE = Log SETTINGS storage_policy = 'default'; + +INSERT INTO test_2554_log SELECT 1; +SELECT * FROM test_2554_log; + +DROP TABLE test_2554_log; + +DROP TABLE IF EXISTS test_2554_tinylog; +CREATE TABLE test_2554_tinylog (n UInt32) ENGINE = Log SETTINGS storage_policy = 'default'; + +INSERT INTO test_2554_tinylog SELECT 1; +SELECT * FROM test_2554_tinylog; + +DROP TABLE test_2554_tinylog; From 37735b49c20f8b31d866eb822897343f9335f62d Mon Sep 17 00:00:00 2001 From: flynn Date: Sat, 4 Feb 2023 14:30:09 +0000 Subject: [PATCH 05/57] update test --- .../02554_log_faminy_support_storage_policy.reference | 1 + .../02554_log_faminy_support_storage_policy.sql | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.reference b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.reference index 6ed281c757a..e8183f05f5d 100644 --- a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.reference +++ b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.reference @@ -1,2 +1,3 @@ 1 1 +1 diff --git a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql index 32fb09c0b42..540c82c598d 100644 --- a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql +++ b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql @@ -13,3 +13,11 @@ INSERT INTO test_2554_tinylog SELECT 1; SELECT * FROM test_2554_tinylog; DROP TABLE test_2554_tinylog; + +DROP TABLE IF EXISTS test_2554_stripelog; +CREATE TABLE test_2554_stripelog (n UInt32) ENGINE = StripeLog SETTINGS storage_policy = 'default'; + +INSERT INTO test_2554_stripelog SELECT 1; +SELECT * FROM test_2554_stripelog; + +DROP TABLE test_2554_stripelog; From 4cd361747d7717e07499bfdd984f8deaae5a1052 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 6 Feb 2023 09:51:11 +0000 Subject: [PATCH 06/57] Address PR comment --- src/IO/S3/Client.cpp | 61 ++++++++++++++++++++++ src/IO/S3/Client.h | 81 ++++++++++++------------------ src/Storages/StorageS3Settings.cpp | 2 - src/Storages/StorageS3Settings.h | 10 ---- 4 files changed, 94 insertions(+), 60 deletions(-) diff --git a/src/IO/S3/Client.cpp b/src/IO/S3/Client.cpp index 2e07dfb19ff..8a3ff366dd9 100644 --- a/src/IO/S3/Client.cpp +++ b/src/IO/S3/Client.cpp @@ -76,6 +76,67 @@ void Client::RetryStrategy::RequestBookkeeping(const Aws::Client::HttpResponseOu return wrapped_strategy->RequestBookkeeping(httpResponseOutcome, lastError); } +namespace +{ + +void verifyClientConfiguration(const Aws::Client::ClientConfiguration & client_config) +{ + if (!client_config.retryStrategy) + throw Exception(ErrorCodes::LOGICAL_ERROR, "The S3 client can only be used with Client::RetryStrategy, define it in the client configuration"); + + assert_cast(*client_config.retryStrategy); +} + +} + +std::unique_ptr Client::create( + size_t max_redirects_, + const std::shared_ptr & credentials_provider, + const Aws::Client::ClientConfiguration & client_configuration, + Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy sign_payloads, + bool use_virtual_addressing) +{ + verifyClientConfiguration(client_configuration); + return std::unique_ptr( + new Client(max_redirects_, credentials_provider, client_configuration, sign_payloads, use_virtual_addressing)); +} + +std::unique_ptr Client::create(const Client & other) +{ + return std::unique_ptr(new Client(other)); +} + +Client::Client( + size_t max_redirects_, + const std::shared_ptr & credentials_provider, + const Aws::Client::ClientConfiguration & client_configuration, + Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy sign_payloads, + bool use_virtual_addressing) + : Aws::S3::S3Client(credentials_provider, client_configuration, std::move(sign_payloads), use_virtual_addressing) + , max_redirects(max_redirects_) + , log(&Poco::Logger::get("S3Client")) +{ + auto * endpoint_provider = dynamic_cast(accessEndpointProvider().get()); + endpoint_provider->GetBuiltInParameters().GetParameter("Region").GetString(explicit_region); + std::string endpoint; + endpoint_provider->GetBuiltInParameters().GetParameter("Endpoint").GetString(endpoint); + detect_region = explicit_region == Aws::Region::AWS_GLOBAL && endpoint.find(".amazonaws.com") != std::string::npos; + + cache = std::make_shared(); + ClientCacheRegistry::instance().registerClient(cache); +} + +Client::Client(const Client & other) + : Aws::S3::S3Client(other) + , explicit_region(other.explicit_region) + , detect_region(other.detect_region) + , max_redirects(other.max_redirects) + , log(&Poco::Logger::get("S3Client")) +{ + cache = std::make_shared(*other.cache); + ClientCacheRegistry::instance().registerClient(cache); +} + bool Client::checkIfWrongRegionDefined(const std::string & bucket, const Aws::S3::S3Error & error, std::string & region) const { if (detect_region) diff --git a/src/IO/S3/Client.h b/src/IO/S3/Client.h index 13f26c214f2..8356b0b25fa 100644 --- a/src/IO/S3/Client.h +++ b/src/IO/S3/Client.h @@ -22,7 +22,6 @@ namespace DB namespace ErrorCodes { - extern const int LOGICAL_ERROR; extern const int TOO_MANY_REDIRECTS; } @@ -78,15 +77,25 @@ private: /// - automatically detect endpoint and regions for each bucket and cache them /// /// For this client to work correctly both Client::RetryStrategy and Requests defined in should be used. -class Client : public Aws::S3::S3Client +/// +/// To add support for new type of request +/// - ExtendedRequest should be defined inside IO/S3/Requests.h +/// - new method accepting that request should be defined in this Client (check other requests for reference) +/// - method handling the request from Aws::S3::S3Client should be left to private so we don't use it by accident +class Client : private Aws::S3::S3Client { public: - template - static std::unique_ptr create(Args &&... args) - { - (verifyArgument(args), ...); - return std::unique_ptr(new Client(std::forward(args)...)); - } + /// we use a factory method to verify arguments before creating a client because + /// there are certain requirements on arguments for it to work correctly + /// e.g. Client::RetryStrategy should be used + static std::unique_ptr create( + size_t max_redirects_, + const std::shared_ptr & credentials_provider, + const Aws::Client::ClientConfiguration & client_configuration, + Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy sign_payloads, + bool use_virtual_addressing); + + static std::unique_ptr create(const Client & other); Client & operator=(const Client &) = delete; @@ -106,7 +115,12 @@ public: } } - /// Decorator for RetryStrategy needed for this client to work correctly + /// Decorator for RetryStrategy needed for this client to work correctly. + /// We want to manually handle permanent moves (status code 301) because: + /// - redirect location is written in XML format inside the response body something that doesn't exist for HEAD + /// requests so we need to manually find the correct location + /// - we want to cache the new location to decrease number of roundtrips for future requests + /// This decorator doesn't retry if 301 is detected and fallbacks to the inner retry strategy otherwise. class RetryStrategy : public Aws::Client::RetryStrategy { public: @@ -147,35 +161,19 @@ public: Model::DeleteObjectOutcome DeleteObject(const DeleteObjectRequest & request) const; Model::DeleteObjectsOutcome DeleteObjects(const DeleteObjectsRequest & request) const; + using Aws::S3::S3Client::EnableRequestProcessing; + using Aws::S3::S3Client::DisableRequestProcessing; private: - template - explicit Client(size_t max_redirects_, Args &&... args) - : Aws::S3::S3Client(std::forward(args)...) - , max_redirects(max_redirects_) - , log(&Poco::Logger::get("S3Client")) - { - auto * endpoint_provider = dynamic_cast(accessEndpointProvider().get()); - endpoint_provider->GetBuiltInParameters().GetParameter("Region").GetString(explicit_region); - std::string endpoint; - endpoint_provider->GetBuiltInParameters().GetParameter("Endpoint").GetString(endpoint); - detect_region = explicit_region == Aws::Region::AWS_GLOBAL && endpoint.find(".amazonaws.com") != std::string::npos; + Client(size_t max_redirects_, + const std::shared_ptr& credentials_provider, + const Aws::Client::ClientConfiguration& client_configuration, + Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy sign_payloads, + bool use_virtual_addressing); - cache = std::make_shared(); - ClientCacheRegistry::instance().registerClient(cache); - } + Client(const Client & other); - Client(const Client & other) - : Aws::S3::S3Client(other) - , explicit_region(other.explicit_region) - , detect_region(other.detect_region) - , max_redirects(other.max_redirects) - , log(&Poco::Logger::get("S3Client")) - { - cache = std::make_shared(*other.cache); - ClientCacheRegistry::instance().registerClient(cache); - } - - /// Make regular functions private + /// Leave regular functions private so we don't accidentally use them + /// otherwise region and endpoint redirection won't work using Aws::S3::S3Client::HeadObject; using Aws::S3::S3Client::ListObjectsV2; using Aws::S3::S3Client::ListObjects; @@ -279,19 +277,6 @@ private: bool checkIfWrongRegionDefined(const std::string & bucket, const Aws::S3::S3Error & error, std::string & region) const; void insertRegionOverride(const std::string & bucket, const std::string & region) const; - template - static void verifyArgument(const T & /*arg*/) - {} - - template T> - static void verifyArgument(const T & client_config) - { - if (!client_config.retryStrategy) - throw Exception(ErrorCodes::LOGICAL_ERROR, "The S3 client can only be used with Client::RetryStrategy, define it in the client configuration"); - - assert_cast(*client_config.retryStrategy); - } - std::string explicit_region; mutable bool detect_region = true; diff --git a/src/Storages/StorageS3Settings.cpp b/src/Storages/StorageS3Settings.cpp index ee0b1fd88bf..ee704b3e750 100644 --- a/src/Storages/StorageS3Settings.cpp +++ b/src/Storages/StorageS3Settings.cpp @@ -166,7 +166,6 @@ S3Settings::RequestSettings::RequestSettings(const NamedCollection & collection) max_single_read_retries = collection.getOrDefault("max_single_read_retries", max_single_read_retries); max_connections = collection.getOrDefault("max_connections", max_connections); list_object_keys_size = collection.getOrDefault("list_object_keys_size", list_object_keys_size); - allow_head_object_request = collection.getOrDefault("allow_head_object_request", allow_head_object_request); } S3Settings::RequestSettings::RequestSettings( @@ -181,7 +180,6 @@ S3Settings::RequestSettings::RequestSettings( max_connections = config.getUInt64(key + "max_connections", settings.s3_max_connections); check_objects_after_upload = config.getBool(key + "check_objects_after_upload", settings.s3_check_objects_after_upload); list_object_keys_size = config.getUInt64(key + "list_object_keys_size", settings.s3_list_object_keys_size); - allow_head_object_request = config.getBool(key + "allow_head_object_request", allow_head_object_request); /// NOTE: it would be better to reuse old throttlers to avoid losing token bucket state on every config reload, /// which could lead to exceeding limit for short time. But it is good enough unless very high `burst` values are used. diff --git a/src/Storages/StorageS3Settings.h b/src/Storages/StorageS3Settings.h index 61da0a37f62..bce772859f0 100644 --- a/src/Storages/StorageS3Settings.h +++ b/src/Storages/StorageS3Settings.h @@ -67,16 +67,6 @@ struct S3Settings ThrottlerPtr get_request_throttler; ThrottlerPtr put_request_throttler; - /// If this is set to false then `S3::getObjectSize()` will use `GetObjectAttributes` request instead of `HeadObject`. - /// Details: `HeadObject` request never returns a response body (even if there is an error) however - /// if the request was sent without specifying a region in the endpoint (i.e. for example "https://test.s3.amazonaws.com/mydata.csv" - /// instead of "https://test.s3-us-west-2.amazonaws.com/mydata.csv") then that response body is one of the main ways to determine - /// the correct region and try to repeat the request again with the correct region. - /// For any other request type (`GetObject`, `ListObjects`, etc.) AWS SDK does that because they have response bodies, but for `HeadObject` - /// there is no response body so this way doesn't work. That's why it's better to use `GetObjectAttributes` requests sometimes. - /// See https://github.com/aws/aws-sdk-cpp/issues/1558 and also the function S3ErrorMarshaller::ExtractRegion() for more information. - bool allow_head_object_request = true; - const PartUploadSettings & getUploadSettings() const { return upload_settings; } RequestSettings() = default; From 28d1fd6c8345263fd34fc2482d976322b87bf930 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Wed, 8 Feb 2023 09:48:35 +0000 Subject: [PATCH 07/57] More fixes --- src/IO/S3/Client.cpp | 79 ++++++++++++++++++++++++++++++++++++- src/IO/S3/Client.h | 75 +---------------------------------- src/IO/S3/getObjectInfo.cpp | 3 +- 3 files changed, 80 insertions(+), 77 deletions(-) diff --git a/src/IO/S3/Client.cpp b/src/IO/S3/Client.cpp index 8a3ff366dd9..b6ae7abc98f 100644 --- a/src/IO/S3/Client.cpp +++ b/src/IO/S3/Client.cpp @@ -192,7 +192,7 @@ Model::HeadObjectOutcome Client::HeadObject(const HeadObjectRequest & request) c if (checkIfWrongRegionDefined(bucket, error, new_region)) { request.overrideRegion(new_region); - return HeadObject(request); + return Aws::S3::S3Client::HeadObject(request); } if (error.GetResponseCode() != Aws::Http::HttpResponseCode::MOVED_PERMANENTLY) @@ -305,6 +305,83 @@ Model::DeleteObjectsOutcome Client::DeleteObjects(const DeleteObjectsRequest & r return doRequest(request, [this](const Model::DeleteObjectsRequest & req) { return Aws::S3::S3Client::DeleteObjects(req); }); } +template +std::invoke_result_t +Client::doRequest(const RequestType & request, RequestFn request_fn) const +{ + const auto & bucket = request.GetBucket(); + + if (auto region = getRegionForBucket(bucket); !region.empty()) + { + if (!detect_region) + LOG_INFO(log, "Using region override {} for bucket {}", region, bucket); + + request.overrideRegion(std::move(region)); + } + + if (auto uri = getURIForBucket(bucket); uri.has_value()) + request.overrideURI(std::move(*uri)); + + + bool found_new_endpoint = false; + // if we found correct endpoint after 301 responses, update the cache for future requests + SCOPE_EXIT( + if (found_new_endpoint) + { + auto uri_override = request.getURIOverride(); + assert(uri_override.has_value()); + updateURIForBucket(bucket, std::move(*uri_override)); + } + ); + + for (size_t attempt = 0; attempt <= max_redirects; ++attempt) + { + auto result = request_fn(request); + if (result.IsSuccess()) + return result; + + const auto & error = result.GetError(); + + std::string new_region; + if (checkIfWrongRegionDefined(bucket, error, new_region)) + { + request.overrideRegion(new_region); + continue; + } + + if (error.GetResponseCode() != Aws::Http::HttpResponseCode::MOVED_PERMANENTLY) + return result; + + // maybe we detect a correct region + if (!detect_region) + { + if (auto region = GetErrorMarshaller()->ExtractRegion(error); !region.empty() && region != explicit_region) + { + request.overrideRegion(region); + insertRegionOverride(bucket, region); + } + } + + // we possibly got new location, need to try with that one + auto new_uri = getURIFromError(error); + if (!new_uri) + return result; + + const auto & current_uri_override = request.getURIOverride(); + /// we already tried with this URI + if (current_uri_override && current_uri_override->uri == new_uri->uri) + { + LOG_INFO(log, "Getting redirected to the same invalid location {}", new_uri->uri.toString()); + return result; + } + + found_new_endpoint = true; + request.overrideURI(*new_uri); + } + + throw Exception(ErrorCodes::TOO_MANY_REDIRECTS, "Too many redirects"); +} + std::string Client::getRegionForBucket(const std::string & bucket, bool force_detect) const { std::lock_guard lock(cache->region_cache_mutex); diff --git a/src/IO/S3/Client.h b/src/IO/S3/Client.h index 8356b0b25fa..c3086499167 100644 --- a/src/IO/S3/Client.h +++ b/src/IO/S3/Client.h @@ -192,80 +192,7 @@ private: template std::invoke_result_t - doRequest(const RequestType & request, RequestFn request_fn) const - { - const auto & bucket = request.GetBucket(); - - if (auto region = getRegionForBucket(bucket); !region.empty()) - { - if (!detect_region) - LOG_INFO(log, "Using region override {} for bucket {}", region, bucket); - - request.overrideRegion(std::move(region)); - } - - if (auto uri = getURIForBucket(bucket); uri.has_value()) - request.overrideURI(std::move(*uri)); - - - bool found_new_endpoint = false; - // if we found correct endpoint after 301 responses, update the cache for future requests - SCOPE_EXIT( - if (found_new_endpoint) - { - auto uri_override = request.getURIOverride(); - assert(uri_override.has_value()); - updateURIForBucket(bucket, std::move(*uri_override)); - } - ); - - for (size_t attempt = 0; attempt <= max_redirects; ++attempt) - { - auto result = request_fn(request); - if (result.IsSuccess()) - return result; - - const auto & error = result.GetError(); - - std::string new_region; - if (checkIfWrongRegionDefined(bucket, error, new_region)) - { - request.overrideRegion(new_region); - continue; - } - - if (error.GetResponseCode() != Aws::Http::HttpResponseCode::MOVED_PERMANENTLY) - return result; - - // maybe we detect a correct region - if (!detect_region) - { - if (auto region = GetErrorMarshaller()->ExtractRegion(error); !region.empty() && region != explicit_region) - { - request.overrideRegion(region); - insertRegionOverride(bucket, region); - } - } - - // we possibly got new location, need to try with that one - auto new_uri = getURIFromError(error); - if (!new_uri) - return result; - - const auto & current_uri_override = request.getURIOverride(); - /// we already tried with this URI - if (current_uri_override && current_uri_override->uri == new_uri->uri) - { - LOG_INFO(log, "Getting redirected to the same invalid location {}", new_uri->uri.toString()); - return result; - } - - found_new_endpoint = true; - request.overrideURI(*new_uri); - } - - throw Exception(ErrorCodes::TOO_MANY_REDIRECTS, "Too many redirects"); - } + doRequest(const RequestType & request, RequestFn request_fn) const; void updateURIForBucket(const std::string & bucket, S3::URI new_uri) const; std::optional getURIFromError(const Aws::S3::S3Error & error) const; diff --git a/src/IO/S3/getObjectInfo.cpp b/src/IO/S3/getObjectInfo.cpp index 20d5e74d6d4..c652f16ab20 100644 --- a/src/IO/S3/getObjectInfo.cpp +++ b/src/IO/S3/getObjectInfo.cpp @@ -42,7 +42,6 @@ namespace } /// Performs a request to get the size and last modification time of an object. - /// The function performs either HeadObject or GetObjectAttributes request depending on the endpoint. std::pair, Aws::S3::S3Error> tryGetObjectInfo( const S3::Client & client, const String & bucket, const String & key, const String & version_id, const S3Settings::RequestSettings & /*request_settings*/, bool with_metadata, bool for_disk_s3) @@ -87,7 +86,7 @@ ObjectInfo getObjectInfo( else if (throw_on_error) { throw DB::Exception(ErrorCodes::S3_ERROR, - "Failed to get object attributes: {}. HTTP response code: {}", + "Failed to get object info: {}. HTTP response code: {}", error.GetMessage(), static_cast(error.GetResponseCode())); } return {}; From 3418a8e955f14318fdcdac299fa2d144f84c26d9 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Wed, 8 Feb 2023 10:07:30 +0000 Subject: [PATCH 08/57] use master for aws --- contrib/aws | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/aws b/contrib/aws index 06a6610e6fb..ecccfc026a4 160000 --- a/contrib/aws +++ b/contrib/aws @@ -1 +1 @@ -Subproject commit 06a6610e6fb3385e22ad85014a67aa307825ffb1 +Subproject commit ecccfc026a42b30023289410a67024d561f4bf3e From 2d26c9cffc8b69df1cc78e8f1ab944b63dcdef18 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Wed, 8 Feb 2023 10:39:32 +0000 Subject: [PATCH 09/57] Fix error codes --- src/IO/S3/Client.cpp | 1 + src/IO/S3/Client.h | 12 +----------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/src/IO/S3/Client.cpp b/src/IO/S3/Client.cpp index b6ae7abc98f..197b405da6a 100644 --- a/src/IO/S3/Client.cpp +++ b/src/IO/S3/Client.cpp @@ -23,6 +23,7 @@ namespace DB namespace ErrorCodes { extern const int LOGICAL_ERROR; + extern const int TOO_MANY_REDIRECTS; } namespace S3 diff --git a/src/IO/S3/Client.h b/src/IO/S3/Client.h index c3086499167..47f7cdd2ed1 100644 --- a/src/IO/S3/Client.h +++ b/src/IO/S3/Client.h @@ -17,15 +17,7 @@ #include #include -namespace DB -{ - -namespace ErrorCodes -{ - extern const int TOO_MANY_REDIRECTS; -} - -namespace S3 +namespace DB::S3 { namespace Model = Aws::S3::Model; @@ -216,6 +208,4 @@ private: } -} - #endif From f61f95fcec41c3800abd49722c34cb4554c495d5 Mon Sep 17 00:00:00 2001 From: serxa Date: Thu, 9 Feb 2023 18:54:15 +0000 Subject: [PATCH 10/57] Replace background executor scheduler merges and mutations with round-robin --- docker/test/stress/run.sh | 2 +- src/Interpreters/Context.h | 8 +- .../MergeTree/MergeTreeBackgroundExecutor.cpp | 5 +- .../MergeTree/MergeTreeBackgroundExecutor.h | 123 +++++++++++++++--- .../MergeTree/tests/gtest_executor.cpp | 77 ++++++++++- 5 files changed, 184 insertions(+), 31 deletions(-) diff --git a/docker/test/stress/run.sh b/docker/test/stress/run.sh index 314be8ae0fd..c99a34a3911 100644 --- a/docker/test/stress/run.sh +++ b/docker/test/stress/run.sh @@ -642,7 +642,7 @@ if [ "$DISABLE_BC_CHECK" -ne "1" ]; then -e "} TCPHandler: Code:" \ -e "} executeQuery: Code:" \ -e "Missing columns: 'v3' while processing query: 'v3, k, v1, v2, p'" \ - -e "[Queue = DB::MergeMutateRuntimeQueue]: Code: 235. DB::Exception: Part" \ + -e "[Queue = DB::RoundRobinRuntimeQueue]: Code: 235. DB::Exception: Part" \ -e "The set of parts restored in place of" \ -e "(ReplicatedMergeTreeAttachThread): Initialization failed. Error" \ -e "Code: 269. DB::Exception: Destination table is myself" \ diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index f40f8608092..000752542c7 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -129,11 +129,11 @@ class StoragePolicySelector; using StoragePolicySelectorPtr = std::shared_ptr; template class MergeTreeBackgroundExecutor; -class MergeMutateRuntimeQueue; -class OrdinaryRuntimeQueue; -using MergeMutateBackgroundExecutor = MergeTreeBackgroundExecutor; +class RoundRobinRuntimeQueue; +class FifoRuntimeQueue; +using MergeMutateBackgroundExecutor = MergeTreeBackgroundExecutor; using MergeMutateBackgroundExecutorPtr = std::shared_ptr; -using OrdinaryBackgroundExecutor = MergeTreeBackgroundExecutor; +using OrdinaryBackgroundExecutor = MergeTreeBackgroundExecutor; using OrdinaryBackgroundExecutorPtr = std::shared_ptr; struct PartUUIDs; using PartUUIDsPtr = std::shared_ptr; diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp index 5bc3fda88bb..ded48dc69ea 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp @@ -268,7 +268,8 @@ void MergeTreeBackgroundExecutor::threadFunction() } -template class MergeTreeBackgroundExecutor; -template class MergeTreeBackgroundExecutor; +template class MergeTreeBackgroundExecutor; +template class MergeTreeBackgroundExecutor; +template class MergeTreeBackgroundExecutor; } diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h index 5c1178a1bc1..3030c28ef1d 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h @@ -67,8 +67,8 @@ struct TaskRuntimeData } }; - -class OrdinaryRuntimeQueue +/// Simplest First-in-First-out queue, ignores priority. +class FifoRuntimeQueue { public: TaskRuntimeDataPtr pop() @@ -83,7 +83,7 @@ public: void remove(StorageID id) { auto it = std::remove_if(queue.begin(), queue.end(), - [&] (auto item) -> bool { return item->task->getStorageID() == id; }); + [&] (auto && item) -> bool { return item->task->getStorageID() == id; }); queue.erase(it, queue.end()); } @@ -94,8 +94,8 @@ private: boost::circular_buffer queue{0}; }; -/// Uses a heap to pop a task with minimal priority -class MergeMutateRuntimeQueue +/// Uses a heap to pop a task with minimal priority. +class PriorityRuntimeQueue { public: TaskRuntimeDataPtr pop() @@ -115,10 +115,7 @@ public: void remove(StorageID id) { - auto it = std::remove_if(buffer.begin(), buffer.end(), - [&] (auto item) -> bool { return item->task->getStorageID() == id; }); - buffer.erase(it, buffer.end()); - + std::erase_if(buffer, [&] (auto && item) -> bool { return item->task->getStorageID() == id; }); std::make_heap(buffer.begin(), buffer.end(), TaskRuntimeData::comparePtrByPriority); } @@ -126,7 +123,85 @@ public: bool empty() { return buffer.empty(); } private: - std::vector buffer{}; + std::vector buffer; +}; + +/// Round-robin queue, ignores priority. +class RoundRobinRuntimeQueue +{ +public: + TaskRuntimeDataPtr pop() + { + assert(buffer.size() != unused); + while (buffer[current] == nullptr) + current = (current + 1) % buffer.size(); + auto result = std::move(buffer[current]); + buffer[current] = nullptr; // mark as unused + unused++; + if (buffer.size() == unused) + { + buffer.clear(); + unused = current = 0; + } + else + current = (current + 1) % buffer.size(); + return result; + } + + // Inserts element just before round-robin pointer. + // This way inserted element will be pop()-ed last. It guarantees fairness to avoid starvation. + void push(TaskRuntimeDataPtr item) + { + if (unused == 0) + { + buffer.insert(buffer.begin() + current, std::move(item)); + current = (current + 1) % buffer.size(); + } + else // Optimization to avoid O(N) complexity -- reuse unused elements + { + assert(!buffer.empty()); + size_t pos = (current + buffer.size() - 1) % buffer.size(); + while (buffer[pos] != nullptr) + { + std::swap(item, buffer[pos]); + pos = (pos + buffer.size() - 1) % buffer.size(); + } + buffer[pos] = std::move(item); + unused--; + } + } + + void remove(StorageID id) + { + // This is similar to `std::erase_if()`, but we also track movement of `current` element + size_t saved = 0; + size_t new_current = 0; + for (size_t i = 0; i < buffer.size(); i++) + { + if (buffer[i] != nullptr && buffer[i]->task->getStorageID() != id) // erase unused and matching elements + { + if (i < current) + new_current++; + if (i != saved) + buffer[saved] = std::move(buffer[i]); + saved++; + } + } + buffer.erase(buffer.begin() + saved, buffer.end()); + current = new_current; + unused = 0; + } + + void setCapacity(size_t count) { buffer.reserve(count); } + bool empty() { return buffer.empty(); } + +private: + // Buffer can contain unused elements (nullptrs) + // Unused elements are created by pop() and reused by push() + std::vector buffer; + + size_t current = 0; // round-robin pointer + size_t unused = 0; // number of nullptr elements }; /** @@ -149,13 +224,21 @@ private: * |s| * * Each task is simply a sequence of steps. Heavier tasks have longer sequences. - * When a step of a task is executed, we move tasks to pending queue. And take another from the queue's head. - * With these architecture all small merges / mutations will be executed faster, than bigger ones. + * When a step of a task is executed, we move tasks to pending queue. And take the next task from pending queue. + * Next task is choosen from pending tasks using one of the scheduling policies (class Queue): + * 1) FifoRuntimeQueue. The simplest First-in-First-out queue. Guaranties tasks are executed in order. + * 2) PriorityRuntimeQueue. Uses heap to select task with smallest priority value. + * With this architecture all small merges / mutations will be executed faster, than bigger ones. + * WARNING: Starvation is possible in case of overload. + * 3) RoundRobinRuntimeQueue. Next task is selected, using round-robin pointer, which iterates over all tasks in rounds. + * When task is added to pending queue, it is placed just before round-robin pointer + * to given every other task an opportunity to execute one step. + * With this architecture all merges / mutations are fairly scheduled and never starved. + * All decisions regarding priorities are left to components creating tasks (e.g. SimpleMergeSelector). * - * We use boost::circular_buffer as a container for queues not to do any allocations. - * - * Another nuisance that we faces with is than background operations always interact with an associated Storage. - * So, when a Storage want to shutdown, it must wait until all its background operations are finished. + * We use boost::circular_buffer as a container for active queue to avoid allocations. + * Another nuisance that we face is that background operations always interact with an associated Storage. + * So, when a Storage wants to shutdown, it must wait until all its background operations are finished. */ template class MergeTreeBackgroundExecutor final : boost::noncopyable @@ -225,10 +308,8 @@ private: Poco::Logger * log = &Poco::Logger::get("MergeTreeBackgroundExecutor"); }; -extern template class MergeTreeBackgroundExecutor; -extern template class MergeTreeBackgroundExecutor; - -using MergeMutateBackgroundExecutor = MergeTreeBackgroundExecutor; -using OrdinaryBackgroundExecutor = MergeTreeBackgroundExecutor; +extern template class MergeTreeBackgroundExecutor; +extern template class MergeTreeBackgroundExecutor; +extern template class MergeTreeBackgroundExecutor; } diff --git a/src/Storages/MergeTree/tests/gtest_executor.cpp b/src/Storages/MergeTree/tests/gtest_executor.cpp index b89692869fd..19f6d345aa6 100644 --- a/src/Storages/MergeTree/tests/gtest_executor.cpp +++ b/src/Storages/MergeTree/tests/gtest_executor.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -55,16 +56,86 @@ private: std::uniform_int_distribution<> distribution; String name; - std::function on_completed; }; +using StepFunc = std::function; + +class LambdaExecutableTask : public IExecutableTask +{ +public: + explicit LambdaExecutableTask(const String & name_, size_t step_count_, StepFunc step_func_ = {}) + : name(name_) + , step_count(step_count_) + , step_func(step_func_) + {} + + bool executeStep() override + { + if (step_func) + step_func(name, step_count); + return --step_count; + } + + StorageID getStorageID() override + { + return {"test", name}; + } + + void onCompleted() override {} + UInt64 getPriority() override { return 0; } + +private: + String name; + size_t step_count; + StepFunc step_func; +}; + + +TEST(Executor, RoundRobin) +{ + auto executor = std::make_shared> + ( + "GTest", + 1, // threads + 100, // max_tasks + CurrentMetrics::BackgroundMergesAndMutationsPoolTask + ); + + String schedule; // mutex is not required because we have a single worker + String expected_schedule = "ABCDEABCDABCDBCDCDD"; + std::barrier barrier(2); + auto task = [&] (const String & name, size_t) + { + schedule += name; + if (schedule.size() == expected_schedule.size()) + barrier.arrive_and_wait(); + }; + + // Schedule tasks from this `init_task` to guarantee atomicity. + // Worker will see pending queue when we push all tasks. + // This is required to check scheduling properties of round-robin in deterministic way. + auto init_task = [&] (const String &, size_t) + { + executor->trySchedule(std::make_shared("A", 3, task)); + executor->trySchedule(std::make_shared("B", 4, task)); + executor->trySchedule(std::make_shared("C", 5, task)); + executor->trySchedule(std::make_shared("D", 6, task)); + executor->trySchedule(std::make_shared("E", 1, task)); + }; + + executor->trySchedule(std::make_shared("init_task", 1, init_task)); + barrier.arrive_and_wait(); // Do not finish until tasks are done + executor->wait(); + ASSERT_EQ(schedule, expected_schedule); +} + TEST(Executor, RemoveTasks) { const size_t tasks_kinds = 25; const size_t batch = 100; - auto executor = std::make_shared + auto executor = std::make_shared> ( "GTest", tasks_kinds, @@ -105,7 +176,7 @@ TEST(Executor, RemoveTasksStress) const size_t schedulers_count = 5; const size_t removers_count = 5; - auto executor = std::make_shared + auto executor = std::make_shared> ( "GTest", tasks_kinds, From cc3c76944b9db2f826331e6b05e792a715484358 Mon Sep 17 00:00:00 2001 From: serxa Date: Thu, 9 Feb 2023 19:31:54 +0000 Subject: [PATCH 11/57] remove redundant code --- src/Interpreters/Context.h | 3 +- .../MergeTree/MergeTreeBackgroundExecutor.cpp | 3 +- .../MergeTree/MergeTreeBackgroundExecutor.h | 92 +------------------ .../MergeTree/tests/gtest_executor.cpp | 6 +- 4 files changed, 10 insertions(+), 94 deletions(-) diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index 000752542c7..abd2818520c 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -130,10 +130,9 @@ using StoragePolicySelectorPtr = std::shared_ptr; template class MergeTreeBackgroundExecutor; class RoundRobinRuntimeQueue; -class FifoRuntimeQueue; using MergeMutateBackgroundExecutor = MergeTreeBackgroundExecutor; using MergeMutateBackgroundExecutorPtr = std::shared_ptr; -using OrdinaryBackgroundExecutor = MergeTreeBackgroundExecutor; +using OrdinaryBackgroundExecutor = MergeTreeBackgroundExecutor; using OrdinaryBackgroundExecutorPtr = std::shared_ptr; struct PartUUIDs; using PartUUIDsPtr = std::shared_ptr; diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp index ded48dc69ea..0fe2634eeba 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp @@ -268,8 +268,7 @@ void MergeTreeBackgroundExecutor::threadFunction() } -template class MergeTreeBackgroundExecutor; -template class MergeTreeBackgroundExecutor; template class MergeTreeBackgroundExecutor; +template class MergeTreeBackgroundExecutor; } diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h index 3030c28ef1d..5fa31edef89 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h @@ -68,7 +68,7 @@ struct TaskRuntimeData }; /// Simplest First-in-First-out queue, ignores priority. -class FifoRuntimeQueue +class RoundRobinRuntimeQueue { public: TaskRuntimeDataPtr pop() @@ -126,84 +126,6 @@ private: std::vector buffer; }; -/// Round-robin queue, ignores priority. -class RoundRobinRuntimeQueue -{ -public: - TaskRuntimeDataPtr pop() - { - assert(buffer.size() != unused); - while (buffer[current] == nullptr) - current = (current + 1) % buffer.size(); - auto result = std::move(buffer[current]); - buffer[current] = nullptr; // mark as unused - unused++; - if (buffer.size() == unused) - { - buffer.clear(); - unused = current = 0; - } - else - current = (current + 1) % buffer.size(); - return result; - } - - // Inserts element just before round-robin pointer. - // This way inserted element will be pop()-ed last. It guarantees fairness to avoid starvation. - void push(TaskRuntimeDataPtr item) - { - if (unused == 0) - { - buffer.insert(buffer.begin() + current, std::move(item)); - current = (current + 1) % buffer.size(); - } - else // Optimization to avoid O(N) complexity -- reuse unused elements - { - assert(!buffer.empty()); - size_t pos = (current + buffer.size() - 1) % buffer.size(); - while (buffer[pos] != nullptr) - { - std::swap(item, buffer[pos]); - pos = (pos + buffer.size() - 1) % buffer.size(); - } - buffer[pos] = std::move(item); - unused--; - } - } - - void remove(StorageID id) - { - // This is similar to `std::erase_if()`, but we also track movement of `current` element - size_t saved = 0; - size_t new_current = 0; - for (size_t i = 0; i < buffer.size(); i++) - { - if (buffer[i] != nullptr && buffer[i]->task->getStorageID() != id) // erase unused and matching elements - { - if (i < current) - new_current++; - if (i != saved) - buffer[saved] = std::move(buffer[i]); - saved++; - } - } - buffer.erase(buffer.begin() + saved, buffer.end()); - current = new_current; - unused = 0; - } - - void setCapacity(size_t count) { buffer.reserve(count); } - bool empty() { return buffer.empty(); } - -private: - // Buffer can contain unused elements (nullptrs) - // Unused elements are created by pop() and reused by push() - std::vector buffer; - - size_t current = 0; // round-robin pointer - size_t unused = 0; // number of nullptr elements -}; - /** * Executor for a background MergeTree related operations such as merges, mutations, fetches and so on. * It can execute only successors of ExecutableTask interface. @@ -226,15 +148,12 @@ private: * Each task is simply a sequence of steps. Heavier tasks have longer sequences. * When a step of a task is executed, we move tasks to pending queue. And take the next task from pending queue. * Next task is choosen from pending tasks using one of the scheduling policies (class Queue): - * 1) FifoRuntimeQueue. The simplest First-in-First-out queue. Guaranties tasks are executed in order. + * 1) RoundRobinRuntimeQueue. Uses boost::circular_buffer as FIFO-queue. Next task is taken from queue's head and after one step + * enqueued into queue's tail. With this architecture all merges / mutations are fairly scheduled and never starved. + * All decisions regarding priorities are left to components creating tasks (e.g. SimpleMergeSelector). * 2) PriorityRuntimeQueue. Uses heap to select task with smallest priority value. * With this architecture all small merges / mutations will be executed faster, than bigger ones. * WARNING: Starvation is possible in case of overload. - * 3) RoundRobinRuntimeQueue. Next task is selected, using round-robin pointer, which iterates over all tasks in rounds. - * When task is added to pending queue, it is placed just before round-robin pointer - * to given every other task an opportunity to execute one step. - * With this architecture all merges / mutations are fairly scheduled and never starved. - * All decisions regarding priorities are left to components creating tasks (e.g. SimpleMergeSelector). * * We use boost::circular_buffer as a container for active queue to avoid allocations. * Another nuisance that we face is that background operations always interact with an associated Storage. @@ -308,8 +227,7 @@ private: Poco::Logger * log = &Poco::Logger::get("MergeTreeBackgroundExecutor"); }; -extern template class MergeTreeBackgroundExecutor; -extern template class MergeTreeBackgroundExecutor; extern template class MergeTreeBackgroundExecutor; +extern template class MergeTreeBackgroundExecutor; } diff --git a/src/Storages/MergeTree/tests/gtest_executor.cpp b/src/Storages/MergeTree/tests/gtest_executor.cpp index 19f6d345aa6..ecf22269b0b 100644 --- a/src/Storages/MergeTree/tests/gtest_executor.cpp +++ b/src/Storages/MergeTree/tests/gtest_executor.cpp @@ -91,7 +91,7 @@ private: }; -TEST(Executor, RoundRobin) +TEST(Executor, Simple) { auto executor = std::make_shared> ( @@ -135,7 +135,7 @@ TEST(Executor, RemoveTasks) const size_t tasks_kinds = 25; const size_t batch = 100; - auto executor = std::make_shared> + auto executor = std::make_shared> ( "GTest", tasks_kinds, @@ -176,7 +176,7 @@ TEST(Executor, RemoveTasksStress) const size_t schedulers_count = 5; const size_t removers_count = 5; - auto executor = std::make_shared> + auto executor = std::make_shared> ( "GTest", tasks_kinds, From a4006ec5a1c471ce41945cc5e8c36ea318d749e8 Mon Sep 17 00:00:00 2001 From: serxa Date: Thu, 9 Feb 2023 19:49:11 +0000 Subject: [PATCH 12/57] add explanation --- src/Interpreters/Context.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index abd2818520c..85a842f904f 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -129,9 +129,14 @@ class StoragePolicySelector; using StoragePolicySelectorPtr = std::shared_ptr; template class MergeTreeBackgroundExecutor; + +/// Concurrent merges are scheduled using `RoundRobinRuntimeQueue` to ensure fair and starvation-free operation. +/// Previously in heavily overloaded shards big merges could possibly be starved by smaller +/// merges due to the use of strict priority scheduling `PriorityRuntimeQueue`. class RoundRobinRuntimeQueue; using MergeMutateBackgroundExecutor = MergeTreeBackgroundExecutor; using MergeMutateBackgroundExecutorPtr = std::shared_ptr; + using OrdinaryBackgroundExecutor = MergeTreeBackgroundExecutor; using OrdinaryBackgroundExecutorPtr = std::shared_ptr; struct PartUUIDs; From 50dace95710f98d70be75dbfaff2f8ca79d56261 Mon Sep 17 00:00:00 2001 From: flynn Date: Sat, 11 Feb 2023 13:58:19 +0000 Subject: [PATCH 13/57] fix --- src/Storages/StorageLogSettings.cpp | 32 +++++++++++++------ ...2554_log_faminy_support_storage_policy.sql | 2 ++ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/Storages/StorageLogSettings.cpp b/src/Storages/StorageLogSettings.cpp index 1c15f8ff392..71039f0ecd1 100644 --- a/src/Storages/StorageLogSettings.cpp +++ b/src/Storages/StorageLogSettings.cpp @@ -6,23 +6,37 @@ namespace DB { + +namespace ErrorCodes +{ + extern const int INVALID_SETTING_VALUE; +} + String getDiskName(ASTStorage & storage_def, ContextPtr context) { if (storage_def.settings) { SettingsChanges changes = storage_def.settings->changes; - for (const auto & change : changes) + + const auto disk_change + = std::find_if(changes.begin(), changes.end(), [&](const SettingChange & change) { return change.name == "disk"; }); + const auto storage_policy_change + = std::find_if(changes.begin(), changes.end(), [&](const SettingChange & change) { return change.name == "storage_policy"; }); + + if (disk_change != changes.end() && storage_policy_change != changes.end()) + throw Exception( + ErrorCodes::INVALID_SETTING_VALUE, "Could not specify `disk` and `storage_policy` at the same time for storage Log Family"); + + if (disk_change != changes.end()) + return disk_change->value.safeGet(); + + if (storage_policy_change != changes.end()) { - /// How about both disk and storage_policy are specified? - if (change.name == "disk") - return change.value.safeGet(); - if (change.name == "storage_policy") - { - auto policy = context->getStoragePolicy(change.value.safeGet()); - return policy->getAnyDisk()->getName(); - } + auto policy = context->getStoragePolicy(storage_policy_change->value.safeGet()); + return policy->getDisks()[0]->getName(); } } + return "default"; } diff --git a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql index 540c82c598d..adb6e19438e 100644 --- a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql +++ b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql @@ -21,3 +21,5 @@ INSERT INTO test_2554_stripelog SELECT 1; SELECT * FROM test_2554_stripelog; DROP TABLE test_2554_stripelog; + +CREATE TABLE test_2554_error (n UInt32) ENGINE = Log SETTINGS disk = 'default', storage_policy = 'default'; -- { serverError 471 } From c58b165b0fe17340a53f3f520886dbec3312eed2 Mon Sep 17 00:00:00 2001 From: serxa Date: Sat, 11 Feb 2023 16:18:42 +0000 Subject: [PATCH 14/57] add config option to select scheduling policy --- .../settings.md | 18 ++++ programs/server/Server.cpp | 2 + programs/server/config.xml | 1 + src/Interpreters/Context.cpp | 11 ++- src/Interpreters/Context.h | 10 +- .../MergeTree/MergeTreeBackgroundExecutor.cpp | 1 + .../MergeTree/MergeTreeBackgroundExecutor.h | 92 ++++++++++++++++++- .../MergeTree/tests/gtest_executor.cpp | 61 ++++++++++-- 8 files changed, 181 insertions(+), 15 deletions(-) diff --git a/docs/en/operations/server-configuration-parameters/settings.md b/docs/en/operations/server-configuration-parameters/settings.md index 7e7422f9045..f31db0f2f4d 100644 --- a/docs/en/operations/server-configuration-parameters/settings.md +++ b/docs/en/operations/server-configuration-parameters/settings.md @@ -1010,6 +1010,24 @@ Default value: 2. 3 ``` +## background_merges_mutations_scheduling_policy {#background_merges_mutations_scheduling_policy} + +Algorithm used to select next merge or mutation to be executed by background thread pool. Policy may be changed at runtime without server restart. +Could be applied from the `default` profile for backward compatibility. + +Possible values: + +- "round_robin" — Every concurrent merge and mutation is executed in round-robin order to ensure starvation-free operation. Smaller merges are completed faster than bigger ones just because they have fewer blocks to merge. +- "shortest_task_first" — Always execute smaller merge or mutation. Merges and mutations are assigned priorities based on their resulting size. Merges with smaller sizes are strictly preferred over bigger ones. This policy ensures the fastest possible merge of small parts but can lead to indefinite starvation of big merges in partitions heavily overloaded by INSERTs. + +Default value: "round_robin". + +**Example** + +```xml +shortest_task_first +``` + ## background_move_pool_size {#background_move_pool_size} Sets the number of threads performing background moves for tables with MergeTree engines. Could be increased at runtime and could be applied at server startup from the `default` profile for backward compatibility. diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index e0288415a2d..b97b48d9c68 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -1282,6 +1282,8 @@ try auto new_pool_size = config->getUInt64("background_pool_size", 16); auto new_ratio = config->getUInt64("background_merges_mutations_concurrency_ratio", 2); global_context->getMergeMutateExecutor()->increaseThreadsAndMaxTasksCount(new_pool_size, new_pool_size * new_ratio); + auto new_scheduling_policy = config->getString("background_merges_mutations_scheduling_policy", "round_robin"); + global_context->getMergeMutateExecutor()->updateSchedulingPolicy(new_scheduling_policy); } if (global_context->areBackgroundExecutorsInitialized() && config->has("background_move_pool_size")) diff --git a/programs/server/config.xml b/programs/server/config.xml index b1a1514edc8..85cb299e188 100644 --- a/programs/server/config.xml +++ b/programs/server/config.xml @@ -339,6 +339,7 @@ 16 16 2 + round_robin 8 8 8 diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 36c88abb2ee..822020c567f 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -3746,6 +3746,12 @@ void Context::initializeBackgroundExecutorsIfNeeded() else if (config.has("profiles.default.background_merges_mutations_concurrency_ratio")) background_merges_mutations_concurrency_ratio = config.getUInt64("profiles.default.background_merges_mutations_concurrency_ratio"); + String background_merges_mutations_scheduling_policy = "round_robin"; + if (config.has("background_merges_mutations_scheduling_policy")) + background_merges_mutations_scheduling_policy = config.getString("background_merges_mutations_scheduling_policy"); + else if (config.has("profiles.default.background_merges_mutations_scheduling_policy")) + background_merges_mutations_scheduling_policy = config.getString("profiles.default.background_merges_mutations_scheduling_policy"); + size_t background_move_pool_size = 8; if (config.has("background_move_pool_size")) background_move_pool_size = config.getUInt64("background_move_pool_size"); @@ -3772,8 +3778,9 @@ void Context::initializeBackgroundExecutorsIfNeeded() /*max_tasks_count*/background_pool_size * background_merges_mutations_concurrency_ratio, CurrentMetrics::BackgroundMergesAndMutationsPoolTask ); - LOG_INFO(shared->log, "Initialized background executor for merges and mutations with num_threads={}, num_tasks={}", - background_pool_size, background_pool_size * background_merges_mutations_concurrency_ratio); + shared->merge_mutate_executor->updateSchedulingPolicy(background_merges_mutations_scheduling_policy); + LOG_INFO(shared->log, "Initialized background executor for merges and mutations with num_threads={}, num_tasks={}, scheduling_policy={}", + background_pool_size, background_pool_size * background_merges_mutations_concurrency_ratio, background_merges_mutations_scheduling_policy); shared->moves_executor = std::make_shared ( diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index 1105d2a8315..8f0bbc986e5 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -130,13 +130,15 @@ using StoragePolicySelectorPtr = std::shared_ptr; template class MergeTreeBackgroundExecutor; -/// Concurrent merges are scheduled using `RoundRobinRuntimeQueue` to ensure fair and starvation-free operation. +/// Scheduling policy can be changed using `background_merges_mutations_scheduling_policy` config option. +/// By default concurrent merges are scheduled using "round_robin" to ensure fair and starvation-free operation. /// Previously in heavily overloaded shards big merges could possibly be starved by smaller -/// merges due to the use of strict priority scheduling `PriorityRuntimeQueue`. -class RoundRobinRuntimeQueue; -using MergeMutateBackgroundExecutor = MergeTreeBackgroundExecutor; +/// merges due to the use of strict priority scheduling "shortest_task_first". +class DynamicRuntimeQueue; +using MergeMutateBackgroundExecutor = MergeTreeBackgroundExecutor; using MergeMutateBackgroundExecutorPtr = std::shared_ptr; +class RoundRobinRuntimeQueue; using OrdinaryBackgroundExecutor = MergeTreeBackgroundExecutor; using OrdinaryBackgroundExecutorPtr = std::shared_ptr; struct PartUUIDs; diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp index 0fe2634eeba..d0469c35cef 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp @@ -270,5 +270,6 @@ void MergeTreeBackgroundExecutor::threadFunction() template class MergeTreeBackgroundExecutor; template class MergeTreeBackgroundExecutor; +template class MergeTreeBackgroundExecutor; } diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h index 5fa31edef89..c556a18ca54 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h @@ -6,7 +6,9 @@ #include #include #include -#include +#include +#include + #include #include @@ -78,7 +80,10 @@ public: return result; } - void push(TaskRuntimeDataPtr item) { queue.push_back(std::move(item));} + void push(TaskRuntimeDataPtr item) + { + queue.push_back(std::move(item)); + } void remove(StorageID id) { @@ -90,6 +95,8 @@ public: void setCapacity(size_t count) { queue.set_capacity(count); } bool empty() { return queue.empty(); } + static constexpr std::string_view name = "round_robin"; + private: boost::circular_buffer queue{0}; }; @@ -122,10 +129,82 @@ public: void setCapacity(size_t count) { buffer.reserve(count); } bool empty() { return buffer.empty(); } + static constexpr std::string_view name = "shortest_task_first"; + private: std::vector buffer; }; +/// Queue that can dynamically change scheduling policy +template +class DynamicRuntimeQueueImpl +{ +public: + TaskRuntimeDataPtr pop() + { + return std::visit([&] (auto && queue) { return queue.pop(); }, impl); + } + + void push(TaskRuntimeDataPtr item) + { + std::visit([&] (auto && queue) { queue.push(std::move(item)); }, impl); + } + + void remove(StorageID id) + { + std::visit([&] (auto && queue) { queue.remove(id); }, impl); + } + + void setCapacity(size_t count) + { + capacity = count; + std::visit([&] (auto && queue) { queue.setCapacity(count); }, impl); + } + + bool empty() + { + return std::visit([&] (auto && queue) { return queue.empty(); }, impl); + } + + // Change policy. It does nothing if new policy is unknown or equals current policy. + void updatePolicy(std::string_view name) + { + // We use this double lambda trick to generate code for all possible pairs of types of old and new queue. + // If types are different it moves tasks from old queue to new one using corresponding pop() and push() + resolve(name, [&] (std::in_place_type_t) + { + std::visit([&] (auto && queue) + { + if constexpr (std::is_same_v) + return; // The same policy + NewQueue new_queue; + new_queue.setCapacity(capacity); + while (!queue.empty()) + new_queue.push(queue.pop()); + impl = std::move(new_queue); + }, impl); + }); + } + +private: + // Find policy with specified `name` and call `func()` if found. + // Tag `std::in_place_type_t` used to help templated lambda to deduce type T w/o creating its instance + template + void resolve(std::string_view name, Func && func) + { + if (T::name == name) + return func(std::in_place_type); + if constexpr (sizeof...(Ts)) + return resolve(name, std::forward(func)); + } + + std::variant impl; + size_t capacity; +}; + +// Avoid typedef and alias to facilitate forward declaration +class DynamicRuntimeQueue : public DynamicRuntimeQueueImpl {}; + /** * Executor for a background MergeTree related operations such as merges, mutations, fetches and so on. * It can execute only successors of ExecutableTask interface. @@ -206,6 +285,14 @@ public: void removeTasksCorrespondingToStorage(StorageID id); void wait(); + /// Update + void updateSchedulingPolicy(std::string_view new_policy) + requires requires(Queue queue) { queue.updatePolicy(new_policy); } // Because we use explicit template instantiation + { + std::lock_guard lock(mutex); + pending.updatePolicy(new_policy); + } + private: String name; size_t threads_count TSA_GUARDED_BY(mutex) = 0; @@ -229,5 +316,6 @@ private: extern template class MergeTreeBackgroundExecutor; extern template class MergeTreeBackgroundExecutor; +extern template class MergeTreeBackgroundExecutor; } diff --git a/src/Storages/MergeTree/tests/gtest_executor.cpp b/src/Storages/MergeTree/tests/gtest_executor.cpp index ecf22269b0b..6a73be28d8c 100644 --- a/src/Storages/MergeTree/tests/gtest_executor.cpp +++ b/src/Storages/MergeTree/tests/gtest_executor.cpp @@ -9,6 +9,7 @@ #include #include + using namespace DB; namespace CurrentMetrics @@ -63,10 +64,11 @@ using StepFunc = std::function; class LambdaExecutableTask : public IExecutableTask { public: - explicit LambdaExecutableTask(const String & name_, size_t step_count_, StepFunc step_func_ = {}) + explicit LambdaExecutableTask(const String & name_, size_t step_count_, StepFunc step_func_ = {}, UInt64 priority_ = 0) : name(name_) , step_count(step_count_) , step_func(step_func_) + , priority(priority_) {} bool executeStep() override @@ -82,12 +84,14 @@ public: } void onCompleted() override {} - UInt64 getPriority() override { return 0; } + + UInt64 getPriority() override { return priority; } private: String name; size_t step_count; StepFunc step_func; + UInt64 priority; }; @@ -116,11 +120,11 @@ TEST(Executor, Simple) // This is required to check scheduling properties of round-robin in deterministic way. auto init_task = [&] (const String &, size_t) { - executor->trySchedule(std::make_shared("A", 3, task)); - executor->trySchedule(std::make_shared("B", 4, task)); - executor->trySchedule(std::make_shared("C", 5, task)); - executor->trySchedule(std::make_shared("D", 6, task)); - executor->trySchedule(std::make_shared("E", 1, task)); + executor->trySchedule(std::make_shared("A", 3, task)); + executor->trySchedule(std::make_shared("B", 4, task)); + executor->trySchedule(std::make_shared("C", 5, task)); + executor->trySchedule(std::make_shared("D", 6, task)); + executor->trySchedule(std::make_shared("E", 1, task)); }; executor->trySchedule(std::make_shared("init_task", 1, init_task)); @@ -222,3 +226,46 @@ TEST(Executor, RemoveTasksStress) ASSERT_EQ(CurrentMetrics::values[CurrentMetrics::BackgroundMergesAndMutationsPoolTask], 0); } + + +TEST(Executor, UpdatePolicy) +{ + auto executor = std::make_shared> + ( + "GTest", + 1, // threads + 100, // max_tasks + CurrentMetrics::BackgroundMergesAndMutationsPoolTask + ); + + String schedule; // mutex is not required because we have a single worker + String expected_schedule = "ABCDEDDDDDCCBACBACB"; + std::barrier barrier(2); + auto task = [&] (const String & name, size_t) + { + schedule += name; + if (schedule.size() == 5) + executor->updateSchedulingPolicy(PriorityRuntimeQueue::name); + if (schedule.size() == 12) + executor->updateSchedulingPolicy(RoundRobinRuntimeQueue::name); + if (schedule.size() == expected_schedule.size()) + barrier.arrive_and_wait(); + }; + + // Schedule tasks from this `init_task` to guarantee atomicity. + // Worker will see pending queue when we push all tasks. + // This is required to check scheduling properties in a deterministic way. + auto init_task = [&] (const String &, size_t) + { + executor->trySchedule(std::make_shared("A", 3, task, 5)); + executor->trySchedule(std::make_shared("B", 4, task, 4)); + executor->trySchedule(std::make_shared("C", 5, task, 3)); + executor->trySchedule(std::make_shared("D", 6, task, 2)); + executor->trySchedule(std::make_shared("E", 1, task, 1)); + }; + + executor->trySchedule(std::make_shared("init_task", 1, init_task)); + barrier.arrive_and_wait(); // Do not finish until tasks are done + executor->wait(); + ASSERT_EQ(schedule, expected_schedule); +} From d2f8377c479b5f3024871e13b42fd457ef1a7002 Mon Sep 17 00:00:00 2001 From: serxa Date: Sat, 11 Feb 2023 16:45:45 +0000 Subject: [PATCH 15/57] fix check --- src/Storages/MergeTree/MergeTreeBackgroundExecutor.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h index c556a18ca54..2e0a24259ad 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h @@ -19,6 +19,7 @@ #include #include + namespace DB { namespace ErrorCodes @@ -175,7 +176,7 @@ public: { std::visit([&] (auto && queue) { - if constexpr (std::is_same_v) + if constexpr (std::is_same_v, NewQueue>) return; // The same policy NewQueue new_queue; new_queue.setCapacity(capacity); From c17dc8e284c55e72b0be0648e419dc2d8f4e0640 Mon Sep 17 00:00:00 2001 From: serxa Date: Sat, 11 Feb 2023 16:48:47 +0000 Subject: [PATCH 16/57] fix --- docker/test/stress/run.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/test/stress/run.sh b/docker/test/stress/run.sh index 78d27dddb65..41ff17bbe10 100644 --- a/docker/test/stress/run.sh +++ b/docker/test/stress/run.sh @@ -642,7 +642,7 @@ if [ "$DISABLE_BC_CHECK" -ne "1" ]; then -e "} TCPHandler: Code:" \ -e "} executeQuery: Code:" \ -e "Missing columns: 'v3' while processing query: 'v3, k, v1, v2, p'" \ - -e "[Queue = DB::RoundRobinRuntimeQueue]: Code: 235. DB::Exception: Part" \ + -e "[Queue = DB::DynamicRuntimeQueue]: Code: 235. DB::Exception: Part" \ -e "The set of parts restored in place of" \ -e "(ReplicatedMergeTreeAttachThread): Initialization failed. Error" \ -e "Code: 269. DB::Exception: Destination table is myself" \ From 9a7f70f595ef3fa77871d0c5de9386be8f9b5b33 Mon Sep 17 00:00:00 2001 From: serxa Date: Sat, 11 Feb 2023 16:54:57 +0000 Subject: [PATCH 17/57] fix comment --- src/Storages/MergeTree/MergeTreeBackgroundExecutor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h index 2e0a24259ad..3e2f5525097 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h @@ -286,7 +286,7 @@ public: void removeTasksCorrespondingToStorage(StorageID id); void wait(); - /// Update + /// Update scheduling policy for pending tasks. It does nothing if `new_policy` is the same or unknown. void updateSchedulingPolicy(std::string_view new_policy) requires requires(Queue queue) { queue.updatePolicy(new_policy); } // Because we use explicit template instantiation { From df6b4c43721de6e5a85ad8ac6c45b687a450f830 Mon Sep 17 00:00:00 2001 From: serxa Date: Sat, 11 Feb 2023 20:18:19 +0000 Subject: [PATCH 18/57] fix typo --- src/Storages/MergeTree/MergeTreeBackgroundExecutor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h index 3e2f5525097..7745e5de334 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h @@ -227,7 +227,7 @@ class DynamicRuntimeQueue : public DynamicRuntimeQueueImpl Date: Tue, 14 Feb 2023 12:42:58 +0100 Subject: [PATCH 19/57] Fix test --- src/Storages/RabbitMQ/RabbitMQHandler.cpp | 5 +++-- src/Storages/RabbitMQ/RabbitMQHandler.h | 2 +- src/Storages/RabbitMQ/RabbitMQProducer.cpp | 12 ++++++++++-- src/Storages/RabbitMQ/RabbitMQProducer.h | 2 +- tests/integration/test_storage_rabbitmq/test.py | 1 - 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/Storages/RabbitMQ/RabbitMQHandler.cpp b/src/Storages/RabbitMQ/RabbitMQHandler.cpp index cbbcd87f79e..5258b9fa7dc 100644 --- a/src/Storages/RabbitMQ/RabbitMQHandler.cpp +++ b/src/Storages/RabbitMQ/RabbitMQHandler.cpp @@ -46,11 +46,12 @@ void RabbitMQHandler::startLoop() loop_running.store(false); } -void RabbitMQHandler::iterateLoop() +bool RabbitMQHandler::iterateLoop() { std::unique_lock lock(startup_mutex, std::defer_lock); if (lock.try_lock()) - uv_run(loop, UV_RUN_NOWAIT); + return uv_run(loop, UV_RUN_NOWAIT); + return 0; } /// Do not need synchronization as in iterateLoop(), because this method is used only for diff --git a/src/Storages/RabbitMQ/RabbitMQHandler.h b/src/Storages/RabbitMQ/RabbitMQHandler.h index 25b32a29f58..4a7c3fc7f78 100644 --- a/src/Storages/RabbitMQ/RabbitMQHandler.h +++ b/src/Storages/RabbitMQ/RabbitMQHandler.h @@ -34,7 +34,7 @@ public: /// Loop to wait for small tasks in a non-blocking mode. /// Adds synchronization with main background loop. - void iterateLoop(); + bool iterateLoop(); /// Loop to wait for small tasks in a blocking mode. /// No synchronization is done with the main loop thread. diff --git a/src/Storages/RabbitMQ/RabbitMQProducer.cpp b/src/Storages/RabbitMQ/RabbitMQProducer.cpp index 2d0719a3293..fc49f74b545 100644 --- a/src/Storages/RabbitMQ/RabbitMQProducer.cpp +++ b/src/Storages/RabbitMQ/RabbitMQProducer.cpp @@ -15,6 +15,7 @@ namespace DB static const auto BATCH = 1000; static const auto RETURNED_LIMIT = 50000; +static const auto FINISH_PRODUCER_NUM_TRIES = 50; namespace ErrorCodes { @@ -254,13 +255,20 @@ void RabbitMQProducer::startProducingTaskLoop() } } + int res = 0; + size_t try_num = 0; + while (++try_num <= FINISH_PRODUCER_NUM_TRIES && (res = iterateEventLoop())) + { + LOG_TEST(log, "Waiting for pending callbacks to finish (count: {}, try: {})", res, try_num); + } + LOG_DEBUG(log, "Producer on channel {} completed", channel_id); } -void RabbitMQProducer::iterateEventLoop() +bool RabbitMQProducer::iterateEventLoop() { - connection.getHandler().iterateLoop(); + return connection.getHandler().iterateLoop(); } } diff --git a/src/Storages/RabbitMQ/RabbitMQProducer.h b/src/Storages/RabbitMQ/RabbitMQProducer.h index 77dadd346ed..e3691c005ee 100644 --- a/src/Storages/RabbitMQ/RabbitMQProducer.h +++ b/src/Storages/RabbitMQ/RabbitMQProducer.h @@ -43,7 +43,7 @@ private: void stopProducingTask() override; void finishImpl() override; - void iterateEventLoop(); + bool iterateEventLoop(); void startProducingTaskLoop() override; void setupChannel(); void removeRecord(UInt64 received_delivery_tag, bool multiple, bool republish); diff --git a/tests/integration/test_storage_rabbitmq/test.py b/tests/integration/test_storage_rabbitmq/test.py index fa3f7f6102d..030d9507d4f 100644 --- a/tests/integration/test_storage_rabbitmq/test.py +++ b/tests/integration/test_storage_rabbitmq/test.py @@ -1019,7 +1019,6 @@ def test_rabbitmq_many_inserts(rabbitmq_cluster): ), "ClickHouse lost some messages: {}".format(result) -@pytest.mark.skip(reason="Flaky") def test_rabbitmq_overloaded_insert(rabbitmq_cluster): instance.query( """ From 043b394ff09457ee90d228380fdbdf2f1ddfba2f Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Tue, 14 Feb 2023 13:36:34 +0000 Subject: [PATCH 20/57] Remove unneeded move --- src/IO/S3/Client.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/IO/S3/Client.cpp b/src/IO/S3/Client.cpp index 6b87ca691be..5c0539ee486 100644 --- a/src/IO/S3/Client.cpp +++ b/src/IO/S3/Client.cpp @@ -597,8 +597,8 @@ std::unique_ptr ClientFactory::create( // NOLINT client_configuration.retryStrategy = std::make_shared(std::move(client_configuration.retryStrategy)); return Client::create( client_configuration.s3_max_redirects, - std::move(credentials_provider), - std::move(client_configuration), // Client configuration. + credentials_provider, + client_configuration, // Client configuration. Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never, is_virtual_hosted_style || client_configuration.endpointOverride.empty() /// Use virtual addressing if endpoint is not specified. ); From 5bda358fb72ff6e0bcea14144fd91dccee01e844 Mon Sep 17 00:00:00 2001 From: kssenii Date: Tue, 14 Feb 2023 15:14:11 +0100 Subject: [PATCH 21/57] Follow-up to #46168 --- .../QueryPlan/ReadFromMergeTree.cpp | 61 ++++- src/Storages/MergeTree/IMergeTreeReadPool.h | 29 +++ .../MergeTree/MergeTreePrefetchedReadPool.cpp | 15 +- .../MergeTree/MergeTreePrefetchedReadPool.h | 8 + src/Storages/MergeTree/MergeTreeReadPool.cpp | 71 +++--- src/Storages/MergeTree/MergeTreeReadPool.h | 223 ++++++++---------- .../MergeTree/MergeTreeReaderCompact.cpp | 32 ++- .../MergeTree/MergeTreeReaderCompact.h | 8 + .../MergeTreeThreadSelectProcessor.cpp | 5 +- 9 files changed, 271 insertions(+), 181 deletions(-) create mode 100644 src/Storages/MergeTree/IMergeTreeReadPool.h diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index ff0c5002e09..a54134a1152 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -283,7 +283,6 @@ Pipe ReadFromMergeTree::readFromPool( total_rows = query_info.limit; const auto & settings = context->getSettingsRef(); - MergeTreeReadPool::BackoffSettings backoff_settings(settings); /// round min_marks_to_read up to nearest multiple of block_size expressed in marks /// If granularity is adaptive it doesn't make sense @@ -295,18 +294,54 @@ Pipe ReadFromMergeTree::readFromPool( / max_block_size * max_block_size / fixed_index_granularity; } - auto pool = std::make_shared( - max_streams, - sum_marks, - min_marks_for_concurrent_read, - std::move(parts_with_range), - storage_snapshot, - prewhere_info, - required_columns, - virt_column_names, - backoff_settings, - settings.preferred_block_size_bytes, - false); + bool all_parts_are_remote = true; + bool all_parts_are_local = true; + for (const auto & part : parts_with_range) + { + const bool is_remote = part.data_part->isStoredOnRemoteDisk(); + all_parts_are_local &= !is_remote; + all_parts_are_remote &= is_remote; + } + + MergeTreeReadPoolPtr pool; + + if ((all_parts_are_remote + && settings.allow_prefetched_read_pool_for_remote_filesystem + && MergeTreePrefetchedReadPool::checkReadMethodAllowed(reader_settings.read_settings.remote_fs_method)) + || (!all_parts_are_local + && settings.allow_prefetched_read_pool_for_local_filesystem + && MergeTreePrefetchedReadPool::checkReadMethodAllowed(reader_settings.read_settings.remote_fs_method))) + { + pool = std::make_shared( + max_streams, + sum_marks, + min_marks_for_concurrent_read, + std::move(parts_with_range), + storage_snapshot, + prewhere_info, + required_columns, + virt_column_names, + settings.preferred_block_size_bytes, + reader_settings, + context, + use_uncompressed_cache, + all_parts_are_remote, + *data.getSettings()); + } + else + { + pool = std::make_shared( + max_streams, + sum_marks, + min_marks_for_concurrent_read, + std::move(parts_with_range), + storage_snapshot, + prewhere_info, + required_columns, + virt_column_names, + context, + false); + } auto * logger = &Poco::Logger::get(data.getLogName() + " (SelectExecutor)"); LOG_DEBUG(logger, "Reading approx. {} rows with {} streams", total_rows, max_streams); diff --git a/src/Storages/MergeTree/IMergeTreeReadPool.h b/src/Storages/MergeTree/IMergeTreeReadPool.h new file mode 100644 index 00000000000..efdfca51c0a --- /dev/null +++ b/src/Storages/MergeTree/IMergeTreeReadPool.h @@ -0,0 +1,29 @@ +#pragma once + +#include +#include +#include +#include + + +namespace DB +{ +struct MergeTreeReadTask; +using MergeTreeReadTaskPtr = std::unique_ptr; + + +class IMergeTreeReadPool : private boost::noncopyable +{ +public: + virtual ~IMergeTreeReadPool() = default; + + virtual Block getHeader() const = 0; + + virtual MergeTreeReadTaskPtr getTask(size_t thread) = 0; + + virtual void profileFeedback(ReadBufferFromFileBase::ProfileInfo info) = 0; +}; + +using MergeTreeReadPoolPtr = std::shared_ptr; + +} diff --git a/src/Storages/MergeTree/MergeTreePrefetchedReadPool.cpp b/src/Storages/MergeTree/MergeTreePrefetchedReadPool.cpp index d7cbf18c115..7ef1e436ec8 100644 --- a/src/Storages/MergeTree/MergeTreePrefetchedReadPool.cpp +++ b/src/Storages/MergeTree/MergeTreePrefetchedReadPool.cpp @@ -39,16 +39,7 @@ MergeTreePrefetchedReadPool::MergeTreePrefetchedReadPool( bool use_uncompressed_cache_, bool is_remote_read_, const MergeTreeSettings & storage_settings_) - : IMergeTreeReadPool( - storage_snapshot_, - column_names_, - virtual_column_names_, - min_marks_for_concurrent_read_, - prewhere_info_, - parts_, - (preferred_block_size_bytes_ > 0), - /*do_not_steal_tasks_*/false) - , WithContext(context_) + : WithContext(context_) , log(&Poco::Logger::get("MergeTreePrefetchedReadPool(" + (parts_.empty() ? "" : parts_.front().data_part->storage.getStorageID().getNameForLogs()) + ")")) , header(storage_snapshot_->getSampleBlockForColumns(column_names_)) , mark_cache(context_->getGlobalContext()->getMarkCache().get()) @@ -57,6 +48,10 @@ MergeTreePrefetchedReadPool::MergeTreePrefetchedReadPool( , profile_callback([this](ReadBufferFromFileBase::ProfileInfo info_) { profileFeedback(info_); }) , index_granularity_bytes(storage_settings_.index_granularity_bytes) , fixed_index_granularity(storage_settings_.index_granularity) + , storage_snapshot(storage_snapshot_) + , column_names(column_names_) + , virtual_column_names(virtual_column_names_) + , prewhere_info(prewhere_info_) , is_remote_read(is_remote_read_) , prefetch_threadpool(getContext()->getPrefetchThreadpool()) { diff --git a/src/Storages/MergeTree/MergeTreePrefetchedReadPool.h b/src/Storages/MergeTree/MergeTreePrefetchedReadPool.h index ca12739ef6b..8f46593a3c6 100644 --- a/src/Storages/MergeTree/MergeTreePrefetchedReadPool.h +++ b/src/Storages/MergeTree/MergeTreePrefetchedReadPool.h @@ -84,12 +84,20 @@ private: ReadBufferFromFileBase::ProfileCallback profile_callback; size_t index_granularity_bytes; size_t fixed_index_granularity; + + StorageSnapshotPtr storage_snapshot; + const Names column_names; + const Names virtual_column_names; + PrewhereInfoPtr prewhere_info; + RangesInDataParts parts_ranges; + [[ maybe_unused ]] const bool is_remote_read; ThreadPool & prefetch_threadpool; PartsInfos parts_infos; ThreadsTasks threads_tasks; + std::mutex mutex; struct TaskHolder { diff --git a/src/Storages/MergeTree/MergeTreeReadPool.cpp b/src/Storages/MergeTree/MergeTreeReadPool.cpp index ebb87be0c41..dd551879fa3 100644 --- a/src/Storages/MergeTree/MergeTreeReadPool.cpp +++ b/src/Storages/MergeTree/MergeTreeReadPool.cpp @@ -20,7 +20,47 @@ namespace ErrorCodes namespace DB { -std::vector IMergeTreeReadPool::fillPerPartInfo(const RangesInDataParts & parts) +MergeTreeReadPool::MergeTreeReadPool( + size_t threads_, + size_t sum_marks_, + size_t min_marks_for_concurrent_read_, + RangesInDataParts && parts_, + const StorageSnapshotPtr & storage_snapshot_, + const PrewhereInfoPtr & prewhere_info_, + const Names & column_names_, + const Names & virtual_column_names_, + ContextPtr context_, + bool do_not_steal_tasks_) + : storage_snapshot(storage_snapshot_) + , column_names(column_names_) + , virtual_column_names(virtual_column_names_) + , min_marks_for_concurrent_read(min_marks_for_concurrent_read_) + , prewhere_info(prewhere_info_) + , parts_ranges(std::move(parts_)) + , predict_block_size_bytes(context_->getSettingsRef().preferred_block_size_bytes > 0) + , do_not_steal_tasks(do_not_steal_tasks_) + , backoff_settings{context_->getSettingsRef()} + , backoff_state{threads_} +{ + /// parts don't contain duplicate MergeTreeDataPart's. + const auto per_part_sum_marks = fillPerPartInfo( + parts_ranges, storage_snapshot, is_part_on_remote_disk, + do_not_steal_tasks, predict_block_size_bytes, + column_names, virtual_column_names, prewhere_info, per_part_params); + + fillPerThreadInfo(threads_, sum_marks_, per_part_sum_marks, parts_ranges); +} + +std::vector MergeTreeReadPool::fillPerPartInfo( + const RangesInDataParts & parts, + const StorageSnapshotPtr & storage_snapshot, + std::vector & is_part_on_remote_disk, + bool & do_not_steal_tasks, + bool & predict_block_size_bytes, + const Names & column_names, + const Names & virtual_column_names, + const PrewhereInfoPtr & prewhere_info, + std::vector & per_part_params) { std::vector per_part_sum_marks; Block sample_block = storage_snapshot->metadata->getSampleBlock(); @@ -65,35 +105,6 @@ std::vector IMergeTreeReadPool::fillPerPartInfo(const RangesInDataParts return per_part_sum_marks; } -MergeTreeReadPool::MergeTreeReadPool( - size_t threads_, - size_t sum_marks_, - size_t min_marks_for_concurrent_read_, - RangesInDataParts && parts_, - const StorageSnapshotPtr & storage_snapshot_, - const PrewhereInfoPtr & prewhere_info_, - const Names & column_names_, - const Names & virtual_column_names_, - const BackoffSettings & backoff_settings_, - size_t preferred_block_size_bytes_, - bool do_not_steal_tasks_) - : IMergeTreeReadPool( - storage_snapshot_, - column_names_, - virtual_column_names_, - min_marks_for_concurrent_read_, - prewhere_info_, - std::move(parts_), - (preferred_block_size_bytes_ > 0), - do_not_steal_tasks_) - , backoff_settings{backoff_settings_} - , backoff_state{threads_} -{ - /// parts don't contain duplicate MergeTreeDataPart's. - const auto per_part_sum_marks = fillPerPartInfo(parts_ranges); - fillPerThreadInfo(threads_, sum_marks_, per_part_sum_marks, parts_ranges); -} - MergeTreeReadTaskPtr MergeTreeReadPool::getTask(size_t thread) { diff --git a/src/Storages/MergeTree/MergeTreeReadPool.h b/src/Storages/MergeTree/MergeTreeReadPool.h index ad9d8b2e225..83b2587568a 100644 --- a/src/Storages/MergeTree/MergeTreeReadPool.h +++ b/src/Storages/MergeTree/MergeTreeReadPool.h @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -14,77 +15,42 @@ namespace DB { - -using MergeTreeReadTaskPtr = std::unique_ptr; - - -class IMergeTreeReadPool -{ -public: - IMergeTreeReadPool( - StorageSnapshotPtr storage_snapshot_, - Names column_names_, - Names virtual_column_names_, - size_t min_marks_for_concurrent_read_, - PrewhereInfoPtr prewhere_info_, - RangesInDataParts parts_ranges_, - bool predict_block_size_bytes_, - bool do_not_steal_tasks_) - : storage_snapshot(storage_snapshot_) - , column_names(column_names_) - , virtual_column_names(virtual_column_names_) - , min_marks_for_concurrent_read(min_marks_for_concurrent_read_) - , prewhere_info(prewhere_info_) - , parts_ranges(parts_ranges_) - , predict_block_size_bytes(predict_block_size_bytes_) - , do_not_steal_tasks(do_not_steal_tasks_) - {} - - virtual MergeTreeReadTaskPtr getTask(size_t thread) = 0; - virtual Block getHeader() const = 0; - virtual void profileFeedback(ReadBufferFromFileBase::ProfileInfo info) = 0; - virtual ~IMergeTreeReadPool() = default; - -protected: - - std::vector fillPerPartInfo(const RangesInDataParts & parts); - - /// Initialized in constructor - StorageSnapshotPtr storage_snapshot; - const Names column_names; - const Names virtual_column_names; - size_t min_marks_for_concurrent_read{0}; - PrewhereInfoPtr prewhere_info; - RangesInDataParts parts_ranges; - bool predict_block_size_bytes; - bool do_not_steal_tasks; - - struct PerPartParams - { - MergeTreeReadTaskColumns task_columns; - NameSet column_name_set; - MergeTreeBlockSizePredictorPtr size_predictor; - RangesInDataPart data_part; - }; - - std::vector per_part_params; - std::vector is_part_on_remote_disk; - - mutable std::mutex mutex; -}; - -using IMergeTreeReadPoolPtr = std::shared_ptr; - -/** Provides read tasks for MergeTreeThreadSelectProcessor`s in fine-grained batches, allowing for more - * uniform distribution of work amongst multiple threads. All parts and their ranges are divided into `threads` - * workloads with at most `sum_marks / threads` marks. Then, threads are performing reads from these workloads - * in "sequential" manner, requesting work in small batches. As soon as some thread has exhausted - * it's workload, it either is signaled that no more work is available (`do_not_steal_tasks == false`) or - * continues taking small batches from other threads' workloads (`do_not_steal_tasks == true`). +/** Provides read tasks for MergeTreeThreadSelectProcessor`s in fine-grained batches, allowing for more + * uniform distribution of work amongst multiple threads. All parts and their ranges are divided into `threads` + * workloads with at most `sum_marks / threads` marks. Then, threads are performing reads from these workloads + * in "sequential" manner, requesting work in small batches. As soon as some thread has exhausted + * it's workload, it either is signaled that no more work is available (`do_not_steal_tasks == false`) or + * continues taking small batches from other threads' workloads (`do_not_steal_tasks == true`). */ -class MergeTreeReadPool final: public IMergeTreeReadPool, private boost::noncopyable +class MergeTreeReadPool : public IMergeTreeReadPool { public: + struct BackoffSettings; + + MergeTreeReadPool( + size_t threads_, + size_t sum_marks_, + size_t min_marks_for_concurrent_read_, + RangesInDataParts && parts_, + const StorageSnapshotPtr & storage_snapshot_, + const PrewhereInfoPtr & prewhere_info_, + const Names & column_names_, + const Names & virtual_column_names_, + ContextPtr context_, + bool do_not_steal_tasks_ = false); + + ~MergeTreeReadPool() override = default; + + MergeTreeReadTaskPtr getTask(size_t thread) override; + + /** Each worker could call this method and pass information about read performance. + * If read performance is too low, pool could decide to lower number of threads: do not assign more tasks to several threads. + * This allows to overcome excessive load to disk subsystem, when reads are not from page cache. + */ + void profileFeedback(ReadBufferFromFileBase::ProfileInfo info) override; + + Block getHeader() const override; + /** Pull could dynamically lower (backoff) number of threads, if read operation are too slow. * Settings for that backoff. */ @@ -107,46 +73,51 @@ public: max_throughput(settings.read_backoff_max_throughput), min_interval_between_events_ms(settings.read_backoff_min_interval_between_events_ms.totalMilliseconds()), min_events(settings.read_backoff_min_events), - min_concurrency(settings.read_backoff_min_concurrency) - { - } + min_concurrency(settings.read_backoff_min_concurrency) {} BackoffSettings() : min_read_latency_ms(0) {} }; - BackoffSettings backoff_settings; + struct PerPartParams + { + MergeTreeReadTaskColumns task_columns; + NameSet column_name_set; + MergeTreeBlockSizePredictorPtr size_predictor; + RangesInDataPart data_part; + }; - MergeTreeReadPool( - size_t threads_, - size_t sum_marks_, - size_t min_marks_for_concurrent_read_, - RangesInDataParts && parts_, - const StorageSnapshotPtr & storage_snapshot_, - const PrewhereInfoPtr & prewhere_info_, - const Names & column_names_, - const Names & virtual_column_names_, - const BackoffSettings & backoff_settings_, - size_t preferred_block_size_bytes_, - bool do_not_steal_tasks_ = false); - - ~MergeTreeReadPool() override = default; - - MergeTreeReadTaskPtr getTask(size_t thread) override; - - /** Each worker could call this method and pass information about read performance. - * If read performance is too low, pool could decide to lower number of threads: do not assign more tasks to several threads. - * This allows to overcome excessive load to disk subsystem, when reads are not from page cache. - */ - void profileFeedback(ReadBufferFromFileBase::ProfileInfo info) override; - - Block getHeader() const override; + static std::vector fillPerPartInfo( + const RangesInDataParts & parts, + const StorageSnapshotPtr & storage_snapshot, + std::vector & is_part_on_remote_disk, + bool & do_not_steal_tasks, + bool & predict_block_size_bytes, + const Names & column_names, + const Names & virtual_column_names, + const PrewhereInfoPtr & prewhere_info, + std::vector & per_part_params); private: - void fillPerThreadInfo( size_t threads, size_t sum_marks, std::vector per_part_sum_marks, const RangesInDataParts & parts); + /// Initialized in constructor + StorageSnapshotPtr storage_snapshot; + const Names column_names; + const Names virtual_column_names; + size_t min_marks_for_concurrent_read{0}; + PrewhereInfoPtr prewhere_info; + RangesInDataParts parts_ranges; + bool predict_block_size_bytes; + bool do_not_steal_tasks; + + std::vector per_part_params; + std::vector is_part_on_remote_disk; + + BackoffSettings backoff_settings; + + mutable std::mutex mutex; /// State to track numbers of slow reads. struct BackoffState { @@ -156,7 +127,6 @@ private: explicit BackoffState(size_t threads) : current_threads(threads) {} }; - BackoffState backoff_state; struct Part @@ -185,9 +155,7 @@ private: }; -using MergeTreeReadPoolPtr = std::shared_ptr; - -class MergeTreeReadPoolParallelReplicas : public IMergeTreeReadPool, private boost::noncopyable +class MergeTreeReadPoolParallelReplicas : public IMergeTreeReadPool { public: @@ -199,21 +167,19 @@ public: const PrewhereInfoPtr & prewhere_info_, const Names & column_names_, const Names & virtual_column_names_, - size_t min_marks_for_concurrent_read_ - ) - : IMergeTreeReadPool( - storage_snapshot_, - column_names_, - virtual_column_names_, - min_marks_for_concurrent_read_, - prewhere_info_, - parts_, - /*predict_block_size*/false, - /*do_not_steal_tasks*/false) - , extension(extension_) - , threads(threads_) + size_t min_marks_for_concurrent_read_) + : extension(extension_) + , threads(threads_) + , prewhere_info(prewhere_info_) + , storage_snapshot(storage_snapshot_) + , min_marks_for_concurrent_read(min_marks_for_concurrent_read_) + , column_names(column_names_) + , virtual_column_names(virtual_column_names_) + , parts_ranges(std::move(parts_)) { - fillPerPartInfo(parts_ranges); + MergeTreeReadPool::fillPerPartInfo( + parts_ranges, storage_snapshot, is_part_on_remote_disk, do_not_steal_tasks, + predict_block_size_bytes, column_names, virtual_column_names, prewhere_info, per_part_params); extension.all_callback({ .description = parts_ranges.getDescriptions(), @@ -223,8 +189,10 @@ public: ~MergeTreeReadPoolParallelReplicas() override; - MergeTreeReadTaskPtr getTask(size_t thread) override; Block getHeader() const override; + + MergeTreeReadTaskPtr getTask(size_t thread) override; + void profileFeedback(ReadBufferFromFileBase::ProfileInfo) override {} private: @@ -234,6 +202,20 @@ private: size_t threads; bool no_more_tasks_available{false}; Poco::Logger * log = &Poco::Logger::get("MergeTreeReadPoolParallelReplicas"); + + std::mutex mutex; + + PrewhereInfoPtr prewhere_info; + StorageSnapshotPtr storage_snapshot; + size_t min_marks_for_concurrent_read; + const Names column_names; + const Names virtual_column_names; + RangesInDataParts parts_ranges; + + bool do_not_steal_tasks = false; + bool predict_block_size_bytes = false; + std::vector is_part_on_remote_disk; + std::vector per_part_params; }; using MergeTreeReadPoolParallelReplicasPtr = std::shared_ptr; @@ -247,10 +229,10 @@ public: ParallelReadingExtension extension_, CoordinationMode mode_, size_t min_marks_for_concurrent_read_) - : parts_ranges(parts_) - , extension(extension_) - , mode(mode_) - , min_marks_for_concurrent_read(min_marks_for_concurrent_read_) + : parts_ranges(parts_) + , extension(extension_) + , mode(mode_) + , min_marks_for_concurrent_read(min_marks_for_concurrent_read_) { for (const auto & part : parts_ranges) request.push_back({part.data_part->info, MarkRanges{}}); @@ -266,6 +248,7 @@ public: MarkRanges getNewTask(RangesInDataPartDescription description); + RangesInDataParts parts_ranges; ParallelReadingExtension extension; CoordinationMode mode; diff --git a/src/Storages/MergeTree/MergeTreeReaderCompact.cpp b/src/Storages/MergeTree/MergeTreeReaderCompact.cpp index d1f69ce6cea..d1796dac6cc 100644 --- a/src/Storages/MergeTree/MergeTreeReaderCompact.cpp +++ b/src/Storages/MergeTree/MergeTreeReaderCompact.cpp @@ -45,6 +45,12 @@ MergeTreeReaderCompact::MergeTreeReaderCompact( settings.read_settings, load_marks_threadpool_, data_part_info_for_read_->getColumns().size()) + , profile_callback(profile_callback_) + , clock_type(clock_type_) +{ +} + +void MergeTreeReaderCompact::initialize() { try { @@ -75,8 +81,8 @@ MergeTreeReaderCompact::MergeTreeReaderCompact( uncompressed_cache, /* allow_different_codecs = */ true); - if (profile_callback_) - buffer->setProfileCallback(profile_callback_, clock_type_); + if (profile_callback) + buffer->setProfileCallback(profile_callback, clock_type); if (!settings.checksum_on_read) buffer->disableChecksumming(); @@ -95,8 +101,8 @@ MergeTreeReaderCompact::MergeTreeReaderCompact( std::nullopt, std::nullopt), /* allow_different_codecs = */ true); - if (profile_callback_) - buffer->setProfileCallback(profile_callback_, clock_type_); + if (profile_callback) + buffer->setProfileCallback(profile_callback, clock_type); if (!settings.checksum_on_read) buffer->disableChecksumming(); @@ -157,6 +163,12 @@ void MergeTreeReaderCompact::fillColumnPositions() size_t MergeTreeReaderCompact::readRows( size_t from_mark, size_t current_task_last_mark, bool continue_reading, size_t max_rows_to_read, Columns & res_columns) { + if (!initialized) + { + initialize(); + initialized = true; + } + if (continue_reading) from_mark = next_mark; @@ -302,6 +314,18 @@ void MergeTreeReaderCompact::readData( last_read_granule.emplace(from_mark, column_position); } +void MergeTreeReaderCompact::prefetchBeginOfRange(int64_t priority) +{ + if (!initialized) + { + initialize(); + initialized = true; + } + + adjustUpperBound(all_mark_ranges.back().end); + seekToMark(all_mark_ranges.front().begin, 0); + data_buffer->prefetch(priority); +} void MergeTreeReaderCompact::seekToMark(size_t row_index, size_t column_index) { diff --git a/src/Storages/MergeTree/MergeTreeReaderCompact.h b/src/Storages/MergeTree/MergeTreeReaderCompact.h index 8dcd00ad733..a994e72d3ff 100644 --- a/src/Storages/MergeTree/MergeTreeReaderCompact.h +++ b/src/Storages/MergeTree/MergeTreeReaderCompact.h @@ -38,9 +38,12 @@ public: bool canReadIncompleteGranules() const override { return false; } + void prefetchBeginOfRange(int64_t priority) override; + private: bool isContinuousReading(size_t mark, size_t column_position); void fillColumnPositions(); + void initialize(); ReadBuffer * data_buffer; CompressedReadBufferBase * compressed_data_buffer; @@ -78,6 +81,11 @@ private: /// For asynchronous reading from remote fs. void adjustUpperBound(size_t last_mark); + + ReadBufferFromFileBase::ProfileCallback profile_callback; + clockid_t clock_type; + + bool initialized = false; }; } diff --git a/src/Storages/MergeTree/MergeTreeThreadSelectProcessor.cpp b/src/Storages/MergeTree/MergeTreeThreadSelectProcessor.cpp index 0c7f90b2349..cae9080d644 100644 --- a/src/Storages/MergeTree/MergeTreeThreadSelectProcessor.cpp +++ b/src/Storages/MergeTree/MergeTreeThreadSelectProcessor.cpp @@ -21,8 +21,7 @@ MergeTreeThreadSelectAlgorithm::MergeTreeThreadSelectAlgorithm( ExpressionActionsSettings actions_settings, const MergeTreeReaderSettings & reader_settings_, const Names & virt_column_names_) - : - IMergeTreeSelectAlgorithm{ + : IMergeTreeSelectAlgorithm{ pool_->getHeader(), storage_, storage_snapshot_, prewhere_info_, std::move(actions_settings), max_block_size_rows_, preferred_block_size_bytes_, preferred_max_column_in_block_size_bytes_, reader_settings_, use_uncompressed_cache_, virt_column_names_}, @@ -59,8 +58,6 @@ void MergeTreeThreadSelectAlgorithm::finalizeNewTask() const bool init_new_readers = !reader || task->reader.valid() || part_name != last_read_part_name; if (init_new_readers) { - initializeMergeTreeReadersForPart( - task->data_part, task->task_columns, metadata_snapshot, task->mark_ranges, value_size_map, profile_callback); initializeMergeTreeReadersForCurrentTask(metadata_snapshot, value_size_map, profile_callback); } From 94d52d2b28fe8ee49d81e4bb0538f59b9f6486d2 Mon Sep 17 00:00:00 2001 From: Nikolay Degterinsky Date: Tue, 14 Feb 2023 16:45:46 +0000 Subject: [PATCH 22/57] Fix include --- .gitmodules | 2 +- contrib/ulid-c | 2 +- contrib/ulid-c-cmake/CMakeLists.txt | 3 +-- src/Functions/generateULID.cpp | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/.gitmodules b/.gitmodules index a3e23a1529e..ca55281e643 100644 --- a/.gitmodules +++ b/.gitmodules @@ -298,7 +298,7 @@ url = https://github.com/ridiculousfish/libdivide [submodule "contrib/ulid-c"] path = contrib/ulid-c - url = https://github.com/evillique/ulid-c.git + url = https://github.com/ClickHouse/ulid-c.git [submodule "contrib/aws-crt-cpp"] path = contrib/aws-crt-cpp url = https://github.com/ClickHouse/aws-crt-cpp diff --git a/contrib/ulid-c b/contrib/ulid-c index 8b8bc280bc3..c433b6783cf 160000 --- a/contrib/ulid-c +++ b/contrib/ulid-c @@ -1 +1 @@ -Subproject commit 8b8bc280bc305f0d3dbdca49131677f2a208c48c +Subproject commit c433b6783cf918b8f996dacd014cb2b68c7de419 diff --git a/contrib/ulid-c-cmake/CMakeLists.txt b/contrib/ulid-c-cmake/CMakeLists.txt index 9a68e09faa2..3afbd965bac 100644 --- a/contrib/ulid-c-cmake/CMakeLists.txt +++ b/contrib/ulid-c-cmake/CMakeLists.txt @@ -8,8 +8,7 @@ endif() set (LIBRARY_DIR "${ClickHouse_SOURCE_DIR}/contrib/ulid-c") set (SRCS - "${LIBRARY_DIR}/include/ulid.h" - "${LIBRARY_DIR}/include/ulid.c" + "${LIBRARY_DIR}/src/ulid.c" ) add_library(_ulid ${SRCS}) diff --git a/src/Functions/generateULID.cpp b/src/Functions/generateULID.cpp index 46b8d190496..648754ddaa2 100644 --- a/src/Functions/generateULID.cpp +++ b/src/Functions/generateULID.cpp @@ -9,7 +9,7 @@ #include #include -#include +#include namespace DB From 04b3c9da2742407c683f3a20a6379515eacbef50 Mon Sep 17 00:00:00 2001 From: Nikolay Degterinsky Date: Tue, 14 Feb 2023 16:58:29 +0000 Subject: [PATCH 23/57] Move docs to separate file --- .../sql-reference/functions/ulid-functions.md | 53 +++++++++++++++++++ .../sql-reference/functions/uuid-functions.md | 45 ---------------- 2 files changed, 53 insertions(+), 45 deletions(-) create mode 100644 docs/en/sql-reference/functions/ulid-functions.md diff --git a/docs/en/sql-reference/functions/ulid-functions.md b/docs/en/sql-reference/functions/ulid-functions.md new file mode 100644 index 00000000000..94167945f76 --- /dev/null +++ b/docs/en/sql-reference/functions/ulid-functions.md @@ -0,0 +1,53 @@ +--- +slug: /en/sql-reference/functions/ulid-functions +sidebar_position: 54 +sidebar_label: ULID +--- + +# Functions for Working with ULID + +## generateULID + +Generates the [ULID](https://github.com/ulid/spec). + +**Syntax** + +``` sql +generateULID([x]) +``` + +**Arguments** + +- `x` — [Expression](../../sql-reference/syntax.md#syntax-expressions) resulting in any of the [supported data types](../../sql-reference/data-types/index.md#data_types). The resulting value is discarded, but the expression itself if used for bypassing [common subexpression elimination](../../sql-reference/functions/index.md#common-subexpression-elimination) if the function is called multiple times in one query. Optional parameter. + +**Returned value** + +The [FixedString](../data-types/fixedstring.md) type value. + +**Usage example** + +``` sql +SELECT generateULID() +``` + +``` text +┌─generateULID()─────────────┐ +│ 01GNB2S2FGN2P93QPXDNB4EN2R │ +└────────────────────────────┘ +``` + +**Usage example if it is needed to generate multiple values in one row** + +```sql +SELECT generateULID(1), generateULID(2) +``` + +``` text +┌─generateULID(1)────────────┬─generateULID(2)────────────┐ +│ 01GNB2SGG4RHKVNT9ZGA4FFMNP │ 01GNB2SGG4V0HMQVH4VBVPSSRB │ +└────────────────────────────┴────────────────────────────┘ +``` + +## See Also + +- [UUID](../../sql-reference/functions/uuid-functions.md) diff --git a/docs/en/sql-reference/functions/uuid-functions.md b/docs/en/sql-reference/functions/uuid-functions.md index 1a1e065180b..474e3248d1f 100644 --- a/docs/en/sql-reference/functions/uuid-functions.md +++ b/docs/en/sql-reference/functions/uuid-functions.md @@ -359,51 +359,6 @@ serverUUID() Type: [UUID](../data-types/uuid.md). - -# Functions for Working with ULID - -## generateULID - -Generates the [ULID](https://github.com/ulid/spec). - -**Syntax** - -``` sql -generateULID([x]) -``` - -**Arguments** - -- `x` — [Expression](../../sql-reference/syntax.md#syntax-expressions) resulting in any of the [supported data types](../../sql-reference/data-types/index.md#data_types). The resulting value is discarded, but the expression itself if used for bypassing [common subexpression elimination](../../sql-reference/functions/index.md#common-subexpression-elimination) if the function is called multiple times in one query. Optional parameter. - -**Returned value** - -The [FixedString](../data-types/fixedstring.md) type value. - -**Usage example** - -``` sql -SELECT generateULID() -``` - -``` text -┌─generateULID()─────────────┐ -│ 01GNB2S2FGN2P93QPXDNB4EN2R │ -└────────────────────────────┘ -``` - -**Usage example if it is needed to generate multiple values in one row** - -```sql -SELECT generateULID(1), generateULID(2) -``` - -``` text -┌─generateULID(1)────────────┬─generateULID(2)────────────┐ -│ 01GNB2SGG4RHKVNT9ZGA4FFMNP │ 01GNB2SGG4V0HMQVH4VBVPSSRB │ -└────────────────────────────┴────────────────────────────┘ -``` - ## See Also - [dictGetUUID](../../sql-reference/functions/ext-dict-functions.md#ext_dict_functions-other) From e07c723032f75c351fcdf4ab98711be8bd876370 Mon Sep 17 00:00:00 2001 From: Denny Crane Date: Tue, 14 Feb 2023 13:03:44 -0400 Subject: [PATCH 24/57] Update prewhere.md --- .../statements/select/prewhere.md | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/docs/en/sql-reference/statements/select/prewhere.md b/docs/en/sql-reference/statements/select/prewhere.md index d0248790bfd..5fb83f60dc6 100644 --- a/docs/en/sql-reference/statements/select/prewhere.md +++ b/docs/en/sql-reference/statements/select/prewhere.md @@ -26,3 +26,44 @@ The `PREWHERE` section is executed before `FINAL`, so the results of `FROM ... F ## Limitations `PREWHERE` is only supported by tables from the [*MergeTree](../../../engines/table-engines/mergetree-family/index.md) family. + +## Example + +```sql +CREATE TABLE mydata +( + `A` Int64, + `B` Int8, + `C` String +) +ENGINE = MergeTree +ORDER BY A AS +SELECT + number, + 0, + if(number between 1000 and 2000, 'x', toString(number)) +FROM numbers(10000000); + +SELECT count() +FROM mydata +WHERE (B = 0) AND (C = 'x'); + +1 row in set. Elapsed: 0.074 sec. Processed 10.00 million rows, 168.89 MB (134.98 million rows/s., 2.28 GB/s.) + +-- let's enable tracing to see which predicate are moved to PREWHERE +set send_logs_level='debug'; + +MergeTreeWhereOptimizer: condition "B = 0" moved to PREWHERE +-- Clickhouse moves automatically `B = 0` to PREWHERE, but it has no sense because B is always 0. + +-- Let's move other predicate `C = 'x'` + +SELECT count() +FROM mydata +PREWHERE C = 'x' +WHERE B = 0; + +1 row in set. Elapsed: 0.069 sec. Processed 10.00 million rows, 158.89 MB (144.90 million rows/s., 2.30 GB/s.) + +-- This query with manual `PREWHERE` processes slightly less data 158.89 MB VS 168.89 MB +``` From 6d1b3dcd789446906340ef2d47a980f14f88b2a0 Mon Sep 17 00:00:00 2001 From: Denny Crane Date: Tue, 14 Feb 2023 13:07:01 -0400 Subject: [PATCH 25/57] Update prewhere.md --- docs/en/sql-reference/statements/select/prewhere.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/sql-reference/statements/select/prewhere.md b/docs/en/sql-reference/statements/select/prewhere.md index 5fb83f60dc6..374113fbd67 100644 --- a/docs/en/sql-reference/statements/select/prewhere.md +++ b/docs/en/sql-reference/statements/select/prewhere.md @@ -65,5 +65,5 @@ WHERE B = 0; 1 row in set. Elapsed: 0.069 sec. Processed 10.00 million rows, 158.89 MB (144.90 million rows/s., 2.30 GB/s.) --- This query with manual `PREWHERE` processes slightly less data 158.89 MB VS 168.89 MB +-- This query with manual `PREWHERE` processes slightly less data: 158.89 MB VS 168.89 MB ``` From a7bbf02baca0d6b98c7b7b15fe23f7d9cd55a1ab Mon Sep 17 00:00:00 2001 From: serxa Date: Tue, 14 Feb 2023 19:37:09 +0000 Subject: [PATCH 26/57] fix possible deadlock --- src/Interpreters/Context.cpp | 4 ++-- src/Storages/MergeTree/MergeTreeBackgroundExecutor.h | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index bae6264e105..726bb99ed09 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -3825,9 +3825,9 @@ void Context::initializeBackgroundExecutorsIfNeeded() "MergeMutate", /*max_threads_count*/background_pool_size, /*max_tasks_count*/background_pool_size * background_merges_mutations_concurrency_ratio, - CurrentMetrics::BackgroundMergesAndMutationsPoolTask + CurrentMetrics::BackgroundMergesAndMutationsPoolTask, + background_merges_mutations_scheduling_policy ); - shared->merge_mutate_executor->updateSchedulingPolicy(background_merges_mutations_scheduling_policy); LOG_INFO(shared->log, "Initialized background executor for merges and mutations with num_threads={}, num_tasks={}, scheduling_policy={}", background_pool_size, background_pool_size * background_merges_mutations_concurrency_ratio, background_merges_mutations_scheduling_policy); diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h index 7745e5de334..9305f36feb5 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h @@ -267,6 +267,18 @@ public: pool.scheduleOrThrowOnError([this] { threadFunction(); }); } + MergeTreeBackgroundExecutor( + String name_, + size_t threads_count_, + size_t max_tasks_count_, + CurrentMetrics::Metric metric_, + std::string_view policy) + requires requires(Queue queue) { queue.updatePolicy(policy); } // Because we use explicit template instantiation + : MergeTreeBackgroundExecutor(name_, threads_count_, max_tasks_count_, metric_) + { + pending.updatePolicy(policy); + } + ~MergeTreeBackgroundExecutor() { wait(); From 6158d9e85f07ac24d23729b25cb77166bf2db9ed Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Tue, 14 Feb 2023 23:11:18 +0100 Subject: [PATCH 27/57] make tests with window view less bad --- .../01052_window_view_proc_tumble_to_now.reference | 1 + .../0_stateless/01052_window_view_proc_tumble_to_now.sh | 5 +++-- .../0_stateless/01053_window_view_proc_hop_to_now.reference | 1 + .../queries/0_stateless/01053_window_view_proc_hop_to_now.sh | 5 +++-- .../0_stateless/01054_window_view_proc_tumble_to.reference | 1 + .../queries/0_stateless/01054_window_view_proc_tumble_to.sh | 5 +++-- .../0_stateless/01055_window_view_proc_hop_to.reference | 1 + tests/queries/0_stateless/01055_window_view_proc_hop_to.sh | 5 +++-- .../01072_window_view_multiple_columns_groupby.reference | 1 + .../01072_window_view_multiple_columns_groupby.sh | 5 +++-- .../01075_window_view_proc_tumble_to_now_populate.reference | 1 + .../01075_window_view_proc_tumble_to_now_populate.sh | 5 +++-- 12 files changed, 24 insertions(+), 12 deletions(-) diff --git a/tests/queries/0_stateless/01052_window_view_proc_tumble_to_now.reference b/tests/queries/0_stateless/01052_window_view_proc_tumble_to_now.reference index d00491fd7e5..3704744470d 100644 --- a/tests/queries/0_stateless/01052_window_view_proc_tumble_to_now.reference +++ b/tests/queries/0_stateless/01052_window_view_proc_tumble_to_now.reference @@ -1 +1,2 @@ +OK 1 diff --git a/tests/queries/0_stateless/01052_window_view_proc_tumble_to_now.sh b/tests/queries/0_stateless/01052_window_view_proc_tumble_to_now.sh index 44afc984fb7..9fdc66191d7 100755 --- a/tests/queries/0_stateless/01052_window_view_proc_tumble_to_now.sh +++ b/tests/queries/0_stateless/01052_window_view_proc_tumble_to_now.sh @@ -17,8 +17,9 @@ CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY tumble INSERT INTO mt VALUES (1); EOF -while true; do - $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && break || sleep .5 ||: +for _ in {1..100}; do + $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && echo 'OK' && break + sleep .5 done $CLICKHOUSE_CLIENT --query="SELECT count FROM dst" diff --git a/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.reference b/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.reference index d00491fd7e5..3704744470d 100644 --- a/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.reference +++ b/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.reference @@ -1 +1,2 @@ +OK 1 diff --git a/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.sh b/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.sh index 2e8df446444..0576b32fb9e 100755 --- a/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.sh +++ b/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.sh @@ -17,8 +17,9 @@ CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY hop(no INSERT INTO mt VALUES (1); EOF -while true; do - $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && break || sleep .5 ||: +for _ in {1..100}; do + $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && echo 'OK' && break + sleep .5 done $CLICKHOUSE_CLIENT --query="SELECT count FROM dst" diff --git a/tests/queries/0_stateless/01054_window_view_proc_tumble_to.reference b/tests/queries/0_stateless/01054_window_view_proc_tumble_to.reference index d00491fd7e5..3704744470d 100644 --- a/tests/queries/0_stateless/01054_window_view_proc_tumble_to.reference +++ b/tests/queries/0_stateless/01054_window_view_proc_tumble_to.reference @@ -1 +1,2 @@ +OK 1 diff --git a/tests/queries/0_stateless/01054_window_view_proc_tumble_to.sh b/tests/queries/0_stateless/01054_window_view_proc_tumble_to.sh index 22138ea9dc9..156ff5a156e 100755 --- a/tests/queries/0_stateless/01054_window_view_proc_tumble_to.sh +++ b/tests/queries/0_stateless/01054_window_view_proc_tumble_to.sh @@ -17,8 +17,9 @@ CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY tumble INSERT INTO mt VALUES (1, now('US/Samoa') + 1); EOF -while true; do - $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && break || sleep .5 ||: +for _ in {1..100}; do + $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && echo 'OK' && break + sleep .5 done $CLICKHOUSE_CLIENT --query="SELECT count FROM dst" diff --git a/tests/queries/0_stateless/01055_window_view_proc_hop_to.reference b/tests/queries/0_stateless/01055_window_view_proc_hop_to.reference index d00491fd7e5..3704744470d 100644 --- a/tests/queries/0_stateless/01055_window_view_proc_hop_to.reference +++ b/tests/queries/0_stateless/01055_window_view_proc_hop_to.reference @@ -1 +1,2 @@ +OK 1 diff --git a/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh b/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh index 85c69cf76cf..3915e425010 100755 --- a/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh +++ b/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh @@ -17,8 +17,9 @@ CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY hop(ti INSERT INTO mt VALUES (1, now('US/Samoa') + 1); EOF -while true; do - $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && break || sleep .5 ||: +for _ in {1..100}; do + $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && echo 'OK' && break + sleep .5 done $CLICKHOUSE_CLIENT --query="SELECT count FROM dst;" diff --git a/tests/queries/0_stateless/01072_window_view_multiple_columns_groupby.reference b/tests/queries/0_stateless/01072_window_view_multiple_columns_groupby.reference index 828667becf2..f4e69c0e2bf 100644 --- a/tests/queries/0_stateless/01072_window_view_multiple_columns_groupby.reference +++ b/tests/queries/0_stateless/01072_window_view_multiple_columns_groupby.reference @@ -1 +1,2 @@ +OK test1 test2 diff --git a/tests/queries/0_stateless/01072_window_view_multiple_columns_groupby.sh b/tests/queries/0_stateless/01072_window_view_multiple_columns_groupby.sh index 15d4da504f1..4a228abaded 100755 --- a/tests/queries/0_stateless/01072_window_view_multiple_columns_groupby.sh +++ b/tests/queries/0_stateless/01072_window_view_multiple_columns_groupby.sh @@ -18,8 +18,9 @@ CREATE WINDOW VIEW wv TO dst AS SELECT tumbleStart(w_id) AS time, colA, colB FRO INSERT INTO mt VALUES ('test1', 'test2'); EOF -while true; do - $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && break || sleep .1 ||: +for _ in {1..100}; do + $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && echo 'OK' && break + sleep .5 done $CLICKHOUSE_CLIENT --query="SELECT colA, colB FROM dst" diff --git a/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.reference b/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.reference index d00491fd7e5..3704744470d 100644 --- a/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.reference +++ b/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.reference @@ -1 +1,2 @@ +OK 1 diff --git a/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.sh b/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.sh index 83c35779059..417a024ac7c 100755 --- a/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.sh +++ b/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.sh @@ -18,8 +18,9 @@ INSERT INTO mt VALUES (1); CREATE WINDOW VIEW wv TO dst POPULATE AS SELECT count(a) AS count, tumbleEnd(wid) FROM mt GROUP BY tumble(now('US/Samoa'), INTERVAL '1' SECOND, 'US/Samoa') AS wid; EOF -while true; do - $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && break || sleep .5 ||: +for _ in {1..100}; do + $CLICKHOUSE_CLIENT --query="SELECT count(*) FROM dst" | grep -q "1" && echo 'OK' && break + sleep .5 done $CLICKHOUSE_CLIENT --query="SELECT count FROM dst" From 6e4b660033566bfb1a4ca82ca220bc717905d2eb Mon Sep 17 00:00:00 2001 From: Nikolay Degterinsky Date: Tue, 14 Feb 2023 22:35:10 +0000 Subject: [PATCH 28/57] Move MongoDB and PostgreSQL sources to Sources folder --- src/Dictionaries/MongoDBDictionarySource.h | 2 +- src/Dictionaries/PostgreSQLDictionarySource.cpp | 2 +- src/Processors/{Transforms => Sources}/MongoDBSource.cpp | 0 src/Processors/{Transforms => Sources}/MongoDBSource.h | 0 src/Processors/{Transforms => Sources}/PostgreSQLSource.cpp | 0 src/Processors/{Transforms => Sources}/PostgreSQLSource.h | 0 src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp | 2 +- src/Storages/StorageMongoDB.cpp | 2 +- src/Storages/StoragePostgreSQL.cpp | 2 +- 9 files changed, 5 insertions(+), 5 deletions(-) rename src/Processors/{Transforms => Sources}/MongoDBSource.cpp (100%) rename src/Processors/{Transforms => Sources}/MongoDBSource.h (100%) rename src/Processors/{Transforms => Sources}/PostgreSQLSource.cpp (100%) rename src/Processors/{Transforms => Sources}/PostgreSQLSource.h (100%) diff --git a/src/Dictionaries/MongoDBDictionarySource.h b/src/Dictionaries/MongoDBDictionarySource.h index ac5f19816d2..4c7ae649f09 100644 --- a/src/Dictionaries/MongoDBDictionarySource.h +++ b/src/Dictionaries/MongoDBDictionarySource.h @@ -1,6 +1,6 @@ #pragma once -#include +#include #include #include "DictionaryStructure.h" diff --git a/src/Dictionaries/PostgreSQLDictionarySource.cpp b/src/Dictionaries/PostgreSQLDictionarySource.cpp index 9153b9cb05d..95bfb1a37d8 100644 --- a/src/Dictionaries/PostgreSQLDictionarySource.cpp +++ b/src/Dictionaries/PostgreSQLDictionarySource.cpp @@ -8,7 +8,7 @@ #if USE_LIBPQXX #include #include -#include +#include #include "readInvalidateQuery.h" #include #include diff --git a/src/Processors/Transforms/MongoDBSource.cpp b/src/Processors/Sources/MongoDBSource.cpp similarity index 100% rename from src/Processors/Transforms/MongoDBSource.cpp rename to src/Processors/Sources/MongoDBSource.cpp diff --git a/src/Processors/Transforms/MongoDBSource.h b/src/Processors/Sources/MongoDBSource.h similarity index 100% rename from src/Processors/Transforms/MongoDBSource.h rename to src/Processors/Sources/MongoDBSource.h diff --git a/src/Processors/Transforms/PostgreSQLSource.cpp b/src/Processors/Sources/PostgreSQLSource.cpp similarity index 100% rename from src/Processors/Transforms/PostgreSQLSource.cpp rename to src/Processors/Sources/PostgreSQLSource.cpp diff --git a/src/Processors/Transforms/PostgreSQLSource.h b/src/Processors/Sources/PostgreSQLSource.h similarity index 100% rename from src/Processors/Transforms/PostgreSQLSource.h rename to src/Processors/Sources/PostgreSQLSource.h diff --git a/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp b/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp index f450604fded..99f6fded669 100644 --- a/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp +++ b/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp @@ -4,7 +4,7 @@ #include #include -#include +#include #include #include #include diff --git a/src/Storages/StorageMongoDB.cpp b/src/Storages/StorageMongoDB.cpp index 25a303620d6..2cb85878000 100644 --- a/src/Storages/StorageMongoDB.cpp +++ b/src/Storages/StorageMongoDB.cpp @@ -16,7 +16,7 @@ #include #include #include -#include +#include #include namespace DB diff --git a/src/Storages/StoragePostgreSQL.cpp b/src/Storages/StoragePostgreSQL.cpp index 729ffdf0dde..400430b9ea2 100644 --- a/src/Storages/StoragePostgreSQL.cpp +++ b/src/Storages/StoragePostgreSQL.cpp @@ -1,7 +1,7 @@ #include "StoragePostgreSQL.h" #if USE_LIBPQXX -#include +#include #include #include From 3feb164c75dc3d2168c38c07505a11c40988497f Mon Sep 17 00:00:00 2001 From: Nikolay Degterinsky Date: Tue, 14 Feb 2023 22:54:30 +0000 Subject: [PATCH 29/57] Add doc --- src/Functions/generateULID.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/Functions/generateULID.cpp b/src/Functions/generateULID.cpp index 648754ddaa2..a8a2a2174cb 100644 --- a/src/Functions/generateULID.cpp +++ b/src/Functions/generateULID.cpp @@ -74,7 +74,19 @@ public: REGISTER_FUNCTION(GenerateULID) { - factory.registerFunction(); + factory.registerFunction( + { + R"( +Generates a Universally Unique Lexicographically Sortable Identifier (ULID). +This function takes an optional argument, the value of which is discarded to generate different values in case the function is called multiple times. +The function returns a value of type FixedString(26). +)", + Documentation::Examples{ + {"ulid", "SELECT generateULID()"}, + {"multiple", "SELECT generateULID(1), generateULID(2)"}}, + Documentation::Categories{"ULID"} + }, + FunctionFactory::CaseSensitive); } } From 4eacbecc16cf75fc052e02a377fb6a34756f5812 Mon Sep 17 00:00:00 2001 From: flynn Date: Wed, 15 Feb 2023 02:38:35 +0000 Subject: [PATCH 30/57] update test --- .../0_stateless/02554_log_faminy_support_storage_policy.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql index adb6e19438e..985c2b3803e 100644 --- a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql +++ b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql @@ -15,7 +15,7 @@ SELECT * FROM test_2554_tinylog; DROP TABLE test_2554_tinylog; DROP TABLE IF EXISTS test_2554_stripelog; -CREATE TABLE test_2554_stripelog (n UInt32) ENGINE = StripeLog SETTINGS storage_policy = 'default'; +CREATE TABLE test_2554_stripelog (n UInt32) ENGINE = StripeLog SETTINGS storage_policy = 's3_cache'; INSERT INTO test_2554_stripelog SELECT 1; SELECT * FROM test_2554_stripelog; From 26e7b3463ad1306efd674f7ab8317bc7558bf71f Mon Sep 17 00:00:00 2001 From: flynn Date: Wed, 15 Feb 2023 03:56:38 +0000 Subject: [PATCH 31/57] fix --- .../0_stateless/02554_log_faminy_support_storage_policy.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql index 985c2b3803e..311df048704 100644 --- a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql +++ b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql @@ -15,7 +15,7 @@ SELECT * FROM test_2554_tinylog; DROP TABLE test_2554_tinylog; DROP TABLE IF EXISTS test_2554_stripelog; -CREATE TABLE test_2554_stripelog (n UInt32) ENGINE = StripeLog SETTINGS storage_policy = 's3_cache'; +CREATE TABLE test_2554_stripelog (n UInt32) ENGINE = StripeLog SETTINGS storage_policy = 's3'; INSERT INTO test_2554_stripelog SELECT 1; SELECT * FROM test_2554_stripelog; From 42103e0d08dfde9196891bb2ff0423688f9a2757 Mon Sep 17 00:00:00 2001 From: flynn Date: Wed, 15 Feb 2023 07:45:24 +0000 Subject: [PATCH 32/57] udpate test --- .../0_stateless/02554_log_faminy_support_storage_policy.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql index 311df048704..985c2b3803e 100644 --- a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql +++ b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql @@ -15,7 +15,7 @@ SELECT * FROM test_2554_tinylog; DROP TABLE test_2554_tinylog; DROP TABLE IF EXISTS test_2554_stripelog; -CREATE TABLE test_2554_stripelog (n UInt32) ENGINE = StripeLog SETTINGS storage_policy = 's3'; +CREATE TABLE test_2554_stripelog (n UInt32) ENGINE = StripeLog SETTINGS storage_policy = 's3_cache'; INSERT INTO test_2554_stripelog SELECT 1; SELECT * FROM test_2554_stripelog; From f75d69a954f76a3b3f2f391110d187841dffebc4 Mon Sep 17 00:00:00 2001 From: kssenii Date: Wed, 15 Feb 2023 11:14:50 +0100 Subject: [PATCH 33/57] Fix --- src/Storages/RabbitMQ/RabbitMQHandler.cpp | 4 ++-- src/Storages/RabbitMQ/RabbitMQHandler.h | 2 +- src/Storages/RabbitMQ/RabbitMQProducer.cpp | 2 +- src/Storages/RabbitMQ/RabbitMQProducer.h | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Storages/RabbitMQ/RabbitMQHandler.cpp b/src/Storages/RabbitMQ/RabbitMQHandler.cpp index 5258b9fa7dc..934753257b4 100644 --- a/src/Storages/RabbitMQ/RabbitMQHandler.cpp +++ b/src/Storages/RabbitMQ/RabbitMQHandler.cpp @@ -46,12 +46,12 @@ void RabbitMQHandler::startLoop() loop_running.store(false); } -bool RabbitMQHandler::iterateLoop() +int RabbitMQHandler::iterateLoop() { std::unique_lock lock(startup_mutex, std::defer_lock); if (lock.try_lock()) return uv_run(loop, UV_RUN_NOWAIT); - return 0; + return 1; /// We cannot know how actual value. } /// Do not need synchronization as in iterateLoop(), because this method is used only for diff --git a/src/Storages/RabbitMQ/RabbitMQHandler.h b/src/Storages/RabbitMQ/RabbitMQHandler.h index 4a7c3fc7f78..948d56416fd 100644 --- a/src/Storages/RabbitMQ/RabbitMQHandler.h +++ b/src/Storages/RabbitMQ/RabbitMQHandler.h @@ -34,7 +34,7 @@ public: /// Loop to wait for small tasks in a non-blocking mode. /// Adds synchronization with main background loop. - bool iterateLoop(); + int iterateLoop(); /// Loop to wait for small tasks in a blocking mode. /// No synchronization is done with the main loop thread. diff --git a/src/Storages/RabbitMQ/RabbitMQProducer.cpp b/src/Storages/RabbitMQ/RabbitMQProducer.cpp index fc49f74b545..5d639b77f53 100644 --- a/src/Storages/RabbitMQ/RabbitMQProducer.cpp +++ b/src/Storages/RabbitMQ/RabbitMQProducer.cpp @@ -266,7 +266,7 @@ void RabbitMQProducer::startProducingTaskLoop() } -bool RabbitMQProducer::iterateEventLoop() +int RabbitMQProducer::iterateEventLoop() { return connection.getHandler().iterateLoop(); } diff --git a/src/Storages/RabbitMQ/RabbitMQProducer.h b/src/Storages/RabbitMQ/RabbitMQProducer.h index e3691c005ee..70afbbb9b90 100644 --- a/src/Storages/RabbitMQ/RabbitMQProducer.h +++ b/src/Storages/RabbitMQ/RabbitMQProducer.h @@ -43,7 +43,7 @@ private: void stopProducingTask() override; void finishImpl() override; - bool iterateEventLoop(); + int iterateEventLoop(); void startProducingTaskLoop() override; void setupChannel(); void removeRecord(UInt64 received_delivery_tag, bool multiple, bool republish); From d7bfe494b2e8063ab7cff7ad502dfeba638909c1 Mon Sep 17 00:00:00 2001 From: kssenii Date: Wed, 15 Feb 2023 12:02:24 +0100 Subject: [PATCH 34/57] Fix test --- .../0_stateless/01605_adaptive_granularity_block_borders.sql | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/queries/0_stateless/01605_adaptive_granularity_block_borders.sql b/tests/queries/0_stateless/01605_adaptive_granularity_block_borders.sql index 85ad7636201..ca7d0f3c950 100644 --- a/tests/queries/0_stateless/01605_adaptive_granularity_block_borders.sql +++ b/tests/queries/0_stateless/01605_adaptive_granularity_block_borders.sql @@ -1,6 +1,7 @@ -- Tags: no-random-merge-tree-settings SET use_uncompressed_cache = 0; +SET allow_prefetched_read_pool_for_remote_filesystem=0; DROP TABLE IF EXISTS adaptive_table; From 63c3c07347c69fe455c58f956a47edfa09edfc23 Mon Sep 17 00:00:00 2001 From: flynn Date: Wed, 15 Feb 2023 10:28:04 +0000 Subject: [PATCH 35/57] fix test --- .../0_stateless/02554_log_faminy_support_storage_policy.sql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql index 985c2b3803e..4dbb4569c0f 100644 --- a/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql +++ b/tests/queries/0_stateless/02554_log_faminy_support_storage_policy.sql @@ -1,3 +1,5 @@ +-- Tags: no-fasttest + DROP TABLE IF EXISTS test_2554_log; CREATE TABLE test_2554_log (n UInt32) ENGINE = Log SETTINGS storage_policy = 'default'; From 8cdafcc2d099f3b488b5448f3a7908256b4d8e99 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Wed, 15 Feb 2023 11:45:57 +0000 Subject: [PATCH 36/57] Correctly check if there were failures --- programs/copier/ClusterCopier.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/programs/copier/ClusterCopier.cpp b/programs/copier/ClusterCopier.cpp index dfeabc6620c..48a3578dd7b 100644 --- a/programs/copier/ClusterCopier.cpp +++ b/programs/copier/ClusterCopier.cpp @@ -1225,8 +1225,8 @@ TaskStatus ClusterCopier::iterateThroughAllPiecesInPartition(const ConnectionTim std::this_thread::sleep_for(retry_delay_ms); } - was_active_pieces = (res == TaskStatus::Active); - was_failed_pieces = (res == TaskStatus::Error); + was_active_pieces |= (res == TaskStatus::Active); + was_failed_pieces |= (res == TaskStatus::Error); } if (was_failed_pieces) From 44815240f5ce18c848d2753622618d67aaa5eda1 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Wed, 15 Feb 2023 12:58:00 +0000 Subject: [PATCH 37/57] Exclude internal databases from async metric "NumberOfDatabases" --- src/Interpreters/ServerAsynchronousMetrics.cpp | 6 +++++- src/Storages/System/StorageSystemDatabases.cpp | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Interpreters/ServerAsynchronousMetrics.cpp b/src/Interpreters/ServerAsynchronousMetrics.cpp index dc24fa45573..0d2032bed78 100644 --- a/src/Interpreters/ServerAsynchronousMetrics.cpp +++ b/src/Interpreters/ServerAsynchronousMetrics.cpp @@ -218,7 +218,11 @@ void ServerAsynchronousMetrics::updateImpl(AsynchronousMetricValues & new_values size_t max_part_count_for_partition = 0; - size_t number_of_databases = databases.size(); + size_t number_of_databases = 0; + for (auto [db_name, _] : databases) + if (db_name != DatabaseCatalog::TEMPORARY_DATABASE) + ++number_of_databases; /// filter out the internal database for temporary tables, system table "system.databases" behaves the same way + size_t total_number_of_tables = 0; size_t total_number_of_bytes = 0; diff --git a/src/Storages/System/StorageSystemDatabases.cpp b/src/Storages/System/StorageSystemDatabases.cpp index 432d2c4ac64..4d1f6c171db 100644 --- a/src/Storages/System/StorageSystemDatabases.cpp +++ b/src/Storages/System/StorageSystemDatabases.cpp @@ -80,7 +80,7 @@ void StorageSystemDatabases::fillData(MutableColumns & res_columns, ContextPtr c continue; if (database_name == DatabaseCatalog::TEMPORARY_DATABASE) - continue; /// We don't want to show the internal database for temporary tables in system.databases + continue; /// filter out the internal database for temporary tables in system.databases, asynchronous metric "NumberOfDatabases" behaves the same way res_columns[0]->insert(database_name); res_columns[1]->insert(database->getEngineName()); From f6c894eb6e906d03d897d159e65ea59bb64db98b Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Wed, 15 Feb 2023 13:03:30 +0000 Subject: [PATCH 38/57] Update version_date.tsv and changelogs after v22.3.18.37-lts --- docs/changelogs/v22.3.18.37-lts.md | 33 ++++++++++++++++++++++++++++ utils/list-versions/version_date.tsv | 1 + 2 files changed, 34 insertions(+) create mode 100644 docs/changelogs/v22.3.18.37-lts.md diff --git a/docs/changelogs/v22.3.18.37-lts.md b/docs/changelogs/v22.3.18.37-lts.md new file mode 100644 index 00000000000..ff6378f09ad --- /dev/null +++ b/docs/changelogs/v22.3.18.37-lts.md @@ -0,0 +1,33 @@ +--- +sidebar_position: 1 +sidebar_label: 2023 +--- + +# 2023 Changelog + +### ClickHouse release v22.3.18.37-lts (fe512717551) FIXME as compared to v22.3.17.13-lts (fcc4de7e805) + +#### Performance Improvement +* Backported in [#46372](https://github.com/ClickHouse/ClickHouse/issues/46372): Fix too big memory usage for vertical merges on non-remote disk. Respect `max_insert_delayed_streams_for_parallel_write` for the remote disk. [#46275](https://github.com/ClickHouse/ClickHouse/pull/46275) ([Nikolai Kochetov](https://github.com/KochetovNicolai)). +* Backported in [#46357](https://github.com/ClickHouse/ClickHouse/issues/46357): Allow using Vertical merge algorithm with parts in Compact format. This will allow ClickHouse server to use much less memory for background operations. This closes [#46084](https://github.com/ClickHouse/ClickHouse/issues/46084). [#46282](https://github.com/ClickHouse/ClickHouse/pull/46282) ([Anton Popov](https://github.com/CurtizJ)). + +#### Build/Testing/Packaging Improvement +* Backported in [#45856](https://github.com/ClickHouse/ClickHouse/issues/45856): Fix zookeeper downloading, update the version, and optimize the image size. [#44853](https://github.com/ClickHouse/ClickHouse/pull/44853) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). + +#### Bug Fix (user-visible misbehavior in official stable or prestable release) + +* Backported in [#45620](https://github.com/ClickHouse/ClickHouse/issues/45620): Another fix for `Cannot read all data` error which could happen while reading `LowCardinality` dictionary from remote fs. Fixes [#44709](https://github.com/ClickHouse/ClickHouse/issues/44709). [#44875](https://github.com/ClickHouse/ClickHouse/pull/44875) ([Nikolai Kochetov](https://github.com/KochetovNicolai)). +* Backported in [#45549](https://github.com/ClickHouse/ClickHouse/issues/45549): Fix `SELECT ... FROM system.dictionaries` exception when there is a dictionary with a bad structure (e.g. incorrect type in xml config). [#45399](https://github.com/ClickHouse/ClickHouse/pull/45399) ([Aleksei Filatov](https://github.com/aalexfvk)). + +#### NOT FOR CHANGELOG / INSIGNIFICANT + +* Automatically merge green backport PRs and green approved PRs [#41110](https://github.com/ClickHouse/ClickHouse/pull/41110) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Fix wrong approved_at, simplify conditions [#45302](https://github.com/ClickHouse/ClickHouse/pull/45302) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Get rid of artifactory in favor of r2 + ch-repos-manager [#45421](https://github.com/ClickHouse/ClickHouse/pull/45421) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Another attempt to fix automerge, or at least to have debug footprint [#45476](https://github.com/ClickHouse/ClickHouse/pull/45476) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Trim refs/tags/ from GITHUB_TAG in release workflow [#45636](https://github.com/ClickHouse/ClickHouse/pull/45636) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Add check for running workflows to merge_pr.py [#45803](https://github.com/ClickHouse/ClickHouse/pull/45803) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Get rid of progress timestamps in release publishing [#45818](https://github.com/ClickHouse/ClickHouse/pull/45818) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Add helping logging to auto-merge script [#46080](https://github.com/ClickHouse/ClickHouse/pull/46080) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Fix write buffer destruction order for vertical merge. [#46205](https://github.com/ClickHouse/ClickHouse/pull/46205) ([Nikolai Kochetov](https://github.com/KochetovNicolai)). + diff --git a/utils/list-versions/version_date.tsv b/utils/list-versions/version_date.tsv index e09a39ff463..d313c4bfb78 100644 --- a/utils/list-versions/version_date.tsv +++ b/utils/list-versions/version_date.tsv @@ -61,6 +61,7 @@ v22.4.5.9-stable 2022-05-06 v22.4.4.7-stable 2022-04-29 v22.4.3.3-stable 2022-04-26 v22.4.2.1-stable 2022-04-22 +v22.3.18.37-lts 2023-02-15 v22.3.17.13-lts 2023-01-12 v22.3.16.1190-lts 2023-01-09 v22.3.15.33-lts 2022-12-02 From 3aa96953f67329bf34b30816a413d03caeacbb19 Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Wed, 15 Feb 2023 14:34:24 +0100 Subject: [PATCH 39/57] Update 01075_window_view_proc_tumble_to_now_populate.sh --- .../01075_window_view_proc_tumble_to_now_populate.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.sh b/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.sh index 417a024ac7c..a0bdf32b134 100755 --- a/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.sh +++ b/tests/queries/0_stateless/01075_window_view_proc_tumble_to_now_populate.sh @@ -15,7 +15,7 @@ CREATE TABLE mt(a Int32) ENGINE=MergeTree ORDER BY tuple(); INSERT INTO mt VALUES (1); -CREATE WINDOW VIEW wv TO dst POPULATE AS SELECT count(a) AS count, tumbleEnd(wid) FROM mt GROUP BY tumble(now('US/Samoa'), INTERVAL '1' SECOND, 'US/Samoa') AS wid; +CREATE WINDOW VIEW wv TO dst POPULATE AS SELECT count(a) AS count, tumbleEnd(wid) FROM mt GROUP BY tumble(now('US/Samoa'), INTERVAL '5' SECOND, 'US/Samoa') AS wid; EOF for _ in {1..100}; do From 299861621e12199e2415507bc2634778b7c4f4aa Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Wed, 15 Feb 2023 14:36:54 +0100 Subject: [PATCH 40/57] Update 01053_window_view_proc_hop_to_now.sh --- tests/queries/0_stateless/01053_window_view_proc_hop_to_now.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.sh b/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.sh index 0576b32fb9e..c1f9e52f831 100755 --- a/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.sh +++ b/tests/queries/0_stateless/01053_window_view_proc_hop_to_now.sh @@ -12,7 +12,7 @@ DROP TABLE IF EXISTS wv; CREATE TABLE dst(count UInt64) Engine=MergeTree ORDER BY tuple(); CREATE TABLE mt(a Int32) ENGINE=MergeTree ORDER BY tuple(); -CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY hop(now('US/Samoa'), INTERVAL '1' SECOND, INTERVAL '1' SECOND, 'US/Samoa') AS wid; +CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY hop(now('US/Samoa'), INTERVAL '5' SECOND, INTERVAL '5' SECOND, 'US/Samoa') AS wid; INSERT INTO mt VALUES (1); EOF From da4389a763a7f154d1b280e26d3ae6a9d5053154 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Wed, 15 Feb 2023 15:29:11 +0100 Subject: [PATCH 41/57] Add docs for KeeperMap --- docs/en/engines/table-engines/index.md | 1 + .../integrations/embedded-rocksdb.md | 36 ++++++ .../table-engines/special/keepermap.md | 111 ++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 docs/en/engines/table-engines/special/keepermap.md diff --git a/docs/en/engines/table-engines/index.md b/docs/en/engines/table-engines/index.md index ebf6826221d..31563e2e727 100644 --- a/docs/en/engines/table-engines/index.md +++ b/docs/en/engines/table-engines/index.md @@ -76,6 +76,7 @@ Engines in the family: - [View](../../engines/table-engines/special/view.md#table_engines-view) - [Memory](../../engines/table-engines/special/memory.md#memory) - [Buffer](../../engines/table-engines/special/buffer.md#buffer) +- [KeeperMap](../../engines/table-engines/special/keepermap.md) ## Virtual Columns {#table_engines-virtual_columns} diff --git a/docs/en/engines/table-engines/integrations/embedded-rocksdb.md b/docs/en/engines/table-engines/integrations/embedded-rocksdb.md index f846f785c5a..35c59b8181a 100644 --- a/docs/en/engines/table-engines/integrations/embedded-rocksdb.md +++ b/docs/en/engines/table-engines/integrations/embedded-rocksdb.md @@ -84,3 +84,39 @@ You can also change any [rocksdb options](https://github.com/facebook/rocksdb/wi ``` + +## Supported operations {#table_engine-EmbeddedRocksDB-supported-operations} + +### Inserts + +When new rows are inserted into `EmbeddedRocksDB`, if the key already exists, the value will be updated, otherwise new key is created. + +Example: + +```sql +INSERT INTO test VALUES ('some key', 1, 'value', 3.2); +``` + +### Deletes + +Rows can be deleted using `DELETE` query or `TRUNCATE`. + +```sql +DELETE FROM test WHERE key LIKE 'some%' AND v1 > 1; +``` + +```sql +ALTER TABLE test DELETE WHERE key LIKE 'some%' AND v1 > 1; +``` + +```sql +TRUNCATE TABLE test; +``` + +### Updates + +Values can be updated using `ALTER TABLE` query. Primary key cannot be updated. + +```sql +ALTER TABLE test UPDATE v1 = v1 * 10 + 2 WHERE key LIKE 'some%' AND v3 > 3.1; +``` diff --git a/docs/en/engines/table-engines/special/keepermap.md b/docs/en/engines/table-engines/special/keepermap.md new file mode 100644 index 00000000000..680413039e7 --- /dev/null +++ b/docs/en/engines/table-engines/special/keepermap.md @@ -0,0 +1,111 @@ +--- +slug: /en/engines/table-engines/special/keeper-map +sidebar_position: 150 +sidebar_label: KeeperMap +--- + +# KeeperMap {#keepermap} + +This engine allows you to use Keeper/ZooKeeper cluster as consistent key-value store with linearizable writes and sequentially consistent reads. + +To enable KeeperMap storage engine, you need to define a ZooKeeper path where the tables will be stored using `` config. + +For example: + +```xml + + /keeper_map_tables + +``` + +where path can be any other valid ZooKeeper path. + +## Creating a Table {#table_engine-KeeperMap-creating-a-table} + +``` sql +CREATE TABLE [IF NOT EXISTS] [db.]table_name [ON CLUSTER cluster] +( + name1 [type1] [DEFAULT|MATERIALIZED|ALIAS expr1], + name2 [type2] [DEFAULT|MATERIALIZED|ALIAS expr2], + ... +) ENGINE = KeeperMap(root_path, [keys_limit]) PRIMARY KEY(primary_key_name) +``` + +Engine parameters: + +- `root_path` - ZooKeeper path where the `table_name` will be stored. +This path should not contain the prefix defined by `` config because the prefix will be automatically appended to the `root_path`. +Additionally, format of `auxiliary_zookeper_cluster_name:/some/path` is also supported where `auxiliary_zookeper_cluster` is a ZooKeeper cluster defined inside `` config. +By default, ZooKeeper cluster defined inside `` config is used. +- `keys_limit` - number of keys allowed inside the table. +This limit is a soft limit and it can be possible that more keys will end up in the table for some edge cases. +- `primary_key_name` – any column name in the column list. +- `primary key` must be specified, it supports only one column in the primary key. The primary key will be serialized in binary as a `node name` inside ZooKeeper. +- columns other than the primary key will be serialized to binary in corresponding order and stored as a value of the resulting node defined by the serialized key. +- queries with key `equals` or `in` filtering will be optimized to multi keys lookup from `Keeper`, otherwise all values will be fetched. + +Example: + +``` sql +CREATE TABLE keeper_map_table +( + `key` String, + `v1` UInt32, + `v2` String, + `v3` Float32 +) +ENGINE = KeeperMap(/keeper_map_table, 4) +PRIMARY KEY key +``` + +with + +```xml + + /keeper_map_tables + +``` + + +Each value, which is binary serialization of `(v1, v2, v3)`, will be stored inside `/keeper_map_tables/keeper_map_table/data/serialized_key` in `Keeper`. +Additionally, number of keys will have a soft limit of 4 for the number of keys. + +If multiple tables are created on the same ZooKeeper path, the values are persisted until there exists at least 1 table using it. +As a result, it is possible to use `ON CLUSTER` clause when creating the table and sharing the data from multiple ClickHouse instances. +Of course, it's possible to manually run `CREATE TABLE` with same path on nonrelated ClickHouse instances to have same data sharing effect. + +## Supported operations {#table_engine-KeeperMap-supported-operations} + +### Inserts + +When new rows are inserted into `KeeperMap`, if the key already exists, the value will be updated, otherwise new key is created. + +Example: + +```sql +INSERT INTO keeper_map_table VALUES ('some key', 1, 'value', 3.2); +``` + +### Deletes + +Rows can be deleted using `DELETE` query or `TRUNCATE`. + +```sql +DELETE FROM keeper_map_table WHERE key LIKE 'some%' AND v1 > 1; +``` + +```sql +ALTER TABLE keeper_map_table DELETE WHERE key LIKE 'some%' AND v1 > 1; +``` + +```sql +TRUNCATE TABLE keeper_map_table; +``` + +### Updates + +Values can be updated using `ALTER TABLE` query. Primary key cannot be updated. + +```sql +ALTER TABLE keeper_map_table UPDATE v1 = v1 * 10 + 2 WHERE key LIKE 'some%' AND v3 > 3.1; +``` From 4a3ad7da85b88f78082a8e9f529d9398ecb3c15c Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 15 Feb 2023 16:04:52 +0100 Subject: [PATCH 42/57] Fix test test_backup_restore_new/test.py::test_async_backups_to_same_destination[http]. --- .../test_backup_restore_new/test.py | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/tests/integration/test_backup_restore_new/test.py b/tests/integration/test_backup_restore_new/test.py index eea4abd719a..a903821071b 100644 --- a/tests/integration/test_backup_restore_new/test.py +++ b/tests/integration/test_backup_restore_new/test.py @@ -632,7 +632,8 @@ def test_async_backups_to_same_destination(interface): f"BACKUP TABLE test.table TO {backup_name} ASYNC" ) - # The second backup to the same destination is expected to fail. It can either fail immediately or after a while. + # One of those two backups to the same destination is expected to fail. + # If the second backup is going to fail it can fail either immediately or after a while. # If it fails immediately we won't even get its ID. id2 = None if err else res.split("\t")[0] @@ -647,17 +648,25 @@ def test_async_backups_to_same_destination(interface): "", ) - # The first backup should succeed. - assert instance.query( - f"SELECT status, error FROM system.backups WHERE id='{id1}'" - ) == TSV([["BACKUP_CREATED", ""]]) - - if id2: - # The second backup should fail. - assert ( - instance.query(f"SELECT status FROM system.backups WHERE id='{id2}'") - == "BACKUP_FAILED\n" + ids_succeeded = ( + instance.query( + f"SELECT id FROM system.backups WHERE id IN {ids_for_query} AND status == 'BACKUP_CREATED'" ) + .rstrip("\n") + .split("\n") + ) + + ids_failed = ( + instance.query( + f"SELECT id FROM system.backups WHERE id IN {ids_for_query} AND status == 'BACKUP_FAILED'" + ) + .rstrip("\n") + .split("\n") + ) + + assert len(ids_succeeded) == 1 + assert len(ids_failed) <= 1 + assert set(ids_succeeded + ids_failed) == set(ids) # Check that the first backup is all right. instance.query("DROP TABLE test.table") From 0496b550034d23d3cf7b00a057e783e61e726970 Mon Sep 17 00:00:00 2001 From: Dan Roscigno Date: Wed, 15 Feb 2023 13:29:26 -0500 Subject: [PATCH 43/57] Update docs/en/engines/table-engines/integrations/embedded-rocksdb.md --- docs/en/engines/table-engines/integrations/embedded-rocksdb.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/engines/table-engines/integrations/embedded-rocksdb.md b/docs/en/engines/table-engines/integrations/embedded-rocksdb.md index 35c59b8181a..4a662bd8ec9 100644 --- a/docs/en/engines/table-engines/integrations/embedded-rocksdb.md +++ b/docs/en/engines/table-engines/integrations/embedded-rocksdb.md @@ -89,7 +89,7 @@ You can also change any [rocksdb options](https://github.com/facebook/rocksdb/wi ### Inserts -When new rows are inserted into `EmbeddedRocksDB`, if the key already exists, the value will be updated, otherwise new key is created. +When new rows are inserted into `EmbeddedRocksDB`, if the key already exists, the value will be updated, otherwise a new key is created. Example: From 10c8f318113aaf01f62d8ae9cfab985b3c79bcc9 Mon Sep 17 00:00:00 2001 From: Dan Roscigno Date: Wed, 15 Feb 2023 13:29:33 -0500 Subject: [PATCH 44/57] Update docs/en/engines/table-engines/integrations/embedded-rocksdb.md --- docs/en/engines/table-engines/integrations/embedded-rocksdb.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/engines/table-engines/integrations/embedded-rocksdb.md b/docs/en/engines/table-engines/integrations/embedded-rocksdb.md index 4a662bd8ec9..a3604b3c332 100644 --- a/docs/en/engines/table-engines/integrations/embedded-rocksdb.md +++ b/docs/en/engines/table-engines/integrations/embedded-rocksdb.md @@ -115,7 +115,7 @@ TRUNCATE TABLE test; ### Updates -Values can be updated using `ALTER TABLE` query. Primary key cannot be updated. +Values can be updated using the `ALTER TABLE` query. The primary key cannot be updated. ```sql ALTER TABLE test UPDATE v1 = v1 * 10 + 2 WHERE key LIKE 'some%' AND v3 > 3.1; From c8d7b72e5d40f2a6a3a2e33d884e2dc48bf0990c Mon Sep 17 00:00:00 2001 From: Kevin Zhang Date: Wed, 15 Feb 2023 15:09:36 -0500 Subject: [PATCH 45/57] move database credential inputs to the center on initial load --- programs/server/dashboard.html | 93 ++++++++++++++++++++++++++++++---- 1 file changed, 83 insertions(+), 10 deletions(-) diff --git a/programs/server/dashboard.html b/programs/server/dashboard.html index 74bd9746a4d..a91040b2701 100644 --- a/programs/server/dashboard.html +++ b/programs/server/dashboard.html @@ -92,7 +92,52 @@ .chart div { position: absolute; } - .inputs { font-size: 14pt; } + .inputs { + height: auto; + width: 100%; + + font-size: 14pt; + + display: flex; + flex-flow: column nowrap; + justify-content: center; + } + + .inputs.unconnected { + height: 100vh; + } + .unconnected #params { + display: flex; + flex-flow: column nowrap; + justify-content: center; + align-items: center; + } + .unconnected #connection-params { + width: 50%; + + display: flex; + flex-flow: row wrap; + } + .unconnected #url { + width: 100%; + } + .unconnected #user { + width: 50%; + } + .unconnected #password { + width: 49.5%; + } + .unconnected input { + margin-bottom: 5px; + } + + .inputs #chart-params { + display: block; + } + + .inputs.unconnected #chart-params { + display: none; + } #connection-params { margin-bottom: 0.5rem; @@ -223,6 +268,10 @@ color: var(--chart-button-hover-color); } + .disabled { + opacity: 0.5; + } + .query-editor { display: none; grid-template-columns: auto fit-content(10%); @@ -286,7 +335,7 @@ -
+
@@ -294,8 +343,8 @@
- - + + 🌚🌞
@@ -845,7 +894,7 @@ async function draw(idx, chart, url_params, query) { error_div.firstChild.data = error; title_div.style.display = 'none'; error_div.style.display = 'block'; - return; + return false; } else { error_div.firstChild.data = ''; error_div.style.display = 'none'; @@ -886,6 +935,7 @@ async function draw(idx, chart, url_params, query) { /// Set title const title = queries[idx] && queries[idx].title ? queries[idx].title.replaceAll(/\{(\w+)\}/g, (_, name) => params[name] ) : ''; chart.querySelector('.title').firstChild.data = title; + return true } function showAuthError(message) { @@ -902,8 +952,6 @@ function showAuthError(message) { function hideAuthError() { const charts = document.querySelector('#charts'); charts.style.display = 'flex'; - const add = document.querySelector('#add'); - add.style.display = 'block'; const authError = document.querySelector('#auth-error'); authError.textContent = ''; @@ -924,9 +972,20 @@ async function drawAll() { if (!firstLoad) { showAuthError(e.message); } + return false; }); - })).then(() => { - firstLoad = false; + })).then((results) => { + if (firstLoad) { + firstLoad = false; + } else { + enableReloadButton(); + } + if (!results.includes(false)) { + const element = document.querySelector('.inputs'); + element.classList.remove('unconnected'); + const add = document.querySelector('#add'); + add.style.display = 'block'; + } }) } @@ -941,11 +1000,25 @@ function resize() { new ResizeObserver(resize).observe(document.body); +function disableReloadButton() { + const reloadButton = document.getElementById('reload') + reloadButton.value = 'Reloading...' + reloadButton.disabled = true + reloadButton.classList.add('disabled') +} + +function enableReloadButton() { + const reloadButton = document.getElementById('reload') + reloadButton.value = 'Reload' + reloadButton.disabled = false + reloadButton.classList.remove('disabled') +} + function reloadAll() { updateParams(); drawAll(); saveState(); - document.getElementById('reload').style.display = 'none'; + disableReloadButton() } document.getElementById('params').onsubmit = function(event) { From 922d7f2c2a828d65a81a77eaf088a84d6a08565d Mon Sep 17 00:00:00 2001 From: Rich Raposa Date: Wed, 15 Feb 2023 13:26:58 -0700 Subject: [PATCH 46/57] Update s3Cluster.md The explanation of how the cluster is used in a query seemed liked it was worded poorly. (It made it sound like you were querying data on a cluster of CH nodes.) I reworded it. --- .../en/sql-reference/table-functions/s3Cluster.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/en/sql-reference/table-functions/s3Cluster.md b/docs/en/sql-reference/table-functions/s3Cluster.md index b81fc51fd18..e77806a3665 100644 --- a/docs/en/sql-reference/table-functions/s3Cluster.md +++ b/docs/en/sql-reference/table-functions/s3Cluster.md @@ -27,18 +27,21 @@ A table with the specified structure for reading or writing data in the specifie **Examples** -Select the data from all files in the cluster `cluster_simple`: +Select the data from all the files in the `/root/data/clickhouse` and `/root/data/database/` folders, using all the nodes in the `cluster_simple` cluster: ``` sql -SELECT * FROM s3Cluster('cluster_simple', 'http://minio1:9001/root/data/{clickhouse,database}/*', 'minio', 'minio123', 'CSV', 'name String, value UInt32, polygon Array(Array(Tuple(Float64, Float64)))') ORDER BY (name, value, polygon); +SELECT * FROM s3Cluster( + 'cluster_simple', + 'http://minio1:9001/root/data/{clickhouse,database}/*', + 'minio', + 'minio123', + 'CSV', + 'name String, value UInt32, polygon Array(Array(Tuple(Float64, Float64)))') ORDER BY (name, value, polygon +); ``` Count the total amount of rows in all files in the cluster `cluster_simple`: -``` sql -SELECT count(*) FROM s3Cluster('cluster_simple', 'http://minio1:9001/root/data/{clickhouse,database}/*', 'minio', 'minio123', 'CSV', 'name String, value UInt32, polygon Array(Array(Tuple(Float64, Float64)))'); -``` - :::warning If your listing of files contains number ranges with leading zeros, use the construction with braces for each digit separately or use `?`. ::: From c89e30814c36d1fe6d57f1335f7d111b1724a235 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Thu, 16 Feb 2023 00:09:53 +0300 Subject: [PATCH 47/57] Update 01054_window_view_proc_tumble_to.sh --- tests/queries/0_stateless/01054_window_view_proc_tumble_to.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/01054_window_view_proc_tumble_to.sh b/tests/queries/0_stateless/01054_window_view_proc_tumble_to.sh index 156ff5a156e..fc6bec80e0f 100755 --- a/tests/queries/0_stateless/01054_window_view_proc_tumble_to.sh +++ b/tests/queries/0_stateless/01054_window_view_proc_tumble_to.sh @@ -12,7 +12,7 @@ DROP TABLE IF EXISTS wv; CREATE TABLE dst(count UInt64) Engine=MergeTree ORDER BY tuple(); CREATE TABLE mt(a Int32, timestamp DateTime) ENGINE=MergeTree ORDER BY tuple(); -CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY tumble(timestamp, INTERVAL '1' SECOND, 'US/Samoa') AS wid; +CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY tumble(timestamp, INTERVAL '5' SECOND, 'US/Samoa') AS wid; INSERT INTO mt VALUES (1, now('US/Samoa') + 1); EOF From 05eb877c5ce42f63a857e981330d332e11d4dd1e Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Thu, 16 Feb 2023 00:10:33 +0300 Subject: [PATCH 48/57] Update 01055_window_view_proc_hop_to.sh --- tests/queries/0_stateless/01055_window_view_proc_hop_to.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh b/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh index 3915e425010..2d5bff42be5 100755 --- a/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh +++ b/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh @@ -12,7 +12,7 @@ DROP TABLE IF EXISTS wv; CREATE TABLE dst(count UInt64) Engine=MergeTree ORDER BY tuple(); CREATE TABLE mt(a Int32, timestamp DateTime) ENGINE=MergeTree ORDER BY tuple(); -CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY hop(timestamp, INTERVAL '1' SECOND, INTERVAL '1' SECOND, 'US/Samoa') AS wid; +CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY hop(timestamp, INTERVAL '5' SECOND, INTERVAL '1' SECOND, 'US/Samoa') AS wid; INSERT INTO mt VALUES (1, now('US/Samoa') + 1); EOF From b167b8344be84601165db75b7b967c31dd42a277 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Thu, 16 Feb 2023 00:45:03 +0300 Subject: [PATCH 49/57] Update 01055_window_view_proc_hop_to.sh --- tests/queries/0_stateless/01055_window_view_proc_hop_to.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh b/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh index 2d5bff42be5..f7bbceeb8ff 100755 --- a/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh +++ b/tests/queries/0_stateless/01055_window_view_proc_hop_to.sh @@ -12,7 +12,7 @@ DROP TABLE IF EXISTS wv; CREATE TABLE dst(count UInt64) Engine=MergeTree ORDER BY tuple(); CREATE TABLE mt(a Int32, timestamp DateTime) ENGINE=MergeTree ORDER BY tuple(); -CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY hop(timestamp, INTERVAL '5' SECOND, INTERVAL '1' SECOND, 'US/Samoa') AS wid; +CREATE WINDOW VIEW wv TO dst AS SELECT count(a) AS count FROM mt GROUP BY hop(timestamp, INTERVAL '5' SECOND, INTERVAL '5' SECOND, 'US/Samoa') AS wid; INSERT INTO mt VALUES (1, now('US/Samoa') + 1); EOF From 0c5648a5a648d0b93e2401935c726839d4433064 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Wed, 15 Feb 2023 17:09:33 +0100 Subject: [PATCH 50/57] Use OK and FAIL for tests status --- tests/ci/install_check.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/ci/install_check.py b/tests/ci/install_check.py index 1444759cea0..c6ecd092aa7 100644 --- a/tests/ci/install_check.py +++ b/tests/ci/install_check.py @@ -36,6 +36,8 @@ DEB_IMAGE = "clickhouse/install-deb-test" TEMP_PATH = Path(TEMP) SUCCESS = "success" FAILURE = "failure" +OK = "OK" +FAIL = "FAIL" def prepare_test_scripts(): @@ -156,9 +158,9 @@ def test_install(image: DockerImage, tests: Dict[str, str]) -> TestResults: with TeePopen(install_command, log_file) as process: retcode = process.wait() if retcode == 0: - status = SUCCESS + status = OK else: - status = FAILURE + status = FAIL subprocess.check_call(f"docker kill -s 9 {container_id}", shell=True) test_results.append( @@ -266,9 +268,11 @@ def main(): test_results.extend(test_install_tgz(docker_images[RPM_IMAGE])) state = SUCCESS + test_status = OK description = "Packages installed successfully" - if FAILURE in (result.status for result in test_results): + if FAIL in (result.status for result in test_results): state = FAILURE + test_status = FAIL description = "Failed to install packages: " + ", ".join( result.name for result in test_results ) @@ -298,7 +302,7 @@ def main(): prepared_events = prepare_tests_results_for_clickhouse( pr_info, test_results, - state, + test_status, stopwatch.duration_seconds, stopwatch.start_time_str, report_url, From 3bcd049d189226b2f9719bec117123d9352931e8 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Wed, 15 Feb 2023 17:42:28 +0100 Subject: [PATCH 51/57] Improve download filter, group keeper mntr command --- tests/ci/install_check.py | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/tests/ci/install_check.py b/tests/ci/install_check.py index c6ecd092aa7..40f8c82132b 100644 --- a/tests/ci/install_check.py +++ b/tests/ci/install_check.py @@ -52,9 +52,11 @@ for i in {1..20}; do done for i in {1..5}; do echo wait for clickhouse-keeper to answer on mntr request - exec 13<>/dev/tcp/127.0.0.1/9181 - echo mntr >&13 - cat <&13 | grep zk_version && break || sleep 1 + { + exec 13<>/dev/tcp/127.0.0.1/9181 + echo mntr >&13 + cat <&13 | grep zk_version + } && break || sleep 1 exec 13>&- done exec 13>&-""" @@ -72,9 +74,11 @@ for i in {1..20}; do done for i in {1..5}; do echo wait for clickhouse-keeper to answer on mntr request - exec 13<>/dev/tcp/127.0.0.1/9181 - echo mntr >&13 - cat <&13 | grep zk_version && break || sleep 1 + { + exec 13<>/dev/tcp/127.0.0.1/9181 + echo mntr >&13 + cat <&13 | grep zk_version + } && break || sleep 1 exec 13>&- done exec 13>&-""" @@ -247,12 +251,16 @@ def main(): if args.download: def filter_artifacts(path: str) -> bool: - return ( - path.endswith(".deb") - or path.endswith(".rpm") - or path.endswith(".tgz") - or path.endswith("/clickhouse") - ) + is_match = False + if args.deb or args.rpm: + is_match = is_match or path.endswith("/clickhouse") + if args.deb: + is_match = is_match or path.endswith(".deb") + if args.rpm: + is_match = is_match or path.endswith(".rpm") + if args.tgz: + is_match = is_match or path.endswith(".tgz") + return is_match download_builds_filter( args.check_name, REPORTS_PATH, TEMP_PATH, filter_artifacts From f39d9b09b01ebcaf5ed527e59fe3ae7d685f76f2 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Wed, 15 Feb 2023 18:52:38 +0100 Subject: [PATCH 52/57] Preserve clickhouse binary to not redownload it --- tests/ci/install_check.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/ci/install_check.py b/tests/ci/install_check.py index 40f8c82132b..ce0d8454bc6 100644 --- a/tests/ci/install_check.py +++ b/tests/ci/install_check.py @@ -7,7 +7,7 @@ import logging import sys import subprocess from pathlib import Path - +from shutil import copy2 from typing import Dict from github import Github @@ -61,8 +61,7 @@ for i in {1..5}; do done exec 13>&-""" binary_test = r"""#!/bin/bash -chmod +x /packages/clickhouse -/packages/clickhouse install +/packages/clickhouse.copy install clickhouse-server start --daemon for i in {1..5}; do clickhouse-client -q 'SELECT version()' && break || sleep 1 @@ -267,6 +266,14 @@ def main(): ) test_results = [] # type: TestResults + ch_binary = Path(TEMP_PATH) / "clickhouse" + if ch_binary.exists(): + # make a copy of clickhouse to avoid redownload of exctracted binary + ch_binary.chmod(0o755) + ch_copy = ch_binary.parent / "clickhouse.copy" + copy2(ch_binary, ch_binary.parent / "clickhouse.copy") + subprocess.check_output(f"{ch_copy.absolute()} local -q 'SELECT 1'", shell=True) + if args.deb: test_results.extend(test_install_deb(docker_images[DEB_IMAGE])) if args.rpm: From 9dba4e7cc91960a21badba522e7742ee11cb37b8 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Wed, 15 Feb 2023 22:37:21 +0100 Subject: [PATCH 53/57] Replace format with f-strings --- tests/ci/compress_files.py | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/ci/compress_files.py b/tests/ci/compress_files.py index ced8879d6d9..8d52d030b84 100644 --- a/tests/ci/compress_files.py +++ b/tests/ci/compress_files.py @@ -6,11 +6,11 @@ import os def compress_file_fast(path, archive_path): if archive_path.endswith(".zst"): - subprocess.check_call("zstd < {} > {}".format(path, archive_path), shell=True) + subprocess.check_call(f"zstd < {path} > {archive_path}", shell=True) elif os.path.exists("/usr/bin/pigz"): - subprocess.check_call("pigz < {} > {}".format(path, archive_path), shell=True) + subprocess.check_call(f"pigz < {path} > {archive_path}", shell=True) else: - subprocess.check_call("gzip < {} > {}".format(path, archive_path), shell=True) + subprocess.check_call(f"gzip < {path} > {archive_path}", shell=True) def compress_fast(path, archive_path, exclude=None): @@ -28,9 +28,9 @@ def compress_fast(path, archive_path, exclude=None): if exclude is None: exclude_part = "" elif isinstance(exclude, list): - exclude_part = " ".join(["--exclude {}".format(x) for x in exclude]) + exclude_part = " ".join([f"--exclude {x}" for x in exclude]) else: - exclude_part = "--exclude {}".format(str(exclude)) + exclude_part = f"--exclude {exclude}" fname = os.path.basename(path) if os.path.isfile(path): @@ -38,9 +38,7 @@ def compress_fast(path, archive_path, exclude=None): else: path += "/.." - cmd = "tar {} {} -cf {} -C {} {}".format( - program_part, exclude_part, archive_path, path, fname - ) + cmd = f"tar {program_part} {exclude_part} -cf {archive_path} -C {path} {fname}" logging.debug("compress_fast cmd: %s", cmd) subprocess.check_call(cmd, shell=True) @@ -70,11 +68,9 @@ def decompress_fast(archive_path, result_path=None): ) if result_path is None: - subprocess.check_call( - "tar {} -xf {}".format(program_part, archive_path), shell=True - ) + subprocess.check_call(f"tar {program_part} -xf {archive_path}", shell=True) else: subprocess.check_call( - "tar {} -xf {} -C {}".format(program_part, archive_path, result_path), + f"tar {program_part} -xf {archive_path} -C {result_path}", shell=True, ) From 346c3e3c49159d9f091d0bcd60ed7681542d0a03 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Wed, 15 Feb 2023 22:47:31 +0100 Subject: [PATCH 54/57] Preserve logs for failed installation tests, retry 3 times --- tests/ci/install_check.py | 62 +++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/tests/ci/install_check.py b/tests/ci/install_check.py index ce0d8454bc6..48d0da195ce 100644 --- a/tests/ci/install_check.py +++ b/tests/ci/install_check.py @@ -19,6 +19,7 @@ from clickhouse_helper import ( prepare_tests_results_for_clickhouse, ) from commit_status_helper import post_commit_status, update_mergeable_check +from compress_files import compress_fast from docker_pull_helper import get_image_with_version, DockerImage from env_helper import CI, TEMP_PATH as TEMP, REPORTS_PATH from get_robot_token import get_best_robot_token @@ -34,6 +35,7 @@ from upload_result_helper import upload_results RPM_IMAGE = "clickhouse/install-rpm-test" DEB_IMAGE = "clickhouse/install-deb-test" TEMP_PATH = Path(TEMP) +LOGS_PATH = TEMP_PATH / "tests_logs" SUCCESS = "success" FAILURE = "failure" OK = "OK" @@ -42,9 +44,13 @@ FAIL = "FAIL" def prepare_test_scripts(): server_test = r"""#!/bin/bash +set -e +trap "bash -ex /packages/preserve_logs.sh" ERR systemctl start clickhouse-server clickhouse-client -q 'SELECT version()'""" keeper_test = r"""#!/bin/bash +set -e +trap "bash -ex /packages/preserve_logs.sh" ERR systemctl start clickhouse-keeper for i in {1..20}; do echo wait for clickhouse-keeper to being up @@ -61,6 +67,8 @@ for i in {1..5}; do done exec 13>&-""" binary_test = r"""#!/bin/bash +set -e +trap "bash -ex /packages/preserve_logs.sh" ERR /packages/clickhouse.copy install clickhouse-server start --daemon for i in {1..5}; do @@ -81,9 +89,18 @@ for i in {1..5}; do exec 13>&- done exec 13>&-""" + preserve_logs = r"""#!/bin/bash +journalctl -u clickhouse-server > /tests_logs/clickhouse-server.service || : +journalctl -u clickhouse-keeper > /tests_logs/clickhouse-keeper.service || : +cp /var/log/clickhouse-server/clickhouse-server.* /tests_logs/ || : +cp /var/log/clickhouse-keeper/clickhouse-keeper.* /tests_logs/ || : +chmod a+rw -R /tests_logs +exit 1 +""" (TEMP_PATH / "server_test.sh").write_text(server_test, encoding="utf-8") (TEMP_PATH / "keeper_test.sh").write_text(keeper_test, encoding="utf-8") (TEMP_PATH / "binary_test.sh").write_text(binary_test, encoding="utf-8") + (TEMP_PATH / "preserve_logs.sh").write_text(preserve_logs, encoding="utf-8") def test_install_deb(image: DockerImage) -> TestResults: @@ -148,27 +165,41 @@ def test_install(image: DockerImage, tests: Dict[str, str]) -> TestResults: stopwatch = Stopwatch() container_name = name.lower().replace(" ", "_").replace("/", "_") log_file = TEMP_PATH / f"{container_name}.log" + logs = [log_file] run_command = ( f"docker run --rm --privileged --detach --cap-add=SYS_PTRACE " - f"--volume={TEMP_PATH}:/packages {image}" + f"--volume={LOGS_PATH}:/tests_logs --volume={TEMP_PATH}:/packages {image}" ) - logging.info("Running docker container: `%s`", run_command) - container_id = subprocess.check_output( - run_command, shell=True, encoding="utf-8" - ).strip() - (TEMP_PATH / "install.sh").write_text(command) - install_command = f"docker exec {container_id} bash -ex /packages/install.sh" - with TeePopen(install_command, log_file) as process: - retcode = process.wait() - if retcode == 0: - status = OK - else: + + for retry in range(1, 4): + for file in LOGS_PATH.glob("*"): + file.unlink() + + logging.info("Running docker container: `%s`", run_command) + container_id = subprocess.check_output( + run_command, shell=True, encoding="utf-8" + ).strip() + (TEMP_PATH / "install.sh").write_text(command) + install_command = ( + f"docker exec {container_id} bash -ex /packages/install.sh" + ) + with TeePopen(install_command, log_file) as process: + retcode = process.wait() + if retcode == 0: + status = OK + break + status = FAIL + copy2(log_file, LOGS_PATH) + archive_path = TEMP_PATH / f"{container_name}-{retry}.tar.gz" + compress_fast( + LOGS_PATH.as_posix(), + archive_path.as_posix(), + ) + logs.append(archive_path) subprocess.check_call(f"docker kill -s 9 {container_id}", shell=True) - test_results.append( - TestResult(name, status, stopwatch.duration_seconds, [log_file]) - ) + test_results.append(TestResult(name, status, stopwatch.duration_seconds, logs)) return test_results @@ -227,6 +258,7 @@ def main(): args = parse_args() TEMP_PATH.mkdir(parents=True, exist_ok=True) + LOGS_PATH.mkdir(parents=True, exist_ok=True) pr_info = PRInfo() From 53e32b19e651d5cfcf457397a0d0a294cecd7d9f Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Thu, 16 Feb 2023 02:13:18 +0300 Subject: [PATCH 55/57] Update AggregatingTransform.cpp --- src/Processors/Transforms/AggregatingTransform.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Processors/Transforms/AggregatingTransform.cpp b/src/Processors/Transforms/AggregatingTransform.cpp index 836458ef792..755af34ad01 100644 --- a/src/Processors/Transforms/AggregatingTransform.cpp +++ b/src/Processors/Transforms/AggregatingTransform.cpp @@ -555,7 +555,7 @@ void AggregatingTransform::initGenerate() double elapsed_seconds = watch.elapsedSeconds(); size_t rows = variants.sizeWithoutOverflowRow(); - LOG_DEBUG(log, "Aggregated. {} to {} rows (from {}) in {} sec. ({:.3f} rows/sec., {}/sec.)", + LOG_TRACE(log, "Aggregated. {} to {} rows (from {}) in {} sec. ({:.3f} rows/sec., {}/sec.)", src_rows, rows, ReadableSize(src_bytes), elapsed_seconds, src_rows / elapsed_seconds, ReadableSize(src_bytes / elapsed_seconds)); From 7de008a58a74a0ac19a27212fbb3eff2a3f45c2f Mon Sep 17 00:00:00 2001 From: SuperDJY Date: Thu, 16 Feb 2023 12:46:24 +0800 Subject: [PATCH 56/57] Fix systemd service file wrong inline comment There is no inline comment in systemd unit file. --- packages/clickhouse-server.service | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/clickhouse-server.service b/packages/clickhouse-server.service index 5ea30c062b0..12c91bf6fb6 100644 --- a/packages/clickhouse-server.service +++ b/packages/clickhouse-server.service @@ -17,7 +17,8 @@ User=clickhouse Group=clickhouse Restart=always RestartSec=30 -RuntimeDirectory=%p # %p is resolved to the systemd unit name + # %p is resolved to the systemd unit name +RuntimeDirectory=%p ExecStart=/usr/bin/clickhouse-server --config=/etc/clickhouse-server/config.xml --pid-file=%t/%p/%p.pid # Minus means that this file is optional. EnvironmentFile=-/etc/default/%p From 1ad9e217e5c838913743ce0a868f5f17f659ad2a Mon Sep 17 00:00:00 2001 From: cmsxbc Date: Thu, 16 Feb 2023 19:23:16 +0800 Subject: [PATCH 57/57] chore: remove starting blank --- packages/clickhouse-server.service | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/clickhouse-server.service b/packages/clickhouse-server.service index 12c91bf6fb6..ace304e0c5e 100644 --- a/packages/clickhouse-server.service +++ b/packages/clickhouse-server.service @@ -17,7 +17,7 @@ User=clickhouse Group=clickhouse Restart=always RestartSec=30 - # %p is resolved to the systemd unit name +# %p is resolved to the systemd unit name RuntimeDirectory=%p ExecStart=/usr/bin/clickhouse-server --config=/etc/clickhouse-server/config.xml --pid-file=%t/%p/%p.pid # Minus means that this file is optional.