From 74bb7625a33d959e6bf93f918c8998ff3425faf6 Mon Sep 17 00:00:00 2001 From: laimuxi <1721261216@qq.com> Date: Sun, 12 Mar 2023 18:09:49 +0800 Subject: [PATCH 01/44] allow empty column names in CSVWithNames/TSVWithNames --- src/Interpreters/TreeRewriter.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/Interpreters/TreeRewriter.cpp b/src/Interpreters/TreeRewriter.cpp index 4c134e175dc..66a08257a46 100644 --- a/src/Interpreters/TreeRewriter.cpp +++ b/src/Interpreters/TreeRewriter.cpp @@ -919,6 +919,26 @@ void TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select RequiredSourceColumnsVisitor::Data columns_context; columns_context.visit_index_hint = visit_index_hint; + + if (auto * t = query->as()) + { + LOG_INFO(&Poco::Logger::get("TreeRewriter"), "lmx_00 enter"); + NameSet select_columns; + std::vector empty_name; + auto & select_query = *t; + for (size_t i = 0; i < select_query.select()->children.size(); i++) { + auto node = select_query.select()->children[i]; + if (auto* identifier = node->as()) { + LOG_INFO(&Poco::Logger::get("TreeRewriter"), "lmx_1 {}", identifier->name()); + if (identifier->name().empty()) { + empty_name.push_back(i); + select_query.select()->children.erase(select_query.select()->children.begin()+i); + } else { + select_columns.insert(identifier->name()); + } + } + } + } RequiredSourceColumnsVisitor(columns_context).visit(query); NameSet source_column_names; From 0ef6aea8280c3f8e7606dcb8e6d4b2186adabb68 Mon Sep 17 00:00:00 2001 From: laimuxi <1721261216@qq.com> Date: Sun, 12 Mar 2023 18:22:10 +0800 Subject: [PATCH 02/44] fix --- src/Interpreters/TreeRewriter.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/Interpreters/TreeRewriter.cpp b/src/Interpreters/TreeRewriter.cpp index 66a08257a46..1ac8b62c11c 100644 --- a/src/Interpreters/TreeRewriter.cpp +++ b/src/Interpreters/TreeRewriter.cpp @@ -922,19 +922,12 @@ void TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select if (auto * t = query->as()) { - LOG_INFO(&Poco::Logger::get("TreeRewriter"), "lmx_00 enter"); - NameSet select_columns; - std::vector empty_name; auto & select_query = *t; for (size_t i = 0; i < select_query.select()->children.size(); i++) { auto node = select_query.select()->children[i]; if (auto* identifier = node->as()) { - LOG_INFO(&Poco::Logger::get("TreeRewriter"), "lmx_1 {}", identifier->name()); if (identifier->name().empty()) { - empty_name.push_back(i); select_query.select()->children.erase(select_query.select()->children.begin()+i); - } else { - select_columns.insert(identifier->name()); } } } From b78e9dcc05d247d1ce9b88456e1edd808cc79c25 Mon Sep 17 00:00:00 2001 From: laimuxi <1721261216@qq.com> Date: Sun, 12 Mar 2023 22:45:25 +0800 Subject: [PATCH 03/44] fix style --- src/Interpreters/TreeRewriter.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Interpreters/TreeRewriter.cpp b/src/Interpreters/TreeRewriter.cpp index 1ac8b62c11c..81da53756ec 100644 --- a/src/Interpreters/TreeRewriter.cpp +++ b/src/Interpreters/TreeRewriter.cpp @@ -923,9 +923,9 @@ void TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select if (auto * t = query->as()) { auto & select_query = *t; - for (size_t i = 0; i < select_query.select()->children.size(); i++) { + for (size_t i = 0; i < select_query.select()->children.size(); ++i) { auto node = select_query.select()->children[i]; - if (auto* identifier = node->as()) { + if (auto * identifier = node->as()) { if (identifier->name().empty()) { select_query.select()->children.erase(select_query.select()->children.begin()+i); } From 76f4ca0e8e60052801f24e5b3ff659b750cef04e Mon Sep 17 00:00:00 2001 From: laimuxi <1721261216@qq.com> Date: Sun, 12 Mar 2023 23:14:56 +0800 Subject: [PATCH 04/44] fix style --- src/Interpreters/TreeRewriter.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Interpreters/TreeRewriter.cpp b/src/Interpreters/TreeRewriter.cpp index 81da53756ec..95d277b490d 100644 --- a/src/Interpreters/TreeRewriter.cpp +++ b/src/Interpreters/TreeRewriter.cpp @@ -926,9 +926,8 @@ void TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select for (size_t i = 0; i < select_query.select()->children.size(); ++i) { auto node = select_query.select()->children[i]; if (auto * identifier = node->as()) { - if (identifier->name().empty()) { + if (identifier->name().empty()) select_query.select()->children.erase(select_query.select()->children.begin()+i); - } } } } From 48a1f1c838ae57e04ec8fb1f84dcd50ca2649908 Mon Sep 17 00:00:00 2001 From: laimuxi <1721261216@qq.com> Date: Sun, 12 Mar 2023 23:49:23 +0800 Subject: [PATCH 05/44] fix style --- src/Interpreters/TreeRewriter.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/Interpreters/TreeRewriter.cpp b/src/Interpreters/TreeRewriter.cpp index 95d277b490d..83886dd1cb0 100644 --- a/src/Interpreters/TreeRewriter.cpp +++ b/src/Interpreters/TreeRewriter.cpp @@ -923,13 +923,10 @@ void TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select if (auto * t = query->as()) { auto & select_query = *t; - for (size_t i = 0; i < select_query.select()->children.size(); ++i) { - auto node = select_query.select()->children[i]; - if (auto * identifier = node->as()) { + for (size_t i = 0; i < select_query.select()->children.size(); ++i) + if (auto * identifier = select_query.select()->children[i]->as()) if (identifier->name().empty()) select_query.select()->children.erase(select_query.select()->children.begin()+i); - } - } } RequiredSourceColumnsVisitor(columns_context).visit(query); From 17efdbf6251e51d945dd1df598a6526c465fe99b Mon Sep 17 00:00:00 2001 From: laimuxi <1721261216@qq.com> Date: Fri, 31 Mar 2023 21:56:35 +0800 Subject: [PATCH 06/44] change --- src/Formats/ReadSchemaUtils.cpp | 374 +++++++++++++++--------------- src/Interpreters/TreeRewriter.cpp | 9 - 2 files changed, 182 insertions(+), 201 deletions(-) diff --git a/src/Formats/ReadSchemaUtils.cpp b/src/Formats/ReadSchemaUtils.cpp index 653efd4f5c1..f12eb8cb71e 100644 --- a/src/Formats/ReadSchemaUtils.cpp +++ b/src/Formats/ReadSchemaUtils.cpp @@ -9,223 +9,213 @@ #include #include -namespace DB -{ +namespace DB { -namespace ErrorCodes -{ - extern const int EMPTY_DATA_PASSED; - extern const int BAD_ARGUMENTS; - extern const int ONLY_NULLS_WHILE_READING_SCHEMA; - extern const int CANNOT_EXTRACT_TABLE_STRUCTURE; -} + namespace ErrorCodes { + extern const int EMPTY_DATA_PASSED; + extern const int BAD_ARGUMENTS; + extern const int ONLY_NULLS_WHILE_READING_SCHEMA; + extern const int CANNOT_EXTRACT_TABLE_STRUCTURE; + } -static std::optional getOrderedColumnsList( - const NamesAndTypesList & columns_list, const Names & columns_order_hint) -{ - if (columns_list.size() != columns_order_hint.size()) - return {}; - - std::unordered_map available_columns; - for (const auto & [name, type] : columns_list) - available_columns.emplace(name, type); - - NamesAndTypesList res; - for (const auto & name : columns_order_hint) - { - auto it = available_columns.find(name); - if (it == available_columns.end()) + static std::optional getOrderedColumnsList( + const NamesAndTypesList &columns_list, const Names &columns_order_hint) { + if (columns_list.size() != columns_order_hint.size()) return {}; - res.emplace_back(name, it->second); - } - return res; -} + std::unordered_map available_columns; + for (const auto &[name, type]: columns_list) + available_columns.emplace(name, type); -bool isRetryableSchemaInferenceError(int code) -{ - return code == ErrorCodes::EMPTY_DATA_PASSED || code == ErrorCodes::ONLY_NULLS_WHILE_READING_SCHEMA; -} + NamesAndTypesList res; + for (const auto &name: columns_order_hint) { + auto it = available_columns.find(name); + if (it == available_columns.end()) + return {}; -ColumnsDescription readSchemaFromFormat( - const String & format_name, - const std::optional & format_settings, - ReadBufferIterator & read_buffer_iterator, - bool retry, - ContextPtr & context, - std::unique_ptr & buf) -{ - NamesAndTypesList names_and_types; - if (FormatFactory::instance().checkIfFormatHasExternalSchemaReader(format_name)) - { - auto external_schema_reader = FormatFactory::instance().getExternalSchemaReader(format_name, context, format_settings); - try - { - names_and_types = external_schema_reader->readSchema(); - } - catch (Exception & e) - { - e.addMessage(fmt::format("Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); - throw; + res.emplace_back(name, it->second); } + return res; } - else if (FormatFactory::instance().checkIfFormatHasSchemaReader(format_name)) - { - std::string exception_messages; - SchemaReaderPtr schema_reader; - size_t max_rows_to_read = format_settings ? format_settings->max_rows_to_read_for_schema_inference : context->getSettingsRef().input_format_max_rows_to_read_for_schema_inference; - size_t iterations = 0; - ColumnsDescription cached_columns; - while (true) - { - bool is_eof = false; - try - { - buf = read_buffer_iterator(cached_columns); - if (!buf) - break; - is_eof = buf->eof(); + + bool isRetryableSchemaInferenceError(int code) { + return code == ErrorCodes::EMPTY_DATA_PASSED || code == ErrorCodes::ONLY_NULLS_WHILE_READING_SCHEMA; + } + + ColumnsDescription readSchemaFromFormat( + const String &format_name, + const std::optional &format_settings, + ReadBufferIterator &read_buffer_iterator, + bool retry, + ContextPtr &context, + std::unique_ptr &buf) { + NamesAndTypesList names_and_types; + if (FormatFactory::instance().checkIfFormatHasExternalSchemaReader(format_name)) { + auto external_schema_reader = FormatFactory::instance().getExternalSchemaReader(format_name, context, + format_settings); + try { + names_and_types = external_schema_reader->readSchema(); } - catch (Exception & e) - { + catch (Exception &e) { e.addMessage(fmt::format( - "Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); + "Cannot extract table structure from {} format file. You can specify the structure manually", + format_name)); throw; } - catch (...) - { - auto exception_message = getCurrentExceptionMessage(false); - throw Exception( - ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, - "Cannot extract table structure from {} format file:\n{}\nYou can specify the structure manually", - format_name, - exception_message); - } - - ++iterations; - - if (is_eof) - { - auto exception_message = fmt::format("Cannot extract table structure from {} format file, file is empty", format_name); - - if (!retry) - throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, "{}. You can specify the structure manually", exception_message); - - exception_messages += "\n" + exception_message; - continue; - } - - try - { - schema_reader = FormatFactory::instance().getSchemaReader(format_name, *buf, context, format_settings); - schema_reader->setMaxRowsToRead(max_rows_to_read); - names_and_types = schema_reader->readSchema(); - break; - } - catch (...) - { - auto exception_message = getCurrentExceptionMessage(false); - if (schema_reader) - { - size_t rows_read = schema_reader->getNumRowsRead(); - assert(rows_read <= max_rows_to_read); - max_rows_to_read -= schema_reader->getNumRowsRead(); - if (rows_read != 0 && max_rows_to_read == 0) - { - exception_message += "\nTo increase the maximum number of rows to read for structure determination, use setting input_format_max_rows_to_read_for_schema_inference"; - if (iterations > 1) - { - exception_messages += "\n" + exception_message; - break; - } - retry = false; - } + } else if (FormatFactory::instance().checkIfFormatHasSchemaReader(format_name)) { + std::string exception_messages; + SchemaReaderPtr schema_reader; + size_t max_rows_to_read = format_settings ? format_settings->max_rows_to_read_for_schema_inference + : context->getSettingsRef().input_format_max_rows_to_read_for_schema_inference; + size_t iterations = 0; + ColumnsDescription cached_columns; + while (true) { + bool is_eof = false; + try { + buf = read_buffer_iterator(cached_columns); + if (!buf) + break; + is_eof = buf->eof(); + } + catch (Exception &e) { + e.addMessage(fmt::format( + "Cannot extract table structure from {} format file. You can specify the structure manually", + format_name)); + throw; + } + catch (...) { + auto exception_message = getCurrentExceptionMessage(false); + throw Exception( + ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, + "Cannot extract table structure from {} format file:\n{}\nYou can specify the structure manually", + format_name, + exception_message); } - if (!retry || !isRetryableSchemaInferenceError(getCurrentExceptionCode())) - { - try - { - throw; - } - catch (Exception & e) - { - e.addMessage(fmt::format("Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); - throw; - } - catch (...) - { + ++iterations; + + if (is_eof) { + auto exception_message = fmt::format( + "Cannot extract table structure from {} format file, file is empty", format_name); + + if (!retry) throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, - "Cannot extract table structure from {} format file. " - "Error: {}. You can specify the structure manually", - format_name, exception_message); - } + "{}. You can specify the structure manually", exception_message); + + exception_messages += "\n" + exception_message; + continue; } - exception_messages += "\n" + exception_message; + try { + schema_reader = FormatFactory::instance().getSchemaReader(format_name, *buf, context, + format_settings); + schema_reader->setMaxRowsToRead(max_rows_to_read); + names_and_types = schema_reader->readSchema(); + break; + } + catch (...) { + auto exception_message = getCurrentExceptionMessage(false); + if (schema_reader) { + size_t rows_read = schema_reader->getNumRowsRead(); + assert(rows_read <= max_rows_to_read); + max_rows_to_read -= schema_reader->getNumRowsRead(); + if (rows_read != 0 && max_rows_to_read == 0) { + exception_message += "\nTo increase the maximum number of rows to read for structure determination, use setting input_format_max_rows_to_read_for_schema_inference"; + if (iterations > 1) { + exception_messages += "\n" + exception_message; + break; + } + retry = false; + } + } + + if (!retry || !isRetryableSchemaInferenceError(getCurrentExceptionCode())) { + try { + throw; + } + catch (Exception &e) { + e.addMessage(fmt::format( + "Cannot extract table structure from {} format file. You can specify the structure manually", + format_name)); + throw; + } + catch (...) { + throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, + "Cannot extract table structure from {} format file. " + "Error: {}. You can specify the structure manually", + format_name, exception_message); + } + } + + exception_messages += "\n" + exception_message; + } } - } - if (!cached_columns.empty()) - return cached_columns; + if (!cached_columns.empty()) + return cached_columns; - if (names_and_types.empty()) - throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, - "All attempts to extract table structure from files failed. " - "Errors:{}\nYou can specify the structure manually", exception_messages); + if (names_and_types.empty()) + throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, + "All attempts to extract table structure from files failed. " + "Errors:{}\nYou can specify the structure manually", exception_messages); - /// If we have "INSERT SELECT" query then try to order - /// columns as they are ordered in table schema for formats - /// without strict column order (like JSON and TSKV). - /// It will allow to execute simple data loading with query - /// "INSERT INTO table SELECT * FROM ..." - const auto & insertion_table = context->getInsertionTable(); - if (!schema_reader->hasStrictOrderOfColumns() && !insertion_table.empty()) - { - auto storage = DatabaseCatalog::instance().getTable(insertion_table, context); - auto metadata = storage->getInMemoryMetadataPtr(); - auto names_in_storage = metadata->getColumns().getNamesOfPhysical(); - auto ordered_list = getOrderedColumnsList(names_and_types, names_in_storage); - if (ordered_list) - names_and_types = *ordered_list; - } + /// If we have "INSERT SELECT" query then try to order + /// columns as they are ordered in table schema for formats + /// without strict column order (like JSON and TSKV). + /// It will allow to execute simple data loading with query + /// "INSERT INTO table SELECT * FROM ..." + const auto &insertion_table = context->getInsertionTable(); + if (!schema_reader->hasStrictOrderOfColumns() && !insertion_table.empty()) { + auto storage = DatabaseCatalog::instance().getTable(insertion_table, context); + auto metadata = storage->getInMemoryMetadataPtr(); + auto names_in_storage = metadata->getColumns().getNamesOfPhysical(); + auto ordered_list = getOrderedColumnsList(names_and_types, names_in_storage); + if (ordered_list) + names_and_types = *ordered_list; + } + } else + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "{} file format doesn't support schema inference. You must specify the structure manually", + format_name); + names_and_types.erase(std::remove_if(names_and_types.begin(), names_and_types.end(), + [](const NameAndTypePair &pair) { return pair.name.empty(); }), + names_and_types.end()); + return ColumnsDescription(names_and_types); } - else - throw Exception(ErrorCodes::BAD_ARGUMENTS, - "{} file format doesn't support schema inference. You must specify the structure manually", - format_name); - return ColumnsDescription(names_and_types); -} + ColumnsDescription + readSchemaFromFormat(const String &format_name, const std::optional &format_settings, + ReadBufferIterator &read_buffer_iterator, bool retry, ContextPtr &context) { + std::unique_ptr buf_out; + return readSchemaFromFormat(format_name, format_settings, read_buffer_iterator, retry, context, buf_out); + } -ColumnsDescription readSchemaFromFormat(const String & format_name, const std::optional & format_settings, ReadBufferIterator & read_buffer_iterator, bool retry, ContextPtr & context) -{ - std::unique_ptr buf_out; - return readSchemaFromFormat(format_name, format_settings, read_buffer_iterator, retry, context, buf_out); -} + SchemaCache::Key getKeyForSchemaCache(const String &source, const String &format, + const std::optional &format_settings, + const ContextPtr &context) { + return getKeysForSchemaCache({source}, format, format_settings, context).front(); + } -SchemaCache::Key getKeyForSchemaCache(const String & source, const String & format, const std::optional & format_settings, const ContextPtr & context) -{ - return getKeysForSchemaCache({source}, format, format_settings, context).front(); -} + static SchemaCache::Key + makeSchemaCacheKey(const String &source, const String &format, const String &additional_format_info) { + return SchemaCache::Key{source, format, additional_format_info}; + } -static SchemaCache::Key makeSchemaCacheKey(const String & source, const String & format, const String & additional_format_info) -{ - return SchemaCache::Key{source, format, additional_format_info}; -} - -SchemaCache::Keys getKeysForSchemaCache(const Strings & sources, const String & format, const std::optional & format_settings, const ContextPtr & context) -{ - /// For some formats data schema depends on some settings, so it's possible that - /// two queries to the same source will get two different schemas. To process this - /// case we add some additional information specific for the format to the cache key. - /// For example, for Protobuf format additional information is the path to the schema - /// and message name. - String additional_format_info = FormatFactory::instance().getAdditionalInfoForSchemaCache(format, context, format_settings); - SchemaCache::Keys cache_keys; - cache_keys.reserve(sources.size()); - std::transform(sources.begin(), sources.end(), std::back_inserter(cache_keys), [&](const auto & source){ return makeSchemaCacheKey(source, format, additional_format_info); }); - return cache_keys; -} + SchemaCache::Keys getKeysForSchemaCache(const Strings &sources, const String &format, + const std::optional &format_settings, + const ContextPtr &context) { + /// For some formats data schema depends on some settings, so it's possible that + /// two queries to the same source will get two different schemas. To process this + /// case we add some additional information specific for the format to the cache key. + /// For example, for Protobuf format additional information is the path to the schema + /// and message name. + String additional_format_info = FormatFactory::instance().getAdditionalInfoForSchemaCache(format, context, + format_settings); + SchemaCache::Keys cache_keys; + cache_keys.reserve(sources.size()); + std::transform(sources.begin(), sources.end(), std::back_inserter(cache_keys), + [&](const auto &source) { return makeSchemaCacheKey(source, format, additional_format_info); }); + return cache_keys; + } } diff --git a/src/Interpreters/TreeRewriter.cpp b/src/Interpreters/TreeRewriter.cpp index 83886dd1cb0..4c134e175dc 100644 --- a/src/Interpreters/TreeRewriter.cpp +++ b/src/Interpreters/TreeRewriter.cpp @@ -919,15 +919,6 @@ void TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select RequiredSourceColumnsVisitor::Data columns_context; columns_context.visit_index_hint = visit_index_hint; - - if (auto * t = query->as()) - { - auto & select_query = *t; - for (size_t i = 0; i < select_query.select()->children.size(); ++i) - if (auto * identifier = select_query.select()->children[i]->as()) - if (identifier->name().empty()) - select_query.select()->children.erase(select_query.select()->children.begin()+i); - } RequiredSourceColumnsVisitor(columns_context).visit(query); NameSet source_column_names; From 3b756ef0261d33a9c8771cad7906f690db11bf75 Mon Sep 17 00:00:00 2001 From: laimuxi <1721261216@qq.com> Date: Fri, 31 Mar 2023 21:58:20 +0800 Subject: [PATCH 07/44] rollback --- src/Formats/ReadSchemaUtils.cpp | 375 ++++++++++++++++---------------- 1 file changed, 193 insertions(+), 182 deletions(-) diff --git a/src/Formats/ReadSchemaUtils.cpp b/src/Formats/ReadSchemaUtils.cpp index f12eb8cb71e..7f1c3bf3e63 100644 --- a/src/Formats/ReadSchemaUtils.cpp +++ b/src/Formats/ReadSchemaUtils.cpp @@ -9,213 +9,224 @@ #include #include -namespace DB { +namespace DB +{ - namespace ErrorCodes { - extern const int EMPTY_DATA_PASSED; - extern const int BAD_ARGUMENTS; - extern const int ONLY_NULLS_WHILE_READING_SCHEMA; - extern const int CANNOT_EXTRACT_TABLE_STRUCTURE; - } +namespace ErrorCodes +{ + extern const int EMPTY_DATA_PASSED; + extern const int BAD_ARGUMENTS; + extern const int ONLY_NULLS_WHILE_READING_SCHEMA; + extern const int CANNOT_EXTRACT_TABLE_STRUCTURE; +} - static std::optional getOrderedColumnsList( - const NamesAndTypesList &columns_list, const Names &columns_order_hint) { - if (columns_list.size() != columns_order_hint.size()) +static std::optional getOrderedColumnsList( + const NamesAndTypesList & columns_list, const Names & columns_order_hint) +{ + if (columns_list.size() != columns_order_hint.size()) + return {}; + + std::unordered_map available_columns; + for (const auto & [name, type] : columns_list) + available_columns.emplace(name, type); + + NamesAndTypesList res; + for (const auto & name : columns_order_hint) + { + auto it = available_columns.find(name); + if (it == available_columns.end()) return {}; - std::unordered_map available_columns; - for (const auto &[name, type]: columns_list) - available_columns.emplace(name, type); + res.emplace_back(name, it->second); + } + return res; +} - NamesAndTypesList res; - for (const auto &name: columns_order_hint) { - auto it = available_columns.find(name); - if (it == available_columns.end()) - return {}; +bool isRetryableSchemaInferenceError(int code) +{ + return code == ErrorCodes::EMPTY_DATA_PASSED || code == ErrorCodes::ONLY_NULLS_WHILE_READING_SCHEMA; +} - res.emplace_back(name, it->second); +ColumnsDescription readSchemaFromFormat( + const String & format_name, + const std::optional & format_settings, + ReadBufferIterator & read_buffer_iterator, + bool retry, + ContextPtr & context, + std::unique_ptr & buf) +{ + NamesAndTypesList names_and_types; + if (FormatFactory::instance().checkIfFormatHasExternalSchemaReader(format_name)) + { + auto external_schema_reader = FormatFactory::instance().getExternalSchemaReader(format_name, context, format_settings); + try + { + names_and_types = external_schema_reader->readSchema(); + } + catch (Exception & e) + { + e.addMessage(fmt::format("Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); + throw; } - return res; } - - bool isRetryableSchemaInferenceError(int code) { - return code == ErrorCodes::EMPTY_DATA_PASSED || code == ErrorCodes::ONLY_NULLS_WHILE_READING_SCHEMA; - } - - ColumnsDescription readSchemaFromFormat( - const String &format_name, - const std::optional &format_settings, - ReadBufferIterator &read_buffer_iterator, - bool retry, - ContextPtr &context, - std::unique_ptr &buf) { - NamesAndTypesList names_and_types; - if (FormatFactory::instance().checkIfFormatHasExternalSchemaReader(format_name)) { - auto external_schema_reader = FormatFactory::instance().getExternalSchemaReader(format_name, context, - format_settings); - try { - names_and_types = external_schema_reader->readSchema(); + else if (FormatFactory::instance().checkIfFormatHasSchemaReader(format_name)) + { + std::string exception_messages; + SchemaReaderPtr schema_reader; + size_t max_rows_to_read = format_settings ? format_settings->max_rows_to_read_for_schema_inference : context->getSettingsRef().input_format_max_rows_to_read_for_schema_inference; + size_t iterations = 0; + ColumnsDescription cached_columns; + while (true) + { + bool is_eof = false; + try + { + buf = read_buffer_iterator(cached_columns); + if (!buf) + break; + is_eof = buf->eof(); } - catch (Exception &e) { + catch (Exception & e) + { e.addMessage(fmt::format( - "Cannot extract table structure from {} format file. You can specify the structure manually", - format_name)); + "Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); throw; } - } else if (FormatFactory::instance().checkIfFormatHasSchemaReader(format_name)) { - std::string exception_messages; - SchemaReaderPtr schema_reader; - size_t max_rows_to_read = format_settings ? format_settings->max_rows_to_read_for_schema_inference - : context->getSettingsRef().input_format_max_rows_to_read_for_schema_inference; - size_t iterations = 0; - ColumnsDescription cached_columns; - while (true) { - bool is_eof = false; - try { - buf = read_buffer_iterator(cached_columns); - if (!buf) - break; - is_eof = buf->eof(); - } - catch (Exception &e) { - e.addMessage(fmt::format( - "Cannot extract table structure from {} format file. You can specify the structure manually", - format_name)); - throw; - } - catch (...) { - auto exception_message = getCurrentExceptionMessage(false); - throw Exception( - ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, - "Cannot extract table structure from {} format file:\n{}\nYou can specify the structure manually", - format_name, - exception_message); + catch (...) + { + auto exception_message = getCurrentExceptionMessage(false); + throw Exception( + ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, + "Cannot extract table structure from {} format file:\n{}\nYou can specify the structure manually", + format_name, + exception_message); + } + + ++iterations; + + if (is_eof) + { + auto exception_message = fmt::format("Cannot extract table structure from {} format file, file is empty", format_name); + + if (!retry) + throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, "{}. You can specify the structure manually", exception_message); + + exception_messages += "\n" + exception_message; + continue; + } + + try + { + schema_reader = FormatFactory::instance().getSchemaReader(format_name, *buf, context, format_settings); + schema_reader->setMaxRowsToRead(max_rows_to_read); + names_and_types = schema_reader->readSchema(); + break; + } + catch (...) + { + auto exception_message = getCurrentExceptionMessage(false); + if (schema_reader) + { + size_t rows_read = schema_reader->getNumRowsRead(); + assert(rows_read <= max_rows_to_read); + max_rows_to_read -= schema_reader->getNumRowsRead(); + if (rows_read != 0 && max_rows_to_read == 0) + { + exception_message += "\nTo increase the maximum number of rows to read for structure determination, use setting input_format_max_rows_to_read_for_schema_inference"; + if (iterations > 1) + { + exception_messages += "\n" + exception_message; + break; + } + retry = false; + } } - ++iterations; - - if (is_eof) { - auto exception_message = fmt::format( - "Cannot extract table structure from {} format file, file is empty", format_name); - - if (!retry) + if (!retry || !isRetryableSchemaInferenceError(getCurrentExceptionCode())) + { + try + { + throw; + } + catch (Exception & e) + { + e.addMessage(fmt::format("Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); + throw; + } + catch (...) + { throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, - "{}. You can specify the structure manually", exception_message); - - exception_messages += "\n" + exception_message; - continue; - } - - try { - schema_reader = FormatFactory::instance().getSchemaReader(format_name, *buf, context, - format_settings); - schema_reader->setMaxRowsToRead(max_rows_to_read); - names_and_types = schema_reader->readSchema(); - break; - } - catch (...) { - auto exception_message = getCurrentExceptionMessage(false); - if (schema_reader) { - size_t rows_read = schema_reader->getNumRowsRead(); - assert(rows_read <= max_rows_to_read); - max_rows_to_read -= schema_reader->getNumRowsRead(); - if (rows_read != 0 && max_rows_to_read == 0) { - exception_message += "\nTo increase the maximum number of rows to read for structure determination, use setting input_format_max_rows_to_read_for_schema_inference"; - if (iterations > 1) { - exception_messages += "\n" + exception_message; - break; - } - retry = false; - } + "Cannot extract table structure from {} format file. " + "Error: {}. You can specify the structure manually", + format_name, exception_message); } - - if (!retry || !isRetryableSchemaInferenceError(getCurrentExceptionCode())) { - try { - throw; - } - catch (Exception &e) { - e.addMessage(fmt::format( - "Cannot extract table structure from {} format file. You can specify the structure manually", - format_name)); - throw; - } - catch (...) { - throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, - "Cannot extract table structure from {} format file. " - "Error: {}. You can specify the structure manually", - format_name, exception_message); - } - } - - exception_messages += "\n" + exception_message; } + + exception_messages += "\n" + exception_message; } + } - if (!cached_columns.empty()) - return cached_columns; + if (!cached_columns.empty()) + return cached_columns; - if (names_and_types.empty()) - throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, - "All attempts to extract table structure from files failed. " - "Errors:{}\nYou can specify the structure manually", exception_messages); + if (names_and_types.empty()) + throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, + "All attempts to extract table structure from files failed. " + "Errors:{}\nYou can specify the structure manually", exception_messages); - /// If we have "INSERT SELECT" query then try to order - /// columns as they are ordered in table schema for formats - /// without strict column order (like JSON and TSKV). - /// It will allow to execute simple data loading with query - /// "INSERT INTO table SELECT * FROM ..." - const auto &insertion_table = context->getInsertionTable(); - if (!schema_reader->hasStrictOrderOfColumns() && !insertion_table.empty()) { - auto storage = DatabaseCatalog::instance().getTable(insertion_table, context); - auto metadata = storage->getInMemoryMetadataPtr(); - auto names_in_storage = metadata->getColumns().getNamesOfPhysical(); - auto ordered_list = getOrderedColumnsList(names_and_types, names_in_storage); - if (ordered_list) - names_and_types = *ordered_list; - } - } else - throw Exception(ErrorCodes::BAD_ARGUMENTS, - "{} file format doesn't support schema inference. You must specify the structure manually", - format_name); - names_and_types.erase(std::remove_if(names_and_types.begin(), names_and_types.end(), - [](const NameAndTypePair &pair) { return pair.name.empty(); }), - names_and_types.end()); - return ColumnsDescription(names_and_types); + /// If we have "INSERT SELECT" query then try to order + /// columns as they are ordered in table schema for formats + /// without strict column order (like JSON and TSKV). + /// It will allow to execute simple data loading with query + /// "INSERT INTO table SELECT * FROM ..." + const auto & insertion_table = context->getInsertionTable(); + if (!schema_reader->hasStrictOrderOfColumns() && !insertion_table.empty()) + { + auto storage = DatabaseCatalog::instance().getTable(insertion_table, context); + auto metadata = storage->getInMemoryMetadataPtr(); + auto names_in_storage = metadata->getColumns().getNamesOfPhysical(); + auto ordered_list = getOrderedColumnsList(names_and_types, names_in_storage); + if (ordered_list) + names_and_types = *ordered_list; + } } + else + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "{} file format doesn't support schema inference. You must specify the structure manually", + format_name); + names_and_types.erase(std::remove_if(names_and_types.begin(), names_and_types.end(), + [](const NameAndTypePair& pair) { return pair.name.empty(); }), names_and_types.end()); + return ColumnsDescription(names_and_types); +} - ColumnsDescription - readSchemaFromFormat(const String &format_name, const std::optional &format_settings, - ReadBufferIterator &read_buffer_iterator, bool retry, ContextPtr &context) { - std::unique_ptr buf_out; - return readSchemaFromFormat(format_name, format_settings, read_buffer_iterator, retry, context, buf_out); - } +ColumnsDescription readSchemaFromFormat(const String & format_name, const std::optional & format_settings, ReadBufferIterator & read_buffer_iterator, bool retry, ContextPtr & context) +{ + std::unique_ptr buf_out; + return readSchemaFromFormat(format_name, format_settings, read_buffer_iterator, retry, context, buf_out); +} - SchemaCache::Key getKeyForSchemaCache(const String &source, const String &format, - const std::optional &format_settings, - const ContextPtr &context) { - return getKeysForSchemaCache({source}, format, format_settings, context).front(); - } +SchemaCache::Key getKeyForSchemaCache(const String & source, const String & format, const std::optional & format_settings, const ContextPtr & context) +{ + return getKeysForSchemaCache({source}, format, format_settings, context).front(); +} - static SchemaCache::Key - makeSchemaCacheKey(const String &source, const String &format, const String &additional_format_info) { - return SchemaCache::Key{source, format, additional_format_info}; - } +static SchemaCache::Key makeSchemaCacheKey(const String & source, const String & format, const String & additional_format_info) +{ + return SchemaCache::Key{source, format, additional_format_info}; +} - SchemaCache::Keys getKeysForSchemaCache(const Strings &sources, const String &format, - const std::optional &format_settings, - const ContextPtr &context) { - /// For some formats data schema depends on some settings, so it's possible that - /// two queries to the same source will get two different schemas. To process this - /// case we add some additional information specific for the format to the cache key. - /// For example, for Protobuf format additional information is the path to the schema - /// and message name. - String additional_format_info = FormatFactory::instance().getAdditionalInfoForSchemaCache(format, context, - format_settings); - SchemaCache::Keys cache_keys; - cache_keys.reserve(sources.size()); - std::transform(sources.begin(), sources.end(), std::back_inserter(cache_keys), - [&](const auto &source) { return makeSchemaCacheKey(source, format, additional_format_info); }); - return cache_keys; - } +SchemaCache::Keys getKeysForSchemaCache(const Strings & sources, const String & format, const std::optional & format_settings, const ContextPtr & context) +{ + /// For some formats data schema depends on some settings, so it's possible that + /// two queries to the same source will get two different schemas. To process this + /// case we add some additional information specific for the format to the cache key. + /// For example, for Protobuf format additional information is the path to the schema + /// and message name. + String additional_format_info = FormatFactory::instance().getAdditionalInfoForSchemaCache(format, context, format_settings); + SchemaCache::Keys cache_keys; + cache_keys.reserve(sources.size()); + std::transform(sources.begin(), sources.end(), std::back_inserter(cache_keys), [&](const auto & source){ return makeSchemaCacheKey(source, format, additional_format_info); }); + return cache_keys; +} } From b869572a5411ce22519152a9a650d7fa65c8c517 Mon Sep 17 00:00:00 2001 From: laimuxi <1721261216@qq.com> Date: Sat, 1 Apr 2023 15:20:26 +0800 Subject: [PATCH 08/44] reformat code --- src/Formats/ReadSchemaUtils.cpp | 81 +++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 30 deletions(-) diff --git a/src/Formats/ReadSchemaUtils.cpp b/src/Formats/ReadSchemaUtils.cpp index 7f1c3bf3e63..d185b938ed6 100644 --- a/src/Formats/ReadSchemaUtils.cpp +++ b/src/Formats/ReadSchemaUtils.cpp @@ -1,13 +1,13 @@ -#include #include -#include -#include #include +#include +#include +#include #include -#include -#include #include +#include #include +#include namespace DB { @@ -20,8 +20,7 @@ namespace ErrorCodes extern const int CANNOT_EXTRACT_TABLE_STRUCTURE; } -static std::optional getOrderedColumnsList( - const NamesAndTypesList & columns_list, const Names & columns_order_hint) +static std::optional getOrderedColumnsList(const NamesAndTypesList & columns_list, const Names & columns_order_hint) { if (columns_list.size() != columns_order_hint.size()) return {}; @@ -65,7 +64,8 @@ ColumnsDescription readSchemaFromFormat( } catch (Exception & e) { - e.addMessage(fmt::format("Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); + e.addMessage( + fmt::format("Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); throw; } } @@ -73,7 +73,8 @@ ColumnsDescription readSchemaFromFormat( { std::string exception_messages; SchemaReaderPtr schema_reader; - size_t max_rows_to_read = format_settings ? format_settings->max_rows_to_read_for_schema_inference : context->getSettingsRef().input_format_max_rows_to_read_for_schema_inference; + size_t max_rows_to_read = format_settings ? format_settings->max_rows_to_read_for_schema_inference + : context->getSettingsRef().input_format_max_rows_to_read_for_schema_inference; size_t iterations = 0; ColumnsDescription cached_columns; while (true) @@ -88,8 +89,8 @@ ColumnsDescription readSchemaFromFormat( } catch (Exception & e) { - e.addMessage(fmt::format( - "Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); + e.addMessage( + fmt::format("Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); throw; } catch (...) @@ -109,7 +110,8 @@ ColumnsDescription readSchemaFromFormat( auto exception_message = fmt::format("Cannot extract table structure from {} format file, file is empty", format_name); if (!retry) - throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, "{}. You can specify the structure manually", exception_message); + throw Exception( + ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, "{}. You can specify the structure manually", exception_message); exception_messages += "\n" + exception_message; continue; @@ -132,7 +134,8 @@ ColumnsDescription readSchemaFromFormat( max_rows_to_read -= schema_reader->getNumRowsRead(); if (rows_read != 0 && max_rows_to_read == 0) { - exception_message += "\nTo increase the maximum number of rows to read for structure determination, use setting input_format_max_rows_to_read_for_schema_inference"; + exception_message += "\nTo increase the maximum number of rows to read for structure determination, use setting " + "input_format_max_rows_to_read_for_schema_inference"; if (iterations > 1) { exception_messages += "\n" + exception_message; @@ -150,15 +153,18 @@ ColumnsDescription readSchemaFromFormat( } catch (Exception & e) { - e.addMessage(fmt::format("Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); + e.addMessage(fmt::format( + "Cannot extract table structure from {} format file. You can specify the structure manually", format_name)); throw; } catch (...) { - throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, - "Cannot extract table structure from {} format file. " - "Error: {}. You can specify the structure manually", - format_name, exception_message); + throw Exception( + ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, + "Cannot extract table structure from {} format file. " + "Error: {}. You can specify the structure manually", + format_name, + exception_message); } } @@ -170,9 +176,11 @@ ColumnsDescription readSchemaFromFormat( return cached_columns; if (names_and_types.empty()) - throw Exception(ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, - "All attempts to extract table structure from files failed. " - "Errors:{}\nYou can specify the structure manually", exception_messages); + throw Exception( + ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE, + "All attempts to extract table structure from files failed. " + "Errors:{}\nYou can specify the structure manually", + exception_messages); /// If we have "INSERT SELECT" query then try to order /// columns as they are ordered in table schema for formats @@ -191,21 +199,29 @@ ColumnsDescription readSchemaFromFormat( } } else - throw Exception(ErrorCodes::BAD_ARGUMENTS, - "{} file format doesn't support schema inference. You must specify the structure manually", - format_name); - names_and_types.erase(std::remove_if(names_and_types.begin(), names_and_types.end(), - [](const NameAndTypePair& pair) { return pair.name.empty(); }), names_and_types.end()); + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "{} file format doesn't support schema inference. You must specify the structure manually", + format_name); + names_and_types.erase( + std::remove_if(names_and_types.begin(), names_and_types.end(), [](const NameAndTypePair & pair) { return pair.name.empty(); }), + names_and_types.end()); return ColumnsDescription(names_and_types); } -ColumnsDescription readSchemaFromFormat(const String & format_name, const std::optional & format_settings, ReadBufferIterator & read_buffer_iterator, bool retry, ContextPtr & context) +ColumnsDescription readSchemaFromFormat( + const String & format_name, + const std::optional & format_settings, + ReadBufferIterator & read_buffer_iterator, + bool retry, + ContextPtr & context) { std::unique_ptr buf_out; return readSchemaFromFormat(format_name, format_settings, read_buffer_iterator, retry, context, buf_out); } -SchemaCache::Key getKeyForSchemaCache(const String & source, const String & format, const std::optional & format_settings, const ContextPtr & context) +SchemaCache::Key getKeyForSchemaCache( + const String & source, const String & format, const std::optional & format_settings, const ContextPtr & context) { return getKeysForSchemaCache({source}, format, format_settings, context).front(); } @@ -215,7 +231,8 @@ static SchemaCache::Key makeSchemaCacheKey(const String & source, const String & return SchemaCache::Key{source, format, additional_format_info}; } -SchemaCache::Keys getKeysForSchemaCache(const Strings & sources, const String & format, const std::optional & format_settings, const ContextPtr & context) +SchemaCache::Keys getKeysForSchemaCache( + const Strings & sources, const String & format, const std::optional & format_settings, const ContextPtr & context) { /// For some formats data schema depends on some settings, so it's possible that /// two queries to the same source will get two different schemas. To process this @@ -225,7 +242,11 @@ SchemaCache::Keys getKeysForSchemaCache(const Strings & sources, const String & String additional_format_info = FormatFactory::instance().getAdditionalInfoForSchemaCache(format, context, format_settings); SchemaCache::Keys cache_keys; cache_keys.reserve(sources.size()); - std::transform(sources.begin(), sources.end(), std::back_inserter(cache_keys), [&](const auto & source){ return makeSchemaCacheKey(source, format, additional_format_info); }); + std::transform( + sources.begin(), + sources.end(), + std::back_inserter(cache_keys), + [&](const auto & source) { return makeSchemaCacheKey(source, format, additional_format_info); }); return cache_keys; } From f087f0e87733eeb2672dc989e792f3c85d462601 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Tue, 11 Apr 2023 14:18:16 +0200 Subject: [PATCH 09/44] Update src/Formats/ReadSchemaUtils.cpp --- src/Formats/ReadSchemaUtils.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Formats/ReadSchemaUtils.cpp b/src/Formats/ReadSchemaUtils.cpp index d185b938ed6..f80d9ee41d2 100644 --- a/src/Formats/ReadSchemaUtils.cpp +++ b/src/Formats/ReadSchemaUtils.cpp @@ -203,6 +203,7 @@ ColumnsDescription readSchemaFromFormat( ErrorCodes::BAD_ARGUMENTS, "{} file format doesn't support schema inference. You must specify the structure manually", format_name); + /// Some formats like CSVWithNames can contain empty column names. We don't support empty column names and futher processing can fail with an exception. Let's just remove columns with empty names from the structure. names_and_types.erase( std::remove_if(names_and_types.begin(), names_and_types.end(), [](const NameAndTypePair & pair) { return pair.name.empty(); }), names_and_types.end()); From 029c92344b2db5b5129b7adb39eb0b5f09addb42 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Mon, 24 Apr 2023 16:57:43 +0200 Subject: [PATCH 10/44] Fix possible terminate called for uncaught exception in Connection --- src/Client/Connection.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/Client/Connection.cpp b/src/Client/Connection.cpp index d39148d3016..3dd78afb79b 100644 --- a/src/Client/Connection.cpp +++ b/src/Client/Connection.cpp @@ -232,12 +232,27 @@ void Connection::disconnect() maybe_compressed_out = nullptr; in = nullptr; last_input_packet_type.reset(); - out = nullptr; // can write to socket + std::exception_ptr finalize_exception; + try + { + // finalize() can write to socket and throw an exception. + out->finalize(); + } + catch (...) + { + /// Don't throw an exception here, it will leave Connection in invalid state. + finalize_exception = std::current_exception(); + } + out = nullptr; + if (socket) socket->close(); socket = nullptr; connected = false; nonce.reset(); + + if (finalize_exception) + std::rethrow_exception(finalize_exception); } From bc0c431eb77d7844429b70da58169dc49d5de4a7 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Mon, 24 Apr 2023 17:03:48 +0200 Subject: [PATCH 11/44] Fix possible terminate called for uncaught exception in InterserverIOHTTPHandler::handleRequest --- src/Server/InterserverIOHTTPHandler.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Server/InterserverIOHTTPHandler.cpp b/src/Server/InterserverIOHTTPHandler.cpp index 6b0cd543053..f7128e7e5a3 100644 --- a/src/Server/InterserverIOHTTPHandler.cpp +++ b/src/Server/InterserverIOHTTPHandler.cpp @@ -93,10 +93,13 @@ void InterserverIOHTTPHandler::handleRequest(HTTPServerRequest & request, HTTPSe auto write_response = [&](const std::string & message) { - if (response.sent()) - return; - auto & out = *used_output.out; + if (response.sent()) + { + out.finalize(); + return; + } + try { writeString(message, out); @@ -127,7 +130,10 @@ void InterserverIOHTTPHandler::handleRequest(HTTPServerRequest & request, HTTPSe catch (Exception & e) { if (e.code() == ErrorCodes::TOO_MANY_SIMULTANEOUS_QUERIES) + { + used_output.out->finalize(); return; + } response.setStatusAndReason(Poco::Net::HTTPResponse::HTTP_INTERNAL_SERVER_ERROR); From c503f6532c19ed98a119328c89a9d6e282767332 Mon Sep 17 00:00:00 2001 From: avogar Date: Mon, 24 Apr 2023 15:11:36 +0000 Subject: [PATCH 12/44] Add more finalize() to avoid terminate --- src/Storages/HDFS/StorageHDFS.cpp | 1 + src/Storages/StorageFile.cpp | 1 + src/Storages/StorageS3.cpp | 1 + src/Storages/StorageURL.cpp | 1 + 4 files changed, 4 insertions(+) diff --git a/src/Storages/HDFS/StorageHDFS.cpp b/src/Storages/HDFS/StorageHDFS.cpp index c915213f4ac..f4dd26435b3 100644 --- a/src/Storages/HDFS/StorageHDFS.cpp +++ b/src/Storages/HDFS/StorageHDFS.cpp @@ -490,6 +490,7 @@ private: { /// Stop ParallelFormattingOutputFormat correctly. writer.reset(); + write_buf->finalize(); throw; } } diff --git a/src/Storages/StorageFile.cpp b/src/Storages/StorageFile.cpp index 7e5a93c13c1..dafb51509ea 100644 --- a/src/Storages/StorageFile.cpp +++ b/src/Storages/StorageFile.cpp @@ -928,6 +928,7 @@ private: { /// Stop ParallelFormattingOutputFormat correctly. writer.reset(); + write_buf->finalize(); throw; } } diff --git a/src/Storages/StorageS3.cpp b/src/Storages/StorageS3.cpp index bd3e8fe886d..b0fde7c8e02 100644 --- a/src/Storages/StorageS3.cpp +++ b/src/Storages/StorageS3.cpp @@ -823,6 +823,7 @@ private: { /// Stop ParallelFormattingOutputFormat correctly. writer.reset(); + write_buf->finalize(); throw; } } diff --git a/src/Storages/StorageURL.cpp b/src/Storages/StorageURL.cpp index d2df3881c71..00b4c174834 100644 --- a/src/Storages/StorageURL.cpp +++ b/src/Storages/StorageURL.cpp @@ -442,6 +442,7 @@ void StorageURLSink::finalize() { /// Stop ParallelFormattingOutputFormat correctly. writer.reset(); + write_buf->finalize(); throw; } } From 57801b7a02b5574c1d385deb38d17846780389ac Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Mon, 24 Apr 2023 19:06:45 +0200 Subject: [PATCH 13/44] Fix --- src/Client/Connection.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Client/Connection.cpp b/src/Client/Connection.cpp index 3dd78afb79b..e328d0c4e43 100644 --- a/src/Client/Connection.cpp +++ b/src/Client/Connection.cpp @@ -236,7 +236,8 @@ void Connection::disconnect() try { // finalize() can write to socket and throw an exception. - out->finalize(); + if (out) + out->finalize(); } catch (...) { From c693c1bd17b62d9bc1bcb969867721415b4cb0a8 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Mon, 24 Apr 2023 19:07:30 +0200 Subject: [PATCH 14/44] Fix style --- src/Server/InterserverIOHTTPHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Server/InterserverIOHTTPHandler.cpp b/src/Server/InterserverIOHTTPHandler.cpp index f7128e7e5a3..ea71d954cc0 100644 --- a/src/Server/InterserverIOHTTPHandler.cpp +++ b/src/Server/InterserverIOHTTPHandler.cpp @@ -99,7 +99,7 @@ void InterserverIOHTTPHandler::handleRequest(HTTPServerRequest & request, HTTPSe out.finalize(); return; } - + try { writeString(message, out); From bacba6e34735be07a37283224326172aa1e9a71b Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Wed, 26 Apr 2023 12:18:12 +0200 Subject: [PATCH 15/44] Fix typo --- src/Formats/ReadSchemaUtils.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Formats/ReadSchemaUtils.cpp b/src/Formats/ReadSchemaUtils.cpp index f80d9ee41d2..61683b226ee 100644 --- a/src/Formats/ReadSchemaUtils.cpp +++ b/src/Formats/ReadSchemaUtils.cpp @@ -203,7 +203,7 @@ ColumnsDescription readSchemaFromFormat( ErrorCodes::BAD_ARGUMENTS, "{} file format doesn't support schema inference. You must specify the structure manually", format_name); - /// Some formats like CSVWithNames can contain empty column names. We don't support empty column names and futher processing can fail with an exception. Let's just remove columns with empty names from the structure. + /// Some formats like CSVWithNames can contain empty column names. We don't support empty column names and further processing can fail with an exception. Let's just remove columns with empty names from the structure. names_and_types.erase( std::remove_if(names_and_types.begin(), names_and_types.end(), [](const NameAndTypePair & pair) { return pair.name.empty(); }), names_and_types.end()); From 20007504a7d475274e07e46a0f4c27e577cad43f Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Thu, 11 May 2023 14:16:48 +0200 Subject: [PATCH 16/44] Handle exception in finalize inside WriteBufferFromPocoSocket destructor --- src/IO/WriteBufferFromPocoSocket.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/IO/WriteBufferFromPocoSocket.cpp b/src/IO/WriteBufferFromPocoSocket.cpp index 039110dfb62..79534b0f030 100644 --- a/src/IO/WriteBufferFromPocoSocket.cpp +++ b/src/IO/WriteBufferFromPocoSocket.cpp @@ -106,7 +106,22 @@ WriteBufferFromPocoSocket::WriteBufferFromPocoSocket(Poco::Net::Socket & socket_ WriteBufferFromPocoSocket::~WriteBufferFromPocoSocket() { - finalize(); +#ifndef NDEBUG + if (!finalized) + { + LOG_ERROR(log, "WriteBufferFromPocoSocket is not finalized in destructor. It's a bug"); + std::terminate(); + } +#else + try + { + finalize(); + } + catch (...) + { + tryLogCurrentException(__PRETTY_FUNCTION__); + } +#endif } } From 8436a093e78880983d4340e0b52822c1782dc84e Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Tue, 16 May 2023 13:36:12 +0200 Subject: [PATCH 17/44] Fix build --- src/IO/WriteBufferFromPocoSocket.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/IO/WriteBufferFromPocoSocket.cpp b/src/IO/WriteBufferFromPocoSocket.cpp index 79534b0f030..df34e8003cb 100644 --- a/src/IO/WriteBufferFromPocoSocket.cpp +++ b/src/IO/WriteBufferFromPocoSocket.cpp @@ -10,6 +10,7 @@ #include #include #include +#include namespace ProfileEvents From cee6c3914fef9913b0b249fcec359a692f413a32 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Mon, 22 May 2023 21:36:55 +0200 Subject: [PATCH 18/44] Fix build --- src/IO/WriteBufferFromPocoSocket.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/IO/WriteBufferFromPocoSocket.cpp b/src/IO/WriteBufferFromPocoSocket.cpp index df34e8003cb..c316ff17e2a 100644 --- a/src/IO/WriteBufferFromPocoSocket.cpp +++ b/src/IO/WriteBufferFromPocoSocket.cpp @@ -110,7 +110,7 @@ WriteBufferFromPocoSocket::~WriteBufferFromPocoSocket() #ifndef NDEBUG if (!finalized) { - LOG_ERROR(log, "WriteBufferFromPocoSocket is not finalized in destructor. It's a bug"); + LOG_ERROR(&Poco::Logger::get("WriteBufferFromPocoSocket"), "WriteBufferFromPocoSocket is not finalized in destructor. It's a bug"); std::terminate(); } #else From 66e111a6aa49d0cd9a16f7e6050b23f8ad4a6e68 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Tue, 23 May 2023 11:52:44 +0200 Subject: [PATCH 19/44] Fix tests --- src/IO/WriteBufferFromPocoSocket.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/IO/WriteBufferFromPocoSocket.cpp b/src/IO/WriteBufferFromPocoSocket.cpp index c316ff17e2a..cf3944e019d 100644 --- a/src/IO/WriteBufferFromPocoSocket.cpp +++ b/src/IO/WriteBufferFromPocoSocket.cpp @@ -10,7 +10,6 @@ #include #include #include -#include namespace ProfileEvents @@ -107,13 +106,6 @@ WriteBufferFromPocoSocket::WriteBufferFromPocoSocket(Poco::Net::Socket & socket_ WriteBufferFromPocoSocket::~WriteBufferFromPocoSocket() { -#ifndef NDEBUG - if (!finalized) - { - LOG_ERROR(&Poco::Logger::get("WriteBufferFromPocoSocket"), "WriteBufferFromPocoSocket is not finalized in destructor. It's a bug"); - std::terminate(); - } -#else try { finalize(); @@ -122,7 +114,6 @@ WriteBufferFromPocoSocket::~WriteBufferFromPocoSocket() { tryLogCurrentException(__PRETTY_FUNCTION__); } -#endif } } From 100b4d0969a5682e464d1de7f58685f3f4f635f3 Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Fri, 26 May 2023 20:07:00 +0200 Subject: [PATCH 20/44] cope with finalize in d-tors --- src/IO/WriteBufferFromS3TaskTracker.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/IO/WriteBufferFromS3TaskTracker.h b/src/IO/WriteBufferFromS3TaskTracker.h index 800e5239cc4..c3f4628b946 100644 --- a/src/IO/WriteBufferFromS3TaskTracker.h +++ b/src/IO/WriteBufferFromS3TaskTracker.h @@ -49,6 +49,8 @@ private: /// waitTilInflightShrink waits til the number of in-flight tasks beyond the limit `max_tasks_inflight`. void waitTilInflightShrink() TSA_NO_THREAD_SAFETY_ANALYSIS; + void collectFinishedFutures(bool propagate_exceptions) TSA_REQUIRES(mutex); + const bool is_async; ThreadPoolCallbackRunner scheduler; const size_t max_tasks_inflight; From 0e019c8e83aead4cd01fa21b0f46164d4fd4fb50 Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Fri, 26 May 2023 22:18:55 +0200 Subject: [PATCH 21/44] turn off some d-tor finalize --- src/Compression/CompressedWriteBuffer.cpp | 1 + .../WriteBufferFromNuraftBuffer.cpp | 1 + .../IO/CachedOnDiskWriteBufferFromFile.cpp | 1 + .../IO/WriteBufferFromAzureBlobStorage.cpp | 1 + .../IO/WriteBufferWithFinalizeCallback.cpp | 17 +++++++++-------- src/IO/Archives/ZipArchiveWriter.cpp | 7 ++++++- src/IO/BrotliWriteBuffer.cpp | 1 + src/IO/Bzip2WriteBuffer.cpp | 1 + src/IO/ForkWriteBuffer.cpp | 1 + src/IO/LZMADeflatingWriteBuffer.cpp | 1 + src/IO/Lz4DeflatingWriteBuffer.cpp | 1 + src/IO/SnappyWriteBuffer.cpp | 1 + src/IO/WriteBufferFromEncryptedFile.cpp | 1 + src/IO/WriteBufferFromFile.cpp | 1 + src/IO/WriteBufferFromFileDecorator.cpp | 17 +++++++++-------- src/IO/WriteBufferFromFileDescriptor.cpp | 1 + src/IO/WriteBufferFromOStream.cpp | 1 + src/IO/WriteBufferFromPocoSocket.cpp | 1 + src/IO/WriteBufferFromVector.h | 1 + src/IO/ZlibDeflatingWriteBuffer.cpp | 1 + src/IO/ZstdDeflatingAppendableWriteBuffer.cpp | 1 + src/IO/ZstdDeflatingWriteBuffer.cpp | 1 + .../Cache/WriteBufferToFileSegment.cpp | 1 + src/Interpreters/TemporaryDataOnDisk.cpp | 1 + .../HTTP/WriteBufferFromHTTPServerResponse.cpp | 1 + src/Storages/HDFS/WriteBufferFromHDFS.cpp | 1 + 26 files changed, 47 insertions(+), 17 deletions(-) diff --git a/src/Compression/CompressedWriteBuffer.cpp b/src/Compression/CompressedWriteBuffer.cpp index cb2ee1140d0..c3152c0adba 100644 --- a/src/Compression/CompressedWriteBuffer.cpp +++ b/src/Compression/CompressedWriteBuffer.cpp @@ -59,6 +59,7 @@ void CompressedWriteBuffer::nextImpl() CompressedWriteBuffer::~CompressedWriteBuffer() { + //! finalize(); } diff --git a/src/Coordination/WriteBufferFromNuraftBuffer.cpp b/src/Coordination/WriteBufferFromNuraftBuffer.cpp index c955d3fdbbe..5bed2da2978 100644 --- a/src/Coordination/WriteBufferFromNuraftBuffer.cpp +++ b/src/Coordination/WriteBufferFromNuraftBuffer.cpp @@ -53,6 +53,7 @@ nuraft::ptr WriteBufferFromNuraftBuffer::getBuffer() WriteBufferFromNuraftBuffer::~WriteBufferFromNuraftBuffer() { + //! try { finalize(); diff --git a/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp b/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp index 9153af90312..2abf3ab5203 100644 --- a/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp +++ b/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp @@ -123,6 +123,7 @@ void FileSegmentRangeWriter::finalize() FileSegmentRangeWriter::~FileSegmentRangeWriter() { + //! try { if (!finalized) diff --git a/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp b/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp index b5d296bd865..56b45f1dc22 100644 --- a/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp +++ b/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp @@ -38,6 +38,7 @@ WriteBufferFromAzureBlobStorage::WriteBufferFromAzureBlobStorage( WriteBufferFromAzureBlobStorage::~WriteBufferFromAzureBlobStorage() { + //! finalize(); } diff --git a/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp b/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp index 49e230b9dc3..63d7edf5437 100644 --- a/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp +++ b/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp @@ -16,14 +16,15 @@ WriteBufferWithFinalizeCallback::WriteBufferWithFinalizeCallback( WriteBufferWithFinalizeCallback::~WriteBufferWithFinalizeCallback() { - try - { - finalize(); - } - catch (...) - { - tryLogCurrentException(__PRETTY_FUNCTION__); - } + //! +// try +// { +// finalize(); +// } +// catch (...) +// { +// tryLogCurrentException(__PRETTY_FUNCTION__); +// } } void WriteBufferWithFinalizeCallback::finalizeImpl() diff --git a/src/IO/Archives/ZipArchiveWriter.cpp b/src/IO/Archives/ZipArchiveWriter.cpp index 088d83cd29e..57bb6452914 100644 --- a/src/IO/Archives/ZipArchiveWriter.cpp +++ b/src/IO/Archives/ZipArchiveWriter.cpp @@ -115,6 +115,7 @@ public: ~WriteBufferFromZipArchive() override { + //! try { finalize(); @@ -191,7 +192,11 @@ namespace explicit StreamFromWriteBuffer(std::unique_ptr write_buffer_) : write_buffer(std::move(write_buffer_)), start_offset(write_buffer->count()) {} - ~StreamFromWriteBuffer() { write_buffer->finalize(); } + ~StreamFromWriteBuffer() + { + //! + write_buffer->finalize(); + } static int closeFileFunc(void *, void * stream) { diff --git a/src/IO/BrotliWriteBuffer.cpp b/src/IO/BrotliWriteBuffer.cpp index 47426d62a6e..0c434b4981e 100644 --- a/src/IO/BrotliWriteBuffer.cpp +++ b/src/IO/BrotliWriteBuffer.cpp @@ -44,6 +44,7 @@ BrotliWriteBuffer::BrotliWriteBuffer(std::unique_ptr out_, int comp BrotliWriteBuffer::~BrotliWriteBuffer() { + //! finalize(); } diff --git a/src/IO/Bzip2WriteBuffer.cpp b/src/IO/Bzip2WriteBuffer.cpp index 4b6bed70d35..e5aab177f3a 100644 --- a/src/IO/Bzip2WriteBuffer.cpp +++ b/src/IO/Bzip2WriteBuffer.cpp @@ -47,6 +47,7 @@ Bzip2WriteBuffer::Bzip2WriteBuffer(std::unique_ptr out_, int compre Bzip2WriteBuffer::~Bzip2WriteBuffer() { + //! finalize(); } diff --git a/src/IO/ForkWriteBuffer.cpp b/src/IO/ForkWriteBuffer.cpp index 8e11b9ff590..b3481203757 100644 --- a/src/IO/ForkWriteBuffer.cpp +++ b/src/IO/ForkWriteBuffer.cpp @@ -53,6 +53,7 @@ void ForkWriteBuffer::finalizeImpl() ForkWriteBuffer::~ForkWriteBuffer() { + //! finalize(); } diff --git a/src/IO/LZMADeflatingWriteBuffer.cpp b/src/IO/LZMADeflatingWriteBuffer.cpp index 30e247b1016..3e1c9dc58b9 100644 --- a/src/IO/LZMADeflatingWriteBuffer.cpp +++ b/src/IO/LZMADeflatingWriteBuffer.cpp @@ -46,6 +46,7 @@ LZMADeflatingWriteBuffer::LZMADeflatingWriteBuffer( LZMADeflatingWriteBuffer::~LZMADeflatingWriteBuffer() { + //! finalize(); } diff --git a/src/IO/Lz4DeflatingWriteBuffer.cpp b/src/IO/Lz4DeflatingWriteBuffer.cpp index c3a1b8282c3..2627643183d 100644 --- a/src/IO/Lz4DeflatingWriteBuffer.cpp +++ b/src/IO/Lz4DeflatingWriteBuffer.cpp @@ -42,6 +42,7 @@ Lz4DeflatingWriteBuffer::Lz4DeflatingWriteBuffer( Lz4DeflatingWriteBuffer::~Lz4DeflatingWriteBuffer() { + //! finalize(); } diff --git a/src/IO/SnappyWriteBuffer.cpp b/src/IO/SnappyWriteBuffer.cpp index ca40d0656d1..3ea6f831d5b 100644 --- a/src/IO/SnappyWriteBuffer.cpp +++ b/src/IO/SnappyWriteBuffer.cpp @@ -22,6 +22,7 @@ SnappyWriteBuffer::SnappyWriteBuffer(std::unique_ptr out_, size_t b SnappyWriteBuffer::~SnappyWriteBuffer() { + //! finish(); } diff --git a/src/IO/WriteBufferFromEncryptedFile.cpp b/src/IO/WriteBufferFromEncryptedFile.cpp index 5bca0dc68d5..7efdbc43238 100644 --- a/src/IO/WriteBufferFromEncryptedFile.cpp +++ b/src/IO/WriteBufferFromEncryptedFile.cpp @@ -21,6 +21,7 @@ WriteBufferFromEncryptedFile::WriteBufferFromEncryptedFile( WriteBufferFromEncryptedFile::~WriteBufferFromEncryptedFile() { + //! finalize(); } diff --git a/src/IO/WriteBufferFromFile.cpp b/src/IO/WriteBufferFromFile.cpp index e58f1e3a60c..964f7415803 100644 --- a/src/IO/WriteBufferFromFile.cpp +++ b/src/IO/WriteBufferFromFile.cpp @@ -77,6 +77,7 @@ WriteBufferFromFile::~WriteBufferFromFile() if (fd < 0) return; + //! finalize(); int err = ::close(fd); /// Everything except for EBADF should be ignored in dtor, since all of diff --git a/src/IO/WriteBufferFromFileDecorator.cpp b/src/IO/WriteBufferFromFileDecorator.cpp index 4cc881f177f..82e38eac72d 100644 --- a/src/IO/WriteBufferFromFileDecorator.cpp +++ b/src/IO/WriteBufferFromFileDecorator.cpp @@ -30,14 +30,15 @@ void WriteBufferFromFileDecorator::finalizeImpl() WriteBufferFromFileDecorator::~WriteBufferFromFileDecorator() { - try - { - finalize(); - } - catch (...) - { - tryLogCurrentException(__PRETTY_FUNCTION__); - } + //! +// try +// { +// finalize(); +// } +// catch (...) +// { +// tryLogCurrentException(__PRETTY_FUNCTION__); +// } /// It is not a mistake that swap is called here /// Swap has been called at constructor, it should be called at destructor diff --git a/src/IO/WriteBufferFromFileDescriptor.cpp b/src/IO/WriteBufferFromFileDescriptor.cpp index 135ff608967..0516171c051 100644 --- a/src/IO/WriteBufferFromFileDescriptor.cpp +++ b/src/IO/WriteBufferFromFileDescriptor.cpp @@ -105,6 +105,7 @@ WriteBufferFromFileDescriptor::WriteBufferFromFileDescriptor( WriteBufferFromFileDescriptor::~WriteBufferFromFileDescriptor() { + //! finalize(); } diff --git a/src/IO/WriteBufferFromOStream.cpp b/src/IO/WriteBufferFromOStream.cpp index 2d0d5976f85..d55aa054174 100644 --- a/src/IO/WriteBufferFromOStream.cpp +++ b/src/IO/WriteBufferFromOStream.cpp @@ -40,6 +40,7 @@ WriteBufferFromOStream::WriteBufferFromOStream( WriteBufferFromOStream::~WriteBufferFromOStream() { + //! finalize(); } diff --git a/src/IO/WriteBufferFromPocoSocket.cpp b/src/IO/WriteBufferFromPocoSocket.cpp index 039110dfb62..a8b28445294 100644 --- a/src/IO/WriteBufferFromPocoSocket.cpp +++ b/src/IO/WriteBufferFromPocoSocket.cpp @@ -106,6 +106,7 @@ WriteBufferFromPocoSocket::WriteBufferFromPocoSocket(Poco::Net::Socket & socket_ WriteBufferFromPocoSocket::~WriteBufferFromPocoSocket() { + //! finalize(); } diff --git a/src/IO/WriteBufferFromVector.h b/src/IO/WriteBufferFromVector.h index 4b2a3581625..b0ff3183291 100644 --- a/src/IO/WriteBufferFromVector.h +++ b/src/IO/WriteBufferFromVector.h @@ -63,6 +63,7 @@ public: ~WriteBufferFromVector() override { + //! finalize(); } diff --git a/src/IO/ZlibDeflatingWriteBuffer.cpp b/src/IO/ZlibDeflatingWriteBuffer.cpp index 43bb0405555..4440254d15f 100644 --- a/src/IO/ZlibDeflatingWriteBuffer.cpp +++ b/src/IO/ZlibDeflatingWriteBuffer.cpp @@ -74,6 +74,7 @@ void ZlibDeflatingWriteBuffer::nextImpl() ZlibDeflatingWriteBuffer::~ZlibDeflatingWriteBuffer() { + //! try { finalize(); diff --git a/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp b/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp index be739c0e654..05ee23158fd 100644 --- a/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp +++ b/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp @@ -87,6 +87,7 @@ void ZstdDeflatingAppendableWriteBuffer::nextImpl() ZstdDeflatingAppendableWriteBuffer::~ZstdDeflatingAppendableWriteBuffer() { + //! finalize(); } diff --git a/src/IO/ZstdDeflatingWriteBuffer.cpp b/src/IO/ZstdDeflatingWriteBuffer.cpp index c6d2ffc39f9..4ec3f5a3fce 100644 --- a/src/IO/ZstdDeflatingWriteBuffer.cpp +++ b/src/IO/ZstdDeflatingWriteBuffer.cpp @@ -33,6 +33,7 @@ ZstdDeflatingWriteBuffer::ZstdDeflatingWriteBuffer( ZstdDeflatingWriteBuffer::~ZstdDeflatingWriteBuffer() { + //! finalize(); } diff --git a/src/Interpreters/Cache/WriteBufferToFileSegment.cpp b/src/Interpreters/Cache/WriteBufferToFileSegment.cpp index 1eac87a804d..ed831350186 100644 --- a/src/Interpreters/Cache/WriteBufferToFileSegment.cpp +++ b/src/Interpreters/Cache/WriteBufferToFileSegment.cpp @@ -73,6 +73,7 @@ std::shared_ptr WriteBufferToFileSegment::getReadBufferImpl() WriteBufferToFileSegment::~WriteBufferToFileSegment() { + //! try { finalize(); diff --git a/src/Interpreters/TemporaryDataOnDisk.cpp b/src/Interpreters/TemporaryDataOnDisk.cpp index 69fef21dbab..14e0bff6966 100644 --- a/src/Interpreters/TemporaryDataOnDisk.cpp +++ b/src/Interpreters/TemporaryDataOnDisk.cpp @@ -184,6 +184,7 @@ struct TemporaryFileStream::OutputWriter ~OutputWriter() { + //! try { finalize(); diff --git a/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp b/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp index c8015cfd185..03222fe7e0b 100644 --- a/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp +++ b/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp @@ -171,6 +171,7 @@ void WriteBufferFromHTTPServerResponse::onProgress(const Progress & progress) WriteBufferFromHTTPServerResponse::~WriteBufferFromHTTPServerResponse() { + //! finalize(); } diff --git a/src/Storages/HDFS/WriteBufferFromHDFS.cpp b/src/Storages/HDFS/WriteBufferFromHDFS.cpp index fad0447d2cf..48f35c378ba 100644 --- a/src/Storages/HDFS/WriteBufferFromHDFS.cpp +++ b/src/Storages/HDFS/WriteBufferFromHDFS.cpp @@ -147,6 +147,7 @@ void WriteBufferFromHDFS::finalizeImpl() WriteBufferFromHDFS::~WriteBufferFromHDFS() { + //! finalize(); } From fe3939287b476b7e0f2cec669dd9606f2fd0438f Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Sat, 27 May 2023 01:02:48 +0200 Subject: [PATCH 22/44] add test, fix .gin_sid files --- src/IO/WriteBufferFromS3.cpp | 12 ++++++++++-- src/IO/WriteBufferFromS3TaskTracker.cpp | 15 ++++++++------- src/Storages/MergeTree/GinIndexStore.cpp | 7 +++++++ .../configs/config.d/storage_conf.xml | 19 +++++++++++++++++++ tests/integration/test_merge_tree_s3/test.py | 5 +++-- 5 files changed, 47 insertions(+), 11 deletions(-) diff --git a/src/IO/WriteBufferFromS3.cpp b/src/IO/WriteBufferFromS3.cpp index 954c996d929..4657a65f931 100644 --- a/src/IO/WriteBufferFromS3.cpp +++ b/src/IO/WriteBufferFromS3.cpp @@ -182,6 +182,14 @@ void WriteBufferFromS3::finalizeImpl() if (!is_prefinalized) preFinalize(); + if (std::uncaught_exception()) + throw Exception( + ErrorCodes::LOGICAL_ERROR, + "Detected buffer finalization when an exception is unwinding the stack." + " Do not call finalize buffer in destructors or when exception thrown." + " Details {}", + getLogDetails()); + chassert(offset() == 0); chassert(hidden_size == 0); @@ -521,7 +529,7 @@ void WriteBufferFromS3::completeMultipartUpload() if (multipart_tags.empty()) throw Exception( - ErrorCodes::LOGICAL_ERROR, + ErrorCodes::S3_ERROR, "Failed to complete multipart upload. No parts have uploaded"); for (size_t i = 0; i < multipart_tags.size(); ++i) @@ -529,7 +537,7 @@ void WriteBufferFromS3::completeMultipartUpload() const auto tag = multipart_tags.at(i); if (tag.empty()) throw Exception( - ErrorCodes::LOGICAL_ERROR, + ErrorCodes::S3_ERROR, "Failed to complete multipart upload. Part {} haven't been uploaded.", i); } diff --git a/src/IO/WriteBufferFromS3TaskTracker.cpp b/src/IO/WriteBufferFromS3TaskTracker.cpp index 4abae90eeac..7cf5a89df86 100644 --- a/src/IO/WriteBufferFromS3TaskTracker.cpp +++ b/src/IO/WriteBufferFromS3TaskTracker.cpp @@ -162,15 +162,16 @@ void WriteBufferFromS3::TaskTracker::waitTilInflightShrink() for (auto & it : finished_futures) { - SCOPE_EXIT({ - /// According to basic exception safety TaskTracker has to be destroyed after exception - /// If it would be true than this SCOPE_EXIT is superfluous - /// However WriteBufferWithFinalizeCallback, WriteBufferFromFileDecorator do call finalize in d-tor - /// TaskTracker has to cope this until the issue with finalizing in d-tor is addressed in #50274 - futures.erase(it); - }); +// SCOPE_EXIT({ +// /// According to basic exception safety TaskTracker has to be destroyed after exception +// /// If it would be true than this SCOPE_EXIT is superfluous +// /// However WriteBufferWithFinalizeCallback, WriteBufferFromFileDecorator do call finalize in d-tor +// /// TaskTracker has to cope this until the issue with finalizing in d-tor is addressed in #50274 +// futures.erase(it); +// }); it->get(); + futures.erase(it); } finished_futures.clear(); diff --git a/src/Storages/MergeTree/GinIndexStore.cpp b/src/Storages/MergeTree/GinIndexStore.cpp index 0904855755c..aa0c1fccbc3 100644 --- a/src/Storages/MergeTree/GinIndexStore.cpp +++ b/src/Storages/MergeTree/GinIndexStore.cpp @@ -166,6 +166,7 @@ UInt32 GinIndexStore::getNextSegmentIDRange(const String & file_name, size_t n) /// Write segment ID 1 writeVarUInt(1, *ostr); ostr->sync(); + ostr->finalize(); } /// Read id in file @@ -188,6 +189,7 @@ UInt32 GinIndexStore::getNextSegmentIDRange(const String & file_name, size_t n) writeVarUInt(result + n, *ostr); ostr->sync(); + ostr->finalize(); } return result; } @@ -317,8 +319,13 @@ void GinIndexStore::writeSegment() current_segment.segment_id = getNextSegmentID(); metadata_file_stream->sync(); + metadata_file_stream->finalize(); + dict_file_stream->sync(); + dict_file_stream->finalize(); + postings_file_stream->sync(); + postings_file_stream->finalize(); } GinIndexStoreDeserializer::GinIndexStoreDeserializer(const GinIndexStorePtr & store_) diff --git a/tests/integration/test_merge_tree_s3/configs/config.d/storage_conf.xml b/tests/integration/test_merge_tree_s3/configs/config.d/storage_conf.xml index cca80143548..504280e4bed 100644 --- a/tests/integration/test_merge_tree_s3/configs/config.d/storage_conf.xml +++ b/tests/integration/test_merge_tree_s3/configs/config.d/storage_conf.xml @@ -33,6 +33,18 @@ 20000 1 + + s3 + http://resolver:8083/root/data/ + minio + minio123 + true + 0 + 20000 + 20000 + 0 + 1 + local / @@ -128,6 +140,13 @@ + + +
+ broken_s3_always_multi_part +
+ + diff --git a/tests/integration/test_merge_tree_s3/test.py b/tests/integration/test_merge_tree_s3/test.py index f87644a6876..4dcc5805294 100644 --- a/tests/integration/test_merge_tree_s3/test.py +++ b/tests/integration/test_merge_tree_s3/test.py @@ -930,8 +930,9 @@ def test_merge_canceled_by_drop(cluster, node_name): ) +@pytest.mark.parametrize("storage_policy", ["broken_s3_always_multi_part", "broken_s3"]) @pytest.mark.parametrize("node_name", ["node"]) -def test_merge_canceled_by_s3_errors(cluster, node_name): +def test_merge_canceled_by_s3_errors(cluster, node_name, storage_policy): node = cluster.instances[node_name] node.query("DROP TABLE IF EXISTS test_merge_canceled_by_s3_errors NO DELAY") node.query( @@ -939,7 +940,7 @@ def test_merge_canceled_by_s3_errors(cluster, node_name): " (key UInt32, value String)" " Engine=MergeTree() " " ORDER BY value " - " SETTINGS storage_policy='broken_s3'" + f" SETTINGS storage_policy='{storage_policy}'" ) node.query("SYSTEM STOP MERGES test_merge_canceled_by_s3_errors") node.query( From 99ad481505798a6da906e6b96d10b641db902f68 Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Sat, 27 May 2023 11:47:44 +0200 Subject: [PATCH 23/44] fix sizes.log writes --- src/Common/FileChecker.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Common/FileChecker.cpp b/src/Common/FileChecker.cpp index a6e37654ff1..876bc4e641c 100644 --- a/src/Common/FileChecker.cpp +++ b/src/Common/FileChecker.cpp @@ -138,7 +138,7 @@ void FileChecker::save() const std::string tmp_files_info_path = parentPath(files_info_path) + "tmp_" + fileName(files_info_path); { - std::unique_ptr out = disk ? disk->writeFile(tmp_files_info_path) : std::make_unique(tmp_files_info_path); + std::unique_ptr out = disk ? disk->writeFile(tmp_files_info_path) : std::make_unique(tmp_files_info_path); /// So complex JSON structure - for compatibility with the old format. writeCString("{\"clickhouse\":{", *out); @@ -157,7 +157,9 @@ void FileChecker::save() const } writeCString("}}", *out); - out->next(); + + out->sync(); + out->finalize(); } if (disk) From d95e5b51af32db90de8f2194e83c0015dfd0f4cb Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Sat, 27 May 2023 12:14:07 +0200 Subject: [PATCH 24/44] mark all finalize calls in buffers d-tors with issue id --- src/Compression/CompressedWriteBuffer.cpp | 2 +- src/Coordination/WriteBufferFromNuraftBuffer.cpp | 2 +- src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp | 2 +- src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp | 2 +- src/Disks/IO/WriteBufferWithFinalizeCallback.cpp | 2 +- src/IO/Archives/ZipArchiveWriter.cpp | 4 ++-- src/IO/BrotliWriteBuffer.cpp | 2 +- src/IO/Bzip2WriteBuffer.cpp | 2 +- src/IO/ForkWriteBuffer.cpp | 2 +- src/IO/LZMADeflatingWriteBuffer.cpp | 2 +- src/IO/Lz4DeflatingWriteBuffer.cpp | 2 +- src/IO/SnappyWriteBuffer.cpp | 2 +- src/IO/WriteBufferFromEncryptedFile.cpp | 2 +- src/IO/WriteBufferFromFile.cpp | 2 +- src/IO/WriteBufferFromFileDecorator.cpp | 2 +- src/IO/WriteBufferFromFileDescriptor.cpp | 2 +- src/IO/WriteBufferFromOStream.cpp | 2 +- src/IO/WriteBufferFromPocoSocket.cpp | 2 +- src/IO/WriteBufferFromVector.h | 2 +- src/IO/ZlibDeflatingWriteBuffer.cpp | 2 +- src/IO/ZstdDeflatingAppendableWriteBuffer.cpp | 2 +- src/IO/ZstdDeflatingWriteBuffer.cpp | 2 +- src/Interpreters/Cache/WriteBufferToFileSegment.cpp | 2 +- src/Interpreters/TemporaryDataOnDisk.cpp | 2 +- src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp | 2 +- src/Storages/HDFS/WriteBufferFromHDFS.cpp | 2 +- 26 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/Compression/CompressedWriteBuffer.cpp b/src/Compression/CompressedWriteBuffer.cpp index c3152c0adba..22c5f235a27 100644 --- a/src/Compression/CompressedWriteBuffer.cpp +++ b/src/Compression/CompressedWriteBuffer.cpp @@ -59,7 +59,7 @@ void CompressedWriteBuffer::nextImpl() CompressedWriteBuffer::~CompressedWriteBuffer() { - //! + /// ! #50274 finalize(); } diff --git a/src/Coordination/WriteBufferFromNuraftBuffer.cpp b/src/Coordination/WriteBufferFromNuraftBuffer.cpp index 5bed2da2978..41b933717c6 100644 --- a/src/Coordination/WriteBufferFromNuraftBuffer.cpp +++ b/src/Coordination/WriteBufferFromNuraftBuffer.cpp @@ -53,7 +53,7 @@ nuraft::ptr WriteBufferFromNuraftBuffer::getBuffer() WriteBufferFromNuraftBuffer::~WriteBufferFromNuraftBuffer() { - //! + /// ! #50274 try { finalize(); diff --git a/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp b/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp index 2abf3ab5203..8da9ce73ae3 100644 --- a/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp +++ b/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp @@ -123,7 +123,7 @@ void FileSegmentRangeWriter::finalize() FileSegmentRangeWriter::~FileSegmentRangeWriter() { - //! + /// ! #50274 try { if (!finalized) diff --git a/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp b/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp index 56b45f1dc22..8ac2c8ca831 100644 --- a/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp +++ b/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp @@ -38,7 +38,7 @@ WriteBufferFromAzureBlobStorage::WriteBufferFromAzureBlobStorage( WriteBufferFromAzureBlobStorage::~WriteBufferFromAzureBlobStorage() { - //! + /// ! #50274 finalize(); } diff --git a/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp b/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp index 63d7edf5437..47bf742fd5b 100644 --- a/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp +++ b/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp @@ -16,7 +16,7 @@ WriteBufferWithFinalizeCallback::WriteBufferWithFinalizeCallback( WriteBufferWithFinalizeCallback::~WriteBufferWithFinalizeCallback() { - //! + /// ! #50274 // try // { // finalize(); diff --git a/src/IO/Archives/ZipArchiveWriter.cpp b/src/IO/Archives/ZipArchiveWriter.cpp index 57bb6452914..1c6ef18d372 100644 --- a/src/IO/Archives/ZipArchiveWriter.cpp +++ b/src/IO/Archives/ZipArchiveWriter.cpp @@ -115,7 +115,7 @@ public: ~WriteBufferFromZipArchive() override { - //! + /// ! #50274 try { finalize(); @@ -194,7 +194,7 @@ namespace ~StreamFromWriteBuffer() { - //! + /// ! #50274 write_buffer->finalize(); } diff --git a/src/IO/BrotliWriteBuffer.cpp b/src/IO/BrotliWriteBuffer.cpp index 0c434b4981e..98786313b09 100644 --- a/src/IO/BrotliWriteBuffer.cpp +++ b/src/IO/BrotliWriteBuffer.cpp @@ -44,7 +44,7 @@ BrotliWriteBuffer::BrotliWriteBuffer(std::unique_ptr out_, int comp BrotliWriteBuffer::~BrotliWriteBuffer() { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/Bzip2WriteBuffer.cpp b/src/IO/Bzip2WriteBuffer.cpp index e5aab177f3a..c7159d4d002 100644 --- a/src/IO/Bzip2WriteBuffer.cpp +++ b/src/IO/Bzip2WriteBuffer.cpp @@ -47,7 +47,7 @@ Bzip2WriteBuffer::Bzip2WriteBuffer(std::unique_ptr out_, int compre Bzip2WriteBuffer::~Bzip2WriteBuffer() { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/ForkWriteBuffer.cpp b/src/IO/ForkWriteBuffer.cpp index b3481203757..ea38f70745d 100644 --- a/src/IO/ForkWriteBuffer.cpp +++ b/src/IO/ForkWriteBuffer.cpp @@ -53,7 +53,7 @@ void ForkWriteBuffer::finalizeImpl() ForkWriteBuffer::~ForkWriteBuffer() { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/LZMADeflatingWriteBuffer.cpp b/src/IO/LZMADeflatingWriteBuffer.cpp index 3e1c9dc58b9..07597ce958b 100644 --- a/src/IO/LZMADeflatingWriteBuffer.cpp +++ b/src/IO/LZMADeflatingWriteBuffer.cpp @@ -46,7 +46,7 @@ LZMADeflatingWriteBuffer::LZMADeflatingWriteBuffer( LZMADeflatingWriteBuffer::~LZMADeflatingWriteBuffer() { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/Lz4DeflatingWriteBuffer.cpp b/src/IO/Lz4DeflatingWriteBuffer.cpp index 2627643183d..988d9861a9b 100644 --- a/src/IO/Lz4DeflatingWriteBuffer.cpp +++ b/src/IO/Lz4DeflatingWriteBuffer.cpp @@ -42,7 +42,7 @@ Lz4DeflatingWriteBuffer::Lz4DeflatingWriteBuffer( Lz4DeflatingWriteBuffer::~Lz4DeflatingWriteBuffer() { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/SnappyWriteBuffer.cpp b/src/IO/SnappyWriteBuffer.cpp index 3ea6f831d5b..5203d8b053e 100644 --- a/src/IO/SnappyWriteBuffer.cpp +++ b/src/IO/SnappyWriteBuffer.cpp @@ -22,7 +22,7 @@ SnappyWriteBuffer::SnappyWriteBuffer(std::unique_ptr out_, size_t b SnappyWriteBuffer::~SnappyWriteBuffer() { - //! + /// ! #50274 finish(); } diff --git a/src/IO/WriteBufferFromEncryptedFile.cpp b/src/IO/WriteBufferFromEncryptedFile.cpp index 7efdbc43238..5831b3aee94 100644 --- a/src/IO/WriteBufferFromEncryptedFile.cpp +++ b/src/IO/WriteBufferFromEncryptedFile.cpp @@ -21,7 +21,7 @@ WriteBufferFromEncryptedFile::WriteBufferFromEncryptedFile( WriteBufferFromEncryptedFile::~WriteBufferFromEncryptedFile() { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/WriteBufferFromFile.cpp b/src/IO/WriteBufferFromFile.cpp index 964f7415803..0777d596184 100644 --- a/src/IO/WriteBufferFromFile.cpp +++ b/src/IO/WriteBufferFromFile.cpp @@ -77,7 +77,7 @@ WriteBufferFromFile::~WriteBufferFromFile() if (fd < 0) return; - //! + /// ! #50274 finalize(); int err = ::close(fd); /// Everything except for EBADF should be ignored in dtor, since all of diff --git a/src/IO/WriteBufferFromFileDecorator.cpp b/src/IO/WriteBufferFromFileDecorator.cpp index 82e38eac72d..67a98d9e43f 100644 --- a/src/IO/WriteBufferFromFileDecorator.cpp +++ b/src/IO/WriteBufferFromFileDecorator.cpp @@ -30,7 +30,7 @@ void WriteBufferFromFileDecorator::finalizeImpl() WriteBufferFromFileDecorator::~WriteBufferFromFileDecorator() { - //! + /// ! #50274 // try // { // finalize(); diff --git a/src/IO/WriteBufferFromFileDescriptor.cpp b/src/IO/WriteBufferFromFileDescriptor.cpp index 0516171c051..03988c09f62 100644 --- a/src/IO/WriteBufferFromFileDescriptor.cpp +++ b/src/IO/WriteBufferFromFileDescriptor.cpp @@ -105,7 +105,7 @@ WriteBufferFromFileDescriptor::WriteBufferFromFileDescriptor( WriteBufferFromFileDescriptor::~WriteBufferFromFileDescriptor() { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/WriteBufferFromOStream.cpp b/src/IO/WriteBufferFromOStream.cpp index d55aa054174..25886b9d10d 100644 --- a/src/IO/WriteBufferFromOStream.cpp +++ b/src/IO/WriteBufferFromOStream.cpp @@ -40,7 +40,7 @@ WriteBufferFromOStream::WriteBufferFromOStream( WriteBufferFromOStream::~WriteBufferFromOStream() { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/WriteBufferFromPocoSocket.cpp b/src/IO/WriteBufferFromPocoSocket.cpp index a8b28445294..a0dcf69e67d 100644 --- a/src/IO/WriteBufferFromPocoSocket.cpp +++ b/src/IO/WriteBufferFromPocoSocket.cpp @@ -106,7 +106,7 @@ WriteBufferFromPocoSocket::WriteBufferFromPocoSocket(Poco::Net::Socket & socket_ WriteBufferFromPocoSocket::~WriteBufferFromPocoSocket() { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/WriteBufferFromVector.h b/src/IO/WriteBufferFromVector.h index b0ff3183291..ab4f27a7bcb 100644 --- a/src/IO/WriteBufferFromVector.h +++ b/src/IO/WriteBufferFromVector.h @@ -63,7 +63,7 @@ public: ~WriteBufferFromVector() override { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/ZlibDeflatingWriteBuffer.cpp b/src/IO/ZlibDeflatingWriteBuffer.cpp index 4440254d15f..7ae8f636f69 100644 --- a/src/IO/ZlibDeflatingWriteBuffer.cpp +++ b/src/IO/ZlibDeflatingWriteBuffer.cpp @@ -74,7 +74,7 @@ void ZlibDeflatingWriteBuffer::nextImpl() ZlibDeflatingWriteBuffer::~ZlibDeflatingWriteBuffer() { - //! + /// ! #50274 try { finalize(); diff --git a/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp b/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp index 05ee23158fd..8f66618f513 100644 --- a/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp +++ b/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp @@ -87,7 +87,7 @@ void ZstdDeflatingAppendableWriteBuffer::nextImpl() ZstdDeflatingAppendableWriteBuffer::~ZstdDeflatingAppendableWriteBuffer() { - //! + /// ! #50274 finalize(); } diff --git a/src/IO/ZstdDeflatingWriteBuffer.cpp b/src/IO/ZstdDeflatingWriteBuffer.cpp index 4ec3f5a3fce..95e2e7d7716 100644 --- a/src/IO/ZstdDeflatingWriteBuffer.cpp +++ b/src/IO/ZstdDeflatingWriteBuffer.cpp @@ -33,7 +33,7 @@ ZstdDeflatingWriteBuffer::ZstdDeflatingWriteBuffer( ZstdDeflatingWriteBuffer::~ZstdDeflatingWriteBuffer() { - //! + /// ! #50274 finalize(); } diff --git a/src/Interpreters/Cache/WriteBufferToFileSegment.cpp b/src/Interpreters/Cache/WriteBufferToFileSegment.cpp index ed831350186..cbbf22078c1 100644 --- a/src/Interpreters/Cache/WriteBufferToFileSegment.cpp +++ b/src/Interpreters/Cache/WriteBufferToFileSegment.cpp @@ -73,7 +73,7 @@ std::shared_ptr WriteBufferToFileSegment::getReadBufferImpl() WriteBufferToFileSegment::~WriteBufferToFileSegment() { - //! + /// ! #50274 try { finalize(); diff --git a/src/Interpreters/TemporaryDataOnDisk.cpp b/src/Interpreters/TemporaryDataOnDisk.cpp index 14e0bff6966..2fc21e84be7 100644 --- a/src/Interpreters/TemporaryDataOnDisk.cpp +++ b/src/Interpreters/TemporaryDataOnDisk.cpp @@ -184,7 +184,7 @@ struct TemporaryFileStream::OutputWriter ~OutputWriter() { - //! + /// ! #50274 try { finalize(); diff --git a/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp b/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp index 03222fe7e0b..7d6710e76e6 100644 --- a/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp +++ b/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp @@ -171,7 +171,7 @@ void WriteBufferFromHTTPServerResponse::onProgress(const Progress & progress) WriteBufferFromHTTPServerResponse::~WriteBufferFromHTTPServerResponse() { - //! + /// ! #50274 finalize(); } diff --git a/src/Storages/HDFS/WriteBufferFromHDFS.cpp b/src/Storages/HDFS/WriteBufferFromHDFS.cpp index 48f35c378ba..169cf845d3b 100644 --- a/src/Storages/HDFS/WriteBufferFromHDFS.cpp +++ b/src/Storages/HDFS/WriteBufferFromHDFS.cpp @@ -147,7 +147,7 @@ void WriteBufferFromHDFS::finalizeImpl() WriteBufferFromHDFS::~WriteBufferFromHDFS() { - //! + /// ! #50274 finalize(); } From a5dcd8dabbd2ba9fd796b0ccacf447fc75ecc97e Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Sat, 27 May 2023 20:51:59 +0200 Subject: [PATCH 25/44] do not call finalize in d-tor MergedBlockOutputStream::Finalizer --- src/IO/WriteBufferFromS3.cpp | 7 +++++-- .../MergeTree/MergedBlockOutputStream.cpp | 17 +++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/IO/WriteBufferFromS3.cpp b/src/IO/WriteBufferFromS3.cpp index 4657a65f931..58d58e86ebe 100644 --- a/src/IO/WriteBufferFromS3.cpp +++ b/src/IO/WriteBufferFromS3.cpp @@ -109,8 +109,8 @@ void WriteBufferFromS3::nextImpl() if (is_prefinalized) throw Exception( - ErrorCodes::LOGICAL_ERROR, - "Cannot write to prefinalized buffer for S3, the file could have been created with PutObjectRequest"); + ErrorCodes::LOGICAL_ERROR, + "Cannot write to prefinalized buffer for S3, the file could have been created with PutObjectRequest"); /// Make sense to call waitIfAny before adding new async task to check if there is an exception /// The faster the exception is propagated the lesser time is spent for cancellation @@ -183,12 +183,15 @@ void WriteBufferFromS3::finalizeImpl() preFinalize(); if (std::uncaught_exception()) + { + tryLogCurrentException(__PRETTY_FUNCTION__); throw Exception( ErrorCodes::LOGICAL_ERROR, "Detected buffer finalization when an exception is unwinding the stack." " Do not call finalize buffer in destructors or when exception thrown." " Details {}", getLogDetails()); + } chassert(offset() == 0); chassert(hidden_size == 0); diff --git a/src/Storages/MergeTree/MergedBlockOutputStream.cpp b/src/Storages/MergeTree/MergedBlockOutputStream.cpp index d97da5a0b50..7bff58fc411 100644 --- a/src/Storages/MergeTree/MergedBlockOutputStream.cpp +++ b/src/Storages/MergeTree/MergedBlockOutputStream.cpp @@ -121,14 +121,15 @@ void MergedBlockOutputStream::Finalizer::Impl::finish() MergedBlockOutputStream::Finalizer::~Finalizer() { - try - { - finish(); - } - catch (...) - { - tryLogCurrentException("MergedBlockOutputStream"); - } + /// ! #50274 +// try +// { +// finish(); +// } +// catch (...) +// { +// tryLogCurrentException("MergedBlockOutputStream"); +// } } MergedBlockOutputStream::Finalizer::Finalizer(Finalizer &&) noexcept = default; From 13dcb62ffb7abfe12bef0fb300e9c9e62610f754 Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Sun, 28 May 2023 12:29:38 +0200 Subject: [PATCH 26/44] fix logs engins --- src/IO/WriteBufferFromS3.cpp | 8 +++++++- src/Storages/StorageLog.cpp | 3 +++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/IO/WriteBufferFromS3.cpp b/src/IO/WriteBufferFromS3.cpp index 58d58e86ebe..5bfc6b91946 100644 --- a/src/IO/WriteBufferFromS3.cpp +++ b/src/IO/WriteBufferFromS3.cpp @@ -253,7 +253,13 @@ WriteBufferFromS3::~WriteBufferFromS3() // That destructor could be call with finalized=false in case of exceptions if (!finalized) { - LOG_ERROR(log, "WriteBufferFromS3 is not finalized in destructor. It could be if an exception occurs. File is not written to S3. {}.", getLogDetails()); + LOG_INFO(log, + "WriteBufferFromS3 is not finalized in destructor. " + "It could be if an exception occurs. File is not written to S3. " + "{}. " + "Stack trace: {}", + getLogDetails(), + StackTrace().toString()); } task_tracker->safeWaitAll(); diff --git a/src/Storages/StorageLog.cpp b/src/Storages/StorageLog.cpp index f698f1881fa..02dc4843660 100644 --- a/src/Storages/StorageLog.cpp +++ b/src/Storages/StorageLog.cpp @@ -341,7 +341,10 @@ private: void finalize() { compressed.next(); + compressed.finalize(); + plain->next(); + plain->finalize(); } }; From b13990efcc184c17356a14b67ec5057718f76e4c Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Sun, 28 May 2023 15:32:10 +0200 Subject: [PATCH 27/44] fix build --- src/IO/WriteBufferFromS3.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/IO/WriteBufferFromS3.cpp b/src/IO/WriteBufferFromS3.cpp index 5bfc6b91946..640763f793c 100644 --- a/src/IO/WriteBufferFromS3.cpp +++ b/src/IO/WriteBufferFromS3.cpp @@ -182,7 +182,7 @@ void WriteBufferFromS3::finalizeImpl() if (!is_prefinalized) preFinalize(); - if (std::uncaught_exception()) + if (std::uncaught_exceptions()) { tryLogCurrentException(__PRETTY_FUNCTION__); throw Exception( From 0b4ea3e2e1d5d91d96bbb13965b00a607664879d Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Mon, 29 May 2023 00:37:45 +0200 Subject: [PATCH 28/44] remove reminder comments --- src/Compression/CompressedWriteBuffer.cpp | 1 - src/Coordination/WriteBufferFromNuraftBuffer.cpp | 1 - src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp | 1 - src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp | 1 - src/Disks/IO/WriteBufferWithFinalizeCallback.cpp | 14 -------------- src/Disks/IO/WriteBufferWithFinalizeCallback.h | 2 -- src/IO/Archives/ZipArchiveWriter.cpp | 2 -- src/IO/BrotliWriteBuffer.cpp | 1 - src/IO/Bzip2WriteBuffer.cpp | 1 - src/IO/ForkWriteBuffer.cpp | 1 - src/IO/LZMADeflatingWriteBuffer.cpp | 1 - src/IO/Lz4DeflatingWriteBuffer.cpp | 1 - src/IO/SnappyWriteBuffer.cpp | 1 - src/IO/WriteBufferFromEncryptedFile.cpp | 1 - src/IO/WriteBufferFromFile.cpp | 1 - src/IO/WriteBufferFromFileDecorator.cpp | 10 ---------- src/IO/WriteBufferFromFileDescriptor.cpp | 1 - src/IO/WriteBufferFromOStream.cpp | 1 - src/IO/WriteBufferFromPocoSocket.cpp | 1 - src/IO/WriteBufferFromS3.cpp | 4 ++-- src/IO/WriteBufferFromS3TaskTracker.cpp | 8 -------- src/IO/WriteBufferFromVector.h | 1 - src/IO/ZlibDeflatingWriteBuffer.cpp | 1 - src/IO/ZstdDeflatingAppendableWriteBuffer.cpp | 1 - src/IO/ZstdDeflatingWriteBuffer.cpp | 1 - .../Cache/WriteBufferToFileSegment.cpp | 1 - src/Interpreters/TemporaryDataOnDisk.cpp | 1 - .../HTTP/WriteBufferFromHTTPServerResponse.cpp | 1 - src/Storages/HDFS/WriteBufferFromHDFS.cpp | 1 - .../MergeTree/MergedBlockOutputStream.cpp | 15 ++------------- src/Storages/MergeTree/MergedBlockOutputStream.h | 3 ++- tests/integration/test_merge_tree_s3/test.py | 8 ++++---- 32 files changed, 10 insertions(+), 79 deletions(-) diff --git a/src/Compression/CompressedWriteBuffer.cpp b/src/Compression/CompressedWriteBuffer.cpp index 22c5f235a27..cb2ee1140d0 100644 --- a/src/Compression/CompressedWriteBuffer.cpp +++ b/src/Compression/CompressedWriteBuffer.cpp @@ -59,7 +59,6 @@ void CompressedWriteBuffer::nextImpl() CompressedWriteBuffer::~CompressedWriteBuffer() { - /// ! #50274 finalize(); } diff --git a/src/Coordination/WriteBufferFromNuraftBuffer.cpp b/src/Coordination/WriteBufferFromNuraftBuffer.cpp index 41b933717c6..c955d3fdbbe 100644 --- a/src/Coordination/WriteBufferFromNuraftBuffer.cpp +++ b/src/Coordination/WriteBufferFromNuraftBuffer.cpp @@ -53,7 +53,6 @@ nuraft::ptr WriteBufferFromNuraftBuffer::getBuffer() WriteBufferFromNuraftBuffer::~WriteBufferFromNuraftBuffer() { - /// ! #50274 try { finalize(); diff --git a/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp b/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp index 8da9ce73ae3..9153af90312 100644 --- a/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp +++ b/src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp @@ -123,7 +123,6 @@ void FileSegmentRangeWriter::finalize() FileSegmentRangeWriter::~FileSegmentRangeWriter() { - /// ! #50274 try { if (!finalized) diff --git a/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp b/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp index 8ac2c8ca831..b5d296bd865 100644 --- a/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp +++ b/src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp @@ -38,7 +38,6 @@ WriteBufferFromAzureBlobStorage::WriteBufferFromAzureBlobStorage( WriteBufferFromAzureBlobStorage::~WriteBufferFromAzureBlobStorage() { - /// ! #50274 finalize(); } diff --git a/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp b/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp index 47bf742fd5b..8703eae4913 100644 --- a/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp +++ b/src/Disks/IO/WriteBufferWithFinalizeCallback.cpp @@ -13,20 +13,6 @@ WriteBufferWithFinalizeCallback::WriteBufferWithFinalizeCallback( { } - -WriteBufferWithFinalizeCallback::~WriteBufferWithFinalizeCallback() -{ - /// ! #50274 -// try -// { -// finalize(); -// } -// catch (...) -// { -// tryLogCurrentException(__PRETTY_FUNCTION__); -// } -} - void WriteBufferWithFinalizeCallback::finalizeImpl() { WriteBufferFromFileDecorator::finalizeImpl(); diff --git a/src/Disks/IO/WriteBufferWithFinalizeCallback.h b/src/Disks/IO/WriteBufferWithFinalizeCallback.h index 73c1b8d25d4..2798582c336 100644 --- a/src/Disks/IO/WriteBufferWithFinalizeCallback.h +++ b/src/Disks/IO/WriteBufferWithFinalizeCallback.h @@ -19,8 +19,6 @@ public: FinalizeCallback && create_callback_, const String & remote_path_); - ~WriteBufferWithFinalizeCallback() override; - String getFileName() const override { return remote_path; } private: diff --git a/src/IO/Archives/ZipArchiveWriter.cpp b/src/IO/Archives/ZipArchiveWriter.cpp index 1c6ef18d372..ed4b3502b2f 100644 --- a/src/IO/Archives/ZipArchiveWriter.cpp +++ b/src/IO/Archives/ZipArchiveWriter.cpp @@ -115,7 +115,6 @@ public: ~WriteBufferFromZipArchive() override { - /// ! #50274 try { finalize(); @@ -194,7 +193,6 @@ namespace ~StreamFromWriteBuffer() { - /// ! #50274 write_buffer->finalize(); } diff --git a/src/IO/BrotliWriteBuffer.cpp b/src/IO/BrotliWriteBuffer.cpp index 98786313b09..47426d62a6e 100644 --- a/src/IO/BrotliWriteBuffer.cpp +++ b/src/IO/BrotliWriteBuffer.cpp @@ -44,7 +44,6 @@ BrotliWriteBuffer::BrotliWriteBuffer(std::unique_ptr out_, int comp BrotliWriteBuffer::~BrotliWriteBuffer() { - /// ! #50274 finalize(); } diff --git a/src/IO/Bzip2WriteBuffer.cpp b/src/IO/Bzip2WriteBuffer.cpp index c7159d4d002..4b6bed70d35 100644 --- a/src/IO/Bzip2WriteBuffer.cpp +++ b/src/IO/Bzip2WriteBuffer.cpp @@ -47,7 +47,6 @@ Bzip2WriteBuffer::Bzip2WriteBuffer(std::unique_ptr out_, int compre Bzip2WriteBuffer::~Bzip2WriteBuffer() { - /// ! #50274 finalize(); } diff --git a/src/IO/ForkWriteBuffer.cpp b/src/IO/ForkWriteBuffer.cpp index ea38f70745d..8e11b9ff590 100644 --- a/src/IO/ForkWriteBuffer.cpp +++ b/src/IO/ForkWriteBuffer.cpp @@ -53,7 +53,6 @@ void ForkWriteBuffer::finalizeImpl() ForkWriteBuffer::~ForkWriteBuffer() { - /// ! #50274 finalize(); } diff --git a/src/IO/LZMADeflatingWriteBuffer.cpp b/src/IO/LZMADeflatingWriteBuffer.cpp index 07597ce958b..30e247b1016 100644 --- a/src/IO/LZMADeflatingWriteBuffer.cpp +++ b/src/IO/LZMADeflatingWriteBuffer.cpp @@ -46,7 +46,6 @@ LZMADeflatingWriteBuffer::LZMADeflatingWriteBuffer( LZMADeflatingWriteBuffer::~LZMADeflatingWriteBuffer() { - /// ! #50274 finalize(); } diff --git a/src/IO/Lz4DeflatingWriteBuffer.cpp b/src/IO/Lz4DeflatingWriteBuffer.cpp index 988d9861a9b..c3a1b8282c3 100644 --- a/src/IO/Lz4DeflatingWriteBuffer.cpp +++ b/src/IO/Lz4DeflatingWriteBuffer.cpp @@ -42,7 +42,6 @@ Lz4DeflatingWriteBuffer::Lz4DeflatingWriteBuffer( Lz4DeflatingWriteBuffer::~Lz4DeflatingWriteBuffer() { - /// ! #50274 finalize(); } diff --git a/src/IO/SnappyWriteBuffer.cpp b/src/IO/SnappyWriteBuffer.cpp index 5203d8b053e..ca40d0656d1 100644 --- a/src/IO/SnappyWriteBuffer.cpp +++ b/src/IO/SnappyWriteBuffer.cpp @@ -22,7 +22,6 @@ SnappyWriteBuffer::SnappyWriteBuffer(std::unique_ptr out_, size_t b SnappyWriteBuffer::~SnappyWriteBuffer() { - /// ! #50274 finish(); } diff --git a/src/IO/WriteBufferFromEncryptedFile.cpp b/src/IO/WriteBufferFromEncryptedFile.cpp index 5831b3aee94..5bca0dc68d5 100644 --- a/src/IO/WriteBufferFromEncryptedFile.cpp +++ b/src/IO/WriteBufferFromEncryptedFile.cpp @@ -21,7 +21,6 @@ WriteBufferFromEncryptedFile::WriteBufferFromEncryptedFile( WriteBufferFromEncryptedFile::~WriteBufferFromEncryptedFile() { - /// ! #50274 finalize(); } diff --git a/src/IO/WriteBufferFromFile.cpp b/src/IO/WriteBufferFromFile.cpp index 0777d596184..e58f1e3a60c 100644 --- a/src/IO/WriteBufferFromFile.cpp +++ b/src/IO/WriteBufferFromFile.cpp @@ -77,7 +77,6 @@ WriteBufferFromFile::~WriteBufferFromFile() if (fd < 0) return; - /// ! #50274 finalize(); int err = ::close(fd); /// Everything except for EBADF should be ignored in dtor, since all of diff --git a/src/IO/WriteBufferFromFileDecorator.cpp b/src/IO/WriteBufferFromFileDecorator.cpp index 67a98d9e43f..0e4e5e13a86 100644 --- a/src/IO/WriteBufferFromFileDecorator.cpp +++ b/src/IO/WriteBufferFromFileDecorator.cpp @@ -30,16 +30,6 @@ void WriteBufferFromFileDecorator::finalizeImpl() WriteBufferFromFileDecorator::~WriteBufferFromFileDecorator() { - /// ! #50274 -// try -// { -// finalize(); -// } -// catch (...) -// { -// tryLogCurrentException(__PRETTY_FUNCTION__); -// } - /// It is not a mistake that swap is called here /// Swap has been called at constructor, it should be called at destructor /// In oreder to provide valid buffer for impl's d-tor call diff --git a/src/IO/WriteBufferFromFileDescriptor.cpp b/src/IO/WriteBufferFromFileDescriptor.cpp index 03988c09f62..135ff608967 100644 --- a/src/IO/WriteBufferFromFileDescriptor.cpp +++ b/src/IO/WriteBufferFromFileDescriptor.cpp @@ -105,7 +105,6 @@ WriteBufferFromFileDescriptor::WriteBufferFromFileDescriptor( WriteBufferFromFileDescriptor::~WriteBufferFromFileDescriptor() { - /// ! #50274 finalize(); } diff --git a/src/IO/WriteBufferFromOStream.cpp b/src/IO/WriteBufferFromOStream.cpp index 25886b9d10d..2d0d5976f85 100644 --- a/src/IO/WriteBufferFromOStream.cpp +++ b/src/IO/WriteBufferFromOStream.cpp @@ -40,7 +40,6 @@ WriteBufferFromOStream::WriteBufferFromOStream( WriteBufferFromOStream::~WriteBufferFromOStream() { - /// ! #50274 finalize(); } diff --git a/src/IO/WriteBufferFromPocoSocket.cpp b/src/IO/WriteBufferFromPocoSocket.cpp index a0dcf69e67d..039110dfb62 100644 --- a/src/IO/WriteBufferFromPocoSocket.cpp +++ b/src/IO/WriteBufferFromPocoSocket.cpp @@ -106,7 +106,6 @@ WriteBufferFromPocoSocket::WriteBufferFromPocoSocket(Poco::Net::Socket & socket_ WriteBufferFromPocoSocket::~WriteBufferFromPocoSocket() { - /// ! #50274 finalize(); } diff --git a/src/IO/WriteBufferFromS3.cpp b/src/IO/WriteBufferFromS3.cpp index 640763f793c..44feb225471 100644 --- a/src/IO/WriteBufferFromS3.cpp +++ b/src/IO/WriteBufferFromS3.cpp @@ -538,7 +538,7 @@ void WriteBufferFromS3::completeMultipartUpload() if (multipart_tags.empty()) throw Exception( - ErrorCodes::S3_ERROR, + ErrorCodes::LOGICAL_ERROR, "Failed to complete multipart upload. No parts have uploaded"); for (size_t i = 0; i < multipart_tags.size(); ++i) @@ -546,7 +546,7 @@ void WriteBufferFromS3::completeMultipartUpload() const auto tag = multipart_tags.at(i); if (tag.empty()) throw Exception( - ErrorCodes::S3_ERROR, + ErrorCodes::LOGICAL_ERROR, "Failed to complete multipart upload. Part {} haven't been uploaded.", i); } diff --git a/src/IO/WriteBufferFromS3TaskTracker.cpp b/src/IO/WriteBufferFromS3TaskTracker.cpp index 7cf5a89df86..7ae31044012 100644 --- a/src/IO/WriteBufferFromS3TaskTracker.cpp +++ b/src/IO/WriteBufferFromS3TaskTracker.cpp @@ -162,14 +162,6 @@ void WriteBufferFromS3::TaskTracker::waitTilInflightShrink() for (auto & it : finished_futures) { -// SCOPE_EXIT({ -// /// According to basic exception safety TaskTracker has to be destroyed after exception -// /// If it would be true than this SCOPE_EXIT is superfluous -// /// However WriteBufferWithFinalizeCallback, WriteBufferFromFileDecorator do call finalize in d-tor -// /// TaskTracker has to cope this until the issue with finalizing in d-tor is addressed in #50274 -// futures.erase(it); -// }); - it->get(); futures.erase(it); } diff --git a/src/IO/WriteBufferFromVector.h b/src/IO/WriteBufferFromVector.h index ab4f27a7bcb..4b2a3581625 100644 --- a/src/IO/WriteBufferFromVector.h +++ b/src/IO/WriteBufferFromVector.h @@ -63,7 +63,6 @@ public: ~WriteBufferFromVector() override { - /// ! #50274 finalize(); } diff --git a/src/IO/ZlibDeflatingWriteBuffer.cpp b/src/IO/ZlibDeflatingWriteBuffer.cpp index 7ae8f636f69..43bb0405555 100644 --- a/src/IO/ZlibDeflatingWriteBuffer.cpp +++ b/src/IO/ZlibDeflatingWriteBuffer.cpp @@ -74,7 +74,6 @@ void ZlibDeflatingWriteBuffer::nextImpl() ZlibDeflatingWriteBuffer::~ZlibDeflatingWriteBuffer() { - /// ! #50274 try { finalize(); diff --git a/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp b/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp index 8f66618f513..be739c0e654 100644 --- a/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp +++ b/src/IO/ZstdDeflatingAppendableWriteBuffer.cpp @@ -87,7 +87,6 @@ void ZstdDeflatingAppendableWriteBuffer::nextImpl() ZstdDeflatingAppendableWriteBuffer::~ZstdDeflatingAppendableWriteBuffer() { - /// ! #50274 finalize(); } diff --git a/src/IO/ZstdDeflatingWriteBuffer.cpp b/src/IO/ZstdDeflatingWriteBuffer.cpp index 95e2e7d7716..c6d2ffc39f9 100644 --- a/src/IO/ZstdDeflatingWriteBuffer.cpp +++ b/src/IO/ZstdDeflatingWriteBuffer.cpp @@ -33,7 +33,6 @@ ZstdDeflatingWriteBuffer::ZstdDeflatingWriteBuffer( ZstdDeflatingWriteBuffer::~ZstdDeflatingWriteBuffer() { - /// ! #50274 finalize(); } diff --git a/src/Interpreters/Cache/WriteBufferToFileSegment.cpp b/src/Interpreters/Cache/WriteBufferToFileSegment.cpp index cbbf22078c1..1eac87a804d 100644 --- a/src/Interpreters/Cache/WriteBufferToFileSegment.cpp +++ b/src/Interpreters/Cache/WriteBufferToFileSegment.cpp @@ -73,7 +73,6 @@ std::shared_ptr WriteBufferToFileSegment::getReadBufferImpl() WriteBufferToFileSegment::~WriteBufferToFileSegment() { - /// ! #50274 try { finalize(); diff --git a/src/Interpreters/TemporaryDataOnDisk.cpp b/src/Interpreters/TemporaryDataOnDisk.cpp index 2fc21e84be7..69fef21dbab 100644 --- a/src/Interpreters/TemporaryDataOnDisk.cpp +++ b/src/Interpreters/TemporaryDataOnDisk.cpp @@ -184,7 +184,6 @@ struct TemporaryFileStream::OutputWriter ~OutputWriter() { - /// ! #50274 try { finalize(); diff --git a/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp b/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp index 7d6710e76e6..c8015cfd185 100644 --- a/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp +++ b/src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp @@ -171,7 +171,6 @@ void WriteBufferFromHTTPServerResponse::onProgress(const Progress & progress) WriteBufferFromHTTPServerResponse::~WriteBufferFromHTTPServerResponse() { - /// ! #50274 finalize(); } diff --git a/src/Storages/HDFS/WriteBufferFromHDFS.cpp b/src/Storages/HDFS/WriteBufferFromHDFS.cpp index 169cf845d3b..fad0447d2cf 100644 --- a/src/Storages/HDFS/WriteBufferFromHDFS.cpp +++ b/src/Storages/HDFS/WriteBufferFromHDFS.cpp @@ -147,7 +147,6 @@ void WriteBufferFromHDFS::finalizeImpl() WriteBufferFromHDFS::~WriteBufferFromHDFS() { - /// ! #50274 finalize(); } diff --git a/src/Storages/MergeTree/MergedBlockOutputStream.cpp b/src/Storages/MergeTree/MergedBlockOutputStream.cpp index 7bff58fc411..7e69f03e959 100644 --- a/src/Storages/MergeTree/MergedBlockOutputStream.cpp +++ b/src/Storages/MergeTree/MergedBlockOutputStream.cpp @@ -119,23 +119,12 @@ void MergedBlockOutputStream::Finalizer::Impl::finish() part->getDataPartStorage().removeFile(file_name); } -MergedBlockOutputStream::Finalizer::~Finalizer() -{ - /// ! #50274 -// try -// { -// finish(); -// } -// catch (...) -// { -// tryLogCurrentException("MergedBlockOutputStream"); -// } -} - MergedBlockOutputStream::Finalizer::Finalizer(Finalizer &&) noexcept = default; MergedBlockOutputStream::Finalizer & MergedBlockOutputStream::Finalizer::operator=(Finalizer &&) noexcept = default; MergedBlockOutputStream::Finalizer::Finalizer(std::unique_ptr impl_) : impl(std::move(impl_)) {} +MergedBlockOutputStream::Finalizer::~Finalizer() {} + void MergedBlockOutputStream::finalizePart( const MergeTreeMutableDataPartPtr & new_part, bool sync, diff --git a/src/Storages/MergeTree/MergedBlockOutputStream.h b/src/Storages/MergeTree/MergedBlockOutputStream.h index ad1bb584788..f3a5653a880 100644 --- a/src/Storages/MergeTree/MergedBlockOutputStream.h +++ b/src/Storages/MergeTree/MergedBlockOutputStream.h @@ -44,9 +44,10 @@ public: std::unique_ptr impl; explicit Finalizer(std::unique_ptr impl_); - ~Finalizer(); Finalizer(Finalizer &&) noexcept; Finalizer & operator=(Finalizer &&) noexcept; + ~Finalizer(); + void finish(); }; diff --git a/tests/integration/test_merge_tree_s3/test.py b/tests/integration/test_merge_tree_s3/test.py index 4dcc5805294..626a71f006e 100644 --- a/tests/integration/test_merge_tree_s3/test.py +++ b/tests/integration/test_merge_tree_s3/test.py @@ -1049,8 +1049,8 @@ def test_s3_engine_heavy_write_check_mem(cluster, node_name, in_flight_memory): " AND type!='QueryStart'" ).split() - assert int(memory_usage) < 1.1 * memory - assert int(memory_usage) > 0.9 * memory + assert int(memory_usage) < 1.2 * memory + assert int(memory_usage) > 0.8 * memory assert int(wait_inflight) > 10 * 1000 * 1000 @@ -1097,7 +1097,7 @@ def test_s3_disk_heavy_write_check_mem(cluster, node_name): " AND type!='QueryStart'" ) - assert int(result) < 1.1 * memory - assert int(result) > 0.9 * memory + assert int(result) < 1.2 * memory + assert int(result) > 0.8 * memory check_no_objects_after_drop(cluster, node_name=node_name) From 2d2b411c26fc4e64bce894a48684c144f3b689b1 Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Tue, 30 May 2023 12:21:33 +0200 Subject: [PATCH 29/44] fix build --- src/Storages/MergeTree/MergedBlockOutputStream.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/MergedBlockOutputStream.cpp b/src/Storages/MergeTree/MergedBlockOutputStream.cpp index 7e69f03e959..c93ad135835 100644 --- a/src/Storages/MergeTree/MergedBlockOutputStream.cpp +++ b/src/Storages/MergeTree/MergedBlockOutputStream.cpp @@ -123,7 +123,7 @@ MergedBlockOutputStream::Finalizer::Finalizer(Finalizer &&) noexcept = default; MergedBlockOutputStream::Finalizer & MergedBlockOutputStream::Finalizer::operator=(Finalizer &&) noexcept = default; MergedBlockOutputStream::Finalizer::Finalizer(std::unique_ptr impl_) : impl(std::move(impl_)) {} -MergedBlockOutputStream::Finalizer::~Finalizer() {} +MergedBlockOutputStream::Finalizer::~Finalizer() = default; void MergedBlockOutputStream::finalizePart( const MergeTreeMutableDataPartPtr & new_part, From 0a128cec6179fbc97c9329538f3e28c2ca1567d1 Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Tue, 30 May 2023 12:28:58 +0200 Subject: [PATCH 30/44] remove tricky debug trap --- src/IO/WriteBufferFromS3.cpp | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/IO/WriteBufferFromS3.cpp b/src/IO/WriteBufferFromS3.cpp index 44feb225471..462cf2674c3 100644 --- a/src/IO/WriteBufferFromS3.cpp +++ b/src/IO/WriteBufferFromS3.cpp @@ -182,17 +182,6 @@ void WriteBufferFromS3::finalizeImpl() if (!is_prefinalized) preFinalize(); - if (std::uncaught_exceptions()) - { - tryLogCurrentException(__PRETTY_FUNCTION__); - throw Exception( - ErrorCodes::LOGICAL_ERROR, - "Detected buffer finalization when an exception is unwinding the stack." - " Do not call finalize buffer in destructors or when exception thrown." - " Details {}", - getLogDetails()); - } - chassert(offset() == 0); chassert(hidden_size == 0); From a69aa3f901d24a083f1be3d90eaa927138617f19 Mon Sep 17 00:00:00 2001 From: Tyler Hannan Date: Tue, 30 May 2023 13:39:32 +0200 Subject: [PATCH 31/44] Update README.md --- README.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index bbedea364fc..9c933540e01 100644 --- a/README.md +++ b/README.md @@ -23,10 +23,9 @@ curl https://clickhouse.com/ | sh ## Upcoming Events * [**v23.5 Release Webinar**](https://clickhouse.com/company/events/v23-5-release-webinar?utm_source=github&utm_medium=social&utm_campaign=release-webinar-2023-05) - May 31 - 23.5 is rapidly approaching. Original creator, co-founder, and CTO of ClickHouse Alexey Milovidov will walk us through the highlights of the release. -* [**ClickHouse Meetup in Barcelona**](https://www.meetup.com/clickhouse-barcelona-user-group/events/292892669) - May 25 -* [**ClickHouse Meetup in London**](https://www.meetup.com/clickhouse-london-user-group/events/292892824) - May 25 +* [**ClickHouse Meetup in Bangalore**](https://www.meetup.com/clickhouse-bangalore-user-group/events/293740066/) - Jun 7 * [**ClickHouse Meetup in San Francisco**](https://www.meetup.com/clickhouse-silicon-valley-meetup-group/events/293426725/) - Jun 7 -* [**ClickHouse Meetup in Stockholm**](https://www.meetup.com/clickhouse-berlin-user-group/events/292892466) - Jun 13 + Also, keep an eye out for upcoming meetups in Amsterdam, Boston, NYC, Beijing, and Toronto. Somewhere else you want us to be? Please feel free to reach out to tyler clickhouse com. From 91a3c881267d1d725db78da4ec8ed5d53ae230fb Mon Sep 17 00:00:00 2001 From: Sema Checherinda Date: Tue, 30 May 2023 15:49:47 +0200 Subject: [PATCH 32/44] less logs in WriteBufferFromS3 --- src/IO/WriteBufferFromS3.cpp | 8 -------- src/IO/WriteBufferFromS3TaskTracker.cpp | 11 ----------- 2 files changed, 19 deletions(-) diff --git a/src/IO/WriteBufferFromS3.cpp b/src/IO/WriteBufferFromS3.cpp index 954c996d929..ffdc23b274b 100644 --- a/src/IO/WriteBufferFromS3.cpp +++ b/src/IO/WriteBufferFromS3.cpp @@ -195,18 +195,14 @@ void WriteBufferFromS3::finalizeImpl() if (request_settings.check_objects_after_upload) { - LOG_TRACE(log, "Checking object {} exists after upload", key); S3::checkObjectExists(*client_ptr, bucket, key, {}, request_settings, /* for_disk_s3= */ write_settings.for_object_storage, "Immediately after upload"); - LOG_TRACE(log, "Checking object {} has size as expected {}", key, total_size); size_t actual_size = S3::getObjectSize(*client_ptr, bucket, key, {}, request_settings, /* for_disk_s3= */ write_settings.for_object_storage); if (actual_size != total_size) throw Exception( ErrorCodes::S3_ERROR, "Object {} from bucket {} has unexpected size {} after upload, expected size {}, it's a bug in S3 or S3 API.", key, bucket, actual_size, total_size); - - LOG_TRACE(log, "Object {} exists after upload", key); } } @@ -286,8 +282,6 @@ void WriteBufferFromS3::reallocateFirstBuffer() WriteBuffer::set(memory.data() + hidden_size, memory.size() - hidden_size); chassert(offset() == 0); - - LOG_TRACE(log, "Reallocated first buffer with size {}. {}", memory.size(), getLogDetails()); } void WriteBufferFromS3::detachBuffer() @@ -310,8 +304,6 @@ void WriteBufferFromS3::allocateFirstBuffer() const auto size = std::min(size_t(DBMS_DEFAULT_BUFFER_SIZE), max_first_buffer); memory = Memory(size); WriteBuffer::set(memory.data(), memory.size()); - - LOG_TRACE(log, "Allocated first buffer with size {}. {}", memory.size(), getLogDetails()); } void WriteBufferFromS3::allocateBuffer() diff --git a/src/IO/WriteBufferFromS3TaskTracker.cpp b/src/IO/WriteBufferFromS3TaskTracker.cpp index 4abae90eeac..b023de16c98 100644 --- a/src/IO/WriteBufferFromS3TaskTracker.cpp +++ b/src/IO/WriteBufferFromS3TaskTracker.cpp @@ -36,8 +36,6 @@ ThreadPoolCallbackRunner WriteBufferFromS3::TaskTracker::syncRunner() void WriteBufferFromS3::TaskTracker::waitAll() { - LOG_TEST(log, "waitAll, in queue {}", futures.size()); - /// Exceptions are propagated for (auto & future : futures) { @@ -51,8 +49,6 @@ void WriteBufferFromS3::TaskTracker::waitAll() void WriteBufferFromS3::TaskTracker::safeWaitAll() { - LOG_TEST(log, "safeWaitAll, wait in queue {}", futures.size()); - for (auto & future : futures) { if (future.valid()) @@ -76,7 +72,6 @@ void WriteBufferFromS3::TaskTracker::safeWaitAll() void WriteBufferFromS3::TaskTracker::waitIfAny() { - LOG_TEST(log, "waitIfAny, in queue {}", futures.size()); if (futures.empty()) return; @@ -101,8 +96,6 @@ void WriteBufferFromS3::TaskTracker::waitIfAny() watch.stop(); ProfileEvents::increment(ProfileEvents::WriteBufferFromS3WaitInflightLimitMicroseconds, watch.elapsedMicroseconds()); - - LOG_TEST(log, "waitIfAny ended, in queue {}", futures.size()); } void WriteBufferFromS3::TaskTracker::add(Callback && func) @@ -147,8 +140,6 @@ void WriteBufferFromS3::TaskTracker::waitTilInflightShrink() if (!max_tasks_inflight) return; - LOG_TEST(log, "waitTilInflightShrink, in queue {}", futures.size()); - Stopwatch watch; /// Alternative approach is to wait until at least futures.size() - max_tasks_inflight element are finished @@ -178,8 +169,6 @@ void WriteBufferFromS3::TaskTracker::waitTilInflightShrink() watch.stop(); ProfileEvents::increment(ProfileEvents::WriteBufferFromS3WaitInflightLimitMicroseconds, watch.elapsedMicroseconds()); - - LOG_TEST(log, "waitTilInflightShrink ended, in queue {}", futures.size()); } bool WriteBufferFromS3::TaskTracker::isAsync() const From 40d658e467549d589116da449f2ac8edbfe54368 Mon Sep 17 00:00:00 2001 From: vdimir Date: Mon, 1 May 2023 15:09:26 +0000 Subject: [PATCH 33/44] Fix join_use_nulls in analyzer --- src/Analyzer/Passes/QueryAnalysisPass.cpp | 81 +++- .../02722_matcher_join_use_nulls.reference | 430 ++++++++++++++++++ .../02722_matcher_join_use_nulls.sql.j2 | 119 +++++ 3 files changed, 612 insertions(+), 18 deletions(-) create mode 100644 tests/queries/0_stateless/02722_matcher_join_use_nulls.reference create mode 100644 tests/queries/0_stateless/02722_matcher_join_use_nulls.sql.j2 diff --git a/src/Analyzer/Passes/QueryAnalysisPass.cpp b/src/Analyzer/Passes/QueryAnalysisPass.cpp index aa915e48d35..36a85739339 100644 --- a/src/Analyzer/Passes/QueryAnalysisPass.cpp +++ b/src/Analyzer/Passes/QueryAnalysisPass.cpp @@ -1205,6 +1205,29 @@ private: static std::string rewriteAggregateFunctionNameIfNeeded(const std::string & aggregate_function_name, const ContextPtr & context); + static std::optional getColumnSideFromJoinTree(QueryTreeNodePtr & resolved_identifier, const JoinNode & join_node) + { + const auto * column_src = resolved_identifier->as().getColumnSource().get(); + + if (join_node.getLeftTableExpression().get() == column_src) + return JoinTableSide::Left; + if (join_node.getRightTableExpression().get() == column_src) + return JoinTableSide::Right; + return {}; + } + + static void convertJoinedColumnTypeToNullIfNeeded(QueryTreeNodePtr & resolved_identifier, const JoinKind & join_kind, std::optional resolved_side) + { + if (resolved_identifier->getNodeType() == QueryTreeNodeType::COLUMN && + (isFull(join_kind) || + (isLeft(join_kind) && resolved_side && *resolved_side == JoinTableSide::Right) || + (isRight(join_kind) && resolved_side && *resolved_side == JoinTableSide::Left))) + { + auto & resolved_column = resolved_identifier->as(); + resolved_column.setColumnType(makeNullableOrLowCardinalityNullable(resolved_column.getColumnType())); + } + } + /// Resolve identifier functions static QueryTreeNodePtr tryResolveTableIdentifierFromDatabaseCatalog(const Identifier & table_identifier, ContextPtr context); @@ -2982,6 +3005,7 @@ QueryTreeNodePtr QueryAnalyzer::tryResolveIdentifierFromJoin(const IdentifierLoo QueryTreeNodePtr resolved_identifier; JoinKind join_kind = from_join_node.getKind(); + bool join_use_nulls = scope.context->getSettingsRef().join_use_nulls; if (left_resolved_identifier && right_resolved_identifier) { @@ -3027,19 +3051,31 @@ QueryTreeNodePtr QueryAnalyzer::tryResolveIdentifierFromJoin(const IdentifierLoo * * Otherwise we prefer column from left table. */ - if (identifier_path_part == right_column_source_alias) - return right_resolved_identifier; - else if (!left_column_source_alias.empty() && - right_column_source_alias.empty() && - identifier_path_part != left_column_source_alias) - return right_resolved_identifier; + bool column_resolved_using_right_alias = identifier_path_part == right_column_source_alias; + bool column_resolved_without_using_left_alias = !left_column_source_alias.empty() + && right_column_source_alias.empty() + && identifier_path_part != left_column_source_alias; + if (column_resolved_using_right_alias || column_resolved_without_using_left_alias) + { + resolved_side = JoinTableSide::Right; + resolved_identifier = right_resolved_identifier; + } + else + { + resolved_side = JoinTableSide::Left; + resolved_identifier = left_resolved_identifier; + } + } + else + { + resolved_side = JoinTableSide::Left; + resolved_identifier = left_resolved_identifier; } - - return left_resolved_identifier; } else if (scope.joins_count == 1 && scope.context->getSettingsRef().single_join_prefer_left_table) { - return left_resolved_identifier; + resolved_side = JoinTableSide::Left; + resolved_identifier = left_resolved_identifier; } else { @@ -3092,17 +3128,10 @@ QueryTreeNodePtr QueryAnalyzer::tryResolveIdentifierFromJoin(const IdentifierLoo if (join_node_in_resolve_process || !resolved_identifier) return resolved_identifier; - bool join_use_nulls = scope.context->getSettingsRef().join_use_nulls; - - if (join_use_nulls && - resolved_identifier->getNodeType() == QueryTreeNodeType::COLUMN && - (isFull(join_kind) || - (isLeft(join_kind) && resolved_side && *resolved_side == JoinTableSide::Right) || - (isRight(join_kind) && resolved_side && *resolved_side == JoinTableSide::Left))) + if (join_use_nulls) { resolved_identifier = resolved_identifier->clone(); - auto & resolved_column = resolved_identifier->as(); - resolved_column.setColumnType(makeNullableOrLowCardinalityNullable(resolved_column.getColumnType())); + convertJoinedColumnTypeToNullIfNeeded(resolved_identifier, join_kind, resolved_side); } return resolved_identifier; @@ -4001,6 +4030,22 @@ ProjectionNames QueryAnalyzer::resolveMatcher(QueryTreeNodePtr & matcher_node, I else matched_expression_nodes_with_names = resolveUnqualifiedMatcher(matcher_node, scope); + if (scope.context->getSettingsRef().join_use_nulls) + { + const auto * nearest_query_scope = scope.getNearestQueryScope(); + const QueryNode * nearest_scope_query_node = nearest_query_scope ? nearest_query_scope->scope_node->as() : nullptr; + const QueryTreeNodePtr & nearest_scope_join_tree = nearest_scope_query_node ? nearest_scope_query_node->getJoinTree() : nullptr; + const JoinNode * nearest_scope_join_node = nearest_scope_join_tree ? nearest_scope_join_tree->as() : nullptr; + if (nearest_scope_join_node) + { + for (auto & [node, node_name] : matched_expression_nodes_with_names) + { + auto join_identifier_side = getColumnSideFromJoinTree(node, *nearest_scope_join_node); + convertJoinedColumnTypeToNullIfNeeded(node, nearest_scope_join_node->getKind(), join_identifier_side); + } + } + } + std::unordered_map> strict_transformer_to_used_column_names; for (const auto & transformer : matcher_node_typed.getColumnTransformers().getNodes()) { diff --git a/tests/queries/0_stateless/02722_matcher_join_use_nulls.reference b/tests/queries/0_stateless/02722_matcher_join_use_nulls.reference new file mode 100644 index 00000000000..746d02dc381 --- /dev/null +++ b/tests/queries/0_stateless/02722_matcher_join_use_nulls.reference @@ -0,0 +1,430 @@ +-- { echoOn } + +SELECT '============ LEFT JOIN ============' FORMAT Null; +SELECT a, toTypeName(a) +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +1 Int32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +1 Int32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +1 \N Int32 Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +1 Int32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +\N Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM t1 + LEFT JOIN t2 + ON t1.a = t2.a +) ORDER BY 1; +\N \N Nullable(UInt32) Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +1 Int64 +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +1 Int64 +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +1 Int32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +\N Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM t1 + LEFT JOIN t2 + USING (a) +) ORDER BY 1; +1 \N Int64 Nullable(UInt32) +SELECT a, toTypeName(a) +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +1 Int32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +1 Int32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +1 \N Int32 Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +1 Int32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + LEFT JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +\N Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM t1 + LEFT JOIN t2 + ON t1.a = t2.key +) ORDER BY 1; +\N \N Nullable(UInt32) Nullable(UInt32) +SELECT '============ RIGHT JOIN ============' FORMAT Null; +SELECT a, toTypeName(a) +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +\N 2 Nullable(Int32) UInt32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +2 UInt32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM t1 + RIGHT JOIN t2 + ON t1.a = t2.a +) ORDER BY 1; +2 2 UInt32 UInt32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +2 Int64 +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +2 Int64 +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +2 Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +2 UInt32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM t1 + RIGHT JOIN t2 + USING (a) +) ORDER BY 1; +2 2 Int64 UInt32 +SELECT a, toTypeName(a) +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +\N 2 Nullable(Int32) UInt32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + RIGHT JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +2 UInt32 +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM t1 + RIGHT JOIN t2 + ON t1.a = t2.key +) ORDER BY 1; +2 2 UInt32 UInt32 +SELECT '============ FULL JOIN ============' FORMAT Null; +SELECT a, toTypeName(a) +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +1 Nullable(Int32) +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +1 Nullable(Int32) +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +1 \N Nullable(Int32) Nullable(UInt32) +\N 2 Nullable(Int32) Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +1 Nullable(Int32) +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as a) t2 + ON t1.a = t2.a +) ORDER BY 1; +2 Nullable(UInt32) +\N Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM t1 + FULL JOIN t2 + ON t1.a = t2.a +) ORDER BY 1; +2 2 Nullable(UInt32) Nullable(UInt32) +\N \N Nullable(UInt32) Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +1 Nullable(Int64) +2 Nullable(Int64) +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +1 Nullable(Int64) +2 Nullable(Int64) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +1 Nullable(Int32) +2 Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as a) t2 + USING (a) +) ORDER BY 1; +2 Nullable(UInt32) +\N Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM t1 + FULL JOIN t2 + USING (a) +) ORDER BY 1; +1 \N Nullable(Int64) Nullable(UInt32) +2 2 Nullable(Int64) Nullable(UInt32) +SELECT a, toTypeName(a) +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +1 Nullable(Int32) +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +1 Nullable(Int32) +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +1 \N Nullable(Int32) Nullable(UInt32) +\N 2 Nullable(Int32) Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +1 Nullable(Int32) +\N Nullable(Int32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + FULL JOIN (SELECT 2 :: UInt32 as key) t2 + ON t1.a = t2.key +) ORDER BY 1; +2 Nullable(UInt32) +\N Nullable(UInt32) +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM t1 + FULL JOIN t2 + ON t1.a = t2.key +) ORDER BY 1; +2 2 Nullable(UInt32) Nullable(UInt32) +\N \N Nullable(UInt32) Nullable(UInt32) diff --git a/tests/queries/0_stateless/02722_matcher_join_use_nulls.sql.j2 b/tests/queries/0_stateless/02722_matcher_join_use_nulls.sql.j2 new file mode 100644 index 00000000000..25451a34867 --- /dev/null +++ b/tests/queries/0_stateless/02722_matcher_join_use_nulls.sql.j2 @@ -0,0 +1,119 @@ +DROP TABLE IF EXISTS t1; +DROP TABLE IF EXISTS t2; + +CREATE TABLE t1 (a Int32) ENGINE = TinyLog; +CREATE TABLE t2 (a UInt32, key UInt32) ENGINE = TinyLog; + +INSERT INTO t1 VALUES (1); +INSERT INTO t2 VALUES (2, 2); + +SET join_use_nulls = 1; +SET allow_experimental_analyzer = 1; + +-- { echoOn } + +{% for KIND in ('LEFT', 'RIGHT', 'FULL') -%} + +SELECT '============ {{ KIND }} JOIN ============' FORMAT Null; + +{% for right_column_name in ['a', 'key'] -%} + +SELECT a, toTypeName(a) +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + {{ KIND }} JOIN (SELECT 2 :: UInt32 as {{ right_column_name }}) t2 + ON t1.a = t2.{{ right_column_name }} +) ORDER BY 1; + +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + {{ KIND }} JOIN (SELECT 2 :: UInt32 as {{ right_column_name }}) t2 + ON t1.a = t2.{{ right_column_name }} +) ORDER BY 1; + +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + {{ KIND }} JOIN (SELECT 2 :: UInt32 as {{ right_column_name }}) t2 + ON t1.a = t2.{{ right_column_name }} +) ORDER BY 1; + +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + {{ KIND }} JOIN (SELECT 2 :: UInt32 as {{ right_column_name }}) t2 + ON t1.a = t2.{{ right_column_name }} +) ORDER BY 1; + +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + {{ KIND }} JOIN (SELECT 2 :: UInt32 as {{ right_column_name }}) t2 + ON t1.a = t2.{{ right_column_name }} +) ORDER BY 1; + +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM t1 + {{ KIND }} JOIN t2 + ON t1.a = t2.{{ right_column_name }} +) ORDER BY 1; + +{% if right_column_name == 'a' -%} + +SELECT *, * APPLY toTypeName +FROM ( + SELECT a + FROM (SELECT 1 :: Int32 as a) t1 + {{ KIND }} JOIN (SELECT 2 :: UInt32 as {{ right_column_name }}) t2 + USING (a) +) ORDER BY 1; + +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM (SELECT 1 :: Int32 as a) t1 + {{ KIND }} JOIN (SELECT 2 :: UInt32 as {{ right_column_name }}) t2 + USING (a) +) ORDER BY 1; + +SELECT *, * APPLY toTypeName +FROM ( + SELECT t1.* + FROM (SELECT 1 :: Int32 as a) t1 + {{ KIND }} JOIN (SELECT 2 :: UInt32 as {{ right_column_name }}) t2 + USING (a) +) ORDER BY 1; + +SELECT *, * APPLY toTypeName +FROM ( + SELECT t2.* + FROM (SELECT 1 :: Int32 as a) t1 + {{ KIND }} JOIN (SELECT 2 :: UInt32 as {{ right_column_name }}) t2 + USING (a) +) ORDER BY 1; + +SELECT *, * APPLY toTypeName +FROM ( + SELECT * + FROM t1 + {{ KIND }} JOIN t2 + USING (a) +) ORDER BY 1; + +{% endif -%} + +{% endfor -%} +{% endfor -%} + +-- { echoOff } + +DROP TABLE IF EXISTS t1; +DROP TABLE IF EXISTS t2; From 12141bb6cdd42e8a2f9f985549d1c970dbb0295f Mon Sep 17 00:00:00 2001 From: vdimir Date: Tue, 9 May 2023 16:43:32 +0000 Subject: [PATCH 34/44] Check can become nullable before applying join_use_nulls --- src/Analyzer/Passes/QueryAnalysisPass.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Analyzer/Passes/QueryAnalysisPass.cpp b/src/Analyzer/Passes/QueryAnalysisPass.cpp index 36a85739339..8bacdfd7bd2 100644 --- a/src/Analyzer/Passes/QueryAnalysisPass.cpp +++ b/src/Analyzer/Passes/QueryAnalysisPass.cpp @@ -1219,6 +1219,7 @@ private: static void convertJoinedColumnTypeToNullIfNeeded(QueryTreeNodePtr & resolved_identifier, const JoinKind & join_kind, std::optional resolved_side) { if (resolved_identifier->getNodeType() == QueryTreeNodeType::COLUMN && + JoinCommon::canBecomeNullable(resolved_identifier->getResultType()) && (isFull(join_kind) || (isLeft(join_kind) && resolved_side && *resolved_side == JoinTableSide::Right) || (isRight(join_kind) && resolved_side && *resolved_side == JoinTableSide::Left))) From 6fb836a4b207aaa4cf174e03b33e8dd41b671952 Mon Sep 17 00:00:00 2001 From: vdimir Date: Wed, 10 May 2023 13:22:57 +0200 Subject: [PATCH 35/44] Update broken_tests.txt --- tests/broken_tests.txt | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/broken_tests.txt b/tests/broken_tests.txt index faee1c5b295..02935712325 100644 --- a/tests/broken_tests.txt +++ b/tests/broken_tests.txt @@ -21,8 +21,6 @@ 01072_optimize_skip_unused_shards_const_expr_eval 01083_expressions_in_engine_arguments 01086_odbc_roundtrip -01142_join_lc_and_nullable_in_key -01142_merge_join_lc_and_nullable_in_key 01152_cross_replication 01155_rename_move_materialized_view 01173_transaction_control_queries @@ -39,8 +37,6 @@ 01319_optimize_skip_unused_shards_nesting 01353_low_cardinality_join_types 01455_shard_leaf_max_rows_bytes_to_read -01476_right_full_join_switch -01477_lc_in_merge_join_left_key 01487_distributed_in_not_default_db 01495_subqueries_in_with_statement 01504_rocksdb From d72789369329661b15fc0b1f1ae1d24b8b2b545f Mon Sep 17 00:00:00 2001 From: vdimir Date: Mon, 15 May 2023 16:40:46 +0000 Subject: [PATCH 36/44] Handle function nodes in getColumnSideFromJoinTree --- src/Analyzer/Passes/QueryAnalysisPass.cpp | 28 ++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/Analyzer/Passes/QueryAnalysisPass.cpp b/src/Analyzer/Passes/QueryAnalysisPass.cpp index 8bacdfd7bd2..637f8a03a76 100644 --- a/src/Analyzer/Passes/QueryAnalysisPass.cpp +++ b/src/Analyzer/Passes/QueryAnalysisPass.cpp @@ -1205,8 +1205,34 @@ private: static std::string rewriteAggregateFunctionNameIfNeeded(const std::string & aggregate_function_name, const ContextPtr & context); - static std::optional getColumnSideFromJoinTree(QueryTreeNodePtr & resolved_identifier, const JoinNode & join_node) + static std::optional getColumnSideFromJoinTree(const QueryTreeNodePtr & resolved_identifier, const JoinNode & join_node) { + if (resolved_identifier->getNodeType() == QueryTreeNodeType::CONSTANT) + return {}; + + if (resolved_identifier->getNodeType() == QueryTreeNodeType::FUNCTION) + { + const auto & resolved_function = resolved_identifier->as(); + + const auto & argument_nodes = resolved_function.getArguments().getNodes(); + + std::optional result; + for (const auto & argument_node : argument_nodes) + { + auto table_side = getColumnSideFromJoinTree(argument_node, join_node); + if (table_side && result && *table_side != *result) + { + throw Exception(ErrorCodes::AMBIGUOUS_IDENTIFIER, + "Ambiguous identifier {}. In scope {}", + resolved_identifier->formatASTForErrorMessage(), + join_node.formatASTForErrorMessage()); + } + if (table_side) + result = *table_side; + } + return result; + } + const auto * column_src = resolved_identifier->as().getColumnSource().get(); if (join_node.getLeftTableExpression().get() == column_src) From 783b54624b7122a591e48f72d69a03f2c6e87978 Mon Sep 17 00:00:00 2001 From: vdimir Date: Tue, 30 May 2023 16:48:56 +0000 Subject: [PATCH 37/44] add comment to join_use_nulls in QueryAnalyzer::resolveMatcher --- src/Analyzer/Passes/QueryAnalysisPass.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Analyzer/Passes/QueryAnalysisPass.cpp b/src/Analyzer/Passes/QueryAnalysisPass.cpp index 637f8a03a76..b2bfa648435 100644 --- a/src/Analyzer/Passes/QueryAnalysisPass.cpp +++ b/src/Analyzer/Passes/QueryAnalysisPass.cpp @@ -4059,6 +4059,11 @@ ProjectionNames QueryAnalyzer::resolveMatcher(QueryTreeNodePtr & matcher_node, I if (scope.context->getSettingsRef().join_use_nulls) { + /** If we are resolving matcher came from the result of JOIN and `join_use_nulls` is set, + * we need to convert joined column type to Nullable. + * We are taking the nearest JoinNode to check to which table column belongs, + * because for LEFT/RIGHT join, we convert only the corresponding side. + */ const auto * nearest_query_scope = scope.getNearestQueryScope(); const QueryNode * nearest_scope_query_node = nearest_query_scope ? nearest_query_scope->scope_node->as() : nullptr; const QueryTreeNodePtr & nearest_scope_join_tree = nearest_scope_query_node ? nearest_scope_query_node->getJoinTree() : nullptr; From f15769ee6c9a7923a8e7651caf965c81e054ba0f Mon Sep 17 00:00:00 2001 From: kssenii Date: Tue, 30 May 2023 21:29:08 +0200 Subject: [PATCH 38/44] Fix --- src/Interpreters/Cache/FileCache.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index 1908a4ce895..65dca790183 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/src/Interpreters/Cache/FileCache.cpp @@ -592,7 +592,6 @@ bool FileCache::tryReserve(FileSegment & file_segment, const size_t size) std::unordered_map to_delete; size_t freeable_space = 0, freeable_count = 0; - size_t removed_size = 0; auto iterate_func = [&](LockedKey & locked_key, FileSegmentMetadataPtr segment_metadata) { chassert(segment_metadata->file_segment->assertCorrectness()); @@ -659,8 +658,8 @@ bool FileCache::tryReserve(FileSegment & file_segment, const size_t size) && freeable_count == 0 && main_priority->getElementsCount(cache_lock) == main_priority->getElementsLimit()); LOG_TEST( - log, "Overflow: {}, size: {}, ready to remove: {}, current cache size: {}/{}, elements: {}/{}, while reserving for {}:{}", - is_overflow, size, removed_size, + log, "Overflow: {}, size: {}, ready to remove: {} ({} in number), current cache size: {}/{}, elements: {}/{}, while reserving for {}:{}", + is_overflow, size, freeable_space, freeable_count, main_priority->getSize(cache_lock), main_priority->getSizeLimit(), main_priority->getElementsCount(cache_lock), main_priority->getElementsLimit(), file_segment.key(), file_segment.offset()); From 45e2fb4b3eefca48b27e90d23147660d3da46e39 Mon Sep 17 00:00:00 2001 From: Tyler Hannan Date: Wed, 31 May 2023 10:58:05 +0200 Subject: [PATCH 39/44] Update README.md changing release webinar date --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 9c933540e01..9561458ba37 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ curl https://clickhouse.com/ | sh ## Upcoming Events -* [**v23.5 Release Webinar**](https://clickhouse.com/company/events/v23-5-release-webinar?utm_source=github&utm_medium=social&utm_campaign=release-webinar-2023-05) - May 31 - 23.5 is rapidly approaching. Original creator, co-founder, and CTO of ClickHouse Alexey Milovidov will walk us through the highlights of the release. +* [**v23.5 Release Webinar**](https://clickhouse.com/company/events/v23-5-release-webinar?utm_source=github&utm_medium=social&utm_campaign=release-webinar-2023-05) - Jun 8 - 23.5 is rapidly approaching. Original creator, co-founder, and CTO of ClickHouse Alexey Milovidov will walk us through the highlights of the release. * [**ClickHouse Meetup in Bangalore**](https://www.meetup.com/clickhouse-bangalore-user-group/events/293740066/) - Jun 7 * [**ClickHouse Meetup in San Francisco**](https://www.meetup.com/clickhouse-silicon-valley-meetup-group/events/293426725/) - Jun 7 From 08d38878326fb9a2793e708008268a6eff000749 Mon Sep 17 00:00:00 2001 From: pufit Date: Wed, 31 May 2023 07:04:29 -0400 Subject: [PATCH 40/44] Add re-creation for cherry-pick PRs (#50373) * Add recreation for cherry-pick PRs. Small PR comment fix. * Automatic style fix --------- Co-authored-by: robot-clickhouse Co-authored-by: Nikita Mikhaylov --- tests/ci/cherry_pick.py | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/tests/ci/cherry_pick.py b/tests/ci/cherry_pick.py index 2fa562a1386..d36315151aa 100644 --- a/tests/ci/cherry_pick.py +++ b/tests/ci/cherry_pick.py @@ -70,9 +70,12 @@ This pull-request will be merged automatically as it reaches the mergeable state ### If the PR was closed and then reopened -If it stuck, check {pr_url} for `{label_backports_created}` and delete it if \ +If it stuck, check {pr_url} for `{backport_created_label}` and delete it if \ necessary. Manually merging will do nothing, since `{label_backports_created}` \ prevents the original PR {pr_url} from being processed. + +If you want to recreate the PR: delete the `{label_cherrypick}` label and delete this branch. +You may also need to delete the `{label_backports_created}` label from the original PR. """ BACKPORT_DESCRIPTION = """This pull-request is a last step of an automated \ backporting. @@ -82,7 +85,13 @@ close it. """ REMOTE = "" - def __init__(self, name: str, pr: PullRequest, repo: Repository): + def __init__( + self, + name: str, + pr: PullRequest, + repo: Repository, + backport_created_label: str = Labels.BACKPORTS_CREATED, + ): self.name = name self.pr = pr self.repo = repo @@ -93,6 +102,8 @@ close it. self.backport_pr = None # type: Optional[PullRequest] self._backported = False + self.backport_created_label = backport_created_label + self.git_prefix = ( # All commits to cherrypick are done as robot-clickhouse "git -c user.email=robot-clickhouse@users.noreply.github.com " "-c user.name=robot-clickhouse -c commit.gpgsign=false" @@ -226,7 +237,8 @@ close it. body=self.CHERRYPICK_DESCRIPTION.format( pr_number=self.pr.number, pr_url=self.pr.html_url, - label_backports_created=Labels.BACKPORTS_CREATED, + backport_created_label=self.backport_created_label, + label_cherrypick=Labels.CHERRYPICK, ), base=self.backport_branch, head=self.cherrypick_branch, @@ -459,11 +471,12 @@ class Backport: pr_labels = [label.name for label in pr.labels] if self.must_create_backport_label in pr_labels: branches = [ - ReleaseBranch(br, pr, self.repo) for br in self.release_branches + ReleaseBranch(br, pr, self.repo, self.backport_created_label) + for br in self.release_branches ] # type: List[ReleaseBranch] else: branches = [ - ReleaseBranch(br, pr, self.repo) + ReleaseBranch(br, pr, self.repo, self.backport_created_label) for br in [ label.split("-", 1)[0][1:] # v21.8-must-backport for label in pr_labels @@ -492,6 +505,7 @@ class Backport: ) bp_cp_prs = self.gh.get_pulls_from_search( query=f"type:pr repo:{self._repo_name} {query_suffix}", + label=f"{Labels.BACKPORT},{Labels.CHERRYPICK}", ) for br in branches: br.pop_prs(bp_cp_prs) From 6d45d0c37404f4d3a7bd03c92e6a2f04adef7beb Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 31 May 2023 13:11:10 +0200 Subject: [PATCH 41/44] Use fingerprints instead of key IDs in encrypted disks (#49882) * Use fingerprints instead of key IDs to find keys in encrypted disks. Always use little endian in the headers of encryption files. * Add tests. * Fix copying binary files to test containers. * Fix ownership for copied files in test containers. * Add comments after review. --------- Co-authored-by: Nikita Mikhaylov --- src/Backups/tests/gtest_backup_entries.cpp | 11 +- src/Disks/DiskEncrypted.cpp | 251 +++++++++++----- src/Disks/DiskEncryptedTransaction.cpp | 43 +-- src/Disks/DiskEncryptedTransaction.h | 8 +- src/Disks/tests/gtest_disk_encrypted.cpp | 8 +- src/IO/FileEncryptionCommon.cpp | 108 +++++-- src/IO/FileEncryptionCommon.h | 42 ++- src/IO/tests/gtest_file_encryption.cpp | 3 +- tests/integration/helpers/cluster.py | 5 +- .../old_versions/version_1be/__marks.mrk | Bin 0 -> 96 bytes .../old_versions/version_1be/data.bin | Bin 0 -> 99 bytes .../old_versions/version_1be/id.bin | Bin 0 -> 102 bytes .../old_versions/version_1be/sizes.json | Bin 0 -> 162 bytes .../old_versions/version_1le/__marks.mrk | Bin 0 -> 96 bytes .../old_versions/version_1le/data.bin | Bin 0 -> 99 bytes .../old_versions/version_1le/id.bin | Bin 0 -> 102 bytes .../old_versions/version_1le/sizes.json | Bin 0 -> 162 bytes .../old_versions/version_2/__marks.mrk | Bin 0 -> 96 bytes .../old_versions/version_2/data.bin | Bin 0 -> 99 bytes .../old_versions/version_2/id.bin | Bin 0 -> 102 bytes .../old_versions/version_2/sizes.json | Bin 0 -> 162 bytes tests/integration/test_encrypted_disk/test.py | 284 ++++++++++++++---- 22 files changed, 543 insertions(+), 220 deletions(-) create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_1be/__marks.mrk create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_1be/data.bin create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_1be/id.bin create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_1be/sizes.json create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_1le/__marks.mrk create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_1le/data.bin create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_1le/id.bin create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_1le/sizes.json create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_2/__marks.mrk create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_2/data.bin create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_2/id.bin create mode 100644 tests/integration/test_encrypted_disk/old_versions/version_2/sizes.json diff --git a/src/Backups/tests/gtest_backup_entries.cpp b/src/Backups/tests/gtest_backup_entries.cpp index 3077bedad0e..ca603d20787 100644 --- a/src/Backups/tests/gtest_backup_entries.cpp +++ b/src/Backups/tests/gtest_backup_entries.cpp @@ -29,10 +29,15 @@ protected: /// Make encrypted disk. auto settings = std::make_unique(); settings->wrapped_disk = local_disk; - settings->current_algorithm = FileEncryption::Algorithm::AES_128_CTR; - settings->keys[0] = "1234567890123456"; - settings->current_key_id = 0; settings->disk_path = "encrypted/"; + + settings->current_algorithm = FileEncryption::Algorithm::AES_128_CTR; + String key = "1234567890123456"; + UInt128 fingerprint = FileEncryption::calculateKeyFingerprint(key); + settings->all_keys[fingerprint] = key; + settings->current_key = key; + settings->current_key_fingerprint = fingerprint; + encrypted_disk = std::make_shared("encrypted_disk", std::move(settings), true); } diff --git a/src/Disks/DiskEncrypted.cpp b/src/Disks/DiskEncrypted.cpp index 2415b432e01..6b515b100c9 100644 --- a/src/Disks/DiskEncrypted.cpp +++ b/src/Disks/DiskEncrypted.cpp @@ -19,7 +19,6 @@ namespace ErrorCodes { extern const int BAD_ARGUMENTS; extern const int INCORRECT_DISK_INDEX; - extern const int DATA_ENCRYPTION_ERROR; extern const int NOT_IMPLEMENTED; } @@ -42,87 +41,201 @@ namespace } } + /// Reads encryption keys from the configuration. + void getKeysFromConfig(const Poco::Util::AbstractConfiguration & config, const String & config_prefix, + std::map & out_keys_by_id, Strings & out_keys_without_id) + { + Strings config_keys; + config.keys(config_prefix, config_keys); + + for (const std::string & config_key : config_keys) + { + String key; + std::optional key_id; + + if ((config_key == "key") || config_key.starts_with("key[")) + { + String key_path = config_prefix + "." + config_key; + key = config.getString(key_path); + String key_id_path = key_path + "[@id]"; + if (config.has(key_id_path)) + key_id = config.getUInt64(key_id_path); + } + else if ((config_key == "key_hex") || config_key.starts_with("key_hex[")) + { + String key_path = config_prefix + "." + config_key; + key = unhexKey(config.getString(key_path)); + String key_id_path = key_path + "[@id]"; + if (config.has(key_id_path)) + key_id = config.getUInt64(key_id_path); + } + else + continue; + + if (key_id) + { + if (!out_keys_by_id.contains(*key_id)) + out_keys_by_id[*key_id] = key; + else + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Multiple keys specified for same ID {}", *key_id); + } + else + out_keys_without_id.push_back(key); + } + + if (out_keys_by_id.empty() && out_keys_without_id.empty()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "No encryption keys found"); + + if (out_keys_by_id.empty() && (out_keys_without_id.size() == 1)) + { + out_keys_by_id[0] = out_keys_without_id.front(); + out_keys_without_id.clear(); + } + } + + /// Reads the current encryption key from the configuration. + String getCurrentKeyFromConfig(const Poco::Util::AbstractConfiguration & config, const String & config_prefix, + const std::map & keys_by_id, const Strings & keys_without_id) + { + String key_path = config_prefix + ".current_key"; + String key_hex_path = config_prefix + ".current_key_hex"; + String key_id_path = config_prefix + ".current_key_id"; + + if (config.has(key_path) + config.has(key_hex_path) + config.has(key_id_path) > 1) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "The current key is specified multiple times"); + + auto check_current_key_found = [&](const String & current_key_) + { + for (const auto & [_, key] : keys_by_id) + { + if (key == current_key_) + return; + } + for (const auto & key : keys_without_id) + { + if (key == current_key_) + return; + } + throw Exception(ErrorCodes::BAD_ARGUMENTS, "The current key is not found in keys"); + }; + + if (config.has(key_path)) + { + String current_key = config.getString(key_path); + check_current_key_found(current_key); + return current_key; + } + else if (config.has(key_hex_path)) + { + String current_key = unhexKey(config.getString(key_hex_path)); + check_current_key_found(current_key); + return current_key; + } + else if (config.has(key_id_path)) + { + UInt64 current_key_id = config.getUInt64(key_id_path); + auto it = keys_by_id.find(current_key_id); + if (it == keys_by_id.end()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Not found a key with the current ID {}", current_key_id); + return it->second; + } + else if (keys_by_id.size() == 1 && keys_without_id.empty() && keys_by_id.begin()->first == 0) + { + /// There is only a single key defined with id=0, so we can choose it as current. + return keys_by_id.begin()->second; + } + else + { + throw Exception(ErrorCodes::BAD_ARGUMENTS, "The current key is not specified"); + } + } + + /// Reads the current encryption algorithm from the configuration. + Algorithm getCurrentAlgorithmFromConfig(const Poco::Util::AbstractConfiguration & config, const String & config_prefix) + { + String path = config_prefix + ".algorithm"; + if (!config.has(path)) + return DEFAULT_ENCRYPTION_ALGORITHM; + return parseAlgorithmFromString(config.getString(path)); + } + + /// Reads the name of a wrapped disk & the path on the wrapped disk and then finds that disk in a disk map. + void getDiskAndPathFromConfig(const Poco::Util::AbstractConfiguration & config, const String & config_prefix, const DisksMap & map, + DiskPtr & out_disk, String & out_path) + { + String disk_name = config.getString(config_prefix + ".disk", ""); + if (disk_name.empty()) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, "Name of the wrapped disk must not be empty. Encrypted disk is a wrapper over another disk"); + + auto disk_it = map.find(disk_name); + if (disk_it == map.end()) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, "The wrapped disk must have been announced earlier. No disk with name {}", disk_name); + + out_disk = disk_it->second; + + out_path = config.getString(config_prefix + ".path", ""); + if (!out_path.empty() && (out_path.back() != '/')) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Disk path must ends with '/', but '{}' doesn't.", quoteString(out_path)); + } + + /// Parses the settings of an ecnrypted disk from the configuration. std::unique_ptr parseDiskEncryptedSettings( - const String & name, const Poco::Util::AbstractConfiguration & config, const String & config_prefix, const DisksMap & map) + const String & disk_name, + const Poco::Util::AbstractConfiguration & config, + const String & config_prefix, + const DisksMap & disk_map) { try { auto res = std::make_unique(); - res->current_algorithm = DEFAULT_ENCRYPTION_ALGORITHM; - if (config.has(config_prefix + ".algorithm")) - parseFromString(res->current_algorithm, config.getString(config_prefix + ".algorithm")); - Strings config_keys; - config.keys(config_prefix, config_keys); - for (const std::string & config_key : config_keys) + std::map keys_by_id; + Strings keys_without_id; + getKeysFromConfig(config, config_prefix, keys_by_id, keys_without_id); + + for (const auto & [key_id, key] : keys_by_id) { - String key; - UInt64 key_id; + auto fingerprint = calculateKeyFingerprint(key); + res->all_keys[fingerprint] = key; - if ((config_key == "key") || config_key.starts_with("key[")) - { - key = config.getString(config_prefix + "." + config_key, ""); - key_id = config.getUInt64(config_prefix + "." + config_key + "[@id]", 0); - } - else if ((config_key == "key_hex") || config_key.starts_with("key_hex[")) - { - key = unhexKey(config.getString(config_prefix + "." + config_key, "")); - key_id = config.getUInt64(config_prefix + "." + config_key + "[@id]", 0); - } - else - continue; - - if (res->keys.contains(key_id)) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Multiple keys have the same ID {}", key_id); - res->keys[key_id] = key; + /// Version 1 used key fingerprints based on the key id. + /// We have to add such fingerprints to the map too to support reading files encrypted by version 1. + auto v1_fingerprint = calculateV1KeyFingerprint(key, key_id); + res->all_keys[v1_fingerprint] = key; } - if (res->keys.empty()) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "No keys, an encrypted disk needs keys to work"); - - if (!config.has(config_prefix + ".current_key_id")) + for (const auto & key : keys_without_id) { - /// In case of multiple keys, current_key_id is mandatory - if (res->keys.size() > 1) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "There are multiple keys in config. current_key_id is required"); - - /// If there is only one key with non zero ID, curren_key_id should be defined. - if (res->keys.size() == 1 && !res->keys.contains(0)) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Config has one key with non zero id. сurrent_key_id is required"); + auto fingerprint = calculateKeyFingerprint(key); + res->all_keys[fingerprint] = key; } - res->current_key_id = config.getUInt64(config_prefix + ".current_key_id", 0); - if (!res->keys.contains(res->current_key_id)) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Not found a key with the current ID {}", res->current_key_id); - FileEncryption::checkKeySize(res->current_algorithm, res->keys[res->current_key_id].size()); + String current_key = getCurrentKeyFromConfig(config, config_prefix, keys_by_id, keys_without_id); + res->current_key = current_key; + res->current_key_fingerprint = calculateKeyFingerprint(current_key); - String wrapped_disk_name = config.getString(config_prefix + ".disk", ""); - if (wrapped_disk_name.empty()) - throw Exception( - ErrorCodes::BAD_ARGUMENTS, - "Name of the wrapped disk must not be empty. Encrypted disk is a wrapper over another disk"); + res->current_algorithm = getCurrentAlgorithmFromConfig(config, config_prefix); - auto wrapped_disk_it = map.find(wrapped_disk_name); - if (wrapped_disk_it == map.end()) - throw Exception( - ErrorCodes::BAD_ARGUMENTS, - "The wrapped disk must have been announced earlier. No disk with name {}", - wrapped_disk_name); - res->wrapped_disk = wrapped_disk_it->second; + FileEncryption::checkKeySize(res->current_key.size(), res->current_algorithm); - res->disk_path = config.getString(config_prefix + ".path", ""); - if (!res->disk_path.empty() && (res->disk_path.back() != '/')) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Disk path must ends with '/', but '{}' doesn't.", quoteString(res->disk_path)); + DiskPtr wrapped_disk; + String disk_path; + getDiskAndPathFromConfig(config, config_prefix, disk_map, wrapped_disk, disk_path); + res->wrapped_disk = wrapped_disk; + res->disk_path = disk_path; return res; } catch (Exception & e) { - e.addMessage("Disk " + name); + e.addMessage("Disk " + disk_name); throw; } } + /// Reads the header of an encrypted file. FileEncryption::Header readHeader(ReadBufferFromFileBase & read_buffer) { try @@ -138,24 +251,6 @@ namespace } } - String getKey(const String & path, const FileEncryption::Header & header, const DiskEncryptedSettings & settings) - { - auto it = settings.keys.find(header.key_id); - if (it == settings.keys.end()) - throw Exception( - ErrorCodes::DATA_ENCRYPTION_ERROR, - "Not found a key with ID {} required to decipher file {}", - header.key_id, - quoteString(path)); - - String key = it->second; - if (calculateKeyHash(key) != header.key_hash) - throw Exception( - ErrorCodes::DATA_ENCRYPTION_ERROR, "Wrong key with ID {}, could not decipher file {}", header.key_id, quoteString(path)); - - return key; - } - bool inline isSameDiskType(const IDisk & one, const IDisk & another) { return typeid(one) == typeid(another); @@ -225,7 +320,7 @@ void DiskEncrypted::copy(const String & from_path, const std::shared_ptr { auto from_settings = current_settings.get(); auto to_settings = to_disk_enc->current_settings.get(); - if (from_settings->keys == to_settings->keys) + if (from_settings->all_keys == to_settings->all_keys) { /// Keys are the same so we can simply copy the encrypted file. auto wrapped_from_path = wrappedPath(from_path); @@ -252,7 +347,7 @@ void DiskEncrypted::copyDirectoryContent(const String & from_dir, const std::sha { auto from_settings = current_settings.get(); auto to_settings = to_disk_enc->current_settings.get(); - if (from_settings->keys == to_settings->keys) + if (from_settings->all_keys == to_settings->all_keys) { /// Keys are the same so we can simply copy the encrypted file. auto wrapped_from_path = wrappedPath(from_dir); @@ -293,7 +388,7 @@ std::unique_ptr DiskEncrypted::readFile( } auto encryption_settings = current_settings.get(); FileEncryption::Header header = readHeader(*buffer); - String key = getKey(path, header, *encryption_settings); + String key = encryption_settings->findKeyByFingerprint(header.key_fingerprint, path); return std::make_unique(settings.local_fs_buffer_size, std::move(buffer), key, header); } diff --git a/src/Disks/DiskEncryptedTransaction.cpp b/src/Disks/DiskEncryptedTransaction.cpp index 4a613374ccf..40df94b309a 100644 --- a/src/Disks/DiskEncryptedTransaction.cpp +++ b/src/Disks/DiskEncryptedTransaction.cpp @@ -38,39 +38,21 @@ FileEncryption::Header readHeader(ReadBufferFromFileBase & read_buffer) } } -String getCurrentKey(const String & path, const DiskEncryptedSettings & settings) +} + +String DiskEncryptedSettings::findKeyByFingerprint(UInt128 key_fingerprint, const String & path_for_logs) const { - auto it = settings.keys.find(settings.current_key_id); - if (it == settings.keys.end()) + auto it = all_keys.find(key_fingerprint); + if (it == all_keys.end()) + { throw Exception( ErrorCodes::DATA_ENCRYPTION_ERROR, - "Not found a key with the current ID {} required to cipher file {}", - settings.current_key_id, - quoteString(path)); - + "Not found an encryption key required to decipher file {}", + quoteString(path_for_logs)); + } return it->second; } -String getKey(const String & path, const FileEncryption::Header & header, const DiskEncryptedSettings & settings) -{ - auto it = settings.keys.find(header.key_id); - if (it == settings.keys.end()) - throw Exception( - ErrorCodes::DATA_ENCRYPTION_ERROR, - "Not found a key with ID {} required to decipher file {}", - header.key_id, - quoteString(path)); - - String key = it->second; - if (FileEncryption::calculateKeyHash(key) != header.key_hash) - throw Exception( - ErrorCodes::DATA_ENCRYPTION_ERROR, "Wrong key with ID {}, could not decipher file {}", header.key_id, quoteString(path)); - - return key; -} - -} - void DiskEncryptedTransaction::copyFile(const std::string & from_file_path, const std::string & to_file_path) { auto wrapped_from_path = wrappedPath(from_file_path); @@ -98,16 +80,15 @@ std::unique_ptr DiskEncryptedTransaction::writeFile( // /// Append mode: we continue to use the same header. auto read_buffer = delegate_disk->readFile(wrapped_path, ReadSettings().adjustBufferSize(FileEncryption::Header::kSize)); header = readHeader(*read_buffer); - key = getKey(path, header, current_settings); + key = current_settings.findKeyByFingerprint(header.key_fingerprint, path); } } if (!old_file_size) { /// Rewrite mode: we generate a new header. - key = getCurrentKey(path, current_settings); header.algorithm = current_settings.current_algorithm; - header.key_id = current_settings.current_key_id; - header.key_hash = FileEncryption::calculateKeyHash(key); + key = current_settings.current_key; + header.key_fingerprint = current_settings.current_key_fingerprint; header.init_vector = FileEncryption::InitVector::random(); } auto buffer = delegate_transaction->writeFile(wrapped_path, buf_size, mode, settings, autocommit); diff --git a/src/Disks/DiskEncryptedTransaction.h b/src/Disks/DiskEncryptedTransaction.h index bae3f2c728c..04cc63f1671 100644 --- a/src/Disks/DiskEncryptedTransaction.h +++ b/src/Disks/DiskEncryptedTransaction.h @@ -18,9 +18,13 @@ struct DiskEncryptedSettings { DiskPtr wrapped_disk; String disk_path; - std::unordered_map keys; - UInt64 current_key_id; + String current_key; + UInt128 current_key_fingerprint; FileEncryption::Algorithm current_algorithm; + std::unordered_map all_keys; + + /// Returns an encryption key found by its fingerprint. + String findKeyByFingerprint(UInt128 key_fingerprint, const String & path_for_logs) const; }; diff --git a/src/Disks/tests/gtest_disk_encrypted.cpp b/src/Disks/tests/gtest_disk_encrypted.cpp index 80a10e8680b..ee9e284d409 100644 --- a/src/Disks/tests/gtest_disk_encrypted.cpp +++ b/src/Disks/tests/gtest_disk_encrypted.cpp @@ -37,8 +37,10 @@ protected: auto settings = std::make_unique(); settings->wrapped_disk = local_disk; settings->current_algorithm = algorithm; - settings->keys[0] = key; - settings->current_key_id = 0; + auto fingerprint = FileEncryption::calculateKeyFingerprint(key); + settings->all_keys[fingerprint] = key; + settings->current_key = key; + settings->current_key_fingerprint = fingerprint; settings->disk_path = path; encrypted_disk = std::make_shared("encrypted_disk", std::move(settings), true); } @@ -255,7 +257,7 @@ TEST_F(DiskEncryptedTest, RandomIV) String bina = getBinaryRepresentation(getDirectory() + "a.txt"); String binb = getBinaryRepresentation(getDirectory() + "b.txt"); - constexpr size_t iv_offset = 16; + constexpr size_t iv_offset = 23; /// See the description of the format in the comment for FileEncryption::Header. constexpr size_t iv_size = FileEncryption::InitVector::kSize; EXPECT_EQ(bina.substr(0, iv_offset), binb.substr(0, iv_offset)); /// Part of the header before IV is the same. EXPECT_NE(bina.substr(iv_offset, iv_size), binb.substr(iv_offset, iv_size)); /// IV differs. diff --git a/src/IO/FileEncryptionCommon.cpp b/src/IO/FileEncryptionCommon.cpp index 4ac4d289b32..5529c813c40 100644 --- a/src/IO/FileEncryptionCommon.cpp +++ b/src/IO/FileEncryptionCommon.cpp @@ -34,6 +34,7 @@ namespace case Algorithm::AES_128_CTR: return EVP_aes_128_ctr(); case Algorithm::AES_192_CTR: return EVP_aes_192_ctr(); case Algorithm::AES_256_CTR: return EVP_aes_256_ctr(); + case Algorithm::MAX: break; } throw Exception( ErrorCodes::BAD_ARGUMENTS, @@ -187,10 +188,14 @@ namespace return plaintext_size; } - constexpr const char kHeaderSignature[] = "ENC"; - constexpr const UInt16 kHeaderCurrentVersion = 1; -} + constexpr const std::string_view kHeaderSignature = "ENC"; + UInt128 calculateV1KeyFingerprint(UInt8 small_key_hash, UInt64 key_id) + { + /// In the version 1 we stored {key_id, very_small_hash(key)} instead of a fingerprint. + return static_cast(key_id) | (static_cast(small_key_hash) << 64); + } +} String toString(Algorithm algorithm) { @@ -199,6 +204,7 @@ String toString(Algorithm algorithm) case Algorithm::AES_128_CTR: return "aes_128_ctr"; case Algorithm::AES_192_CTR: return "aes_192_ctr"; case Algorithm::AES_256_CTR: return "aes_256_ctr"; + case Algorithm::MAX: break; } throw Exception( ErrorCodes::BAD_ARGUMENTS, @@ -206,14 +212,14 @@ String toString(Algorithm algorithm) static_cast(algorithm)); } -void parseFromString(Algorithm & algorithm, const String & str) +Algorithm parseAlgorithmFromString(const String & str) { if (boost::iequals(str, "aes_128_ctr")) - algorithm = Algorithm::AES_128_CTR; + return Algorithm::AES_128_CTR; else if (boost::iequals(str, "aes_192_ctr")) - algorithm = Algorithm::AES_192_CTR; + return Algorithm::AES_192_CTR; else if (boost::iequals(str, "aes_256_ctr")) - algorithm = Algorithm::AES_256_CTR; + return Algorithm::AES_256_CTR; else throw Exception( ErrorCodes::BAD_ARGUMENTS, @@ -221,7 +227,7 @@ void parseFromString(Algorithm & algorithm, const String & str) str); } -void checkKeySize(Algorithm algorithm, size_t key_size) { checkKeySize(getCipher(algorithm), key_size); } +void checkKeySize(size_t key_size, Algorithm algorithm) { checkKeySize(getCipher(algorithm), key_size); } String InitVector::toString() const @@ -364,54 +370,92 @@ void Encryptor::decrypt(const char * data, size_t size, char * out) void Header::read(ReadBuffer & in) { - constexpr size_t header_signature_size = std::size(kHeaderSignature) - 1; - char signature[std::size(kHeaderSignature)] = {}; - in.readStrict(signature, header_signature_size); - if (strcmp(signature, kHeaderSignature) != 0) + char signature[kHeaderSignature.length()]; + in.readStrict(signature, kHeaderSignature.length()); + if (memcmp(signature, kHeaderSignature.data(), kHeaderSignature.length()) != 0) throw Exception(ErrorCodes::DATA_ENCRYPTION_ERROR, "Wrong signature, this is not an encrypted file"); - UInt16 version; - readPODBinary(version, in); - if (version != kHeaderCurrentVersion) + /// The endianness of how the header is written. + /// Starting from version 2 the header is always in little endian. + std::endian endian = std::endian::little; + + readBinaryLittleEndian(version, in); + + if (version == 0x0100ULL) + { + /// Version 1 could write the header of an encrypted file in either little-endian or big-endian. + /// So now if we read the version as little-endian and it's 256 that means two things: the version is actually 1 and the whole header is in big endian. + endian = std::endian::big; + version = 1; + } + + if (version < 1 || version > kCurrentVersion) throw Exception(ErrorCodes::DATA_ENCRYPTION_ERROR, "Version {} of the header is not supported", version); UInt16 algorithm_u16; readPODBinary(algorithm_u16, in); + if (std::endian::native != endian) + algorithm_u16 = std::byteswap(algorithm_u16); + if (algorithm_u16 >= static_cast(Algorithm::MAX)) + throw Exception(ErrorCodes::DATA_ENCRYPTION_ERROR, "Algorithm {} is not supported", algorithm_u16); algorithm = static_cast(algorithm_u16); - readPODBinary(key_id, in); - readPODBinary(key_hash, in); + size_t bytes_to_skip = kSize - kHeaderSignature.length() - sizeof(version) - sizeof(algorithm_u16) - InitVector::kSize; + + if (version < 2) + { + UInt64 key_id; + UInt8 small_key_hash; + readPODBinary(key_id, in); + readPODBinary(small_key_hash, in); + bytes_to_skip -= sizeof(key_id) + sizeof(small_key_hash); + if (std::endian::native != endian) + key_id = std::byteswap(key_id); + key_fingerprint = calculateV1KeyFingerprint(small_key_hash, key_id); + } + else + { + readBinaryLittleEndian(key_fingerprint, in); + bytes_to_skip -= sizeof(key_fingerprint); + } + init_vector.read(in); - constexpr size_t reserved_size = kSize - header_signature_size - sizeof(version) - sizeof(algorithm_u16) - sizeof(key_id) - sizeof(key_hash) - InitVector::kSize; - static_assert(reserved_size < kSize); - in.ignore(reserved_size); + chassert(bytes_to_skip < kSize); + in.ignore(bytes_to_skip); } void Header::write(WriteBuffer & out) const { - constexpr size_t header_signature_size = std::size(kHeaderSignature) - 1; - out.write(kHeaderSignature, header_signature_size); + writeString(kHeaderSignature, out); - UInt16 version = kHeaderCurrentVersion; - writePODBinary(version, out); + writeBinaryLittleEndian(version, out); UInt16 algorithm_u16 = static_cast(algorithm); - writePODBinary(algorithm_u16, out); + writeBinaryLittleEndian(algorithm_u16, out); + + writeBinaryLittleEndian(key_fingerprint, out); - writePODBinary(key_id, out); - writePODBinary(key_hash, out); init_vector.write(out); - constexpr size_t reserved_size = kSize - header_signature_size - sizeof(version) - sizeof(algorithm_u16) - sizeof(key_id) - sizeof(key_hash) - InitVector::kSize; + constexpr size_t reserved_size = kSize - kHeaderSignature.length() - sizeof(version) - sizeof(algorithm_u16) - sizeof(key_fingerprint) - InitVector::kSize; static_assert(reserved_size < kSize); - char reserved_zero_bytes[reserved_size] = {}; - out.write(reserved_zero_bytes, reserved_size); + char zero_bytes[reserved_size] = {}; + out.write(zero_bytes, reserved_size); } -UInt8 calculateKeyHash(const String & key) +UInt128 calculateKeyFingerprint(const String & key) { - return static_cast(sipHash64(key.data(), key.size())) & 0x0F; + const UInt64 seed0 = 0x4368456E63727970ULL; // ChEncryp + const UInt64 seed1 = 0x7465644469736B46ULL; // tedDiskF + return sipHash128Keyed(seed0, seed1, key.data(), key.size()); +} + +UInt128 calculateV1KeyFingerprint(const String & key, UInt64 key_id) +{ + /// In the version 1 we stored {key_id, very_small_hash(key)} instead of a fingerprint. + UInt8 small_key_hash = sipHash64(key.data(), key.size()) & 0x0F; + return calculateV1KeyFingerprint(small_key_hash, key_id); } } diff --git a/src/IO/FileEncryptionCommon.h b/src/IO/FileEncryptionCommon.h index efc0194da52..87aa1194273 100644 --- a/src/IO/FileEncryptionCommon.h +++ b/src/IO/FileEncryptionCommon.h @@ -23,13 +23,14 @@ enum class Algorithm AES_128_CTR, /// Size of key is 16 bytes. AES_192_CTR, /// Size of key is 24 bytes. AES_256_CTR, /// Size of key is 32 bytes. + MAX }; String toString(Algorithm algorithm); -void parseFromString(Algorithm & algorithm, const String & str); +Algorithm parseAlgorithmFromString(const String & str); /// Throws an exception if a specified key size doesn't correspond a specified encryption algorithm. -void checkKeySize(Algorithm algorithm, size_t key_size); +void checkKeySize(size_t key_size, Algorithm algorithm); /// Initialization vector. Its size is always 16 bytes. @@ -103,15 +104,34 @@ private: /// File header which is stored at the beginning of encrypted files. +/// +/// The format of that header is following: +/// +--------+------+--------------------------------------------------------------------------+ +/// | offset | size | description | +/// +--------+------+--------------------------------------------------------------------------+ +/// | 0 | 3 | 'E', 'N', 'C' (file's signature) | +/// | 3 | 2 | version of this header (1..2) | +/// | 5 | 2 | encryption algorithm (0..2, 0=AES_128_CTR, 1=AES_192_CTR, 2=AES_256_CTR) | +/// | 7 | 16 | fingerprint of encryption key (SipHash) | +/// | 23 | 16 | initialization vector (randomly generated) | +/// | 39 | 25 | reserved for future use | +/// +--------+------+--------------------------------------------------------------------------+ +/// struct Header { + /// Versions: + /// 1 - Initial version + /// 2 - The header of an encrypted file contains the fingerprint of a used encryption key instead of a pair {key_id, very_small_hash(key)}. + /// The header is always stored in little endian. + static constexpr const UInt16 kCurrentVersion = 2; + + UInt16 version = kCurrentVersion; + + /// Encryption algorithm. Algorithm algorithm = Algorithm::AES_128_CTR; - /// Identifier of the key to encrypt or decrypt this file. - UInt64 key_id = 0; - - /// Hash of the key to encrypt or decrypt this file. - UInt8 key_hash = 0; + /// Fingerprint of a key. + UInt128 key_fingerprint = 0; InitVector init_vector; @@ -122,9 +142,11 @@ struct Header void write(WriteBuffer & out) const; }; -/// Calculates the hash of a passed key. -/// 1 byte is enough because this hash is used only for the first check. -UInt8 calculateKeyHash(const String & key); +/// Calculates the fingerprint of a passed encryption key. +UInt128 calculateKeyFingerprint(const String & key); + +/// Calculates kind of the fingerprint of a passed encryption key & key ID as it was implemented in version 1. +UInt128 calculateV1KeyFingerprint(const String & key, UInt64 key_id); } } diff --git a/src/IO/tests/gtest_file_encryption.cpp b/src/IO/tests/gtest_file_encryption.cpp index 6a090ff0810..2b3d7ce81c5 100644 --- a/src/IO/tests/gtest_file_encryption.cpp +++ b/src/IO/tests/gtest_file_encryption.cpp @@ -226,8 +226,7 @@ TEST(FileEncryptionPositionUpdateTest, Decryption) String key = "1234567812345678"; FileEncryption::Header header; header.algorithm = Algorithm::AES_128_CTR; - header.key_id = 1; - header.key_hash = calculateKeyHash(key); + header.key_fingerprint = calculateKeyFingerprint(key); header.init_vector = InitVector::random(); auto lwb = std::make_unique(tmp_path); diff --git a/tests/integration/helpers/cluster.py b/tests/integration/helpers/cluster.py index 950663cb429..f57ebf40e54 100644 --- a/tests/integration/helpers/cluster.py +++ b/tests/integration/helpers/cluster.py @@ -1963,9 +1963,9 @@ class ClickHouseCluster: return output def copy_file_to_container(self, container_id, local_path, dest_path): - with open(local_path, "r") as fdata: + with open(local_path, "rb") as fdata: data = fdata.read() - encodedBytes = base64.b64encode(data.encode("utf-8")) + encodedBytes = base64.b64encode(data) encodedStr = str(encodedBytes, "utf-8") self.exec_in_container( container_id, @@ -1974,7 +1974,6 @@ class ClickHouseCluster: "-c", "echo {} | base64 --decode > {}".format(encodedStr, dest_path), ], - user="root", ) def wait_for_url( diff --git a/tests/integration/test_encrypted_disk/old_versions/version_1be/__marks.mrk b/tests/integration/test_encrypted_disk/old_versions/version_1be/__marks.mrk new file mode 100644 index 0000000000000000000000000000000000000000..88ad4b6cf3a1b43e3a27eff0696429c94bee8597 GIT binary patch literal 96 zcmZ?ub7o*F)A02 literal 0 HcmV?d00001 diff --git a/tests/integration/test_encrypted_disk/old_versions/version_1be/data.bin b/tests/integration/test_encrypted_disk/old_versions/version_1be/data.bin new file mode 100644 index 0000000000000000000000000000000000000000..f82db60e077eec4aec1ff5e7885e71e4fb67587c GIT binary patch literal 99 zcmZ?ub7o*n;quo=<- literal 0 HcmV?d00001 diff --git a/tests/integration/test_encrypted_disk/old_versions/version_1be/id.bin b/tests/integration/test_encrypted_disk/old_versions/version_1be/id.bin new file mode 100644 index 0000000000000000000000000000000000000000..0c2426599a7cdafed1b0835df4962a6b4be8b0d2 GIT binary patch literal 102 zcmZ?ub7o*1(U%=%e;^La?0M$MLunY#jeu5XR2z7 YJrWis`8aqU?pvBM-}Zcv!kO9M0LYjcNB{r; literal 0 HcmV?d00001 diff --git a/tests/integration/test_encrypted_disk/old_versions/version_1be/sizes.json b/tests/integration/test_encrypted_disk/old_versions/version_1be/sizes.json new file mode 100644 index 0000000000000000000000000000000000000000..6d610f2da837e016e0dd58162e3fa47b3fdd8723 GIT binary patch literal 162 zcmZ?ub7o*(<1jvv&|4{XdzVvOGW)@xdOrrw;qjl6pl*Dbh_wq@spu#6KD7ua3}on$qCxAl-@ l_Vn!wPt8i^(JEOzCwNxJMXuhz7YoltPj!>n7vP&`4FHC@I$i(( literal 0 HcmV?d00001 diff --git a/tests/integration/test_encrypted_disk/old_versions/version_1le/__marks.mrk b/tests/integration/test_encrypted_disk/old_versions/version_1le/__marks.mrk new file mode 100644 index 0000000000000000000000000000000000000000..919c95234e5c54930c013a7f7f602b8a82d40841 GIT binary patch literal 96 zcmZ?ub7o{TRIz3A{p@GOcv~$@NmHm+R`0m20NkJvON8<9y_ m|1Z5Bz;phefdB6v(eAx#0#kB11CA=HobP^ZT^9CnZZ80`Q8xDg literal 0 HcmV?d00001 diff --git a/tests/integration/test_encrypted_disk/old_versions/version_2/__marks.mrk b/tests/integration/test_encrypted_disk/old_versions/version_2/__marks.mrk new file mode 100644 index 0000000000000000000000000000000000000000..e85676068f7e6ebe4952560368177d9e87987834 GIT binary patch literal 96 zcmZ?ub7o>-V3^Y8qto>FLP|yPKkMk--(;PlWBJu1r|n273|JHCH~j@r33jj{EcuQo h=ldzYWq4Js8YDihd0xh_KPBk}-{vE;FTS5M0RXtmAgurZ literal 0 HcmV?d00001 diff --git a/tests/integration/test_encrypted_disk/old_versions/version_2/data.bin b/tests/integration/test_encrypted_disk/old_versions/version_2/data.bin new file mode 100644 index 0000000000000000000000000000000000000000..b0b5e06a28009caccea335a822ccae153c432902 GIT binary patch literal 99 zcmZ?ub7o>-V3^Y8qto>FLP|yPKkMk--(>H;TzD?zhE~IueU93t?-V3^Y8qto>FLP|yPKkMk--(=k*J-Rk77E*mA^(d`dd*)G~66}D#>VZt6 n_SLQDubuKL4;Aq~InCh8nW`(@UduI>7Ajm%Ymk(EGx;-V3^Y8qto>FLP|yPKkMk--(>r(e!mW4uVz^qyi_D;+M2IGCD?)9(#c#4 zVrtY(^m|UdpT6?Ks#O7(CO>%Mf77C0pwZU-c!b-Gd8MhGw{`a1KJ$FGW}(;0*D3aY y8zy{XoSm7pbNSjGDs44hF*$6tMpc3ucAK_0__1@XivP~0@bL|UZ^+TM<|F{?^gwX{ literal 0 HcmV?d00001 diff --git a/tests/integration/test_encrypted_disk/test.py b/tests/integration/test_encrypted_disk/test.py index 66ff073f02b..9f5415f4bea 100644 --- a/tests/integration/test_encrypted_disk/test.py +++ b/tests/integration/test_encrypted_disk/test.py @@ -1,9 +1,11 @@ import pytest +import os.path from helpers.cluster import ClickHouseCluster from helpers.client import QueryRuntimeException import os.path from helpers.test_tools import assert_eq_with_retry +SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) FIRST_PART_NAME = "all_1_1_0" @@ -170,53 +172,62 @@ def test_optimize_table(policy, encrypted_disk): assert node.query(select_query) == "(0,'data'),(1,'data'),(2,'data'),(3,'data')" -# Test adding encryption key on the fly. -def test_add_key(): - def make_storage_policy_with_keys(policy_name, keys): - node.exec_in_container( - [ - "bash", - "-c", - """cat > /etc/clickhouse-server/config.d/storage_policy_{policy_name}.xml << EOF +def make_storage_policy_with_keys( + policy_name, keys, check_system_storage_policies=False +): + if check_system_storage_policies: + node.query("SELECT policy_name FROM system.storage_policies") + + node.exec_in_container( + [ + "bash", + "-c", + """cat > /etc/clickhouse-server/config.d/storage_policy_{policy_name}.xml << EOF - - - <{policy_name}_disk> - encrypted - disk_local - {policy_name}_dir/ - {keys} - - - - <{policy_name}> - -
- {policy_name}_disk -
- - - - + + + <{policy_name}_disk> + encrypted + disk_local + {policy_name}_dir/ + {keys} + + + + <{policy_name}> + +
+ {policy_name}_disk +
+ + + + EOF""".format( - policy_name=policy_name, keys=keys - ), - ] + policy_name=policy_name, keys=keys + ), + ] + ) + + node.query("SYSTEM RELOAD CONFIG") + + if check_system_storage_policies: + assert_eq_with_retry( + node, + f"SELECT policy_name FROM system.storage_policies WHERE policy_name='{policy_name}'", + policy_name, ) - node.query("SYSTEM RELOAD CONFIG") + + +# Test adding encryption key on the fly. +def test_add_keys(): + keys = "firstfirstfirstf" + make_storage_policy_with_keys( + "encrypted_policy_multikeys", keys, check_system_storage_policies=True + ) # Add some data to an encrypted disk. - node.query("SELECT policy_name FROM system.storage_policies") - make_storage_policy_with_keys( - "encrypted_policy_multikeys", "firstfirstfirstf" - ) - assert_eq_with_retry( - node, - "SELECT policy_name FROM system.storage_policies WHERE policy_name='encrypted_policy_multikeys'", - "encrypted_policy_multikeys", - ) - node.query( """ CREATE TABLE encrypted_test ( @@ -233,31 +244,39 @@ EOF""".format( assert node.query(select_query) == "(0,'data'),(1,'data')" # Add a second key and start using it. - make_storage_policy_with_keys( - "encrypted_policy_multikeys", + keys = """ + firstfirstfirstf + secondsecondseco + secondsecondseco """ - firstfirstfirstf - secondsecondseco - 1 - """, - ) + make_storage_policy_with_keys("encrypted_policy_multikeys", keys) + node.query("INSERT INTO encrypted_test VALUES (2,'data'),(3,'data')") # Now "(0,'data'),(1,'data')" is encrypted with the first key and "(2,'data'),(3,'data')" is encrypted with the second key. # All data are accessible. assert node.query(select_query) == "(0,'data'),(1,'data'),(2,'data'),(3,'data')" - # Try to replace the first key with something wrong, and check that "(0,'data'),(1,'data')" cannot be read. - make_storage_policy_with_keys( - "encrypted_policy_multikeys", - """ - wrongwrongwrongw + # Keys can be reordered. + keys = """ secondsecondseco + firstfirstfirstf 1 - """, - ) + """ + make_storage_policy_with_keys("encrypted_policy_multikeys", keys) - expected_error = "Wrong key" + # All data are still accessible. + assert node.query(select_query) == "(0,'data'),(1,'data'),(2,'data'),(3,'data')" + + # Try to replace the first key with something wrong, and check that "(0,'data'),(1,'data')" cannot be read. + keys = """ + secondsecondseco + wrongwrongwrongw + secondsecondseco + """ + make_storage_policy_with_keys("encrypted_policy_multikeys", keys) + + expected_error = "Not found an encryption key required to decipher" assert expected_error in node.query_and_get_error(select_query) # Detach the part encrypted with the wrong key and check that another part containing "(2,'data'),(3,'data')" still can be read. @@ -265,6 +284,159 @@ EOF""".format( assert node.query(select_query) == "(2,'data'),(3,'data')" +# Test adding encryption key on the fly. +def test_add_keys_with_id(): + keys = "firstfirstfirstf" + make_storage_policy_with_keys( + "encrypted_policy_multikeys", keys, check_system_storage_policies=True + ) + + # Add some data to an encrypted disk. + node.query( + """ + CREATE TABLE encrypted_test ( + id Int64, + data String + ) ENGINE=MergeTree() + ORDER BY id + SETTINGS storage_policy='encrypted_policy_multikeys' + """ + ) + + node.query("INSERT INTO encrypted_test VALUES (0,'data'),(1,'data')") + select_query = "SELECT * FROM encrypted_test ORDER BY id FORMAT Values" + assert node.query(select_query) == "(0,'data'),(1,'data')" + + # Add a second key and start using it. + keys = """ + firstfirstfirstf + secondsecondseco + 1 + """ + make_storage_policy_with_keys("encrypted_policy_multikeys", keys) + + node.query("INSERT INTO encrypted_test VALUES (2,'data'),(3,'data')") + + # Now "(0,'data'),(1,'data')" is encrypted with the first key and "(2,'data'),(3,'data')" is encrypted with the second key. + # All data are accessible. + assert node.query(select_query) == "(0,'data'),(1,'data'),(2,'data'),(3,'data')" + + # Keys can be reordered. + keys = """ + secondsecondseco + firstfirstfirstf + 1 + """ + make_storage_policy_with_keys("encrypted_policy_multikeys", keys) + + # All data are still accessible. + assert node.query(select_query) == "(0,'data'),(1,'data'),(2,'data'),(3,'data')" + + # Try to replace the first key with something wrong, and check that "(0,'data'),(1,'data')" cannot be read. + keys = """ + secondsecondseco + wrongwrongwrongw + 1 + """ + make_storage_policy_with_keys("encrypted_policy_multikeys", keys) + + expected_error = "Not found an encryption key required to decipher" + assert expected_error in node.query_and_get_error(select_query) + + # Detach the part encrypted with the wrong key and check that another part containing "(2,'data'),(3,'data')" still can be read. + node.query("ALTER TABLE encrypted_test DETACH PART '{}'".format(FIRST_PART_NAME)) + assert node.query(select_query) == "(2,'data'),(3,'data')" + + +# Test appending of encrypted files. +def test_log_family(): + keys = "firstfirstfirstf" + make_storage_policy_with_keys( + "encrypted_policy_multikeys", keys, check_system_storage_policies=True + ) + + # Add some data to an encrypted disk. + node.query( + """ + CREATE TABLE encrypted_test ( + id Int64, + data String + ) ENGINE=Log + SETTINGS storage_policy='encrypted_policy_multikeys' + """ + ) + + node.query("INSERT INTO encrypted_test VALUES (0,'data'),(1,'data')") + select_query = "SELECT * FROM encrypted_test ORDER BY id FORMAT Values" + assert node.query(select_query) == "(0,'data'),(1,'data')" + + # Add a second key and start using it. + keys = """ + firstfirstfirstf + secondsecondseco + secondsecondseco + """ + make_storage_policy_with_keys("encrypted_policy_multikeys", keys) + + node.query("INSERT INTO encrypted_test VALUES (2,'data'),(3,'data')") + assert node.query(select_query) == "(0,'data'),(1,'data'),(2,'data'),(3,'data')" + + # Everything is still encrypted with the first key (because the Log engine appends files), so the second key can be removed. + keys = "firstfirstfirstf" + make_storage_policy_with_keys("encrypted_policy_multikeys", keys) + + assert node.query(select_query) == "(0,'data'),(1,'data'),(2,'data'),(3,'data')" + + +@pytest.mark.parametrize( + "old_version", + ["version_1le", "version_1be", "version_2"], +) +def test_migration_from_old_version(old_version): + keys = """ + first_key_first_ + second_key_secon + third_key_third_ + 3 + """ + make_storage_policy_with_keys( + "migration_from_old_version", keys, check_system_storage_policies=True + ) + + # Create a table without data. + node.query( + """ + CREATE TABLE encrypted_test ( + id Int64, + data String + ) ENGINE=Log + SETTINGS storage_policy='migration_from_old_version' + """ + ) + + # Copy table's data from an old version. + data_path = node.query( + "SELECT data_paths[1] FROM system.tables WHERE table = 'encrypted_test'" + ).splitlines()[0] + node.query("DETACH TABLE encrypted_test") + + old_version_dir = os.path.join(SCRIPT_DIR, "old_versions", old_version) + for file_name in os.listdir(old_version_dir): + src_path = os.path.join(old_version_dir, file_name) + dest_path = os.path.join(data_path, file_name) + node.copy_file_to_container(src_path, dest_path) + + node.query("ATTACH TABLE encrypted_test") + + # We can read from encrypted disk after migration. + select_query = "SELECT * FROM encrypted_test ORDER BY id FORMAT Values" + assert node.query(select_query) == "(0,'ab'),(1,'cdefg')" + + # We can append files on encrypted disk after migration. + node.query("INSERT INTO encrypted_test VALUES (2,'xyz')") + assert node.query(select_query) == "(0,'ab'),(1,'cdefg'),(2,'xyz')" + + def test_read_in_order(): node.query( "CREATE TABLE encrypted_test(`a` UInt64, `b` String(150)) ENGINE = MergeTree() ORDER BY (a, b) SETTINGS storage_policy='encrypted_policy'" From 4d4112ff536f819514973dfd0cb8274cf044bb3e Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Wed, 31 May 2023 15:26:56 +0300 Subject: [PATCH 42/44] Revert "less logs in WriteBufferFromS3" (#50390) --- src/IO/WriteBufferFromS3.cpp | 8 ++++++++ src/IO/WriteBufferFromS3TaskTracker.cpp | 11 +++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/IO/WriteBufferFromS3.cpp b/src/IO/WriteBufferFromS3.cpp index 6992c3ea4ac..462cf2674c3 100644 --- a/src/IO/WriteBufferFromS3.cpp +++ b/src/IO/WriteBufferFromS3.cpp @@ -195,14 +195,18 @@ void WriteBufferFromS3::finalizeImpl() if (request_settings.check_objects_after_upload) { + LOG_TRACE(log, "Checking object {} exists after upload", key); S3::checkObjectExists(*client_ptr, bucket, key, {}, request_settings, /* for_disk_s3= */ write_settings.for_object_storage, "Immediately after upload"); + LOG_TRACE(log, "Checking object {} has size as expected {}", key, total_size); size_t actual_size = S3::getObjectSize(*client_ptr, bucket, key, {}, request_settings, /* for_disk_s3= */ write_settings.for_object_storage); if (actual_size != total_size) throw Exception( ErrorCodes::S3_ERROR, "Object {} from bucket {} has unexpected size {} after upload, expected size {}, it's a bug in S3 or S3 API.", key, bucket, actual_size, total_size); + + LOG_TRACE(log, "Object {} exists after upload", key); } } @@ -288,6 +292,8 @@ void WriteBufferFromS3::reallocateFirstBuffer() WriteBuffer::set(memory.data() + hidden_size, memory.size() - hidden_size); chassert(offset() == 0); + + LOG_TRACE(log, "Reallocated first buffer with size {}. {}", memory.size(), getLogDetails()); } void WriteBufferFromS3::detachBuffer() @@ -310,6 +316,8 @@ void WriteBufferFromS3::allocateFirstBuffer() const auto size = std::min(size_t(DBMS_DEFAULT_BUFFER_SIZE), max_first_buffer); memory = Memory(size); WriteBuffer::set(memory.data(), memory.size()); + + LOG_TRACE(log, "Allocated first buffer with size {}. {}", memory.size(), getLogDetails()); } void WriteBufferFromS3::allocateBuffer() diff --git a/src/IO/WriteBufferFromS3TaskTracker.cpp b/src/IO/WriteBufferFromS3TaskTracker.cpp index c10af5d0672..7ae31044012 100644 --- a/src/IO/WriteBufferFromS3TaskTracker.cpp +++ b/src/IO/WriteBufferFromS3TaskTracker.cpp @@ -36,6 +36,8 @@ ThreadPoolCallbackRunner WriteBufferFromS3::TaskTracker::syncRunner() void WriteBufferFromS3::TaskTracker::waitAll() { + LOG_TEST(log, "waitAll, in queue {}", futures.size()); + /// Exceptions are propagated for (auto & future : futures) { @@ -49,6 +51,8 @@ void WriteBufferFromS3::TaskTracker::waitAll() void WriteBufferFromS3::TaskTracker::safeWaitAll() { + LOG_TEST(log, "safeWaitAll, wait in queue {}", futures.size()); + for (auto & future : futures) { if (future.valid()) @@ -72,6 +76,7 @@ void WriteBufferFromS3::TaskTracker::safeWaitAll() void WriteBufferFromS3::TaskTracker::waitIfAny() { + LOG_TEST(log, "waitIfAny, in queue {}", futures.size()); if (futures.empty()) return; @@ -96,6 +101,8 @@ void WriteBufferFromS3::TaskTracker::waitIfAny() watch.stop(); ProfileEvents::increment(ProfileEvents::WriteBufferFromS3WaitInflightLimitMicroseconds, watch.elapsedMicroseconds()); + + LOG_TEST(log, "waitIfAny ended, in queue {}", futures.size()); } void WriteBufferFromS3::TaskTracker::add(Callback && func) @@ -140,6 +147,8 @@ void WriteBufferFromS3::TaskTracker::waitTilInflightShrink() if (!max_tasks_inflight) return; + LOG_TEST(log, "waitTilInflightShrink, in queue {}", futures.size()); + Stopwatch watch; /// Alternative approach is to wait until at least futures.size() - max_tasks_inflight element are finished @@ -162,6 +171,8 @@ void WriteBufferFromS3::TaskTracker::waitTilInflightShrink() watch.stop(); ProfileEvents::increment(ProfileEvents::WriteBufferFromS3WaitInflightLimitMicroseconds, watch.elapsedMicroseconds()); + + LOG_TEST(log, "waitTilInflightShrink ended, in queue {}", futures.size()); } bool WriteBufferFromS3::TaskTracker::isAsync() const From 2efebee5a37b25b26898705ecd833a2c3091eaf9 Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Wed, 31 May 2023 16:24:36 +0300 Subject: [PATCH 43/44] Compare functions NaN update test (#50366) Co-authored-by: Nikita Mikhaylov --- ...e => 02769_compare_functions_nan.reference} | 1 + ...son.sql => 02769_compare_functions_nan.sql} | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) rename tests/queries/0_stateless/{02769_nan_equality_comparison.reference => 02769_compare_functions_nan.reference} (96%) rename tests/queries/0_stateless/{02769_nan_equality_comparison.sql => 02769_compare_functions_nan.sql} (78%) diff --git a/tests/queries/0_stateless/02769_nan_equality_comparison.reference b/tests/queries/0_stateless/02769_compare_functions_nan.reference similarity index 96% rename from tests/queries/0_stateless/02769_nan_equality_comparison.reference rename to tests/queries/0_stateless/02769_compare_functions_nan.reference index a8ba06cfce6..81f0ee6da73 100644 --- a/tests/queries/0_stateless/02769_nan_equality_comparison.reference +++ b/tests/queries/0_stateless/02769_compare_functions_nan.reference @@ -8,3 +8,4 @@ nan 1 1 1 1 nan nan 1 1 1 1 -- nan +-- diff --git a/tests/queries/0_stateless/02769_nan_equality_comparison.sql b/tests/queries/0_stateless/02769_compare_functions_nan.sql similarity index 78% rename from tests/queries/0_stateless/02769_nan_equality_comparison.sql rename to tests/queries/0_stateless/02769_compare_functions_nan.sql index 6cce19a2204..1e1a9df9ce2 100644 --- a/tests/queries/0_stateless/02769_nan_equality_comparison.sql +++ b/tests/queries/0_stateless/02769_compare_functions_nan.sql @@ -7,11 +7,13 @@ SELECT nan AS lhs, cast(nan, 'Float32') AS rhs, lhs = rhs, lhs = materialize(rhs SELECT '--'; +DROP TABLE IF EXISTS test_table; CREATE TABLE test_table ( id UInt32, value UInt32 ) ENGINE = MergeTree ORDER BY id; + INSERT INTO test_table VALUES (76, 57); SELECT value FROM (SELECT stddevSamp(id) AS value FROM test_table) as subquery @@ -33,6 +35,7 @@ CREATE TABLE test_table value_1 UInt32, value_2 Float32 ) ENGINE = MergeTree ORDER BY id; + INSERT INTO test_table VALUES (12000, 36, 77.94); SELECT value @@ -40,3 +43,18 @@ FROM (SELECT (corr(value_1, value_1) OVER test_window) AS value FROM test_table WHERE not (not (value <> value)); DROP TABLE test_table; + +SELECT '--'; + +CREATE TABLE test_table +( + id Float32, + value Float32 +) ENGINE=MergeTree ORDER BY id; + +INSERT INTO test_table VALUES (-10.75, 95.57); + +SELECT * FROM (SELECT corr(id, id) as corr_value FROM test_table GROUP BY value) AS subquery LEFT ANTI JOIN test_table ON (subquery.corr_value = test_table.id) +WHERE (test_table.id >= test_table.id) AND (NOT (test_table.id >= test_table.id)); + +DROP TABLE test_table; From aedd3afb8aa6127d24a7a84146be113f88936bed Mon Sep 17 00:00:00 2001 From: Sema Checherinda <104093494+CheSema@users.noreply.github.com> Date: Wed, 31 May 2023 18:20:58 +0200 Subject: [PATCH 44/44] fix hung in unit tests (#50391) * fix hung in unit tests * Update gtest_writebuffer_s3.cpp * Update gtest_writebuffer_s3.cpp --------- Co-authored-by: Alexander Tokmakov --- src/IO/tests/gtest_writebuffer_s3.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/IO/tests/gtest_writebuffer_s3.cpp b/src/IO/tests/gtest_writebuffer_s3.cpp index bc16af7f779..cd38291fb31 100644 --- a/src/IO/tests/gtest_writebuffer_s3.cpp +++ b/src/IO/tests/gtest_writebuffer_s3.cpp @@ -609,9 +609,16 @@ protected: test_with_pool = GetParam(); client = MockS3::Client::CreateClient(bucket); if (test_with_pool) + { + /// Do not block the main thread awaiting the others task. + /// This test use the only one thread at all + getSettings().s3_max_inflight_parts_for_one_file = 0; async_policy = std::make_unique(); + } else + { async_policy = std::make_unique(); + } } };