From 7abb251b973c464dc07febf29a58fb7e943ab8b0 Mon Sep 17 00:00:00 2001 From: flynn Date: Tue, 4 Oct 2022 15:29:53 +0000 Subject: [PATCH 01/65] filesystemAvailable and related functions support one optional argument with disk name --- src/Functions/filesystem.cpp | 86 +++++++++++++++---- tests/fuzz/dictionaries/functions.dict | 2 +- .../queries/0_stateless/00824_filesystem.sql | 2 +- .../0_stateless/02345_filesystem_local.sh | 2 +- ...new_functions_must_be_documented.reference | 2 +- .../02457_filesystem_function.reference | 1 + .../0_stateless/02457_filesystem_function.sql | 2 + 7 files changed, 74 insertions(+), 23 deletions(-) create mode 100644 tests/queries/0_stateless/02457_filesystem_function.reference create mode 100644 tests/queries/0_stateless/02457_filesystem_function.sql diff --git a/src/Functions/filesystem.cpp b/src/Functions/filesystem.cpp index 12813c3d852..9b1ed83bc55 100644 --- a/src/Functions/filesystem.cpp +++ b/src/Functions/filesystem.cpp @@ -1,31 +1,40 @@ -#include -#include +#include +#include #include +#include +#include +#include #include -#include #include namespace DB { +namespace ErrorCodes +{ + extern const int ILLEGAL_COLUMN; + extern const int ILLEGAL_TYPE_OF_ARGUMENT; + extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH; + extern const int UNKNOWN_DISK; +} namespace { struct FilesystemAvailable { static constexpr auto name = "filesystemAvailable"; - static std::uintmax_t get(const std::filesystem::space_info & spaceinfo) { return spaceinfo.available; } + static std::uintmax_t get(const DiskPtr & disk) { return disk->getAvailableSpace(); } }; -struct FilesystemFree +struct FilesystemUnreserved { - static constexpr auto name = "filesystemFree"; - static std::uintmax_t get(const std::filesystem::space_info & spaceinfo) { return spaceinfo.free; } + static constexpr auto name = "filesystemUnreserved"; + static std::uintmax_t get(const DiskPtr & disk) { return disk->getUnreservedSpace(); } }; struct FilesystemCapacity { static constexpr auto name = "filesystemCapacity"; - static std::uintmax_t get(const std::filesystem::space_info & spaceinfo) { return spaceinfo.capacity; } + static std::uintmax_t get(const DiskPtr & disk) { return disk->getTotalSpace(); } }; template @@ -34,34 +43,73 @@ class FilesystemImpl : public IFunction public: static constexpr auto name = Impl::name; - static FunctionPtr create(ContextPtr context) - { - return std::make_shared>(std::filesystem::space(context->getPath())); - } + static FunctionPtr create(ContextPtr context_) { return std::make_shared>(context_); } + + explicit FilesystemImpl(ContextPtr context_) : context(context_) { } + + bool useDefaultImplementationForConstants() const override { return true; } bool isSuitableForShortCircuitArgumentsExecution(const DataTypesWithConstInfo & /*arguments*/) const override { return false; } - explicit FilesystemImpl(std::filesystem::space_info spaceinfo_) : spaceinfo(spaceinfo_) { } - String getName() const override { return name; } + + bool isVariadic() const override { return true; } + size_t getNumberOfArguments() const override { return 0; } bool isDeterministic() const override { return false; } - DataTypePtr getReturnTypeImpl(const DataTypes & /*arguments*/) const override + DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override { + if (arguments.size() > 1) + { + throw Exception("Arguments size of function " + getName() + " should be 0 or 1", ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH); + } + if (arguments.size() == 1 && !isStringOrFixedString(arguments[0])) + { + throw Exception( + "Arguments of function " + getName() + " should be String or FixedString", ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); + } return std::make_shared(); } - ColumnPtr executeImpl(const ColumnsWithTypeAndName &, const DataTypePtr &, size_t input_rows_count) const override + ColumnPtr executeImpl(const ColumnsWithTypeAndName & arguments, const DataTypePtr &, size_t input_rows_count) const override { - return DataTypeUInt64().createColumnConst(input_rows_count, static_cast(Impl::get(spaceinfo))); + if (arguments.empty()) + { + auto disk = context->getDisk("default"); + return DataTypeUInt64().createColumnConst(input_rows_count, Impl::get(disk)); + } + else + { + auto col = arguments[0].column; + if (const ColumnString * col_str = checkAndGetColumn(col.get())) + { + auto disk_map = context->getDisksMap(); + + auto col_res = ColumnVector::create(col_str->size()); + auto & data = col_res->getData(); + for (size_t i = 0; i < col_str->size(); ++i) + { + auto disk_name = col_str->getDataAt(i).toString(); + if (auto it = disk_map.find(disk_name); it != disk_map.end()) + data[i] = Impl::get(it->second); + else + throw Exception( + "Unknown disk name " + disk_name + " while execute function " + getName(), ErrorCodes::UNKNOWN_DISK); + } + return col_res; + } + throw Exception( + "Illegal column " + arguments[0].column->getName() + " of argument of function " + getName(), + ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); + } } private: - std::filesystem::space_info spaceinfo; + ContextPtr context; }; } @@ -70,7 +118,7 @@ REGISTER_FUNCTION(Filesystem) { factory.registerFunction>(); factory.registerFunction>(); - factory.registerFunction>(); + factory.registerFunction>(); } } diff --git a/tests/fuzz/dictionaries/functions.dict b/tests/fuzz/dictionaries/functions.dict index b90697f0c3d..5df9376aff1 100644 --- a/tests/fuzz/dictionaries/functions.dict +++ b/tests/fuzz/dictionaries/functions.dict @@ -249,7 +249,7 @@ "cosh" "basename" "evalMLMethod" -"filesystemFree" +"filesystemUnreserved" "filesystemCapacity" "reinterpretAsDate" "filesystemAvailable" diff --git a/tests/queries/0_stateless/00824_filesystem.sql b/tests/queries/0_stateless/00824_filesystem.sql index cd4d69a703e..cecf66810cf 100644 --- a/tests/queries/0_stateless/00824_filesystem.sql +++ b/tests/queries/0_stateless/00824_filesystem.sql @@ -1 +1 @@ -SELECT filesystemCapacity() >= filesystemFree() AND filesystemFree() >= filesystemAvailable() AND filesystemAvailable() >= 0; +SELECT filesystemCapacity() >= filesystemAvailable() AND filesystemAvailable() >= filesystemUnreserved() AND filesystemUnreserved() >= 0; diff --git a/tests/queries/0_stateless/02345_filesystem_local.sh b/tests/queries/0_stateless/02345_filesystem_local.sh index 6771df2ae2d..aac66f9f7b9 100755 --- a/tests/queries/0_stateless/02345_filesystem_local.sh +++ b/tests/queries/0_stateless/02345_filesystem_local.sh @@ -5,4 +5,4 @@ CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) . "$CUR_DIR"/../shell_config.sh # Checks that these functions are working inside clickhouse-local. Does not check specific values. -$CLICKHOUSE_LOCAL --query "SELECT filesystemAvailable() > 0, filesystemFree() <= filesystemCapacity()" +$CLICKHOUSE_LOCAL --query "SELECT filesystemAvailable() > 0, filesystemUnreserved() <= filesystemCapacity()" diff --git a/tests/queries/0_stateless/02415_all_new_functions_must_be_documented.reference b/tests/queries/0_stateless/02415_all_new_functions_must_be_documented.reference index c7ac00ee18f..da4bd49b62b 100644 --- a/tests/queries/0_stateless/02415_all_new_functions_must_be_documented.reference +++ b/tests/queries/0_stateless/02415_all_new_functions_must_be_documented.reference @@ -328,7 +328,7 @@ farmHash64 file filesystemAvailable filesystemCapacity -filesystemFree +filesystemUnreserved finalizeAggregation firstSignificantSubdomain firstSignificantSubdomainCustom diff --git a/tests/queries/0_stateless/02457_filesystem_function.reference b/tests/queries/0_stateless/02457_filesystem_function.reference new file mode 100644 index 00000000000..d00491fd7e5 --- /dev/null +++ b/tests/queries/0_stateless/02457_filesystem_function.reference @@ -0,0 +1 @@ +1 diff --git a/tests/queries/0_stateless/02457_filesystem_function.sql b/tests/queries/0_stateless/02457_filesystem_function.sql new file mode 100644 index 00000000000..fac418c046a --- /dev/null +++ b/tests/queries/0_stateless/02457_filesystem_function.sql @@ -0,0 +1,2 @@ +select filesystemCapacity('default') >= filesystemAvailable('default') and filesystemAvailable('default') >= filesystemUnreserved('default'); +select filesystemCapacity('__un_exists_disk'); -- { serverError UNKNOWN_DISK } From a4fdfb658661d687ff6cc2ea7a9d010aa9608fab Mon Sep 17 00:00:00 2001 From: flynn Date: Tue, 4 Oct 2022 16:32:31 +0000 Subject: [PATCH 02/65] fix --- src/Functions/filesystem.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Functions/filesystem.cpp b/src/Functions/filesystem.cpp index 9b1ed83bc55..7af1c61d3b8 100644 --- a/src/Functions/filesystem.cpp +++ b/src/Functions/filesystem.cpp @@ -103,8 +103,7 @@ public: return col_res; } throw Exception( - "Illegal column " + arguments[0].column->getName() + " of argument of function " + getName(), - ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); + "Illegal column " + arguments[0].column->getName() + " of argument of function " + getName(), ErrorCodes::ILLEGAL_COLUMN); } } From 54129115924e9fe8d61f48d61737c723c9ebe92f Mon Sep 17 00:00:00 2001 From: avogar Date: Fri, 28 Oct 2022 17:43:07 +0000 Subject: [PATCH 03/65] Add documentation for 'format' table function --- .../sql-reference/table-functions/format.md | 75 +++++++++++++++++++ .../sql-reference/table-functions/format.md | 1 + .../sql-reference/table-functions/format,md | 1 + 3 files changed, 77 insertions(+) create mode 100644 docs/en/sql-reference/table-functions/format.md create mode 120000 docs/ru/sql-reference/table-functions/format.md create mode 120000 docs/zh/sql-reference/table-functions/format,md diff --git a/docs/en/sql-reference/table-functions/format.md b/docs/en/sql-reference/table-functions/format.md new file mode 100644 index 00000000000..bc0ada9795b --- /dev/null +++ b/docs/en/sql-reference/table-functions/format.md @@ -0,0 +1,75 @@ +--- +slug: /en/sql-reference/table-functions/format +sidebar_position: 56 +sidebar_label: format +--- + +# format + +Extracts table structure from data and parse it according to specified input format. + +**Syntax** + +``` sql +format(format_name, data) +``` + +**Parameters** + +- `format_name` — The [format](../../interfaces/formats.md#formats) of the data. +- `data` — String literal containing data in specified format + +**Returned value** + +A table with data parsed from `data` argument according specified format and extracted schema. + +**Examples** + +**Query:** +``` sql +:) select * from format(JSONEachRow, +$$ +{"a": "Hello", "b": 111} +{"a": "World", "b": 123} +{"a": "Hello", "b": 112} +{"a": "World", "b": 124} +$$) +``` + +**Result:** + +```text +┌───b─┬─a─────┐ +│ 111 │ Hello │ +│ 123 │ World │ +│ 112 │ Hello │ +│ 124 │ World │ +└─────┴───────┘ +``` + +**Query:** +```sql + +:) desc format(JSONEachRow, +$$ +{"a": "Hello", "b": 111} +{"a": "World", "b": 123} +{"a": "Hello", "b": 112} +{"a": "World", "b": 124} +$$) +``` + +**Result:** + +```text +┌─name─┬─type──────────────┬─default_type─┬─default_expression─┬─comment─┬─codec_expression─┬─ttl_expression─┐ +│ b │ Nullable(Float64) │ │ │ │ │ │ +│ a │ Nullable(String) │ │ │ │ │ │ +└──────┴───────────────────┴──────────────┴────────────────────┴─────────┴──────────────────┴────────────────┘ +``` + +**See Also** + +- [Formats](../../interfaces/formats.md) + +[Original article](https://clickhouse.com/docs/en/sql-reference/table-functions/format) diff --git a/docs/ru/sql-reference/table-functions/format.md b/docs/ru/sql-reference/table-functions/format.md new file mode 120000 index 00000000000..cc5e3a5a142 --- /dev/null +++ b/docs/ru/sql-reference/table-functions/format.md @@ -0,0 +1 @@ +../../../en/sql-reference/table-functions/format.md \ No newline at end of file diff --git a/docs/zh/sql-reference/table-functions/format,md b/docs/zh/sql-reference/table-functions/format,md new file mode 120000 index 00000000000..cc5e3a5a142 --- /dev/null +++ b/docs/zh/sql-reference/table-functions/format,md @@ -0,0 +1 @@ +../../../en/sql-reference/table-functions/format.md \ No newline at end of file From c7031569b66671a1f8eeda5b3a78c0cb7de5d651 Mon Sep 17 00:00:00 2001 From: avogar Date: Fri, 28 Oct 2022 21:58:34 +0000 Subject: [PATCH 04/65] Fix typo --- docs/zh/sql-reference/table-functions/{format,md => format.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename docs/zh/sql-reference/table-functions/{format,md => format.md} (100%) diff --git a/docs/zh/sql-reference/table-functions/format,md b/docs/zh/sql-reference/table-functions/format.md similarity index 100% rename from docs/zh/sql-reference/table-functions/format,md rename to docs/zh/sql-reference/table-functions/format.md From 800e789ac44292cd59823fd0a736fc6110b27d11 Mon Sep 17 00:00:00 2001 From: Alexander Gololobov <440544+davenger@users.noreply.github.com> Date: Sat, 29 Oct 2022 09:26:16 +0200 Subject: [PATCH 05/65] Small fix --- docs/en/sql-reference/table-functions/format.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/sql-reference/table-functions/format.md b/docs/en/sql-reference/table-functions/format.md index bc0ada9795b..da287c77283 100644 --- a/docs/en/sql-reference/table-functions/format.md +++ b/docs/en/sql-reference/table-functions/format.md @@ -6,7 +6,7 @@ sidebar_label: format # format -Extracts table structure from data and parse it according to specified input format. +Extracts table structure from data and parses it according to specified input format. **Syntax** From 95e64310a611e4c01ca6376bc8ab8c9480268a8f Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Tue, 1 Nov 2022 14:06:45 +0100 Subject: [PATCH 06/65] Update format.md --- docs/en/sql-reference/table-functions/format.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/sql-reference/table-functions/format.md b/docs/en/sql-reference/table-functions/format.md index da287c77283..4d1488ea640 100644 --- a/docs/en/sql-reference/table-functions/format.md +++ b/docs/en/sql-reference/table-functions/format.md @@ -17,7 +17,7 @@ format(format_name, data) **Parameters** - `format_name` — The [format](../../interfaces/formats.md#formats) of the data. -- `data` — String literal containing data in specified format +- `data` — String literal or constant expression that returns a string containing data in specified format **Returned value** From 9d9bca3dbba9ce0d413ae9da85808eb31549fa13 Mon Sep 17 00:00:00 2001 From: avogar Date: Tue, 1 Nov 2022 14:08:34 +0000 Subject: [PATCH 07/65] Add embedded documentation --- src/TableFunctions/TableFunctionFormat.cpp | 67 +++++++++++++++++++++- 1 file changed, 65 insertions(+), 2 deletions(-) diff --git a/src/TableFunctions/TableFunctionFormat.cpp b/src/TableFunctions/TableFunctionFormat.cpp index 9f239adb538..15a82044dab 100644 --- a/src/TableFunctions/TableFunctionFormat.cpp +++ b/src/TableFunctions/TableFunctionFormat.cpp @@ -89,9 +89,72 @@ StoragePtr TableFunctionFormat::executeImpl(const ASTPtr & /*ast_function*/, Con return res; } +static const Documentation format_table_function_documentation = +{ + R"( +Extracts table structure from data and parses it according to specified input format. +Syntax: `format(format_name, data)`. +Parameters: + - `format_name` - the format of the data. + - `data ` - String literal or constant expression that returns a string containing data in specified format. +Returned value: A table with data parsed from `data` argument according specified format and extracted schema. +)", + Documentation::Examples + { + { + "First example", + R"( +Query: +``` +:) select * from format(JSONEachRow, +$$ +{"a": "Hello", "b": 111} +{"a": "World", "b": 123} +{"a": "Hello", "b": 112} +{"a": "World", "b": 124} +$$) +``` + +Result: +``` +┌───b─┬─a─────┐ +│ 111 │ Hello │ +│ 123 │ World │ +│ 112 │ Hello │ +│ 124 │ World │ +└─────┴───────┘ +``` +)" + }, + { + "Second example", + R"( +Query: +``` +:) desc format(JSONEachRow, +$$ +{"a": "Hello", "b": 111} +{"a": "World", "b": 123} +{"a": "Hello", "b": 112} +{"a": "World", "b": 124} +$$) +``` + +Result: +``` +┌─name─┬─type──────────────┬─default_type─┬─default_expression─┬─comment─┬─codec_expression─┬─ttl_expression─┐ +│ b │ Nullable(Float64) │ │ │ │ │ │ +│ a │ Nullable(String) │ │ │ │ │ │ +└──────┴───────────────────┴──────────────┴────────────────────┴─────────┴──────────────────┴────────────────┘ +``` +)" + }, + }, + Documentation::Categories{"format", "table-functions"} +}; + void registerTableFunctionFormat(TableFunctionFactory & factory) { - factory.registerFunction({}, TableFunctionFactory::CaseInsensitive); + factory.registerFunction(format_table_function_documentation, TableFunctionFactory::CaseInsensitive); } - } From ca708dd4b6d52e0f29f163c017f49b8d4761ce09 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Wed, 2 Nov 2022 13:25:09 +0100 Subject: [PATCH 08/65] Fix test --- .../02414_all_new_table_functions_must_be_documented.reference | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/queries/0_stateless/02414_all_new_table_functions_must_be_documented.reference b/tests/queries/0_stateless/02414_all_new_table_functions_must_be_documented.reference index bb8c8c2228a..2277e19cf25 100644 --- a/tests/queries/0_stateless/02414_all_new_table_functions_must_be_documented.reference +++ b/tests/queries/0_stateless/02414_all_new_table_functions_must_be_documented.reference @@ -3,7 +3,6 @@ clusterAllReplicas dictionary executable file -format generateRandom input jdbc From 5a3f324311e1f164b40f7782cef0be6c66ebfccb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Fri, 11 Nov 2022 17:54:48 +0100 Subject: [PATCH 09/65] Try adding compatibility on write instead of on read --- .../AggregateFunctionMinMaxAny.h | 41 ++++++--- .../test_functions.py | 6 +- ...gle_value_data_string_regression.reference | 19 ++++ ...77_single_value_data_string_regression.sql | 87 +++++++++++++++++++ 4 files changed, 142 insertions(+), 11 deletions(-) create mode 100644 tests/queries/0_stateless/02477_single_value_data_string_regression.reference create mode 100644 tests/queries/0_stateless/02477_single_value_data_string_regression.sql diff --git a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h index f8d252cf8e9..b39594a624b 100644 --- a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h +++ b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h @@ -471,10 +471,7 @@ public: static constexpr bool is_nullable = false; static constexpr bool is_any = false; - bool has() const - { - return size >= 0; - } + bool has() const { return size >= 1; } const char * getData() const { @@ -489,16 +486,40 @@ public: void insertResultInto(IColumn & to) const { if (has()) - assert_cast(to).insertData(getData(), size); - else - assert_cast(to).insertDefault(); + { + const char * data = getData(); + size_t final_size = data[size - 1] != '\0' ? size : size - 1; + if (final_size) + { + assert_cast(to).insertData(data, final_size); + return; + } + } + + assert_cast(to).insertDefault(); } void write(WriteBuffer & buf, const ISerialization & /*serialization*/) const { - writeBinary(size, buf); - if (has()) - buf.write(getData(), size); + if (!has()) + { + writeBinary(Int32{1}, buf); + buf.write('\0'); + return; + } + + const char * data = getData(); + if (data[size - 1] != '\0') + { + writeBinary(size + 1, buf); + buf.write(data, size); + buf.write('\0'); + } + else + { + writeBinary(size, buf); + buf.write(data, size); + } } void read(ReadBuffer & buf, const ISerialization & /*serialization*/, Arena * arena) diff --git a/tests/integration/test_backward_compatibility/test_functions.py b/tests/integration/test_backward_compatibility/test_functions.py index fe1c0ea7108..a0db431087d 100644 --- a/tests/integration/test_backward_compatibility/test_functions.py +++ b/tests/integration/test_backward_compatibility/test_functions.py @@ -13,7 +13,11 @@ upstream = cluster.add_instance("upstream") backward = cluster.add_instance( "backward", image="clickhouse/clickhouse-server", - tag="22.9", + # Note that a bug changed the string representation of several aggregations in 22.9 and 22.10 and some minor + # releases of 22.8, 22.7 and 22.3 + # See https://github.com/ClickHouse/ClickHouse/issues/42916 + # Affected at least: singleValueOrNull, last_value, min, max, any, anyLast, anyHeavy, first_value, argMin, argMax + tag="22.6", with_installed_binary=True, ) diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.reference b/tests/queries/0_stateless/02477_single_value_data_string_regression.reference new file mode 100644 index 00000000000..508b6cae649 --- /dev/null +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.reference @@ -0,0 +1,19 @@ +1 +22.8.5.29 10 +22.8.6.71 10 +1 +22.8.5.29 52 +22.8.6.71 52 +1 +22.8.5.29 0 +22.8.6.71 0 +46_OK 0123456789012345678901234567890123456789012345 +46_KO 0123456789012345678901234567890123456789012345 +47_OK 01234567890123456789012345678901234567890123456 +47_KO 01234567890123456789012345678901234567890123456 +48_OK 012345678901234567890123456789012345678901234567 +48_KO 012345678901234567890123456789012345678901234567 +63_OK 012345678901234567890123456789012345678901234567890123456789012 +63_KO 012345678901234567890123456789012345678901234567890123456789012 +64_OK 0123456789012345678901234567890123456789012345678901234567890123 +64_KO 0123456789012345678901234567890123456789012345678901234567890123 diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql new file mode 100644 index 00000000000..b126707535f --- /dev/null +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql @@ -0,0 +1,87 @@ + +-- Context: https://github.com/ClickHouse/ClickHouse/issues/42916 + +-- STRING WITH 10 CHARACTERS +-- SELECT version() AS v, hex(argMaxState('0123456789', number)) AS state FROM numbers(1) FORMAT CSV + +CREATE TABLE argmaxstate_hex_small +( + `v` String, + `state` String +) +ENGINE = TinyLog; + +INSERT into argmaxstate_hex_small VALUES ('22.8.5.29','0B0000003031323334353637383900010000000000000000'), ('22.8.6.71','0A00000030313233343536373839010000000000000000'); + +-- Assert that the current version will write the same as 22.8.5 (last known good 22.8 minor) +SELECT + (SELECT hex(argMaxState('0123456789', number)) FROM numbers(1)) = state +FROM argmaxstate_hex_small +WHERE v = '22.8.5.29'; + +-- Assert that the current version can read correctly both the old and the regression states +SELECT + v, + length(finalizeAggregation(CAST(unhex(state) AS AggregateFunction(argMax, String, UInt64)))) +FROM argmaxstate_hex_small; + +-- STRING WITH 54 characters +-- SELECT version() AS v, hex(argMaxState('ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz', number)) AS state FROM numbers(1) FORMAT CSV +CREATE TABLE argmaxstate_hex_large +( + `v` String, + `state` String +) +ENGINE = TinyLog; + +INSERT into argmaxstate_hex_large VALUES ('22.8.5.29','350000004142434445464748494A4B4C4D4E4F505152535455565758595A6162636465666768696A6B6C6D6E6F707172737475767778797A00010000000000000000'), ('22.8.6.71','340000004142434445464748494A4B4C4D4E4F505152535455565758595A6162636465666768696A6B6C6D6E6F707172737475767778797A010000000000000000'); + +SELECT + (SELECT hex(argMaxState('ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz', number)) FROM numbers(1)) = state +FROM argmaxstate_hex_large +WHERE v = '22.8.5.29'; + +SELECT + v, + length(finalizeAggregation(CAST(unhex(state) AS AggregateFunction(argMax, String, UInt64)))) +FROM argmaxstate_hex_large; + +-- STRING WITH 0 characters +-- SELECT version() AS v, hex(argMaxState('', number)) AS state FROM numbers(1) FORMAT CSV +CREATE TABLE argmaxstate_hex_empty +( + `v` String, + `state` String +) +ENGINE = TinyLog; + +INSERT into argmaxstate_hex_empty VALUES ('22.8.5.29','0100000000010000000000000000'), ('22.8.6.71','00000000010000000000000000'); + +SELECT + (SELECT hex(argMaxState('', number)) FROM numbers(1)) = state +FROM argmaxstate_hex_empty +WHERE v = '22.8.5.29'; + +SELECT v, length(finalizeAggregation(CAST(unhex(state) AS AggregateFunction(argMax, String, UInt64)))) +FROM argmaxstate_hex_empty; + +-- Right in the border of small and large buffers +-- SELECT hex(argMaxState('0123456789012345678901234567890123456789012345' as a, number)) AS state, length(a) FROM numbers(1) FORMAT CSV +SELECT '46_OK', finalizeAggregation(CAST(unhex('2F0000003031323334353637383930313233343536373839303132333435363738393031323334353637383930313233343500010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); +SELECT '46_KO', finalizeAggregation(CAST(unhex('2E00000030313233343536373839303132333435363738393031323334353637383930313233343536373839303132333435010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); + +-- SELECT hex(argMaxState('01234567890123456789012345678901234567890123456' as a, number)) AS state, length(a) FROM numbers(1) FORMAT CSV +SELECT '47_OK', finalizeAggregation(CAST(unhex('30000000303132333435363738393031323334353637383930313233343536373839303132333435363738393031323334353600010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); +SELECT '47_KO', finalizeAggregation(CAST(unhex('2F0000003031323334353637383930313233343536373839303132333435363738393031323334353637383930313233343536010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); + +-- SELECT hex(argMaxState('012345678901234567890123456789012345678901234567' as a, number)) AS state, length(a) FROM numbers(1) FORMAT CSV +SELECT '48_OK', finalizeAggregation(CAST(unhex('3100000030313233343536373839303132333435363738393031323334353637383930313233343536373839303132333435363700010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); +SELECT '48_KO', finalizeAggregation(CAST(unhex('30000000303132333435363738393031323334353637383930313233343536373839303132333435363738393031323334353637010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); + +-- Right in the allocation limit (power of 2) +-- SELECT hex(argMaxState('012345678901234567890123456789012345678901234567890123456789012' as a, number)) AS state, length(a) FROM numbers(1) FORMAT CSV +SELECT '63_OK', finalizeAggregation(CAST(unhex('4000000030313233343536373839303132333435363738393031323334353637383930313233343536373839303132333435363738393031323334353637383930313200010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); +SELECT '63_KO', finalizeAggregation(CAST(unhex('3F000000303132333435363738393031323334353637383930313233343536373839303132333435363738393031323334353637383930313233343536373839303132010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); +-- SELECT hex(argMaxState('0123456789012345678901234567890123456789012345678901234567890123' as a, number)) AS state, length(a) FROM numbers(1) FORMAT CSV +SELECT '64_OK', finalizeAggregation(CAST(unhex('410000003031323334353637383930313233343536373839303132333435363738393031323334353637383930313233343536373839303132333435363738393031323300010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); +SELECT '64_KO', finalizeAggregation(CAST(unhex('4000000030313233343536373839303132333435363738393031323334353637383930313233343536373839303132333435363738393031323334353637383930313233010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); From 831abbaff94e3efadc5d816b999d304cc961a3f7 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Mon, 14 Nov 2022 17:29:30 +0100 Subject: [PATCH 10/65] Fix build --- src/TableFunctions/TableFunctionFormat.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/TableFunctions/TableFunctionFormat.cpp b/src/TableFunctions/TableFunctionFormat.cpp index 15a82044dab..b15b350f00b 100644 --- a/src/TableFunctions/TableFunctionFormat.cpp +++ b/src/TableFunctions/TableFunctionFormat.cpp @@ -155,6 +155,6 @@ Result: void registerTableFunctionFormat(TableFunctionFactory & factory) { - factory.registerFunction(format_table_function_documentation, TableFunctionFactory::CaseInsensitive); + factory.registerFunction({format_table_function_documentation, false}, TableFunctionFactory::CaseInsensitive); } } From 790cfe2b7ea57de80eca3def68deed476dfe70ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 14 Nov 2022 18:10:36 +0100 Subject: [PATCH 11/65] Allow backward incompatible change for toDate32 since it was declared as such in 22.8 --- .../test_functions.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/integration/test_backward_compatibility/test_functions.py b/tests/integration/test_backward_compatibility/test_functions.py index a0db431087d..304d25bc8ac 100644 --- a/tests/integration/test_backward_compatibility/test_functions.py +++ b/tests/integration/test_backward_compatibility/test_functions.py @@ -143,6 +143,10 @@ def test_string_functions(start_cluster): "substring", "CAST", # NOTE: no need to ignore now()/now64() since they will fail because they don't accept any argument + + # 22.8 Backward Incompatible Change: Extended range of Date32 + "toDate32OrZero", + "toDate32OrDefault" ] functions = filter(lambda x: x not in excludes, functions) @@ -153,14 +157,15 @@ def test_string_functions(start_cluster): failed = 0 passed = 0 - def get_function_value(node, function_name, value="foo"): + def get_function_value(node, function_name, value): return node.query(f"select {function_name}('{value}')").strip() + v = "foo" for function in functions: - logging.info("Checking %s", function) + logging.info("Checking %s('%s')", function, v) try: - backward_value = get_function_value(backward, function) + backward_value = get_function_value(backward, function, v) except QueryRuntimeException as e: error_message = str(e) allowed_errors = [ @@ -203,11 +208,12 @@ def test_string_functions(start_cluster): failed += 1 continue - upstream_value = get_function_value(upstream, function) + upstream_value = get_function_value(upstream, function, v) if upstream_value != backward_value: - logging.info( - "Failed %s, %s (backward) != %s (upstream)", + logging.warning( + "Failed %s('%s') %s (backward) != %s (upstream)", function, + v, backward_value, upstream_value, ) From 19d39d7881fd2cbb6cafaa44c3f9b1837859108a Mon Sep 17 00:00:00 2001 From: Mikhail Filimonov Date: Thu, 3 Nov 2022 19:10:22 +0100 Subject: [PATCH 12/65] Add SensitiveDataMasker to exceptions messages --- src/Common/Exception.cpp | 14 ++++++++--- src/Common/Exception.h | 22 +++++++++++++--- .../00956_sensitive_data_masking.reference | 3 +++ .../00956_sensitive_data_masking.sh | 25 ++++++++++++++++++- 4 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/Common/Exception.cpp b/src/Common/Exception.cpp index 399ccecf000..35231354651 100644 --- a/src/Common/Exception.cpp +++ b/src/Common/Exception.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -63,11 +64,18 @@ void handle_error_code([[maybe_unused]] const std::string & msg, int code, bool ErrorCodes::increment(code, remote, msg, trace); } -Exception::Exception(const std::string & msg, int code, bool remote_) - : Poco::Exception(msg, code) +Exception::MessageMasked::MessageMasked(const std::string & msg_) + : msg(msg_) +{ + if (auto * masker = SensitiveDataMasker::getInstance()) + masker->wipeSensitiveData(msg); +} + +Exception::Exception(const MessageMasked & msg_masked, int code, bool remote_) + : Poco::Exception(msg_masked.msg, code) , remote(remote_) { - handle_error_code(msg, code, remote, getStackFramePointers()); + handle_error_code(msg_masked.msg, code, remote, getStackFramePointers()); } Exception::Exception(CreateFromPocoTag, const Poco::Exception & exc) diff --git a/src/Common/Exception.h b/src/Common/Exception.h index 84687581e52..458fc0aa19a 100644 --- a/src/Common/Exception.h +++ b/src/Common/Exception.h @@ -27,7 +27,18 @@ public: using FramePointers = std::vector; Exception() = default; - Exception(const std::string & msg, int code, bool remote_ = false); + + // used to remove the sensitive information from exceptions if query_masking_rules is configured + struct MessageMasked { + std::string msg; + MessageMasked(const std::string & msg_); + }; + + Exception(const MessageMasked & msg_masked, int code, bool remote_); + + // delegating constructor to mask sensitive information from the message + Exception(const std::string & msg, int code, bool remote_ = false): Exception(MessageMasked(msg), code, remote_) + {} Exception(int code, const std::string & message) : Exception(message, code) @@ -54,12 +65,17 @@ public: template void addMessage(fmt::format_string format, Args &&... args) { - extendedMessage(fmt::format(format, std::forward(args)...)); + addMessage(fmt::format(format, std::forward(args)...)); } void addMessage(const std::string& message) { - extendedMessage(message); + addMessage(MessageMasked(message)); + } + + void addMessage(const MessageMasked & msg_masked) + { + extendedMessage(msg_masked.msg); } /// Used to distinguish local exceptions from the one that was received from remote node. diff --git a/tests/queries/0_stateless/00956_sensitive_data_masking.reference b/tests/queries/0_stateless/00956_sensitive_data_masking.reference index 86323ec45e8..457ab9118f1 100644 --- a/tests/queries/0_stateless/00956_sensitive_data_masking.reference +++ b/tests/queries/0_stateless/00956_sensitive_data_masking.reference @@ -1,11 +1,14 @@ 1 2 3 +3.1 4 5 5.1 6 7 +7.1 +7.2 8 9 text_log non empty diff --git a/tests/queries/0_stateless/00956_sensitive_data_masking.sh b/tests/queries/0_stateless/00956_sensitive_data_masking.sh index e36031c54be..ccd9bbcf10e 100755 --- a/tests/queries/0_stateless/00956_sensitive_data_masking.sh +++ b/tests/queries/0_stateless/00956_sensitive_data_masking.sh @@ -37,12 +37,20 @@ rm -f "$tmp_file" >/dev/null 2>&1 echo 3 # failure at before query start $CLICKHOUSE_CLIENT \ - --query="SELECT 'find_me_TOPSECRET=TOPSECRET' FROM non_existing_table FORMAT Null" \ + --query="SELECT 1 FROM system.numbers WHERE credit_card_number='find_me_TOPSECRET=TOPSECRET' FORMAT Null" \ --log_queries=1 --ignore-error --multiquery |& grep -v '^(query: ' > "$tmp_file" grep -F 'find_me_[hidden]' "$tmp_file" >/dev/null || echo 'fail 3a' grep -F 'TOPSECRET' "$tmp_file" && echo 'fail 3b' +echo '3.1' +echo "SELECT 1 FROM system.numbers WHERE credit_card_number='find_me_TOPSECRET=TOPSECRET' FORMAT Null" | ${CLICKHOUSE_CURL} -sSg "${CLICKHOUSE_URL}" -d @- >"$tmp_file" 2>&1 + +grep -F 'find_me_[hidden]' "$tmp_file" >/dev/null || echo 'fail 3.1a' +grep -F 'TOPSECRET' "$tmp_file" && echo 'fail 3.1b' + +#echo "SELECT 1 FROM system.numbers WHERE credit_card_number='find_me_TOPSECRET=TOPSECRET' FORMAT Null" | curl -sSg http://172.17.0.3:8123/ -d @- + rm -f "$tmp_file" >/dev/null 2>&1 echo 4 # failure at the end of query @@ -100,6 +108,21 @@ $CLICKHOUSE_CLIENT \ --server_logs_file=/dev/null \ --query="select * from system.query_log where current_database = currentDatabase() AND event_date >= yesterday() and query like '%TOPSECRET%';" +echo '7.1' +# query_log exceptions +$CLICKHOUSE_CLIENT \ + --server_logs_file=/dev/null \ + --query="select * from system.query_log where current_database = currentDatabase() AND event_date >= yesterday() and exception like '%TOPSECRET%'" + +echo '7.2' + +# not perfect: when run in parallel with other tests that check can give false-negative result +# because other tests can overwrite the last_error_message, where we check the absence of sensitive data. +# But it's still good enough for CI - in case of regressions it will start flapping (normally it shouldn't) +$CLICKHOUSE_CLIENT \ + --server_logs_file=/dev/null \ + --query="select * from system.errors where last_error_message like '%TOPSECRET%';" + rm -f "$tmp_file" >/dev/null 2>&1 echo 8 From 8369998286c2c7061274dcd7b0ce6cb3b2d43f9c Mon Sep 17 00:00:00 2001 From: filimonov <1549571+filimonov@users.noreply.github.com> Date: Mon, 14 Nov 2022 22:06:22 +0100 Subject: [PATCH 13/65] Style --- src/Common/Exception.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Common/Exception.h b/src/Common/Exception.h index 458fc0aa19a..169479a7918 100644 --- a/src/Common/Exception.h +++ b/src/Common/Exception.h @@ -29,7 +29,8 @@ public: Exception() = default; // used to remove the sensitive information from exceptions if query_masking_rules is configured - struct MessageMasked { + struct MessageMasked + { std::string msg; MessageMasked(const std::string & msg_); }; From ec6698395e127502e3b5c22798f95b1b12fd852d Mon Sep 17 00:00:00 2001 From: xiedeyantu Date: Tue, 15 Nov 2022 13:25:15 +0800 Subject: [PATCH 14/65] fix skip_unavailable_shards does not work using hdfsCluster table function --- src/Storages/HDFS/StorageHDFSCluster.cpp | 38 ++++++++----------- .../test_storage_hdfs/configs/cluster.xml | 18 +++++++++ tests/integration/test_storage_hdfs/test.py | 33 +++++++++++++++- 3 files changed, 65 insertions(+), 24 deletions(-) create mode 100644 tests/integration/test_storage_hdfs/configs/cluster.xml diff --git a/src/Storages/HDFS/StorageHDFSCluster.cpp b/src/Storages/HDFS/StorageHDFSCluster.cpp index 5f9d5ea3d6d..cdea8749fac 100644 --- a/src/Storages/HDFS/StorageHDFSCluster.cpp +++ b/src/Storages/HDFS/StorageHDFSCluster.cpp @@ -99,32 +99,24 @@ Pipe StorageHDFSCluster::read( addColumnsStructureToQueryWithClusterEngine( query_to_send, StorageDictionary::generateNamesAndTypesDescription(storage_snapshot->metadata->getColumns().getAll()), 3, getName()); - for (const auto & replicas : cluster->getShardsAddresses()) + const auto & current_settings = context->getSettingsRef(); + auto timeouts = ConnectionTimeouts::getTCPTimeoutsWithFailover(current_settings); + for (const auto & shard_info : cluster->getShardsInfo()) { - /// There will be only one replica, because we consider each replica as a shard - for (const auto & node : replicas) + auto try_results = shard_info.pool->getMany(timeouts, ¤t_settings, PoolMode::GET_MANY); + for (auto & try_result : try_results) { - auto connection = std::make_shared( - node.host_name, node.port, context->getGlobalContext()->getCurrentDatabase(), - node.user, node.password, node.quota_key, node.cluster, node.cluster_secret, - "HDFSClusterInititiator", - node.compression, - node.secure - ); - - - /// For unknown reason global context is passed to IStorage::read() method - /// So, task_identifier is passed as constructor argument. It is more obvious. auto remote_query_executor = std::make_shared( - connection, - queryToString(query_to_send), - header, - context, - /*throttler=*/nullptr, - scalars, - Tables(), - processed_stage, - RemoteQueryExecutor::Extension{.task_iterator = callback}); + shard_info.pool, + std::vector{try_result}, + queryToString(query_to_send), + header, + context, + /*throttler=*/nullptr, + scalars, + Tables(), + processed_stage, + RemoteQueryExecutor::Extension{.task_iterator = callback}); pipes.emplace_back(std::make_shared(remote_query_executor, add_agg_info, false)); } diff --git a/tests/integration/test_storage_hdfs/configs/cluster.xml b/tests/integration/test_storage_hdfs/configs/cluster.xml new file mode 100644 index 00000000000..3d72462332e --- /dev/null +++ b/tests/integration/test_storage_hdfs/configs/cluster.xml @@ -0,0 +1,18 @@ + + + + + + node1 + 9000 + + + + + node1 + 19000 + + + + + \ No newline at end of file diff --git a/tests/integration/test_storage_hdfs/test.py b/tests/integration/test_storage_hdfs/test.py index 34243e4b58d..defc0d1fa1d 100644 --- a/tests/integration/test_storage_hdfs/test.py +++ b/tests/integration/test_storage_hdfs/test.py @@ -9,7 +9,7 @@ from pyhdfs import HdfsClient cluster = ClickHouseCluster(__file__) node1 = cluster.add_instance( "node1", - main_configs=["configs/macro.xml", "configs/schema_cache.xml"], + main_configs=["configs/macro.xml", "configs/schema_cache.xml", "configs/cluster.xml"], with_hdfs=True, ) @@ -783,6 +783,37 @@ def test_schema_inference_cache(started_cluster): check_cache_misses(node1, files, 4) +def test_test_hdfsCluster_skip_unavailable_shards(started_cluster): + node = started_cluster.instances["node1"] + result = node.query( + """ + SELECT count(*) FROM hdfsCluster( + 'cluster_non_existent_port', + 'hdfs://hdfs1:9000/test_hdfsCluster/file*', + 'TSV', + 'id UInt32') + SETTINGS skip_unavailable_shards = 1 + """ + ) + + assert result == "3\n" + + +def test_test_hdfsCluster_unskip_unavailable_shards(started_cluster): + node = started_cluster.instances["node1"] + error = node.query_and_get_error( + """ + SELECT count(*) FROM hdfsCluster( + 'cluster_non_existent_port', + 'hdfs://hdfs1:9000/test_hdfsCluster/file*', + 'TSV', + 'id UInt32') + """ + ) + + assert "NETWORK_ERROR" in error + + if __name__ == "__main__": cluster.start() input("Cluster created, press any key to destroy...") From 09c749fbd0842e4dd54500cdaaf9c3f3e9d6d469 Mon Sep 17 00:00:00 2001 From: xiedeyantu Date: Tue, 15 Nov 2022 13:30:29 +0800 Subject: [PATCH 15/65] update --- tests/integration/test_storage_hdfs/configs/cluster.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_storage_hdfs/configs/cluster.xml b/tests/integration/test_storage_hdfs/configs/cluster.xml index 3d72462332e..9efe0ebf273 100644 --- a/tests/integration/test_storage_hdfs/configs/cluster.xml +++ b/tests/integration/test_storage_hdfs/configs/cluster.xml @@ -15,4 +15,4 @@ - \ No newline at end of file + From 3d218795beddd01d9f57561c23b46a18ad6c2f14 Mon Sep 17 00:00:00 2001 From: xiedeyantu Date: Wed, 16 Nov 2022 09:18:45 +0800 Subject: [PATCH 16/65] fix --- tests/integration/test_storage_hdfs/test.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_storage_hdfs/test.py b/tests/integration/test_storage_hdfs/test.py index defc0d1fa1d..a0a2d3f8426 100644 --- a/tests/integration/test_storage_hdfs/test.py +++ b/tests/integration/test_storage_hdfs/test.py @@ -9,7 +9,10 @@ from pyhdfs import HdfsClient cluster = ClickHouseCluster(__file__) node1 = cluster.add_instance( "node1", - main_configs=["configs/macro.xml", "configs/schema_cache.xml", "configs/cluster.xml"], + main_configs=[ + "configs/macro.xml", + "configs/schema_cache.xml", + "configs/cluster.xml"], with_hdfs=True, ) From 8144516e53cc2c12b71f02eacaa9fcd746811c66 Mon Sep 17 00:00:00 2001 From: xiedeyantu Date: Wed, 16 Nov 2022 09:33:46 +0800 Subject: [PATCH 17/65] fix --- tests/integration/test_storage_hdfs/test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_storage_hdfs/test.py b/tests/integration/test_storage_hdfs/test.py index a0a2d3f8426..745756476da 100644 --- a/tests/integration/test_storage_hdfs/test.py +++ b/tests/integration/test_storage_hdfs/test.py @@ -12,7 +12,8 @@ node1 = cluster.add_instance( main_configs=[ "configs/macro.xml", "configs/schema_cache.xml", - "configs/cluster.xml"], + "configs/cluster.xml" + ], with_hdfs=True, ) From c23cd091a36bda02522ad2f81ff09b624678aed0 Mon Sep 17 00:00:00 2001 From: xiedeyantu Date: Wed, 16 Nov 2022 09:59:44 +0800 Subject: [PATCH 18/65] fix --- tests/integration/test_storage_hdfs/test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/test_storage_hdfs/test.py b/tests/integration/test_storage_hdfs/test.py index 745756476da..4968deb7810 100644 --- a/tests/integration/test_storage_hdfs/test.py +++ b/tests/integration/test_storage_hdfs/test.py @@ -10,9 +10,9 @@ cluster = ClickHouseCluster(__file__) node1 = cluster.add_instance( "node1", main_configs=[ - "configs/macro.xml", - "configs/schema_cache.xml", - "configs/cluster.xml" + "configs/macro.xml", + "configs/schema_cache.xml", + "configs/cluster.xml", ], with_hdfs=True, ) From 7dc941cacd13948180dc448f7e3440199a77a93a Mon Sep 17 00:00:00 2001 From: xiedeyantu Date: Wed, 16 Nov 2022 19:47:57 +0800 Subject: [PATCH 19/65] fix --- tests/integration/test_storage_hdfs/test.py | 31 ++++++++------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/tests/integration/test_storage_hdfs/test.py b/tests/integration/test_storage_hdfs/test.py index 4968deb7810..4b174a42943 100644 --- a/tests/integration/test_storage_hdfs/test.py +++ b/tests/integration/test_storage_hdfs/test.py @@ -787,32 +787,23 @@ def test_schema_inference_cache(started_cluster): check_cache_misses(node1, files, 4) -def test_test_hdfsCluster_skip_unavailable_shards(started_cluster): +def test_hdfsCluster_skip_unavailable_shards(started_cluster): node = started_cluster.instances["node1"] - result = node.query( - """ - SELECT count(*) FROM hdfsCluster( - 'cluster_non_existent_port', - 'hdfs://hdfs1:9000/test_hdfsCluster/file*', - 'TSV', - 'id UInt32') - SETTINGS skip_unavailable_shards = 1 - """ + data = "1\tSerialize\t555.222\n2\tData\t777.333\n" + hdfs_api.write_data("/simple_table_function", data) + + assert ( + node1.query( + "select * from hdfs('hdfs://hdfs1:9000/simple_table_function', 'TSV', 'id UInt64, text String, number Float64') settings skip_unavailable_shards = 1" + ) + == data ) - assert result == "3\n" - -def test_test_hdfsCluster_unskip_unavailable_shards(started_cluster): +def test_hdfsCluster_unskip_unavailable_shards(started_cluster): node = started_cluster.instances["node1"] error = node.query_and_get_error( - """ - SELECT count(*) FROM hdfsCluster( - 'cluster_non_existent_port', - 'hdfs://hdfs1:9000/test_hdfsCluster/file*', - 'TSV', - 'id UInt32') - """ + "select * from hdfs('hdfs://hdfs1:9000/simple_table_function', 'TSV', 'id UInt64, text String, number Float64')" ) assert "NETWORK_ERROR" in error From 45a611bc1d78222013850ceb02cf356303eeed02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Wed, 16 Nov 2022 13:28:04 +0100 Subject: [PATCH 20/65] Fix style with black --- .../integration/test_backward_compatibility/test_functions.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/test_backward_compatibility/test_functions.py b/tests/integration/test_backward_compatibility/test_functions.py index 304d25bc8ac..afb19901e74 100644 --- a/tests/integration/test_backward_compatibility/test_functions.py +++ b/tests/integration/test_backward_compatibility/test_functions.py @@ -143,10 +143,9 @@ def test_string_functions(start_cluster): "substring", "CAST", # NOTE: no need to ignore now()/now64() since they will fail because they don't accept any argument - # 22.8 Backward Incompatible Change: Extended range of Date32 "toDate32OrZero", - "toDate32OrDefault" + "toDate32OrDefault", ] functions = filter(lambda x: x not in excludes, functions) From 78d4a4b5c2a4d8471975c0591d518c5f6d5f9acf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Wed, 16 Nov 2022 13:59:37 +0100 Subject: [PATCH 21/65] Add a detailed comment about the change --- .../AggregateFunctionMinMaxAny.h | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h index 4e519b8e878..024112dd65f 100644 --- a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h +++ b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h @@ -485,6 +485,8 @@ public: void insertResultInto(IColumn & to) const { + /// Same comment from write() applies here, but in this case we remove the last NULL character as we expect it to come from + /// old statuses that always included it if (has()) { const char * data = getData(); @@ -501,6 +503,24 @@ public: void write(WriteBuffer & buf, const ISerialization & /*serialization*/) const { + /// This used to be serialized using insertDataWithTerminatingZero up until 22.9 when it was changed to use insertData + /// The problem is that this change, inadvertently, changed the serialization of the state, as insertDataWithTerminatingZero + /// included the final NULL in its size count, while insertData doesn't. That is a string like "1234" would have been inserted + /// as { size: 5, data: "1234\0" }, but with the change it was inserted as { size: 4, data: "1234\0" }- + /// This changed in the stored size means that values created prior to this change, when read in an updated server, would + /// consider the additional \0 as being part of the string; and that if you went backwards (downgrade or replication to an older + /// server), newer values would remove the ending character of the string. + /// + /// To revert this change, the serialization has been changed yet again to replicate the old behaviour with one important + /// caveat: As we can read both statuses with or without the change, we only add the extra +1 if we need it, as we decide if we + /// need it based on whether the string is ended with a NULL (always on old statuses) or not (most of the time in newer statuses). + /// The main problem with this approach is that there is a chance of newer status containing a NULL ending character, as it might + /// have been part of the inserted string "naturally" (the data was inserted with a NULL) + /// + /// This should be taken as a patch applied to be as compatible as possible with older releases (with the NULL caveat), that is + /// to be able to work with data generated by any prior release, with or without the intended change. + /// In the future a new serialization **version** should be introduced replacing the current one in a backwards compatible manner. + /// Refs: https://github.com/ClickHouse/ClickHouse/pull/43038 and https://github.com/ClickHouse/ClickHouse/issues/42916 if (!has()) { writeBinary(Int32{1}, buf); From 589648c444395d1ddc7afb7230928d08195a5f0f Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Wed, 16 Nov 2022 15:47:32 +0100 Subject: [PATCH 22/65] Event data for 'labeled' has an added label data --- tests/ci/cancel_and_rerun_workflow_lambda/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ci/cancel_and_rerun_workflow_lambda/app.py b/tests/ci/cancel_and_rerun_workflow_lambda/app.py index 21a5ce517f6..f168c487492 100644 --- a/tests/ci/cancel_and_rerun_workflow_lambda/app.py +++ b/tests/ci/cancel_and_rerun_workflow_lambda/app.py @@ -308,7 +308,7 @@ def main(event): urls_to_cancel.append(workflow_description.cancel_url) print(f"Found {len(urls_to_cancel)} workflows to cancel") exec_workflow_url(urls_to_cancel, token) - elif action == "labeled" and "can be tested" in labels: + elif action == "labeled" and event_data["label"]["name"] == "can be tested": print("PR marked with can be tested label, rerun workflow") workflow_descriptions = get_workflows_description_for_pull_request(pull_request) workflow_descriptions = ( From cb069d8bfa1083eea2b57043edbba095779f5e1a Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Wed, 16 Nov 2022 15:50:26 +0100 Subject: [PATCH 23/65] Use authorized requests for GET --- .../cancel_and_rerun_workflow_lambda/app.py | 50 +++++++++++++------ 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/tests/ci/cancel_and_rerun_workflow_lambda/app.py b/tests/ci/cancel_and_rerun_workflow_lambda/app.py index f168c487492..01044b750f5 100644 --- a/tests/ci/cancel_and_rerun_workflow_lambda/app.py +++ b/tests/ci/cancel_and_rerun_workflow_lambda/app.py @@ -29,16 +29,19 @@ DEBUG_INFO = {} # type: Dict[str, Any] class Worker(Thread): - def __init__(self, request_queue: Queue, ignore_exception: bool = False): + def __init__( + self, request_queue: Queue, token: str, ignore_exception: bool = False + ): Thread.__init__(self) self.queue = request_queue + self.token = token self.ignore_exception = ignore_exception self.response = {} # type: Dict def run(self): m = self.queue.get() try: - self.response = _exec_get_with_retry(m) + self.response = _exec_get_with_retry(m, self.token) except Exception as e: if not self.ignore_exception: raise @@ -98,10 +101,11 @@ def get_token_from_aws(): return get_access_token(encoded_jwt, installation_id) -def _exec_get_with_retry(url): +def _exec_get_with_retry(url: str, token: str) -> dict: + headers = {"Authorization": f"token {token}"} for i in range(MAX_RETRY): try: - response = requests.get(url) + response = requests.get(url, headers=headers) response.raise_for_status() return response.json() except Exception as ex: @@ -119,6 +123,7 @@ WorkflowDescription = namedtuple( def get_workflows_description_for_pull_request( pull_request_event, + token, ) -> List[WorkflowDescription]: head_repo = pull_request_event["head"]["repo"]["full_name"] head_branch = pull_request_event["head"]["ref"] @@ -129,7 +134,7 @@ def get_workflows_description_for_pull_request( # Get all workflows for the current branch for i in range(1, 11): workflows = _exec_get_with_retry( - f"{request_url}&event=pull_request&branch={head_branch}&page={i}" + f"{request_url}&event=pull_request&branch={head_branch}&page={i}", token ) if not workflows["workflow_runs"]: break @@ -176,7 +181,9 @@ def get_workflows_description_for_pull_request( return workflow_descriptions -def get_workflow_description_fallback(pull_request_event) -> List[WorkflowDescription]: +def get_workflow_description_fallback( + pull_request_event, token +) -> List[WorkflowDescription]: head_repo = pull_request_event["head"]["repo"]["full_name"] head_branch = pull_request_event["head"]["ref"] print("Get last 500 workflows from API to search related there") @@ -188,7 +195,7 @@ def get_workflow_description_fallback(pull_request_event) -> List[WorkflowDescri i = 1 for i in range(1, 6): q.put(f"{request_url}&page={i}") - worker = Worker(q, True) + worker = Worker(q, token, True) worker.start() workers.append(worker) @@ -233,8 +240,8 @@ def get_workflow_description_fallback(pull_request_event) -> List[WorkflowDescri return workflow_descriptions -def get_workflow_description(workflow_id) -> WorkflowDescription: - workflow = _exec_get_with_retry(API_URL + f"/actions/runs/{workflow_id}") +def get_workflow_description(workflow_id, token) -> WorkflowDescription: + workflow = _exec_get_with_retry(API_URL + f"/actions/runs/{workflow_id}", token) return WorkflowDescription( run_id=workflow["id"], head_sha=workflow["head_sha"], @@ -279,9 +286,12 @@ def main(event): print("PR has labels", labels) if action == "closed" or "do not test" in labels: print("PR merged/closed or manually labeled 'do not test' will kill workflows") - workflow_descriptions = get_workflows_description_for_pull_request(pull_request) + workflow_descriptions = get_workflows_description_for_pull_request( + pull_request, token + ) workflow_descriptions = ( - workflow_descriptions or get_workflow_description_fallback(pull_request) + workflow_descriptions + or get_workflow_description_fallback(pull_request, token) ) urls_to_cancel = [] for workflow_description in workflow_descriptions: @@ -294,9 +304,12 @@ def main(event): exec_workflow_url(urls_to_cancel, token) elif action == "synchronize": print("PR is synchronized, going to stop old actions") - workflow_descriptions = get_workflows_description_for_pull_request(pull_request) + workflow_descriptions = get_workflows_description_for_pull_request( + pull_request, token + ) workflow_descriptions = ( - workflow_descriptions or get_workflow_description_fallback(pull_request) + workflow_descriptions + or get_workflow_description_fallback(pull_request, token) ) urls_to_cancel = [] for workflow_description in workflow_descriptions: @@ -310,9 +323,12 @@ def main(event): exec_workflow_url(urls_to_cancel, token) elif action == "labeled" and event_data["label"]["name"] == "can be tested": print("PR marked with can be tested label, rerun workflow") - workflow_descriptions = get_workflows_description_for_pull_request(pull_request) + workflow_descriptions = get_workflows_description_for_pull_request( + pull_request, token + ) workflow_descriptions = ( - workflow_descriptions or get_workflow_description_fallback(pull_request) + workflow_descriptions + or get_workflow_description_fallback(pull_request, token) ) if not workflow_descriptions: print("Not found any workflows") @@ -330,7 +346,9 @@ def main(event): print("Cancelled") for _ in range(45): - latest_workflow_desc = get_workflow_description(most_recent_workflow.run_id) + latest_workflow_desc = get_workflow_description( + most_recent_workflow.run_id, token + ) print("Checking latest workflow", latest_workflow_desc) if latest_workflow_desc.status in ("completed", "cancelled"): print("Finally latest workflow done, going to rerun") From fc1d06f096db5f6374bed2a728ef99fd5e4d5d2f Mon Sep 17 00:00:00 2001 From: xiedeyantu Date: Wed, 16 Nov 2022 23:43:23 +0800 Subject: [PATCH 24/65] fix --- tests/integration/test_storage_hdfs/test.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/integration/test_storage_hdfs/test.py b/tests/integration/test_storage_hdfs/test.py index 4b174a42943..319f8b1f86c 100644 --- a/tests/integration/test_storage_hdfs/test.py +++ b/tests/integration/test_storage_hdfs/test.py @@ -788,22 +788,26 @@ def test_schema_inference_cache(started_cluster): def test_hdfsCluster_skip_unavailable_shards(started_cluster): + hdfs_api = started_cluster.hdfs_api node = started_cluster.instances["node1"] data = "1\tSerialize\t555.222\n2\tData\t777.333\n" - hdfs_api.write_data("/simple_table_function", data) + hdfs_api.write_data("/skip_unavailable_shards", data) assert ( node1.query( - "select * from hdfs('hdfs://hdfs1:9000/simple_table_function', 'TSV', 'id UInt64, text String, number Float64') settings skip_unavailable_shards = 1" + "select * from hdfs('hdfs://hdfs1:9000/skip_unavailable_shards', 'TSV', 'id UInt64, text String, number Float64') settings skip_unavailable_shards = 1" ) == data ) def test_hdfsCluster_unskip_unavailable_shards(started_cluster): + hdfs_api = started_cluster.hdfs_api node = started_cluster.instances["node1"] + data = "1\tSerialize\t555.222\n2\tData\t777.333\n" + hdfs_api.write_data("/unskip_unavailable_shards", data) error = node.query_and_get_error( - "select * from hdfs('hdfs://hdfs1:9000/simple_table_function', 'TSV', 'id UInt64, text String, number Float64')" + "select * from hdfs('hdfs://hdfs1:9000/unskip_unavailable_shards', 'TSV', 'id UInt64, text String, number Float64')" ) assert "NETWORK_ERROR" in error From cb8333758661fba19d2d67c2c0d8ee1290b4dd72 Mon Sep 17 00:00:00 2001 From: chen Date: Thu, 17 Nov 2022 06:41:43 +0800 Subject: [PATCH 25/65] Update test.py --- tests/integration/test_storage_hdfs/test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_storage_hdfs/test.py b/tests/integration/test_storage_hdfs/test.py index 319f8b1f86c..d4752d6cf2e 100644 --- a/tests/integration/test_storage_hdfs/test.py +++ b/tests/integration/test_storage_hdfs/test.py @@ -795,7 +795,7 @@ def test_hdfsCluster_skip_unavailable_shards(started_cluster): assert ( node1.query( - "select * from hdfs('hdfs://hdfs1:9000/skip_unavailable_shards', 'TSV', 'id UInt64, text String, number Float64') settings skip_unavailable_shards = 1" + "select * from hdfsCluster('cluster_non_existent_port', 'hdfs://hdfs1:9000/skip_unavailable_shards', 'TSV', 'id UInt64, text String, number Float64') settings skip_unavailable_shards = 1" ) == data ) @@ -807,7 +807,7 @@ def test_hdfsCluster_unskip_unavailable_shards(started_cluster): data = "1\tSerialize\t555.222\n2\tData\t777.333\n" hdfs_api.write_data("/unskip_unavailable_shards", data) error = node.query_and_get_error( - "select * from hdfs('hdfs://hdfs1:9000/unskip_unavailable_shards', 'TSV', 'id UInt64, text String, number Float64')" + "select * from hdfsCluster('cluster_non_existent_port', 'hdfs://hdfs1:9000/unskip_unavailable_shards', 'TSV', 'id UInt64, text String, number Float64')" ) assert "NETWORK_ERROR" in error From d5848d53cc06419539b7e1d622ed7287268d4013 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Thu, 17 Nov 2022 11:46:17 +0000 Subject: [PATCH 26/65] Fix DESCRIBE for deltaLake and hudi table functions --- src/Storages/StorageDelta.cpp | 84 +++++-- src/Storages/StorageDelta.h | 18 +- src/Storages/StorageHudi.cpp | 247 +++++++++++-------- src/Storages/StorageHudi.h | 13 +- src/TableFunctions/TableFunctionDelta.cpp | 2 +- src/TableFunctions/TableFunctionHudi.cpp | 2 +- tests/integration/test_storage_delta/test.py | 23 +- tests/integration/test_storage_hudi/test.py | 29 ++- 8 files changed, 265 insertions(+), 153 deletions(-) diff --git a/src/Storages/StorageDelta.cpp b/src/Storages/StorageDelta.cpp index e8287a2fd61..739c001fb81 100644 --- a/src/Storages/StorageDelta.cpp +++ b/src/Storages/StorageDelta.cpp @@ -47,7 +47,7 @@ void DeltaLakeMetadata::remove(const String & filename, uint64_t /*timestamp */) throw Exception(ErrorCodes::INCORRECT_DATA, "Invalid table metadata, tried to remove {} before adding it", filename); } -std::vector DeltaLakeMetadata::ListCurrentFiles() && +std::vector DeltaLakeMetadata::listCurrentFiles() && { std::vector keys; keys.reserve(file_update_time.size()); @@ -61,10 +61,10 @@ std::vector DeltaLakeMetadata::ListCurrentFiles() && JsonMetadataGetter::JsonMetadataGetter(StorageS3::S3Configuration & configuration_, const String & table_path_, ContextPtr context) : base_configuration(configuration_), table_path(table_path_) { - Init(context); + init(context); } -void JsonMetadataGetter::Init(ContextPtr context) +void JsonMetadataGetter::init(ContextPtr context) { auto keys = getJsonLogFiles(); @@ -178,6 +178,52 @@ void JsonMetadataGetter::handleJSON(const JSON & json) } } +namespace +{ + +StorageS3::S3Configuration getBaseConfiguration(const StorageS3Configuration & configuration) +{ + return {configuration.url, configuration.auth_settings, configuration.rw_settings, configuration.headers}; +} + +// DeltaLake stores data in parts in different files +// keys is vector of parts with latest version +// generateQueryFromKeys constructs query from parts filenames for +// underlying StorageS3 engine +String generateQueryFromKeys(const std::vector & keys) +{ + std::string new_query = fmt::format("{{{}}}", fmt::join(keys, ",")); + return new_query; +} + + +StorageS3Configuration getAdjustedS3Configuration( + const ContextPtr & context, + StorageS3::S3Configuration & base_configuration, + const StorageS3Configuration & configuration, + const std::string & table_path, + Poco::Logger * log) +{ + JsonMetadataGetter getter{base_configuration, table_path, context}; + + auto keys = getter.getFiles(); + auto new_uri = base_configuration.uri.uri.toString() + generateQueryFromKeys(keys); + + LOG_DEBUG(log, "New uri: {}", new_uri); + LOG_DEBUG(log, "Table path: {}", table_path); + + // set new url in configuration + StorageS3Configuration new_configuration; + new_configuration.url = new_uri; + new_configuration.auth_settings.access_key_id = configuration.auth_settings.access_key_id; + new_configuration.auth_settings.secret_access_key = configuration.auth_settings.secret_access_key; + new_configuration.format = configuration.format; + + return new_configuration; +} + +} + StorageDelta::StorageDelta( const StorageS3Configuration & configuration_, const StorageID & table_id_, @@ -187,28 +233,14 @@ StorageDelta::StorageDelta( ContextPtr context_, std::optional format_settings_) : IStorage(table_id_) - , base_configuration{configuration_.url, configuration_.auth_settings, configuration_.rw_settings, configuration_.headers} + , base_configuration{getBaseConfiguration(configuration_)} , log(&Poco::Logger::get("StorageDeltaLake (" + table_id_.table_name + ")")) , table_path(base_configuration.uri.key) { StorageInMemoryMetadata storage_metadata; StorageS3::updateS3Configuration(context_, base_configuration); - JsonMetadataGetter getter{base_configuration, table_path, context_}; - - auto keys = getter.getFiles(); - auto new_uri = base_configuration.uri.uri.toString() + generateQueryFromKeys(std::move(keys)); - - LOG_DEBUG(log, "New uri: {}", new_uri); - LOG_DEBUG(log, "Table path: {}", table_path); - - // set new url in configuration - StorageS3Configuration new_configuration; - new_configuration.url = new_uri; - new_configuration.auth_settings.access_key_id = configuration_.auth_settings.access_key_id; - new_configuration.auth_settings.secret_access_key = configuration_.auth_settings.secret_access_key; - new_configuration.format = configuration_.format; - + auto new_configuration = getAdjustedS3Configuration(context_, base_configuration, configuration_, table_path, log); if (columns_.empty()) { @@ -250,13 +282,15 @@ Pipe StorageDelta::read( return s3engine->read(column_names, storage_snapshot, query_info, context, processed_stage, max_block_size, num_streams); } -String StorageDelta::generateQueryFromKeys(std::vector && keys) +ColumnsDescription StorageDelta::getTableStructureFromData( + const StorageS3Configuration & configuration, const std::optional & format_settings, ContextPtr ctx) { - // DeltaLake store data parts in different files - // keys are filenames of parts - // for StorageS3 to read all parts we need format {key1,key2,key3,...keyn} - std::string new_query = fmt::format("{{{}}}", fmt::join(keys, ",")); - return new_query; + auto base_configuration = getBaseConfiguration(configuration); + StorageS3::updateS3Configuration(ctx, base_configuration); + auto new_configuration = getAdjustedS3Configuration( + ctx, base_configuration, configuration, base_configuration.uri.key, &Poco::Logger::get("StorageDeltaLake")); + return StorageS3::getTableStructureFromData( + new_configuration, /*distributed processing*/ false, format_settings, ctx, /*object_infos*/ nullptr); } void registerStorageDelta(StorageFactory & factory) diff --git a/src/Storages/StorageDelta.h b/src/Storages/StorageDelta.h index e3bb4c0b416..22c95d87a0f 100644 --- a/src/Storages/StorageDelta.h +++ b/src/Storages/StorageDelta.h @@ -32,7 +32,7 @@ public: void setLastModifiedTime(const String & filename, uint64_t timestamp); void remove(const String & filename, uint64_t timestamp); - std::vector ListCurrentFiles() &&; + std::vector listCurrentFiles() &&; private: std::unordered_map file_update_time; @@ -44,10 +44,10 @@ class JsonMetadataGetter public: JsonMetadataGetter(StorageS3::S3Configuration & configuration_, const String & table_path_, ContextPtr context); - std::vector getFiles() { return std::move(metadata).ListCurrentFiles(); } + std::vector getFiles() { return std::move(metadata).listCurrentFiles(); } private: - void Init(ContextPtr context); + void init(ContextPtr context); std::vector getJsonLogFiles(); @@ -87,14 +87,12 @@ public: size_t max_block_size, size_t num_streams) override; + static ColumnsDescription getTableStructureFromData( + const StorageS3Configuration & configuration, + const std::optional & format_settings, + ContextPtr ctx); private: - void Init(); - - // DeltaLake stores data in parts in different files - // keys is vector of parts with latest version - // generateQueryFromKeys constructs query from parts filenames for - // underlying StorageS3 engine - static String generateQueryFromKeys(std::vector && keys); + void init(); StorageS3::S3Configuration base_configuration; std::shared_ptr s3engine; diff --git a/src/Storages/StorageHudi.cpp b/src/Storages/StorageHudi.cpp index 121856c4a57..be09f608c3b 100644 --- a/src/Storages/StorageHudi.cpp +++ b/src/Storages/StorageHudi.cpp @@ -28,115 +28,20 @@ namespace ErrorCodes extern const int LOGICAL_ERROR; } -StorageHudi::StorageHudi( - const StorageS3Configuration & configuration_, - const StorageID & table_id_, - ColumnsDescription columns_, - const ConstraintsDescription & constraints_, - const String & comment, - ContextPtr context_, - std::optional format_settings_) - : IStorage(table_id_) - , base_configuration{configuration_.url, configuration_.auth_settings, configuration_.rw_settings, configuration_.headers} - , log(&Poco::Logger::get("StorageHudi (" + table_id_.table_name + ")")) - , table_path(base_configuration.uri.key) +namespace { - StorageInMemoryMetadata storage_metadata; - StorageS3::updateS3Configuration(context_, base_configuration); - auto keys = getKeysFromS3(); - auto new_uri = base_configuration.uri.uri.toString() + generateQueryFromKeys(keys, configuration_.format); - - LOG_DEBUG(log, "New uri: {}", new_uri); - LOG_DEBUG(log, "Table path: {}", table_path); - - StorageS3Configuration new_configuration; - new_configuration.url = new_uri; - new_configuration.auth_settings.access_key_id = configuration_.auth_settings.access_key_id; - new_configuration.auth_settings.secret_access_key = configuration_.auth_settings.secret_access_key; - new_configuration.format = configuration_.format; - - if (columns_.empty()) - { - columns_ = StorageS3::getTableStructureFromData( - new_configuration, /*distributed processing*/ false, format_settings_, context_, nullptr); - storage_metadata.setColumns(columns_); - } - else - storage_metadata.setColumns(columns_); - - storage_metadata.setConstraints(constraints_); - storage_metadata.setComment(comment); - setInMemoryMetadata(storage_metadata); - - s3engine = std::make_shared( - new_configuration, - table_id_, - columns_, - constraints_, - comment, - context_, - format_settings_, - /* distributed_processing_ */ false, - nullptr); +StorageS3::S3Configuration getBaseConfiguration(const StorageS3Configuration & configuration) +{ + return {configuration.url, configuration.auth_settings, configuration.rw_settings, configuration.headers}; } -Pipe StorageHudi::read( - const Names & column_names, - const StorageSnapshotPtr & storage_snapshot, - SelectQueryInfo & query_info, - ContextPtr context, - QueryProcessingStage::Enum processed_stage, - size_t max_block_size, - size_t num_streams) -{ - StorageS3::updateS3Configuration(context, base_configuration); - return s3engine->read(column_names, storage_snapshot, query_info, context, processed_stage, max_block_size, num_streams); -} - -std::vector StorageHudi::getKeysFromS3() -{ - std::vector keys; - - const auto & client = base_configuration.client; - - Aws::S3::Model::ListObjectsV2Request request; - Aws::S3::Model::ListObjectsV2Outcome outcome; - - bool is_finished{false}; - const auto bucket{base_configuration.uri.bucket}; - - request.SetBucket(bucket); - request.SetPrefix(table_path); - - while (!is_finished) - { - outcome = client->ListObjectsV2(request); - if (!outcome.IsSuccess()) - throw Exception( - ErrorCodes::S3_ERROR, - "Could not list objects in bucket {} with key {}, S3 exception: {}, message: {}", - quoteString(bucket), - quoteString(table_path), - backQuote(outcome.GetError().GetExceptionName()), - quoteString(outcome.GetError().GetMessage())); - - const auto & result_batch = outcome.GetResult().GetContents(); - for (const auto & obj : result_batch) - { - const auto & filename = obj.GetKey().substr(table_path.size()); /// Object name without tablepath prefix. - keys.push_back(filename); - LOG_DEBUG(log, "Found file: {}", filename); - } - - request.SetContinuationToken(outcome.GetResult().GetNextContinuationToken()); - is_finished = !outcome.GetResult().GetIsTruncated(); - } - - return keys; -} - -String StorageHudi::generateQueryFromKeys(const std::vector & keys, const String & format) +/// Apache Hudi store parts of data in different files. +/// Every part file has timestamp in it. +/// Every partition(directory) in Apache Hudi has different versions of part. +/// To find needed parts we need to find out latest part file for every partition. +/// Part format is usually parquet, but can differ. +String generateQueryFromKeys(const std::vector & keys, const String & format) { /// For each partition path take only latest file. struct FileInfo @@ -187,6 +92,138 @@ String StorageHudi::generateQueryFromKeys(const std::vector & keys, return "{" + list_of_keys + "}"; } +std::vector getKeysFromS3(const StorageS3::S3Configuration & base_configuration, const std::string & table_path, Poco::Logger * log) +{ + std::vector keys; + + const auto & client = base_configuration.client; + + Aws::S3::Model::ListObjectsV2Request request; + Aws::S3::Model::ListObjectsV2Outcome outcome; + + bool is_finished{false}; + const auto bucket{base_configuration.uri.bucket}; + + request.SetBucket(bucket); + request.SetPrefix(table_path); + + while (!is_finished) + { + outcome = client->ListObjectsV2(request); + if (!outcome.IsSuccess()) + throw Exception( + ErrorCodes::S3_ERROR, + "Could not list objects in bucket {} with key {}, S3 exception: {}, message: {}", + quoteString(bucket), + quoteString(table_path), + backQuote(outcome.GetError().GetExceptionName()), + quoteString(outcome.GetError().GetMessage())); + + const auto & result_batch = outcome.GetResult().GetContents(); + for (const auto & obj : result_batch) + { + const auto & filename = obj.GetKey().substr(table_path.size()); /// Object name without tablepath prefix. + keys.push_back(filename); + LOG_DEBUG(log, "Found file: {}", filename); + } + + request.SetContinuationToken(outcome.GetResult().GetNextContinuationToken()); + is_finished = !outcome.GetResult().GetIsTruncated(); + } + + return keys; +} + + +StorageS3Configuration getAdjustedS3Configuration( + StorageS3::S3Configuration & base_configuration, + const StorageS3Configuration & configuration, + const std::string & table_path, + Poco::Logger * log) +{ + auto keys = getKeysFromS3(base_configuration, table_path, log); + auto new_uri = base_configuration.uri.uri.toString() + generateQueryFromKeys(keys, configuration.format); + + LOG_DEBUG(log, "New uri: {}", new_uri); + LOG_DEBUG(log, "Table path: {}", table_path); + + StorageS3Configuration new_configuration; + new_configuration.url = new_uri; + new_configuration.auth_settings.access_key_id = configuration.auth_settings.access_key_id; + new_configuration.auth_settings.secret_access_key = configuration.auth_settings.secret_access_key; + new_configuration.format = configuration.format; + + return new_configuration; +} + +} + +StorageHudi::StorageHudi( + const StorageS3Configuration & configuration_, + const StorageID & table_id_, + ColumnsDescription columns_, + const ConstraintsDescription & constraints_, + const String & comment, + ContextPtr context_, + std::optional format_settings_) + : IStorage(table_id_) + , base_configuration{getBaseConfiguration(configuration_)} + , log(&Poco::Logger::get("StorageHudi (" + table_id_.table_name + ")")) + , table_path(base_configuration.uri.key) +{ + StorageInMemoryMetadata storage_metadata; + StorageS3::updateS3Configuration(context_, base_configuration); + + auto new_configuration = getAdjustedS3Configuration(base_configuration, configuration_, table_path, log); + + if (columns_.empty()) + { + columns_ = StorageS3::getTableStructureFromData( + new_configuration, /*distributed processing*/ false, format_settings_, context_, nullptr); + storage_metadata.setColumns(columns_); + } + else + storage_metadata.setColumns(columns_); + + storage_metadata.setConstraints(constraints_); + storage_metadata.setComment(comment); + setInMemoryMetadata(storage_metadata); + + s3engine = std::make_shared( + new_configuration, + table_id_, + columns_, + constraints_, + comment, + context_, + format_settings_, + /* distributed_processing_ */ false, + nullptr); +} + +Pipe StorageHudi::read( + const Names & column_names, + const StorageSnapshotPtr & storage_snapshot, + SelectQueryInfo & query_info, + ContextPtr context, + QueryProcessingStage::Enum processed_stage, + size_t max_block_size, + size_t num_streams) +{ + StorageS3::updateS3Configuration(context, base_configuration); + return s3engine->read(column_names, storage_snapshot, query_info, context, processed_stage, max_block_size, num_streams); +} + +ColumnsDescription StorageHudi::getTableStructureFromData( + const StorageS3Configuration & configuration, const std::optional & format_settings, ContextPtr ctx) +{ + auto base_configuration = getBaseConfiguration(configuration); + StorageS3::updateS3Configuration(ctx, base_configuration); + auto new_configuration = getAdjustedS3Configuration( + base_configuration, configuration, base_configuration.uri.key, &Poco::Logger::get("StorageDeltaLake")); + return StorageS3::getTableStructureFromData( + new_configuration, /*distributed processing*/ false, format_settings, ctx, /*object_infos*/ nullptr); +} void registerStorageHudi(StorageFactory & factory) { diff --git a/src/Storages/StorageHudi.h b/src/Storages/StorageHudi.h index bebda4cd4f6..00b8c01a46d 100644 --- a/src/Storages/StorageHudi.h +++ b/src/Storages/StorageHudi.h @@ -48,16 +48,11 @@ public: size_t max_block_size, size_t num_streams) override; + static ColumnsDescription getTableStructureFromData( + const StorageS3Configuration & configuration, + const std::optional & format_settings, + ContextPtr ctx); private: - std::vector getKeysFromS3(); - - /// Apache Hudi store parts of data in different files. - /// Every part file has timestamp in it. - /// Every partition(directory) in Apache Hudi has different versions of part. - /// To find needed parts we need to find out latest part file for every partition. - /// Part format is usually parquet, but can differ. - static String generateQueryFromKeys(const std::vector & keys, const String & format); - StorageS3::S3Configuration base_configuration; std::shared_ptr s3engine; Poco::Logger * log; diff --git a/src/TableFunctions/TableFunctionDelta.cpp b/src/TableFunctions/TableFunctionDelta.cpp index 25ea2aaa77f..df4ea75894d 100644 --- a/src/TableFunctions/TableFunctionDelta.cpp +++ b/src/TableFunctions/TableFunctionDelta.cpp @@ -130,7 +130,7 @@ ColumnsDescription TableFunctionDelta::getActualTableStructure(ContextPtr contex if (configuration.structure == "auto") { context->checkAccess(getSourceAccessType()); - return StorageS3::getTableStructureFromData(configuration, false, std::nullopt, context); + return StorageDelta::getTableStructureFromData(configuration, std::nullopt, context); } return parseColumnsListFromString(configuration.structure, context); diff --git a/src/TableFunctions/TableFunctionHudi.cpp b/src/TableFunctions/TableFunctionHudi.cpp index b1db90da550..c24c14b1b85 100644 --- a/src/TableFunctions/TableFunctionHudi.cpp +++ b/src/TableFunctions/TableFunctionHudi.cpp @@ -130,7 +130,7 @@ ColumnsDescription TableFunctionHudi::getActualTableStructure(ContextPtr context if (configuration.structure == "auto") { context->checkAccess(getSourceAccessType()); - return StorageS3::getTableStructureFromData(configuration, false, std::nullopt, context); + return StorageHudi::getTableStructureFromData(configuration, std::nullopt, context); } return parseColumnsListFromString(configuration.structure, context); diff --git a/tests/integration/test_storage_delta/test.py b/tests/integration/test_storage_delta/test.py index a63244df814..3f9da071281 100644 --- a/tests/integration/test_storage_delta/test.py +++ b/tests/integration/test_storage_delta/test.py @@ -1,7 +1,6 @@ import logging import os import json - import helpers.client import pytest from helpers.cluster import ClickHouseCluster @@ -143,3 +142,25 @@ def test_select_query(started_cluster): ), ).splitlines() assert len(result) > 0 + + +def test_describe_query(started_cluster): + instance = started_cluster.instances["main_server"] + bucket = started_cluster.minio_bucket + result = instance.query( + f"DESCRIBE deltaLake('http://{started_cluster.minio_ip}:{started_cluster.minio_port}/{bucket}/test_table/', 'minio', 'minio123') FORMAT TSV", + ) + + assert result == TSV( + [ + ["begin_lat", "Nullable(Float64)"], + ["begin_lon", "Nullable(Float64)"], + ["driver", "Nullable(String)"], + ["end_lat", "Nullable(Float64)"], + ["end_lon", "Nullable(Float64)"], + ["fare", "Nullable(Float64)"], + ["rider", "Nullable(String)"], + ["ts", "Nullable(Int64)"], + ["uuid", "Nullable(String)"], + ] + ) diff --git a/tests/integration/test_storage_hudi/test.py b/tests/integration/test_storage_hudi/test.py index dd870aae42e..7748ddf6160 100644 --- a/tests/integration/test_storage_hudi/test.py +++ b/tests/integration/test_storage_hudi/test.py @@ -161,7 +161,7 @@ def test_select_query(started_cluster): result = run_query(instance, distinct_select_query) result_table_function = run_query( instance, - distinct_select_query.format( + distinct_select_table_function_query.format( ip=started_cluster.minio_ip, port=started_cluster.minio_port, bucket=bucket ), ) @@ -173,3 +173,30 @@ def test_select_query(started_cluster): assert TSV(result) == TSV(expected) assert TSV(result_table_function) == TSV(expected) + +def test_describe_query(started_cluster): + instance = started_cluster.instances["main_server"] + bucket = started_cluster.minio_bucket + result = instance.query( + f"DESCRIBE hudi('http://{started_cluster.minio_ip}:{started_cluster.minio_port}/{bucket}/test_table/', 'minio', 'minio123') FORMAT TSV", + ) + + assert result == TSV( + [ + ["_hoodie_commit_time", "Nullable(String)"], + ["_hoodie_commit_seqno", "Nullable(String)"], + ["_hoodie_record_key", "Nullable(String)"], + ["_hoodie_partition_path", "Nullable(String)"], + ["_hoodie_file_name", "Nullable(String)"], + ["begin_lat", "Nullable(Float64)"], + ["begin_lon", "Nullable(Float64)"], + ["driver", "Nullable(String)"], + ["end_lat", "Nullable(Float64)"], + ["end_lon", "Nullable(Float64)"], + ["fare", "Nullable(Float64)"], + ["partitionpath", "Nullable(String)"], + ["rider", "Nullable(String)"], + ["ts", "Nullable(Int64)"], + ["uuid", "Nullable(String)"], + ] + ) From 1ad7362db76cfbe4a0576da7a72dfbb9d6ef75a4 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Thu, 17 Nov 2022 11:54:13 +0000 Subject: [PATCH 27/65] Automatic style fix --- tests/integration/test_storage_hudi/test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/test_storage_hudi/test.py b/tests/integration/test_storage_hudi/test.py index 7748ddf6160..3328f859406 100644 --- a/tests/integration/test_storage_hudi/test.py +++ b/tests/integration/test_storage_hudi/test.py @@ -174,6 +174,7 @@ def test_select_query(started_cluster): assert TSV(result) == TSV(expected) assert TSV(result_table_function) == TSV(expected) + def test_describe_query(started_cluster): instance = started_cluster.instances["main_server"] bucket = started_cluster.minio_bucket From 2e5e9a87295dbc6d8aec5a90273b83fcaa9c34f7 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Thu, 17 Nov 2022 13:31:41 +0100 Subject: [PATCH 28/65] Get rid of API_URL environment --- .../cancel_and_rerun_workflow_lambda/app.py | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/tests/ci/cancel_and_rerun_workflow_lambda/app.py b/tests/ci/cancel_and_rerun_workflow_lambda/app.py index 01044b750f5..47aec0609cb 100644 --- a/tests/ci/cancel_and_rerun_workflow_lambda/app.py +++ b/tests/ci/cancel_and_rerun_workflow_lambda/app.py @@ -19,10 +19,6 @@ NEED_RERUN_OR_CANCELL_WORKFLOWS = { "BackportPR", } -# https://docs.github.com/en/rest/reference/actions#cancel-a-workflow-run -# -API_URL = os.getenv("API_URL", "https://api.github.com/repos/ClickHouse/ClickHouse") - MAX_RETRY = 5 DEBUG_INFO = {} # type: Dict[str, Any] @@ -117,7 +113,7 @@ def _exec_get_with_retry(url: str, token: str) -> dict: WorkflowDescription = namedtuple( "WorkflowDescription", - ["run_id", "head_sha", "status", "rerun_url", "cancel_url", "conclusion"], + ["url", "run_id", "head_sha", "status", "rerun_url", "cancel_url", "conclusion"], ) @@ -130,7 +126,8 @@ def get_workflows_description_for_pull_request( print("PR", pull_request_event["number"], "has head ref", head_branch) workflows_data = [] - request_url = f"{API_URL}/actions/runs?per_page=100" + repo_url = pull_request_event["base"]["repo"]["url"] + request_url = f"{repo_url}/actions/runs?per_page=100" # Get all workflows for the current branch for i in range(1, 11): workflows = _exec_get_with_retry( @@ -169,6 +166,7 @@ def get_workflows_description_for_pull_request( ): workflow_descriptions.append( WorkflowDescription( + url=workflow["url"], run_id=workflow["id"], head_sha=workflow["head_sha"], status=workflow["status"], @@ -188,7 +186,8 @@ def get_workflow_description_fallback( head_branch = pull_request_event["head"]["ref"] print("Get last 500 workflows from API to search related there") # Fallback for a case of an already deleted branch and no workflows received - request_url = f"{API_URL}/actions/runs?per_page=100" + repo_url = pull_request_event["base"]["repo"]["url"] + request_url = f"{repo_url}/actions/runs?per_page=100" q = Queue() # type: Queue workers = [] workflows_data = [] @@ -227,6 +226,7 @@ def get_workflow_description_fallback( workflow_descriptions = [ WorkflowDescription( + url=wf["url"], run_id=wf["id"], head_sha=wf["head_sha"], status=wf["status"], @@ -240,9 +240,10 @@ def get_workflow_description_fallback( return workflow_descriptions -def get_workflow_description(workflow_id, token) -> WorkflowDescription: - workflow = _exec_get_with_retry(API_URL + f"/actions/runs/{workflow_id}", token) +def get_workflow_description(workflow_url, token) -> WorkflowDescription: + workflow = _exec_get_with_retry(workflow_url, token) return WorkflowDescription( + url=workflow["url"], run_id=workflow["id"], head_sha=workflow["head_sha"], status=workflow["status"], @@ -346,8 +347,9 @@ def main(event): print("Cancelled") for _ in range(45): + # If the number of retries is changed: tune the lambda limits accordingly latest_workflow_desc = get_workflow_description( - most_recent_workflow.run_id, token + most_recent_workflow.url, token ) print("Checking latest workflow", latest_workflow_desc) if latest_workflow_desc.status in ("completed", "cancelled"): From d2d6e75d97b75ebc05745d4e7e1bb53cb01d639c Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Thu, 17 Nov 2022 12:40:14 +0000 Subject: [PATCH 29/65] Fix: make test_read_only_table more stable + add retries to INSERT queries after keeper node restart --- tests/integration/test_read_only_table/test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_read_only_table/test.py b/tests/integration/test_read_only_table/test.py index 28abbf6601e..b97b8edb85e 100644 --- a/tests/integration/test_read_only_table/test.py +++ b/tests/integration/test_read_only_table/test.py @@ -84,6 +84,6 @@ def test_restart_zookeeper(start_cluster): time.sleep(5) for table_id in range(NUM_TABLES): - node1.query( - f"INSERT INTO test_table_{table_id} VALUES (6), (7), (8), (9), (10);" + node1.query_with_retry( + sql = f"INSERT INTO test_table_{table_id} VALUES (6), (7), (8), (9), (10);", retry_count=10, sleep_time=1 ) From 13e051a5de5ba50d5c3763d5d4e3c1bf312f37d6 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Thu, 17 Nov 2022 13:46:21 +0000 Subject: [PATCH 30/65] Automatic style fix --- tests/integration/test_read_only_table/test.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_read_only_table/test.py b/tests/integration/test_read_only_table/test.py index b97b8edb85e..914c6a99508 100644 --- a/tests/integration/test_read_only_table/test.py +++ b/tests/integration/test_read_only_table/test.py @@ -85,5 +85,7 @@ def test_restart_zookeeper(start_cluster): for table_id in range(NUM_TABLES): node1.query_with_retry( - sql = f"INSERT INTO test_table_{table_id} VALUES (6), (7), (8), (9), (10);", retry_count=10, sleep_time=1 + sql=f"INSERT INTO test_table_{table_id} VALUES (6), (7), (8), (9), (10);", + retry_count=10, + sleep_time=1, ) From 1fe095ae900aea07d088680a2f83d67bbdf07b00 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Thu, 17 Nov 2022 15:55:25 +0100 Subject: [PATCH 31/65] Explicitly return OK for python lambdas --- tests/ci/cancel_and_rerun_workflow_lambda/app.py | 6 ++++++ tests/ci/workflow_approve_rerun_lambda/app.py | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/tests/ci/cancel_and_rerun_workflow_lambda/app.py b/tests/ci/cancel_and_rerun_workflow_lambda/app.py index 47aec0609cb..321aa068598 100644 --- a/tests/ci/cancel_and_rerun_workflow_lambda/app.py +++ b/tests/ci/cancel_and_rerun_workflow_lambda/app.py @@ -367,6 +367,12 @@ def main(event): def handler(event, _): try: main(event) + + return { + "statusCode": 200, + "headers": {"Content-Type": "application/json"}, + "body": '{"status": "OK"}', + } finally: for name, value in DEBUG_INFO.items(): print(f"Value of {name}: ", value) diff --git a/tests/ci/workflow_approve_rerun_lambda/app.py b/tests/ci/workflow_approve_rerun_lambda/app.py index f2b785840d8..23e808b0861 100644 --- a/tests/ci/workflow_approve_rerun_lambda/app.py +++ b/tests/ci/workflow_approve_rerun_lambda/app.py @@ -491,6 +491,12 @@ def main(event): def handler(event, _): try: main(event) + + return { + "statusCode": 200, + "headers": {"Content-Type": "application/json"}, + "body": '{"status": "OK"}', + } except Exception: print("Received event: ", event) raise From 6d532d310dbe624bdecf76c1c7e6f7371098ac33 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Thu, 17 Nov 2022 15:55:48 +0100 Subject: [PATCH 32/65] Migrate cancel-lambda to python package --- .../ci/cancel_and_rerun_workflow_lambda/Dockerfile | 13 ------------- .../build_and_deploy_archive.sh | 1 + 2 files changed, 1 insertion(+), 13 deletions(-) delete mode 100644 tests/ci/cancel_and_rerun_workflow_lambda/Dockerfile create mode 120000 tests/ci/cancel_and_rerun_workflow_lambda/build_and_deploy_archive.sh diff --git a/tests/ci/cancel_and_rerun_workflow_lambda/Dockerfile b/tests/ci/cancel_and_rerun_workflow_lambda/Dockerfile deleted file mode 100644 index 0d50224c51d..00000000000 --- a/tests/ci/cancel_and_rerun_workflow_lambda/Dockerfile +++ /dev/null @@ -1,13 +0,0 @@ -FROM public.ecr.aws/lambda/python:3.9 - -# Install the function's dependencies using file requirements.txt -# from your project folder. - -COPY requirements.txt . -RUN pip3 install -r requirements.txt --target "${LAMBDA_TASK_ROOT}" - -# Copy function code -COPY app.py ${LAMBDA_TASK_ROOT} - -# Set the CMD to your handler (could also be done as a parameter override outside of the Dockerfile) -CMD [ "app.handler" ] diff --git a/tests/ci/cancel_and_rerun_workflow_lambda/build_and_deploy_archive.sh b/tests/ci/cancel_and_rerun_workflow_lambda/build_and_deploy_archive.sh new file mode 120000 index 00000000000..96ba3fa024e --- /dev/null +++ b/tests/ci/cancel_and_rerun_workflow_lambda/build_and_deploy_archive.sh @@ -0,0 +1 @@ +../team_keys_lambda/build_and_deploy_archive.sh \ No newline at end of file From b5872b8a05599fed281275c91471b4997239c0d0 Mon Sep 17 00:00:00 2001 From: Nikolay Degterinsky Date: Fri, 18 Nov 2022 01:16:28 +0000 Subject: [PATCH 33/65] Fix function parameters parsing --- src/Parsers/ExpressionListParsers.cpp | 2 +- src/Parsers/ParserTablesInSelectQuery.cpp | 2 +- .../queries/0_stateless/02481_fix_parameters_parsing.reference | 0 tests/queries/0_stateless/02481_fix_parameters_parsing.sql | 2 ++ 4 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 tests/queries/0_stateless/02481_fix_parameters_parsing.reference create mode 100644 tests/queries/0_stateless/02481_fix_parameters_parsing.sql diff --git a/src/Parsers/ExpressionListParsers.cpp b/src/Parsers/ExpressionListParsers.cpp index d29aa248ec4..da39afa4bb9 100644 --- a/src/Parsers/ExpressionListParsers.cpp +++ b/src/Parsers/ExpressionListParsers.cpp @@ -920,7 +920,7 @@ public: , ErrorCodes::SYNTAX_ERROR); } - if (allow_function_parameters && ParserToken(TokenType::OpeningRoundBracket).ignore(pos, expected)) + if (allow_function_parameters && !parameters && ParserToken(TokenType::OpeningRoundBracket).ignore(pos, expected)) { parameters = std::make_shared(); std::swap(parameters->children, elements); diff --git a/src/Parsers/ParserTablesInSelectQuery.cpp b/src/Parsers/ParserTablesInSelectQuery.cpp index ef39df8ca52..cff4c959267 100644 --- a/src/Parsers/ParserTablesInSelectQuery.cpp +++ b/src/Parsers/ParserTablesInSelectQuery.cpp @@ -22,7 +22,7 @@ bool ParserTableExpression::parseImpl(Pos & pos, ASTPtr & node, Expected & expec auto res = std::make_shared(); if (!ParserWithOptionalAlias(std::make_unique(), true).parse(pos, res->subquery, expected) - && !ParserWithOptionalAlias(std::make_unique(true, true), true).parse(pos, res->table_function, expected) + && !ParserWithOptionalAlias(std::make_unique(false, true), true).parse(pos, res->table_function, expected) && !ParserWithOptionalAlias(std::make_unique(true, true), true) .parse(pos, res->database_and_table_name, expected)) return false; diff --git a/tests/queries/0_stateless/02481_fix_parameters_parsing.reference b/tests/queries/0_stateless/02481_fix_parameters_parsing.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/02481_fix_parameters_parsing.sql b/tests/queries/0_stateless/02481_fix_parameters_parsing.sql new file mode 100644 index 00000000000..6164ec77774 --- /dev/null +++ b/tests/queries/0_stateless/02481_fix_parameters_parsing.sql @@ -0,0 +1,2 @@ +SELECT func(1)(2)(3); -- { clientError SYNTAX_ERROR } +SELECT * FROM VALUES(1)(2); -- { clientError SYNTAX_ERROR } From b1fcdfcaadd370c2b0a507e78302f3504ace0ccd Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Fri, 18 Nov 2022 14:26:19 +0100 Subject: [PATCH 34/65] Preserve the whole event for debugging --- tests/ci/cancel_and_rerun_workflow_lambda/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ci/cancel_and_rerun_workflow_lambda/app.py b/tests/ci/cancel_and_rerun_workflow_lambda/app.py index 321aa068598..9f4ea3a9c48 100644 --- a/tests/ci/cancel_and_rerun_workflow_lambda/app.py +++ b/tests/ci/cancel_and_rerun_workflow_lambda/app.py @@ -276,7 +276,7 @@ def exec_workflow_url(urls_to_cancel, token): def main(event): token = get_token_from_aws() - DEBUG_INFO["event_body"] = event["body"] + DEBUG_INFO["event"] = event event_data = json.loads(event["body"]) print("Got event for PR", event_data["number"]) From e9e355dd82590392fb36b07b116f4fcb79ce7866 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Fri, 18 Nov 2022 15:17:36 +0100 Subject: [PATCH 35/65] Process optionally base64-encoded bodies --- tests/ci/cancel_and_rerun_workflow_lambda/app.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/ci/cancel_and_rerun_workflow_lambda/app.py b/tests/ci/cancel_and_rerun_workflow_lambda/app.py index 9f4ea3a9c48..6d63aaa141e 100644 --- a/tests/ci/cancel_and_rerun_workflow_lambda/app.py +++ b/tests/ci/cancel_and_rerun_workflow_lambda/app.py @@ -1,5 +1,6 @@ #!/usr/bin/env python3 +from base64 import b64decode from collections import namedtuple from typing import Any, Dict, List from threading import Thread @@ -277,7 +278,10 @@ def exec_workflow_url(urls_to_cancel, token): def main(event): token = get_token_from_aws() DEBUG_INFO["event"] = event - event_data = json.loads(event["body"]) + if event["isBase64Encoded"]: + event_data = json.loads(b64decode(event["body"])) + else: + event_data = json.loads(event["body"]) print("Got event for PR", event_data["number"]) action = event_data["action"] From 6a3b57c27d322650e58f2c0ade2b281b3ad9780f Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Fri, 18 Nov 2022 19:44:36 +0100 Subject: [PATCH 36/65] revert changes in AggregateFunctionMinMaxAny.h --- .../AggregateFunctionMinMaxAny.h | 61 +++---------------- 1 file changed, 10 insertions(+), 51 deletions(-) diff --git a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h index 024112dd65f..46be7331195 100644 --- a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h +++ b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h @@ -471,7 +471,10 @@ public: static constexpr bool is_nullable = false; static constexpr bool is_any = false; - bool has() const { return size >= 1; } + bool has() const + { + return size >= 0; + } const char * getData() const { @@ -485,61 +488,17 @@ public: void insertResultInto(IColumn & to) const { - /// Same comment from write() applies here, but in this case we remove the last NULL character as we expect it to come from - /// old statuses that always included it if (has()) - { - const char * data = getData(); - size_t final_size = data[size - 1] != '\0' ? size : size - 1; - if (final_size) - { - assert_cast(to).insertData(data, final_size); - return; - } - } - - assert_cast(to).insertDefault(); + assert_cast(to).insertData(getData(), size); + else + assert_cast(to).insertDefault(); } void write(WriteBuffer & buf, const ISerialization & /*serialization*/) const { - /// This used to be serialized using insertDataWithTerminatingZero up until 22.9 when it was changed to use insertData - /// The problem is that this change, inadvertently, changed the serialization of the state, as insertDataWithTerminatingZero - /// included the final NULL in its size count, while insertData doesn't. That is a string like "1234" would have been inserted - /// as { size: 5, data: "1234\0" }, but with the change it was inserted as { size: 4, data: "1234\0" }- - /// This changed in the stored size means that values created prior to this change, when read in an updated server, would - /// consider the additional \0 as being part of the string; and that if you went backwards (downgrade or replication to an older - /// server), newer values would remove the ending character of the string. - /// - /// To revert this change, the serialization has been changed yet again to replicate the old behaviour with one important - /// caveat: As we can read both statuses with or without the change, we only add the extra +1 if we need it, as we decide if we - /// need it based on whether the string is ended with a NULL (always on old statuses) or not (most of the time in newer statuses). - /// The main problem with this approach is that there is a chance of newer status containing a NULL ending character, as it might - /// have been part of the inserted string "naturally" (the data was inserted with a NULL) - /// - /// This should be taken as a patch applied to be as compatible as possible with older releases (with the NULL caveat), that is - /// to be able to work with data generated by any prior release, with or without the intended change. - /// In the future a new serialization **version** should be introduced replacing the current one in a backwards compatible manner. - /// Refs: https://github.com/ClickHouse/ClickHouse/pull/43038 and https://github.com/ClickHouse/ClickHouse/issues/42916 - if (!has()) - { - writeBinary(Int32{1}, buf); - buf.write('\0'); - return; - } - - const char * data = getData(); - if (data[size - 1] != '\0') - { - writeBinary(size + 1, buf); - buf.write(data, size); - buf.write('\0'); - } - else - { - writeBinary(size, buf); - buf.write(data, size); - } + writeBinary(size, buf); + if (has()) + buf.write(getData(), size); } void read(ReadBuffer & buf, const ISerialization & /*serialization*/, Arena * arena) From 415eaff8d5535538bda90015f0b56ae5a9eda3c8 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Thu, 17 Nov 2022 16:52:52 +0100 Subject: [PATCH 37/65] revert incompatible changes --- .../AggregateFunctionMinMaxAny.h | 43 ++++++++++++++++--- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h index 46be7331195..a8e618c3a34 100644 --- a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h +++ b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h @@ -447,6 +447,34 @@ public: }; +struct Compatibility +{ + /// Old versions used to store terminating null-character in SingleValueDataString. + /// Then -WithTerminatingZero methods were removed from IColumn interface, + /// because these methods are quite dangerous and easy to misuse. It introduced incompatibility. + /// See https://github.com/ClickHouse/ClickHouse/pull/41431 and https://github.com/ClickHouse/ClickHouse/issues/42916 + /// Here we keep these functions for compatibility. + /// It's safe because there's no way unsanitized user input (without \0 at the end) can reach these functions. + + static StringRef getDataAtWithTerminatingZero(const ColumnString & column, size_t n) + { + auto res = column.getDataAt(n); + /// ColumnString always reserves extra byte for null-character after string. + /// But getDataAt returns StringRef without the null-character. Let's add it. + chassert(res.data[res.size] == 0); + ++res.size; + return res; + } + + static void insertDataWithTerminatingZero(ColumnString & column, const char * pos, size_t length) + { + /// String already has terminating null-character. + /// But insertData will add another one unconditionally. Trim existing null-character to avoid duplication. + chassert(0 < length); + chassert(pos[length - 1] == 0); + column.insertData(pos, length - 1); + } +}; /** For strings. Short strings are stored in the object itself, and long strings are allocated separately. * NOTE It could also be suitable for arrays of numbers. @@ -476,6 +504,7 @@ public: return size >= 0; } +private: const char * getData() const { return size <= MAX_SMALL_STRING_SIZE ? small_data : large_data; @@ -483,13 +512,17 @@ public: StringRef getStringRef() const { + /// It must always be terminated with null-character + chassert(0 < size); + chassert(getData()[size - 1] == 0); return StringRef(getData(), size); } +public: void insertResultInto(IColumn & to) const { if (has()) - assert_cast(to).insertData(getData(), size); + Compatibility::insertDataWithTerminatingZero(assert_cast(to), getData(), size); else assert_cast(to).insertDefault(); } @@ -566,7 +599,7 @@ public: void change(const IColumn & column, size_t row_num, Arena * arena) { - changeImpl(assert_cast(column).getDataAt(row_num), arena); + changeImpl(Compatibility::getDataAtWithTerminatingZero(assert_cast(column), row_num), arena); } void change(const Self & to, Arena * arena) @@ -615,7 +648,7 @@ public: bool changeIfLess(const IColumn & column, size_t row_num, Arena * arena) { - if (!has() || assert_cast(column).getDataAt(row_num) < getStringRef()) + if (!has() || Compatibility::getDataAtWithTerminatingZero(assert_cast(column), row_num) < getStringRef()) { change(column, row_num, arena); return true; @@ -637,7 +670,7 @@ public: bool changeIfGreater(const IColumn & column, size_t row_num, Arena * arena) { - if (!has() || assert_cast(column).getDataAt(row_num) > getStringRef()) + if (!has() || Compatibility::getDataAtWithTerminatingZero(assert_cast(column), row_num) > getStringRef()) { change(column, row_num, arena); return true; @@ -664,7 +697,7 @@ public: bool isEqualTo(const IColumn & column, size_t row_num) const { - return has() && assert_cast(column).getDataAt(row_num) == getStringRef(); + return has() && Compatibility::getDataAtWithTerminatingZero(assert_cast(column), row_num) == getStringRef(); } static bool allocatesMemoryInArena() From 35f677d7d8a3c0bb14b44c642dec49017c3d74e1 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Thu, 17 Nov 2022 18:54:07 +0100 Subject: [PATCH 38/65] make read(...) compatible with buggy versions --- .../AggregateFunctionMinMaxAny.h | 110 ++++++++++++------ 1 file changed, 73 insertions(+), 37 deletions(-) diff --git a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h index a8e618c3a34..9722dec59e1 100644 --- a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h +++ b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h @@ -29,6 +29,7 @@ namespace ErrorCodes { extern const int ILLEGAL_TYPE_OF_ARGUMENT; extern const int NOT_IMPLEMENTED; + extern const int TOO_LARGE_STRING_SIZE; } /** Aggregate functions that store one of passed values. @@ -461,7 +462,7 @@ struct Compatibility auto res = column.getDataAt(n); /// ColumnString always reserves extra byte for null-character after string. /// But getDataAt returns StringRef without the null-character. Let's add it. - chassert(res.data[res.size] == 0); + chassert(res.data[res.size] == '\0'); ++res.size; return res; } @@ -471,7 +472,7 @@ struct Compatibility /// String already has terminating null-character. /// But insertData will add another one unconditionally. Trim existing null-character to avoid duplication. chassert(0 < length); - chassert(pos[length - 1] == 0); + chassert(pos[length - 1] == '\0'); column.insertData(pos, length - 1); } }; @@ -505,16 +506,22 @@ public: } private: - const char * getData() const + char * getDataMutable() { return size <= MAX_SMALL_STRING_SIZE ? small_data : large_data; } - StringRef getStringRef() const + const char * getData() const { + const char * data_ptr = size <= MAX_SMALL_STRING_SIZE ? small_data : large_data; /// It must always be terminated with null-character chassert(0 < size); - chassert(getData()[size - 1] == 0); + chassert(data_ptr[size - 1] == '\0'); + return data_ptr; + } + + StringRef getStringRef() const + { return StringRef(getData(), size); } @@ -534,40 +541,75 @@ public: buf.write(getData(), size); } + void allocateLargeDataIfNeeded(Int32 size_to_reserve, Arena * arena) + { + if (capacity < size_to_reserve) + { + capacity = static_cast(roundUpToPowerOfTwoOrZero(size_to_reserve)); + /// It might happen if the size was too big and the rounded value does not fit a size_t + if (unlikely(capacity < size_to_reserve)) + throw Exception(ErrorCodes::TOO_LARGE_STRING_SIZE, "String size is too big ({})", size_to_reserve); + + /// Don't free large_data here. + large_data = arena->alloc(capacity); + } + } + void read(ReadBuffer & buf, const ISerialization & /*serialization*/, Arena * arena) { Int32 rhs_size; readBinary(rhs_size, buf); - if (rhs_size >= 0) - { - if (rhs_size <= MAX_SMALL_STRING_SIZE) - { - /// Don't free large_data here. - - size = rhs_size; - - if (size > 0) - buf.readStrict(small_data, size); - } - else - { - if (capacity < rhs_size) - { - capacity = static_cast(roundUpToPowerOfTwoOrZero(rhs_size)); - /// Don't free large_data here. - large_data = arena->alloc(capacity); - } - - size = rhs_size; - buf.readStrict(large_data, size); - } - } - else + if (rhs_size < 0) { /// Don't free large_data here. size = rhs_size; + return; } + + if (rhs_size <= MAX_SMALL_STRING_SIZE) + { + /// Don't free large_data here. + + size = rhs_size; + + if (size > 0) + buf.readStrict(small_data, size); + } + else + { + /// Reserve one byte more for null-character + Int32 rhs_size_to_reserve = rhs_size + 1; + allocateLargeDataIfNeeded(rhs_size_to_reserve, arena); + size = rhs_size; + buf.readStrict(large_data, size); + } + + /// Check if the string we read is null-terminated (getDataMutable does not have the assertion) + if (0 < size && getDataMutable()[size - 1] == '\0') + return; + + /// It's not null-terminated, but it must be (for historical reasons). There are two variants: + /// - The value was serialized by one of the incompatible versions of ClickHouse. We had some range of versions + /// that used to serialize SingleValueDataString without terminating '\0'. Let's just append it. + /// - An attacker sent crafted data. Sanitize it and append '\0'. + /// In all other cases the string must be already null-terminated. + + /// NOTE We cannot add '\0' unconditionally, because it will be duplicated. + /// NOTE It's possible that a string that actually ends with '\0' was written by one of the incompatible versions. + /// Unfortunately, we cannot distinguish it from normal string written by normal version. + /// So such strings will be trimmed. + + if (size == MAX_SMALL_STRING_SIZE) + { + /// Special case: We have to move value to large_data + allocateLargeDataIfNeeded(size + 1, arena); + memcpy(large_data, small_data, size); + } + + /// We have enough space to append + getDataMutable()[size] = '\0'; + ++size; } /// Assuming to.has() @@ -585,13 +627,7 @@ public: } else { - if (capacity < value_size) - { - /// Don't free large_data here. - capacity = static_cast(roundUpToPowerOfTwoOrZero(value_size)); - large_data = arena->alloc(capacity); - } - + allocateLargeDataIfNeeded(value_size, arena); size = value_size; memcpy(large_data, value.data, size); } From 4af8ef381be5e2ef0dbcb344b229eb623d7ce0fe Mon Sep 17 00:00:00 2001 From: Alexander Gololobov <440544+davenger@users.noreply.github.com> Date: Fri, 18 Nov 2022 17:33:51 +0100 Subject: [PATCH 39/65] Test with default value used in row level policy --- ...t_value_used_in_row_level_filter.reference | 16 ++++++++++++ ...default_value_used_in_row_level_filter.sql | 25 +++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 tests/queries/0_stateless/02481_default_value_used_in_row_level_filter.reference create mode 100644 tests/queries/0_stateless/02481_default_value_used_in_row_level_filter.sql diff --git a/tests/queries/0_stateless/02481_default_value_used_in_row_level_filter.reference b/tests/queries/0_stateless/02481_default_value_used_in_row_level_filter.reference new file mode 100644 index 00000000000..c8e17be819a --- /dev/null +++ b/tests/queries/0_stateless/02481_default_value_used_in_row_level_filter.reference @@ -0,0 +1,16 @@ +-- { echoOn } + +SELECT a, c FROM test_rlp WHERE c%2 == 0 AND b < 5; +0 10 +2 12 +4 14 +DROP POLICY IF EXISTS test_rlp_policy ON test_rlp; +CREATE ROW POLICY test_rlp_policy ON test_rlp FOR SELECT USING c%2 == 0 TO default; +SELECT a, c FROM test_rlp WHERE b < 5 SETTINGS optimize_move_to_prewhere = 0; +0 10 +2 12 +4 14 +SELECT a, c FROM test_rlp PREWHERE b < 5; +0 10 +2 12 +4 14 diff --git a/tests/queries/0_stateless/02481_default_value_used_in_row_level_filter.sql b/tests/queries/0_stateless/02481_default_value_used_in_row_level_filter.sql new file mode 100644 index 00000000000..6835a3a57ea --- /dev/null +++ b/tests/queries/0_stateless/02481_default_value_used_in_row_level_filter.sql @@ -0,0 +1,25 @@ +DROP TABLE IF EXISTS test_rlp; + +CREATE TABLE test_rlp (a Int32, b Int32) ENGINE=MergeTree() ORDER BY a SETTINGS index_granularity=5; + +INSERT INTO test_rlp SELECT number, number FROM numbers(15); + +ALTER TABLE test_rlp ADD COLUMN c Int32 DEFAULT b+10; + +-- { echoOn } + +SELECT a, c FROM test_rlp WHERE c%2 == 0 AND b < 5; + +DROP POLICY IF EXISTS test_rlp_policy ON test_rlp; + +CREATE ROW POLICY test_rlp_policy ON test_rlp FOR SELECT USING c%2 == 0 TO default; + +SELECT a, c FROM test_rlp WHERE b < 5 SETTINGS optimize_move_to_prewhere = 0; + +SELECT a, c FROM test_rlp PREWHERE b < 5; + +-- { echoOff } + +DROP POLICY test_rlp_policy ON test_rlp; + +DROP TABLE test_rlp; From f004eea413804004885dfd223a743d78df8de219 Mon Sep 17 00:00:00 2001 From: Alexander Gololobov <440544+davenger@users.noreply.github.com> Date: Fri, 18 Nov 2022 23:42:45 +0100 Subject: [PATCH 40/65] Add columns required fro defaults calculation --- src/Storages/MergeTree/MergeTreeBlockReadUtils.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Storages/MergeTree/MergeTreeBlockReadUtils.cpp b/src/Storages/MergeTree/MergeTreeBlockReadUtils.cpp index c3f069498be..525d76d0f0f 100644 --- a/src/Storages/MergeTree/MergeTreeBlockReadUtils.cpp +++ b/src/Storages/MergeTree/MergeTreeBlockReadUtils.cpp @@ -315,7 +315,9 @@ MergeTreeReadTaskColumns getReadTaskColumns( /// 1. Columns for row level filter if (prewhere_info->row_level_filter) { - Names row_filter_column_names = prewhere_info->row_level_filter->getRequiredColumnsNames(); + Names row_filter_column_names = prewhere_info->row_level_filter->getRequiredColumnsNames(); + injectRequiredColumns( + data_part_info_for_reader, storage_snapshot, with_subcolumns, row_filter_column_names); result.pre_columns.push_back(storage_snapshot->getColumnsByNames(options, row_filter_column_names)); pre_name_set.insert(row_filter_column_names.begin(), row_filter_column_names.end()); } @@ -323,7 +325,7 @@ MergeTreeReadTaskColumns getReadTaskColumns( /// 2. Columns for prewhere Names all_pre_column_names = prewhere_info->prewhere_actions->getRequiredColumnsNames(); - const auto injected_pre_columns = injectRequiredColumns( + injectRequiredColumns( data_part_info_for_reader, storage_snapshot, with_subcolumns, all_pre_column_names); for (const auto & name : all_pre_column_names) From 4ee5d8bd43f2c179b6233a19a0625d1abba6accd Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Sat, 19 Nov 2022 11:51:14 +0000 Subject: [PATCH 41/65] Update version_date.tsv and changelogs after v22.8.9.24-lts --- docs/changelogs/v22.8.9.24-lts.md | 31 ++++++++++++++++++++++++++++ utils/list-versions/version_date.tsv | 1 + 2 files changed, 32 insertions(+) create mode 100644 docs/changelogs/v22.8.9.24-lts.md diff --git a/docs/changelogs/v22.8.9.24-lts.md b/docs/changelogs/v22.8.9.24-lts.md new file mode 100644 index 00000000000..e1f4c2bcdf0 --- /dev/null +++ b/docs/changelogs/v22.8.9.24-lts.md @@ -0,0 +1,31 @@ +--- +sidebar_position: 1 +sidebar_label: 2022 +--- + +# 2022 Changelog + +### ClickHouse release v22.8.9.24-lts (a1b69551d40) FIXME as compared to v22.8.8.3-lts (ac5a6cababc) + +#### Performance Improvement +* Backported in [#43012](https://github.com/ClickHouse/ClickHouse/issues/43012): Keeper performance improvement: improve commit performance for cases when many different nodes have uncommitted states. This should help with cases when a follower node can't sync fast enough. [#42926](https://github.com/ClickHouse/ClickHouse/pull/42926) ([Antonio Andelic](https://github.com/antonio2368)). + +#### Improvement +* Backported in [#42840](https://github.com/ClickHouse/ClickHouse/issues/42840): Update tzdata to 2022f. Mexico will no longer observe DST except near the US border: https://www.timeanddate.com/news/time/mexico-abolishes-dst-2022.html. Chihuahua moves to year-round UTC-6 on 2022-10-30. Fiji no longer observes DST. See https://github.com/google/cctz/pull/235 and https://bugs.launchpad.net/ubuntu/+source/tzdata/+bug/1995209. [#42796](https://github.com/ClickHouse/ClickHouse/pull/42796) ([Alexey Milovidov](https://github.com/alexey-milovidov)). + +#### Build/Testing/Packaging Improvement +* Backported in [#42964](https://github.com/ClickHouse/ClickHouse/issues/42964): Before the fix, the user-defined config was preserved by RPM in `$file.rpmsave`. The PR fixes it and won't replace the user's files from packages. [#42936](https://github.com/ClickHouse/ClickHouse/pull/42936) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Backported in [#43040](https://github.com/ClickHouse/ClickHouse/issues/43040): Add a CI step to mark commits as ready for release; soft-forbid launching a release script from branches but master. [#43017](https://github.com/ClickHouse/ClickHouse/pull/43017) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). + +#### Bug Fix (user-visible misbehavior in official stable or prestable release) + +* Backported in [#42720](https://github.com/ClickHouse/ClickHouse/issues/42720): Fixed `Unknown identifier (aggregate-function)` exception which appears when a user tries to calculate WINDOW ORDER BY/PARTITION BY expressions over aggregate functions: ``` CREATE TABLE default.tenk1 ( `unique1` Int32, `unique2` Int32, `ten` Int32 ) ENGINE = MergeTree ORDER BY tuple() SETTINGS index_granularity = 8192; SELECT ten, sum(unique1) + sum(unique2) AS res, rank() OVER (ORDER BY sum(unique1) + sum(unique2) ASC) AS rank FROM _complex GROUP BY ten ORDER BY ten ASC; ``` which gives: ``` Code: 47. DB::Exception: Received from localhost:9000. DB::Exception: Unknown identifier: sum(unique1); there are columns: unique1, unique2, ten: While processing sum(unique1) + sum(unique2) ASC. (UNKNOWN_IDENTIFIER) ```. [#39762](https://github.com/ClickHouse/ClickHouse/pull/39762) ([Vladimir Chebotaryov](https://github.com/quickhouse)). +* Backported in [#42748](https://github.com/ClickHouse/ClickHouse/issues/42748): A segmentation fault related to DNS & c-ares has been reported. The below error ocurred in multiple threads: ``` 2022-09-28 15:41:19.008,2022.09.28 15:41:19.008088 [ 356 ] {} BaseDaemon: ######################################## 2022-09-28 15:41:19.008,"2022.09.28 15:41:19.008147 [ 356 ] {} BaseDaemon: (version 22.8.5.29 (official build), build id: 92504ACA0B8E2267) (from thread 353) (no query) Received signal Segmentation fault (11)" 2022-09-28 15:41:19.008,2022.09.28 15:41:19.008196 [ 356 ] {} BaseDaemon: Address: 0xf Access: write. Address not mapped to object. 2022-09-28 15:41:19.008,2022.09.28 15:41:19.008216 [ 356 ] {} BaseDaemon: Stack trace: 0x188f8212 0x1626851b 0x1626a69e 0x16269b3f 0x16267eab 0x13cf8284 0x13d24afc 0x13c5217e 0x14ec2495 0x15ba440f 0x15b9d13b 0x15bb2699 0x1891ccb3 0x1891e00d 0x18ae0769 0x18ade022 0x7f76aa985609 0x7f76aa8aa133 2022-09-28 15:41:19.008,2022.09.28 15:41:19.008274 [ 356 ] {} BaseDaemon: 2. Poco::Net::IPAddress::family() const @ 0x188f8212 in /usr/bin/clickhouse 2022-09-28 15:41:19.008,2022.09.28 15:41:19.008297 [ 356 ] {} BaseDaemon: 3. ? @ 0x1626851b in /usr/bin/clickhouse 2022-09-28 15:41:19.008,2022.09.28 15:41:19.008309 [ 356 ] {} BaseDaemon: 4. ? @ 0x1626a69e in /usr/bin/clickhouse ```. [#42234](https://github.com/ClickHouse/ClickHouse/pull/42234) ([Arthur Passos](https://github.com/arthurpassos)). +* Backported in [#43062](https://github.com/ClickHouse/ClickHouse/issues/43062): Fix rare NOT_FOUND_COLUMN_IN_BLOCK error when projection is possible to use but there is no projection available. This fixes [#42771](https://github.com/ClickHouse/ClickHouse/issues/42771) . The bug was introduced in https://github.com/ClickHouse/ClickHouse/pull/25563. [#42938](https://github.com/ClickHouse/ClickHouse/pull/42938) ([Amos Bird](https://github.com/amosbird)). + +#### NOT FOR CHANGELOG / INSIGNIFICANT + +* Do not warn about kvm-clock [#41217](https://github.com/ClickHouse/ClickHouse/pull/41217) ([Sergei Trifonov](https://github.com/serxa)). +* Revert revert 41268 disable s3 parallel write for part moves to disk s3 [#42617](https://github.com/ClickHouse/ClickHouse/pull/42617) ([Nikolai Kochetov](https://github.com/KochetovNicolai)). +* Always run `BuilderReport` and `BuilderSpecialReport` in all CI types [#42684](https://github.com/ClickHouse/ClickHouse/pull/42684) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). + diff --git a/utils/list-versions/version_date.tsv b/utils/list-versions/version_date.tsv index 9ed5d0e8528..2c1061c3333 100644 --- a/utils/list-versions/version_date.tsv +++ b/utils/list-versions/version_date.tsv @@ -5,6 +5,7 @@ v22.9.4.32-stable 2022-10-26 v22.9.3.18-stable 2022-09-30 v22.9.2.7-stable 2022-09-23 v22.9.1.2603-stable 2022-09-22 +v22.8.9.24-lts 2022-11-19 v22.8.8.3-lts 2022-10-27 v22.8.7.34-lts 2022-10-26 v22.8.6.71-lts 2022-09-30 From 09de30a51a9e4f8f32fdaf14ee7b9d923ba3d675 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Sat, 19 Nov 2022 18:58:40 +0100 Subject: [PATCH 42/65] fix overflow, add more tests --- .../AggregateFunctionMinMaxAny.h | 7 ++++--- ..._single_value_data_string_regression.reference | 4 ++++ .../02477_single_value_data_string_regression.sql | 15 +++++++++++++++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h index 9722dec59e1..6e20e91025f 100644 --- a/src/AggregateFunctions/AggregateFunctionMinMaxAny.h +++ b/src/AggregateFunctions/AggregateFunctionMinMaxAny.h @@ -541,7 +541,7 @@ public: buf.write(getData(), size); } - void allocateLargeDataIfNeeded(Int32 size_to_reserve, Arena * arena) + void allocateLargeDataIfNeeded(Int64 size_to_reserve, Arena * arena) { if (capacity < size_to_reserve) { @@ -579,7 +579,8 @@ public: else { /// Reserve one byte more for null-character - Int32 rhs_size_to_reserve = rhs_size + 1; + Int64 rhs_size_to_reserve = rhs_size; + rhs_size_to_reserve += 1; /// Avoid overflow allocateLargeDataIfNeeded(rhs_size_to_reserve, arena); size = rhs_size; buf.readStrict(large_data, size); @@ -608,8 +609,8 @@ public: } /// We have enough space to append - getDataMutable()[size] = '\0'; ++size; + getDataMutable()[size - 1] = '\0'; } /// Assuming to.has() diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.reference b/tests/queries/0_stateless/02477_single_value_data_string_regression.reference index 508b6cae649..2174a5d40d2 100644 --- a/tests/queries/0_stateless/02477_single_value_data_string_regression.reference +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.reference @@ -17,3 +17,7 @@ 63_KO 012345678901234567890123456789012345678901234567890123456789012 64_OK 0123456789012345678901234567890123456789012345678901234567890123 64_KO 0123456789012345678901234567890123456789012345678901234567890123 +-1 0 +-2 0 +-2^31 0 +1M 1048576 diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql index b126707535f..f399702e003 100644 --- a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql @@ -85,3 +85,18 @@ SELECT '63_KO', finalizeAggregation(CAST(unhex('3F000000303132333435363738393031 -- SELECT hex(argMaxState('0123456789012345678901234567890123456789012345678901234567890123' as a, number)) AS state, length(a) FROM numbers(1) FORMAT CSV SELECT '64_OK', finalizeAggregation(CAST(unhex('410000003031323334353637383930313233343536373839303132333435363738393031323334353637383930313233343536373839303132333435363738393031323300010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); SELECT '64_KO', finalizeAggregation(CAST(unhex('4000000030313233343536373839303132333435363738393031323334353637383930313233343536373839303132333435363738393031323334353637383930313233010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); + +SELECT '-1', maxMerge(x), length(maxMerge(x)) from (select CAST(unhex('ffffffff') || randomString(100500), 'AggregateFunction(max, String)') as x); +SELECT '-2', maxMerge(x), length(maxMerge(x)) from (select CAST(unhex('fffffffe') || randomString(100500), 'AggregateFunction(max, String)') as x); +SELECT '-2^31', maxMerge(x), length(maxMerge(x)) from (select CAST(unhex('00000080') || randomString(100500), 'AggregateFunction(max, String)') as x); + +SELECT '2^31-2', maxMerge(x) from (select CAST(unhex('feffff7f') || randomString(100500), 'AggregateFunction(max, String)') as x); -- { serverError TOO_LARGE_STRING_SIZE } +SELECT '2^31-1', maxMerge(x) from (select CAST(unhex('ffffff7f') || randomString(100500), 'AggregateFunction(max, String)') as x); -- { serverError TOO_LARGE_STRING_SIZE } + +SELECT '2^30', maxMerge(x) from (select CAST(unhex('00000040') || randomString(100500), 'AggregateFunction(max, String)') as x); -- { serverError TOO_LARGE_STRING_SIZE } +SELECT '2^30+1', maxMerge(x) from (select CAST(unhex('01000040') || randomString(100500), 'AggregateFunction(max, String)') as x); -- { serverError TOO_LARGE_STRING_SIZE } + +SELECT '2^30-1', maxMerge(x) from (select CAST(unhex('ffffff3f') || randomString(100500), 'AggregateFunction(max, String)') as x); -- { serverError CANNOT_READ_ALL_DATA } +-- The following query works, but it's too long and consumes to much memory +-- SELECT '2^30-1', length(maxMerge(x)) from (select CAST(unhex('ffffff3f') || randomString(0x3FFFFFFF), 'AggregateFunction(max, String)') as x); +SELECT '1M', length(maxMerge(x)) from (select CAST(unhex('00001000') || randomString(0x00100000), 'AggregateFunction(max, String)') as x); From a00f7d142ddb7ec176d78cd23111a213bb384abb Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Sat, 19 Nov 2022 21:02:48 +0100 Subject: [PATCH 43/65] fix flaky test --- .../02477_single_value_data_string_regression.reference | 3 ++- .../02477_single_value_data_string_regression.sql | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.reference b/tests/queries/0_stateless/02477_single_value_data_string_regression.reference index 2174a5d40d2..2fe0effb902 100644 --- a/tests/queries/0_stateless/02477_single_value_data_string_regression.reference +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.reference @@ -20,4 +20,5 @@ -1 0 -2 0 -2^31 0 -1M 1048576 +1M without 0 1048576 +1M with 0 1048575 diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql index f399702e003..27faf00af56 100644 --- a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql @@ -98,5 +98,6 @@ SELECT '2^30+1', maxMerge(x) from (select CAST(unhex('01000040') || randomString SELECT '2^30-1', maxMerge(x) from (select CAST(unhex('ffffff3f') || randomString(100500), 'AggregateFunction(max, String)') as x); -- { serverError CANNOT_READ_ALL_DATA } -- The following query works, but it's too long and consumes to much memory --- SELECT '2^30-1', length(maxMerge(x)) from (select CAST(unhex('ffffff3f') || randomString(0x3FFFFFFF), 'AggregateFunction(max, String)') as x); -SELECT '1M', length(maxMerge(x)) from (select CAST(unhex('00001000') || randomString(0x00100000), 'AggregateFunction(max, String)') as x); +-- SELECT '2^30-1', length(maxMerge(x)) from (select CAST(unhex('ffffff3f') || randomString(0x3FFFFFFF - 1) || 'x', 'AggregateFunction(max, String)') as x); +SELECT '1M without 0', length(maxMerge(x)) from (select CAST(unhex('00001000') || randomString(0x00100000 - 1) || 'x', 'AggregateFunction(max, String)') as x); +SELECT '1M with 0', length(maxMerge(x)) from (select CAST(unhex('00001000') || randomString(0x00100000 - 1) || '\0', 'AggregateFunction(max, String)') as x); From 854657d372329427d0a1da53b503e8918d54024a Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Sat, 19 Nov 2022 23:14:53 +0100 Subject: [PATCH 44/65] fix another issue --- src/AggregateFunctions/AggregateFunctionArgMinMax.h | 3 +++ .../0_stateless/02477_single_value_data_string_regression.sql | 2 ++ 2 files changed, 5 insertions(+) diff --git a/src/AggregateFunctions/AggregateFunctionArgMinMax.h b/src/AggregateFunctions/AggregateFunctionArgMinMax.h index 516d33f42de..0cf0a989a32 100644 --- a/src/AggregateFunctions/AggregateFunctionArgMinMax.h +++ b/src/AggregateFunctions/AggregateFunctionArgMinMax.h @@ -13,6 +13,7 @@ struct Settings; namespace ErrorCodes { extern const int ILLEGAL_TYPE_OF_ARGUMENT; + extern const int CORRUPTED_DATA; } @@ -89,6 +90,8 @@ public: { this->data(place).result.read(buf, *serialization_res, arena); this->data(place).value.read(buf, *serialization_val, arena); + if (unlikely(this->data(place).value.has() && !this->data(place).result.has())) + throw Exception(ErrorCodes::CORRUPTED_DATA, "State of aggregate function {} has value, but does not have result", getName()); } bool allocatesMemoryInArena() const override diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql index 27faf00af56..82b74868fc5 100644 --- a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql @@ -101,3 +101,5 @@ SELECT '2^30-1', maxMerge(x) from (select CAST(unhex('ffffff3f') || randomString -- SELECT '2^30-1', length(maxMerge(x)) from (select CAST(unhex('ffffff3f') || randomString(0x3FFFFFFF - 1) || 'x', 'AggregateFunction(max, String)') as x); SELECT '1M without 0', length(maxMerge(x)) from (select CAST(unhex('00001000') || randomString(0x00100000 - 1) || 'x', 'AggregateFunction(max, String)') as x); SELECT '1M with 0', length(maxMerge(x)) from (select CAST(unhex('00001000') || randomString(0x00100000 - 1) || '\0', 'AggregateFunction(max, String)') as x); + +SELECT 'fuzz1', finalizeAggregation(CAST(unhex('3000000\0303132333435363738393031323334353637383930313233343536373839303132333435363738393031323334353600010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); -- { serverError CORRUPTED_DATA } From b3a06175e758e5f10397d0efd4fee96bf66a4bc4 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sun, 20 Nov 2022 07:47:20 +0100 Subject: [PATCH 45/65] Add a comment --- src/TableFunctions/ITableFunction.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/TableFunctions/ITableFunction.h b/src/TableFunctions/ITableFunction.h index 79c58083020..21c26062566 100644 --- a/src/TableFunctions/ITableFunction.h +++ b/src/TableFunctions/ITableFunction.h @@ -86,6 +86,16 @@ private: struct TableFunctionProperties { Documentation documentation; + + /** It is determined by the possibility of modifying any data or making requests to arbitrary hostnames. + * + * If users can make a request to an arbitrary hostname, they can get the info from the internal network + * or manipulate internal APIs (say - put some data into Memcached, which is available only in the corporate network). + * This is named "SSRF attack". + * Or a user can use an open ClickHouse server to amplify DoS attacks. + * + * In those cases, the table function should not be allowed in readonly mode. + */ bool allow_readonly = false; }; From 324b1a7658da3078d72fedac2bcc7598ca7acfcb Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 10 Nov 2022 20:11:37 +0100 Subject: [PATCH 46/65] Add server UUID for the S3 disks checks to avoid possible races Otherwise, if you are lucky enough, race condition is possible, and you can get some errors because one server already removed the file while another was trying to read it. But it was possible only for: - batch deletes check for "s3" disk - and all checks for "s3_plain" disk, since this disk does not uses random names Signed-off-by: Azat Khuzhin --- .../ObjectStorages/S3/registerDiskS3.cpp | 128 ++++++++++-------- 1 file changed, 74 insertions(+), 54 deletions(-) diff --git a/src/Disks/ObjectStorages/S3/registerDiskS3.cpp b/src/Disks/ObjectStorages/S3/registerDiskS3.cpp index e73accbb956..dbb9b51c26b 100644 --- a/src/Disks/ObjectStorages/S3/registerDiskS3.cpp +++ b/src/Disks/ObjectStorages/S3/registerDiskS3.cpp @@ -22,6 +22,7 @@ #include #include +#include namespace DB @@ -31,79 +32,100 @@ namespace ErrorCodes { extern const int BAD_ARGUMENTS; extern const int PATH_ACCESS_DENIED; + extern const int LOGICAL_ERROR; } namespace { -void checkWriteAccess(IDisk & disk) +class CheckAccess { - auto file = disk.writeFile("test_acl", DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite); - try +public: + static void check(const String & disk_name, IDisk & disk) { - file->write("test", 4); - } - catch (...) - { - /// Log current exception, because finalize() can throw a different exception. - tryLogCurrentException(__PRETTY_FUNCTION__); - file->finalize(); - throw; - } -} + /// NOTE: you don't need to pass prefix here since the writes is done + /// with IDisk interface that will do this automatically. + const String path = fmt::format("clickhouse_access_check_{}", getServerUUID()); -void checkReadAccess(const String & disk_name, IDisk & disk) -{ - auto file = disk.readFile("test_acl"); - String buf(4, '0'); - file->readStrict(buf.data(), 4); - if (buf != "test") - throw Exception("No read access to S3 bucket in disk " + disk_name, ErrorCodes::PATH_ACCESS_DENIED); -} + /// write + { + auto file = disk.writeFile(path, DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite); + try + { + file->write("test", 4); + } + catch (...) + { + /// Log current exception, because finalize() can throw a different exception. + tryLogCurrentException(__PRETTY_FUNCTION__); + file->finalize(); + throw; + } + } -void checkRemoveAccess(IDisk & disk) -{ - disk.removeFile("test_acl"); -} + /// read + auto file = disk.readFile(path); + String buf(4, '0'); + file->readStrict(buf.data(), 4); + if (buf != "test") + throw Exception(ErrorCodes::PATH_ACCESS_DENIED, "No read access to S3 bucket in disk {}", disk_name); -bool checkBatchRemoveIsMissing(S3ObjectStorage & storage, const String & key_with_trailing_slash) -{ - StoredObject object(key_with_trailing_slash + "_test_remove_objects_capability"); - try - { - auto file = storage.writeObject(object, WriteMode::Rewrite); - file->write("test", 4); - file->finalize(); + /// remove + disk.removeFile(path); } - catch (...) + + static bool checkBatchRemove(S3ObjectStorage & storage, const String & key_with_trailing_slash) { + /// NOTE: key_with_trailing_slash is the disk prefix, it is required + /// because access is done via S3ObjectStorage not via IDisk interface + /// (since we don't have disk yet). + const String path = fmt::format("{}clickhouse_remove_objects_capability_{}", key_with_trailing_slash, getServerUUID()); + StoredObject object(path); try { - storage.removeObject(object); + auto file = storage.writeObject(object, WriteMode::Rewrite); + file->write("test", 4); + file->finalize(); } catch (...) { + try + { + storage.removeObject(object); + } + catch (...) + { + } + return true; /// We don't have write access, therefore no information about batch remove. } - return false; /// We don't have write access, therefore no information about batch remove. - } - try - { - /// Uses `DeleteObjects` request (batch delete). - storage.removeObjects({object}); - return false; - } - catch (const Exception &) - { try { - storage.removeObject(object); + /// Uses `DeleteObjects` request (batch delete). + storage.removeObjects({object}); + return true; } - catch (...) + catch (const Exception &) { + try + { + storage.removeObject(object); + } + catch (...) + { + } + return false; } - return true; } -} + +private: + static String getServerUUID() + { + DB::UUID server_uuid = DB::ServerUUID::get(); + if (server_uuid == DB::UUIDHelpers::Nil) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Server UUID is not initialized"); + return DB::toString(server_uuid); + } +}; } @@ -144,12 +166,12 @@ void registerDiskS3(DiskFactory & factory) metadata_storage = std::make_shared(metadata_disk, uri.key); } + CheckAccess access_checker; bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); - if (!skip_access_check) { /// If `support_batch_delete` is turned on (default), check and possibly switch it off. - if (s3_capabilities.support_batch_delete && checkBatchRemoveIsMissing(*s3_storage, uri.key)) + if (s3_capabilities.support_batch_delete && !access_checker.checkBatchRemove(*s3_storage, uri.key)) { LOG_WARNING( &Poco::Logger::get("registerDiskS3"), @@ -177,9 +199,7 @@ void registerDiskS3(DiskFactory & factory) /// This code is used only to check access to the corresponding disk. if (!skip_access_check) { - checkWriteAccess(*s3disk); - checkReadAccess(name, *s3disk); - checkRemoveAccess(*s3disk); + access_checker.check(name, *s3disk); } s3disk->startup(context); From 2efd29f49d8037511138a0aa1fbf1a5d3f9dbffe Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 17 Nov 2022 21:20:36 +0100 Subject: [PATCH 47/65] Implement access (read/read-by-offset/write/delete) check for all disks Previously we had such access (read/write/delete) checks only for S3 and Azure disks (read/read-by-offset/write/delete), this patch adds check for all disks. Also I've added the disk name into IDisk interface, since it is required for the error message in IDisk::checkAccess(), but I have to add DiskEncrypted::encrypted_name due DiskEncrypted inherits from DiskDecorator not from IDisk, and so does not have ability to set disk name (DiskEncrypted could pass the disk name to the DiskDecorator, but it is not used anywere, and besides this will require to duplicate the name for each user of DiskDecorator). Also from nwo on, skip_access_check will make sense for every disk, plus now startup() called for each disk (before it was missed for some of them). And I've added skip_access_check as as a member for DiskRestartProxy, since it calls startup() on restart(). Signed-off-by: Azat Khuzhin --- src/Disks/DiskDecorator.cpp | 9 ++- src/Disks/DiskDecorator.h | 2 +- src/Disks/DiskEncrypted.cpp | 9 ++- src/Disks/DiskEncrypted.h | 4 +- src/Disks/DiskLocal.cpp | 9 ++- src/Disks/DiskLocal.h | 5 +- src/Disks/DiskMemory.cpp | 19 ++++- src/Disks/DiskMemory.h | 7 +- src/Disks/DiskRestartProxy.cpp | 8 +- src/Disks/DiskRestartProxy.h | 3 +- src/Disks/IDisk.cpp | 73 +++++++++++++++++++ src/Disks/IDisk.h | 24 +++++- .../registerDiskAzureBlobStorage.cpp | 59 +-------------- .../ObjectStorages/DiskObjectStorage.cpp | 6 +- src/Disks/ObjectStorages/DiskObjectStorage.h | 5 +- .../ObjectStorages/HDFS/registerDiskHDFS.cpp | 12 +-- .../ObjectStorages/S3/registerDiskS3.cpp | 49 +------------ .../Web/registerDiskWebServer.cpp | 5 +- 18 files changed, 159 insertions(+), 149 deletions(-) diff --git a/src/Disks/DiskDecorator.cpp b/src/Disks/DiskDecorator.cpp index af17289c8af..f9017446dda 100644 --- a/src/Disks/DiskDecorator.cpp +++ b/src/Disks/DiskDecorator.cpp @@ -4,7 +4,10 @@ namespace DB { -DiskDecorator::DiskDecorator(const DiskPtr & delegate_) : delegate(delegate_) + +DiskDecorator::DiskDecorator(const DiskPtr & delegate_) + : IDisk(/* name_= */ "") + , delegate(delegate_) { } @@ -226,9 +229,9 @@ void DiskDecorator::shutdown() delegate->shutdown(); } -void DiskDecorator::startup(ContextPtr context) +void DiskDecorator::startupImpl(ContextPtr context) { - delegate->startup(context); + delegate->startupImpl(context); } void DiskDecorator::applyNewSettings(const Poco::Util::AbstractConfiguration & config, ContextPtr context, const String & config_prefix, const DisksMap & map) diff --git a/src/Disks/DiskDecorator.h b/src/Disks/DiskDecorator.h index 8b4840a4556..f7eface8c66 100644 --- a/src/Disks/DiskDecorator.h +++ b/src/Disks/DiskDecorator.h @@ -81,7 +81,7 @@ public: void onFreeze(const String & path) override; SyncGuardPtr getDirectorySyncGuard(const String & path) const override; void shutdown() override; - void startup(ContextPtr context) override; + void startupImpl(ContextPtr context) override; void applyNewSettings(const Poco::Util::AbstractConfiguration & config, ContextPtr context, const String & config_prefix, const DisksMap & map) override; bool supportsCache() const override { return delegate->supportsCache(); } diff --git a/src/Disks/DiskEncrypted.cpp b/src/Disks/DiskEncrypted.cpp index e6479727aad..50df990269e 100644 --- a/src/Disks/DiskEncrypted.cpp +++ b/src/Disks/DiskEncrypted.cpp @@ -210,7 +210,7 @@ DiskEncrypted::DiskEncrypted( DiskEncrypted::DiskEncrypted(const String & name_, std::unique_ptr settings_) : DiskDecorator(settings_->wrapped_disk) - , name(name_) + , encrypted_name(name_) , disk_path(settings_->disk_path) , disk_absolute_path(settings_->wrapped_disk->getPath() + settings_->disk_path) , current_settings(std::move(settings_)) @@ -374,10 +374,13 @@ void registerDiskEncrypted(DiskFactory & factory) auto creator = [](const String & name, const Poco::Util::AbstractConfiguration & config, const String & config_prefix, - ContextPtr /*context*/, + ContextPtr context, const DisksMap & map) -> DiskPtr { - return std::make_shared(name, config, config_prefix, map); + bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); + DiskPtr disk = std::make_shared(name, config, config_prefix, map); + disk->startup(context, skip_access_check); + return disk; }; factory.registerDiskType("encrypted", creator); } diff --git a/src/Disks/DiskEncrypted.h b/src/Disks/DiskEncrypted.h index 02b4104f36a..74da7cfa2c0 100644 --- a/src/Disks/DiskEncrypted.h +++ b/src/Disks/DiskEncrypted.h @@ -33,7 +33,7 @@ public: DiskEncrypted(const String & name_, const Poco::Util::AbstractConfiguration & config_, const String & config_prefix_, const DisksMap & map_); DiskEncrypted(const String & name_, std::unique_ptr settings_); - const String & getName() const override { return name; } + const String & getName() const override { return encrypted_name; } const String & getPath() const override { return disk_absolute_path; } ReservationPtr reserve(UInt64 bytes) override; @@ -261,7 +261,7 @@ private: return disk_path + path; } - const String name; + const String encrypted_name; const String disk_path; const String disk_absolute_path; MultiVersion current_settings; diff --git a/src/Disks/DiskLocal.cpp b/src/Disks/DiskLocal.cpp index afd6a1b7b58..b6c078611bd 100644 --- a/src/Disks/DiskLocal.cpp +++ b/src/Disks/DiskLocal.cpp @@ -500,7 +500,7 @@ void DiskLocal::applyNewSettings(const Poco::Util::AbstractConfiguration & confi } DiskLocal::DiskLocal(const String & name_, const String & path_, UInt64 keep_free_space_bytes_) - : name(name_) + : IDisk(name_) , disk_path(path_) , keep_free_space_bytes(keep_free_space_bytes_) , logger(&Poco::Logger::get("DiskLocal")) @@ -528,7 +528,7 @@ DataSourceDescription DiskLocal::getDataSourceDescription() const return data_source_description; } -void DiskLocal::startup(ContextPtr) +void DiskLocal::startupImpl(ContextPtr) { try { @@ -757,10 +757,11 @@ void registerDiskLocal(DiskFactory & factory) if (path == disk_ptr->getPath()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Disk {} and disk {} cannot have the same path ({})", name, disk_name, path); + bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); std::shared_ptr disk = std::make_shared(name, path, keep_free_space_bytes, context, config.getUInt("local_disk_check_period_ms", 0)); - disk->startup(context); - return std::make_shared(disk); + disk->startup(context, skip_access_check); + return std::make_shared(disk, skip_access_check); }; factory.registerDiskType("local", creator); } diff --git a/src/Disks/DiskLocal.h b/src/Disks/DiskLocal.h index f79647b8541..85fb8d7aea7 100644 --- a/src/Disks/DiskLocal.h +++ b/src/Disks/DiskLocal.h @@ -28,8 +28,6 @@ public: ContextPtr context, UInt64 local_disk_check_period_ms); - const String & getName() const override { return name; } - const String & getPath() const override { return disk_path; } ReservationPtr reserve(UInt64 bytes) override; @@ -113,7 +111,7 @@ public: bool isBroken() const override { return broken; } - void startup(ContextPtr) override; + void startupImpl(ContextPtr) override; void shutdown() override; @@ -143,7 +141,6 @@ private: /// Read magic number from disk checker file. Return std::nullopt if exception happens. std::optional readDiskCheckerMagicNumber() const noexcept; - const String name; const String disk_path; const String disk_checker_path = ".disk_checker_file"; std::atomic keep_free_space_bytes; diff --git a/src/Disks/DiskMemory.cpp b/src/Disks/DiskMemory.cpp index f4ca2a7459a..84dfa1aa37b 100644 --- a/src/Disks/DiskMemory.cpp +++ b/src/Disks/DiskMemory.cpp @@ -141,6 +141,11 @@ private: }; +DiskMemory::DiskMemory(const String & name_) + : IDisk(name_) + , disk_path("memory://" + name_ + '/') +{} + ReservationPtr DiskMemory::reserve(UInt64 /*bytes*/) { throw Exception("Method reserve is not implemented for memory disks", ErrorCodes::NOT_IMPLEMENTED); @@ -459,10 +464,16 @@ using DiskMemoryPtr = std::shared_ptr; void registerDiskMemory(DiskFactory & factory) { auto creator = [](const String & name, - const Poco::Util::AbstractConfiguration & /*config*/, - const String & /*config_prefix*/, - ContextPtr /*context*/, - const DisksMap & /*map*/) -> DiskPtr { return std::make_shared(name); }; + const Poco::Util::AbstractConfiguration & config, + const String & config_prefix, + ContextPtr context, + const DisksMap & /*map*/) -> DiskPtr + { + bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); + DiskPtr disk = std::make_shared(name); + disk->startup(context, skip_access_check); + return disk; + }; factory.registerDiskType("memory", creator); } diff --git a/src/Disks/DiskMemory.h b/src/Disks/DiskMemory.h index 78fb52a768d..624b89f6a50 100644 --- a/src/Disks/DiskMemory.h +++ b/src/Disks/DiskMemory.h @@ -8,7 +8,7 @@ namespace DB { -class DiskMemory; + class ReadBufferFromFileBase; class WriteBufferFromFileBase; @@ -22,9 +22,7 @@ class WriteBufferFromFileBase; class DiskMemory : public IDisk { public: - explicit DiskMemory(const String & name_) : name(name_), disk_path("memory://" + name_ + '/') {} - - const String & getName() const override { return name; } + explicit DiskMemory(const String & name_); const String & getPath() const override { return disk_path; } @@ -121,7 +119,6 @@ private: }; using Files = std::unordered_map; /// file path -> file data - const String name; const String disk_path; Files files; mutable std::mutex mutex; diff --git a/src/Disks/DiskRestartProxy.cpp b/src/Disks/DiskRestartProxy.cpp index 2d923d71622..e8148703014 100644 --- a/src/Disks/DiskRestartProxy.cpp +++ b/src/Disks/DiskRestartProxy.cpp @@ -78,8 +78,10 @@ private: ReadLock lock; }; -DiskRestartProxy::DiskRestartProxy(DiskPtr & delegate_) - : DiskDecorator(delegate_) { } +DiskRestartProxy::DiskRestartProxy(DiskPtr & delegate_, bool skip_access_check_) + : DiskDecorator(delegate_) + , skip_access_check(skip_access_check_) +{} ReservationPtr DiskRestartProxy::reserve(UInt64 bytes) { @@ -368,7 +370,7 @@ void DiskRestartProxy::restart(ContextPtr context) LOG_INFO(log, "Restart lock acquired. Restarting disk {}", DiskDecorator::getName()); - DiskDecorator::startup(context); + DiskDecorator::startup(context, skip_access_check); LOG_INFO(log, "Disk restarted {}", DiskDecorator::getName()); } diff --git a/src/Disks/DiskRestartProxy.h b/src/Disks/DiskRestartProxy.h index fb4dde3bfa3..d74d1dd718d 100644 --- a/src/Disks/DiskRestartProxy.h +++ b/src/Disks/DiskRestartProxy.h @@ -21,7 +21,7 @@ class RestartAwareWriteBuffer; class DiskRestartProxy : public DiskDecorator { public: - explicit DiskRestartProxy(DiskPtr & delegate_); + explicit DiskRestartProxy(DiskPtr & delegate_, bool skip_access_check_); ReservationPtr reserve(UInt64 bytes) override; const String & getPath() const override; @@ -79,6 +79,7 @@ private: /// Mutex to protect RW access. mutable std::shared_timed_mutex mutex; + bool skip_access_check; Poco::Logger * log = &Poco::Logger::get("DiskRestartProxy"); }; diff --git a/src/Disks/IDisk.cpp b/src/Disks/IDisk.cpp index 8a6bea2565b..42208eb0260 100644 --- a/src/Disks/IDisk.cpp +++ b/src/Disks/IDisk.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -17,6 +18,8 @@ namespace DB namespace ErrorCodes { extern const int NOT_IMPLEMENTED; + extern const int CANNOT_READ_ALL_DATA; + extern const int LOGICAL_ERROR; } bool IDisk::isDirectoryEmpty(const String & path) const @@ -126,4 +129,74 @@ SyncGuardPtr IDisk::getDirectorySyncGuard(const String & /* path */) const return nullptr; } +void IDisk::checkAccess() +try +{ + if (isReadOnly()) + { + LOG_DEBUG(&Poco::Logger::get("IDisk"), + "Skip access check for disk {} (read-only disk).", + getName()); + return; + } + + DB::UUID server_uuid = DB::ServerUUID::get(); + if (server_uuid == DB::UUIDHelpers::Nil) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Server UUID is not initialized"); + const String path = fmt::format("clickhouse_access_check_{}", DB::toString(server_uuid)); + const std::string_view payload("test", 4); + + /// NOTE: should we mark the disk readonly if the write/unlink fails instead of throws? + + /// write + { + auto file = writeFile(path, DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite); + try + { + file->write(payload.data(), payload.size()); + } + catch (...) + { + /// Log current exception, because finalize() can throw a different exception. + tryLogCurrentException(__PRETTY_FUNCTION__); + file->finalize(); + throw; + } + } + + /// read + { + auto file = readFile(path); + String buf(payload.size(), '0'); + file->readStrict(buf.data(), buf.size()); + if (buf != payload) + { + throw Exception(ErrorCodes::CANNOT_READ_ALL_DATA, + "Content of {}::{} does not matches after read ({} vs {})", name, path, buf, payload); + } + } + + /// read with offset + { + auto file = readFile(path); + auto offset = 2; + String buf(payload.size() - offset, '0'); + file->seek(offset, 0); + file->readStrict(buf.data(), buf.size()); + if (buf != payload.substr(offset)) + { + throw Exception(ErrorCodes::CANNOT_READ_ALL_DATA, + "Content of {}::{} does not matches after read with offset ({} vs {})", name, path, buf, payload.substr(offset)); + } + } + + /// remove + removeFile(path); +} +catch (Exception & e) +{ + e.addMessage(fmt::format("While checking access for disk {}", name)); + throw; +} + } diff --git a/src/Disks/IDisk.h b/src/Disks/IDisk.h index 59c11ef0b45..6d2fe0c149a 100644 --- a/src/Disks/IDisk.h +++ b/src/Disks/IDisk.h @@ -107,8 +107,9 @@ class IDisk : public Space { public: /// Default constructor. - explicit IDisk(std::shared_ptr executor_ = std::make_shared()) - : executor(executor_) + explicit IDisk(const String & name_, std::shared_ptr executor_ = std::make_shared()) + : name(name_) + , executor(executor_) { } @@ -121,6 +122,9 @@ public: /// It's not required to be a local filesystem path. virtual const String & getPath() const = 0; + /// Return disk name. + const String & getName() const override { return name; } + /// Total available space on the disk. virtual UInt64 getTotalSpace() const = 0; @@ -316,8 +320,14 @@ public: /// Invoked when Global Context is shutdown. virtual void shutdown() {} - /// Performs action on disk startup. - virtual void startup(ContextPtr) {} + void startup(ContextPtr context, bool skip_access_check) + { + if (!skip_access_check) + checkAccess(); + startupImpl(context); + } + /// Performs custom action on disk startup. + virtual void startupImpl(ContextPtr) {} /// Return some uniq string for file, overrode for IDiskRemote /// Required for distinguish different copies of the same part on remote disk @@ -408,8 +418,14 @@ protected: /// A derived class may override copy() to provide a faster implementation. void copyThroughBuffers(const String & from_path, const std::shared_ptr & to_disk, const String & to_path, bool copy_root_dir = true); +protected: + const String name; + private: std::shared_ptr executor; + + /// Check access to the disk. + void checkAccess(); }; using Disks = std::vector; diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp b/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp index 6a12d8ef2e8..a138dd6e566 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp +++ b/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp @@ -17,52 +17,6 @@ namespace DB { -namespace ErrorCodes -{ - extern const int PATH_ACCESS_DENIED; -} - -namespace -{ - -constexpr char test_file[] = "test.txt"; -constexpr char test_str[] = "test"; -constexpr size_t test_str_size = 4; - -void checkWriteAccess(IDisk & disk) -{ - auto file = disk.writeFile(test_file, DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite); - file->write(test_str, test_str_size); -} - -void checkReadAccess(IDisk & disk) -{ - auto file = disk.readFile(test_file); - String buf(test_str_size, '0'); - file->readStrict(buf.data(), test_str_size); - if (buf != test_str) - throw Exception("No read access to disk", ErrorCodes::PATH_ACCESS_DENIED); -} - -void checkReadWithOffset(IDisk & disk) -{ - auto file = disk.readFile(test_file); - auto offset = 2; - auto test_size = test_str_size - offset; - String buf(test_size, '0'); - file->seek(offset, 0); - file->readStrict(buf.data(), test_size); - if (buf != test_str + offset) - throw Exception("Failed to read file with offset", ErrorCodes::PATH_ACCESS_DENIED); -} - -void checkRemoveAccess(IDisk & disk) -{ - disk.removeFile(test_file); -} - -} - void registerDiskAzureBlobStorage(DiskFactory & factory) { auto creator = []( @@ -94,17 +48,10 @@ void registerDiskAzureBlobStorage(DiskFactory & factory) copy_thread_pool_size ); - if (!config.getBool(config_prefix + ".skip_access_check", false)) - { - checkWriteAccess(*azure_blob_storage_disk); - checkReadAccess(*azure_blob_storage_disk); - checkReadWithOffset(*azure_blob_storage_disk); - checkRemoveAccess(*azure_blob_storage_disk); - } + bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); + azure_blob_storage_disk->startup(context, skip_access_check); - azure_blob_storage_disk->startup(context); - - return std::make_shared(azure_blob_storage_disk); + return std::make_shared(azure_blob_storage_disk, skip_access_check); }; factory.registerDiskType("azure_blob_storage", creator); diff --git a/src/Disks/ObjectStorages/DiskObjectStorage.cpp b/src/Disks/ObjectStorages/DiskObjectStorage.cpp index de1097bb3ec..263a9a9d0e1 100644 --- a/src/Disks/ObjectStorages/DiskObjectStorage.cpp +++ b/src/Disks/ObjectStorages/DiskObjectStorage.cpp @@ -109,8 +109,7 @@ DiskObjectStorage::DiskObjectStorage( ObjectStoragePtr object_storage_, bool send_metadata_, uint64_t thread_pool_size_) - : IDisk(getAsyncExecutor(log_name, thread_pool_size_)) - , name(name_) + : IDisk(name_, getAsyncExecutor(log_name, thread_pool_size_)) , object_storage_root_path(object_storage_root_path_) , log (&Poco::Logger::get("DiskObjectStorage(" + log_name + ")")) , metadata_storage(std::move(metadata_storage_)) @@ -420,9 +419,8 @@ void DiskObjectStorage::shutdown() LOG_INFO(log, "Disk {} shut down", name); } -void DiskObjectStorage::startup(ContextPtr context) +void DiskObjectStorage::startupImpl(ContextPtr context) { - LOG_INFO(log, "Starting up disk {}", name); object_storage->startup(); diff --git a/src/Disks/ObjectStorages/DiskObjectStorage.h b/src/Disks/ObjectStorages/DiskObjectStorage.h index 1f09b79a208..00e3cf98142 100644 --- a/src/Disks/ObjectStorages/DiskObjectStorage.h +++ b/src/Disks/ObjectStorages/DiskObjectStorage.h @@ -45,8 +45,6 @@ public: bool supportParallelWrite() const override { return object_storage->supportParallelWrite(); } - const String & getName() const override { return name; } - const String & getPath() const override { return metadata_storage->getPath(); } StoredObjects getStorageObjects(const String & local_path) const override; @@ -138,7 +136,7 @@ public: void shutdown() override; - void startup(ContextPtr context) override; + void startupImpl(ContextPtr context) override; ReservationPtr reserve(UInt64 bytes) override; @@ -212,7 +210,6 @@ private: /// execution. DiskTransactionPtr createObjectStorageTransaction(); - const String name; const String object_storage_root_path; Poco::Logger * log; diff --git a/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp b/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp index a9189e0101b..1966d233f46 100644 --- a/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp +++ b/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp @@ -19,7 +19,7 @@ void registerDiskHDFS(DiskFactory & factory) auto creator = [](const String & name, const Poco::Util::AbstractConfiguration & config, const String & config_prefix, - ContextPtr context_, + ContextPtr context, const DisksMap & /*map*/) -> DiskPtr { String uri{config.getString(config_prefix + ".endpoint")}; @@ -31,19 +31,20 @@ void registerDiskHDFS(DiskFactory & factory) std::unique_ptr settings = std::make_unique( config.getUInt64(config_prefix + ".min_bytes_for_seek", 1024 * 1024), config.getInt(config_prefix + ".objects_chunk_size_to_delete", 1000), - context_->getSettingsRef().hdfs_replication + context->getSettingsRef().hdfs_replication ); /// FIXME Cache currently unsupported :( ObjectStoragePtr hdfs_storage = std::make_unique(uri, std::move(settings), config); - auto [_, metadata_disk] = prepareForLocalMetadata(name, config, config_prefix, context_); + auto [_, metadata_disk] = prepareForLocalMetadata(name, config, config_prefix, context); auto metadata_storage = std::make_shared(metadata_disk, uri); uint64_t copy_thread_pool_size = config.getUInt(config_prefix + ".thread_pool_size", 16); + bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); - DiskPtr disk_result = std::make_shared( + DiskPtr disk = std::make_shared( name, uri, "DiskHDFS", @@ -51,8 +52,9 @@ void registerDiskHDFS(DiskFactory & factory) std::move(hdfs_storage), /* send_metadata = */ false, copy_thread_pool_size); + disk->startup(context, skip_access_check); - return std::make_shared(disk_result); + return std::make_shared(disk, skip_access_check); }; factory.registerDiskType("hdfs", creator); diff --git a/src/Disks/ObjectStorages/S3/registerDiskS3.cpp b/src/Disks/ObjectStorages/S3/registerDiskS3.cpp index dbb9b51c26b..01c46996fec 100644 --- a/src/Disks/ObjectStorages/S3/registerDiskS3.cpp +++ b/src/Disks/ObjectStorages/S3/registerDiskS3.cpp @@ -31,7 +31,6 @@ namespace DB namespace ErrorCodes { extern const int BAD_ARGUMENTS; - extern const int PATH_ACCESS_DENIED; extern const int LOGICAL_ERROR; } @@ -41,39 +40,6 @@ namespace class CheckAccess { public: - static void check(const String & disk_name, IDisk & disk) - { - /// NOTE: you don't need to pass prefix here since the writes is done - /// with IDisk interface that will do this automatically. - const String path = fmt::format("clickhouse_access_check_{}", getServerUUID()); - - /// write - { - auto file = disk.writeFile(path, DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite); - try - { - file->write("test", 4); - } - catch (...) - { - /// Log current exception, because finalize() can throw a different exception. - tryLogCurrentException(__PRETTY_FUNCTION__); - file->finalize(); - throw; - } - } - - /// read - auto file = disk.readFile(path); - String buf(4, '0'); - file->readStrict(buf.data(), 4); - if (buf != "test") - throw Exception(ErrorCodes::PATH_ACCESS_DENIED, "No read access to S3 bucket in disk {}", disk_name); - - /// remove - disk.removeFile(path); - } - static bool checkBatchRemove(S3ObjectStorage & storage, const String & key_with_trailing_slash) { /// NOTE: key_with_trailing_slash is the disk prefix, it is required @@ -166,12 +132,11 @@ void registerDiskS3(DiskFactory & factory) metadata_storage = std::make_shared(metadata_disk, uri.key); } - CheckAccess access_checker; bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); if (!skip_access_check) { /// If `support_batch_delete` is turned on (default), check and possibly switch it off. - if (s3_capabilities.support_batch_delete && !access_checker.checkBatchRemove(*s3_storage, uri.key)) + if (s3_capabilities.support_batch_delete && !CheckAccess::checkBatchRemove(*s3_storage, uri.key)) { LOG_WARNING( &Poco::Logger::get("registerDiskS3"), @@ -187,7 +152,7 @@ void registerDiskS3(DiskFactory & factory) bool send_metadata = config.getBool(config_prefix + ".send_metadata", false); uint64_t copy_thread_pool_size = config.getUInt(config_prefix + ".thread_pool_size", 16); - std::shared_ptr s3disk = std::make_shared( + DiskObjectStoragePtr s3disk = std::make_shared( name, uri.key, type == "s3" ? "DiskS3" : "DiskS3Plain", @@ -196,17 +161,11 @@ void registerDiskS3(DiskFactory & factory) send_metadata, copy_thread_pool_size); - /// This code is used only to check access to the corresponding disk. - if (!skip_access_check) - { - access_checker.check(name, *s3disk); - } - - s3disk->startup(context); + s3disk->startup(context, skip_access_check); std::shared_ptr disk_result = s3disk; - return std::make_shared(disk_result); + return std::make_shared(disk_result, skip_access_check); }; factory.registerDiskType("s3", creator); factory.registerDiskType("s3_plain", creator); diff --git a/src/Disks/ObjectStorages/Web/registerDiskWebServer.cpp b/src/Disks/ObjectStorages/Web/registerDiskWebServer.cpp index 5ef3fad4a0a..d8daa7b44fc 100644 --- a/src/Disks/ObjectStorages/Web/registerDiskWebServer.cpp +++ b/src/Disks/ObjectStorages/Web/registerDiskWebServer.cpp @@ -23,6 +23,7 @@ void registerDiskWebServer(DiskFactory & factory) const DisksMap & /*map*/) -> DiskPtr { String uri{config.getString(config_prefix + ".endpoint")}; + bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); if (!uri.ends_with('/')) throw Exception( @@ -41,7 +42,7 @@ void registerDiskWebServer(DiskFactory & factory) auto metadata_storage = std::make_shared(assert_cast(*object_storage)); std::string root_path; - return std::make_shared( + DiskPtr disk = std::make_shared( disk_name, root_path, "DiskWebServer", @@ -49,6 +50,8 @@ void registerDiskWebServer(DiskFactory & factory) object_storage, /* send_metadata */false, /* threadpool_size */16); + disk->startup(context, skip_access_check); + return disk; }; factory.registerDiskType("web", creator); From 44f23c256825c14d10c8e13f45cf5dcc3338311b Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sat, 19 Nov 2022 09:09:24 +0100 Subject: [PATCH 48/65] Make disks checks only for clickhouse-server This will fix clickhouse-disks Signed-off-by: Azat Khuzhin --- programs/copier/ClusterCopierApp.cpp | 2 +- programs/disks/DisksApp.cpp | 2 +- programs/local/LocalServer.cpp | 2 +- programs/server/Server.cpp | 2 +- src/Disks/DiskEncrypted.cpp | 15 ++++---- src/Disks/DiskLocal.cpp | 15 ++++---- src/Disks/DiskMemory.cpp | 15 ++++---- .../registerDiskAzureBlobStorage.cpp | 8 ++--- .../Cached/registerDiskCache.cpp | 2 +- .../ObjectStorages/HDFS/registerDiskHDFS.cpp | 15 ++++---- .../ObjectStorages/S3/registerDiskS3.cpp | 18 +++++----- .../Web/registerDiskWebServer.cpp | 15 ++++---- src/Disks/registerDisks.cpp | 34 +++++++++---------- src/Disks/registerDisks.h | 7 +++- .../fuzzers/execute_query_fuzzer.cpp | 2 +- 15 files changed, 83 insertions(+), 71 deletions(-) diff --git a/programs/copier/ClusterCopierApp.cpp b/programs/copier/ClusterCopierApp.cpp index a0d5344236e..bd505b319bb 100644 --- a/programs/copier/ClusterCopierApp.cpp +++ b/programs/copier/ClusterCopierApp.cpp @@ -160,7 +160,7 @@ void ClusterCopierApp::mainImpl() registerTableFunctions(); registerStorages(); registerDictionaries(); - registerDisks(); + registerDisks(/* global_skip_access_check= */ true); registerFormats(); static const std::string default_database = "_local"; diff --git a/programs/disks/DisksApp.cpp b/programs/disks/DisksApp.cpp index 91472a8df33..ea46bb1ba8d 100644 --- a/programs/disks/DisksApp.cpp +++ b/programs/disks/DisksApp.cpp @@ -176,7 +176,7 @@ int DisksApp::main(const std::vector & /*args*/) Poco::Logger::root().setLevel(Poco::Logger::parseLevel(log_level)); } - registerDisks(); + registerDisks(/* global_skip_access_check= */ true); registerFormats(); shared_context = Context::createShared(); diff --git a/programs/local/LocalServer.cpp b/programs/local/LocalServer.cpp index 4c07fa0a02d..ce7e27026f1 100644 --- a/programs/local/LocalServer.cpp +++ b/programs/local/LocalServer.cpp @@ -413,7 +413,7 @@ try registerTableFunctions(); registerStorages(); registerDictionaries(); - registerDisks(); + registerDisks(/* global_skip_access_check= */ true); registerFormats(); processConfig(); diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index a5321997779..d2ad9b04993 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -679,7 +679,7 @@ int Server::main(const std::vector & /*args*/) registerTableFunctions(); registerStorages(); registerDictionaries(); - registerDisks(); + registerDisks(/* global_skip_access_check= */ false); registerFormats(); registerRemoteFileMetadatas(); diff --git a/src/Disks/DiskEncrypted.cpp b/src/Disks/DiskEncrypted.cpp index 50df990269e..79905283ddb 100644 --- a/src/Disks/DiskEncrypted.cpp +++ b/src/Disks/DiskEncrypted.cpp @@ -369,15 +369,16 @@ void DiskEncrypted::applyNewSettings( current_settings.set(std::move(new_settings)); } -void registerDiskEncrypted(DiskFactory & factory) +void registerDiskEncrypted(DiskFactory & factory, bool global_skip_access_check) { - auto creator = [](const String & name, - const Poco::Util::AbstractConfiguration & config, - const String & config_prefix, - ContextPtr context, - const DisksMap & map) -> DiskPtr + auto creator = [global_skip_access_check]( + const String & name, + const Poco::Util::AbstractConfiguration & config, + const String & config_prefix, + ContextPtr context, + const DisksMap & map) -> DiskPtr { - bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); + bool skip_access_check = global_skip_access_check || config.getBool(config_prefix + ".skip_access_check", false); DiskPtr disk = std::make_shared(name, config, config_prefix, map); disk->startup(context, skip_access_check); return disk; diff --git a/src/Disks/DiskLocal.cpp b/src/Disks/DiskLocal.cpp index b6c078611bd..ce7c6320d39 100644 --- a/src/Disks/DiskLocal.cpp +++ b/src/Disks/DiskLocal.cpp @@ -741,13 +741,14 @@ MetadataStoragePtr DiskLocal::getMetadataStorage() std::static_pointer_cast(shared_from_this()), object_storage, getPath()); } -void registerDiskLocal(DiskFactory & factory) +void registerDiskLocal(DiskFactory & factory, bool global_skip_access_check) { - auto creator = [](const String & name, - const Poco::Util::AbstractConfiguration & config, - const String & config_prefix, - ContextPtr context, - const DisksMap & map) -> DiskPtr + auto creator = [global_skip_access_check]( + const String & name, + const Poco::Util::AbstractConfiguration & config, + const String & config_prefix, + ContextPtr context, + const DisksMap & map) -> DiskPtr { String path; UInt64 keep_free_space_bytes; @@ -757,7 +758,7 @@ void registerDiskLocal(DiskFactory & factory) if (path == disk_ptr->getPath()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Disk {} and disk {} cannot have the same path ({})", name, disk_name, path); - bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); + bool skip_access_check = global_skip_access_check || config.getBool(config_prefix + ".skip_access_check", false); std::shared_ptr disk = std::make_shared(name, path, keep_free_space_bytes, context, config.getUInt("local_disk_check_period_ms", 0)); disk->startup(context, skip_access_check); diff --git a/src/Disks/DiskMemory.cpp b/src/Disks/DiskMemory.cpp index 84dfa1aa37b..289a95a312e 100644 --- a/src/Disks/DiskMemory.cpp +++ b/src/Disks/DiskMemory.cpp @@ -461,15 +461,16 @@ MetadataStoragePtr DiskMemory::getMetadataStorage() using DiskMemoryPtr = std::shared_ptr; -void registerDiskMemory(DiskFactory & factory) +void registerDiskMemory(DiskFactory & factory, bool global_skip_access_check) { - auto creator = [](const String & name, - const Poco::Util::AbstractConfiguration & config, - const String & config_prefix, - ContextPtr context, - const DisksMap & /*map*/) -> DiskPtr + auto creator = [global_skip_access_check]( + const String & name, + const Poco::Util::AbstractConfiguration & config, + const String & config_prefix, + ContextPtr context, + const DisksMap & /*map*/) -> DiskPtr { - bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); + bool skip_access_check = global_skip_access_check || config.getBool(config_prefix + ".skip_access_check", false); DiskPtr disk = std::make_shared(name); disk->startup(context, skip_access_check); return disk; diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp b/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp index a138dd6e566..ca3f4aba063 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp +++ b/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp @@ -17,9 +17,9 @@ namespace DB { -void registerDiskAzureBlobStorage(DiskFactory & factory) +void registerDiskAzureBlobStorage(DiskFactory & factory, bool global_skip_access_check) { - auto creator = []( + auto creator = [global_skip_access_check]( const String & name, const Poco::Util::AbstractConfiguration & config, const String & config_prefix, @@ -48,7 +48,7 @@ void registerDiskAzureBlobStorage(DiskFactory & factory) copy_thread_pool_size ); - bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); + bool skip_access_check = global_skip_access_check || config.getBool(config_prefix + ".skip_access_check", false); azure_blob_storage_disk->startup(context, skip_access_check); return std::make_shared(azure_blob_storage_disk, skip_access_check); @@ -64,7 +64,7 @@ void registerDiskAzureBlobStorage(DiskFactory & factory) namespace DB { -void registerDiskAzureBlobStorage(DiskFactory &) {} +void registerDiskAzureBlobStorage(DiskFactory &, bool /* global_skip_access_check */) {} } diff --git a/src/Disks/ObjectStorages/Cached/registerDiskCache.cpp b/src/Disks/ObjectStorages/Cached/registerDiskCache.cpp index 902ebd0fcc8..d8c4a9d42fd 100644 --- a/src/Disks/ObjectStorages/Cached/registerDiskCache.cpp +++ b/src/Disks/ObjectStorages/Cached/registerDiskCache.cpp @@ -16,7 +16,7 @@ namespace ErrorCodes extern const int BAD_ARGUMENTS; } -void registerDiskCache(DiskFactory & factory) +void registerDiskCache(DiskFactory & factory, bool /* global_skip_access_check */) { auto creator = [](const String & name, const Poco::Util::AbstractConfiguration & config, diff --git a/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp b/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp index 1966d233f46..891a85f5bb0 100644 --- a/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp +++ b/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp @@ -14,13 +14,14 @@ namespace ErrorCodes extern const int BAD_ARGUMENTS; } -void registerDiskHDFS(DiskFactory & factory) +void registerDiskHDFS(DiskFactory & factory, bool global_skip_access_check) { - auto creator = [](const String & name, - const Poco::Util::AbstractConfiguration & config, - const String & config_prefix, - ContextPtr context, - const DisksMap & /*map*/) -> DiskPtr + auto creator = [global_skip_access_check]( + const String & name, + const Poco::Util::AbstractConfiguration & config, + const String & config_prefix, + ContextPtr context, + const DisksMap & /*map*/) -> DiskPtr { String uri{config.getString(config_prefix + ".endpoint")}; checkHDFSURL(uri); @@ -42,7 +43,7 @@ void registerDiskHDFS(DiskFactory & factory) auto metadata_storage = std::make_shared(metadata_disk, uri); uint64_t copy_thread_pool_size = config.getUInt(config_prefix + ".thread_pool_size", 16); - bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); + bool skip_access_check = global_skip_access_check || config.getBool(config_prefix + ".skip_access_check", false); DiskPtr disk = std::make_shared( name, diff --git a/src/Disks/ObjectStorages/S3/registerDiskS3.cpp b/src/Disks/ObjectStorages/S3/registerDiskS3.cpp index 01c46996fec..7d03c6cf5d6 100644 --- a/src/Disks/ObjectStorages/S3/registerDiskS3.cpp +++ b/src/Disks/ObjectStorages/S3/registerDiskS3.cpp @@ -95,13 +95,14 @@ private: } -void registerDiskS3(DiskFactory & factory) +void registerDiskS3(DiskFactory & factory, bool global_skip_access_check) { - auto creator = [](const String & name, - const Poco::Util::AbstractConfiguration & config, - const String & config_prefix, - ContextPtr context, - const DisksMap & /*map*/) -> DiskPtr + auto creator = [global_skip_access_check]( + const String & name, + const Poco::Util::AbstractConfiguration & config, + const String & config_prefix, + ContextPtr context, + const DisksMap & /*map*/) -> DiskPtr { S3::URI uri(Poco::URI(config.getString(config_prefix + ".endpoint"))); @@ -132,7 +133,8 @@ void registerDiskS3(DiskFactory & factory) metadata_storage = std::make_shared(metadata_disk, uri.key); } - bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); + /// NOTE: should we still perform this check for clickhouse-disks? + bool skip_access_check = global_skip_access_check || config.getBool(config_prefix + ".skip_access_check", false); if (!skip_access_check) { /// If `support_batch_delete` is turned on (default), check and possibly switch it off. @@ -175,6 +177,6 @@ void registerDiskS3(DiskFactory & factory) #else -void registerDiskS3(DiskFactory &) {} +void registerDiskS3(DiskFactory &, bool /* global_skip_access_check */) {} #endif diff --git a/src/Disks/ObjectStorages/Web/registerDiskWebServer.cpp b/src/Disks/ObjectStorages/Web/registerDiskWebServer.cpp index d8daa7b44fc..253d32ceb14 100644 --- a/src/Disks/ObjectStorages/Web/registerDiskWebServer.cpp +++ b/src/Disks/ObjectStorages/Web/registerDiskWebServer.cpp @@ -14,16 +14,17 @@ namespace ErrorCodes extern const int BAD_ARGUMENTS; } -void registerDiskWebServer(DiskFactory & factory) +void registerDiskWebServer(DiskFactory & factory, bool global_skip_access_check) { - auto creator = [](const String & disk_name, - const Poco::Util::AbstractConfiguration & config, - const String & config_prefix, - ContextPtr context, - const DisksMap & /*map*/) -> DiskPtr + auto creator = [global_skip_access_check]( + const String & disk_name, + const Poco::Util::AbstractConfiguration & config, + const String & config_prefix, + ContextPtr context, + const DisksMap & /*map*/) -> DiskPtr { String uri{config.getString(config_prefix + ".endpoint")}; - bool skip_access_check = config.getBool(config_prefix + ".skip_access_check", false); + bool skip_access_check = global_skip_access_check || config.getBool(config_prefix + ".skip_access_check", false); if (!uri.ends_with('/')) throw Exception( diff --git a/src/Disks/registerDisks.cpp b/src/Disks/registerDisks.cpp index 54ad74d47b5..bc677438243 100644 --- a/src/Disks/registerDisks.cpp +++ b/src/Disks/registerDisks.cpp @@ -7,55 +7,55 @@ namespace DB { -void registerDiskLocal(DiskFactory & factory); -void registerDiskMemory(DiskFactory & factory); +void registerDiskLocal(DiskFactory & factory, bool global_skip_access_check); +void registerDiskMemory(DiskFactory & factory, bool global_skip_access_check); #if USE_AWS_S3 -void registerDiskS3(DiskFactory & factory); +void registerDiskS3(DiskFactory & factory, bool global_skip_access_check); #endif #if USE_AZURE_BLOB_STORAGE -void registerDiskAzureBlobStorage(DiskFactory & factory); +void registerDiskAzureBlobStorage(DiskFactory & factory, bool global_skip_access_check); #endif #if USE_SSL -void registerDiskEncrypted(DiskFactory & factory); +void registerDiskEncrypted(DiskFactory & factory, bool global_skip_access_check); #endif #if USE_HDFS -void registerDiskHDFS(DiskFactory & factory); +void registerDiskHDFS(DiskFactory & factory, bool global_skip_access_check); #endif -void registerDiskWebServer(DiskFactory & factory); +void registerDiskWebServer(DiskFactory & factory, bool global_skip_access_check); -void registerDiskCache(DiskFactory & factory); +void registerDiskCache(DiskFactory & factory, bool global_skip_access_check); -void registerDisks() +void registerDisks(bool global_skip_access_check) { auto & factory = DiskFactory::instance(); - registerDiskLocal(factory); - registerDiskMemory(factory); + registerDiskLocal(factory, global_skip_access_check); + registerDiskMemory(factory, global_skip_access_check); #if USE_AWS_S3 - registerDiskS3(factory); + registerDiskS3(factory, global_skip_access_check); #endif #if USE_AZURE_BLOB_STORAGE - registerDiskAzureBlobStorage(factory); + registerDiskAzureBlobStorage(factory, global_skip_access_check); #endif #if USE_SSL - registerDiskEncrypted(factory); + registerDiskEncrypted(factory, global_skip_access_check); #endif #if USE_HDFS - registerDiskHDFS(factory); + registerDiskHDFS(factory, global_skip_access_check); #endif - registerDiskWebServer(factory); + registerDiskWebServer(factory, global_skip_access_check); - registerDiskCache(factory); + registerDiskCache(factory, global_skip_access_check); } } diff --git a/src/Disks/registerDisks.h b/src/Disks/registerDisks.h index 8c68cc52bde..1658f18f86b 100644 --- a/src/Disks/registerDisks.h +++ b/src/Disks/registerDisks.h @@ -2,5 +2,10 @@ namespace DB { -void registerDisks(); + +/// @param global_skip_access_check - skip access check regardless regardless +/// .skip_access_check config directive (used +/// for clickhouse-disks) +void registerDisks(bool global_skip_access_check); + } diff --git a/src/Interpreters/fuzzers/execute_query_fuzzer.cpp b/src/Interpreters/fuzzers/execute_query_fuzzer.cpp index f843c4880df..30db25668cf 100644 --- a/src/Interpreters/fuzzers/execute_query_fuzzer.cpp +++ b/src/Interpreters/fuzzers/execute_query_fuzzer.cpp @@ -33,7 +33,7 @@ try registerTableFunctions(); registerStorages(); registerDictionaries(); - registerDisks(); + registerDisks(/* global_skip_access_check= */ true); registerFormats(); return true; From dddcca5cc13dd7dab440e6686d035a309b91aa29 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sat, 19 Nov 2022 10:05:29 +0100 Subject: [PATCH 49/65] Fix deadlock in DiskRestartProxy on disk restart stacktrace: contrib/libcxx/src/condition_variable.cpp:47::std::__1::condition_variable::wait(std::__1::unique_lock&) contrib/libcxx/src/shared_mutex.cpp:65::std::__1::shared_timed_mutex::lock_shared() src/Disks/DiskRestartProxy.cpp:229::DB::DiskRestartProxy::writeFile(std::__1::basic_string, std::__1::allocator> const&, unsigned long, DB::WriteMode, DB::WriteSettings const&) src/Disks/IDisk.cpp:0::DB::IDisk::checkAccessImpl(std::__1::basic_string, std::__1::allocator> const&) contrib/libcxx/include/string:1499::DB::IDisk::checkAccess() src/Disks/IDisk.cpp:0::DB::IDisk::startup(std::__1::shared_ptr, bool) src/Disks/DiskRestartProxy.cpp:375::DB::DiskRestartProxy::restart(std::__1::shared_ptr) contrib/libcxx/include/__memory/shared_ptr.h:701::DB::InterpreterSystemQuery::restartDisk(std::__1::basic_string, std::__1::allocator>&) src/Interpreters/InterpreterSystemQuery.cpp:508::DB::InterpreterSystemQuery::execute() src/Interpreters/executeQuery.cpp:0::DB::executeQueryImpl(char const*, char const*, std::__1::shared_ptr, bool, DB::QueryProcessingStage::Enum, DB::ReadBuffer*) src/Interpreters/executeQuery.cpp:1083::DB::executeQuery(std::__1::basic_string, std::__1::allocator> const&, std::__1::shared_ptr, bool, DB::QueryProcessingStage::Enum) src/Server/TCPHandler.cpp:0::DB::TCPHandler::runImpl() src/Server/TCPHandler.cpp:1904::DB::TCPHandler::run() contrib/poco/Net/src/TCPServerConnection.cpp:57::Poco::Net::TCPServerConnection::start() contrib/libcxx/include/__memory/unique_ptr.h:48::Poco::Net::TCPServerDispatcher::run() contrib/poco/Foundation/src/ThreadPool.cpp:213::Poco::PooledThread::run() contrib/poco/Foundation/include/Poco/SharedPtr.h:156::Poco::ThreadImpl::runnableEntry(void*) Signed-off-by: Azat Khuzhin --- src/Disks/DiskLocal.cpp | 2 +- src/Disks/DiskRestartProxy.cpp | 6 +++--- src/Disks/DiskRestartProxy.h | 3 +-- .../AzureBlobStorage/registerDiskAzureBlobStorage.cpp | 2 +- src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp | 2 +- src/Disks/ObjectStorages/S3/registerDiskS3.cpp | 2 +- 6 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/Disks/DiskLocal.cpp b/src/Disks/DiskLocal.cpp index ce7c6320d39..ee93a80cded 100644 --- a/src/Disks/DiskLocal.cpp +++ b/src/Disks/DiskLocal.cpp @@ -762,7 +762,7 @@ void registerDiskLocal(DiskFactory & factory, bool global_skip_access_check) std::shared_ptr disk = std::make_shared(name, path, keep_free_space_bytes, context, config.getUInt("local_disk_check_period_ms", 0)); disk->startup(context, skip_access_check); - return std::make_shared(disk, skip_access_check); + return std::make_shared(disk); }; factory.registerDiskType("local", creator); } diff --git a/src/Disks/DiskRestartProxy.cpp b/src/Disks/DiskRestartProxy.cpp index e8148703014..0b79ee51db9 100644 --- a/src/Disks/DiskRestartProxy.cpp +++ b/src/Disks/DiskRestartProxy.cpp @@ -78,9 +78,8 @@ private: ReadLock lock; }; -DiskRestartProxy::DiskRestartProxy(DiskPtr & delegate_, bool skip_access_check_) +DiskRestartProxy::DiskRestartProxy(DiskPtr & delegate_) : DiskDecorator(delegate_) - , skip_access_check(skip_access_check_) {} ReservationPtr DiskRestartProxy::reserve(UInt64 bytes) @@ -370,7 +369,8 @@ void DiskRestartProxy::restart(ContextPtr context) LOG_INFO(log, "Restart lock acquired. Restarting disk {}", DiskDecorator::getName()); - DiskDecorator::startup(context, skip_access_check); + /// NOTE: access checking will cause deadlock here, so skip it. + DiskDecorator::startup(context, /* skip_access_check= */ true); LOG_INFO(log, "Disk restarted {}", DiskDecorator::getName()); } diff --git a/src/Disks/DiskRestartProxy.h b/src/Disks/DiskRestartProxy.h index d74d1dd718d..fb4dde3bfa3 100644 --- a/src/Disks/DiskRestartProxy.h +++ b/src/Disks/DiskRestartProxy.h @@ -21,7 +21,7 @@ class RestartAwareWriteBuffer; class DiskRestartProxy : public DiskDecorator { public: - explicit DiskRestartProxy(DiskPtr & delegate_, bool skip_access_check_); + explicit DiskRestartProxy(DiskPtr & delegate_); ReservationPtr reserve(UInt64 bytes) override; const String & getPath() const override; @@ -79,7 +79,6 @@ private: /// Mutex to protect RW access. mutable std::shared_timed_mutex mutex; - bool skip_access_check; Poco::Logger * log = &Poco::Logger::get("DiskRestartProxy"); }; diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp b/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp index ca3f4aba063..df377cdf710 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp +++ b/src/Disks/ObjectStorages/AzureBlobStorage/registerDiskAzureBlobStorage.cpp @@ -51,7 +51,7 @@ void registerDiskAzureBlobStorage(DiskFactory & factory, bool global_skip_access bool skip_access_check = global_skip_access_check || config.getBool(config_prefix + ".skip_access_check", false); azure_blob_storage_disk->startup(context, skip_access_check); - return std::make_shared(azure_blob_storage_disk, skip_access_check); + return std::make_shared(azure_blob_storage_disk); }; factory.registerDiskType("azure_blob_storage", creator); diff --git a/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp b/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp index 891a85f5bb0..7bec0ee5a6c 100644 --- a/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp +++ b/src/Disks/ObjectStorages/HDFS/registerDiskHDFS.cpp @@ -55,7 +55,7 @@ void registerDiskHDFS(DiskFactory & factory, bool global_skip_access_check) copy_thread_pool_size); disk->startup(context, skip_access_check); - return std::make_shared(disk, skip_access_check); + return std::make_shared(disk); }; factory.registerDiskType("hdfs", creator); diff --git a/src/Disks/ObjectStorages/S3/registerDiskS3.cpp b/src/Disks/ObjectStorages/S3/registerDiskS3.cpp index 7d03c6cf5d6..76f2e5994fb 100644 --- a/src/Disks/ObjectStorages/S3/registerDiskS3.cpp +++ b/src/Disks/ObjectStorages/S3/registerDiskS3.cpp @@ -167,7 +167,7 @@ void registerDiskS3(DiskFactory & factory, bool global_skip_access_check) std::shared_ptr disk_result = s3disk; - return std::make_shared(disk_result, skip_access_check); + return std::make_shared(disk_result); }; factory.registerDiskType("s3", creator); factory.registerDiskType("s3_plain", creator); From 11be9b9ad16f5049f8729fa30be6abe6d712038b Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Fri, 18 Nov 2022 21:48:50 +0100 Subject: [PATCH 50/65] Create disk directory before access check for local disk Signed-off-by: Azat Khuzhin --- src/Disks/DiskLocal.cpp | 52 ++++++++++++++++++++++++----------------- src/Disks/DiskLocal.h | 5 +++- src/Disks/IDisk.cpp | 31 +++++++++++++++++------- src/Disks/IDisk.h | 14 +++++------ 4 files changed, 62 insertions(+), 40 deletions(-) diff --git a/src/Disks/DiskLocal.cpp b/src/Disks/DiskLocal.cpp index ee93a80cded..c3428f54073 100644 --- a/src/Disks/DiskLocal.cpp +++ b/src/Disks/DiskLocal.cpp @@ -528,26 +528,6 @@ DataSourceDescription DiskLocal::getDataSourceDescription() const return data_source_description; } -void DiskLocal::startupImpl(ContextPtr) -{ - try - { - broken = false; - disk_checker_magic_number = -1; - disk_checker_can_check_read = true; - readonly = !setup(); - } - catch (...) - { - tryLogCurrentException(logger, fmt::format("Disk {} is marked as broken during startup", name)); - broken = true; - /// Disk checker is disabled when failing to start up. - disk_checker_can_check_read = false; - } - if (disk_checker && disk_checker_can_check_read) - disk_checker->startup(); -} - void DiskLocal::shutdown() { if (disk_checker) @@ -641,7 +621,7 @@ DiskObjectStoragePtr DiskLocal::createDiskObjectStorage() ); } -bool DiskLocal::setup() +void DiskLocal::checkAccessImpl(const String & path) { try { @@ -650,9 +630,15 @@ bool DiskLocal::setup() catch (...) { LOG_ERROR(logger, "Cannot create the directory of disk {} ({}).", name, disk_path); - throw; + readonly = true; + return; } + IDisk::checkAccessImpl(path); +} + +bool DiskLocal::setup() +{ try { if (!FS::canRead(disk_path)) @@ -717,6 +703,28 @@ bool DiskLocal::setup() return true; } +void DiskLocal::startupImpl(ContextPtr) +{ + broken = false; + disk_checker_magic_number = -1; + disk_checker_can_check_read = true; + + try + { + if (!setup()) + readonly = true; + } + catch (...) + { + tryLogCurrentException(logger, fmt::format("Disk {} is marked as broken during startup", name)); + broken = true; + /// Disk checker is disabled when failing to start up. + disk_checker_can_check_read = false; + } + if (disk_checker && disk_checker_can_check_read) + disk_checker->startup(); +} + struct stat DiskLocal::stat(const String & path) const { struct stat st; diff --git a/src/Disks/DiskLocal.h b/src/Disks/DiskLocal.h index 85fb8d7aea7..9e22aecdc8b 100644 --- a/src/Disks/DiskLocal.h +++ b/src/Disks/DiskLocal.h @@ -111,7 +111,7 @@ public: bool isBroken() const override { return broken; } - void startupImpl(ContextPtr) override; + void startupImpl(ContextPtr context) override; void shutdown() override; @@ -131,6 +131,9 @@ public: MetadataStoragePtr getMetadataStorage() override; +protected: + void checkAccessImpl(const String & path) override; + private: std::optional tryReserve(UInt64 bytes); diff --git a/src/Disks/IDisk.cpp b/src/Disks/IDisk.cpp index 42208eb0260..2a60f32929c 100644 --- a/src/Disks/IDisk.cpp +++ b/src/Disks/IDisk.cpp @@ -129,24 +129,37 @@ SyncGuardPtr IDisk::getDirectorySyncGuard(const String & /* path */) const return nullptr; } -void IDisk::checkAccess() -try +void IDisk::startup(ContextPtr context, bool skip_access_check) { - if (isReadOnly()) + if (!skip_access_check) { - LOG_DEBUG(&Poco::Logger::get("IDisk"), - "Skip access check for disk {} (read-only disk).", - getName()); - return; + if (isReadOnly()) + { + LOG_DEBUG(&Poco::Logger::get("IDisk"), + "Skip access check for disk {} (read-only disk).", + getName()); + } + else + checkAccess(); } + startupImpl(context); +} +void IDisk::checkAccess() +{ DB::UUID server_uuid = DB::ServerUUID::get(); if (server_uuid == DB::UUIDHelpers::Nil) throw Exception(ErrorCodes::LOGICAL_ERROR, "Server UUID is not initialized"); const String path = fmt::format("clickhouse_access_check_{}", DB::toString(server_uuid)); - const std::string_view payload("test", 4); - /// NOTE: should we mark the disk readonly if the write/unlink fails instead of throws? + checkAccessImpl(path); +} + +/// NOTE: should we mark the disk readonly if the write/unlink fails instead of throws? +void IDisk::checkAccessImpl(const String & path) +try +{ + const std::string_view payload("test", 4); /// write { diff --git a/src/Disks/IDisk.h b/src/Disks/IDisk.h index 6d2fe0c149a..381c491470f 100644 --- a/src/Disks/IDisk.h +++ b/src/Disks/IDisk.h @@ -320,12 +320,9 @@ public: /// Invoked when Global Context is shutdown. virtual void shutdown() {} - void startup(ContextPtr context, bool skip_access_check) - { - if (!skip_access_check) - checkAccess(); - startupImpl(context); - } + /// Performs access check and custom action on disk startup. + void startup(ContextPtr context, bool skip_access_check); + /// Performs custom action on disk startup. virtual void startupImpl(ContextPtr) {} @@ -410,6 +407,8 @@ public: protected: friend class DiskDecorator; + const String name; + /// Returns executor to perform asynchronous operations. virtual Executor & getExecutor() { return *executor; } @@ -418,8 +417,7 @@ protected: /// A derived class may override copy() to provide a faster implementation. void copyThroughBuffers(const String & from_path, const std::shared_ptr & to_disk, const String & to_path, bool copy_root_dir = true); -protected: - const String name; + virtual void checkAccessImpl(const String & path); private: std::shared_ptr executor; From 0678fba3d1ba7c237f6ca7670a590ef16ea15827 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Fri, 18 Nov 2022 22:10:15 +0100 Subject: [PATCH 51/65] tests: fix test_disk_types (by adding mkdir for HDFS) Signed-off-by: Azat Khuzhin --- tests/integration/test_disk_types/test.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/integration/test_disk_types/test.py b/tests/integration/test_disk_types/test.py index 099821bf494..d12a9b0c6d1 100644 --- a/tests/integration/test_disk_types/test.py +++ b/tests/integration/test_disk_types/test.py @@ -1,6 +1,7 @@ import pytest from helpers.cluster import ClickHouseCluster from helpers.test_tools import TSV +from pyhdfs import HdfsClient disk_types = { "default": "local", @@ -22,6 +23,10 @@ def cluster(): with_hdfs=True, ) cluster.start() + + fs = HdfsClient(hosts=cluster.hdfs_ip) + fs.mkdirs("/data") + yield cluster finally: cluster.shutdown() From 3e42ffd372061db8567e8a38d0782417a9ec36c1 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sat, 19 Nov 2022 20:37:57 +0100 Subject: [PATCH 52/65] tests: fix hdfs disks (chicken and an egg problem with current cluster.py) Right now cluster.py first creates HDFS and then creates clickhouse in one API call, so you cannot interract and add missing mkdirs for the clickhouse, fix this by using root dir where it is possible. --- tests/integration/test_disk_types/configs/storage.xml | 2 +- tests/integration/test_disk_types/test.py | 4 ---- .../test_log_family_hdfs/configs/storage_conf.xml | 2 ++ .../test_merge_tree_hdfs/configs/config.d/storage_conf.xml | 2 ++ .../configs/config.d/storage_conf.xml | 6 ++++++ 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/integration/test_disk_types/configs/storage.xml b/tests/integration/test_disk_types/configs/storage.xml index c55d589d19e..3c8f16a5226 100644 --- a/tests/integration/test_disk_types/configs/storage.xml +++ b/tests/integration/test_disk_types/configs/storage.xml @@ -12,7 +12,7 @@ hdfs - hdfs://hdfs1:9000/data/ + hdfs://hdfs1:9000/ encrypted diff --git a/tests/integration/test_disk_types/test.py b/tests/integration/test_disk_types/test.py index d12a9b0c6d1..683caf3497b 100644 --- a/tests/integration/test_disk_types/test.py +++ b/tests/integration/test_disk_types/test.py @@ -1,7 +1,6 @@ import pytest from helpers.cluster import ClickHouseCluster from helpers.test_tools import TSV -from pyhdfs import HdfsClient disk_types = { "default": "local", @@ -24,9 +23,6 @@ def cluster(): ) cluster.start() - fs = HdfsClient(hosts=cluster.hdfs_ip) - fs.mkdirs("/data") - yield cluster finally: cluster.shutdown() diff --git a/tests/integration/test_log_family_hdfs/configs/storage_conf.xml b/tests/integration/test_log_family_hdfs/configs/storage_conf.xml index 82cea6730ff..74270320508 100644 --- a/tests/integration/test_log_family_hdfs/configs/storage_conf.xml +++ b/tests/integration/test_log_family_hdfs/configs/storage_conf.xml @@ -4,6 +4,8 @@ hdfs hdfs://hdfs1:9000/clickhouse/ + + true diff --git a/tests/integration/test_merge_tree_hdfs/configs/config.d/storage_conf.xml b/tests/integration/test_merge_tree_hdfs/configs/config.d/storage_conf.xml index 7f816724c43..890c396ed95 100644 --- a/tests/integration/test_merge_tree_hdfs/configs/config.d/storage_conf.xml +++ b/tests/integration/test_merge_tree_hdfs/configs/config.d/storage_conf.xml @@ -4,6 +4,8 @@ hdfs hdfs://hdfs1:9000/clickhouse/ + + true local diff --git a/tests/integration/test_replicated_merge_tree_hdfs_zero_copy/configs/config.d/storage_conf.xml b/tests/integration/test_replicated_merge_tree_hdfs_zero_copy/configs/config.d/storage_conf.xml index 1b1ead2d7cb..cb444c728c9 100644 --- a/tests/integration/test_replicated_merge_tree_hdfs_zero_copy/configs/config.d/storage_conf.xml +++ b/tests/integration/test_replicated_merge_tree_hdfs_zero_copy/configs/config.d/storage_conf.xml @@ -4,14 +4,20 @@ hdfs hdfs://hdfs1:9000/clickhouse1/ + + true hdfs hdfs://hdfs1:9000/clickhouse1/ + + true hdfs hdfs://hdfs1:9000/clickhouse2/ + + true From 38c009214af06e1037ea728f1b5a87eaef3fe35a Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sun, 20 Nov 2022 16:12:19 +0100 Subject: [PATCH 53/65] Change the name pattern for memory disks Signed-off-by: Azat Khuzhin --- src/Disks/DiskMemory.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Disks/DiskMemory.cpp b/src/Disks/DiskMemory.cpp index 289a95a312e..17d8d5b2d25 100644 --- a/src/Disks/DiskMemory.cpp +++ b/src/Disks/DiskMemory.cpp @@ -143,7 +143,7 @@ private: DiskMemory::DiskMemory(const String & name_) : IDisk(name_) - , disk_path("memory://" + name_ + '/') + , disk_path("memory(" + name_ + ')') {} ReservationPtr DiskMemory::reserve(UInt64 /*bytes*/) From cb60576221a6d44581ca91f5cd7fb620a0cd9b27 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sun, 20 Nov 2022 16:31:10 +0100 Subject: [PATCH 54/65] Change DiskLocal::setup() signature (it always return true) Signed-off-by: Azat Khuzhin --- src/Disks/DiskLocal.cpp | 12 ++++++------ src/Disks/DiskLocal.h | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Disks/DiskLocal.cpp b/src/Disks/DiskLocal.cpp index c3428f54073..0bb4bb0bb27 100644 --- a/src/Disks/DiskLocal.cpp +++ b/src/Disks/DiskLocal.cpp @@ -637,7 +637,7 @@ void DiskLocal::checkAccessImpl(const String & path) IDisk::checkAccessImpl(path); } -bool DiskLocal::setup() +void DiskLocal::setup() { try { @@ -652,7 +652,7 @@ bool DiskLocal::setup() /// If disk checker is disabled, just assume RW by default. if (!disk_checker) - return true; + return; try { @@ -676,6 +676,7 @@ bool DiskLocal::setup() /// Try to create a new checker file. The disk status can be either broken or readonly. if (disk_checker_magic_number == -1) + { try { pcg32_fast rng(randomSeed()); @@ -695,12 +696,12 @@ bool DiskLocal::setup() disk_checker_path, name); disk_checker_can_check_read = false; - return true; + return; } + } if (disk_checker_magic_number == -1) throw Exception("disk_checker_magic_number is not initialized. It's a bug", ErrorCodes::LOGICAL_ERROR); - return true; } void DiskLocal::startupImpl(ContextPtr) @@ -711,8 +712,7 @@ void DiskLocal::startupImpl(ContextPtr) try { - if (!setup()) - readonly = true; + setup(); } catch (...) { diff --git a/src/Disks/DiskLocal.h b/src/Disks/DiskLocal.h index 9e22aecdc8b..7f6fc096ef5 100644 --- a/src/Disks/DiskLocal.h +++ b/src/Disks/DiskLocal.h @@ -137,9 +137,9 @@ protected: private: std::optional tryReserve(UInt64 bytes); - /// Setup disk for healthy check. Returns true if it's read-write, false if read-only. + /// Setup disk for healthy check. /// Throw exception if it's not possible to setup necessary files and directories. - bool setup(); + void setup(); /// Read magic number from disk checker file. Return std::nullopt if exception happens. std::optional readDiskCheckerMagicNumber() const noexcept; From e6e223adc2c9aa380703f77abed15c48415e515f Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sun, 20 Nov 2022 16:33:55 +0100 Subject: [PATCH 55/65] Check if local disk is readonly before access checks --- src/Disks/DiskLocal.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Disks/DiskLocal.cpp b/src/Disks/DiskLocal.cpp index 0bb4bb0bb27..a1f91319cf2 100644 --- a/src/Disks/DiskLocal.cpp +++ b/src/Disks/DiskLocal.cpp @@ -626,10 +626,16 @@ void DiskLocal::checkAccessImpl(const String & path) try { fs::create_directories(disk_path); + if (!FS::canWrite(disk_path)) + { + LOG_ERROR(logger, "Cannot write to the root directory of disk {} ({}).", name, disk_path); + readonly = true; + return; + } } catch (...) { - LOG_ERROR(logger, "Cannot create the directory of disk {} ({}).", name, disk_path); + LOG_ERROR(logger, "Cannot create the root directory of disk {} ({}).", name, disk_path); readonly = true; return; } From c393af2812cb56a60a99d48c15c0443d2da98a8b Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sun, 20 Nov 2022 16:33:37 +0100 Subject: [PATCH 56/65] Fix is_read_only property of local disks But it wasn't a problem before, since before it was not possible to have readonly==true. Signed-off-by: Azat Khuzhin --- src/Disks/DiskLocal.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Disks/DiskLocal.h b/src/Disks/DiskLocal.h index 7f6fc096ef5..14e29904422 100644 --- a/src/Disks/DiskLocal.h +++ b/src/Disks/DiskLocal.h @@ -110,6 +110,7 @@ public: void applyNewSettings(const Poco::Util::AbstractConfiguration & config, ContextPtr context, const String & config_prefix, const DisksMap &) override; bool isBroken() const override { return broken; } + bool isReadOnly() const override { return readonly; } void startupImpl(ContextPtr context) override; From 1831aa45d9b5c9e7491e1f945bd811da81211d65 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sun, 20 Nov 2022 21:03:22 +0100 Subject: [PATCH 57/65] Fix flaky 01926_order_by_desc_limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI captures [1] too long logs flush: $ clickhouse-local --file query_log.tsv.gz --input-format TabSeparatedWithNamesAndTypes -q "select event_time, query from table where Settings['log_comment'] = '01926_order_by_desc_limit.sql' and type = 'QueryFinish' and (lower(query) LIKE lower('SELECT s FROM order_by_desc ORDER BY u%') or query like '%query_log%') format Vertical" Row 1: ────── event_time: 2022-11-20 12:46:42 query: SELECT s FROM order_by_desc ORDER BY u DESC LIMIT 10 FORMAT Null SETTINGS max_memory_usage = '400M'; Row 2: ────── event_time: 2022-11-20 12:46:47 query: SELECT s FROM order_by_desc ORDER BY u LIMIT 10 FORMAT Null SETTINGS max_memory_usage = '400M'; Row 3: ────── event_time: 2022-11-20 12:46:54 query: SELECT read_rows < 110000 FROM system.query_log WHERE type = 'QueryFinish' AND current_database = currentDatabase() AND event_time > now() - INTERVAL 10 SECOND AND lower(query) LIKE lower('SELECT s FROM order_by_desc ORDER BY u%'); [1]: https://s3.amazonaws.com/clickhouse-test-reports/43143/c393af2812cb56a60a99d48c15c0443d2da98a8b/stateless_tests__tsan__[2/5].html Signed-off-by: Azat Khuzhin --- tests/queries/0_stateless/01926_order_by_desc_limit.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/01926_order_by_desc_limit.sql b/tests/queries/0_stateless/01926_order_by_desc_limit.sql index 86468b4fcd6..92c7a27bc9a 100644 --- a/tests/queries/0_stateless/01926_order_by_desc_limit.sql +++ b/tests/queries/0_stateless/01926_order_by_desc_limit.sql @@ -21,5 +21,5 @@ SYSTEM FLUSH LOGS; SELECT read_rows < 110000 FROM system.query_log WHERE type = 'QueryFinish' AND current_database = currentDatabase() -AND event_time > now() - INTERVAL 10 SECOND +AND event_date >= yesterday() AND lower(query) LIKE lower('SELECT s FROM order_by_desc ORDER BY u%'); From fffb711097fa4a55fa740d9dbcd0533a01cf2153 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Mon, 21 Nov 2022 12:54:21 +0100 Subject: [PATCH 58/65] add tests --- .../02477_single_value_data_string_regression.reference | 3 +++ .../0_stateless/02477_single_value_data_string_regression.sql | 3 +++ 2 files changed, 6 insertions(+) diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.reference b/tests/queries/0_stateless/02477_single_value_data_string_regression.reference index 2fe0effb902..13f66b70bfb 100644 --- a/tests/queries/0_stateless/02477_single_value_data_string_regression.reference +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.reference @@ -22,3 +22,6 @@ -2^31 0 1M without 0 1048576 1M with 0 1048575 +fuzz2 0123 4 +fuzz3 0 +fuzz4 0 diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql index 82b74868fc5..0eae7c97320 100644 --- a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql @@ -103,3 +103,6 @@ SELECT '1M without 0', length(maxMerge(x)) from (select CAST(unhex('00001000') | SELECT '1M with 0', length(maxMerge(x)) from (select CAST(unhex('00001000') || randomString(0x00100000 - 1) || '\0', 'AggregateFunction(max, String)') as x); SELECT 'fuzz1', finalizeAggregation(CAST(unhex('3000000\0303132333435363738393031323334353637383930313233343536373839303132333435363738393031323334353600010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); -- { serverError CORRUPTED_DATA } +SELECT 'fuzz2', finalizeAggregation(CAST(unhex('04000000' || '30313233' || '01' || 'ffffffffffffffff'), 'AggregateFunction(argMax, String, UInt64)')) as x, length(x); +SELECT 'fuzz3', finalizeAggregation(CAST(unhex('04000000' || '30313233' || '00' || 'ffffffffffffffff'), 'AggregateFunction(argMax, String, UInt64)')) as x, length(x); +SELECT 'fuzz4', finalizeAggregation(CAST(unhex('04000000' || '30313233' || '00'), 'AggregateFunction(argMax, String, UInt64)')) as x, length(x); \ No newline at end of file From a52bdca989c8a636613177a3b4470a345376ccbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 21 Nov 2022 13:33:16 +0100 Subject: [PATCH 59/65] Detect the other corrupted state too --- src/AggregateFunctions/AggregateFunctionArgMinMax.h | 9 +++++++-- .../02477_single_value_data_string_regression.reference | 2 -- .../02477_single_value_data_string_regression.sql | 5 +++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/AggregateFunctions/AggregateFunctionArgMinMax.h b/src/AggregateFunctions/AggregateFunctionArgMinMax.h index 0cf0a989a32..decb572b019 100644 --- a/src/AggregateFunctions/AggregateFunctionArgMinMax.h +++ b/src/AggregateFunctions/AggregateFunctionArgMinMax.h @@ -90,8 +90,13 @@ public: { this->data(place).result.read(buf, *serialization_res, arena); this->data(place).value.read(buf, *serialization_val, arena); - if (unlikely(this->data(place).value.has() && !this->data(place).result.has())) - throw Exception(ErrorCodes::CORRUPTED_DATA, "State of aggregate function {} has value, but does not have result", getName()); + if (unlikely(this->data(place).value.has() != this->data(place).result.has())) + throw Exception( + ErrorCodes::CORRUPTED_DATA, + "Invalid state of the aggregate function {}: has_value ({}) != has_result ({})", + getName(), + this->data(place).value.has(), + this->data(place).result.has()); } bool allocatesMemoryInArena() const override diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.reference b/tests/queries/0_stateless/02477_single_value_data_string_regression.reference index 13f66b70bfb..e89b8ff7d99 100644 --- a/tests/queries/0_stateless/02477_single_value_data_string_regression.reference +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.reference @@ -23,5 +23,3 @@ 1M without 0 1048576 1M with 0 1048575 fuzz2 0123 4 -fuzz3 0 -fuzz4 0 diff --git a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql index 0eae7c97320..c8030733e34 100644 --- a/tests/queries/0_stateless/02477_single_value_data_string_regression.sql +++ b/tests/queries/0_stateless/02477_single_value_data_string_regression.sql @@ -104,5 +104,6 @@ SELECT '1M with 0', length(maxMerge(x)) from (select CAST(unhex('00001000') || r SELECT 'fuzz1', finalizeAggregation(CAST(unhex('3000000\0303132333435363738393031323334353637383930313233343536373839303132333435363738393031323334353600010000000000000000'), 'AggregateFunction(argMax, String, UInt64)')); -- { serverError CORRUPTED_DATA } SELECT 'fuzz2', finalizeAggregation(CAST(unhex('04000000' || '30313233' || '01' || 'ffffffffffffffff'), 'AggregateFunction(argMax, String, UInt64)')) as x, length(x); -SELECT 'fuzz3', finalizeAggregation(CAST(unhex('04000000' || '30313233' || '00' || 'ffffffffffffffff'), 'AggregateFunction(argMax, String, UInt64)')) as x, length(x); -SELECT 'fuzz4', finalizeAggregation(CAST(unhex('04000000' || '30313233' || '00'), 'AggregateFunction(argMax, String, UInt64)')) as x, length(x); \ No newline at end of file +SELECT 'fuzz3', finalizeAggregation(CAST(unhex('04000000' || '30313233' || '00' || 'ffffffffffffffff'), 'AggregateFunction(argMax, String, UInt64)')) as x, length(x); -- { serverError CORRUPTED_DATA } +SELECT 'fuzz4', finalizeAggregation(CAST(unhex('04000000' || '30313233' || '00'), 'AggregateFunction(argMax, String, UInt64)')) as x, length(x); -- { serverError CORRUPTED_DATA } +SELECT 'fuzz5', finalizeAggregation(CAST(unhex('0100000000000000000FFFFFFFF0'), 'AggregateFunction(argMax, UInt64, String)')); -- { serverError CORRUPTED_DATA } From b17dc24a94f38a5981168a02e16dc04fc7909a34 Mon Sep 17 00:00:00 2001 From: Dmitry Novik Date: Mon, 21 Nov 2022 16:10:47 +0000 Subject: [PATCH 60/65] Do not reuse jemalloc memory in test --- programs/server/Server.cpp | 3 +++ src/Common/MemoryTracker.cpp | 2 +- src/Common/MemoryTracker.h | 5 +++++ .../configs/global_overcommit_tracker.xml | 1 + 4 files changed, 10 insertions(+), 1 deletion(-) diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index d2ad9b04993..cfc153629e0 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -1148,6 +1148,9 @@ int Server::main(const std::vector & /*args*/) total_memory_tracker.setDescription("(total)"); total_memory_tracker.setMetric(CurrentMetrics::MemoryTracking); + bool allow_use_jemalloc_memory = config->getBool("allow_use_jemalloc_memory", true); + total_memory_tracker.setAllowUseJemallocMemory(allow_use_jemalloc_memory); + auto * global_overcommit_tracker = global_context->getGlobalOvercommitTracker(); total_memory_tracker.setOvercommitTracker(global_overcommit_tracker); diff --git a/src/Common/MemoryTracker.cpp b/src/Common/MemoryTracker.cpp index b530410ec63..f556b255fc2 100644 --- a/src/Common/MemoryTracker.cpp +++ b/src/Common/MemoryTracker.cpp @@ -220,7 +220,7 @@ void MemoryTracker::allocImpl(Int64 size, bool throw_if_memory_exceeded, MemoryT Int64 limit_to_check = current_hard_limit; #if USE_JEMALLOC - if (level == VariableContext::Global) + if (level == VariableContext::Global && allow_use_jemalloc_memory.load(std::memory_order_relaxed)) { /// Jemalloc arenas may keep some extra memory. /// This memory was substucted from RSS to decrease memory drift. diff --git a/src/Common/MemoryTracker.h b/src/Common/MemoryTracker.h index 2d898935dcf..f6113d31423 100644 --- a/src/Common/MemoryTracker.h +++ b/src/Common/MemoryTracker.h @@ -55,6 +55,7 @@ private: std::atomic soft_limit {0}; std::atomic hard_limit {0}; std::atomic profiler_limit {0}; + std::atomic_bool allow_use_jemalloc_memory {true}; static std::atomic free_memory_in_allocator_arenas; @@ -125,6 +126,10 @@ public: { return soft_limit.load(std::memory_order_relaxed); } + void setAllowUseJemallocMemory(bool value) + { + allow_use_jemalloc_memory.store(value, std::memory_order_relaxed); + } /** Set limit if it was not set. * Otherwise, set limit to new value, if new value is greater than previous limit. diff --git a/tests/integration/test_global_overcommit_tracker/configs/global_overcommit_tracker.xml b/tests/integration/test_global_overcommit_tracker/configs/global_overcommit_tracker.xml index 6f83a570ccc..a51009542a3 100644 --- a/tests/integration/test_global_overcommit_tracker/configs/global_overcommit_tracker.xml +++ b/tests/integration/test_global_overcommit_tracker/configs/global_overcommit_tracker.xml @@ -1,3 +1,4 @@ 2000000000 + false \ No newline at end of file From 86a0dd010f0a95af1991a363f60c57fd1737f37b Mon Sep 17 00:00:00 2001 From: Alexander Gololobov <440544+davenger@users.noreply.github.com> Date: Mon, 21 Nov 2022 22:21:49 +0100 Subject: [PATCH 61/65] Use read_hint and file_size for choosing buffer size --- src/Disks/IO/createReadBufferFromFileBase.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/Disks/IO/createReadBufferFromFileBase.cpp b/src/Disks/IO/createReadBufferFromFileBase.cpp index 98da89f81ed..b274786f162 100644 --- a/src/Disks/IO/createReadBufferFromFileBase.cpp +++ b/src/Disks/IO/createReadBufferFromFileBase.cpp @@ -42,7 +42,7 @@ std::unique_ptr createReadBufferFromFileBase( if (read_hint.has_value()) estimated_size = *read_hint; else if (file_size.has_value()) - estimated_size = file_size.has_value() ? *file_size : 0; + estimated_size = *file_size; if (!existing_memory && settings.local_fs_method == LocalFSReadMethod::mmap @@ -158,7 +158,15 @@ std::unique_ptr createReadBufferFromFileBase( #endif ProfileEvents::increment(ProfileEvents::CreatedReadBufferOrdinary); - return create(settings.local_fs_buffer_size, flags); + + size_t buffer_size = settings.local_fs_buffer_size; + /// Check if the buffer can be smaller than default + if (read_hint.has_value() && *read_hint > 0 && *read_hint < buffer_size) + buffer_size = *read_hint; + if (file_size.has_value() && *file_size < buffer_size) + buffer_size = *file_size; + + return create(buffer_size, flags); } } From 5afd2a1adde44e751096a8796b7e0002ee2734d1 Mon Sep 17 00:00:00 2001 From: Alexander Gololobov <440544+davenger@users.noreply.github.com> Date: Mon, 21 Nov 2022 22:23:27 +0100 Subject: [PATCH 62/65] Pass file size to better choose buffer size --- .../MetadataStorageFromDiskTransactionOperations.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Disks/ObjectStorages/MetadataStorageFromDiskTransactionOperations.cpp b/src/Disks/ObjectStorages/MetadataStorageFromDiskTransactionOperations.cpp index 010fc103254..ce5171fedee 100644 --- a/src/Disks/ObjectStorages/MetadataStorageFromDiskTransactionOperations.cpp +++ b/src/Disks/ObjectStorages/MetadataStorageFromDiskTransactionOperations.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -62,7 +63,7 @@ UnlinkFileOperation::UnlinkFileOperation(const std::string & path_, IDisk & disk void UnlinkFileOperation::execute(std::unique_lock &) { - auto buf = disk.readFile(path); + auto buf = disk.readFile(path, ReadSettings{}, std::nullopt, disk.getFileSize(path)); readStringUntilEOF(prev_data, *buf); disk.removeFile(path); } From 4502fc0e815b27b3bedc9909ef26743d55e1bb7d Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Tue, 22 Nov 2022 01:02:36 +0000 Subject: [PATCH 63/65] fix flaky tests --- tests/queries/0_stateless/00824_filesystem.sql | 2 +- .../queries/0_stateless/02457_filesystem_function.reference | 1 + tests/queries/0_stateless/02457_filesystem_function.sql | 6 +++++- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/queries/0_stateless/00824_filesystem.sql b/tests/queries/0_stateless/00824_filesystem.sql index cecf66810cf..c8ac9179d42 100644 --- a/tests/queries/0_stateless/00824_filesystem.sql +++ b/tests/queries/0_stateless/00824_filesystem.sql @@ -1 +1 @@ -SELECT filesystemCapacity() >= filesystemAvailable() AND filesystemAvailable() >= filesystemUnreserved() AND filesystemUnreserved() >= 0; +SELECT filesystemCapacity() >= filesystemAvailable() AND filesystemAvailable() >= 0 AND filesystemUnreserved() >= 0; diff --git a/tests/queries/0_stateless/02457_filesystem_function.reference b/tests/queries/0_stateless/02457_filesystem_function.reference index d00491fd7e5..6ed281c757a 100644 --- a/tests/queries/0_stateless/02457_filesystem_function.reference +++ b/tests/queries/0_stateless/02457_filesystem_function.reference @@ -1 +1,2 @@ 1 +1 diff --git a/tests/queries/0_stateless/02457_filesystem_function.sql b/tests/queries/0_stateless/02457_filesystem_function.sql index fac418c046a..d8322bc65b5 100644 --- a/tests/queries/0_stateless/02457_filesystem_function.sql +++ b/tests/queries/0_stateless/02457_filesystem_function.sql @@ -1,2 +1,6 @@ -select filesystemCapacity('default') >= filesystemAvailable('default') and filesystemAvailable('default') >= filesystemUnreserved('default'); +-- Tags: no-fasttest + +select filesystemCapacity('s3_disk') >= filesystemAvailable('s3_disk') and filesystemAvailable('s3_disk') >= filesystemUnreserved('s3_disk'); +select filesystemCapacity('default') >= filesystemAvailable('default') and filesystemAvailable('default') >= 0 and filesystemUnreserved('default') >= 0; + select filesystemCapacity('__un_exists_disk'); -- { serverError UNKNOWN_DISK } From fb95ca43d8f94b5e1b4e43029364745d1b956a84 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Tue, 22 Nov 2022 12:43:17 +0100 Subject: [PATCH 64/65] Use all parameters with prefixes from ssm --- tests/ci/get_robot_token.py | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/tests/ci/get_robot_token.py b/tests/ci/get_robot_token.py index 4fb8cb8f49f..263add077b1 100644 --- a/tests/ci/get_robot_token.py +++ b/tests/ci/get_robot_token.py @@ -1,4 +1,6 @@ #!/usr/bin/env python3 +import logging + import boto3 # type: ignore from github import Github # type: ignore @@ -9,14 +11,28 @@ def get_parameter_from_ssm(name, decrypt=True, client=None): return client.get_parameter(Name=name, WithDecryption=decrypt)["Parameter"]["Value"] -def get_best_robot_token(token_prefix_env_name="github_robot_token_", total_tokens=4): +def get_best_robot_token(token_prefix_env_name="github_robot_token_"): client = boto3.client("ssm", region_name="us-east-1") - tokens = {} - for i in range(1, total_tokens + 1): - token_name = token_prefix_env_name + str(i) - token = get_parameter_from_ssm(token_name, True, client) - gh = Github(token, per_page=100) - rest, _ = gh.rate_limiting - tokens[token] = rest + parameters = client.describe_parameters( + ParameterFilters=[ + {"Key": "Name", "Option": "BeginsWith", "Values": [token_prefix_env_name]} + ] + )["Parameters"] + assert parameters + token = {"login": "", "value": "", "rest": 0} - return max(tokens.items(), key=lambda x: x[1])[0] + for token_name in [p["Name"] for p in parameters]: + value = get_parameter_from_ssm(token_name, True, client) + gh = Github(value, per_page=100) + login = gh.get_user().login + rest, _ = gh.rate_limiting + logging.info("Get token for user %s with %s remaining requests", login, rest) + if token["rest"] < rest: + token = {"login": login, "value": value, "rest": rest} + + assert token["login"] + logging.info( + "User %s with %s remaining requests is used", token["login"], token["rest"] + ) + + return token["value"] From 7048f36fc70c0d5572595118c304f6292351715b Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Tue, 22 Nov 2022 13:37:08 +0100 Subject: [PATCH 65/65] Save unnecessary API requests --- tests/ci/get_robot_token.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/ci/get_robot_token.py b/tests/ci/get_robot_token.py index 263add077b1..163e1ce071e 100644 --- a/tests/ci/get_robot_token.py +++ b/tests/ci/get_robot_token.py @@ -24,15 +24,17 @@ def get_best_robot_token(token_prefix_env_name="github_robot_token_"): for token_name in [p["Name"] for p in parameters]: value = get_parameter_from_ssm(token_name, True, client) gh = Github(value, per_page=100) - login = gh.get_user().login + # Do not spend additional request to API by accessin user.login unless + # the token is chosen by the remaining requests number + user = gh.get_user() rest, _ = gh.rate_limiting - logging.info("Get token for user %s with %s remaining requests", login, rest) + logging.info("Get token with %s remaining requests", rest) if token["rest"] < rest: - token = {"login": login, "value": value, "rest": rest} + token = {"user": user, "value": value, "rest": rest} - assert token["login"] + assert token["value"] logging.info( - "User %s with %s remaining requests is used", token["login"], token["rest"] + "User %s with %s remaining requests is used", token["user"].login, token["rest"] ) return token["value"]