From 0d6561ee77041725f12d350e620c1d126563e82e Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Mon, 17 Apr 2023 16:52:30 +0200 Subject: [PATCH 001/101] Prototype --- src/Common/ZooKeeper/ZooKeeperCommon.cpp | 1 + src/Common/ZooKeeper/ZooKeeperCommon.h | 9 +++++-- src/Common/ZooKeeper/ZooKeeperConstants.cpp | 3 +++ src/Common/ZooKeeper/ZooKeeperConstants.h | 1 + src/Coordination/KeeperStorage.cpp | 27 ++++++++++++++++++--- 5 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.cpp b/src/Common/ZooKeeper/ZooKeeperCommon.cpp index 5031af38812..527b04c8c43 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.cpp +++ b/src/Common/ZooKeeper/ZooKeeperCommon.cpp @@ -962,6 +962,7 @@ ZooKeeperRequestFactory::ZooKeeperRequestFactory() registerZooKeeperRequest(*this); registerZooKeeperRequest(*this); registerZooKeeperRequest(*this); + registerZooKeeperRequest(*this); registerZooKeeperRequest(*this); registerZooKeeperRequest(*this); registerZooKeeperRequest(*this); diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.h b/src/Common/ZooKeeper/ZooKeeperCommon.h index 5f00698423e..69f4dd84860 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.h +++ b/src/Common/ZooKeeper/ZooKeeperCommon.h @@ -194,7 +194,7 @@ struct ZooKeeperCloseResponse final : ZooKeeperResponse OpNum getOpNum() const override { return OpNum::Close; } }; -struct ZooKeeperCreateRequest final : public CreateRequest, ZooKeeperRequest +struct ZooKeeperCreateRequest : public CreateRequest, ZooKeeperRequest { /// used only during restore from zookeeper log int32_t parent_cversion = -1; @@ -215,7 +215,7 @@ struct ZooKeeperCreateRequest final : public CreateRequest, ZooKeeperRequest void createLogElements(LogElements & elems) const override; }; -struct ZooKeeperCreateResponse final : CreateResponse, ZooKeeperResponse +struct ZooKeeperCreateResponse : CreateResponse, ZooKeeperResponse { void readImpl(ReadBuffer & in) override; @@ -228,6 +228,11 @@ struct ZooKeeperCreateResponse final : CreateResponse, ZooKeeperResponse void fillLogElements(LogElements & elems, size_t idx) const override; }; +struct ZooKeeperCreateIfNotExistsRequest final : public ZooKeeperCreateRequest +{ + OpNum getOpNum() const override { return OpNum::CreateIfNotExists; } +}; + struct ZooKeeperRemoveRequest final : RemoveRequest, ZooKeeperRequest { ZooKeeperRemoveRequest() = default; diff --git a/src/Common/ZooKeeper/ZooKeeperConstants.cpp b/src/Common/ZooKeeper/ZooKeeperConstants.cpp index 86f70ea547a..334afde52f2 100644 --- a/src/Common/ZooKeeper/ZooKeeperConstants.cpp +++ b/src/Common/ZooKeeper/ZooKeeperConstants.cpp @@ -21,6 +21,7 @@ static const std::unordered_set VALID_OPERATIONS = static_cast(OpNum::Check), static_cast(OpNum::Multi), static_cast(OpNum::MultiRead), + static_cast(OpNum::CreateIfNotExists), static_cast(OpNum::Auth), static_cast(OpNum::SessionID), static_cast(OpNum::SetACL), @@ -57,6 +58,8 @@ std::string toString(OpNum op_num) return "Multi"; case OpNum::MultiRead: return "MultiRead"; + case OpNum::CreateIfNotExists: + return "CreateIfNotExists"; case OpNum::Sync: return "Sync"; case OpNum::Heartbeat: diff --git a/src/Common/ZooKeeper/ZooKeeperConstants.h b/src/Common/ZooKeeper/ZooKeeperConstants.h index 6b50c5c5d09..6582e58e92c 100644 --- a/src/Common/ZooKeeper/ZooKeeperConstants.h +++ b/src/Common/ZooKeeper/ZooKeeperConstants.h @@ -37,6 +37,7 @@ enum class OpNum : int32_t // CH Keeper specific operations FilteredList = 500, CheckNotExists = 501, + CreateIfNotExists = 502, SessionID = 997, /// Special internal request }; diff --git a/src/Coordination/KeeperStorage.cpp b/src/Coordination/KeeperStorage.cpp index 7a1a5e42632..e0c69933337 100644 --- a/src/Coordination/KeeperStorage.cpp +++ b/src/Coordination/KeeperStorage.cpp @@ -1001,11 +1001,25 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr Coordination::ZooKeeperResponsePtr process(KeeperStorage & storage, int64_t zxid) const override { Coordination::ZooKeeperResponsePtr response_ptr = zk_request->makeResponse(); - Coordination::ZooKeeperCreateResponse & response = dynamic_cast(*response_ptr); + Coordination::ZooKeeperCreateResponse * response = dynamic_cast(response_ptr.get()); + + Coordination::ZooKeeperCreateIfNotExistsRequest * create_if_not_exists_request = dynamic_cast(zk_request.get()); + + if (create_if_not_exists_request != nullptr) { + Coordination::ZooKeeperCreateIfNotExistsRequest & request = dynamic_cast(*zk_request); + + auto & container = storage.container; + auto node_it = container.find(request.path); + if (node_it != container.end()) + { + response->error = Coordination::Error::ZOK; + return response_ptr; + } + } if (const auto result = storage.commit(zxid); result != Coordination::Error::ZOK) { - response.error = result; + response->error = result; return response_ptr; } @@ -1016,8 +1030,8 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr [zxid](const auto & delta) { return delta.zxid == zxid && std::holds_alternative(delta.operation); }); - response.path_created = create_delta_it->path; - response.error = Coordination::Error::ZOK; + response->path_created = create_delta_it->path; + response->error = Coordination::Error::ZOK; return response_ptr; } }; @@ -1730,6 +1744,10 @@ struct KeeperStorageMultiRequestProcessor final : public KeeperStorageRequestPro check_operation_type(OperationType::Write); concrete_requests.push_back(std::make_shared(sub_zk_request)); break; + case Coordination::OpNum::CreateIfNotExists: + check_operation_type(OperationType::Write); + concrete_requests.push_back(std::make_shared(sub_zk_request)); + break; case Coordination::OpNum::Remove: check_operation_type(OperationType::Write); concrete_requests.push_back(std::make_shared(sub_zk_request)); @@ -1993,6 +2011,7 @@ KeeperStorageRequestProcessorsFactory::KeeperStorageRequestProcessorsFactory() registerKeeperRequestProcessor(*this); registerKeeperRequestProcessor(*this); registerKeeperRequestProcessor(*this); + registerKeeperRequestProcessor(*this); registerKeeperRequestProcessor(*this); registerKeeperRequestProcessor(*this); registerKeeperRequestProcessor(*this); From b53f36369e9a148634d6b4c1fa9526b3bcd3c67c Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Mon, 17 Apr 2023 17:47:57 +0200 Subject: [PATCH 002/101] Remove new request object --- src/Common/ZooKeeper/ZooKeeperCommon.cpp | 2 +- src/Common/ZooKeeper/ZooKeeperCommon.h | 9 ++------- src/Coordination/KeeperStorage.cpp | 16 +++++++--------- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.cpp b/src/Common/ZooKeeper/ZooKeeperCommon.cpp index 527b04c8c43..c148b68b95e 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.cpp +++ b/src/Common/ZooKeeper/ZooKeeperCommon.cpp @@ -962,7 +962,7 @@ ZooKeeperRequestFactory::ZooKeeperRequestFactory() registerZooKeeperRequest(*this); registerZooKeeperRequest(*this); registerZooKeeperRequest(*this); - registerZooKeeperRequest(*this); + registerZooKeeperRequest(*this); registerZooKeeperRequest(*this); registerZooKeeperRequest(*this); registerZooKeeperRequest(*this); diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.h b/src/Common/ZooKeeper/ZooKeeperCommon.h index 69f4dd84860..5f00698423e 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.h +++ b/src/Common/ZooKeeper/ZooKeeperCommon.h @@ -194,7 +194,7 @@ struct ZooKeeperCloseResponse final : ZooKeeperResponse OpNum getOpNum() const override { return OpNum::Close; } }; -struct ZooKeeperCreateRequest : public CreateRequest, ZooKeeperRequest +struct ZooKeeperCreateRequest final : public CreateRequest, ZooKeeperRequest { /// used only during restore from zookeeper log int32_t parent_cversion = -1; @@ -215,7 +215,7 @@ struct ZooKeeperCreateRequest : public CreateRequest, ZooKeeperRequest void createLogElements(LogElements & elems) const override; }; -struct ZooKeeperCreateResponse : CreateResponse, ZooKeeperResponse +struct ZooKeeperCreateResponse final : CreateResponse, ZooKeeperResponse { void readImpl(ReadBuffer & in) override; @@ -228,11 +228,6 @@ struct ZooKeeperCreateResponse : CreateResponse, ZooKeeperResponse void fillLogElements(LogElements & elems, size_t idx) const override; }; -struct ZooKeeperCreateIfNotExistsRequest final : public ZooKeeperCreateRequest -{ - OpNum getOpNum() const override { return OpNum::CreateIfNotExists; } -}; - struct ZooKeeperRemoveRequest final : RemoveRequest, ZooKeeperRequest { ZooKeeperRemoveRequest() = default; diff --git a/src/Coordination/KeeperStorage.cpp b/src/Coordination/KeeperStorage.cpp index e0c69933337..825fcca42f5 100644 --- a/src/Coordination/KeeperStorage.cpp +++ b/src/Coordination/KeeperStorage.cpp @@ -1001,25 +1001,23 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr Coordination::ZooKeeperResponsePtr process(KeeperStorage & storage, int64_t zxid) const override { Coordination::ZooKeeperResponsePtr response_ptr = zk_request->makeResponse(); - Coordination::ZooKeeperCreateResponse * response = dynamic_cast(response_ptr.get()); + Coordination::ZooKeeperCreateResponse & response = dynamic_cast(*response_ptr); - Coordination::ZooKeeperCreateIfNotExistsRequest * create_if_not_exists_request = dynamic_cast(zk_request.get()); - - if (create_if_not_exists_request != nullptr) { - Coordination::ZooKeeperCreateIfNotExistsRequest & request = dynamic_cast(*zk_request); + if (zk_request->getOpNum() == Coordination::OpNum::CreateIfNotExists) { + Coordination::ZooKeeperCreateRequest & request = dynamic_cast(*zk_request); auto & container = storage.container; auto node_it = container.find(request.path); if (node_it != container.end()) { - response->error = Coordination::Error::ZOK; + response.error = Coordination::Error::ZOK; return response_ptr; } } if (const auto result = storage.commit(zxid); result != Coordination::Error::ZOK) { - response->error = result; + response.error = result; return response_ptr; } @@ -1030,8 +1028,8 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr [zxid](const auto & delta) { return delta.zxid == zxid && std::holds_alternative(delta.operation); }); - response->path_created = create_delta_it->path; - response->error = Coordination::Error::ZOK; + response.path_created = create_delta_it->path; + response.error = Coordination::Error::ZOK; return response_ptr; } }; From f84fdb7f1079c688278d1378d677ecdc60bd942b Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Mon, 17 Apr 2023 17:48:22 +0200 Subject: [PATCH 003/101] Increment Keeper API version --- src/Coordination/KeeperConstants.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Coordination/KeeperConstants.h b/src/Coordination/KeeperConstants.h index 4b5a5b54be0..afe3fcfb4c9 100644 --- a/src/Coordination/KeeperConstants.h +++ b/src/Coordination/KeeperConstants.h @@ -11,9 +11,10 @@ enum class KeeperApiVersion : uint8_t WITH_FILTERED_LIST, WITH_MULTI_READ, WITH_CHECK_NOT_EXISTS, + WITH_CREATE_IF_NOT_EXISTS }; -inline constexpr auto current_keeper_api_version = KeeperApiVersion::WITH_CHECK_NOT_EXISTS; +inline constexpr auto current_keeper_api_version = KeeperApiVersion::WITH_CREATE_IF_NOT_EXISTS; const std::string keeper_system_path = "/keeper"; const std::string keeper_api_version_path = keeper_system_path + "/api_version"; From 965f7850f8b7d5150bfedaffc11f9a5d4c2fe6fd Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Thu, 20 Apr 2023 13:26:52 +0200 Subject: [PATCH 004/101] Cleanup --- src/Common/ZooKeeper/ZooKeeperCommon.cpp | 2 ++ src/Common/ZooKeeper/ZooKeeperCommon.h | 5 ++++- src/Coordination/KeeperStorage.cpp | 21 +++++++++++---------- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.cpp b/src/Common/ZooKeeper/ZooKeeperCommon.cpp index c148b68b95e..89999f7d56b 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.cpp +++ b/src/Common/ZooKeeper/ZooKeeperCommon.cpp @@ -941,6 +941,8 @@ void registerZooKeeperRequest(ZooKeeperRequestFactory & factory) res->operation_type = ZooKeeperMultiRequest::OperationType::Write; else if constexpr (num == OpNum::CheckNotExists) res->not_exists = true; + else if constexpr (num == OpNum::CreateIfNotExists) + res->not_exists = true; return res; }); diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.h b/src/Common/ZooKeeper/ZooKeeperCommon.h index 5f00698423e..ee4eab1156c 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.h +++ b/src/Common/ZooKeeper/ZooKeeperCommon.h @@ -199,10 +199,13 @@ struct ZooKeeperCreateRequest final : public CreateRequest, ZooKeeperRequest /// used only during restore from zookeeper log int32_t parent_cversion = -1; + /// should it fail if node already exists + bool not_exists = false; + ZooKeeperCreateRequest() = default; explicit ZooKeeperCreateRequest(const CreateRequest & base) : CreateRequest(base) {} - OpNum getOpNum() const override { return OpNum::Create; } + OpNum getOpNum() const override { return not_exists ? OpNum::CreateIfNotExists : OpNum::Create; } void writeImpl(WriteBuffer & out) const override; void readImpl(ReadBuffer & in) override; std::string toStringImpl() const override; diff --git a/src/Coordination/KeeperStorage.cpp b/src/Coordination/KeeperStorage.cpp index 825fcca42f5..8bfe1e667b4 100644 --- a/src/Coordination/KeeperStorage.cpp +++ b/src/Coordination/KeeperStorage.cpp @@ -917,6 +917,15 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr std::vector new_deltas; + if (zk_request->getOpNum() == Coordination::OpNum::CreateIfNotExists) { + auto & container = storage.container; + auto node_it = container.find(request.path); + if (node_it != container.end()) + { + return new_deltas; + } + } + auto parent_path = parentPath(request.path); auto parent_node = storage.uncommitted_state.getNode(parent_path); if (parent_node == nullptr) @@ -1003,16 +1012,8 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr Coordination::ZooKeeperResponsePtr response_ptr = zk_request->makeResponse(); Coordination::ZooKeeperCreateResponse & response = dynamic_cast(*response_ptr); - if (zk_request->getOpNum() == Coordination::OpNum::CreateIfNotExists) { - Coordination::ZooKeeperCreateRequest & request = dynamic_cast(*zk_request); - - auto & container = storage.container; - auto node_it = container.find(request.path); - if (node_it != container.end()) - { - response.error = Coordination::Error::ZOK; - return response_ptr; - } + if (storage.uncommitted_state.deltas.begin()->zxid != zxid) { + return response_ptr; } if (const auto result = storage.commit(zxid); result != Coordination::Error::ZOK) From 7e151428acb257dbaab24940351159c9b50b6a9c Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Fri, 21 Apr 2023 15:10:38 +0200 Subject: [PATCH 005/101] Lint --- src/Common/ZooKeeper/ZooKeeperCommon.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.cpp b/src/Common/ZooKeeper/ZooKeeperCommon.cpp index 89999f7d56b..61b9d6e5172 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.cpp +++ b/src/Common/ZooKeeper/ZooKeeperCommon.cpp @@ -1,4 +1,5 @@ #include "Common/ZooKeeper/IKeeper.h" +#include "Common/ZooKeeper/ZooKeeperConstants.h" #include #include #include @@ -939,9 +940,7 @@ void registerZooKeeperRequest(ZooKeeperRequestFactory & factory) res->operation_type = ZooKeeperMultiRequest::OperationType::Read; else if constexpr (num == OpNum::Multi) res->operation_type = ZooKeeperMultiRequest::OperationType::Write; - else if constexpr (num == OpNum::CheckNotExists) - res->not_exists = true; - else if constexpr (num == OpNum::CreateIfNotExists) + else if constexpr (num == OpNum::CheckNotExists || num == OpNum::CreateIfNotExists) res->not_exists = true; return res; From 203276dfcc40b9d4f2b19a1eb0af430e612e8949 Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Fri, 21 Apr 2023 15:18:39 +0200 Subject: [PATCH 006/101] Use `storage.uncommited_state` --- src/Coordination/KeeperStorage.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/Coordination/KeeperStorage.cpp b/src/Coordination/KeeperStorage.cpp index 8bfe1e667b4..308f6dcf815 100644 --- a/src/Coordination/KeeperStorage.cpp +++ b/src/Coordination/KeeperStorage.cpp @@ -918,12 +918,14 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr std::vector new_deltas; if (zk_request->getOpNum() == Coordination::OpNum::CreateIfNotExists) { - auto & container = storage.container; - auto node_it = container.find(request.path); - if (node_it != container.end()) - { + // auto & container = storage.container; + // auto node_it = container.find(request.path); + // if (node_it != container.end()) + // { + // return new_deltas; + // } + if (storage.uncommitted_state.getNode(request.path) != nullptr) return new_deltas; - } } auto parent_path = parentPath(request.path); From 2b94418dc3e3a891212a7a677e11930b987e2f34 Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Fri, 21 Apr 2023 15:19:08 +0200 Subject: [PATCH 007/101] Add new OpNum to ZooKeeperLogElement --- src/Interpreters/ZooKeeperLog.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Interpreters/ZooKeeperLog.cpp b/src/Interpreters/ZooKeeperLog.cpp index 48f4d510af7..880b9d3686d 100644 --- a/src/Interpreters/ZooKeeperLog.cpp +++ b/src/Interpreters/ZooKeeperLog.cpp @@ -88,6 +88,7 @@ NamesAndTypesList ZooKeeperLogElement::getNamesAndTypes() {"SessionID", static_cast(Coordination::OpNum::SessionID)}, {"FilteredList", static_cast(Coordination::OpNum::FilteredList)}, {"CheckNotExists", static_cast(Coordination::OpNum::CheckNotExists)}, + {"CreateIfNotExists", static_cast(Coordination::OpNum::CreateIfNotExists)}, }); auto error_enum = getCoordinationErrorCodesEnumType(); From bd2718c79dc0dbbd84b81f8086082f8164f887e7 Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Mon, 8 May 2023 17:55:49 +0200 Subject: [PATCH 008/101] Fix comment --- src/Common/ZooKeeper/ZooKeeperCommon.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.h b/src/Common/ZooKeeper/ZooKeeperCommon.h index ee4eab1156c..264b2bb9606 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.h +++ b/src/Common/ZooKeeper/ZooKeeperCommon.h @@ -199,7 +199,7 @@ struct ZooKeeperCreateRequest final : public CreateRequest, ZooKeeperRequest /// used only during restore from zookeeper log int32_t parent_cversion = -1; - /// should it fail if node already exists + /// should it succeed if node already exists bool not_exists = false; ZooKeeperCreateRequest() = default; From 3a3539b9965012752f955e956e12624b54083bd2 Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Mon, 8 May 2023 18:22:35 +0200 Subject: [PATCH 009/101] Style fix --- src/Coordination/KeeperStorage.cpp | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/Coordination/KeeperStorage.cpp b/src/Coordination/KeeperStorage.cpp index 308f6dcf815..f368c11a2c6 100644 --- a/src/Coordination/KeeperStorage.cpp +++ b/src/Coordination/KeeperStorage.cpp @@ -917,16 +917,8 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr std::vector new_deltas; - if (zk_request->getOpNum() == Coordination::OpNum::CreateIfNotExists) { - // auto & container = storage.container; - // auto node_it = container.find(request.path); - // if (node_it != container.end()) - // { - // return new_deltas; - // } - if (storage.uncommitted_state.getNode(request.path) != nullptr) - return new_deltas; - } + if (zk_request->getOpNum() == Coordination::OpNum::CreateIfNotExists && storage.uncommitted_state.getNode(request.path) != nullptr) + return new_deltas; auto parent_path = parentPath(request.path); auto parent_node = storage.uncommitted_state.getNode(parent_path); @@ -1014,9 +1006,8 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr Coordination::ZooKeeperResponsePtr response_ptr = zk_request->makeResponse(); Coordination::ZooKeeperCreateResponse & response = dynamic_cast(*response_ptr); - if (storage.uncommitted_state.deltas.begin()->zxid != zxid) { + if (storage.uncommitted_state.deltas.begin()->zxid != zxid) return response_ptr; - } if (const auto result = storage.commit(zxid); result != Coordination::Error::ZOK) { From b61ffe3ff7873fe17a33d8ce90a795bd1d6879b6 Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Tue, 9 May 2023 18:11:55 +0200 Subject: [PATCH 010/101] Merge checks --- src/Coordination/KeeperStorage.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Coordination/KeeperStorage.cpp b/src/Coordination/KeeperStorage.cpp index f368c11a2c6..53df5451e67 100644 --- a/src/Coordination/KeeperStorage.cpp +++ b/src/Coordination/KeeperStorage.cpp @@ -917,9 +917,6 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr std::vector new_deltas; - if (zk_request->getOpNum() == Coordination::OpNum::CreateIfNotExists && storage.uncommitted_state.getNode(request.path) != nullptr) - return new_deltas; - auto parent_path = parentPath(request.path); auto parent_node = storage.uncommitted_state.getNode(parent_path); if (parent_node == nullptr) @@ -949,7 +946,12 @@ struct KeeperStorageCreateRequestProcessor final : public KeeperStorageRequestPr } if (storage.uncommitted_state.getNode(path_created)) + { + if (zk_request->getOpNum() == Coordination::OpNum::CreateIfNotExists) + return new_deltas; + return {KeeperStorage::Delta{zxid, Coordination::Error::ZNODEEXISTS}}; + } if (getBaseName(path_created).size == 0) return {KeeperStorage::Delta{zxid, Coordination::Error::ZBADARGUMENTS}}; From b2324d723e02616d472f4b0c7622c99f4acd6296 Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Tue, 30 May 2023 21:09:37 +0200 Subject: [PATCH 011/101] Use native `createIfNotExists` for `createAncestors` if available --- src/Common/ZooKeeper/IKeeper.h | 3 +++ src/Common/ZooKeeper/Types.h | 2 +- src/Common/ZooKeeper/ZooKeeper.cpp | 30 +++++++++++++++++++++++- src/Common/ZooKeeper/ZooKeeperCommon.cpp | 8 ++++++- src/Common/ZooKeeper/ZooKeeperCommon.h | 11 +++++---- 5 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/Common/ZooKeeper/IKeeper.h b/src/Common/ZooKeeper/IKeeper.h index 3eb5819df90..efc05fd7db1 100644 --- a/src/Common/ZooKeeper/IKeeper.h +++ b/src/Common/ZooKeeper/IKeeper.h @@ -199,6 +199,9 @@ struct CreateRequest : virtual Request bool is_sequential = false; ACLs acls; + /// should it succeed if node already exists + bool not_exists = false; + void addRootPath(const String & root_path) override; String getPath() const override { return path; } diff --git a/src/Common/ZooKeeper/Types.h b/src/Common/ZooKeeper/Types.h index 0309f56ad5b..d2876adaabc 100644 --- a/src/Common/ZooKeeper/Types.h +++ b/src/Common/ZooKeeper/Types.h @@ -29,7 +29,7 @@ using EventPtr = std::shared_ptr; template using AsyncResponses = std::vector>>; -Coordination::RequestPtr makeCreateRequest(const std::string & path, const std::string & data, int create_mode); +Coordination::RequestPtr makeCreateRequest(const std::string & path, const std::string & data, int create_mode, bool ignore_if_exists = false); Coordination::RequestPtr makeRemoveRequest(const std::string & path, int version); Coordination::RequestPtr makeSetRequest(const std::string & path, const std::string & data, int version); Coordination::RequestPtr makeCheckRequest(const std::string & path, int version); diff --git a/src/Common/ZooKeeper/ZooKeeper.cpp b/src/Common/ZooKeeper/ZooKeeper.cpp index a587ad6caf4..3357c75f50a 100644 --- a/src/Common/ZooKeeper/ZooKeeper.cpp +++ b/src/Common/ZooKeeper/ZooKeeper.cpp @@ -1,4 +1,5 @@ #include "ZooKeeper.h" +#include "Coordination/KeeperConstants.h" #include "ZooKeeperImpl.h" #include "KeeperException.h" #include "TestKeeper.h" @@ -351,6 +352,32 @@ void ZooKeeper::createIfNotExists(const std::string & path, const std::string & void ZooKeeper::createAncestors(const std::string & path) { size_t pos = 1; + + if (getApiVersion() >= DB::KeeperApiVersion::WITH_CREATE_IF_NOT_EXISTS) + { + Coordination::Requests create_ops; + + while (true) + { + pos = path.find('/', pos); + if (pos == std::string::npos) + break; + + auto request = makeCreateRequest(path.substr(0, pos), "", CreateMode::Persistent, true); + create_ops.emplace_back(request); + + ++pos; + } + + Coordination::Responses responses; + Coordination::Error code = multiImpl(create_ops, responses); + + if (code == Coordination::Error::ZOK) + return; + + throw KeeperException(code, path); + } + while (true) { pos = path.find('/', pos); @@ -1261,13 +1288,14 @@ void KeeperMultiException::check( } -Coordination::RequestPtr makeCreateRequest(const std::string & path, const std::string & data, int create_mode) +Coordination::RequestPtr makeCreateRequest(const std::string & path, const std::string & data, int create_mode, bool ignore_if_exists) { auto request = std::make_shared(); request->path = path; request->data = data; request->is_ephemeral = create_mode == CreateMode::Ephemeral || create_mode == CreateMode::EphemeralSequential; request->is_sequential = create_mode == CreateMode::PersistentSequential || create_mode == CreateMode::EphemeralSequential; + request->not_exists = ignore_if_exists; return request; } diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.cpp b/src/Common/ZooKeeper/ZooKeeperCommon.cpp index 61b9d6e5172..5a34f7cc8d5 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.cpp +++ b/src/Common/ZooKeeper/ZooKeeperCommon.cpp @@ -660,7 +660,6 @@ void ZooKeeperMultiResponse::writeImpl(WriteBuffer & out) const ZooKeeperResponsePtr ZooKeeperHeartbeatRequest::makeResponse() const { return setTime(std::make_shared()); } ZooKeeperResponsePtr ZooKeeperSyncRequest::makeResponse() const { return setTime(std::make_shared()); } ZooKeeperResponsePtr ZooKeeperAuthRequest::makeResponse() const { return setTime(std::make_shared()); } -ZooKeeperResponsePtr ZooKeeperCreateRequest::makeResponse() const { return setTime(std::make_shared()); } ZooKeeperResponsePtr ZooKeeperRemoveRequest::makeResponse() const { return setTime(std::make_shared()); } ZooKeeperResponsePtr ZooKeeperExistsRequest::makeResponse() const { return setTime(std::make_shared()); } ZooKeeperResponsePtr ZooKeeperGetRequest::makeResponse() const { return setTime(std::make_shared()); } @@ -668,6 +667,13 @@ ZooKeeperResponsePtr ZooKeeperSetRequest::makeResponse() const { return setTime( ZooKeeperResponsePtr ZooKeeperListRequest::makeResponse() const { return setTime(std::make_shared()); } ZooKeeperResponsePtr ZooKeeperSimpleListRequest::makeResponse() const { return setTime(std::make_shared()); } +ZooKeeperResponsePtr ZooKeeperCreateRequest::makeResponse() const +{ + if (not_exists) + return setTime(std::make_shared()); + return setTime(std::make_shared()); +} + ZooKeeperResponsePtr ZooKeeperCheckRequest::makeResponse() const { if (not_exists) diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.h b/src/Common/ZooKeeper/ZooKeeperCommon.h index 264b2bb9606..b79cbc204a0 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.h +++ b/src/Common/ZooKeeper/ZooKeeperCommon.h @@ -199,9 +199,6 @@ struct ZooKeeperCreateRequest final : public CreateRequest, ZooKeeperRequest /// used only during restore from zookeeper log int32_t parent_cversion = -1; - /// should it succeed if node already exists - bool not_exists = false; - ZooKeeperCreateRequest() = default; explicit ZooKeeperCreateRequest(const CreateRequest & base) : CreateRequest(base) {} @@ -218,7 +215,7 @@ struct ZooKeeperCreateRequest final : public CreateRequest, ZooKeeperRequest void createLogElements(LogElements & elems) const override; }; -struct ZooKeeperCreateResponse final : CreateResponse, ZooKeeperResponse +struct ZooKeeperCreateResponse : CreateResponse, ZooKeeperResponse { void readImpl(ReadBuffer & in) override; @@ -231,6 +228,12 @@ struct ZooKeeperCreateResponse final : CreateResponse, ZooKeeperResponse void fillLogElements(LogElements & elems, size_t idx) const override; }; +struct ZooKeeperCreateIfNotExistsResponse : ZooKeeperCreateResponse +{ + OpNum getOpNum() const override { return OpNum::CreateIfNotExists; } + using ZooKeeperCreateResponse::ZooKeeperCreateResponse; +}; + struct ZooKeeperRemoveRequest final : RemoveRequest, ZooKeeperRequest { ZooKeeperRemoveRequest() = default; From 6e8ccafbb22813f7c736c3fd2f5363124985da3e Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 5 Jun 2023 09:40:29 +0200 Subject: [PATCH 012/101] Fix test --- .../0_stateless/02735_system_zookeeper_connection.reference | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02735_system_zookeeper_connection.reference b/tests/queries/0_stateless/02735_system_zookeeper_connection.reference index 1deabd88b88..2176580086b 100644 --- a/tests/queries/0_stateless/02735_system_zookeeper_connection.reference +++ b/tests/queries/0_stateless/02735_system_zookeeper_connection.reference @@ -1,2 +1,2 @@ -default ::1 9181 0 0 3 +default ::1 9181 0 0 4 zookeeper2 ::1 9181 0 0 0 From eb0e14b870108acf131456abee6acfab389bfa42 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Tue, 6 Jun 2023 00:44:20 +0000 Subject: [PATCH 013/101] allow to replace long file names to hashes --- src/Common/SipHash.h | 11 ++++ src/DataTypes/Serializations/ISerialization.h | 1 + .../MergeTree/MergeTreeDataPartChecksum.cpp | 13 ++++ .../MergeTree/MergeTreeDataPartChecksum.h | 2 + .../MergeTree/MergeTreeDataPartWriterWide.cpp | 40 +++++++----- .../MergeTree/MergeTreeDataPartWriterWide.h | 4 ++ .../MergeTree/MergeTreeReaderWide.cpp | 61 +++++++++++-------- src/Storages/MergeTree/MergeTreeSettings.h | 2 + 8 files changed, 93 insertions(+), 41 deletions(-) diff --git a/src/Common/SipHash.h b/src/Common/SipHash.h index 9e6479d81c1..e1cd5cc0aa3 100644 --- a/src/Common/SipHash.h +++ b/src/Common/SipHash.h @@ -20,6 +20,7 @@ #include #include #include +#include #include @@ -284,6 +285,16 @@ inline UInt128 sipHash128(const char * data, const size_t size) return sipHash128Keyed(0, 0, data, size); } +inline String sipHash128String(const char * data, const size_t size) +{ + return getHexUIntLowercase(sipHash128(data, size)); +} + +inline String sipHash128String(const String & str) +{ + return sipHash128String(str.data(), str.size()); +} + inline UInt128 sipHash128ReferenceKeyed(UInt64 key0, UInt64 key1, const char * data, const size_t size) { SipHash hash(key0, key1, true); diff --git a/src/DataTypes/Serializations/ISerialization.h b/src/DataTypes/Serializations/ISerialization.h index 17e6dfb85bc..ed090cefa38 100644 --- a/src/DataTypes/Serializations/ISerialization.h +++ b/src/DataTypes/Serializations/ISerialization.h @@ -368,6 +368,7 @@ public: static String getFileNameForStream(const NameAndTypePair & column, const SubstreamPath & path); static String getFileNameForStream(const String & name_in_storage, const SubstreamPath & path); + static String getSubcolumnNameForStream(const SubstreamPath & path); static String getSubcolumnNameForStream(const SubstreamPath & path, size_t prefix_len); diff --git a/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp b/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp index 78f68ea72fe..2f97edd1a9c 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp @@ -8,6 +8,7 @@ #include #include #include +#include namespace DB @@ -340,6 +341,18 @@ MergeTreeDataPartChecksums::Checksum::uint128 MergeTreeDataPartChecksums::getTot return ret; } +std::optional MergeTreeDataPartChecksums::getFileNameOrHash(const String & name) const +{ + if (files.contains(name + ".bin")) + return name; + + auto hash = sipHash128String(name); + if (files.contains(hash + ".bin")) + return hash; + + return std::nullopt; +} + void MinimalisticDataPartChecksums::serialize(WriteBuffer & to) const { writeString("checksums format version: 5\n", to); diff --git a/src/Storages/MergeTree/MergeTreeDataPartChecksum.h b/src/Storages/MergeTree/MergeTreeDataPartChecksum.h index db110043b74..626b0a90839 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartChecksum.h +++ b/src/Storages/MergeTree/MergeTreeDataPartChecksum.h @@ -88,6 +88,8 @@ struct MergeTreeDataPartChecksums static MergeTreeDataPartChecksums deserializeFrom(const String & s); UInt64 getTotalSizeOnDisk() const; + + std::optional getFileNameOrHash(const String & name) const; }; diff --git a/src/Storages/MergeTree/MergeTreeDataPartWriterWide.cpp b/src/Storages/MergeTree/MergeTreeDataPartWriterWide.cpp index f9fe6f2c8ab..60bb1119770 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWriterWide.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWriterWide.cpp @@ -97,7 +97,15 @@ void MergeTreeDataPartWriterWide::addStreams( ISerialization::StreamCallback callback = [&](const auto & substream_path) { assert(!substream_path.empty()); - String stream_name = ISerialization::getFileNameForStream(column, substream_path); + + auto storage_settings = storage.getSettings(); + auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); + + String stream_name; + if (storage_settings->replace_long_file_name_to_hash && full_stream_name.size() > storage_settings->max_file_name_length) + stream_name = sipHash128String(full_stream_name); + else + stream_name = full_stream_name; /// Shared offsets for Nested type. if (column_streams.contains(stream_name)) @@ -126,12 +134,21 @@ void MergeTreeDataPartWriterWide::addStreams( marks_compression_codec, settings.marks_compress_block_size, settings.query_write_settings); + + full_name_to_stream_name.emplace(full_stream_name, stream_name); }; ISerialization::SubstreamPath path; data_part->getSerialization(column.name)->enumerateStreams(callback, column.type); } +const String & MergeTreeDataPartWriterWide::getStreamName( + const NameAndTypePair & column, + const ISerialization::SubstreamPath & substream_path) const +{ + auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); + return full_name_to_stream_name.at(full_stream_name); +} ISerialization::OutputStreamGetter MergeTreeDataPartWriterWide::createStreamGetter( const NameAndTypePair & column, WrittenOffsetColumns & offset_columns) const @@ -139,8 +156,7 @@ ISerialization::OutputStreamGetter MergeTreeDataPartWriterWide::createStreamGett return [&, this] (const ISerialization::SubstreamPath & substream_path) -> WriteBuffer * { bool is_offsets = !substream_path.empty() && substream_path.back().type == ISerialization::Substream::ArraySizes; - - String stream_name = ISerialization::getFileNameForStream(column, substream_path); + auto stream_name = getStreamName(column, substream_path); /// Don't write offsets more than one time for Nested type. if (is_offsets && offset_columns.contains(stream_name)) @@ -289,8 +305,7 @@ StreamsWithMarks MergeTreeDataPartWriterWide::getCurrentMarksForColumn( data_part->getSerialization(column.name)->enumerateStreams([&] (const ISerialization::SubstreamPath & substream_path) { bool is_offsets = !substream_path.empty() && substream_path.back().type == ISerialization::Substream::ArraySizes; - - String stream_name = ISerialization::getFileNameForStream(column, substream_path); + auto stream_name = getStreamName(column, substream_path); /// Don't write offsets more than one time for Nested type. if (is_offsets && offset_columns.contains(stream_name)) @@ -328,14 +343,13 @@ void MergeTreeDataPartWriterWide::writeSingleGranule( serialization->enumerateStreams([&] (const ISerialization::SubstreamPath & substream_path) { bool is_offsets = !substream_path.empty() && substream_path.back().type == ISerialization::Substream::ArraySizes; - - String stream_name = ISerialization::getFileNameForStream(name_and_type, substream_path); + auto stream_name = getStreamName(name_and_type, substream_path); /// Don't write offsets more than one time for Nested type. if (is_offsets && offset_columns.contains(stream_name)) return; - column_streams[stream_name]->compressed_hashing.nextIfAtEnd(); + column_streams.at(stream_name)->compressed_hashing.nextIfAtEnd(); }); } @@ -406,10 +420,7 @@ void MergeTreeDataPartWriterWide::writeColumn( { bool is_offsets = !substream_path.empty() && substream_path.back().type == ISerialization::Substream::ArraySizes; if (is_offsets) - { - String stream_name = ISerialization::getFileNameForStream(name_and_type, substream_path); - offset_columns.insert(stream_name); - } + offset_columns.insert(getStreamName(name_and_type, substream_path)); }); } @@ -656,10 +667,7 @@ void MergeTreeDataPartWriterWide::writeFinalMark( { bool is_offsets = !substream_path.empty() && substream_path.back().type == ISerialization::Substream::ArraySizes; if (is_offsets) - { - String stream_name = ISerialization::getFileNameForStream(column, substream_path); - offset_columns.insert(stream_name); - } + offset_columns.insert(getStreamName(column, substream_path)); }); } diff --git a/src/Storages/MergeTree/MergeTreeDataPartWriterWide.h b/src/Storages/MergeTree/MergeTreeDataPartWriterWide.h index 633b5119474..de7419fedb2 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWriterWide.h +++ b/src/Storages/MergeTree/MergeTreeDataPartWriterWide.h @@ -101,6 +101,7 @@ private: void adjustLastMarkIfNeedAndFlushToDisk(size_t new_rows_in_last_mark); ISerialization::OutputStreamGetter createStreamGetter(const NameAndTypePair & column, WrittenOffsetColumns & offset_columns) const; + const String & getStreamName(const NameAndTypePair & column, const ISerialization::SubstreamPath & substream_path) const; using SerializationState = ISerialization::SerializeBinaryBulkStatePtr; using SerializationStates = std::unordered_map; @@ -110,6 +111,9 @@ private: using ColumnStreams = std::map; ColumnStreams column_streams; + /// TODO: + std::unordered_map full_name_to_stream_name; + /// Non written marks to disk (for each column). Waiting until all rows for /// this marks will be written to disk. using MarksForColumns = std::unordered_map; diff --git a/src/Storages/MergeTree/MergeTreeReaderWide.cpp b/src/Storages/MergeTree/MergeTreeReaderWide.cpp index baacfa55c94..cd641a5cd2a 100644 --- a/src/Storages/MergeTree/MergeTreeReaderWide.cpp +++ b/src/Storages/MergeTree/MergeTreeReaderWide.cpp @@ -198,13 +198,21 @@ size_t MergeTreeReaderWide::readRows( catch (...) { data_part_info_for_read->reportBroken(); - throw; } return read_rows; } +std::optional getStreamName( + const NameAndTypePair & column, + const ISerialization::SubstreamPath & substream_path, + const MergeTreeDataPartChecksums & checksums) +{ + auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); + return checksums.getFileNameOrHash(full_stream_name); +} + void MergeTreeReaderWide::addStreams( const NameAndTypePair & name_and_type, const SerializationPtr & serialization, @@ -216,35 +224,33 @@ void MergeTreeReaderWide::addStreams( ISerialization::StreamCallback callback = [&] (const ISerialization::SubstreamPath & substream_path) { - String stream_name = ISerialization::getFileNameForStream(name_and_type, substream_path); - - if (streams.contains(stream_name)) - { - has_any_stream = true; - return; - } - - bool data_file_exists = data_part_info_for_read->getChecksums().files.contains(stream_name + DATA_FILE_EXTENSION); + auto stream_name = getStreamName(name_and_type, substream_path, data_part_info_for_read->getChecksums()); /** If data file is missing then we will not try to open it. * It is necessary since it allows to add new column to structure of the table without creating new files for old parts. */ - if (!data_file_exists) + if (!stream_name) { has_all_streams = false; return; } + if (streams.contains(*stream_name)) + { + has_any_stream = true; + return; + } + has_any_stream = true; bool is_lc_dict = substream_path.size() > 1 && substream_path[substream_path.size() - 2].type == ISerialization::Substream::Type::DictionaryKeys; auto context = data_part_info_for_read->getContext(); auto * load_marks_threadpool = settings.read_settings.load_marks_asynchronously ? &context->getLoadMarksThreadpool() : nullptr; - streams.emplace(stream_name, std::make_unique( - data_part_info_for_read, stream_name, DATA_FILE_EXTENSION, + streams.emplace(*stream_name, std::make_unique( + data_part_info_for_read, *stream_name, DATA_FILE_EXTENSION, data_part_info_for_read->getMarksCount(), all_mark_ranges, settings, mark_cache, - uncompressed_cache, data_part_info_for_read->getFileSizeOrZero(stream_name + DATA_FILE_EXTENSION), + uncompressed_cache, data_part_info_for_read->getFileSizeOrZero(*stream_name + DATA_FILE_EXTENSION), &data_part_info_for_read->getIndexGranularityInfo(), profile_callback, clock_type, is_lc_dict, load_marks_threadpool)); }; @@ -255,13 +261,14 @@ void MergeTreeReaderWide::addStreams( partially_read_columns.insert(name_and_type.name); } - static ReadBuffer * getStream( bool seek_to_start, const ISerialization::SubstreamPath & substream_path, + const MergeTreeDataPartChecksums & checksums, MergeTreeReaderWide::FileStreams & streams, const NameAndTypePair & name_and_type, - size_t from_mark, bool seek_to_mark, + size_t from_mark, + bool seek_to_mark, size_t current_task_last_mark, ISerialization::SubstreamsCache & cache) { @@ -269,9 +276,12 @@ static ReadBuffer * getStream( if (cache.contains(ISerialization::getSubcolumnNameForStream(substream_path))) return nullptr; - String stream_name = ISerialization::getFileNameForStream(name_and_type, substream_path); + auto stream_name = getStreamName(name_and_type, substream_path, checksums); - auto it = streams.find(stream_name); + if (!stream_name) + return nullptr; + + auto it = streams.find(*stream_name); if (it == streams.end()) return nullptr; @@ -298,7 +308,7 @@ void MergeTreeReaderWide::deserializePrefix( ISerialization::DeserializeBinaryBulkSettings deserialize_settings; deserialize_settings.getter = [&](const ISerialization::SubstreamPath & substream_path) { - return getStream(/* seek_to_start = */true, substream_path, streams, name_and_type, 0, /* seek_to_mark = */false, current_task_last_mark, cache); + return getStream(/* seek_to_start = */true, substream_path, data_part_info_for_read->getChecksums(), streams, name_and_type, 0, /* seek_to_mark = */false, current_task_last_mark, cache); }; serialization->deserializeBinaryBulkStatePrefix(deserialize_settings, deserialize_binary_bulk_state_map[name]); } @@ -317,15 +327,15 @@ void MergeTreeReaderWide::prefetchForColumn( serialization->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { - String stream_name = ISerialization::getFileNameForStream(name_and_type, substream_path); + auto stream_name = getStreamName(name_and_type, substream_path, data_part_info_for_read->getChecksums()); - if (!prefetched_streams.contains(stream_name)) + if (stream_name && !prefetched_streams.contains(*stream_name)) { bool seek_to_mark = !continue_reading; - if (ReadBuffer * buf = getStream(false, substream_path, streams, name_and_type, from_mark, seek_to_mark, current_task_last_mark, cache)) + if (ReadBuffer * buf = getStream(false, substream_path, data_part_info_for_read->getChecksums(), streams, name_and_type, from_mark, seek_to_mark, current_task_last_mark, cache)) { buf->prefetch(priority); - prefetched_streams.insert(stream_name); + prefetched_streams.insert(*stream_name); } } }); @@ -348,8 +358,9 @@ void MergeTreeReaderWide::readData( bool seek_to_mark = !was_prefetched && !continue_reading; return getStream( - /* seek_to_start = */false, substream_path, streams, name_and_type, from_mark, - seek_to_mark, current_task_last_mark, cache); + /* seek_to_start = */false, substream_path, + data_part_info_for_read->getChecksums(), streams, + name_and_type, from_mark, seek_to_mark, current_task_last_mark, cache); }; deserialize_settings.continuous_reading = continue_reading; diff --git a/src/Storages/MergeTree/MergeTreeSettings.h b/src/Storages/MergeTree/MergeTreeSettings.h index 5ea99009756..ae4d585e5fe 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.h +++ b/src/Storages/MergeTree/MergeTreeSettings.h @@ -34,6 +34,8 @@ struct Settings; M(UInt64, min_bytes_for_wide_part, 10485760, "Minimal uncompressed size in bytes to create part in wide format instead of compact", 0) \ M(UInt64, min_rows_for_wide_part, 0, "Minimal number of rows to create part in wide format instead of compact", 0) \ M(Float, ratio_of_defaults_for_sparse_serialization, 1.0, "Minimal ratio of number of default values to number of all values in column to store it in sparse serializations. If >= 1, columns will be always written in full serialization.", 0) \ + M(Bool, replace_long_file_name_to_hash, false, "", 0) \ + M(UInt64, max_file_name_length, 128, "", 0) \ \ /** Merge settings. */ \ M(UInt64, merge_max_block_size, 8192, "How many rows in blocks should be formed for merge operations. By default has the same value as `index_granularity`.", 0) \ From b30544a6ab6b773ed3dc7bd6c3cffcebbb6ae1b8 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Tue, 6 Jun 2023 01:39:45 +0000 Subject: [PATCH 014/101] allow to replace long file names to hashes --- src/Storages/MergeTree/IMergeTreeDataPart.cpp | 6 +++- .../MergeTree/IMergedBlockOutputStream.cpp | 8 +++-- .../MergeTree/MergeTreeDataPartChecksum.cpp | 4 +-- .../MergeTree/MergeTreeDataPartChecksum.h | 2 +- .../MergeTree/MergeTreeDataPartWide.cpp | 24 +++++++++----- .../MergeTree/MergeTreeReaderWide.cpp | 33 +++++++++---------- src/Storages/MergeTree/MergeTreeSettings.h | 4 +-- src/Storages/MergeTree/MutateTask.cpp | 15 ++++++--- src/Storages/MergeTree/checkDataPart.cpp | 13 +++++++- .../System/StorageSystemPartsColumns.cpp | 7 ++-- 10 files changed, 75 insertions(+), 41 deletions(-) diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.cpp b/src/Storages/MergeTree/IMergeTreeDataPart.cpp index d27b03fff44..dfc1fe0c262 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.cpp +++ b/src/Storages/MergeTree/IMergeTreeDataPart.cpp @@ -1,4 +1,5 @@ #include "IMergeTreeDataPart.h" +#include "Common/SipHash.h" #include "Storages/MergeTree/IDataPartStorage.h" #include @@ -1015,7 +1016,10 @@ CompressionCodecPtr IMergeTreeDataPart::detectDefaultCompressionCodec() const { if (path_to_data_file.empty()) { - String candidate_path = /*fs::path(getRelativePath()) */ (ISerialization::getFileNameForStream(part_column, substream_path) + ".bin"); + auto candidate_path = ISerialization::getFileNameForStream(part_column, substream_path) + ".bin"; + + if (!getDataPartStorage().exists(candidate_path)) + candidate_path = sipHash128String(candidate_path) + ".bin"; /// We can have existing, but empty .bin files. Example: LowCardinality(Nullable(...)) columns and column_name.dict.null.bin file. if (getDataPartStorage().exists(candidate_path) && getDataPartStorage().getFileSize(candidate_path) != 0) diff --git a/src/Storages/MergeTree/IMergedBlockOutputStream.cpp b/src/Storages/MergeTree/IMergedBlockOutputStream.cpp index 21bead2864a..2df3b6d15a6 100644 --- a/src/Storages/MergeTree/IMergedBlockOutputStream.cpp +++ b/src/Storages/MergeTree/IMergedBlockOutputStream.cpp @@ -51,7 +51,9 @@ NameSet IMergedBlockOutputStream::removeEmptyColumnsFromPart( data_part->getSerialization(column.name)->enumerateStreams( [&](const ISerialization::SubstreamPath & substream_path) { - ++stream_counts[ISerialization::getFileNameForStream(column.name, substream_path)]; + auto full_stream_name = ISerialization::getFileNameForStream(column.name, substream_path); + auto stream_name = checksums.getFileNameOrHash(full_stream_name); + ++stream_counts[stream_name]; }); } @@ -65,7 +67,9 @@ NameSet IMergedBlockOutputStream::removeEmptyColumnsFromPart( ISerialization::StreamCallback callback = [&](const ISerialization::SubstreamPath & substream_path) { - String stream_name = ISerialization::getFileNameForStream(column_name, substream_path); + auto full_stream_name = ISerialization::getFileNameForStream(column_name, substream_path); + auto stream_name = checksums.getFileNameOrHash(full_stream_name); + /// Delete files if they are no longer shared with another column. if (--stream_counts[stream_name] == 0) { diff --git a/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp b/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp index 2f97edd1a9c..7d39ea0707f 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp @@ -341,7 +341,7 @@ MergeTreeDataPartChecksums::Checksum::uint128 MergeTreeDataPartChecksums::getTot return ret; } -std::optional MergeTreeDataPartChecksums::getFileNameOrHash(const String & name) const +String MergeTreeDataPartChecksums::getFileNameOrHash(const String & name) const { if (files.contains(name + ".bin")) return name; @@ -350,7 +350,7 @@ std::optional MergeTreeDataPartChecksums::getFileNameOrHash(const String if (files.contains(hash + ".bin")) return hash; - return std::nullopt; + return name; } void MinimalisticDataPartChecksums::serialize(WriteBuffer & to) const diff --git a/src/Storages/MergeTree/MergeTreeDataPartChecksum.h b/src/Storages/MergeTree/MergeTreeDataPartChecksum.h index 626b0a90839..2a38b52c72a 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartChecksum.h +++ b/src/Storages/MergeTree/MergeTreeDataPartChecksum.h @@ -89,7 +89,7 @@ struct MergeTreeDataPartChecksums UInt64 getTotalSizeOnDisk() const; - std::optional getFileNameOrHash(const String & name) const; + String getFileNameOrHash(const String & name) const; }; diff --git a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp index f44cbdd8628..645e16eed38 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp @@ -73,19 +73,20 @@ ColumnSize MergeTreeDataPartWide::getColumnSizeImpl( getSerialization(column.name)->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { - String file_name = ISerialization::getFileNameForStream(column, substream_path); + auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); + auto stream_name = checksums.getFileNameOrHash(full_stream_name); - if (processed_substreams && !processed_substreams->insert(file_name).second) + if (processed_substreams && !processed_substreams->insert(stream_name).second) return; - auto bin_checksum = checksums.files.find(file_name + ".bin"); + auto bin_checksum = checksums.files.find(stream_name + ".bin"); if (bin_checksum != checksums.files.end()) { size.data_compressed += bin_checksum->second.file_size; size.data_uncompressed += bin_checksum->second.uncompressed_size; } - auto mrk_checksum = checksums.files.find(file_name + getMarksFileExtension()); + auto mrk_checksum = checksums.files.find(stream_name + getMarksFileExtension()); if (mrk_checksum != checksums.files.end()) size.marks += mrk_checksum->second.file_size; }); @@ -185,9 +186,11 @@ void MergeTreeDataPartWide::checkConsistency(bool require_part_metadata) const { getSerialization(name_type.name)->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { - String file_name = ISerialization::getFileNameForStream(name_type, substream_path); - String mrk_file_name = file_name + marks_file_extension; - String bin_file_name = file_name + DATA_FILE_EXTENSION; + String full_stream_name = ISerialization::getFileNameForStream(name_type, substream_path); + String stream_name = checksums.getFileNameOrHash(full_stream_name); + + String mrk_file_name = stream_name + marks_file_extension; + String bin_file_name = stream_name + DATA_FILE_EXTENSION; if (!checksums.files.contains(mrk_file_name)) throw Exception( @@ -213,6 +216,8 @@ void MergeTreeDataPartWide::checkConsistency(bool require_part_metadata) const getSerialization(name_type.name)->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { auto file_path = ISerialization::getFileNameForStream(name_type, substream_path) + marks_file_extension; + if (!getDataPartStorage().exists(file_path)) + file_path = sipHash128String(file_path) + marks_file_extension; /// Missing file is Ok for case when new column was added. if (getDataPartStorage().exists(file_path)) @@ -266,7 +271,10 @@ String MergeTreeDataPartWide::getFileNameForColumn(const NameAndTypePair & colum getSerialization(column.name)->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { if (filename.empty()) - filename = ISerialization::getFileNameForStream(column, substream_path); + { + auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); + auto filname = checksums.getFileNameOrHash(full_stream_name); + } }); return filename; } diff --git a/src/Storages/MergeTree/MergeTreeReaderWide.cpp b/src/Storages/MergeTree/MergeTreeReaderWide.cpp index cd641a5cd2a..0ce20dc02f0 100644 --- a/src/Storages/MergeTree/MergeTreeReaderWide.cpp +++ b/src/Storages/MergeTree/MergeTreeReaderWide.cpp @@ -204,7 +204,7 @@ size_t MergeTreeReaderWide::readRows( return read_rows; } -std::optional getStreamName( +String getStreamName( const NameAndTypePair & column, const ISerialization::SubstreamPath & substream_path, const MergeTreeDataPartChecksums & checksums) @@ -226,18 +226,20 @@ void MergeTreeReaderWide::addStreams( { auto stream_name = getStreamName(name_and_type, substream_path, data_part_info_for_read->getChecksums()); - /** If data file is missing then we will not try to open it. - * It is necessary since it allows to add new column to structure of the table without creating new files for old parts. - */ - if (!stream_name) + if (streams.contains(stream_name)) { - has_all_streams = false; + has_any_stream = true; return; } - if (streams.contains(*stream_name)) + bool data_file_exists = data_part_info_for_read->getChecksums().files.contains(stream_name + DATA_FILE_EXTENSION); + + /** If data file is missing then we will not try to open it. + * It is necessary since it allows to add new column to structure of the table without creating new files for old parts. + */ + if (!data_file_exists) { - has_any_stream = true; + has_all_streams = false; return; } @@ -247,10 +249,10 @@ void MergeTreeReaderWide::addStreams( auto context = data_part_info_for_read->getContext(); auto * load_marks_threadpool = settings.read_settings.load_marks_asynchronously ? &context->getLoadMarksThreadpool() : nullptr; - streams.emplace(*stream_name, std::make_unique( - data_part_info_for_read, *stream_name, DATA_FILE_EXTENSION, + streams.emplace(stream_name, std::make_unique( + data_part_info_for_read, stream_name, DATA_FILE_EXTENSION, data_part_info_for_read->getMarksCount(), all_mark_ranges, settings, mark_cache, - uncompressed_cache, data_part_info_for_read->getFileSizeOrZero(*stream_name + DATA_FILE_EXTENSION), + uncompressed_cache, data_part_info_for_read->getFileSizeOrZero(stream_name + DATA_FILE_EXTENSION), &data_part_info_for_read->getIndexGranularityInfo(), profile_callback, clock_type, is_lc_dict, load_marks_threadpool)); }; @@ -278,10 +280,7 @@ static ReadBuffer * getStream( auto stream_name = getStreamName(name_and_type, substream_path, checksums); - if (!stream_name) - return nullptr; - - auto it = streams.find(*stream_name); + auto it = streams.find(stream_name); if (it == streams.end()) return nullptr; @@ -329,13 +328,13 @@ void MergeTreeReaderWide::prefetchForColumn( { auto stream_name = getStreamName(name_and_type, substream_path, data_part_info_for_read->getChecksums()); - if (stream_name && !prefetched_streams.contains(*stream_name)) + if (!prefetched_streams.contains(stream_name)) { bool seek_to_mark = !continue_reading; if (ReadBuffer * buf = getStream(false, substream_path, data_part_info_for_read->getChecksums(), streams, name_and_type, from_mark, seek_to_mark, current_task_last_mark, cache)) { buf->prefetch(priority); - prefetched_streams.insert(*stream_name); + prefetched_streams.insert(stream_name); } } }); diff --git a/src/Storages/MergeTree/MergeTreeSettings.h b/src/Storages/MergeTree/MergeTreeSettings.h index ae4d585e5fe..0d32567d2fa 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.h +++ b/src/Storages/MergeTree/MergeTreeSettings.h @@ -34,8 +34,8 @@ struct Settings; M(UInt64, min_bytes_for_wide_part, 10485760, "Minimal uncompressed size in bytes to create part in wide format instead of compact", 0) \ M(UInt64, min_rows_for_wide_part, 0, "Minimal number of rows to create part in wide format instead of compact", 0) \ M(Float, ratio_of_defaults_for_sparse_serialization, 1.0, "Minimal ratio of number of default values to number of all values in column to store it in sparse serializations. If >= 1, columns will be always written in full serialization.", 0) \ - M(Bool, replace_long_file_name_to_hash, false, "", 0) \ - M(UInt64, max_file_name_length, 128, "", 0) \ + M(Bool, replace_long_file_name_to_hash, false, "If the file name for column is too long (more than 'max_file_name_length' bytes) replace it to SipHash128", 0) \ + M(UInt64, max_file_name_length, 128, "The maximal length of the file name to keep it as is without hashing", 0) \ \ /** Merge settings. */ \ M(UInt64, merge_max_block_size, 8192, "How many rows in blocks should be formed for merge operations. By default has the same value as `index_granularity`.", 0) \ diff --git a/src/Storages/MergeTree/MutateTask.cpp b/src/Storages/MergeTree/MutateTask.cpp index 76096d00641..4bcaea53337 100644 --- a/src/Storages/MergeTree/MutateTask.cpp +++ b/src/Storages/MergeTree/MutateTask.cpp @@ -1,3 +1,4 @@ +#include "Common/SipHash.h" #include #include @@ -591,7 +592,8 @@ static std::unordered_map getStreamCounts( { auto callback = [&](const ISerialization::SubstreamPath & substream_path) { - auto stream_name = ISerialization::getFileNameForStream(column_name, substream_path); + auto full_stream_name = ISerialization::getFileNameForStream(column_name, substream_path); + auto stream_name = data_part->checksums.getFileNameOrHash(full_stream_name); ++stream_counts[stream_name]; }; @@ -705,7 +707,9 @@ static NameToNameVector collectFilesForRenames( { ISerialization::StreamCallback callback = [&](const ISerialization::SubstreamPath & substream_path) { - String stream_name = ISerialization::getFileNameForStream({command.column_name, command.data_type}, substream_path); + auto full_stream_name = ISerialization::getFileNameForStream({command.column_name, command.data_type}, substream_path); + auto stream_name = source_part->checksums.getFileNameOrHash(full_stream_name); + /// Delete files if they are no longer shared with another column. if (--stream_counts[stream_name] == 0) { @@ -724,8 +728,11 @@ static NameToNameVector collectFilesForRenames( ISerialization::StreamCallback callback = [&](const ISerialization::SubstreamPath & substream_path) { - String stream_from = ISerialization::getFileNameForStream(command.column_name, substream_path); - String stream_to = boost::replace_first_copy(stream_from, escaped_name_from, escaped_name_to); + String full_stream_from = ISerialization::getFileNameForStream(command.column_name, substream_path); + String full_stream_to = boost::replace_first_copy(full_stream_from, escaped_name_from, escaped_name_to); + + String stream_from = source_part->checksums.getFileNameOrHash(full_stream_from); + String stream_to = stream_from == full_stream_from ? full_stream_to : sipHash128String(full_stream_to); if (stream_from != stream_to) { diff --git a/src/Storages/MergeTree/checkDataPart.cpp b/src/Storages/MergeTree/checkDataPart.cpp index 00710ed3ed6..561f76d8b5f 100644 --- a/src/Storages/MergeTree/checkDataPart.cpp +++ b/src/Storages/MergeTree/checkDataPart.cpp @@ -11,6 +11,7 @@ #include #include #include +#include namespace CurrentMetrics @@ -30,6 +31,7 @@ namespace ErrorCodes extern const int CANNOT_MUNMAP; extern const int CANNOT_MREMAP; extern const int UNEXPECTED_FILE_IN_DATA_PART; + extern const int NO_FILE_IN_DATA_PART; } @@ -137,7 +139,16 @@ IMergeTreeDataPart::Checksums checkDataPart( { get_serialization(column)->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { - String file_name = ISerialization::getFileNameForStream(column, substream_path) + ".bin"; + auto file_name = ISerialization::getFileNameForStream(column, substream_path) + ".bin"; + + if (!data_part_storage.exists(file_name)) + file_name = sipHash128String(file_name); + + if (!data_part_storage.exists(file_name)) + throw Exception(ErrorCodes::NO_FILE_IN_DATA_PART, + "There is no file for column '{}' in data part '{}'", + column.name, data_part->name); + checksums_data.files[file_name] = checksum_compressed_file(data_part_storage, file_name); }); } diff --git a/src/Storages/System/StorageSystemPartsColumns.cpp b/src/Storages/System/StorageSystemPartsColumns.cpp index 00b958b015f..de874b22e7e 100644 --- a/src/Storages/System/StorageSystemPartsColumns.cpp +++ b/src/Storages/System/StorageSystemPartsColumns.cpp @@ -261,16 +261,17 @@ void StorageSystemPartsColumns::processNextStorage( ColumnSize size; NameAndTypePair subcolumn(column.name, name, column.type, data.type); - String file_name = ISerialization::getFileNameForStream(subcolumn, subpath); + String full_stream_name = ISerialization::getFileNameForStream(subcolumn, subpath); + String stream_name = part->checksums.getFileNameOrHash(full_stream_name); - auto bin_checksum = part->checksums.files.find(file_name + ".bin"); + auto bin_checksum = part->checksums.files.find(stream_name + ".bin"); if (bin_checksum != part->checksums.files.end()) { size.data_compressed += bin_checksum->second.file_size; size.data_uncompressed += bin_checksum->second.uncompressed_size; } - auto mrk_checksum = part->checksums.files.find(file_name + part->index_granularity_info.mark_type.getFileExtension()); + auto mrk_checksum = part->checksums.files.find(stream_name + part->index_granularity_info.mark_type.getFileExtension()); if (mrk_checksum != part->checksums.files.end()) size.marks += mrk_checksum->second.file_size; From 0b06f247829d15fcf493d3d1804592ef0b9bd9c2 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Tue, 6 Jun 2023 01:40:34 +0000 Subject: [PATCH 015/101] temporarly enable hashing of names --- src/Storages/MergeTree/MergeTreeSettings.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Storages/MergeTree/MergeTreeSettings.h b/src/Storages/MergeTree/MergeTreeSettings.h index 0d32567d2fa..d63e33e2477 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.h +++ b/src/Storages/MergeTree/MergeTreeSettings.h @@ -34,8 +34,8 @@ struct Settings; M(UInt64, min_bytes_for_wide_part, 10485760, "Minimal uncompressed size in bytes to create part in wide format instead of compact", 0) \ M(UInt64, min_rows_for_wide_part, 0, "Minimal number of rows to create part in wide format instead of compact", 0) \ M(Float, ratio_of_defaults_for_sparse_serialization, 1.0, "Minimal ratio of number of default values to number of all values in column to store it in sparse serializations. If >= 1, columns will be always written in full serialization.", 0) \ - M(Bool, replace_long_file_name_to_hash, false, "If the file name for column is too long (more than 'max_file_name_length' bytes) replace it to SipHash128", 0) \ - M(UInt64, max_file_name_length, 128, "The maximal length of the file name to keep it as is without hashing", 0) \ + M(Bool, replace_long_file_name_to_hash, true, "If the file name for column is too long (more than 'max_file_name_length' bytes) replace it to SipHash128", 0) \ + M(UInt64, max_file_name_length, 0, "The maximal length of the file name to keep it as is without hashing", 0) \ \ /** Merge settings. */ \ M(UInt64, merge_max_block_size, 8192, "How many rows in blocks should be formed for merge operations. By default has the same value as `index_granularity`.", 0) \ From a8a561b28cd8f1f5835c0bce288755fbe0819928 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Tue, 6 Jun 2023 01:54:01 +0000 Subject: [PATCH 016/101] fix typo --- src/Storages/MergeTree/MergeTreeDataPartWide.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp index 645e16eed38..04e672933a5 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp @@ -273,7 +273,7 @@ String MergeTreeDataPartWide::getFileNameForColumn(const NameAndTypePair & colum if (filename.empty()) { auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); - auto filname = checksums.getFileNameOrHash(full_stream_name); + filename = checksums.getFileNameOrHash(full_stream_name); } }); return filename; From 562ad9536669b9932cc196852354bfdb8f484402 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Tue, 6 Jun 2023 18:01:11 +0000 Subject: [PATCH 017/101] fix getting the size of column --- src/Storages/MergeTree/MergeTreeDataPartWide.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp index 04e672933a5..f8627ec8073 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp @@ -257,8 +257,10 @@ bool MergeTreeDataPartWide::hasColumnFiles(const NameAndTypePair & column) const bool res = true; getSerialization(column.name)->enumerateStreams([&](const auto & substream_path) { - String file_name = ISerialization::getFileNameForStream(column, substream_path); - if (!check_stream_exists(file_name)) + auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); + auto stream_name = checksums.getFileNameOrHash(full_stream_name); + + if (!check_stream_exists(stream_name)) res = false; }); From 8864e30d2eee950d5d4d6eaaa910754cb66b9343 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Thu, 22 Jun 2023 15:17:13 +0000 Subject: [PATCH 018/101] fix replacing column names after mutation --- src/Storages/MergeTree/IMergeTreeDataPart.cpp | 1 - .../MergeTree/IMergeTreeDataPartWriter.h | 2 +- .../MergeTree/MergeTreeDataPartChecksum.cpp | 37 ++++++------------- .../MergeTreeDataPartWriterCompact.cpp | 2 +- .../MergeTreeDataPartWriterCompact.h | 2 +- .../MergeTreeDataPartWriterInMemory.cpp | 2 +- .../MergeTreeDataPartWriterInMemory.h | 2 +- .../MergeTree/MergeTreeDataPartWriterWide.cpp | 22 ++++++++--- .../MergeTree/MergeTreeDataPartWriterWide.h | 9 +++-- .../MergeTree/MergedBlockOutputStream.cpp | 6 ++- .../MergedColumnOnlyOutputStream.cpp | 10 +++-- .../configs/wide_parts_only.xml | 1 + .../configs/wide_parts_only.xml | 1 + .../test_filesystem_layout/test.py | 2 +- .../configs/wide_parts_only.xml | 1 + tests/integration/test_partition/test.py | 2 +- 16 files changed, 54 insertions(+), 48 deletions(-) diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.cpp b/src/Storages/MergeTree/IMergeTreeDataPart.cpp index fa33bef1582..289c41e5d10 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.cpp +++ b/src/Storages/MergeTree/IMergeTreeDataPart.cpp @@ -1,5 +1,4 @@ #include "IMergeTreeDataPart.h" -#include "Common/SipHash.h" #include "Storages/MergeTree/IDataPartStorage.h" #include diff --git a/src/Storages/MergeTree/IMergeTreeDataPartWriter.h b/src/Storages/MergeTree/IMergeTreeDataPartWriter.h index fa3c675f7da..3f359904ddd 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPartWriter.h +++ b/src/Storages/MergeTree/IMergeTreeDataPartWriter.h @@ -32,7 +32,7 @@ public: virtual void write(const Block & block, const IColumn::Permutation * permutation) = 0; - virtual void fillChecksums(IMergeTreeDataPart::Checksums & checksums) = 0; + virtual void fillChecksums(IMergeTreeDataPart::Checksums & checksums, NameSet & checksums_to_remove) = 0; virtual void finish(bool sync) = 0; diff --git a/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp b/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp index 7d39ea0707f..5dc71147246 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp @@ -68,44 +68,35 @@ void MergeTreeDataPartChecksum::checkSize(const IDataPartStorage & storage, cons void MergeTreeDataPartChecksums::checkEqual(const MergeTreeDataPartChecksums & rhs, bool have_uncompressed) const { - for (const auto & it : rhs.files) - { - const String & name = it.first; - + for (const auto & [name, _] : rhs.files) if (!files.contains(name)) throw Exception(ErrorCodes::UNEXPECTED_FILE_IN_DATA_PART, "Unexpected file {} in data part", name); - } - for (const auto & it : files) + for (const auto & [name, checksum] : files) { - const String & name = it.first; - /// Exclude files written by inverted index from check. No correct checksums are available for them currently. if (name.ends_with(".gin_dict") || name.ends_with(".gin_post") || name.ends_with(".gin_seg") || name.ends_with(".gin_sid")) continue; - auto jt = rhs.files.find(name); - if (jt == rhs.files.end()) + auto it = rhs.files.find(name); + if (it == rhs.files.end()) throw Exception(ErrorCodes::NO_FILE_IN_DATA_PART, "No file {} in data part", name); - it.second.checkEqual(jt->second, have_uncompressed, name); + checksum.checkEqual(it->second, have_uncompressed, name); } } void MergeTreeDataPartChecksums::checkSizes(const IDataPartStorage & storage) const { - for (const auto & it : files) - { - const String & name = it.first; - it.second.checkSize(storage, name); - } + for (const auto & [name, checksum] : files) + checksum.checkSize(storage, name); } UInt64 MergeTreeDataPartChecksums::getTotalSizeOnDisk() const { UInt64 res = 0; - for (const auto & it : files) - res += it.second.file_size; + for (const auto & [_, checksum] : files) + res += checksum.file_size; return res; } @@ -219,11 +210,8 @@ void MergeTreeDataPartChecksums::write(WriteBuffer & to) const writeVarUInt(files.size(), out); - for (const auto & it : files) + for (const auto & [name, sum] : files) { - const String & name = it.first; - const Checksum & sum = it.second; - writeBinary(name, out); writeVarUInt(sum.file_size, out); writePODBinary(sum.file_hash, out); @@ -256,11 +244,8 @@ void MergeTreeDataPartChecksums::add(MergeTreeDataPartChecksums && rhs_checksums void MergeTreeDataPartChecksums::computeTotalChecksumDataOnly(SipHash & hash) const { /// We use fact that iteration is in deterministic (lexicographical) order. - for (const auto & it : files) + for (const auto & [name, sum] : files) { - const String & name = it.first; - const Checksum & sum = it.second; - if (!endsWith(name, ".bin")) continue; diff --git a/src/Storages/MergeTree/MergeTreeDataPartWriterCompact.cpp b/src/Storages/MergeTree/MergeTreeDataPartWriterCompact.cpp index 0b650eb9f16..9b8f1155912 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWriterCompact.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWriterCompact.cpp @@ -403,7 +403,7 @@ size_t MergeTreeDataPartWriterCompact::ColumnsBuffer::size() const return accumulated_columns.at(0)->size(); } -void MergeTreeDataPartWriterCompact::fillChecksums(IMergeTreeDataPart::Checksums & checksums) +void MergeTreeDataPartWriterCompact::fillChecksums(IMergeTreeDataPart::Checksums & checksums, NameSet & /*checksums_to_remove*/) { // If we don't have anything to write, skip finalization. if (!columns_list.empty()) diff --git a/src/Storages/MergeTree/MergeTreeDataPartWriterCompact.h b/src/Storages/MergeTree/MergeTreeDataPartWriterCompact.h index 06f8122393f..b1cfefd2d8f 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWriterCompact.h +++ b/src/Storages/MergeTree/MergeTreeDataPartWriterCompact.h @@ -22,7 +22,7 @@ public: void write(const Block & block, const IColumn::Permutation * permutation) override; - void fillChecksums(IMergeTreeDataPart::Checksums & checksums) override; + void fillChecksums(IMergeTreeDataPart::Checksums & checksums, NameSet & checksums_to_remove) override; void finish(bool sync) override; private: diff --git a/src/Storages/MergeTree/MergeTreeDataPartWriterInMemory.cpp b/src/Storages/MergeTree/MergeTreeDataPartWriterInMemory.cpp index 9afa7a1e80d..048339b58c9 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWriterInMemory.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWriterInMemory.cpp @@ -76,7 +76,7 @@ void MergeTreeDataPartWriterInMemory::calculateAndSerializePrimaryIndex(const Bl } } -void MergeTreeDataPartWriterInMemory::fillChecksums(IMergeTreeDataPart::Checksums & checksums) +void MergeTreeDataPartWriterInMemory::fillChecksums(IMergeTreeDataPart::Checksums & checksums, NameSet & /*checksums_to_remove*/) { /// If part is empty we still need to initialize block by empty columns. if (!part_in_memory->block) diff --git a/src/Storages/MergeTree/MergeTreeDataPartWriterInMemory.h b/src/Storages/MergeTree/MergeTreeDataPartWriterInMemory.h index 9e1e868beac..2d333822652 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWriterInMemory.h +++ b/src/Storages/MergeTree/MergeTreeDataPartWriterInMemory.h @@ -18,7 +18,7 @@ public: /// You can write only one block. In-memory part can be written only at INSERT. void write(const Block & block, const IColumn::Permutation * permutation) override; - void fillChecksums(IMergeTreeDataPart::Checksums & checksums) override; + void fillChecksums(IMergeTreeDataPart::Checksums & checksums, NameSet & checksums_to_remove) override; void finish(bool /*sync*/) override {} private: diff --git a/src/Storages/MergeTree/MergeTreeDataPartWriterWide.cpp b/src/Storages/MergeTree/MergeTreeDataPartWriterWide.cpp index 60bb1119770..c9dae9a1f2c 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWriterWide.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWriterWide.cpp @@ -136,6 +136,7 @@ void MergeTreeDataPartWriterWide::addStreams( settings.query_write_settings); full_name_to_stream_name.emplace(full_stream_name, stream_name); + stream_name_to_full_name.emplace(stream_name, full_stream_name); }; ISerialization::SubstreamPath path; @@ -562,7 +563,7 @@ void MergeTreeDataPartWriterWide::validateColumnOfFixedSize(const NameAndTypePai } -void MergeTreeDataPartWriterWide::fillDataChecksums(IMergeTreeDataPart::Checksums & checksums) +void MergeTreeDataPartWriterWide::fillDataChecksums(IMergeTreeDataPart::Checksums & checksums, NameSet & checksums_to_remove) { const auto & global_settings = storage.getContext()->getSettingsRef(); ISerialization::SerializeBinaryBulkSettings serialize_settings; @@ -598,10 +599,19 @@ void MergeTreeDataPartWriterWide::fillDataChecksums(IMergeTreeDataPart::Checksum } } - for (auto & stream : column_streams) + for (auto & [stream_name, stream] : column_streams) { - stream.second->preFinalize(); - stream.second->addToChecksums(checksums); + /// Remove checksums for old stream name if file was + /// renamed due to replacing the name to the hash of name. + const auto & full_stream_name = stream_name_to_full_name.at(stream_name); + if (stream_name != full_stream_name) + { + checksums_to_remove.insert(full_stream_name + stream->data_file_extension); + checksums_to_remove.insert(full_stream_name + stream->marks_file_extension); + } + + stream->preFinalize(); + stream->addToChecksums(checksums); } } @@ -633,11 +643,11 @@ void MergeTreeDataPartWriterWide::finishDataSerialization(bool sync) } -void MergeTreeDataPartWriterWide::fillChecksums(IMergeTreeDataPart::Checksums & checksums) +void MergeTreeDataPartWriterWide::fillChecksums(IMergeTreeDataPart::Checksums & checksums, NameSet & checksums_to_remove) { // If we don't have anything to write, skip finalization. if (!columns_list.empty()) - fillDataChecksums(checksums); + fillDataChecksums(checksums, checksums_to_remove); if (settings.rewrite_primary_key) fillPrimaryIndexChecksums(checksums); diff --git a/src/Storages/MergeTree/MergeTreeDataPartWriterWide.h b/src/Storages/MergeTree/MergeTreeDataPartWriterWide.h index de7419fedb2..c274fc9807c 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWriterWide.h +++ b/src/Storages/MergeTree/MergeTreeDataPartWriterWide.h @@ -29,14 +29,14 @@ public: void write(const Block & block, const IColumn::Permutation * permutation) override; - void fillChecksums(IMergeTreeDataPart::Checksums & checksums) final; + void fillChecksums(IMergeTreeDataPart::Checksums & checksums, NameSet & checksums_to_remove) final; void finish(bool sync) final; private: /// Finish serialization of data: write final mark if required and compute checksums /// Also validate written data in debug mode - void fillDataChecksums(IMergeTreeDataPart::Checksums & checksums); + void fillDataChecksums(IMergeTreeDataPart::Checksums & checksums, NameSet & checksums_to_remove); void finishDataSerialization(bool sync); /// Write data of one column. @@ -111,8 +111,11 @@ private: using ColumnStreams = std::map; ColumnStreams column_streams; - /// TODO: + /// Some long column names may be replaced to hashes. + /// Below are mapping from original stream name to actual + /// stream name (probably hash of the stream) and vice versa. std::unordered_map full_name_to_stream_name; + std::unordered_map stream_name_to_full_name; /// Non written marks to disk (for each column). Waiting until all rows for /// this marks will be written to disk. diff --git a/src/Storages/MergeTree/MergedBlockOutputStream.cpp b/src/Storages/MergeTree/MergedBlockOutputStream.cpp index c93ad135835..1ebb1d87aae 100644 --- a/src/Storages/MergeTree/MergedBlockOutputStream.cpp +++ b/src/Storages/MergeTree/MergedBlockOutputStream.cpp @@ -142,12 +142,16 @@ MergedBlockOutputStream::Finalizer MergedBlockOutputStream::finalizePartAsync( { /// Finish write and get checksums. MergeTreeData::DataPart::Checksums checksums; + NameSet checksums_to_remove; if (additional_column_checksums) checksums = std::move(*additional_column_checksums); /// Finish columns serialization. - writer->fillChecksums(checksums); + writer->fillChecksums(checksums, checksums_to_remove); + + for (const auto & name : checksums_to_remove) + checksums.files.erase(name); LOG_TRACE(&Poco::Logger::get("MergedBlockOutputStream"), "filled checksums {}", new_part->getNameWithState()); diff --git a/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp b/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp index 3b2eb96f2d4..108f364fc2d 100644 --- a/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp +++ b/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp @@ -63,7 +63,11 @@ MergedColumnOnlyOutputStream::fillChecksums( { /// Finish columns serialization. MergeTreeData::DataPart::Checksums checksums; - writer->fillChecksums(checksums); + NameSet checksums_to_remove; + writer->fillChecksums(checksums, checksums_to_remove); + + for (const auto & filename : checksums_to_remove) + all_checksums.files.erase(filename); for (const auto & [projection_name, projection_part] : new_part->getProjectionParts()) checksums.addFile( @@ -80,9 +84,7 @@ MergedColumnOnlyOutputStream::fillChecksums( for (const String & removed_file : removed_files) { new_part->getDataPartStorage().removeFileIfExists(removed_file); - - if (all_checksums.files.contains(removed_file)) - all_checksums.files.erase(removed_file); + all_checksums.files.erase(removed_file); } new_part->setColumns(columns, serialization_infos, metadata_snapshot->getMetadataVersion()); diff --git a/tests/integration/test_backward_compatibility/configs/wide_parts_only.xml b/tests/integration/test_backward_compatibility/configs/wide_parts_only.xml index e9cf053f1c5..674ffff6c93 100644 --- a/tests/integration/test_backward_compatibility/configs/wide_parts_only.xml +++ b/tests/integration/test_backward_compatibility/configs/wide_parts_only.xml @@ -1,5 +1,6 @@ 0 + 0 diff --git a/tests/integration/test_default_compression_codec/configs/wide_parts_only.xml b/tests/integration/test_default_compression_codec/configs/wide_parts_only.xml index 10b9edef36d..4d1a3357799 100644 --- a/tests/integration/test_default_compression_codec/configs/wide_parts_only.xml +++ b/tests/integration/test_default_compression_codec/configs/wide_parts_only.xml @@ -2,5 +2,6 @@ 0 0 + 0 diff --git a/tests/integration/test_filesystem_layout/test.py b/tests/integration/test_filesystem_layout/test.py index 2be478f95d0..81f3b67cb75 100644 --- a/tests/integration/test_filesystem_layout/test.py +++ b/tests/integration/test_filesystem_layout/test.py @@ -23,7 +23,7 @@ def test_file_path_escaping(started_cluster): node.query( """ CREATE TABLE test.`T.a_b,l-e!` (`~Id` UInt32) - ENGINE = MergeTree() PARTITION BY `~Id` ORDER BY `~Id` SETTINGS min_bytes_for_wide_part = 0; + ENGINE = MergeTree() PARTITION BY `~Id` ORDER BY `~Id` SETTINGS min_bytes_for_wide_part = 0, replace_long_file_name_to_hash = 0; """ ) node.query("""INSERT INTO test.`T.a_b,l-e!` VALUES (1);""") diff --git a/tests/integration/test_mutations_hardlinks/configs/wide_parts_only.xml b/tests/integration/test_mutations_hardlinks/configs/wide_parts_only.xml index 10b9edef36d..4d1a3357799 100644 --- a/tests/integration/test_mutations_hardlinks/configs/wide_parts_only.xml +++ b/tests/integration/test_mutations_hardlinks/configs/wide_parts_only.xml @@ -2,5 +2,6 @@ 0 0 + 0 diff --git a/tests/integration/test_partition/test.py b/tests/integration/test_partition/test.py index 93f03f4420e..7634c81f807 100644 --- a/tests/integration/test_partition/test.py +++ b/tests/integration/test_partition/test.py @@ -150,7 +150,7 @@ def partition_table_complex(started_cluster): q("DROP TABLE IF EXISTS test.partition_complex") q( "CREATE TABLE test.partition_complex (p Date, k Int8, v1 Int8 MATERIALIZED k + 1) " - "ENGINE = MergeTree PARTITION BY p ORDER BY k SETTINGS index_granularity=1, index_granularity_bytes=0, compress_marks=false, compress_primary_key=false" + "ENGINE = MergeTree PARTITION BY p ORDER BY k SETTINGS index_granularity=1, index_granularity_bytes=0, compress_marks=false, compress_primary_key=false, replace_long_file_name_to_hash = false" ) q("INSERT INTO test.partition_complex (p, k) VALUES(toDate(31), 1)") q("INSERT INTO test.partition_complex (p, k) VALUES(toDate(1), 2)") From a71cd56a906a05fd31e878112b79f58676a8156e Mon Sep 17 00:00:00 2001 From: avogar Date: Tue, 1 Aug 2023 10:06:56 +0000 Subject: [PATCH 019/101] Output valid JSON/XML on excetpion during HTTP query execution --- docs/en/interfaces/http.md | 13 + src/Core/Settings.h | 1 + src/Core/SettingsChangesHistory.h | 1 + src/Formats/FormatFactory.cpp | 8 +- src/Formats/FormatSettings.h | 6 + src/Formats/JSONUtils.cpp | 6 + src/Formats/JSONUtils.h | 2 + src/IO/PeekableWriteBuffer.cpp | 85 ++++ src/IO/PeekableWriteBuffer.h | 59 +++ src/Interpreters/executeQuery.cpp | 18 +- src/Interpreters/executeQuery.h | 5 +- src/Processors/Formats/IOutputFormat.h | 8 + .../Impl/JSONColumnsBlockOutputFormatBase.cpp | 3 +- .../Impl/JSONColumnsBlockOutputFormatBase.h | 1 + .../JSONCompactEachRowRowOutputFormat.cpp | 16 +- .../Impl/JSONCompactEachRowRowOutputFormat.h | 8 +- .../Impl/JSONEachRowRowOutputFormat.cpp | 18 +- .../Formats/Impl/JSONEachRowRowOutputFormat.h | 9 +- .../Impl/JSONObjectEachRowRowOutputFormat.cpp | 7 + .../Formats/Impl/JSONRowOutputFormat.cpp | 13 +- .../Formats/Impl/JSONRowOutputFormat.h | 5 +- .../Impl/ParallelFormattingOutputFormat.cpp | 48 +- .../Impl/ParallelFormattingOutputFormat.h | 27 ++ .../Formats/Impl/XMLRowOutputFormat.cpp | 13 +- .../Formats/Impl/XMLRowOutputFormat.h | 5 +- .../OutputFormatWithExceptionHandlerAdaptor.h | 75 +++ .../OutputFormatWithUTF8ValidationAdaptor.h | 32 +- ...wOutputFormatWithExceptionHandlerAdaptor.h | 104 +++++ src/Server/HTTPHandler.cpp | 80 ++-- src/Server/HTTPHandler.h | 2 + ...d_json_and_xml_on_http_exception.reference | 432 ++++++++++++++++++ ...41_valid_json_and_xml_on_http_exception.sh | 106 +++++ 32 files changed, 1138 insertions(+), 78 deletions(-) create mode 100644 src/IO/PeekableWriteBuffer.cpp create mode 100644 src/IO/PeekableWriteBuffer.h create mode 100644 src/Processors/Formats/OutputFormatWithExceptionHandlerAdaptor.h create mode 100644 src/Processors/Formats/RowOutputFormatWithExceptionHandlerAdaptor.h create mode 100644 tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.reference create mode 100755 tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh diff --git a/docs/en/interfaces/http.md b/docs/en/interfaces/http.md index 37821f0fee1..b28180fec67 100644 --- a/docs/en/interfaces/http.md +++ b/docs/en/interfaces/http.md @@ -697,3 +697,16 @@ $ curl -vv -H 'XXX:xxx' 'http://localhost:8123/get_relative_path_static_handler' Relative Path File * Connection #0 to host localhost left intact ``` + +## Valid JSON/XML response on exception during HTTP streaming {valid-output-on-exception-http-streaming} + +While query execution over HTTP an exception can happen when part of the data has already been sent. Usually an exception is sent to the client in plain text +even if some specific data format was used to output data and the output may become invalid in terms of specified data format. +To prevent it, you can use setting `http_write_exception_in_output_format` (enabled by default) that will tell ClickHouse to write an exception in specified format (currently supported for XML and JSON* formats). + +Examples: + +```bash + +``` + diff --git a/src/Core/Settings.h b/src/Core/Settings.h index c69d132ea25..b8ba6454f61 100644 --- a/src/Core/Settings.h +++ b/src/Core/Settings.h @@ -276,6 +276,7 @@ class IColumn; \ M(UInt64, http_headers_progress_interval_ms, 100, "Do not send HTTP headers X-ClickHouse-Progress more frequently than at each specified interval.", 0) \ M(Bool, http_wait_end_of_query, false, "Enable HTTP response buffering on the server-side.", 0) \ + M(Bool, http_write_exception_in_output_format, true, "Write exception in output format to produce valid output. Works with JSON and XML formats.", 0) \ M(UInt64, http_response_buffer_size, 0, "The number of bytes to buffer in the server memory before sending a HTTP response to the client or flushing to disk (when http_wait_end_of_query is enabled).", 0) \ \ M(Bool, fsync_metadata, true, "Do fsync after changing metadata for tables and databases (.sql files). Could be disabled in case of poor latency on server with high load of DDL queries and high load of disk subsystem.", 0) \ diff --git a/src/Core/SettingsChangesHistory.h b/src/Core/SettingsChangesHistory.h index 70b702f1b33..3172b246d68 100644 --- a/src/Core/SettingsChangesHistory.h +++ b/src/Core/SettingsChangesHistory.h @@ -80,6 +80,7 @@ namespace SettingsChangesHistory /// It's used to implement `compatibility` setting (see https://github.com/ClickHouse/ClickHouse/issues/35972) static std::map settings_changes_history = { + {"23.8", {{"http_write_exception_in_output_format", false, true, "Output valid JSON/XML on exception in HTTP streaming."}}}, {"23.7", {{"function_sleep_max_microseconds_per_block", 0, 3000000, "In previous versions, the maximum sleep time of 3 seconds was applied only for `sleep`, but not for `sleepEachRow` function. In the new version, we introduce this setting. If you set compatibility with the previous versions, we will disable the limit altogether."}}}, {"23.6", {{"http_send_timeout", 180, 30, "3 minutes seems crazy long. Note that this is timeout for a single network write call, not for the whole upload operation."}, {"http_receive_timeout", 180, 30, "See http_send_timeout."}}}, diff --git a/src/Formats/FormatFactory.cpp b/src/Formats/FormatFactory.cpp index 663b7f1ba95..4cd2ad5be03 100644 --- a/src/Formats/FormatFactory.cpp +++ b/src/Formats/FormatFactory.cpp @@ -1,7 +1,7 @@ #include #include -#include +//#include #include #include #include @@ -224,6 +224,12 @@ FormatSettings getFormatSettings(ContextPtr context, const Settings & settings) context->getRemoteHostFilter().checkURL(avro_schema_registry_url); } + if (context->getClientInfo().interface == ClientInfo::Interface::HTTP && context->getSettingsRef().http_write_exception_in_output_format.value) + { + format_settings.json.valid_output_on_exception = true; + format_settings.xml.valid_output_on_exception = true; + } + return format_settings; } diff --git a/src/Formats/FormatSettings.h b/src/Formats/FormatSettings.h index 3259c46e5ff..a2ef0b035e9 100644 --- a/src/Formats/FormatSettings.h +++ b/src/Formats/FormatSettings.h @@ -198,6 +198,7 @@ struct FormatSettings bool validate_types_from_metadata = true; bool validate_utf8 = false; bool allow_object_type = false; + bool valid_output_on_exception = false; } json; struct @@ -399,6 +400,11 @@ struct FormatSettings { bool allow_types_conversion = true; } native; + + struct + { + bool valid_output_on_exception = false; + } xml; }; } diff --git a/src/Formats/JSONUtils.cpp b/src/Formats/JSONUtils.cpp index 0aac72c68fe..aead2a3806a 100644 --- a/src/Formats/JSONUtils.cpp +++ b/src/Formats/JSONUtils.cpp @@ -451,6 +451,12 @@ namespace JSONUtils } } + void writeException(const String & exception_message, WriteBuffer & out, const FormatSettings & settings, size_t indent) + { + writeTitle("exception", out, indent, " "); + writeJSONString(exception_message, out, settings); + } + Strings makeNamesValidJSONStrings(const Strings & names, const FormatSettings & settings, bool validate_utf8) { Strings result; diff --git a/src/Formats/JSONUtils.h b/src/Formats/JSONUtils.h index fd1ba7db980..c023125ce66 100644 --- a/src/Formats/JSONUtils.h +++ b/src/Formats/JSONUtils.h @@ -105,6 +105,8 @@ namespace JSONUtils bool write_statistics, WriteBuffer & out); + void writeException(const String & exception_message, WriteBuffer & out, const FormatSettings & settings, size_t indent = 0); + void skipColon(ReadBuffer & in); void skipComma(ReadBuffer & in); diff --git a/src/IO/PeekableWriteBuffer.cpp b/src/IO/PeekableWriteBuffer.cpp new file mode 100644 index 00000000000..dc7f87dd539 --- /dev/null +++ b/src/IO/PeekableWriteBuffer.cpp @@ -0,0 +1,85 @@ +#include + +namespace DB +{ + +PeekableWriteBuffer::PeekableWriteBuffer(DB::WriteBuffer & sub_buf_) : BufferWithOwnMemory(0), sub_buf(sub_buf_) +{ + Buffer & sub_working = sub_buf.buffer(); + BufferBase::set(sub_working.begin(), sub_working.size(), sub_buf.offset()); +} + +void PeekableWriteBuffer::nextImpl() +{ + if (checkpoint) + { + if (write_to_own_memory) + { + size_t prev_size = position() - memory.data(); + size_t new_size = memory.size() * 2; + memory.resize(new_size); + BufferBase::set(memory.data(), memory.size(), prev_size); + return; + } + + if (memory.size() == 0) + memory.resize(DBMS_DEFAULT_BUFFER_SIZE); + + sub_buf.position() = position(); + BufferBase::set(memory.data(), memory.size(), 0); + write_to_own_memory = true; + return; + } + + sub_buf.position() = position(); + sub_buf.next(); + BufferBase::set(sub_buf.buffer().begin(), sub_buf.buffer().size(), sub_buf.offset()); +} + + +void PeekableWriteBuffer::dropCheckpoint() +{ + assert(checkpoint); + checkpoint = std::nullopt; + /// If we have saved data in own memory, write it to sub-buf. + if (write_to_own_memory) + { + try + { + sub_buf.next(); + sub_buf.write(memory.data(), position() - memory.data()); + Buffer & sub_working = sub_buf.buffer(); + BufferBase::set(sub_working.begin(), sub_working.size(), sub_buf.offset()); + write_to_own_memory = false; + } + catch (...) + { + /// If exception happened during writing to sub buffer, we should + /// update buffer to not leave it in invalid state. + Buffer & sub_working = sub_buf.buffer(); + BufferBase::set(sub_working.begin(), sub_working.size(), sub_buf.offset()); + write_to_own_memory = false; + } + } + +} + +void PeekableWriteBuffer::rollbackToCheckpoint(bool drop) +{ + assert(checkpoint); + + /// Just ignore all data written after checkpoint. + if (write_to_own_memory) + { + Buffer & sub_working = sub_buf.buffer(); + BufferBase::set(sub_working.begin(), sub_working.size(), sub_buf.offset()); + write_to_own_memory = false; + } + + position() = *checkpoint; + + if (drop) + checkpoint = std::nullopt; +} + +} diff --git a/src/IO/PeekableWriteBuffer.h b/src/IO/PeekableWriteBuffer.h new file mode 100644 index 00000000000..e7094f11fcb --- /dev/null +++ b/src/IO/PeekableWriteBuffer.h @@ -0,0 +1,59 @@ +#pragma once +#include +#include +#include + +namespace DB +{ + +namespace ErrorCodes +{ + extern const int LOGICAL_ERROR; +} + +/// Similar to PeekableReadBuffer. +/// Allows to set checkpoint at some position in stream and come back to this position later. +/// When next() is called, saves data between checkpoint and current position to own memory instead of writing it to sub-buffer. +/// So, all the data after checkpoint won't be written in sub-buffer until checkpoint is dropped. +/// Rollback to checkpoint means that all data after checkpoint will be ignored and not sent to sub-buffer. +/// Sub-buffer should not be accessed directly during the lifetime of peekable buffer (unless +/// you reset() the state of peekable buffer after each change of underlying buffer) +/// If position() of peekable buffer is explicitly set to some position before checkpoint +/// (e.g. by istr.position() = prev_pos), behavior is undefined. +class PeekableWriteBuffer : public BufferWithOwnMemory +{ + friend class PeekableWriteBufferCheckpoint; +public: + explicit PeekableWriteBuffer(WriteBuffer & sub_buf_); + + /// Sets checkpoint at current position + ALWAYS_INLINE inline void setCheckpoint() + { + if (checkpoint) + throw Exception(ErrorCodes::LOGICAL_ERROR, "PeekableWriteBuffer does not support recursive checkpoints."); + + checkpoint.emplace(pos); + } + + /// Forget checkpoint and send all data from checkpoint to position to sub-buffer. + void dropCheckpoint(); + + /// Sets position at checkpoint and forget all data written from checkpoint to position. + /// All pointers (such as this->buffer().end()) may be invalidated + void rollbackToCheckpoint(bool drop = false); + + void finalizeImpl() override + { + assert(!checkpoint); + sub_buf.position() = position(); + } + +private: + void nextImpl() override; + + WriteBuffer & sub_buf; + bool write_to_own_memory = false; + std::optional checkpoint = std::nullopt; +}; + +} diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index 578ca3b41f9..651305d9c52 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -1250,7 +1250,8 @@ void executeQuery( bool allow_into_outfile, ContextMutablePtr context, SetResultDetailsFunc set_result_details, - const std::optional & output_format_settings) + const std::optional & output_format_settings, + HandleExceptionInOutputFormatFunc handle_exception_in_output_format) { PODArray parse_buf; const char * begin; @@ -1308,6 +1309,7 @@ void executeQuery( ASTPtr ast; BlockIO streams; + OutputFormatPtr output_format; std::tie(ast, streams) = executeQueryImpl(begin, end, context, false, QueryProcessingStage::Complete, &istr); auto & pipeline = streams.pipeline; @@ -1350,30 +1352,30 @@ void executeQuery( ? getIdentifierName(ast_query_with_output->format) : context->getDefaultFormat(); - auto out = FormatFactory::instance().getOutputFormatParallelIfPossible( + output_format = FormatFactory::instance().getOutputFormatParallelIfPossible( format_name, compressed_buffer ? *compressed_buffer : *out_buf, materializeBlock(pipeline.getHeader()), context, output_format_settings); - out->setAutoFlush(); + output_format->setAutoFlush(); /// Save previous progress callback if any. TODO Do it more conveniently. auto previous_progress_callback = context->getProgressCallback(); /// NOTE Progress callback takes shared ownership of 'out'. - pipeline.setProgressCallback([out, previous_progress_callback] (const Progress & progress) + pipeline.setProgressCallback([output_format, previous_progress_callback] (const Progress & progress) { if (previous_progress_callback) previous_progress_callback(progress); - out->onProgress(progress); + output_format->onProgress(progress); }); - result_details.content_type = out->getContentType(); + result_details.content_type = output_format->getContentType(); result_details.format = format_name; - pipeline.complete(std::move(out)); + pipeline.complete(output_format); } else { @@ -1403,6 +1405,8 @@ void executeQuery( } catch (...) { + if (handle_exception_in_output_format && output_format) + handle_exception_in_output_format(*output_format); streams.onException(); throw; } diff --git a/src/Interpreters/executeQuery.h b/src/Interpreters/executeQuery.h index f2a12bbef18..11ef17aaade 100644 --- a/src/Interpreters/executeQuery.h +++ b/src/Interpreters/executeQuery.h @@ -15,6 +15,7 @@ namespace DB class IInterpreter; class ReadBuffer; class WriteBuffer; +class IOutputFormat; struct QueryStatusInfo; struct QueryResultDetails @@ -26,6 +27,7 @@ struct QueryResultDetails }; using SetResultDetailsFunc = std::function; +using HandleExceptionInOutputFormatFunc = std::function; /// Parse and execute a query. void executeQuery( @@ -34,7 +36,8 @@ void executeQuery( bool allow_into_outfile, /// If true and the query contains INTO OUTFILE section, redirect output to that file. ContextMutablePtr context, /// DB, tables, data types, storage engines, functions, aggregate functions... SetResultDetailsFunc set_result_details, /// If a non-empty callback is passed, it will be called with the query id, the content-type, the format, and the timezone. - const std::optional & output_format_settings = std::nullopt /// Format settings for output format, will be calculated from the context if not set. + const std::optional & output_format_settings = std::nullopt, /// Format settings for output format, will be calculated from the context if not set. + HandleExceptionInOutputFormatFunc handle_exception_in_output_format = {} /// If a non-empty callback is passed, it will be called on exception with created output format. ); diff --git a/src/Processors/Formats/IOutputFormat.h b/src/Processors/Formats/IOutputFormat.h index 58700a978ff..cae2ab7691e 100644 --- a/src/Processors/Formats/IOutputFormat.h +++ b/src/Processors/Formats/IOutputFormat.h @@ -71,6 +71,9 @@ public: consumeExtremes(Chunk(extremes.getColumns(), extremes.rows())); } + virtual bool supportsWritingException() const { return false; } + virtual void setException(const String & /*exception_message*/) {} + size_t getResultRows() const { return result_rows; } size_t getResultBytes() const { return result_bytes; } @@ -162,6 +165,11 @@ protected: /// outputs them in finalize() method. virtual bool areTotalsAndExtremesUsedInFinalize() const { return false; } + /// Derived classes can use some wrappers around out WriteBuffer + /// and can override this method to return wrapper + /// that should be used in its derived classes. + virtual WriteBuffer * getWriteBufferPtr() { return &out; } + WriteBuffer & out; Chunk current_chunk; diff --git a/src/Processors/Formats/Impl/JSONColumnsBlockOutputFormatBase.cpp b/src/Processors/Formats/Impl/JSONColumnsBlockOutputFormatBase.cpp index 490516b7eb4..72a009c20bf 100644 --- a/src/Processors/Formats/Impl/JSONColumnsBlockOutputFormatBase.cpp +++ b/src/Processors/Formats/Impl/JSONColumnsBlockOutputFormatBase.cpp @@ -9,10 +9,11 @@ namespace DB JSONColumnsBlockOutputFormatBase::JSONColumnsBlockOutputFormatBase( WriteBuffer & out_, const Block & header_, const FormatSettings & format_settings_, bool validate_utf8) - : OutputFormatWithUTF8ValidationAdaptor(validate_utf8, header_, out_) + : OutputFormatWithUTF8ValidationAdaptor(header_, out_, validate_utf8) , format_settings(format_settings_) , serializations(header_.getSerializations()) { + ostr = OutputFormatWithUTF8ValidationAdaptor::getWriteBufferPtr(); } void JSONColumnsBlockOutputFormatBase::consume(Chunk chunk) diff --git a/src/Processors/Formats/Impl/JSONColumnsBlockOutputFormatBase.h b/src/Processors/Formats/Impl/JSONColumnsBlockOutputFormatBase.h index 235a6d4da96..d73ac53b97a 100644 --- a/src/Processors/Formats/Impl/JSONColumnsBlockOutputFormatBase.h +++ b/src/Processors/Formats/Impl/JSONColumnsBlockOutputFormatBase.h @@ -38,6 +38,7 @@ protected: Chunk mono_chunk; size_t written_rows = 0; + WriteBuffer * ostr; }; } diff --git a/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.cpp b/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.cpp index 0cafc053467..c5c9af60982 100644 --- a/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.cpp @@ -15,12 +15,13 @@ JSONCompactEachRowRowOutputFormat::JSONCompactEachRowRowOutputFormat(WriteBuffer bool with_names_, bool with_types_, bool yield_strings_) - : RowOutputFormatWithUTF8ValidationAdaptor(settings_.json.validate_utf8, header_, out_) + : RowOutputFormatWithExceptionHandlerAdaptor(header_, out_, settings_.json.valid_output_on_exception, settings_.json.validate_utf8) , settings(settings_) , with_names(with_names_) , with_types(with_types_) , yield_strings(yield_strings_) { + ostr = RowOutputFormatWithExceptionHandlerAdaptor::getWriteBufferPtr(); } @@ -102,6 +103,19 @@ void JSONCompactEachRowRowOutputFormat::consumeTotals(DB::Chunk chunk) IRowOutputFormat::consumeTotals(std::move(chunk)); } +void JSONCompactEachRowRowOutputFormat::writeSuffix() +{ + if (!exception_message.empty()) + { + if (haveWrittenData()) + writeRowBetweenDelimiter(); + + writeRowStartDelimiter(); + writeJSONString(exception_message, *ostr, settings); + writeRowEndDelimiter(); + } +} + void registerOutputFormatJSONCompactEachRow(FormatFactory & factory) { for (bool yield_strings : {false, true}) diff --git a/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.h b/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.h index 2be39669dd2..a05fff699a5 100644 --- a/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.h +++ b/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.h @@ -3,15 +3,16 @@ #include #include #include +#include #include namespace DB { -/** The stream for outputting data in JSON format, by object per line. +/** The stream for outputting data in JSON format, by JSON array per line. */ -class JSONCompactEachRowRowOutputFormat final : public RowOutputFormatWithUTF8ValidationAdaptor +class JSONCompactEachRowRowOutputFormat final : public RowOutputFormatWithExceptionHandlerAdaptor { public: JSONCompactEachRowRowOutputFormat( @@ -33,6 +34,7 @@ private: void writeFieldDelimiter() override; void writeRowStartDelimiter() override; void writeRowEndDelimiter() override; + void writeSuffix() override; bool supportTotals() const override { return true; } void consumeTotals(Chunk) override; @@ -43,5 +45,7 @@ private: bool with_names; bool with_types; bool yield_strings; + + WriteBuffer * ostr; }; } diff --git a/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.cpp b/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.cpp index 5b8f6cc1af7..2169d815fbf 100644 --- a/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.cpp @@ -3,6 +3,7 @@ #include #include #include +#include namespace DB @@ -14,10 +15,12 @@ JSONEachRowRowOutputFormat::JSONEachRowRowOutputFormat( const Block & header_, const FormatSettings & settings_, bool pretty_json_) - : RowOutputFormatWithUTF8ValidationAdaptor(settings_.json.validate_utf8, header_, out_), - pretty_json(pretty_json_), - settings(settings_) + : RowOutputFormatWithExceptionHandlerAdaptor( + header_, out_, settings_.json.valid_output_on_exception, settings_.json.validate_utf8) + , pretty_json(pretty_json_) + , settings(settings_) { + ostr = RowOutputFormatWithExceptionHandlerAdaptor::getWriteBufferPtr(); fields = JSONUtils::makeNamesValidJSONStrings(getPort(PortKind::Main).getHeader().getNames(), settings, settings.json.validate_utf8); } @@ -76,6 +79,15 @@ void JSONEachRowRowOutputFormat::writePrefix() void JSONEachRowRowOutputFormat::writeSuffix() { + if (!exception_message.empty()) + { + if (haveWrittenData()) + writeRowBetweenDelimiter(); + writeRowStartDelimiter(); + JSONUtils::writeException(exception_message, *ostr, settings, pretty_json ? 1 : 0); + writeRowEndDelimiter(); + } + if (settings.json.array_of_rows) writeCString("\n]\n", *ostr); } diff --git a/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.h b/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.h index e05d189afe9..28bfbf2e6ac 100644 --- a/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.h +++ b/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.h @@ -2,7 +2,9 @@ #include #include +#include #include +#include #include @@ -11,7 +13,7 @@ namespace DB /** The stream for outputting data in JSON format, by object per line. */ -class JSONEachRowRowOutputFormat : public RowOutputFormatWithUTF8ValidationAdaptor +class JSONEachRowRowOutputFormat : public RowOutputFormatWithExceptionHandlerAdaptor { public: JSONEachRowRowOutputFormat( @@ -40,10 +42,11 @@ protected: size_t field_number = 0; bool pretty_json; + FormatSettings settings; + WriteBuffer * ostr; + private: Names fields; - - FormatSettings settings; }; } diff --git a/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp b/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp index a02199d6075..8f4d11a604a 100644 --- a/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp @@ -62,6 +62,13 @@ void JSONObjectEachRowRowOutputFormat::writeRowBetweenDelimiter() void JSONObjectEachRowRowOutputFormat::writeSuffix() { + if (!exception_message.empty()) + { + if (haveWrittenData()) + writeRowBetweenDelimiter(); + JSONUtils::writeException(exception_message, *ostr, settings, 1); + } + JSONUtils::writeObjectEnd(*ostr); writeChar('\n', *ostr); } diff --git a/src/Processors/Formats/Impl/JSONRowOutputFormat.cpp b/src/Processors/Formats/Impl/JSONRowOutputFormat.cpp index 0193ec7e3d3..e4c4e2a3bc6 100644 --- a/src/Processors/Formats/Impl/JSONRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONRowOutputFormat.cpp @@ -13,9 +13,10 @@ JSONRowOutputFormat::JSONRowOutputFormat( const Block & header, const FormatSettings & settings_, bool yield_strings_) - : RowOutputFormatWithUTF8ValidationAdaptor(true, header, out_), settings(settings_), yield_strings(yield_strings_) + : RowOutputFormatWithExceptionHandlerAdaptor(header, out_, settings_.json.valid_output_on_exception, true), settings(settings_), yield_strings(yield_strings_) { names = JSONUtils::makeNamesValidJSONStrings(header.getNames(), settings, true); + ostr = RowOutputFormatWithExceptionHandlerAdaptor::getWriteBufferPtr(); } @@ -117,9 +118,15 @@ void JSONRowOutputFormat::finalizeImpl() statistics.applied_limit, statistics.watch, statistics.progress, - settings.write_statistics, + settings.write_statistics && exception_message.empty(), *ostr); + if (!exception_message.empty()) + { + writeCString(",\n\n", *ostr); + JSONUtils::writeException(exception_message, *ostr, settings, 1); + } + JSONUtils::writeObjectEnd(*ostr); writeChar('\n', *ostr); ostr->next(); @@ -127,7 +134,7 @@ void JSONRowOutputFormat::finalizeImpl() void JSONRowOutputFormat::resetFormatterImpl() { - RowOutputFormatWithUTF8ValidationAdaptor::resetFormatterImpl(); + RowOutputFormatWithExceptionHandlerAdaptor::resetFormatterImpl(); row_count = 0; statistics = Statistics(); } diff --git a/src/Processors/Formats/Impl/JSONRowOutputFormat.h b/src/Processors/Formats/Impl/JSONRowOutputFormat.h index dc3f0541af0..a38cd0e8db9 100644 --- a/src/Processors/Formats/Impl/JSONRowOutputFormat.h +++ b/src/Processors/Formats/Impl/JSONRowOutputFormat.h @@ -3,8 +3,10 @@ #include #include #include +#include #include #include +#include #include @@ -13,7 +15,7 @@ namespace DB /** Stream for output data in JSON format. */ -class JSONRowOutputFormat : public RowOutputFormatWithUTF8ValidationAdaptor +class JSONRowOutputFormat : public RowOutputFormatWithExceptionHandlerAdaptor { public: JSONRowOutputFormat( @@ -69,6 +71,7 @@ protected: FormatSettings settings; bool yield_strings; + WriteBuffer * ostr; }; } diff --git a/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.cpp b/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.cpp index 46fe2ba26a8..3e63e2abd6c 100644 --- a/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.cpp +++ b/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.cpp @@ -8,7 +8,6 @@ namespace DB void ParallelFormattingOutputFormat::finalizeImpl() { need_flush = true; - IOutputFormat::finalized = true; /// Don't throw any background_exception here, because we want to finalize the execution. /// Exception will be checked after main thread is finished. addChunk(Chunk{}, ProcessingUnitType::FINALIZE, /*can_throw_exception*/ false); @@ -24,8 +23,29 @@ namespace DB std::lock_guard lock(mutex); if (background_exception) - std::rethrow_exception(background_exception); + { + collector_finished.set(); + rethrowBackgroundException(); + } } + + if (collected_prefix && collected_suffix && collected_finalize) + return; + + auto formatter = internal_formatter_creator(out); + formatter->setRowsReadBefore(rows_collected); + formatter->setException(exception_message); + + if (!collected_prefix) + formatter->writePrefix(); + + if (!collected_suffix) + formatter->writeSuffix(); + + if (!collected_finalize) + formatter->finalizeImpl(); + + formatter->finalizeBuffers(); } void ParallelFormattingOutputFormat::addChunk(Chunk chunk, ProcessingUnitType type, bool can_throw_exception) @@ -33,7 +53,7 @@ namespace DB { std::lock_guard lock(mutex); if (background_exception && can_throw_exception) - std::rethrow_exception(background_exception); + rethrowBackgroundException(); } const auto current_unit_number = writer_unit_number % processing_units.size(); @@ -62,7 +82,10 @@ namespace DB size_t first_row_num = rows_consumed; if (unit.type == ProcessingUnitType::PLAIN) + { rows_consumed += unit.chunk.getNumRows(); + unit.rows_num = unit.chunk.getNumRows(); + } scheduleFormatterThreadForUnitWithNumber(current_unit_number, first_row_num); ++writer_unit_number; @@ -125,7 +148,7 @@ namespace DB assert(unit.status == READY_TO_READ); /// Use this copy to after notification to stop the execution. - auto copy_if_unit_type = unit.type; + auto copy_of_unit_type = unit.type; /// Do main work here. out.write(unit.segment.data(), unit.actual_memory_size); @@ -134,6 +157,7 @@ namespace DB IOutputFormat::flush(); ++collector_unit_number; + rows_collected += unit.rows_num; { /// Notify other threads. @@ -141,9 +165,19 @@ namespace DB unit.status = READY_TO_INSERT; writer_condvar.notify_all(); } - /// We can exit only after writing last piece of to out buffer. - if (copy_if_unit_type == ProcessingUnitType::FINALIZE) + + if (copy_of_unit_type == ProcessingUnitType::START) { + collected_prefix = true; + } + else if (copy_of_unit_type == ProcessingUnitType::PLAIN_FINISH) + { + collected_suffix = true; + } + /// We can exit only after writing last piece of data to out buffer. + else if (copy_of_unit_type == ProcessingUnitType::FINALIZE) + { + collected_finalize = true; break; } } @@ -156,7 +190,6 @@ namespace DB } } - void ParallelFormattingOutputFormat::formatterThreadFunction(size_t current_unit_number, size_t first_row_num, const ThreadGroupPtr & thread_group) { SCOPE_EXIT_SAFE( @@ -184,6 +217,7 @@ namespace DB auto formatter = internal_formatter_creator(out_buffer); formatter->setRowsReadBefore(first_row_num); + formatter->setException(exception_message); switch (unit.type) { diff --git a/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.h b/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.h index 490f033b87e..b9a3b7638fa 100644 --- a/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.h +++ b/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.h @@ -142,6 +142,14 @@ public: return internal_formatter_creator(buffer)->getContentType(); } + bool supportsWritingException() const override + { + WriteBufferFromOwnString buffer; + return internal_formatter_creator(buffer)->supportsWritingException(); + } + + void setException(const String & exception_message_) override { exception_message = exception_message_; } + private: void consume(Chunk chunk) override final { @@ -214,6 +222,7 @@ private: Memory<> segment; size_t actual_memory_size{0}; Statistics statistics; + size_t rows_num; }; Poco::Event collector_finished{}; @@ -241,12 +250,19 @@ private: std::condition_variable writer_condvar; size_t rows_consumed = 0; + size_t rows_collected = 0; std::atomic_bool are_totals_written = false; /// We change statistics in onProgress() which can be called from different threads. std::mutex statistics_mutex; bool save_totals_and_extremes_in_statistics; + String exception_message; + bool exception_is_rethrown = false; + bool collected_prefix = false; + bool collected_suffix = false; + bool collected_finalize = false; + void finishAndWait(); void onBackgroundException() @@ -261,6 +277,17 @@ private: collector_condvar.notify_all(); } + void rethrowBackgroundException() + { + /// Rethrow background exception only once, because + /// OutputFormat can be used after it to write an exception. + if (!exception_is_rethrown) + { + exception_is_rethrown = true; + std::rethrow_exception(background_exception); + } + } + void scheduleFormatterThreadForUnitWithNumber(size_t ticket_number, size_t first_row_num) { pool.scheduleOrThrowOnError([this, thread_group = CurrentThread::getGroup(), ticket_number, first_row_num] diff --git a/src/Processors/Formats/Impl/XMLRowOutputFormat.cpp b/src/Processors/Formats/Impl/XMLRowOutputFormat.cpp index 1d6fb62275c..eb735cc93aa 100644 --- a/src/Processors/Formats/Impl/XMLRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/XMLRowOutputFormat.cpp @@ -8,8 +8,9 @@ namespace DB { XMLRowOutputFormat::XMLRowOutputFormat(WriteBuffer & out_, const Block & header_, const FormatSettings & format_settings_) - : RowOutputFormatWithUTF8ValidationAdaptor(true, header_, out_), fields(header_.getNamesAndTypes()), format_settings(format_settings_) + : RowOutputFormatWithExceptionHandlerAdaptor(header_, out_, true, format_settings_.xml.valid_output_on_exception), fields(header_.getNamesAndTypes()), format_settings(format_settings_) { + ostr = RowOutputFormatWithExceptionHandlerAdaptor::getWriteBufferPtr(); const auto & sample = getPort(PortKind::Main).getHeader(); field_tag_names.resize(sample.columns()); @@ -191,7 +192,9 @@ void XMLRowOutputFormat::finalizeImpl() writeRowsBeforeLimitAtLeast(); - if (format_settings.write_statistics) + if (!exception_message.empty()) + writeException(); + else if (format_settings.write_statistics) writeStatistics(); writeCString("\n", *ostr); @@ -230,6 +233,12 @@ void XMLRowOutputFormat::writeStatistics() writeCString("\t\n", *ostr); } +void XMLRowOutputFormat::writeException() +{ + writeCString("\t", *ostr); + writeXMLStringForTextElement(exception_message, *ostr); + writeCString("\n", *ostr); +} void registerOutputFormatXML(FormatFactory & factory) { diff --git a/src/Processors/Formats/Impl/XMLRowOutputFormat.h b/src/Processors/Formats/Impl/XMLRowOutputFormat.h index e25e7129109..daf03539d0b 100644 --- a/src/Processors/Formats/Impl/XMLRowOutputFormat.h +++ b/src/Processors/Formats/Impl/XMLRowOutputFormat.h @@ -6,6 +6,7 @@ #include #include #include +#include namespace DB @@ -13,7 +14,7 @@ namespace DB /** A stream for outputting data in XML format. */ -class XMLRowOutputFormat final : public RowOutputFormatWithUTF8ValidationAdaptor +class XMLRowOutputFormat final : public RowOutputFormatWithExceptionHandlerAdaptor { public: XMLRowOutputFormat(WriteBuffer & out_, const Block & header_, const FormatSettings & format_settings_); @@ -56,6 +57,7 @@ private: void writeExtremesElement(const char * title, const Columns & columns, size_t row_num); void writeRowsBeforeLimitAtLeast(); void writeStatistics(); + void writeException(); size_t field_number = 0; size_t row_count = 0; @@ -63,6 +65,7 @@ private: Names field_tag_names; const FormatSettings format_settings; + WriteBuffer * ostr; }; } diff --git a/src/Processors/Formats/OutputFormatWithExceptionHandlerAdaptor.h b/src/Processors/Formats/OutputFormatWithExceptionHandlerAdaptor.h new file mode 100644 index 00000000000..bb318dae81e --- /dev/null +++ b/src/Processors/Formats/OutputFormatWithExceptionHandlerAdaptor.h @@ -0,0 +1,75 @@ +#pragma once + +#include +#include + +#include +#include + +namespace DB +{ + +template +class RowOutputFormatWithExceptionHandlerAdaptorBase : public Base +{ +public: + RowOutputFormatWithExceptionHandlerAdaptorBase(bool handle_exceptions, const Block & header, WriteBuffer & out_, Args... args) + : Base(header, out_, std::forward(args)...) + { + if (handle_exceptions) + peekable_out = std::make_unique(*Base::getWriteBufferPtr()); + } + + void write(const Columns & columns, size_t row_num) + { + if (!peekable_out) + Base::write(columns, row_num); + + + PeekableWriteBufferCheckpoint checkpoint(*peekable_out); + try + { + Base::write(columns, row_num); + } + catch (...) + { + peekable_out->rollbackToCheckpoint(); + throw; + } + } + + void flush() override + { + getWriteBufferPtr()->next(); + + if (peekable_out) + Base::getWriteBufferPtr()->next(); + } + + void finalizeBuffers() override + { + if (peekable_out) + peekable_out->finalize(); + } + + void resetFormatterImpl() override + { + peekable_out = std::make_unique(*Base::getWriteBufferPtr()); + } + +protected: + /// Returns buffer that should be used in derived classes instead of out. + WriteBuffer * getWriteBufferPtr() override + { + if (peekable_out) + peekable_out.get(); + return Base::getWriteBufferPtr(); + } + +private: + + std::unique_ptr peekable_out; +}; + +} + diff --git a/src/Processors/Formats/OutputFormatWithUTF8ValidationAdaptor.h b/src/Processors/Formats/OutputFormatWithUTF8ValidationAdaptor.h index 8d8fb9ef0c6..f86ff278b33 100644 --- a/src/Processors/Formats/OutputFormatWithUTF8ValidationAdaptor.h +++ b/src/Processors/Formats/OutputFormatWithUTF8ValidationAdaptor.h @@ -9,12 +9,12 @@ namespace DB { -template +template class OutputFormatWithUTF8ValidationAdaptorBase : public Base { public: - OutputFormatWithUTF8ValidationAdaptorBase(bool validate_utf8, const Block & header, WriteBuffer & out_, Args... args) - : Base(header, out_, std::forward(args)...) + OutputFormatWithUTF8ValidationAdaptorBase(const Block & header, WriteBuffer & out_, bool validate_utf8) + : Base(header, out_) { bool values_can_contain_invalid_utf8 = false; for (const auto & type : this->getPort(IOutputFormat::PortKind::Main).getHeader().getDataTypes()) @@ -24,37 +24,37 @@ public: } if (validate_utf8 && values_can_contain_invalid_utf8) - { - validating_ostr = std::make_unique(this->out); - ostr = validating_ostr.get(); - } - else - ostr = &this->out; + validating_ostr = std::make_unique(*Base::getWriteBufferPtr()); } void flush() override { - ostr->next(); - if (validating_ostr) - this->out.next(); + validating_ostr->next(); + Base::flush(); } void finalizeBuffers() override { if (validating_ostr) validating_ostr->finalize(); + Base::finalizeBuffers(); } void resetFormatterImpl() override { - validating_ostr = std::make_unique(this->out); - ostr = validating_ostr.get(); + Base::resetFormatterImpl(); + validating_ostr = std::make_unique(*Base::getWriteBufferPtr()); } protected: - /// Point to validating_ostr or out from IOutputFormat, should be used in derived classes instead of out. - WriteBuffer * ostr; + /// Returns buffer that should be used in derived classes instead of out. + WriteBuffer * getWriteBufferPtr() override + { + if (validating_ostr) + return validating_ostr.get(); + return Base::getWriteBufferPtr(); + } private: /// Validates UTF-8 sequences, replaces bad sequences with replacement character. diff --git a/src/Processors/Formats/RowOutputFormatWithExceptionHandlerAdaptor.h b/src/Processors/Formats/RowOutputFormatWithExceptionHandlerAdaptor.h new file mode 100644 index 00000000000..4e797c521c0 --- /dev/null +++ b/src/Processors/Formats/RowOutputFormatWithExceptionHandlerAdaptor.h @@ -0,0 +1,104 @@ +#pragma once + +#include +#include +#include + +#include +#include +#include + +namespace DB +{ + +template +class RowOutputFormatWithExceptionHandlerAdaptor : public Base +{ +public: + RowOutputFormatWithExceptionHandlerAdaptor(const Block & header, WriteBuffer & out_, bool handle_exceptions, Args... args) + : Base(header, out_, std::forward(args)...) + { + if (handle_exceptions) + peekable_out = std::make_unique(*Base::getWriteBufferPtr()); + } + + void consume(DB::Chunk chunk) override + { + if (!peekable_out) + { + Base::consume(std::move(chunk)); + return; + } + + auto num_rows = chunk.getNumRows(); + const auto & columns = chunk.getColumns(); + + for (size_t row = 0; row < num_rows; ++row) + { + /// It's important to set a checkpoint before writing row-between delimiter + peekable_out->setCheckpoint(); + + if (Base::haveWrittenData()) + writeRowBetweenDelimiter(); + + try + { + write(columns, row); + } + catch (...) + { + peekable_out->rollbackToCheckpoint(/*drop=*/true); + throw; + } + peekable_out->dropCheckpoint(); + + Base::first_row = false; + } + } + + void write(const Columns & columns, size_t row_num) override { Base::write(columns, row_num); } + void writeRowBetweenDelimiter() override { Base::writeRowBetweenDelimiter(); } + + void flush() override + { + if (peekable_out) + peekable_out->next(); + + Base::flush(); + } + + void finalizeBuffers() override + { + if (peekable_out) + peekable_out->finalize(); + Base::finalizeBuffers(); + } + + void resetFormatterImpl() override + { + Base::resetFormatterImpl(); + peekable_out = std::make_unique(*Base::getWriteBufferPtr()); + } + + bool supportsWritingException() const override { return true; } + + void setException(const String & exception_message_) override { exception_message = exception_message_; } + +protected: + /// Returns buffer that should be used in derived classes instead of out. + WriteBuffer * getWriteBufferPtr() override + { + if (peekable_out) + return peekable_out.get(); + return Base::getWriteBufferPtr(); + } + + String exception_message; + +private: + + std::unique_ptr peekable_out; +}; + +} + diff --git a/src/Server/HTTPHandler.cpp b/src/Server/HTTPHandler.cpp index a0bfcd49dfd..a5102ea9383 100644 --- a/src/Server/HTTPHandler.cpp +++ b/src/Server/HTTPHandler.cpp @@ -28,6 +28,8 @@ #include #include #include +#include +#include #include #include @@ -835,23 +837,40 @@ void HTTPHandler::processQuery( customizeContext(request, context, *in_post_maybe_compressed); in = has_external_data ? std::move(in_param) : std::make_unique(*in_param, *in_post_maybe_compressed); - executeQuery(*in, *used_output.out_maybe_delayed_and_compressed, /* allow_into_outfile = */ false, context, - [&response, this] (const QueryResultDetails & details) + auto set_query_result = [&response, this] (const QueryResultDetails & details) + { + response.add("X-ClickHouse-Query-Id", details.query_id); + + if (content_type_override) + response.setContentType(*content_type_override); + else if (details.content_type) + response.setContentType(*details.content_type); + + if (details.format) + response.add("X-ClickHouse-Format", *details.format); + + if (details.timezone) + response.add("X-ClickHouse-Timezone", *details.timezone); + }; + + auto handle_exception_in_output_format = [&](IOutputFormat & output_format) + { + if (settings.http_write_exception_in_output_format && output_format.supportsWritingException()) { - response.add("X-ClickHouse-Query-Id", details.query_id); - - if (content_type_override) - response.setContentType(*content_type_override); - else if (details.content_type) - response.setContentType(*details.content_type); - - if (details.format) - response.add("X-ClickHouse-Format", *details.format); - - if (details.timezone) - response.add("X-ClickHouse-Timezone", *details.timezone); + output_format.setException(getCurrentExceptionMessage(false)); + output_format.finalize(); + used_output.exception_is_written = true; } - ); + }; + + executeQuery( + *in, + *used_output.out_maybe_delayed_and_compressed, + /* allow_into_outfile = */ false, + context, + set_query_result, + {}, + handle_exception_in_output_format); if (used_output.hasDelayed()) { @@ -895,7 +914,7 @@ try response.setStatusAndReason(exceptionCodeToHTTPStatus(exception_code)); } - if (!response.sent() && !used_output.out_maybe_compressed) + if (!response.sent() && !used_output.out_maybe_compressed && !used_output.exception_is_written) { /// If nothing was sent yet and we don't even know if we must compress the response. *response.send() << s << std::endl; @@ -911,21 +930,24 @@ try used_output.out_maybe_delayed_and_compressed.reset(); } - /// Send the error message into already used (and possibly compressed) stream. - /// Note that the error message will possibly be sent after some data. - /// Also HTTP code 200 could have already been sent. - - /// If buffer has data, and that data wasn't sent yet, then no need to send that data - bool data_sent = used_output.out->count() != used_output.out->offset(); - - if (!data_sent) + if (!used_output.exception_is_written) { - used_output.out_maybe_compressed->position() = used_output.out_maybe_compressed->buffer().begin(); - used_output.out->position() = used_output.out->buffer().begin(); - } + /// Send the error message into already used (and possibly compressed) stream. + /// Note that the error message will possibly be sent after some data. + /// Also HTTP code 200 could have already been sent. - writeString(s, *used_output.out_maybe_compressed); - writeChar('\n', *used_output.out_maybe_compressed); + /// If buffer has data, and that data wasn't sent yet, then no need to send that data + bool data_sent = used_output.out->count() != used_output.out->offset(); + + if (!data_sent) + { + used_output.out_maybe_compressed->position() = used_output.out_maybe_compressed->buffer().begin(); + used_output.out->position() = used_output.out->buffer().begin(); + } + + writeString(s, *used_output.out_maybe_compressed); + writeChar('\n', *used_output.out_maybe_compressed); + } used_output.out_maybe_compressed->next(); } diff --git a/src/Server/HTTPHandler.h b/src/Server/HTTPHandler.h index 5eda5927538..94b5a44f105 100644 --- a/src/Server/HTTPHandler.h +++ b/src/Server/HTTPHandler.h @@ -62,6 +62,8 @@ private: bool finalized = false; + bool exception_is_written = false; + inline bool hasDelayed() const { return out_maybe_delayed_and_compressed != out_maybe_compressed; diff --git a/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.reference b/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.reference new file mode 100644 index 00000000000..452aa9d5022 --- /dev/null +++ b/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.reference @@ -0,0 +1,432 @@ +One block +Parallel formatting: 0 +JSON +{ + "meta": + [ + { + "name": "number", + "type": "UInt64" + }, + { + "name": "res", + "type": "UInt8" + } + ], + + "data": + [ + + ], + + "rows": 0, + + "exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) " +} +JSONEachRow +{"exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) "} +JSONCompact +{ + "meta": + [ + { + "name": "number", + "type": "UInt64" + }, + { + "name": "res", + "type": "UInt8" + } + ], + + "data": + [ + + ], + + "rows": 0, + + "exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) " +} +JSONCompactEachRow +["Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) "] +JSONObjectEachRow +{ + "exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) " +} +XML + + + + + + number + UInt64 + + + res + UInt8 + + + + + + 0 + Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) + +Parallel formatting: 1 +JSON +{ + "meta": + [ + { + "name": "number", + "type": "UInt64" + }, + { + "name": "res", + "type": "UInt8" + } + ], + + "data": + [ + + ], + + "rows": 0, + + "exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) " +} +JSONEachRow +{"exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) "} +JSONCompact +{ + "meta": + [ + { + "name": "number", + "type": "UInt64" + }, + { + "name": "res", + "type": "UInt8" + } + ], + + "data": + [ + + ], + + "rows": 0, + + "exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) " +} +JSONCompactEachRow +["Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) "] +JSONObjectEachRow +{ + "exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) " +} +XML + + + + + + number + UInt64 + + + res + UInt8 + + + + + + 0 + Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) + +Several blocks +Without parallel formatting +JSON +{ + "meta": + [ + { + "name": "number", + "type": "UInt64" + }, + { + "name": "res", + "type": "UInt8" + } + ], + + "data": + [ + { + "number": "0", + "res": 0 + }, + { + "number": "1", + "res": 0 + }, + { + "number": "2", + "res": 0 + }, + { + "number": "3", + "res": 0 + } + ], + + "rows": 4, + + "exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) " +} +JSONEachRow +{"number":"0","res":0} +{"number":"1","res":0} +{"number":"2","res":0} +{"number":"3","res":0} +{"exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) "} +JSONCompact +{ + "meta": + [ + { + "name": "number", + "type": "UInt64" + }, + { + "name": "res", + "type": "UInt8" + } + ], + + "data": + [ + ["0", 0], + ["1", 0], + ["2", 0], + ["3", 0] + ], + + "rows": 4, + + "exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) " +} +JSONCompactEachRow +["0", 0] +["1", 0] +["2", 0] +["3", 0] +["Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) "] +JSONObjectEachRow +{ + "row_1": {"number":"0","res":0}, + "row_1": {"number":"1","res":0}, + "row_1": {"number":"2","res":0}, + "row_1": {"number":"3","res":0}, + "exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) " +} +XML + + + + + + number + UInt64 + + + res + UInt8 + + + + + + 0 + 0 + + + 1 + 0 + + + 2 + 0 + + + 3 + 0 + + + 4 + Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) + +With parallel formatting +JSON +1 +JSONCompact +1 +JSONObjectEachRow +1 +JSONEachRow +1 +JSONCompactEachRow +1 +Formatting error +Without parallel formatting +JSON +{ + "meta": + [ + { + "name": "x", + "type": "UInt32" + }, + { + "name": "s", + "type": "String" + }, + { + "name": "y", + "type": "Enum8('a' = 1)" + } + ], + + "data": + [ + { + "x": 1, + "s": "str1", + "y": "a" + }, + { + "x": 2, + "s": "str2", + "y": "a" + }, + { + "x": 3, + "s": "str3", + "y": "a" + } + ], + + "rows": 3, + + "exception": "Code: 36. : Unexpected value 99 in enum: While executing JSONRowOutputFormat. (BAD_ARGUMENTS) " +} +JSONEachRow +{"x":1,"s":"str1","y":"a"} +{"x":2,"s":"str2","y":"a"} +{"x":3,"s":"str3","y":"a"} +{"exception": "Code: 36. : Unexpected value 99 in enum: While executing JSONEachRowRowOutputFormat. (BAD_ARGUMENTS) "} +JSONCompact +{ + "meta": + [ + { + "name": "x", + "type": "UInt32" + }, + { + "name": "s", + "type": "String" + }, + { + "name": "y", + "type": "Enum8('a' = 1)" + } + ], + + "data": + [ + [1, "str1", "a"], + [2, "str2", "a"], + [3, "str3", "a"] + ], + + "rows": 3, + + "exception": "Code: 36. : Unexpected value 99 in enum: While executing JSONCompactRowOutputFormat. (BAD_ARGUMENTS) " +} +JSONCompactEachRow +[1, "str1", "a"] +[2, "str2", "a"] +[3, "str3", "a"] +["Code: 36. : Unexpected value 99 in enum: While executing JSONCompactEachRowRowOutputFormat. (BAD_ARGUMENTS) "] +JSONObjectEachRow +{ + "row_1": {"x":1,"s":"str1","y":"a"}, + "row_1": {"x":2,"s":"str2","y":"a"}, + "row_1": {"x":3,"s":"str3","y":"a"}, + "exception": "Code: 36. : Unexpected value 99 in enum: While executing JSONObjectEachRowRowOutputFormat. (BAD_ARGUMENTS) " +} +XML + + + + + + x + UInt32 + + + s + String + + + y + Enum8('a' = 1) + + + + + + 1 + str1 + a + + + 2 + str2 + a + + + 3 + str3 + a + + + 3 + Code: 36. : Unexpected value 99 in enum: While executing XMLRowOutputFormat. (BAD_ARGUMENTS) + +With parallel formatting +JSON +1 +JSONCompact +1 +JSONObjectEachRow +1 +JSONEachRow +1 +JSONCompactEachRow +1 +Test 1 +1 +1 +Test 2 +1 +1 +Test 3 +1 +1 diff --git a/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh b/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh new file mode 100755 index 00000000000..cb4d1b6aee1 --- /dev/null +++ b/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh @@ -0,0 +1,106 @@ +#!/usr/bin/env bash + +CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CUR_DIR"/../shell_config.sh + +echo "One block" +for parallel in 0 1 +do + echo "Parallel formatting: $parallel" + for format in JSON JSONEachRow JSONCompact JSONCompactEachRow JSONObjectEachRow XML + do + echo $format + ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select number, throwIf(number > 3) as res from numbers(10) format $format settings output_format_parallel_formatting=$parallel" | sed "s/(version .*)//" | sed "s/DB::Exception//" + done +done + +echo "Several blocks" +echo "Without parallel formatting" +for format in JSON JSONEachRow JSONCompact JSONCompactEachRow JSONObjectEachRow XML +do + echo $format + ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select number, throwIf(number > 3) as res from system.numbers format $format settings max_block_size=1, output_format_parallel_formatting=0" | sed "s/(version .*)//" | sed "s/DB::Exception//" +done + +echo "With parallel formatting" +for format in JSON JSONCompact JSONObjectEachRow +do + echo $format + ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select number, throwIf(number > 3) as res from system.numbers format $format settings max_block_size=1, output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +done + +for format in JSONEachRow JSONCompactEachRow +do + echo $format + ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select number, throwIf(number > 3) as res from system.numbers format $format settings max_block_size=1, output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=LineAsString -q "select min(isValidJSON(line)) from table" +done + +echo "Formatting error" +$CLICKHOUSE_CLIENT -q "drop table if exists test_02841" +$CLICKHOUSE_CLIENT -q "create table test_02841 (x UInt32, s String, y Enum('a' = 1)) engine=MergeTree order by x" +$CLICKHOUSE_CLIENT -q "system stop merges test_02841" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (1, 'str1', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (2, 'str2', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (3, 'str3', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (5, 'str5', 99)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (6, 'str6', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (7, 'str7', 1)" + +echo "Without parallel formatting" +for format in JSON JSONEachRow JSONCompact JSONCompactEachRow JSONObjectEachRow XML +do + echo $format + ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 order by x format $format settings output_format_parallel_formatting=0" | sed "s/(version .*)//" | sed "s/DB::Exception//" +done + +echo "With parallel formatting" +for format in JSON JSONCompact JSONObjectEachRow +do + echo $format + ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format $format settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +done + +for format in JSONEachRow JSONCompactEachRow +do + echo $format + ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format $format settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=LineAsString -q "select min(isValidJSON(line)) from table" +done + + +echo "Test 1" +$CLICKHOUSE_CLIENT -q "truncate table test_02841" +$CLICKHOUSE_CLIENT -q "insert into test_02841 select 1, repeat('aaaaa', 1000000), 1" +$CLICKHOUSE_CLIENT -q "insert into test_02841 select 2, repeat('aaaaa', 1000000), 99" +$CLICKHOUSE_CLIENT -q "insert into test_02841 select 3, repeat('aaaaa', 1000000), 1" + +${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=0" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" + + +echo "Test 2" +$CLICKHOUSE_CLIENT -q "truncate table test_02841" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (1, 'str1', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (2, 'str2', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 select number, 'str_numbers_1', 1 from numbers(10000)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (3, 'str4', 99)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (4, 'str5', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 select number, 'str_numbers_2', 1 from numbers(10000)" + +${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=0" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" + +echo "Test 3" +$CLICKHOUSE_CLIENT -q "truncate table test_02841" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (1, 'str1', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (2, 'str2', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 select number, 'str_numbers_1', number > 9000 ? 99 : 1 from numbers(10000)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (3, 'str4', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 values (4, 'str5', 1)" +$CLICKHOUSE_CLIENT -q "insert into test_02841 select number, 'str_numbers_2', 1 from numbers(10000)" + +${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=0" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" + +$CLICKHOUSE_CLIENT -q "drop table test_02841" + From fa905ebd27f56c4dcf3b4550963d7f4cab94c2e5 Mon Sep 17 00:00:00 2001 From: avogar Date: Tue, 1 Aug 2023 10:14:09 +0000 Subject: [PATCH 020/101] Clean up --- src/Formats/FormatFactory.cpp | 1 - .../OutputFormatWithExceptionHandlerAdaptor.h | 75 ------------------- 2 files changed, 76 deletions(-) delete mode 100644 src/Processors/Formats/OutputFormatWithExceptionHandlerAdaptor.h diff --git a/src/Formats/FormatFactory.cpp b/src/Formats/FormatFactory.cpp index 4cd2ad5be03..1ad2c2285d9 100644 --- a/src/Formats/FormatFactory.cpp +++ b/src/Formats/FormatFactory.cpp @@ -1,7 +1,6 @@ #include #include -//#include #include #include #include diff --git a/src/Processors/Formats/OutputFormatWithExceptionHandlerAdaptor.h b/src/Processors/Formats/OutputFormatWithExceptionHandlerAdaptor.h deleted file mode 100644 index bb318dae81e..00000000000 --- a/src/Processors/Formats/OutputFormatWithExceptionHandlerAdaptor.h +++ /dev/null @@ -1,75 +0,0 @@ -#pragma once - -#include -#include - -#include -#include - -namespace DB -{ - -template -class RowOutputFormatWithExceptionHandlerAdaptorBase : public Base -{ -public: - RowOutputFormatWithExceptionHandlerAdaptorBase(bool handle_exceptions, const Block & header, WriteBuffer & out_, Args... args) - : Base(header, out_, std::forward(args)...) - { - if (handle_exceptions) - peekable_out = std::make_unique(*Base::getWriteBufferPtr()); - } - - void write(const Columns & columns, size_t row_num) - { - if (!peekable_out) - Base::write(columns, row_num); - - - PeekableWriteBufferCheckpoint checkpoint(*peekable_out); - try - { - Base::write(columns, row_num); - } - catch (...) - { - peekable_out->rollbackToCheckpoint(); - throw; - } - } - - void flush() override - { - getWriteBufferPtr()->next(); - - if (peekable_out) - Base::getWriteBufferPtr()->next(); - } - - void finalizeBuffers() override - { - if (peekable_out) - peekable_out->finalize(); - } - - void resetFormatterImpl() override - { - peekable_out = std::make_unique(*Base::getWriteBufferPtr()); - } - -protected: - /// Returns buffer that should be used in derived classes instead of out. - WriteBuffer * getWriteBufferPtr() override - { - if (peekable_out) - peekable_out.get(); - return Base::getWriteBufferPtr(); - } - -private: - - std::unique_ptr peekable_out; -}; - -} - From 2adb25e5caacc87182ea5aa7c8a431b5867d3180 Mon Sep 17 00:00:00 2001 From: avogar Date: Tue, 1 Aug 2023 10:21:32 +0000 Subject: [PATCH 021/101] Add examples in docs --- docs/en/interfaces/http.md | 68 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/docs/en/interfaces/http.md b/docs/en/interfaces/http.md index b28180fec67..a66b4ff5d5d 100644 --- a/docs/en/interfaces/http.md +++ b/docs/en/interfaces/http.md @@ -707,6 +707,74 @@ To prevent it, you can use setting `http_write_exception_in_output_format` (enab Examples: ```bash +$ curl 'http://localhost:8123/?query=SELECT+number,+throwIf(number>3)+from+system.numbers+format+JSON+settings+max_block_size=1&http_write_exception_in_output_format=1' +{ + "meta": + [ + { + "name": "number", + "type": "UInt64" + }, + { + "name": "throwIf(greater(number, 2))", + "type": "UInt8" + } + ], + "data": + [ + { + "number": "0", + "throwIf(greater(number, 2))": 0 + }, + { + "number": "1", + "throwIf(greater(number, 2))": 0 + }, + { + "number": "2", + "throwIf(greater(number, 2))": 0 + } + ], + + "rows": 3, + + "exception": "Code: 395. DB::Exception: Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 2) :: 2) -> throwIf(greater(number, 2)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) (version 23.8.1.1)" +} +``` + +```bash +$ curl 'http://localhost:8123/?query=SELECT+number,+throwIf(number>2)+from+system.numbers+format+XML+settings+max_block_size=1&http_write_exception_in_output_format=1' + + + + + + number + UInt64 + + + throwIf(greater(number, 2)) + UInt8 + + + + + + 0 + 0 + + + 1 + 0 + + + 2 + 0 + + + 3 + Code: 395. DB::Exception: Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 2) :: 2) -> throwIf(greater(number, 2)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) (version 23.8.1.1) + ``` From d12e96177a390464349d71888aef3d1b19243c2d Mon Sep 17 00:00:00 2001 From: avogar Date: Tue, 1 Aug 2023 16:17:03 +0000 Subject: [PATCH 022/101] Fix tests --- .../OptimizeDateOrDateTimeConverterWithPreimagePass.cpp | 2 +- src/IO/PeekableWriteBuffer.cpp | 2 +- .../Formats/Impl/JSONCompactEachRowRowOutputFormat.cpp | 6 ++++++ .../Formats/Impl/JSONCompactEachRowRowOutputFormat.h | 2 ++ src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.cpp | 5 +++++ src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.h | 2 ++ .../Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp | 2 +- src/Processors/Formats/Impl/JSONRowOutputFormat.cpp | 1 + .../Formats/Impl/ParallelFormattingOutputFormat.cpp | 4 ++-- .../Formats/Impl/ParallelFormattingOutputFormat.h | 4 ++++ src/Processors/Formats/Impl/XMLRowOutputFormat.cpp | 5 +++-- .../Formats/OutputFormatWithUTF8ValidationAdaptor.h | 6 +++++- .../Formats/RowOutputFormatWithExceptionHandlerAdaptor.h | 4 +++- 13 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/Analyzer/Passes/OptimizeDateOrDateTimeConverterWithPreimagePass.cpp b/src/Analyzer/Passes/OptimizeDateOrDateTimeConverterWithPreimagePass.cpp index 7205ac299a9..774c07ef1d4 100644 --- a/src/Analyzer/Passes/OptimizeDateOrDateTimeConverterWithPreimagePass.cpp +++ b/src/Analyzer/Passes/OptimizeDateOrDateTimeConverterWithPreimagePass.cpp @@ -48,7 +48,7 @@ public: return true; } - void visitImpl(QueryTreeNodePtr & node) const + void enterImpl(QueryTreeNodePtr & node) const { const static std::unordered_map swap_relations = { {"equals", "equals"}, diff --git a/src/IO/PeekableWriteBuffer.cpp b/src/IO/PeekableWriteBuffer.cpp index dc7f87dd539..87c7291c377 100644 --- a/src/IO/PeekableWriteBuffer.cpp +++ b/src/IO/PeekableWriteBuffer.cpp @@ -6,7 +6,7 @@ namespace DB PeekableWriteBuffer::PeekableWriteBuffer(DB::WriteBuffer & sub_buf_) : BufferWithOwnMemory(0), sub_buf(sub_buf_) { Buffer & sub_working = sub_buf.buffer(); - BufferBase::set(sub_working.begin(), sub_working.size(), sub_buf.offset()); + BufferBase::set(sub_working.begin() + sub_buf.offset(), sub_working.size() - sub_buf.offset(), 0); } void PeekableWriteBuffer::nextImpl() diff --git a/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.cpp b/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.cpp index c5c9af60982..530d09d5c87 100644 --- a/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.cpp @@ -116,6 +116,12 @@ void JSONCompactEachRowRowOutputFormat::writeSuffix() } } +void JSONCompactEachRowRowOutputFormat::resetFormatterImpl() +{ + RowOutputFormatWithExceptionHandlerAdaptor::resetFormatterImpl(); + ostr = RowOutputFormatWithExceptionHandlerAdaptor::getWriteBufferPtr(); +} + void registerOutputFormatJSONCompactEachRow(FormatFactory & factory) { for (bool yield_strings : {false, true}) diff --git a/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.h b/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.h index a05fff699a5..bd32592a4a0 100644 --- a/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.h +++ b/src/Processors/Formats/Impl/JSONCompactEachRowRowOutputFormat.h @@ -36,6 +36,8 @@ private: void writeRowEndDelimiter() override; void writeSuffix() override; + void resetFormatterImpl() override; + bool supportTotals() const override { return true; } void consumeTotals(Chunk) override; diff --git a/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.cpp b/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.cpp index 2169d815fbf..a7118c2154a 100644 --- a/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.cpp @@ -92,6 +92,11 @@ void JSONEachRowRowOutputFormat::writeSuffix() writeCString("\n]\n", *ostr); } +void JSONEachRowRowOutputFormat::resetFormatterImpl() +{ + RowOutputFormatWithExceptionHandlerAdaptor::resetFormatterImpl(); + ostr = RowOutputFormatWithExceptionHandlerAdaptor::getWriteBufferPtr(); +} void registerOutputFormatJSONEachRow(FormatFactory & factory) { diff --git a/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.h b/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.h index 28bfbf2e6ac..2de9369846b 100644 --- a/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.h +++ b/src/Processors/Formats/Impl/JSONEachRowRowOutputFormat.h @@ -39,6 +39,8 @@ protected: void writePrefix() override; void writeSuffix() override; + void resetFormatterImpl() override; + size_t field_number = 0; bool pretty_json; diff --git a/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp b/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp index 8f4d11a604a..26aa0aad97c 100644 --- a/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp @@ -68,7 +68,7 @@ void JSONObjectEachRowRowOutputFormat::writeSuffix() writeRowBetweenDelimiter(); JSONUtils::writeException(exception_message, *ostr, settings, 1); } - + JSONUtils::writeObjectEnd(*ostr); writeChar('\n', *ostr); } diff --git a/src/Processors/Formats/Impl/JSONRowOutputFormat.cpp b/src/Processors/Formats/Impl/JSONRowOutputFormat.cpp index e4c4e2a3bc6..20182d84917 100644 --- a/src/Processors/Formats/Impl/JSONRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONRowOutputFormat.cpp @@ -135,6 +135,7 @@ void JSONRowOutputFormat::finalizeImpl() void JSONRowOutputFormat::resetFormatterImpl() { RowOutputFormatWithExceptionHandlerAdaptor::resetFormatterImpl(); + ostr = RowOutputFormatWithExceptionHandlerAdaptor::getWriteBufferPtr(); row_count = 0; statistics = Statistics(); } diff --git a/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.cpp b/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.cpp index 3e63e2abd6c..841ef683228 100644 --- a/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.cpp +++ b/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.cpp @@ -36,10 +36,10 @@ namespace DB formatter->setRowsReadBefore(rows_collected); formatter->setException(exception_message); - if (!collected_prefix) + if (!collected_prefix && (need_write_prefix || started_prefix)) formatter->writePrefix(); - if (!collected_suffix) + if (!collected_suffix && (need_write_suffix || started_suffix)) formatter->writeSuffix(); if (!collected_finalize) diff --git a/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.h b/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.h index b9a3b7638fa..bf8968dd376 100644 --- a/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.h +++ b/src/Processors/Formats/Impl/ParallelFormattingOutputFormat.h @@ -118,6 +118,7 @@ public: void writePrefix() override { addChunk(Chunk{}, ProcessingUnitType::START, /*can_throw_exception*/ true); + started_prefix = true; } void onCancel() override @@ -134,6 +135,7 @@ public: void writeSuffix() override { addChunk(Chunk{}, ProcessingUnitType::PLAIN_FINISH, /*can_throw_exception*/ true); + started_suffix = true; } String getContentType() const override @@ -259,7 +261,9 @@ private: String exception_message; bool exception_is_rethrown = false; + bool started_prefix = false; bool collected_prefix = false; + bool started_suffix = false; bool collected_suffix = false; bool collected_finalize = false; diff --git a/src/Processors/Formats/Impl/XMLRowOutputFormat.cpp b/src/Processors/Formats/Impl/XMLRowOutputFormat.cpp index eb735cc93aa..52c161c3208 100644 --- a/src/Processors/Formats/Impl/XMLRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/XMLRowOutputFormat.cpp @@ -8,7 +8,7 @@ namespace DB { XMLRowOutputFormat::XMLRowOutputFormat(WriteBuffer & out_, const Block & header_, const FormatSettings & format_settings_) - : RowOutputFormatWithExceptionHandlerAdaptor(header_, out_, true, format_settings_.xml.valid_output_on_exception), fields(header_.getNamesAndTypes()), format_settings(format_settings_) + : RowOutputFormatWithExceptionHandlerAdaptor(header_, out_, format_settings_.xml.valid_output_on_exception, true), fields(header_.getNamesAndTypes()), format_settings(format_settings_) { ostr = RowOutputFormatWithExceptionHandlerAdaptor::getWriteBufferPtr(); const auto & sample = getPort(PortKind::Main).getHeader(); @@ -203,7 +203,8 @@ void XMLRowOutputFormat::finalizeImpl() void XMLRowOutputFormat::resetFormatterImpl() { - RowOutputFormatWithUTF8ValidationAdaptor::resetFormatterImpl(); + RowOutputFormatWithExceptionHandlerAdaptor::resetFormatterImpl(); + ostr = RowOutputFormatWithExceptionHandlerAdaptor::getWriteBufferPtr(); row_count = 0; statistics = Statistics(); } diff --git a/src/Processors/Formats/OutputFormatWithUTF8ValidationAdaptor.h b/src/Processors/Formats/OutputFormatWithUTF8ValidationAdaptor.h index f86ff278b33..4c5c3ef72e9 100644 --- a/src/Processors/Formats/OutputFormatWithUTF8ValidationAdaptor.h +++ b/src/Processors/Formats/OutputFormatWithUTF8ValidationAdaptor.h @@ -6,6 +6,8 @@ #include #include +#include + namespace DB { @@ -43,8 +45,10 @@ public: void resetFormatterImpl() override { + LOG_DEBUG(&Poco::Logger::get("RowOutputFormatWithExceptionHandlerAdaptor"), "resetFormatterImpl"); Base::resetFormatterImpl(); - validating_ostr = std::make_unique(*Base::getWriteBufferPtr()); + if (validating_ostr) + validating_ostr = std::make_unique(*Base::getWriteBufferPtr()); } protected: diff --git a/src/Processors/Formats/RowOutputFormatWithExceptionHandlerAdaptor.h b/src/Processors/Formats/RowOutputFormatWithExceptionHandlerAdaptor.h index 4e797c521c0..22232e9f654 100644 --- a/src/Processors/Formats/RowOutputFormatWithExceptionHandlerAdaptor.h +++ b/src/Processors/Formats/RowOutputFormatWithExceptionHandlerAdaptor.h @@ -76,8 +76,10 @@ public: void resetFormatterImpl() override { + LOG_DEBUG(&Poco::Logger::get("RowOutputFormatWithExceptionHandlerAdaptor"), "resetFormatterImpl"); Base::resetFormatterImpl(); - peekable_out = std::make_unique(*Base::getWriteBufferPtr()); + if (peekable_out) + peekable_out = std::make_unique(*Base::getWriteBufferPtr()); } bool supportsWritingException() const override { return true; } From 81866bcc9c3f7fcf68ec9cd908a0fa7013dbf980 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Wed, 2 Aug 2023 12:35:58 +0200 Subject: [PATCH 023/101] Fix special build --- .../Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp b/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp index 26aa0aad97c..46b3f56f3cc 100644 --- a/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp @@ -28,7 +28,7 @@ void JSONObjectEachRowRowOutputFormat::write(const Columns & columns, size_t row else object_name = "row_" + std::to_string(row + 1); - IRowOutputFormat::write(columns, row); + RowOutputFormatWithExceptionHandlerAdaptor::write(columns, row); } void JSONObjectEachRowRowOutputFormat::writeFieldDelimiter() From 77cc84a4d2937bea024c1cefbcf17f99ee7ac7c7 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Wed, 2 Aug 2023 12:43:25 +0200 Subject: [PATCH 024/101] Fix test --- ...41_valid_json_and_xml_on_http_exception.sh | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh b/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh index cb4d1b6aee1..60ce7eb3b6f 100755 --- a/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh +++ b/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh @@ -4,6 +4,8 @@ CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) # shellcheck source=../shell_config.sh . "$CUR_DIR"/../shell_config.sh +CH_URL='$CLICKHOUSE_URL&http_write_exception_in_output_format=1&allow_experimental_analyzer=0' + echo "One block" for parallel in 0 1 do @@ -11,7 +13,7 @@ do for format in JSON JSONEachRow JSONCompact JSONCompactEachRow JSONObjectEachRow XML do echo $format - ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select number, throwIf(number > 3) as res from numbers(10) format $format settings output_format_parallel_formatting=$parallel" | sed "s/(version .*)//" | sed "s/DB::Exception//" + ${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select number, throwIf(number > 3) as res from numbers(10) format $format settings output_format_parallel_formatting=$parallel" | sed "s/(version .*)//" | sed "s/DB::Exception//" done done @@ -20,20 +22,20 @@ echo "Without parallel formatting" for format in JSON JSONEachRow JSONCompact JSONCompactEachRow JSONObjectEachRow XML do echo $format - ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select number, throwIf(number > 3) as res from system.numbers format $format settings max_block_size=1, output_format_parallel_formatting=0" | sed "s/(version .*)//" | sed "s/DB::Exception//" + ${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select number, throwIf(number > 3) as res from system.numbers format $format settings max_block_size=1, output_format_parallel_formatting=0" | sed "s/(version .*)//" | sed "s/DB::Exception//" done echo "With parallel formatting" for format in JSON JSONCompact JSONObjectEachRow do echo $format - ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select number, throwIf(number > 3) as res from system.numbers format $format settings max_block_size=1, output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" + ${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select number, throwIf(number > 3) as res from system.numbers format $format settings max_block_size=1, output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" done for format in JSONEachRow JSONCompactEachRow do echo $format - ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select number, throwIf(number > 3) as res from system.numbers format $format settings max_block_size=1, output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=LineAsString -q "select min(isValidJSON(line)) from table" + ${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select number, throwIf(number > 3) as res from system.numbers format $format settings max_block_size=1, output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=LineAsString -q "select min(isValidJSON(line)) from table" done echo "Formatting error" @@ -51,20 +53,20 @@ echo "Without parallel formatting" for format in JSON JSONEachRow JSONCompact JSONCompactEachRow JSONObjectEachRow XML do echo $format - ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 order by x format $format settings output_format_parallel_formatting=0" | sed "s/(version .*)//" | sed "s/DB::Exception//" + ${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select * from test_02841 order by x format $format settings output_format_parallel_formatting=0" | sed "s/(version .*)//" | sed "s/DB::Exception//" done echo "With parallel formatting" for format in JSON JSONCompact JSONObjectEachRow do echo $format - ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format $format settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" + ${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select * from test_02841 format $format settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" done for format in JSONEachRow JSONCompactEachRow do echo $format - ${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format $format settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=LineAsString -q "select min(isValidJSON(line)) from table" + ${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select * from test_02841 format $format settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=LineAsString -q "select min(isValidJSON(line)) from table" done @@ -74,8 +76,8 @@ $CLICKHOUSE_CLIENT -q "insert into test_02841 select 1, repeat('aaaaa', 1000000) $CLICKHOUSE_CLIENT -q "insert into test_02841 select 2, repeat('aaaaa', 1000000), 99" $CLICKHOUSE_CLIENT -q "insert into test_02841 select 3, repeat('aaaaa', 1000000), 1" -${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=0" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" -${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=0" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" echo "Test 2" @@ -87,8 +89,8 @@ $CLICKHOUSE_CLIENT -q "insert into test_02841 values (3, 'str4', 99)" $CLICKHOUSE_CLIENT -q "insert into test_02841 values (4, 'str5', 1)" $CLICKHOUSE_CLIENT -q "insert into test_02841 select number, 'str_numbers_2', 1 from numbers(10000)" -${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=0" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" -${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=0" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" echo "Test 3" $CLICKHOUSE_CLIENT -q "truncate table test_02841" @@ -99,8 +101,8 @@ $CLICKHOUSE_CLIENT -q "insert into test_02841 values (3, 'str4', 1)" $CLICKHOUSE_CLIENT -q "insert into test_02841 values (4, 'str5', 1)" $CLICKHOUSE_CLIENT -q "insert into test_02841 select number, 'str_numbers_2', 1 from numbers(10000)" -${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=0" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" -${CLICKHOUSE_CURL} -sS "$CLICKHOUSE_URL&http_write_exception_in_output_format=1" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=0" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" +${CLICKHOUSE_CURL} -sS "$CH_URL" -d "select * from test_02841 format JSON settings output_format_parallel_formatting=1" | $CLICKHOUSE_LOCAL --input-format=JSONAsString -q "select isValidJSON(json) from table" $CLICKHOUSE_CLIENT -q "drop table test_02841" From d85b16dc71256049269d279bbcc2aab45ff688ee Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Wed, 2 Aug 2023 12:45:26 +0200 Subject: [PATCH 025/101] Fix test reference --- ...2841_valid_json_and_xml_on_http_exception.reference | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.reference b/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.reference index 452aa9d5022..1818ca7b5f2 100644 --- a/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.reference +++ b/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.reference @@ -230,9 +230,9 @@ JSONCompactEachRow JSONObjectEachRow { "row_1": {"number":"0","res":0}, - "row_1": {"number":"1","res":0}, - "row_1": {"number":"2","res":0}, - "row_1": {"number":"3","res":0}, + "row_2": {"number":"1","res":0}, + "row_3": {"number":"2","res":0}, + "row_4": {"number":"3","res":0}, "exception": "Code: 395. : Value passed to 'throwIf' function is non-zero: while executing 'FUNCTION throwIf(greater(number, 3) :: 2) -> throwIf(greater(number, 3)) UInt8 : 1'. (FUNCTION_THROW_IF_VALUE_IS_NON_ZERO) " } XML @@ -367,8 +367,8 @@ JSONCompactEachRow JSONObjectEachRow { "row_1": {"x":1,"s":"str1","y":"a"}, - "row_1": {"x":2,"s":"str2","y":"a"}, - "row_1": {"x":3,"s":"str3","y":"a"}, + "row_2": {"x":2,"s":"str2","y":"a"}, + "row_3": {"x":3,"s":"str3","y":"a"}, "exception": "Code: 36. : Unexpected value 99 in enum: While executing JSONObjectEachRowRowOutputFormat. (BAD_ARGUMENTS) " } XML From ab78f9a94301441d077c6fb902618cdff3a3d443 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Fri, 4 Aug 2023 19:08:35 +0200 Subject: [PATCH 026/101] Fix test --- .../0_stateless/02841_valid_json_and_xml_on_http_exception.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh b/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh index 60ce7eb3b6f..26b3ef64d61 100755 --- a/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh +++ b/tests/queries/0_stateless/02841_valid_json_and_xml_on_http_exception.sh @@ -4,7 +4,7 @@ CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) # shellcheck source=../shell_config.sh . "$CUR_DIR"/../shell_config.sh -CH_URL='$CLICKHOUSE_URL&http_write_exception_in_output_format=1&allow_experimental_analyzer=0' +CH_URL="$CLICKHOUSE_URL&http_write_exception_in_output_format=1&allow_experimental_analyzer=0" echo "One block" for parallel in 0 1 From 67ee1a2385e3dc3235bee24f7d20ff181dc3aa36 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Mon, 7 Aug 2023 15:00:25 +0000 Subject: [PATCH 027/101] fix tests --- .../test_compression_nested_columns/test.py | 2 +- .../configs/long_names.xml | 5 +++++ .../configs/wide_parts_only.xml | 1 - .../test_default_compression_codec/test.py | 12 ++++++++++-- tests/integration/test_filesystem_layout/test.py | 2 +- 5 files changed, 17 insertions(+), 5 deletions(-) create mode 100644 tests/integration/test_default_compression_codec/configs/long_names.xml diff --git a/tests/integration/test_compression_nested_columns/test.py b/tests/integration/test_compression_nested_columns/test.py index 55d88174287..3541a9f6061 100644 --- a/tests/integration/test_compression_nested_columns/test.py +++ b/tests/integration/test_compression_nested_columns/test.py @@ -48,7 +48,7 @@ def test_nested_compression_codec(start_cluster): column_array Array(Array(UInt64)) CODEC(T64, LZ4), column_bad LowCardinality(Int64) CODEC(Delta) ) ENGINE = ReplicatedMergeTree('/t', '{}') ORDER BY tuple() PARTITION BY key - SETTINGS min_rows_for_wide_part = 0, min_bytes_for_wide_part = 0; + SETTINGS min_rows_for_wide_part = 0, min_bytes_for_wide_part = 0, replace_long_file_name_to_hash = 0; """.format( i ), diff --git a/tests/integration/test_default_compression_codec/configs/long_names.xml b/tests/integration/test_default_compression_codec/configs/long_names.xml new file mode 100644 index 00000000000..1dc241dbf05 --- /dev/null +++ b/tests/integration/test_default_compression_codec/configs/long_names.xml @@ -0,0 +1,5 @@ + + + 0 + + diff --git a/tests/integration/test_default_compression_codec/configs/wide_parts_only.xml b/tests/integration/test_default_compression_codec/configs/wide_parts_only.xml index 4d1a3357799..10b9edef36d 100644 --- a/tests/integration/test_default_compression_codec/configs/wide_parts_only.xml +++ b/tests/integration/test_default_compression_codec/configs/wide_parts_only.xml @@ -2,6 +2,5 @@ 0 0 - 0 diff --git a/tests/integration/test_default_compression_codec/test.py b/tests/integration/test_default_compression_codec/test.py index c7c30f5eea4..abaf160e26a 100644 --- a/tests/integration/test_default_compression_codec/test.py +++ b/tests/integration/test_default_compression_codec/test.py @@ -9,12 +9,20 @@ cluster = ClickHouseCluster(__file__) node1 = cluster.add_instance( "node1", - main_configs=["configs/default_compression.xml", "configs/wide_parts_only.xml"], + main_configs=[ + "configs/default_compression.xml", + "configs/wide_parts_only.xml", + "configs/long_names.xml", + ], with_zookeeper=True, ) node2 = cluster.add_instance( "node2", - main_configs=["configs/default_compression.xml", "configs/wide_parts_only.xml"], + main_configs=[ + "configs/default_compression.xml", + "configs/wide_parts_only.xml", + "configs/long_names.xml", + ], with_zookeeper=True, ) node3 = cluster.add_instance( diff --git a/tests/integration/test_filesystem_layout/test.py b/tests/integration/test_filesystem_layout/test.py index 81f3b67cb75..4e719aa0fe9 100644 --- a/tests/integration/test_filesystem_layout/test.py +++ b/tests/integration/test_filesystem_layout/test.py @@ -48,7 +48,7 @@ def test_file_path_escaping(started_cluster): node.query( """ CREATE TABLE `test 2`.`T.a_b,l-e!` UUID '12345678-1000-4000-8000-000000000001' (`~Id` UInt32) - ENGINE = MergeTree() PARTITION BY `~Id` ORDER BY `~Id` SETTINGS min_bytes_for_wide_part = 0; + ENGINE = MergeTree() PARTITION BY `~Id` ORDER BY `~Id` SETTINGS min_bytes_for_wide_part = 0, replace_long_file_name_to_hash = 0; """ ) node.query("""INSERT INTO `test 2`.`T.a_b,l-e!` VALUES (1);""") From 8fa1f69fdf7143d0fbf27ff93853e6f64844f23c Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Tue, 8 Aug 2023 11:16:37 +0000 Subject: [PATCH 028/101] fix part columns modification time --- src/Storages/MergeTree/MergeTreeDataPartWide.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp index b3ef45b46e5..1b0de863289 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp @@ -271,7 +271,8 @@ std::optional MergeTreeDataPartWide::getColumnModificationTime(const Str { try { - return getDataPartStorage().getFileLastModified(column_name + DATA_FILE_EXTENSION).epochTime(); + auto name_on_disk = checksums.getFileNameOrHash(column_name); + return getDataPartStorage().getFileLastModified(name_on_disk + DATA_FILE_EXTENSION).epochTime(); } catch (const fs::filesystem_error &) { From 078683e226c5beb1ab1870881feab42def97f904 Mon Sep 17 00:00:00 2001 From: avogar Date: Thu, 10 Aug 2023 13:07:06 +0000 Subject: [PATCH 029/101] Fix tests --- .../Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp | 3 ++- src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp b/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp index 46b3f56f3cc..5d8e74309e3 100644 --- a/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp +++ b/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.cpp @@ -26,8 +26,9 @@ void JSONObjectEachRowRowOutputFormat::write(const Columns & columns, size_t row if (field_index_for_object_name) object_name = columns[*field_index_for_object_name]->getDataAt(row).toString(); else - object_name = "row_" + std::to_string(row + 1); + object_name = "row_" + std::to_string(getRowsReadBefore() + rows + 1); + ++rows; RowOutputFormatWithExceptionHandlerAdaptor::write(columns, row); } diff --git a/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.h b/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.h index 1981931e91b..d8ab2b09e66 100644 --- a/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.h +++ b/src/Processors/Formats/Impl/JSONObjectEachRowRowOutputFormat.h @@ -40,6 +40,7 @@ private: std::optional field_index_for_object_name; String object_name; + size_t rows = 0; }; } From 2e22b17d57aa772134f64abd41d8377b2191f211 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Tue, 15 Aug 2023 00:38:07 +0000 Subject: [PATCH 030/101] add docs and settings randomizations --- docs/en/operations/settings/merge-tree-settings.md | 11 +++++++++-- src/Storages/MergeTree/MergeTreeSettings.h | 4 ++-- tests/clickhouse-test | 2 ++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/docs/en/operations/settings/merge-tree-settings.md b/docs/en/operations/settings/merge-tree-settings.md index 4122b4af40f..36365c59b35 100644 --- a/docs/en/operations/settings/merge-tree-settings.md +++ b/docs/en/operations/settings/merge-tree-settings.md @@ -555,7 +555,7 @@ Merge reads rows from parts in blocks of `merge_max_block_size` rows, then merge ## number_of_free_entries_in_pool_to_lower_max_size_of_merge {#number-of-free-entries-in-pool-to-lower-max-size-of-merge} -When there is less than specified number of free entries in pool (or replicated queue), start to lower maximum size of merge to process (or to put in queue). +When there is less than specified number of free entries in pool (or replicated queue), start to lower maximum size of merge to process (or to put in queue). This is to allow small merges to process - not filling the pool with long running merges. Possible values: @@ -566,7 +566,7 @@ Default value: 8 ## number_of_free_entries_in_pool_to_execute_mutation {#number-of-free-entries-in-pool-to-execute-mutation} -When there is less than specified number of free entries in pool, do not execute part mutations. +When there is less than specified number of free entries in pool, do not execute part mutations. This is to leave free threads for regular merges and avoid "Too many parts". Possible values: @@ -832,6 +832,13 @@ You can see which parts of `s` were stored using the sparse serialization: └────────┴────────────────────┘ ``` +## replace_long_file_name_to_hash {#ratio_of_defaults_for_sparse_serialization} +If the file name for column is too long (more than `max_file_name_length` bytes) replace it to SipHash128. Default value: `false`. + +## max_file_name_length {#max_file_name_length} + +The maximal length of the file name to keep it as is without hashing. Takes effect only if setting `replace_long_file_name_to_hash` is enabled. Default value: 128. + ## clean_deleted_rows Enable/disable automatic deletion of rows flagged as `is_deleted` when perform `OPTIMIZE ... FINAL` on a table using the ReplacingMergeTree engine. When disabled, the `CLEANUP` keyword has to be added to the `OPTIMIZE ... FINAL` to have the same behaviour. diff --git a/src/Storages/MergeTree/MergeTreeSettings.h b/src/Storages/MergeTree/MergeTreeSettings.h index 802bd74dbf8..38bcb0fc94c 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.h +++ b/src/Storages/MergeTree/MergeTreeSettings.h @@ -34,8 +34,8 @@ struct Settings; M(UInt64, min_bytes_for_wide_part, 10485760, "Minimal uncompressed size in bytes to create part in wide format instead of compact", 0) \ M(UInt64, min_rows_for_wide_part, 0, "Minimal number of rows to create part in wide format instead of compact", 0) \ M(Float, ratio_of_defaults_for_sparse_serialization, 0.9375f, "Minimal ratio of number of default values to number of all values in column to store it in sparse serializations. If >= 1, columns will be always written in full serialization.", 0) \ - M(Bool, replace_long_file_name_to_hash, true, "If the file name for column is too long (more than 'max_file_name_length' bytes) replace it to SipHash128", 0) \ - M(UInt64, max_file_name_length, 0, "The maximal length of the file name to keep it as is without hashing", 0) \ + M(Bool, replace_long_file_name_to_hash, false, "If the file name for column is too long (more than 'max_file_name_length' bytes) replace it to SipHash128", 0) \ + M(UInt64, max_file_name_length, 128, "The maximal length of the file name to keep it as is without hashing", 0) \ /** Merge settings. */ \ M(UInt64, merge_max_block_size, 8192, "How many rows in blocks should be formed for merge operations. By default has the same value as `index_granularity`.", 0) \ M(UInt64, merge_max_block_size_bytes, 10 * 1024 * 1024, "How many bytes in blocks should be formed for merge operations. By default has the same value as `index_granularity_bytes`.", 0) \ diff --git a/tests/clickhouse-test b/tests/clickhouse-test index 1ce5ad981ad..7f8bada4a09 100755 --- a/tests/clickhouse-test +++ b/tests/clickhouse-test @@ -668,6 +668,8 @@ class MergeTreeSettingsRandomizer: "compress_primary_key": lambda: random.randint(0, 1), "marks_compress_block_size": lambda: random.randint(8000, 100000), "primary_key_compress_block_size": lambda: random.randint(8000, 100000), + "replace_long_file_name_to_hash": lambda: random.randint(0, 1), + "max_file_name_length": threshold_generator(0.3, 0.3, 0, 128), } @staticmethod From 3e9a1825556a12e9dc5362b1bb29b877be158a4e Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Mon, 21 Aug 2023 13:49:14 +0000 Subject: [PATCH 031/101] better interfaces for getting of stream name in part --- .../settings/merge-tree-settings.md | 2 +- src/Storages/MergeTree/IMergeTreeDataPart.cpp | 83 +++++++++++-- src/Storages/MergeTree/IMergeTreeDataPart.h | 33 +++++- .../MergeTree/IMergedBlockOutputStream.cpp | 15 ++- .../MergeTree/MergeTreeDataPartChecksum.cpp | 12 -- .../MergeTree/MergeTreeDataPartChecksum.h | 3 - .../MergeTree/MergeTreeDataPartCompact.h | 2 +- .../MergeTree/MergeTreeDataPartInMemory.h | 2 +- .../MergeTree/MergeTreeDataPartWide.cpp | 110 +++++++++--------- .../MergeTree/MergeTreeDataPartWide.h | 2 +- .../MergeTree/MergeTreeReaderWide.cpp | 45 +++---- src/Storages/MergeTree/MergeTreeSettings.h | 2 +- src/Storages/MergeTree/MutateTask.cpp | 32 +++-- src/Storages/MergeTree/checkDataPart.cpp | 9 +- .../System/StorageSystemPartsColumns.cpp | 22 ++-- 15 files changed, 227 insertions(+), 147 deletions(-) diff --git a/docs/en/operations/settings/merge-tree-settings.md b/docs/en/operations/settings/merge-tree-settings.md index 36365c59b35..e1d9e76c2ba 100644 --- a/docs/en/operations/settings/merge-tree-settings.md +++ b/docs/en/operations/settings/merge-tree-settings.md @@ -832,7 +832,7 @@ You can see which parts of `s` were stored using the sparse serialization: └────────┴────────────────────┘ ``` -## replace_long_file_name_to_hash {#ratio_of_defaults_for_sparse_serialization} +## replace_long_file_name_to_hash {#replace_long_file_name_to_hash} If the file name for column is too long (more than `max_file_name_length` bytes) replace it to SipHash128. Default value: `false`. ## max_file_name_length {#max_file_name_length} diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.cpp b/src/Storages/MergeTree/IMergeTreeDataPart.cpp index 5b2f75b6f00..81330255a5f 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.cpp +++ b/src/Storages/MergeTree/IMergeTreeDataPart.cpp @@ -1034,14 +1034,14 @@ CompressionCodecPtr IMergeTreeDataPart::detectDefaultCompressionCodec() const { if (path_to_data_file.empty()) { - auto candidate_path = ISerialization::getFileNameForStream(part_column, substream_path) + ".bin"; - - if (!getDataPartStorage().exists(candidate_path)) - candidate_path = sipHash128String(candidate_path) + ".bin"; + auto stream_name = getStreamNameForColumn(part_column, substream_path, ".bin", getDataPartStorage()); + if (!stream_name) + return; + auto file_name = *stream_name + ".bin"; /// We can have existing, but empty .bin files. Example: LowCardinality(Nullable(...)) columns and column_name.dict.null.bin file. - if (getDataPartStorage().exists(candidate_path) && getDataPartStorage().getFileSize(candidate_path) != 0) - path_to_data_file = candidate_path; + if (getDataPartStorage().getFileSize(file_name) != 0) + path_to_data_file = file_name; } }); @@ -1326,8 +1326,8 @@ void IMergeTreeDataPart::loadColumns(bool require) auto metadata_snapshot = storage.getInMemoryMetadataPtr(); if (parent_part) metadata_snapshot = metadata_snapshot->projections.get(name).metadata; - NamesAndTypesList loaded_columns; + NamesAndTypesList loaded_columns; bool is_readonly_storage = getDataPartStorage().isReadonly(); if (!metadata_manager->exists("columns.txt")) @@ -1339,7 +1339,7 @@ void IMergeTreeDataPart::loadColumns(bool require) /// If there is no file with a list of columns, write it down. for (const NameAndTypePair & column : metadata_snapshot->getColumns().getAllPhysical()) - if (getDataPartStorage().exists(getFileNameForColumn(column) + ".bin")) + if (getFileNameForColumn(column)) loaded_columns.push_back(column); if (columns.empty()) @@ -2090,6 +2090,73 @@ IMergeTreeDataPart::uint128 IMergeTreeDataPart::getActualChecksumByFile(const St return in_hash.getHash(); } +std::optional IMergeTreeDataPart::getStreamNameOrHash( + const String & stream_name, + const Checksums & checksums_) +{ + if (checksums_.files.contains(stream_name + ".bin")) + return stream_name; + + auto hash = sipHash128String(stream_name); + if (checksums_.files.contains(hash + ".bin")) + return hash; + + return {}; +} + +std::optional IMergeTreeDataPart::getStreamNameOrHash( + const String & stream_name, + const String & extension, + const IDataPartStorage & storage_) +{ + if (storage_.exists(stream_name + extension)) + return stream_name; + + auto hash = sipHash128String(stream_name); + if (storage_.exists(hash + extension)) + return stream_name; + + return {}; +} + +std::optional IMergeTreeDataPart::getStreamNameForColumn( + const String & column_name, + const ISerialization::SubstreamPath & substream_path, + const Checksums & checksums_) +{ + auto stream_name = ISerialization::getFileNameForStream(column_name, substream_path); + return getStreamNameOrHash(stream_name, checksums_); +} + +std::optional IMergeTreeDataPart::getStreamNameForColumn( + const NameAndTypePair & column, + const ISerialization::SubstreamPath & substream_path, + const Checksums & checksums_) +{ + auto stream_name = ISerialization::getFileNameForStream(column, substream_path); + return getStreamNameOrHash(stream_name, checksums_); +} + +std::optional IMergeTreeDataPart::getStreamNameForColumn( + const String & column_name, + const ISerialization::SubstreamPath & substream_path, + const String & extension, + const IDataPartStorage & storage_) +{ + auto stream_name = ISerialization::getFileNameForStream(column_name, substream_path); + return getStreamNameOrHash(stream_name, extension, storage_); +} + +std::optional IMergeTreeDataPart::getStreamNameForColumn( + const NameAndTypePair & column, + const ISerialization::SubstreamPath & substream_path, + const String & extension, + const IDataPartStorage & storage_) +{ + auto stream_name = ISerialization::getFileNameForStream(column, substream_path); + return getStreamNameOrHash(stream_name, extension, storage_); +} + std::unordered_map IMergeTreeDataPart::checkMetadata() const { return metadata_manager->check(); diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.h b/src/Storages/MergeTree/IMergeTreeDataPart.h index a8e053a9c7b..34e6801c3ef 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.h +++ b/src/Storages/MergeTree/IMergeTreeDataPart.h @@ -131,7 +131,7 @@ public: /// Return information about secondary indexes size on disk for all indexes in part IndexSize getTotalSeconaryIndicesSize() const { return total_secondary_indices_size; } - virtual String getFileNameForColumn(const NameAndTypePair & column) const = 0; + virtual std::optional getFileNameForColumn(const NameAndTypePair & column) const = 0; virtual ~IMergeTreeDataPart(); @@ -503,6 +503,37 @@ public: /// This one is about removing file with version of part's metadata (columns, pk and so on) void removeMetadataVersion(); + static std::optional getStreamNameOrHash( + const String & name, + const IMergeTreeDataPart::Checksums & checksums); + + static std::optional getStreamNameOrHash( + const String & name, + const String & extension, + const IDataPartStorage & storage_); + + static std::optional getStreamNameForColumn( + const String & column_name, + const ISerialization::SubstreamPath & substream_path, + const Checksums & checksums_); + + static std::optional getStreamNameForColumn( + const NameAndTypePair & column, + const ISerialization::SubstreamPath & substream_path, + const Checksums & checksums_); + + static std::optional getStreamNameForColumn( + const String & column_name, + const ISerialization::SubstreamPath & substream_path, + const String & extension, + const IDataPartStorage & storage_); + + static std::optional getStreamNameForColumn( + const NameAndTypePair & column, + const ISerialization::SubstreamPath & substream_path, + const String & extension, + const IDataPartStorage & storage_); + mutable std::atomic removal_state = DataPartRemovalState::NOT_ATTEMPTED; mutable std::atomic last_removal_attempt_time = 0; diff --git a/src/Storages/MergeTree/IMergedBlockOutputStream.cpp b/src/Storages/MergeTree/IMergedBlockOutputStream.cpp index 2df3b6d15a6..c8d6aa0ba65 100644 --- a/src/Storages/MergeTree/IMergedBlockOutputStream.cpp +++ b/src/Storages/MergeTree/IMergedBlockOutputStream.cpp @@ -51,9 +51,9 @@ NameSet IMergedBlockOutputStream::removeEmptyColumnsFromPart( data_part->getSerialization(column.name)->enumerateStreams( [&](const ISerialization::SubstreamPath & substream_path) { - auto full_stream_name = ISerialization::getFileNameForStream(column.name, substream_path); - auto stream_name = checksums.getFileNameOrHash(full_stream_name); - ++stream_counts[stream_name]; + auto stream_name = IMergeTreeDataPart::getStreamNameForColumn(column, substream_path, checksums); + if (stream_name) + ++stream_counts[*stream_name]; }); } @@ -67,14 +67,13 @@ NameSet IMergedBlockOutputStream::removeEmptyColumnsFromPart( ISerialization::StreamCallback callback = [&](const ISerialization::SubstreamPath & substream_path) { - auto full_stream_name = ISerialization::getFileNameForStream(column_name, substream_path); - auto stream_name = checksums.getFileNameOrHash(full_stream_name); + auto stream_name = IMergeTreeDataPart::getStreamNameForColumn(column_name, substream_path, checksums); /// Delete files if they are no longer shared with another column. - if (--stream_counts[stream_name] == 0) + if (stream_name && --stream_counts[*stream_name] == 0) { - remove_files.emplace(stream_name + ".bin"); - remove_files.emplace(stream_name + mrk_extension); + remove_files.emplace(*stream_name + ".bin"); + remove_files.emplace(*stream_name + mrk_extension); } }; diff --git a/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp b/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp index 794eba809a2..ed2202fcb19 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartChecksum.cpp @@ -312,18 +312,6 @@ MergeTreeDataPartChecksums::Checksum::uint128 MergeTreeDataPartChecksums::getTot return getSipHash128AsPair(hash_of_all_files); } -String MergeTreeDataPartChecksums::getFileNameOrHash(const String & name) const -{ - if (files.contains(name + ".bin")) - return name; - - auto hash = sipHash128String(name); - if (files.contains(hash + ".bin")) - return hash; - - return name; -} - void MinimalisticDataPartChecksums::serialize(WriteBuffer & to) const { writeString("checksums format version: 5\n", to); diff --git a/src/Storages/MergeTree/MergeTreeDataPartChecksum.h b/src/Storages/MergeTree/MergeTreeDataPartChecksum.h index 2a38b52c72a..8e5e8c8c448 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartChecksum.h +++ b/src/Storages/MergeTree/MergeTreeDataPartChecksum.h @@ -88,11 +88,8 @@ struct MergeTreeDataPartChecksums static MergeTreeDataPartChecksums deserializeFrom(const String & s); UInt64 getTotalSizeOnDisk() const; - - String getFileNameOrHash(const String & name) const; }; - /// A kind of MergeTreeDataPartChecksums intended to be stored in ZooKeeper (to save its RAM) /// MinimalisticDataPartChecksums and MergeTreeDataPartChecksums have the same serialization format /// for versions less than MINIMAL_VERSION_WITH_MINIMALISTIC_CHECKSUMS. diff --git a/src/Storages/MergeTree/MergeTreeDataPartCompact.h b/src/Storages/MergeTree/MergeTreeDataPartCompact.h index 2bbac766c8e..7850e7c976c 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartCompact.h +++ b/src/Storages/MergeTree/MergeTreeDataPartCompact.h @@ -57,7 +57,7 @@ public: std::optional getColumnModificationTime(const String & column_name) const override; - String getFileNameForColumn(const NameAndTypePair & /* column */) const override { return DATA_FILE_NAME; } + std::optional getFileNameForColumn(const NameAndTypePair & /* column */) const override { return DATA_FILE_NAME; } ~MergeTreeDataPartCompact() override; diff --git a/src/Storages/MergeTree/MergeTreeDataPartInMemory.h b/src/Storages/MergeTree/MergeTreeDataPartInMemory.h index 95f7b796f9a..c7b7dde50a6 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartInMemory.h +++ b/src/Storages/MergeTree/MergeTreeDataPartInMemory.h @@ -40,7 +40,7 @@ public: bool isStoredOnRemoteDisk() const override { return false; } bool isStoredOnRemoteDiskWithZeroCopySupport() const override { return false; } bool hasColumnFiles(const NameAndTypePair & column) const override { return !!getColumnPosition(column.getNameInStorage()); } - String getFileNameForColumn(const NameAndTypePair & /* column */) const override { return ""; } + std::optional getFileNameForColumn(const NameAndTypePair & /* column */) const override { return ""; } void renameTo(const String & new_relative_path, bool remove_new_dir_if_exists) override; DataPartStoragePtr makeCloneInDetached(const String & prefix, const StorageMetadataPtr & metadata_snapshot, const DiskTransactionPtr & disk_transaction) const override; diff --git a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp index 417b5f3d19b..9b71c8df3a3 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWide.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWide.cpp @@ -73,20 +73,22 @@ ColumnSize MergeTreeDataPartWide::getColumnSizeImpl( getSerialization(column.name)->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { - auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); - auto stream_name = checksums.getFileNameOrHash(full_stream_name); + auto stream_name = IMergeTreeDataPart::getStreamNameForColumn(column, substream_path, checksums); - if (processed_substreams && !processed_substreams->insert(stream_name).second) + if (!stream_name) return; - auto bin_checksum = checksums.files.find(stream_name + ".bin"); + if (processed_substreams && !processed_substreams->insert(*stream_name).second) + return; + + auto bin_checksum = checksums.files.find(*stream_name + ".bin"); if (bin_checksum != checksums.files.end()) { size.data_compressed += bin_checksum->second.file_size; size.data_uncompressed += bin_checksum->second.uncompressed_size; } - auto mrk_checksum = checksums.files.find(stream_name + getMarksFileExtension()); + auto mrk_checksum = checksums.files.find(*stream_name + getMarksFileExtension()); if (mrk_checksum != checksums.files.end()) size.marks += mrk_checksum->second.file_size; }); @@ -154,7 +156,13 @@ void MergeTreeDataPartWide::loadIndexGranularity() if (columns.empty()) throw Exception(ErrorCodes::NO_FILE_IN_DATA_PART, "No columns in part {}", name); - loadIndexGranularityImpl(index_granularity, index_granularity_info, getDataPartStorage(), getFileNameForColumn(columns.front())); + auto any_column_filename = getFileNameForColumn(columns.front()); + if (!any_column_filename) + throw Exception(ErrorCodes::NO_FILE_IN_DATA_PART, + "There are no files for column {} in part {}", + columns.front().name, getDataPartStorage().getFullPath()); + + loadIndexGranularityImpl(index_granularity, index_granularity_info, getDataPartStorage(), *any_column_filename); } @@ -186,23 +194,19 @@ void MergeTreeDataPartWide::checkConsistency(bool require_part_metadata) const { getSerialization(name_type.name)->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { - String full_stream_name = ISerialization::getFileNameForStream(name_type, substream_path); - String stream_name = checksums.getFileNameOrHash(full_stream_name); - - String mrk_file_name = stream_name + marks_file_extension; - String bin_file_name = stream_name + DATA_FILE_EXTENSION; + auto stream_name = getStreamNameForColumn(name_type, substream_path, checksums); + if (!stream_name) + throw Exception( + ErrorCodes::NO_FILE_IN_DATA_PART, + "No {}.{} file checksum for column {} in part {}", + *stream_name, DATA_FILE_EXTENSION, name_type.name, getDataPartStorage().getFullPath()); + auto mrk_file_name = *stream_name + marks_file_extension; if (!checksums.files.contains(mrk_file_name)) throw Exception( ErrorCodes::NO_FILE_IN_DATA_PART, "No {} file checksum for column {} in part {} ", mrk_file_name, name_type.name, getDataPartStorage().getFullPath()); - - if (!checksums.files.contains(bin_file_name)) - throw Exception( - ErrorCodes::NO_FILE_IN_DATA_PART, - "No {} file checksum for column {} in part {}", - bin_file_name, name_type.name, getDataPartStorage().getFullPath()); }); } } @@ -215,29 +219,28 @@ void MergeTreeDataPartWide::checkConsistency(bool require_part_metadata) const { getSerialization(name_type.name)->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { - auto file_path = ISerialization::getFileNameForStream(name_type, substream_path) + marks_file_extension; - if (!getDataPartStorage().exists(file_path)) - file_path = sipHash128String(file_path) + marks_file_extension; + auto stream_name = getStreamNameForColumn(name_type, substream_path, marks_file_extension, getDataPartStorage()); /// Missing file is Ok for case when new column was added. - if (getDataPartStorage().exists(file_path)) - { - UInt64 file_size = getDataPartStorage().getFileSize(file_path); + if (!stream_name) + return; - if (!file_size) - throw Exception( - ErrorCodes::BAD_SIZE_OF_FILE_IN_DATA_PART, - "Part {} is broken: {} is empty.", - getDataPartStorage().getFullPath(), - std::string(fs::path(getDataPartStorage().getFullPath()) / file_path)); + auto file_path = *stream_name + marks_file_extension; + UInt64 file_size = getDataPartStorage().getFileSize(file_path); - if (!marks_size) - marks_size = file_size; - else if (file_size != *marks_size) - throw Exception( - ErrorCodes::BAD_SIZE_OF_FILE_IN_DATA_PART, - "Part {} is broken: marks have different sizes.", getDataPartStorage().getFullPath()); - } + if (!file_size) + throw Exception( + ErrorCodes::BAD_SIZE_OF_FILE_IN_DATA_PART, + "Part {} is broken: {} is empty.", + getDataPartStorage().getFullPath(), + std::string(fs::path(getDataPartStorage().getFullPath()) / file_path)); + + if (!marks_size) + marks_size = file_size; + else if (file_size != *marks_size) + throw Exception( + ErrorCodes::BAD_SIZE_OF_FILE_IN_DATA_PART, + "Part {} is broken: marks have different sizes.", getDataPartStorage().getFullPath()); }); } } @@ -245,22 +248,13 @@ void MergeTreeDataPartWide::checkConsistency(bool require_part_metadata) const bool MergeTreeDataPartWide::hasColumnFiles(const NameAndTypePair & column) const { - std::string marks_file_extension = index_granularity_info.mark_type.getFileExtension(); - auto check_stream_exists = [this, &marks_file_extension](const String & stream_name) - { - auto bin_checksum = checksums.files.find(stream_name + DATA_FILE_EXTENSION); - auto mrk_checksum = checksums.files.find(stream_name + marks_file_extension); - - return bin_checksum != checksums.files.end() && mrk_checksum != checksums.files.end(); - }; + auto marks_file_extension = index_granularity_info.mark_type.getFileExtension(); bool res = true; getSerialization(column.name)->enumerateStreams([&](const auto & substream_path) { - auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); - auto stream_name = checksums.getFileNameOrHash(full_stream_name); - - if (!check_stream_exists(stream_name)) + auto stream_name = getStreamNameForColumn(column, substream_path, checksums); + if (!stream_name || !checksums.files.contains(*stream_name + marks_file_extension)) res = false; }); @@ -271,8 +265,11 @@ std::optional MergeTreeDataPartWide::getColumnModificationTime(const Str { try { - auto name_on_disk = checksums.getFileNameOrHash(column_name); - return getDataPartStorage().getFileLastModified(name_on_disk + DATA_FILE_EXTENSION).epochTime(); + auto stream_name = getStreamNameOrHash(column_name, checksums); + if (!stream_name) + return {}; + + return getDataPartStorage().getFileLastModified(*stream_name + DATA_FILE_EXTENSION).epochTime(); } catch (const fs::filesystem_error &) { @@ -280,15 +277,18 @@ std::optional MergeTreeDataPartWide::getColumnModificationTime(const Str } } -String MergeTreeDataPartWide::getFileNameForColumn(const NameAndTypePair & column) const +std::optional MergeTreeDataPartWide::getFileNameForColumn(const NameAndTypePair & column) const { - String filename; + std::optional filename; getSerialization(column.name)->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { - if (filename.empty()) + if (!filename.has_value()) { - auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); - filename = checksums.getFileNameOrHash(full_stream_name); + /// This method may be called when checksums are not initialized yet. + if (!checksums.empty()) + filename = getStreamNameForColumn(column, substream_path, checksums); + else + filename = getStreamNameForColumn(column, substream_path, DATA_FILE_EXTENSION, getDataPartStorage()); } }); return filename; diff --git a/src/Storages/MergeTree/MergeTreeDataPartWide.h b/src/Storages/MergeTree/MergeTreeDataPartWide.h index 2076a1ec028..bcf70426fa6 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWide.h +++ b/src/Storages/MergeTree/MergeTreeDataPartWide.h @@ -48,7 +48,7 @@ public: bool isStoredOnRemoteDiskWithZeroCopySupport() const override; - String getFileNameForColumn(const NameAndTypePair & column) const override; + std::optional getFileNameForColumn(const NameAndTypePair & column) const override; ~MergeTreeDataPartWide() override; diff --git a/src/Storages/MergeTree/MergeTreeReaderWide.cpp b/src/Storages/MergeTree/MergeTreeReaderWide.cpp index 999e3d0f7ec..4ba6402e3c5 100644 --- a/src/Storages/MergeTree/MergeTreeReaderWide.cpp +++ b/src/Storages/MergeTree/MergeTreeReaderWide.cpp @@ -202,15 +202,6 @@ size_t MergeTreeReaderWide::readRows( return read_rows; } -String getStreamName( - const NameAndTypePair & column, - const ISerialization::SubstreamPath & substream_path, - const MergeTreeDataPartChecksums & checksums) -{ - auto full_stream_name = ISerialization::getFileNameForStream(column, substream_path); - return checksums.getFileNameOrHash(full_stream_name); -} - void MergeTreeReaderWide::addStreams( const NameAndTypePair & name_and_type, const SerializationPtr & serialization, @@ -222,35 +213,33 @@ void MergeTreeReaderWide::addStreams( ISerialization::StreamCallback callback = [&] (const ISerialization::SubstreamPath & substream_path) { - auto stream_name = getStreamName(name_and_type, substream_path, data_part_info_for_read->getChecksums()); - - if (streams.contains(stream_name)) - { - has_any_stream = true; - return; - } - - bool data_file_exists = data_part_info_for_read->getChecksums().files.contains(stream_name + DATA_FILE_EXTENSION); + auto stream_name = IMergeTreeDataPart::getStreamNameForColumn(name_and_type, substream_path, data_part_info_for_read->getChecksums()); /** If data file is missing then we will not try to open it. * It is necessary since it allows to add new column to structure of the table without creating new files for old parts. */ - if (!data_file_exists) + if (!stream_name) { has_all_streams = false; return; } + if (streams.contains(*stream_name)) + { + has_any_stream = true; + return; + } + has_any_stream = true; bool is_lc_dict = substream_path.size() > 1 && substream_path[substream_path.size() - 2].type == ISerialization::Substream::Type::DictionaryKeys; auto context = data_part_info_for_read->getContext(); auto * load_marks_threadpool = settings.read_settings.load_marks_asynchronously ? &context->getLoadMarksThreadpool() : nullptr; - streams.emplace(stream_name, std::make_unique( - data_part_info_for_read, stream_name, DATA_FILE_EXTENSION, + streams.emplace(*stream_name, std::make_unique( + data_part_info_for_read, *stream_name, DATA_FILE_EXTENSION, data_part_info_for_read->getMarksCount(), all_mark_ranges, settings, mark_cache, - uncompressed_cache, data_part_info_for_read->getFileSizeOrZero(stream_name + DATA_FILE_EXTENSION), + uncompressed_cache, data_part_info_for_read->getFileSizeOrZero(*stream_name + DATA_FILE_EXTENSION), &data_part_info_for_read->getIndexGranularityInfo(), profile_callback, clock_type, is_lc_dict, load_marks_threadpool)); }; @@ -276,9 +265,11 @@ static ReadBuffer * getStream( if (cache.contains(ISerialization::getSubcolumnNameForStream(substream_path))) return nullptr; - auto stream_name = getStreamName(name_and_type, substream_path, checksums); + auto stream_name = IMergeTreeDataPart::getStreamNameForColumn(name_and_type, substream_path, checksums); + if (!stream_name) + return nullptr; - auto it = streams.find(stream_name); + auto it = streams.find(*stream_name); if (it == streams.end()) return nullptr; @@ -324,15 +315,15 @@ void MergeTreeReaderWide::prefetchForColumn( serialization->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { - auto stream_name = getStreamName(name_and_type, substream_path, data_part_info_for_read->getChecksums()); + auto stream_name = IMergeTreeDataPart::getStreamNameForColumn(name_and_type, substream_path, data_part_info_for_read->getChecksums()); - if (!prefetched_streams.contains(stream_name)) + if (stream_name && !prefetched_streams.contains(*stream_name)) { bool seek_to_mark = !continue_reading; if (ReadBuffer * buf = getStream(false, substream_path, data_part_info_for_read->getChecksums(), streams, name_and_type, from_mark, seek_to_mark, current_task_last_mark, cache)) { buf->prefetch(priority); - prefetched_streams.insert(stream_name); + prefetched_streams.insert(*stream_name); } } }); diff --git a/src/Storages/MergeTree/MergeTreeSettings.h b/src/Storages/MergeTree/MergeTreeSettings.h index 38bcb0fc94c..4f02c1c543e 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.h +++ b/src/Storages/MergeTree/MergeTreeSettings.h @@ -35,7 +35,7 @@ struct Settings; M(UInt64, min_rows_for_wide_part, 0, "Minimal number of rows to create part in wide format instead of compact", 0) \ M(Float, ratio_of_defaults_for_sparse_serialization, 0.9375f, "Minimal ratio of number of default values to number of all values in column to store it in sparse serializations. If >= 1, columns will be always written in full serialization.", 0) \ M(Bool, replace_long_file_name_to_hash, false, "If the file name for column is too long (more than 'max_file_name_length' bytes) replace it to SipHash128", 0) \ - M(UInt64, max_file_name_length, 128, "The maximal length of the file name to keep it as is without hashing", 0) \ + M(UInt64, max_file_name_length, 127, "The maximal length of the file name to keep it as is without hashing", 0) \ /** Merge settings. */ \ M(UInt64, merge_max_block_size, 8192, "How many rows in blocks should be formed for merge operations. By default has the same value as `index_granularity`.", 0) \ M(UInt64, merge_max_block_size_bytes, 10 * 1024 * 1024, "How many bytes in blocks should be formed for merge operations. By default has the same value as `index_granularity_bytes`.", 0) \ diff --git a/src/Storages/MergeTree/MutateTask.cpp b/src/Storages/MergeTree/MutateTask.cpp index 5ce5355c794..6de23f3e294 100644 --- a/src/Storages/MergeTree/MutateTask.cpp +++ b/src/Storages/MergeTree/MutateTask.cpp @@ -534,9 +534,9 @@ static std::unordered_map getStreamCounts( { auto callback = [&](const ISerialization::SubstreamPath & substream_path) { - auto full_stream_name = ISerialization::getFileNameForStream(column_name, substream_path); - auto stream_name = source_part_checksums.getFileNameOrHash(full_stream_name); - ++stream_counts[stream_name]; + auto stream_name = IMergeTreeDataPart::getStreamNameForColumn(column_name, substream_path, source_part_checksums); + if (stream_name) + ++stream_counts[*stream_name]; }; serialization->enumerateStreams(callback); @@ -654,14 +654,13 @@ static NameToNameVector collectFilesForRenames( { ISerialization::StreamCallback callback = [&](const ISerialization::SubstreamPath & substream_path) { - auto full_stream_name = ISerialization::getFileNameForStream({command.column_name, command.data_type}, substream_path); - auto stream_name = source_part->checksums.getFileNameOrHash(full_stream_name); + auto stream_name = IMergeTreeDataPart::getStreamNameForColumn(command.column_name, substream_path, source_part->checksums); /// Delete files if they are no longer shared with another column. - if (--stream_counts[stream_name] == 0) + if (stream_name && --stream_counts[*stream_name] == 0) { - add_rename(stream_name + ".bin", ""); - add_rename(stream_name + mrk_extension, ""); + add_rename(*stream_name + ".bin", ""); + add_rename(*stream_name + mrk_extension, ""); } }; @@ -678,13 +677,22 @@ static NameToNameVector collectFilesForRenames( String full_stream_from = ISerialization::getFileNameForStream(command.column_name, substream_path); String full_stream_to = boost::replace_first_copy(full_stream_from, escaped_name_from, escaped_name_to); - String stream_from = source_part->checksums.getFileNameOrHash(full_stream_from); - String stream_to = stream_from == full_stream_from ? full_stream_to : sipHash128String(full_stream_to); + auto stream_from = IMergeTreeDataPart::getStreamNameOrHash(full_stream_from, source_part->checksums); + if (!stream_from) + return; + + String stream_to; + auto storage_settings = source_part->storage.getSettings(); + + if (storage_settings->replace_long_file_name_to_hash && full_stream_to.size() > storage_settings->max_file_name_length) + stream_to = sipHash128String(full_stream_to); + else + stream_to = full_stream_to; if (stream_from != stream_to) { - add_rename(stream_from + ".bin", stream_to + ".bin"); - add_rename(stream_from + mrk_extension, stream_to + mrk_extension); + add_rename(*stream_from + ".bin", stream_to + ".bin"); + add_rename(*stream_from + mrk_extension, stream_to + mrk_extension); } }; diff --git a/src/Storages/MergeTree/checkDataPart.cpp b/src/Storages/MergeTree/checkDataPart.cpp index 33715785574..f54056421a7 100644 --- a/src/Storages/MergeTree/checkDataPart.cpp +++ b/src/Storages/MergeTree/checkDataPart.cpp @@ -165,17 +165,14 @@ static IMergeTreeDataPart::Checksums checkDataPart( { get_serialization(column)->enumerateStreams([&](const ISerialization::SubstreamPath & substream_path) { - auto stream_name = ISerialization::getFileNameForStream(column, substream_path); - auto file_name = stream_name + ".bin"; + auto stream_name = IMergeTreeDataPart::getStreamNameForColumn(column, substream_path, ".bin", data_part_storage); - if (!data_part_storage.exists(file_name)) - file_name = sipHash128String(stream_name) + ".bin"; - - if (!data_part_storage.exists(file_name)) + if (!stream_name) throw Exception(ErrorCodes::NO_FILE_IN_DATA_PART, "There is no file for column '{}' in data part '{}'", column.name, data_part->name); + auto file_name = *stream_name + ".bin"; checksums_data.files[file_name] = checksum_compressed_file(data_part_storage, file_name); }); } diff --git a/src/Storages/System/StorageSystemPartsColumns.cpp b/src/Storages/System/StorageSystemPartsColumns.cpp index 0510c733e65..275d56c3da5 100644 --- a/src/Storages/System/StorageSystemPartsColumns.cpp +++ b/src/Storages/System/StorageSystemPartsColumns.cpp @@ -271,19 +271,21 @@ void StorageSystemPartsColumns::processNextStorage( ColumnSize size; NameAndTypePair subcolumn(column.name, name, column.type, data.type); - String full_stream_name = ISerialization::getFileNameForStream(subcolumn, subpath); - String stream_name = part->checksums.getFileNameOrHash(full_stream_name); - auto bin_checksum = part->checksums.files.find(stream_name + ".bin"); - if (bin_checksum != part->checksums.files.end()) + auto stream_name = IMergeTreeDataPart::getStreamNameForColumn(subcolumn, subpath, part->checksums); + if (stream_name) { - size.data_compressed += bin_checksum->second.file_size; - size.data_uncompressed += bin_checksum->second.uncompressed_size; - } + auto bin_checksum = part->checksums.files.find(*stream_name + ".bin"); + if (bin_checksum != part->checksums.files.end()) + { + size.data_compressed += bin_checksum->second.file_size; + size.data_uncompressed += bin_checksum->second.uncompressed_size; + } - auto mrk_checksum = part->checksums.files.find(stream_name + part->index_granularity_info.mark_type.getFileExtension()); - if (mrk_checksum != part->checksums.files.end()) - size.marks += mrk_checksum->second.file_size; + auto mrk_checksum = part->checksums.files.find(*stream_name + part->index_granularity_info.mark_type.getFileExtension()); + if (mrk_checksum != part->checksums.files.end()) + size.marks += mrk_checksum->second.file_size; + } subcolumn_bytes_on_disk.push_back(size.data_compressed + size.marks); subcolumn_data_compressed_bytes.push_back(size.data_compressed); From 03f5d3feb4c11ebf7ef8fa4af0dd634dfd3fa41b Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Mon, 28 Aug 2023 14:58:45 +0000 Subject: [PATCH 032/101] fix checkDataPart --- src/Storages/MergeTree/IMergeTreeDataPart.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.cpp b/src/Storages/MergeTree/IMergeTreeDataPart.cpp index 81330255a5f..58fa6e2e66d 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.cpp +++ b/src/Storages/MergeTree/IMergeTreeDataPart.cpp @@ -2114,7 +2114,7 @@ std::optional IMergeTreeDataPart::getStreamNameOrHash( auto hash = sipHash128String(stream_name); if (storage_.exists(hash + extension)) - return stream_name; + return hash; return {}; } From 54755d7e095a0818fa5a8bf234114c2e7e192f80 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Tue, 29 Aug 2023 12:02:43 +0000 Subject: [PATCH 033/101] fix test --- tests/queries/0_stateless/02253_empty_part_checksums.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02253_empty_part_checksums.sh b/tests/queries/0_stateless/02253_empty_part_checksums.sh index 5d4e750f46d..371c0768e3d 100755 --- a/tests/queries/0_stateless/02253_empty_part_checksums.sh +++ b/tests/queries/0_stateless/02253_empty_part_checksums.sh @@ -10,7 +10,7 @@ $CLICKHOUSE_CLIENT -q "drop table if exists rmt sync;" $CLICKHOUSE_CLIENT -q "CREATE TABLE rmt (a UInt8, b Int16, c Float32, d String, e Array(UInt8), f Nullable(UUID), g Tuple(UInt8, UInt16)) ENGINE = ReplicatedMergeTree('/test/02253/$CLICKHOUSE_TEST_ZOOKEEPER_PREFIX/rmt', '1') ORDER BY a PARTITION BY b % 10 SETTINGS old_parts_lifetime = 1, cleanup_delay_period = 0, cleanup_delay_period_random_add = 0, -cleanup_thread_preferred_points_per_iteration=0, min_bytes_for_wide_part=0, remove_empty_parts=0" +cleanup_thread_preferred_points_per_iteration=0, min_bytes_for_wide_part=0, remove_empty_parts=0, replace_long_file_name_to_hash=0" $CLICKHOUSE_CLIENT --insert_keeper_fault_injection_probability=0 -q "INSERT INTO rmt SELECT rand(1), 0, 1 / rand(3), toString(rand(4)), [rand(5), rand(6)], rand(7) % 2 ? NULL : generateUUIDv4(), (rand(8), rand(9)) FROM numbers(1000);" From 1910434174234ccdf5e30291453eb5ebda687b9b Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 30 Aug 2023 20:48:26 +0200 Subject: [PATCH 034/101] Remove CurrentStatusInfo --- src/Common/StatusInfo.cpp | 57 -------------------------- src/Common/StatusInfo.h | 39 ------------------ src/Interpreters/ExternalLoader.cpp | 10 ----- src/Server/PrometheusMetricsWriter.cpp | 38 ----------------- src/Server/PrometheusMetricsWriter.h | 2 - 5 files changed, 146 deletions(-) delete mode 100644 src/Common/StatusInfo.cpp delete mode 100644 src/Common/StatusInfo.h diff --git a/src/Common/StatusInfo.cpp b/src/Common/StatusInfo.cpp deleted file mode 100644 index 1f9ddfaf4b9..00000000000 --- a/src/Common/StatusInfo.cpp +++ /dev/null @@ -1,57 +0,0 @@ -#include -#include - -/// Available status. Add something here as you wish. -#define APPLY_FOR_STATUS(M) \ - M(DictionaryStatus, "Dictionary Status.", DB::getStatusEnumAllPossibleValues()) \ - - -namespace CurrentStatusInfo -{ - #define M(NAME, DOCUMENTATION, ENUM) extern const Status NAME = Status(__COUNTER__); - APPLY_FOR_STATUS(M) - #undef M - constexpr Status END = Status(__COUNTER__); - - std::mutex locks[END] {}; - std::unordered_map values[END] {}; - - const char * getName(Status event) - { - static const char * strings[] = - { - #define M(NAME, DOCUMENTATION, ENUM) #NAME, - APPLY_FOR_STATUS(M) - #undef M - }; - - return strings[event]; - } - - const char * getDocumentation(Status event) - { - static const char * strings[] = - { - #define M(NAME, DOCUMENTATION, ENUM) #DOCUMENTATION, - APPLY_FOR_STATUS(M) - #undef M - }; - - return strings[event]; - } - - const std::vector> & getAllPossibleValues(Status event) - { - static const std::vector> enum_values [] = - { - #define M(NAME, DOCUMENTATION, ENUM) ENUM, - APPLY_FOR_STATUS(M) - #undef M - }; - return enum_values[event]; - } - - Status end() { return END; } -} - -#undef APPLY_FOR_STATUS diff --git a/src/Common/StatusInfo.h b/src/Common/StatusInfo.h deleted file mode 100644 index 91e6d4d3b85..00000000000 --- a/src/Common/StatusInfo.h +++ /dev/null @@ -1,39 +0,0 @@ -#pragma once - -#include -#include -#include -#include -#include -#include -#include -#include -#include - - -namespace CurrentStatusInfo -{ - using Status = StrongTypedef; - using Key = std::string; - - const char * getName(Status event); - const char * getDocumentation(Status event); - const std::vector> & getAllPossibleValues(Status event); - - extern std::unordered_map values[]; - extern std::mutex locks[]; - - Status end(); - - inline void set(Status status, Key key, Int8 value) - { - std::lock_guard lock(locks[status]); - values[status][key] = value; - } - - inline void unset(Status status, Key key) - { - std::lock_guard lock(locks[status]); - values[status].erase(key); - } -} diff --git a/src/Interpreters/ExternalLoader.cpp b/src/Interpreters/ExternalLoader.cpp index 5dee750889c..56d480d8735 100644 --- a/src/Interpreters/ExternalLoader.cpp +++ b/src/Interpreters/ExternalLoader.cpp @@ -1,7 +1,6 @@ #include "ExternalLoader.h" #include -#include #include #include #include @@ -9,7 +8,6 @@ #include #include #include -#include #include #include #include @@ -18,12 +16,6 @@ #include -namespace CurrentStatusInfo -{ - extern const Status DictionaryStatus; -} - - namespace DB { namespace ErrorCodes @@ -1145,7 +1137,6 @@ private: if (info && (info->loading_id == loading_id)) { info->loading_id = info->state_id; - CurrentStatusInfo::set(CurrentStatusInfo::DictionaryStatus, name, static_cast(info->status())); } min_id_to_finish_loading_dependencies.erase(std::this_thread::get_id()); @@ -1307,7 +1298,6 @@ scope_guard ExternalLoader::addConfigRepository(std::unique_ptrremoveConfigRepository(ptr); - CurrentStatusInfo::unset(CurrentStatusInfo::DictionaryStatus, name); reloadConfig(name); }; } diff --git a/src/Server/PrometheusMetricsWriter.cpp b/src/Server/PrometheusMetricsWriter.cpp index 2331e455225..a7d90b9985c 100644 --- a/src/Server/PrometheusMetricsWriter.cpp +++ b/src/Server/PrometheusMetricsWriter.cpp @@ -1,7 +1,6 @@ #include "PrometheusMetricsWriter.h" #include -#include #include /// TODO: this library is harmful. #include @@ -51,7 +50,6 @@ PrometheusMetricsWriter::PrometheusMetricsWriter( , send_events(config.getBool(config_name + ".events", true)) , send_metrics(config.getBool(config_name + ".metrics", true)) , send_asynchronous_metrics(config.getBool(config_name + ".asynchronous_metrics", true)) - , send_status_info(config.getBool(config_name + ".status_info", true)) { } @@ -120,42 +118,6 @@ void PrometheusMetricsWriter::write(WriteBuffer & wb) const writeOutLine(wb, key, value.value); } } - - if (send_status_info) - { - for (size_t i = 0, end = CurrentStatusInfo::end(); i < end; ++i) - { - std::lock_guard lock(CurrentStatusInfo::locks[static_cast(i)]); - std::string metric_name{CurrentStatusInfo::getName(static_cast(i))}; - std::string metric_doc{CurrentStatusInfo::getDocumentation(static_cast(i))}; - - convertHelpToSingleLine(metric_doc); - - if (!replaceInvalidChars(metric_name)) - continue; - std::string key{current_status_prefix + metric_name}; - - writeOutLine(wb, "# HELP", key, metric_doc); - writeOutLine(wb, "# TYPE", key, "gauge"); - - for (const auto & value: CurrentStatusInfo::values[i]) - { - for (const auto & enum_value: CurrentStatusInfo::getAllPossibleValues(static_cast(i))) - { - DB::writeText(key, wb); - DB::writeChar('{', wb); - DB::writeText(key, wb); - DB::writeChar('=', wb); - writeDoubleQuotedString(enum_value.first, wb); - DB::writeText(",name=", wb); - writeDoubleQuotedString(value.first, wb); - DB::writeText("} ", wb); - DB::writeText(value.second == enum_value.second, wb); - DB::writeChar('\n', wb); - } - } - } - } } } diff --git a/src/Server/PrometheusMetricsWriter.h b/src/Server/PrometheusMetricsWriter.h index b4f6ab57def..b05eeaf0a3a 100644 --- a/src/Server/PrometheusMetricsWriter.h +++ b/src/Server/PrometheusMetricsWriter.h @@ -27,12 +27,10 @@ private: const bool send_events; const bool send_metrics; const bool send_asynchronous_metrics; - const bool send_status_info; static inline constexpr auto profile_events_prefix = "ClickHouseProfileEvents_"; static inline constexpr auto current_metrics_prefix = "ClickHouseMetrics_"; static inline constexpr auto asynchronous_metrics_prefix = "ClickHouseAsyncMetrics_"; - static inline constexpr auto current_status_prefix = "ClickHouseStatusInfo_"; }; } From 9d6dc1067f172187a739e0939e23acad0e631dc1 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 30 Aug 2023 20:49:02 +0200 Subject: [PATCH 035/101] Remove CurrentStatusInfo --- ...HouseStatusInfo_DictionaryStatus.reference | 18 --------- ...s_ClickHouseStatusInfo_DictionaryStatus.sh | 38 ------------------- 2 files changed, 56 deletions(-) delete mode 100644 tests/queries/0_stateless/02390_prometheus_ClickHouseStatusInfo_DictionaryStatus.reference delete mode 100755 tests/queries/0_stateless/02390_prometheus_ClickHouseStatusInfo_DictionaryStatus.sh diff --git a/tests/queries/0_stateless/02390_prometheus_ClickHouseStatusInfo_DictionaryStatus.reference b/tests/queries/0_stateless/02390_prometheus_ClickHouseStatusInfo_DictionaryStatus.reference deleted file mode 100644 index 50c91c3fa0c..00000000000 --- a/tests/queries/0_stateless/02390_prometheus_ClickHouseStatusInfo_DictionaryStatus.reference +++ /dev/null @@ -1,18 +0,0 @@ -status before reload -status after reload -NOT_LOADED 0 -LOADED 0 -FAILED 1 -LOADING 0 -FAILED_AND_RELOADING 0 -LOADED_AND_RELOADING 0 -NOT_EXIST 0 -status after reload, table exists -NOT_LOADED 0 -LOADED 1 -FAILED 0 -LOADING 0 -FAILED_AND_RELOADING 0 -LOADED_AND_RELOADING 0 -NOT_EXIST 0 -status after drop diff --git a/tests/queries/0_stateless/02390_prometheus_ClickHouseStatusInfo_DictionaryStatus.sh b/tests/queries/0_stateless/02390_prometheus_ClickHouseStatusInfo_DictionaryStatus.sh deleted file mode 100755 index 65025858e20..00000000000 --- a/tests/queries/0_stateless/02390_prometheus_ClickHouseStatusInfo_DictionaryStatus.sh +++ /dev/null @@ -1,38 +0,0 @@ -#!/usr/bin/env bash -# Tags: no-ordinary-database - -CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) -# shellcheck source=../shell_config.sh -. "$CUR_DIR"/../shell_config.sh - -function get_dictionary_status() -{ - local name=$1 && shift - $CLICKHOUSE_CURL -sS "$CLICKHOUSE_URL_PROMETHEUS" | { - awk -F'[{}=," ]' -vname="$name" '/ClickHouseStatusInfo_DictionaryStatus{/ && $(NF-3) == name { print $4, $NF }' - } -} - -$CLICKHOUSE_CLIENT -q "CREATE DICTIONARY dict (key Int, value String) PRIMARY KEY key SOURCE(CLICKHOUSE(TABLE data)) LAYOUT(HASHED()) LIFETIME(0)" -uuid="$($CLICKHOUSE_CLIENT -q "SELECT uuid FROM system.dictionaries WHERE database = '$CLICKHOUSE_DATABASE' AND name = 'dict'")" - -echo 'status before reload' -get_dictionary_status "$uuid" - -# source table does not exists -# NOTE: when dictionary does not exist it produce BAD_ARGUMENTS error, so using UNKNOWN_TABLE is safe -$CLICKHOUSE_CLIENT -n -q "SYSTEM RELOAD DICTIONARY dict -- { serverError UNKNOWN_TABLE }" -echo 'status after reload' -get_dictionary_status "$uuid" - -# create source -$CLICKHOUSE_CLIENT -q "CREATE TABLE data (key Int, value String) Engine=Null" -$CLICKHOUSE_CLIENT -q "SYSTEM RELOAD DICTIONARY dict" -echo 'status after reload, table exists' -get_dictionary_status "$uuid" - -# remove dictionary -$CLICKHOUSE_CLIENT -q "DROP DICTIONARY dict" -$CLICKHOUSE_CLIENT -q "DROP TABLE data" -echo 'status after drop' -get_dictionary_status "$uuid" From bb47e2cc234baab26d018f081cb257ff1d0b9cf0 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 30 Aug 2023 20:56:14 +0200 Subject: [PATCH 036/101] Remove CurrentStatusInfo --- .../testdata/configs/xml/config.xml | 21 ------------------- .../testdata/configs/yandex_xml/config.xml | 21 ------------------- programs/server/config.xml | 2 -- .../test_config_xml_full/configs/config.xml | 21 ------------------- 4 files changed, 65 deletions(-) diff --git a/programs/diagnostics/testdata/configs/xml/config.xml b/programs/diagnostics/testdata/configs/xml/config.xml index c08b0b2970f..50352ca800e 100644 --- a/programs/diagnostics/testdata/configs/xml/config.xml +++ b/programs/diagnostics/testdata/configs/xml/config.xml @@ -760,27 +760,6 @@ --> - - - - - - - - diff --git a/tests/integration/test_config_xml_full/configs/config.xml b/tests/integration/test_config_xml_full/configs/config.xml index d142df18af8..ac59b3428e8 100644 --- a/tests/integration/test_config_xml_full/configs/config.xml +++ b/tests/integration/test_config_xml_full/configs/config.xml @@ -674,27 +674,6 @@ --> - - - -