From 6734b592bb59939a7a01136ab24b4033154be2fc Mon Sep 17 00:00:00 2001 From: feng lv Date: Sun, 26 Dec 2021 09:04:12 +0000 Subject: [PATCH 01/31] decimal plus float --- src/Functions/FunctionBinaryArithmetic.h | 74 +++++++++++++++--------- src/Functions/plus.cpp | 7 ++- 2 files changed, 52 insertions(+), 29 deletions(-) diff --git a/src/Functions/FunctionBinaryArithmetic.h b/src/Functions/FunctionBinaryArithmetic.h index 69d65bfcf66..8d0ad5db208 100644 --- a/src/Functions/FunctionBinaryArithmetic.h +++ b/src/Functions/FunctionBinaryArithmetic.h @@ -7,37 +7,38 @@ #include #include -#include -#include +#include +#include +#include +#include +#include +#include +#include +#include #include #include #include -#include -#include +#include #include +#include #include +#include +#include #include #include -#include -#include -#include -#include -#include -#include -#include -#include "Core/DecimalFunctions.h" -#include "IFunction.h" -#include "FunctionHelpers.h" -#include "IsOperation.h" -#include "DivisionUtils.h" -#include "castTypeToEither.h" -#include "FunctionFactory.h" -#include -#include -#include -#include #include #include +#include +#include +#include +#include +#include "Core/DecimalFunctions.h" +#include "DivisionUtils.h" +#include "FunctionFactory.h" +#include "FunctionHelpers.h" +#include "IFunction.h" +#include "IsOperation.h" +#include "castTypeToEither.h" #include @@ -134,11 +135,29 @@ public: Case && IsIntegralOrExtended, LeftDataType>, Case && IsIntegralOrExtended, RightDataType>, + /// e.g Decimal + Float64 = Float64 + Case::plus && IsDataTypeDecimal && IsFloatingPoint, + DataTypeFloat64>, + Case::plus && IsDataTypeDecimal && IsFloatingPoint, + DataTypeFloat64>, + + /// e.g Decimal - Float64 = Float64 + Case::minus && IsDataTypeDecimal && IsFloatingPoint, + DataTypeFloat64>, + Case::multiply && IsDataTypeDecimal && IsFloatingPoint, + DataTypeFloat64>, + /// e.g Decimal * Float64 = Float64 Case::multiply && IsDataTypeDecimal && IsFloatingPoint, - RightDataType>, + DataTypeFloat64>, Case::multiply && IsDataTypeDecimal && IsFloatingPoint, - LeftDataType>, + DataTypeFloat64>, + + /// e.g Decimal / Float64 = Float64 + Case::multiply && IsDataTypeDecimal && IsFloatingPoint, + DataTypeFloat64>, + Case::multiply && IsDataTypeDecimal && IsFloatingPoint, + DataTypeFloat64>, /// Decimal Real is not supported (traditional DBs convert Decimal Real to Real) Case && !IsIntegralOrExtendedOrDecimal, InvalidType>, @@ -966,9 +985,9 @@ class FunctionBinaryArithmetic : public IFunction const ResultDataType type = [&] { if constexpr (left_is_decimal && IsFloatingPoint) - return RightDataType(); + return DataTypeFloat64{}; else if constexpr (right_is_decimal && IsFloatingPoint) - return LeftDataType(); + return DataTypeFloat64{}; else return decimalResultType(left, right); }(); @@ -1219,8 +1238,7 @@ public: } else if constexpr ((IsDataTypeDecimal && IsFloatingPoint) || (IsDataTypeDecimal && IsFloatingPoint)) - type_res = std::make_shared, - LeftDataType, RightDataType>>(); + type_res = std::make_shared(); else if constexpr (IsDataTypeDecimal) type_res = std::make_shared(left.getPrecision(), left.getScale()); else if constexpr (IsDataTypeDecimal) diff --git a/src/Functions/plus.cpp b/src/Functions/plus.cpp index 997cae0dbed..96e05b57f12 100644 --- a/src/Functions/plus.cpp +++ b/src/Functions/plus.cpp @@ -32,7 +32,12 @@ struct PlusImpl template static inline bool apply(A a, B b, Result & c) { - return common::addOverflow(static_cast(a), b, c); + if constexpr (std::is_same_v) + { + c = static_cast(a) + b; + } + else + return common::addOverflow(static_cast(a), b, c); } #if USE_EMBEDDED_COMPILER From 48c0b41ad5691c97ec704e050ef2893c652b36c9 Mon Sep 17 00:00:00 2001 From: feng lv Date: Sat, 1 Jan 2022 09:05:29 +0000 Subject: [PATCH 02/31] Enable binary arithmetic between Decimal and Float fix --- src/Functions/FunctionBinaryArithmetic.h | 45 +++- src/Functions/IsOperation.h | 5 +- src/Functions/plus.cpp | 7 +- ...ary_op_between_float_and_decimal.reference | 215 ++++++++++++++++++ ...55_binary_op_between_float_and_decimal.sql | 96 ++++++++ 5 files changed, 350 insertions(+), 18 deletions(-) create mode 100644 tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.reference create mode 100644 tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.sql diff --git a/src/Functions/FunctionBinaryArithmetic.h b/src/Functions/FunctionBinaryArithmetic.h index 8d0ad5db208..acb973b57d8 100644 --- a/src/Functions/FunctionBinaryArithmetic.h +++ b/src/Functions/FunctionBinaryArithmetic.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -144,7 +145,7 @@ public: /// e.g Decimal - Float64 = Float64 Case::minus && IsDataTypeDecimal && IsFloatingPoint, DataTypeFloat64>, - Case::multiply && IsDataTypeDecimal && IsFloatingPoint, + Case::minus && IsDataTypeDecimal && IsFloatingPoint, DataTypeFloat64>, /// e.g Decimal * Float64 = Float64 @@ -154,9 +155,21 @@ public: DataTypeFloat64>, /// e.g Decimal / Float64 = Float64 - Case::multiply && IsDataTypeDecimal && IsFloatingPoint, + Case::division && IsDataTypeDecimal && IsFloatingPoint, DataTypeFloat64>, - Case::multiply && IsDataTypeDecimal && IsFloatingPoint, + Case::division && IsDataTypeDecimal && IsFloatingPoint, + DataTypeFloat64>, + + /// e.g least(Decimal, Float64) = Float64 + Case::least && IsDataTypeDecimal && IsFloatingPoint, + DataTypeFloat64>, + Case::least && IsDataTypeDecimal && IsFloatingPoint, + DataTypeFloat64>, + + /// e.g greatest(Decimal, Float64) = Float64 + Case::greatest && IsDataTypeDecimal && IsFloatingPoint, + DataTypeFloat64>, + Case::greatest && IsDataTypeDecimal && IsFloatingPoint, DataTypeFloat64>, /// Decimal Real is not supported (traditional DBs convert Decimal Real to Real) @@ -1471,15 +1484,31 @@ public: else // we can't avoid the else because otherwise the compiler may assume the ResultDataType may be Invalid // and that would produce the compile error. { - using T0 = typename LeftDataType::FieldType; - using T1 = typename RightDataType::FieldType; + using T0 = std::conditional_t, Float64, typename LeftDataType::FieldType>; + using T1 = std::conditional_t, Float64, typename RightDataType::FieldType>; using ResultType = typename ResultDataType::FieldType; using ColVecT0 = ColumnVectorOrDecimal; using ColVecT1 = ColumnVectorOrDecimal; using ColVecResult = ColumnVectorOrDecimal; - const auto * const col_left_raw = arguments[0].column.get(); - const auto * const col_right_raw = arguments[1].column.get(); + + const IColumn * col_left_raw; + const IColumn * col_right_raw; + + /// When Decimal op Float32/64, convert both of them into Float64 + if constexpr ((IsDataTypeDecimal || IsDataTypeDecimal)&&IsFloatingPoint) + { + const auto converted_type = std::make_shared(); + auto c0_converted = castColumn(arguments[0], converted_type); + auto c1_converted = castColumn(arguments[1], converted_type); + col_left_raw = c0_converted.get(); + col_right_raw = c1_converted.get(); + } + else + { + col_left_raw = arguments[0].column.get(); + col_right_raw = arguments[1].column.get(); + } const size_t col_left_size = col_left_raw->size(); @@ -1489,7 +1518,7 @@ public: const ColVecT0 * const col_left = checkAndGetColumn(col_left_raw); const ColVecT1 * const col_right = checkAndGetColumn(col_right_raw); - if constexpr (IsDataTypeDecimal || IsDataTypeDecimal) + if constexpr ((IsDataTypeDecimal || IsDataTypeDecimal)&&!IsFloatingPoint) { return executeNumericWithDecimal( left, right, diff --git a/src/Functions/IsOperation.h b/src/Functions/IsOperation.h index 369978fe271..5af8ae77727 100644 --- a/src/Functions/IsOperation.h +++ b/src/Functions/IsOperation.h @@ -57,10 +57,7 @@ struct IsOperation static constexpr bool division = div_floating || div_int || div_int_or_zero; - static constexpr bool allow_decimal = - plus || minus || multiply || - div_floating || div_int || div_int_or_zero || - least || greatest; + static constexpr bool allow_decimal = plus || minus || multiply || division || least || greatest; }; } diff --git a/src/Functions/plus.cpp b/src/Functions/plus.cpp index 96e05b57f12..997cae0dbed 100644 --- a/src/Functions/plus.cpp +++ b/src/Functions/plus.cpp @@ -32,12 +32,7 @@ struct PlusImpl template static inline bool apply(A a, B b, Result & c) { - if constexpr (std::is_same_v) - { - c = static_cast(a) + b; - } - else - return common::addOverflow(static_cast(a), b, c); + return common::addOverflow(static_cast(a), b, c); } #if USE_EMBEDDED_COMPILER diff --git a/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.reference b/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.reference new file mode 100644 index 00000000000..861eba18203 --- /dev/null +++ b/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.reference @@ -0,0 +1,215 @@ +3 +0 +2.25 +1 +3 +0 +2.25 +1 +inf +1 +1.5 + +plus +-4.5 2.5 -2 +-2.5 2.5 0 +2.5 -2.5 0 +4.5 -2.5 2 +-4.5 -2.5 -7 +-2.5 -2.5 -5 +2.5 2.5 5 +4.5 2.5 7 +-45.5 2.5 -43 +-25.5 2.5 -23 +25.5 -2.5 23 +45.5 -2.5 43 +-45.5 -2.5 -48 +-25.5 -2.5 -27.999999999999996 +25.5 2.5 27.999999999999996 +45.5 2.5 48 +-4.5 -2.5 -7 +-2.5 -2.5 -5 +2.5 2.5 5 +4.5 2.5 7 +-4.5 2.5 -2 +2.5 -2.5 0 +-2.5 2.5 0 +4.5 -2.5 2 +-45.5 -2.5 -48 +-25.5 -2.5 -28 +25.5 2.5 28 +45.5 2.5 48 +-45.5 2.5 -43 +-25.5 2.5 -22.999999999999996 +25.5 -2.5 22.999999999999996 +45.5 -2.5 43 + +minus +-4.5 2.5 -7 +-2.5 2.5 -5 +2.5 -2.5 5 +4.5 -2.5 7 +-4.5 -2.5 -2 +-2.5 -2.5 0 +2.5 2.5 0 +4.5 2.5 2 +-45.5 2.5 -48 +-25.5 2.5 -28 +25.5 -2.5 28 +45.5 -2.5 48 +-45.5 -2.5 -43 +-25.5 -2.5 -22.999999999999996 +25.5 2.5 22.999999999999996 +45.5 2.5 43 +-4.5 -2.5 -2 +2.5 2.5 0 +-2.5 -2.5 0 +4.5 2.5 2 +-4.5 2.5 -7 +-2.5 2.5 -5 +2.5 -2.5 5 +4.5 -2.5 7 +-45.5 -2.5 -43 +-25.5 -2.5 -23 +25.5 2.5 23 +45.5 2.5 43 +-45.5 2.5 -48 +-25.5 2.5 -27.999999999999996 +25.5 -2.5 27.999999999999996 +45.5 -2.5 48 + +multiply +4.5 -2.5 -11.25 +-4.5 2.5 -11.25 +-2.5 2.5 -6.25 +2.5 -2.5 -6.25 +-2.5 -2.5 6.25 +2.5 2.5 6.25 +-4.5 -2.5 11.25 +4.5 2.5 11.25 +45.5 -2.5 -113.75 +-45.5 2.5 -113.75 +25.5 -2.5 -63.75 +-25.5 2.5 -63.75 +-25.5 -2.5 63.74999999999999 +25.5 2.5 63.74999999999999 +45.5 2.5 113.75 +-45.5 -2.5 113.75 +-2.5 -2.5 6.25 +2.5 2.5 6.25 +-4.5 -2.5 11.25 +4.5 2.5 11.25 +4.5 -2.5 -11.25 +-4.5 2.5 -11.25 +-2.5 2.5 -6.25 +2.5 -2.5 -6.25 +25.5 2.5 63.75 +-25.5 -2.5 63.75 +-45.5 -2.5 113.75 +45.5 2.5 113.75 +45.5 -2.5 -113.75 +-45.5 2.5 -113.75 +-25.5 2.5 -63.74999999999999 +25.5 -2.5 -63.74999999999999 + +division +-4.5 2.5 -1.8 +4.5 -2.5 -1.8 +-2.5 2.5 -1 +2.5 -2.5 -1 +-2.5 -2.5 1 +2.5 2.5 1 +4.5 2.5 1.8 +-4.5 -2.5 1.8 +-45.5 2.5 -18.2 +45.5 -2.5 -18.2 +25.5 -2.5 -10.2 +-25.5 2.5 -10.2 +25.5 2.5 10.2 +-25.5 -2.5 10.2 +45.5 2.5 18.2 +-45.5 -2.5 18.2 +2.5 2.5 1 +-2.5 -2.5 1 +-4.5 -2.5 1.8 +4.5 2.5 1.8 +-4.5 2.5 -1.8 +4.5 -2.5 -1.8 +-2.5 2.5 -1 +2.5 -2.5 -1 +25.5 2.5 10.2 +-25.5 -2.5 10.2 +-45.5 -2.5 18.2 +45.5 2.5 18.2 +45.5 -2.5 -18.2 +-45.5 2.5 -18.2 +-25.5 2.5 -10.2 +25.5 -2.5 -10.2 + +least +-4.5 2.5 -4.5 +4.5 -2.5 -2.5 +2.5 -2.5 -2.5 +-2.5 2.5 -2.5 +-4.5 -2.5 -4.5 +-2.5 -2.5 -2.5 +4.5 2.5 2.5 +2.5 2.5 2.5 +-45.5 2.5 -45.5 +-25.5 2.5 -25.5 +45.5 -2.5 -2.5 +25.5 -2.5 -2.5 +-45.5 -2.5 -45.5 +-25.5 -2.5 -25.499999999999996 +25.5 2.5 2.5 +45.5 2.5 2.5 +-4.5 -2.5 -4.5 +-2.5 -2.5 -2.5 +4.5 2.5 2.5 +2.5 2.5 2.5 +-4.5 2.5 -4.5 +4.5 -2.5 -2.5 +-2.5 2.5 -2.5 +2.5 -2.5 -2.5 +-45.5 -2.5 -45.5 +-25.5 -2.5 -25.5 +45.5 2.5 2.5 +25.5 2.5 2.5 +-45.5 2.5 -45.5 +-25.5 2.5 -25.499999999999996 +45.5 -2.5 -2.5 +25.5 -2.5 -2.5 + +greatest +-4.5 2.5 2.5 +-2.5 2.5 2.5 +2.5 -2.5 2.5 +4.5 -2.5 4.5 +-4.5 -2.5 -2.5 +-2.5 -2.5 -2.5 +2.5 2.5 2.5 +4.5 2.5 4.5 +-45.5 2.5 2.5 +-25.5 2.5 2.5 +25.5 -2.5 25.5 +45.5 -2.5 45.5 +-45.5 -2.5 -2.5 +-25.5 -2.5 -2.5 +25.5 2.5 25.499999999999996 +45.5 2.5 45.5 +-2.5 -2.5 -2.5 +-4.5 -2.5 -2.5 +2.5 2.5 2.5 +4.5 2.5 4.5 +-4.5 2.5 2.5 +-2.5 2.5 2.5 +2.5 -2.5 2.5 +4.5 -2.5 4.5 +-45.5 -2.5 -2.5 +-25.5 -2.5 -2.5 +25.5 2.5 25.5 +45.5 2.5 45.5 +-45.5 2.5 2.5 +-25.5 2.5 2.5 +25.5 -2.5 25.499999999999996 +45.5 -2.5 45.5 diff --git a/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.sql b/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.sql new file mode 100644 index 00000000000..1a7f1b77b9f --- /dev/null +++ b/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.sql @@ -0,0 +1,96 @@ +SELECT 1.5::Decimal32(5) + 1.5; +SELECT 1.5::Decimal32(5) - 1.5; +SELECT 1.5::Decimal32(5) * 1.5; +SELECT 1.5::Decimal32(5) / 1.5; + +SELECT 1.5 + 1.5::Decimal32(5); +SELECT 1.5 - 1.5::Decimal32(5); +SELECT 1.5 * 1.5::Decimal32(5); +SELECT 1.5 / 1.5::Decimal32(5); + +SELECT 1.0::Decimal32(5) / 0.0; + +SELECT least(1.5, 1.0::Decimal32(5)); +SELECT greatest(1.5, 1.0::Decimal32(5)); + +DROP TABLE IF EXISTS t; +CREATE TABLE t(d1 Decimal32(5), d2 Decimal64(10), d3 Decimal128(20), d4 Decimal256(40), f1 Float32, f2 Float64) ENGINE=Memory; + +INSERT INTO t values (-4.5, 4.5, -45.5, 45.5, 2.5, -2.5); +INSERT INTO t values (4.5, -4.5, 45.5, -45.5, -2.5, 2.5); +INSERT INTO t values (2.5, -2.5, 25.5, -25.5, -2.5, 2.5); +INSERT INTO t values (-2.5, 2.5, -25.5, 25.5, 2.5, -2.5); + +SELECT ''; +SELECT 'plus'; +SELECT d1, f1, d1 + f1 as plus FROM t ORDER BY plus; +SELECT d2, f1, d2 + f1 as plus FROM t ORDER BY plus; +SELECT d3, f1, d3 + f1 as plus FROM t ORDER BY plus; +SELECT d4, f1, d4 + f1 as plus FROM t ORDER BY plus; + +SELECT d1, f2, d1 + f2 as plus FROM t ORDER BY plus; +SELECT d2, f2, d2 + f2 as plus FROM t ORDER BY plus; +SELECT d3, f2, d3 + f2 as plus FROM t ORDER BY plus; +SELECT d4, f2, d4 + f2 as plus FROM t ORDER BY plus; + +SELECT ''; +SELECT 'minus'; +SELECT d1, f1, d1 - f1 as minus FROM t ORDER BY minus; +SELECT d2, f1, d2 - f1 as minus FROM t ORDER BY minus; +SELECT d3, f1, d3 - f1 as minus FROM t ORDER BY minus; +SELECT d4, f1, d4 - f1 as minus FROM t ORDER BY minus; + +SELECT d1, f2, d1 - f2 as minus FROM t ORDER BY minus; +SELECT d2, f2, d2 - f2 as minus FROM t ORDER BY minus; +SELECT d3, f2, d3 - f2 as minus FROM t ORDER BY minus; +SELECT d4, f2, d4 - f2 as minus FROM t ORDER BY minus; + +SELECT ''; +SELECT 'multiply'; +SELECT d1, f1, d1 * f1 as multiply FROM t ORDER BY multiply; +SELECT d2, f1, d2 * f1 as multiply FROM t ORDER BY multiply; +SELECT d3, f1, d3 * f1 as multiply FROM t ORDER BY multiply; +SELECT d4, f1, d4 * f1 as multiply FROM t ORDER BY multiply; + +SELECT d1, f2, d1 * f2 as multiply FROM t ORDER BY multiply; +SELECT d2, f2, d2 * f2 as multiply FROM t ORDER BY multiply; +SELECT d3, f2, d3 * f2 as multiply FROM t ORDER BY multiply; +SELECT d4, f2, d4 * f2 as multiply FROM t ORDER BY multiply; + +SELECT ''; +SELECT 'division'; +SELECT d1, f1, d1 / f1 as division FROM t ORDER BY division; +SELECT d2, f1, d2 / f1 as division FROM t ORDER BY division; +SELECT d3, f1, d3 / f1 as division FROM t ORDER BY division; +SELECT d4, f1, d4 / f1 as division FROM t ORDER BY division; + +SELECT d1, f2, d1 / f2 as division FROM t ORDER BY division; +SELECT d2, f2, d2 / f2 as division FROM t ORDER BY division; +SELECT d3, f2, d3 / f2 as division FROM t ORDER BY division; +SELECT d4, f2, d4 / f2 as division FROM t ORDER BY division; + +SELECT ''; +SELECT 'least'; +SELECT d1, f1, least(d1, f1) as least FROM t ORDER BY least; +SELECT d2, f1, least(d2, f1) as least FROM t ORDER BY least; +SELECT d3, f1, least(d3, f1) as least FROM t ORDER BY least; +SELECT d4, f1, least(d4, f1) as least FROM t ORDER BY least; + +SELECT d1, f2, least(d1, f2) as least FROM t ORDER BY least; +SELECT d2, f2, least(d2, f2) as least FROM t ORDER BY least; +SELECT d3, f2, least(d3, f2) as least FROM t ORDER BY least; +SELECT d4, f2, least(d4, f2) as least FROM t ORDER BY least; + +SELECT ''; +SELECT 'greatest'; +SELECT d1, f1, greatest(d1, f1) as greatest FROM t ORDER BY greatest; +SELECT d2, f1, greatest(d2, f1) as greatest FROM t ORDER BY greatest; +SELECT d3, f1, greatest(d3, f1) as greatest FROM t ORDER BY greatest; +SELECT d4, f1, greatest(d4, f1) as greatest FROM t ORDER BY greatest; + +SELECT d1, f2, greatest(d1, f2) as greatest FROM t ORDER BY greatest; +SELECT d2, f2, greatest(d2, f2) as greatest FROM t ORDER BY greatest; +SELECT d3, f2, greatest(d3, f2) as greatest FROM t ORDER BY greatest; +SELECT d4, f2, greatest(d4, f2) as greatest FROM t ORDER BY greatest; + +DROP TABLE t; From a8578e0ad9d165b1bbc0b720ceca1e72d0f87668 Mon Sep 17 00:00:00 2001 From: feng lv Date: Sat, 1 Jan 2022 09:25:52 +0000 Subject: [PATCH 03/31] fix test --- ...ary_op_between_float_and_decimal.reference | 276 +++++++++--------- ...55_binary_op_between_float_and_decimal.sql | 104 +++---- 2 files changed, 190 insertions(+), 190 deletions(-) diff --git a/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.reference b/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.reference index 861eba18203..a024d51e285 100644 --- a/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.reference +++ b/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.reference @@ -11,205 +11,205 @@ inf 1.5 plus --4.5 2.5 -2 --2.5 2.5 0 +4.5 -3.5 1 2.5 -2.5 0 -4.5 -2.5 2 --4.5 -2.5 -7 +-4.5 2.5 -2 +-2.5 3.5 1 +-4.5 -3.5 -8 -2.5 -2.5 -5 -2.5 2.5 5 4.5 2.5 7 --45.5 2.5 -43 --25.5 2.5 -23 +2.5 3.5 6 +45.5 -3.5 42 25.5 -2.5 23 -45.5 -2.5 43 --45.5 -2.5 -48 --25.5 -2.5 -27.999999999999996 -25.5 2.5 27.999999999999996 -45.5 2.5 48 --4.5 -2.5 -7 --2.5 -2.5 -5 -2.5 2.5 5 -4.5 2.5 7 --4.5 2.5 -2 -2.5 -2.5 0 --2.5 2.5 0 -4.5 -2.5 2 --45.5 -2.5 -48 --25.5 -2.5 -28 -25.5 2.5 28 -45.5 2.5 48 -45.5 2.5 -43 --25.5 2.5 -22.999999999999996 +-25.5 3.5 -22 +-45.5 -3.5 -49 +-25.5 -2.5 -27.999999999999996 +45.5 2.5 48 +25.5 3.5 28.999999999999996 +-4.5 -3.5 -8 +-2.5 -2.5 -5 +4.5 2.5 7 +2.5 3.5 6 +4.5 -3.5 1 +2.5 -2.5 0 +-4.5 2.5 -2 +-2.5 3.5 1 +-45.5 -3.5 -49 +-25.5 -2.5 -28 +45.5 2.5 48 +25.5 3.5 29 +45.5 -3.5 42 25.5 -2.5 22.999999999999996 -45.5 -2.5 43 +-45.5 2.5 -43 +-25.5 3.5 -21.999999999999996 minus --4.5 2.5 -7 --2.5 2.5 -5 +4.5 -3.5 8 2.5 -2.5 5 -4.5 -2.5 7 --4.5 -2.5 -2 +-4.5 2.5 -7 +-2.5 3.5 -6 +-4.5 -3.5 -1 -2.5 -2.5 0 -2.5 2.5 0 4.5 2.5 2 --45.5 2.5 -48 --25.5 2.5 -28 +2.5 3.5 -1 +45.5 -3.5 49 25.5 -2.5 28 -45.5 -2.5 48 --45.5 -2.5 -43 +-45.5 2.5 -48 +-25.5 3.5 -29 +-45.5 -3.5 -42 -25.5 -2.5 -22.999999999999996 -25.5 2.5 22.999999999999996 45.5 2.5 43 --4.5 -2.5 -2 -2.5 2.5 0 +25.5 3.5 21.999999999999996 +-4.5 -3.5 -1 -2.5 -2.5 0 4.5 2.5 2 --4.5 2.5 -7 --2.5 2.5 -5 +2.5 3.5 -1 +4.5 -3.5 8 2.5 -2.5 5 -4.5 -2.5 7 --45.5 -2.5 -43 +-4.5 2.5 -7 +-2.5 3.5 -6 +-45.5 -3.5 -42 -25.5 -2.5 -23 -25.5 2.5 23 45.5 2.5 43 --45.5 2.5 -48 --25.5 2.5 -27.999999999999996 +25.5 3.5 22 +45.5 -3.5 49 25.5 -2.5 27.999999999999996 -45.5 -2.5 48 +-45.5 2.5 -48 +-25.5 3.5 -28.999999999999996 multiply -4.5 -2.5 -11.25 --4.5 2.5 -11.25 --2.5 2.5 -6.25 +4.5 -3.5 -15.75 2.5 -2.5 -6.25 +-4.5 2.5 -11.25 +-2.5 3.5 -8.75 +-4.5 -3.5 15.75 -2.5 -2.5 6.25 -2.5 2.5 6.25 --4.5 -2.5 11.25 4.5 2.5 11.25 -45.5 -2.5 -113.75 --45.5 2.5 -113.75 +2.5 3.5 8.75 +45.5 -3.5 -159.25 25.5 -2.5 -63.75 --25.5 2.5 -63.75 --25.5 -2.5 63.74999999999999 -25.5 2.5 63.74999999999999 -45.5 2.5 113.75 --45.5 -2.5 113.75 --2.5 -2.5 6.25 -2.5 2.5 6.25 --4.5 -2.5 11.25 -4.5 2.5 11.25 -4.5 -2.5 -11.25 --4.5 2.5 -11.25 --2.5 2.5 -6.25 -2.5 -2.5 -6.25 -25.5 2.5 63.75 --25.5 -2.5 63.75 --45.5 -2.5 113.75 -45.5 2.5 113.75 -45.5 -2.5 -113.75 -45.5 2.5 -113.75 --25.5 2.5 -63.74999999999999 +-25.5 3.5 -89.25 +-45.5 -3.5 159.25 +-25.5 -2.5 63.74999999999999 +45.5 2.5 113.75 +25.5 3.5 89.24999999999999 +-4.5 -3.5 15.75 +-2.5 -2.5 6.25 +4.5 2.5 11.25 +2.5 3.5 8.75 +4.5 -3.5 -15.75 +2.5 -2.5 -6.25 +-4.5 2.5 -11.25 +-2.5 3.5 -8.75 +-45.5 -3.5 159.25 +-25.5 -2.5 63.75 +45.5 2.5 113.75 +25.5 3.5 89.25 +45.5 -3.5 -159.25 25.5 -2.5 -63.74999999999999 +-45.5 2.5 -113.75 +-25.5 3.5 -89.24999999999999 division --4.5 2.5 -1.8 -4.5 -2.5 -1.8 --2.5 2.5 -1 +4.5 -3.5 -1.2857142857142858 2.5 -2.5 -1 +-4.5 2.5 -1.8 +-2.5 3.5 -0.7142857142857143 +-4.5 -3.5 1.2857142857142858 -2.5 -2.5 1 -2.5 2.5 1 4.5 2.5 1.8 --4.5 -2.5 1.8 --45.5 2.5 -18.2 -45.5 -2.5 -18.2 +2.5 3.5 0.7142857142857143 +45.5 -3.5 -13 25.5 -2.5 -10.2 --25.5 2.5 -10.2 -25.5 2.5 10.2 +-45.5 2.5 -18.2 +-25.5 3.5 -7.285714285714286 +-45.5 -3.5 13 -25.5 -2.5 10.2 45.5 2.5 18.2 --45.5 -2.5 18.2 -2.5 2.5 1 +25.5 3.5 7.285714285714285 +-4.5 -3.5 1.2857142857142858 -2.5 -2.5 1 --4.5 -2.5 1.8 4.5 2.5 1.8 --4.5 2.5 -1.8 -4.5 -2.5 -1.8 --2.5 2.5 -1 +2.5 3.5 0.7142857142857143 +4.5 -3.5 -1.2857142857142858 2.5 -2.5 -1 -25.5 2.5 10.2 +-4.5 2.5 -1.8 +-2.5 3.5 -0.7142857142857143 +-45.5 -3.5 13 -25.5 -2.5 10.2 --45.5 -2.5 18.2 45.5 2.5 18.2 -45.5 -2.5 -18.2 --45.5 2.5 -18.2 --25.5 2.5 -10.2 +25.5 3.5 7.285714285714286 +45.5 -3.5 -13 25.5 -2.5 -10.2 +-45.5 2.5 -18.2 +-25.5 3.5 -7.285714285714285 least --4.5 2.5 -4.5 -4.5 -2.5 -2.5 +4.5 -3.5 -3.5 2.5 -2.5 -2.5 --2.5 2.5 -2.5 --4.5 -2.5 -4.5 +-4.5 2.5 -4.5 +-2.5 3.5 -2.5 +-4.5 -3.5 -4.5 -2.5 -2.5 -2.5 4.5 2.5 2.5 -2.5 2.5 2.5 --45.5 2.5 -45.5 --25.5 2.5 -25.5 -45.5 -2.5 -2.5 +2.5 3.5 2.5 +45.5 -3.5 -3.5 25.5 -2.5 -2.5 --45.5 -2.5 -45.5 +-45.5 2.5 -45.5 +-25.5 3.5 -25.5 +-45.5 -3.5 -45.5 -25.5 -2.5 -25.499999999999996 -25.5 2.5 2.5 45.5 2.5 2.5 --4.5 -2.5 -4.5 +25.5 3.5 3.5 +-4.5 -3.5 -4.5 -2.5 -2.5 -2.5 4.5 2.5 2.5 -2.5 2.5 2.5 --4.5 2.5 -4.5 -4.5 -2.5 -2.5 --2.5 2.5 -2.5 +2.5 3.5 2.5 +4.5 -3.5 -3.5 2.5 -2.5 -2.5 --45.5 -2.5 -45.5 +-4.5 2.5 -4.5 +-2.5 3.5 -2.5 +-45.5 -3.5 -45.5 -25.5 -2.5 -25.5 45.5 2.5 2.5 -25.5 2.5 2.5 --45.5 2.5 -45.5 --25.5 2.5 -25.499999999999996 -45.5 -2.5 -2.5 +25.5 3.5 3.5 +45.5 -3.5 -3.5 25.5 -2.5 -2.5 +-45.5 2.5 -45.5 +-25.5 3.5 -25.499999999999996 greatest --4.5 2.5 2.5 --2.5 2.5 2.5 +4.5 -3.5 4.5 2.5 -2.5 2.5 -4.5 -2.5 4.5 --4.5 -2.5 -2.5 +-4.5 2.5 2.5 +-2.5 3.5 3.5 +-4.5 -3.5 -3.5 -2.5 -2.5 -2.5 -2.5 2.5 2.5 4.5 2.5 4.5 --45.5 2.5 2.5 --25.5 2.5 2.5 +2.5 3.5 3.5 +45.5 -3.5 45.5 25.5 -2.5 25.5 -45.5 -2.5 45.5 --45.5 -2.5 -2.5 --25.5 -2.5 -2.5 -25.5 2.5 25.499999999999996 -45.5 2.5 45.5 --2.5 -2.5 -2.5 --4.5 -2.5 -2.5 -2.5 2.5 2.5 -4.5 2.5 4.5 --4.5 2.5 2.5 --2.5 2.5 2.5 -2.5 -2.5 2.5 -4.5 -2.5 4.5 --45.5 -2.5 -2.5 --25.5 -2.5 -2.5 -25.5 2.5 25.5 -45.5 2.5 45.5 -45.5 2.5 2.5 --25.5 2.5 2.5 +-25.5 3.5 3.5 +-45.5 -3.5 -3.5 +-25.5 -2.5 -2.5 +45.5 2.5 45.5 +25.5 3.5 25.499999999999996 +-4.5 -3.5 -3.5 +-2.5 -2.5 -2.5 +4.5 2.5 4.5 +2.5 3.5 3.5 +4.5 -3.5 4.5 +2.5 -2.5 2.5 +-4.5 2.5 2.5 +-2.5 3.5 3.5 +-45.5 -3.5 -3.5 +-25.5 -2.5 -2.5 +45.5 2.5 45.5 +25.5 3.5 25.5 +45.5 -3.5 45.5 25.5 -2.5 25.499999999999996 -45.5 -2.5 45.5 +-45.5 2.5 2.5 +-25.5 3.5 3.5 diff --git a/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.sql b/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.sql index 1a7f1b77b9f..2e8ac32462e 100644 --- a/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.sql +++ b/tests/queries/0_stateless/02155_binary_op_between_float_and_decimal.sql @@ -16,81 +16,81 @@ SELECT greatest(1.5, 1.0::Decimal32(5)); DROP TABLE IF EXISTS t; CREATE TABLE t(d1 Decimal32(5), d2 Decimal64(10), d3 Decimal128(20), d4 Decimal256(40), f1 Float32, f2 Float64) ENGINE=Memory; -INSERT INTO t values (-4.5, 4.5, -45.5, 45.5, 2.5, -2.5); -INSERT INTO t values (4.5, -4.5, 45.5, -45.5, -2.5, 2.5); -INSERT INTO t values (2.5, -2.5, 25.5, -25.5, -2.5, 2.5); -INSERT INTO t values (-2.5, 2.5, -25.5, 25.5, 2.5, -2.5); +INSERT INTO t values (-4.5, 4.5, -45.5, 45.5, 2.5, -3.5); +INSERT INTO t values (4.5, -4.5, 45.5, -45.5, -3.5, 2.5); +INSERT INTO t values (2.5, -2.5, 25.5, -25.5, -2.5, 3.5); +INSERT INTO t values (-2.5, 2.5, -25.5, 25.5, 3.5, -2.5); SELECT ''; SELECT 'plus'; -SELECT d1, f1, d1 + f1 as plus FROM t ORDER BY plus; -SELECT d2, f1, d2 + f1 as plus FROM t ORDER BY plus; -SELECT d3, f1, d3 + f1 as plus FROM t ORDER BY plus; -SELECT d4, f1, d4 + f1 as plus FROM t ORDER BY plus; +SELECT d1, f1, d1 + f1 FROM t ORDER BY f1; +SELECT d2, f1, d2 + f1 FROM t ORDER BY f1; +SELECT d3, f1, d3 + f1 FROM t ORDER BY f1; +SELECT d4, f1, d4 + f1 FROM t ORDER BY f1; -SELECT d1, f2, d1 + f2 as plus FROM t ORDER BY plus; -SELECT d2, f2, d2 + f2 as plus FROM t ORDER BY plus; -SELECT d3, f2, d3 + f2 as plus FROM t ORDER BY plus; -SELECT d4, f2, d4 + f2 as plus FROM t ORDER BY plus; +SELECT d1, f2, d1 + f2 FROM t ORDER BY f2; +SELECT d2, f2, d2 + f2 FROM t ORDER BY f2; +SELECT d3, f2, d3 + f2 FROM t ORDER BY f2; +SELECT d4, f2, d4 + f2 FROM t ORDER BY f2; SELECT ''; SELECT 'minus'; -SELECT d1, f1, d1 - f1 as minus FROM t ORDER BY minus; -SELECT d2, f1, d2 - f1 as minus FROM t ORDER BY minus; -SELECT d3, f1, d3 - f1 as minus FROM t ORDER BY minus; -SELECT d4, f1, d4 - f1 as minus FROM t ORDER BY minus; +SELECT d1, f1, d1 - f1 FROM t ORDER BY f1; +SELECT d2, f1, d2 - f1 FROM t ORDER BY f1; +SELECT d3, f1, d3 - f1 FROM t ORDER BY f1; +SELECT d4, f1, d4 - f1 FROM t ORDER BY f1; -SELECT d1, f2, d1 - f2 as minus FROM t ORDER BY minus; -SELECT d2, f2, d2 - f2 as minus FROM t ORDER BY minus; -SELECT d3, f2, d3 - f2 as minus FROM t ORDER BY minus; -SELECT d4, f2, d4 - f2 as minus FROM t ORDER BY minus; +SELECT d1, f2, d1 - f2 FROM t ORDER BY f2; +SELECT d2, f2, d2 - f2 FROM t ORDER BY f2; +SELECT d3, f2, d3 - f2 FROM t ORDER BY f2; +SELECT d4, f2, d4 - f2 FROM t ORDER BY f2; SELECT ''; SELECT 'multiply'; -SELECT d1, f1, d1 * f1 as multiply FROM t ORDER BY multiply; -SELECT d2, f1, d2 * f1 as multiply FROM t ORDER BY multiply; -SELECT d3, f1, d3 * f1 as multiply FROM t ORDER BY multiply; -SELECT d4, f1, d4 * f1 as multiply FROM t ORDER BY multiply; +SELECT d1, f1, d1 * f1 FROM t ORDER BY f1; +SELECT d2, f1, d2 * f1 FROM t ORDER BY f1; +SELECT d3, f1, d3 * f1 FROM t ORDER BY f1; +SELECT d4, f1, d4 * f1 FROM t ORDER BY f1; -SELECT d1, f2, d1 * f2 as multiply FROM t ORDER BY multiply; -SELECT d2, f2, d2 * f2 as multiply FROM t ORDER BY multiply; -SELECT d3, f2, d3 * f2 as multiply FROM t ORDER BY multiply; -SELECT d4, f2, d4 * f2 as multiply FROM t ORDER BY multiply; +SELECT d1, f2, d1 * f2 FROM t ORDER BY f2; +SELECT d2, f2, d2 * f2 FROM t ORDER BY f2; +SELECT d3, f2, d3 * f2 FROM t ORDER BY f2; +SELECT d4, f2, d4 * f2 FROM t ORDER BY f2; SELECT ''; SELECT 'division'; -SELECT d1, f1, d1 / f1 as division FROM t ORDER BY division; -SELECT d2, f1, d2 / f1 as division FROM t ORDER BY division; -SELECT d3, f1, d3 / f1 as division FROM t ORDER BY division; -SELECT d4, f1, d4 / f1 as division FROM t ORDER BY division; +SELECT d1, f1, d1 / f1 FROM t ORDER BY f1; +SELECT d2, f1, d2 / f1 FROM t ORDER BY f1; +SELECT d3, f1, d3 / f1 FROM t ORDER BY f1; +SELECT d4, f1, d4 / f1 FROM t ORDER BY f1; -SELECT d1, f2, d1 / f2 as division FROM t ORDER BY division; -SELECT d2, f2, d2 / f2 as division FROM t ORDER BY division; -SELECT d3, f2, d3 / f2 as division FROM t ORDER BY division; -SELECT d4, f2, d4 / f2 as division FROM t ORDER BY division; +SELECT d1, f2, d1 / f2 FROM t ORDER BY f2; +SELECT d2, f2, d2 / f2 FROM t ORDER BY f2; +SELECT d3, f2, d3 / f2 FROM t ORDER BY f2; +SELECT d4, f2, d4 / f2 FROM t ORDER BY f2; SELECT ''; SELECT 'least'; -SELECT d1, f1, least(d1, f1) as least FROM t ORDER BY least; -SELECT d2, f1, least(d2, f1) as least FROM t ORDER BY least; -SELECT d3, f1, least(d3, f1) as least FROM t ORDER BY least; -SELECT d4, f1, least(d4, f1) as least FROM t ORDER BY least; +SELECT d1, f1, least(d1, f1) FROM t ORDER BY f1; +SELECT d2, f1, least(d2, f1) FROM t ORDER BY f1; +SELECT d3, f1, least(d3, f1) FROM t ORDER BY f1; +SELECT d4, f1, least(d4, f1) FROM t ORDER BY f1; -SELECT d1, f2, least(d1, f2) as least FROM t ORDER BY least; -SELECT d2, f2, least(d2, f2) as least FROM t ORDER BY least; -SELECT d3, f2, least(d3, f2) as least FROM t ORDER BY least; -SELECT d4, f2, least(d4, f2) as least FROM t ORDER BY least; +SELECT d1, f2, least(d1, f2) FROM t ORDER BY f2; +SELECT d2, f2, least(d2, f2) FROM t ORDER BY f2; +SELECT d3, f2, least(d3, f2) FROM t ORDER BY f2; +SELECT d4, f2, least(d4, f2) FROM t ORDER BY f2; SELECT ''; SELECT 'greatest'; -SELECT d1, f1, greatest(d1, f1) as greatest FROM t ORDER BY greatest; -SELECT d2, f1, greatest(d2, f1) as greatest FROM t ORDER BY greatest; -SELECT d3, f1, greatest(d3, f1) as greatest FROM t ORDER BY greatest; -SELECT d4, f1, greatest(d4, f1) as greatest FROM t ORDER BY greatest; +SELECT d1, f1, greatest(d1, f1) FROM t ORDER BY f1; +SELECT d2, f1, greatest(d2, f1) FROM t ORDER BY f1; +SELECT d3, f1, greatest(d3, f1) FROM t ORDER BY f1; +SELECT d4, f1, greatest(d4, f1) FROM t ORDER BY f1; -SELECT d1, f2, greatest(d1, f2) as greatest FROM t ORDER BY greatest; -SELECT d2, f2, greatest(d2, f2) as greatest FROM t ORDER BY greatest; -SELECT d3, f2, greatest(d3, f2) as greatest FROM t ORDER BY greatest; -SELECT d4, f2, greatest(d4, f2) as greatest FROM t ORDER BY greatest; +SELECT d1, f2, greatest(d1, f2) FROM t ORDER BY f2; +SELECT d2, f2, greatest(d2, f2) FROM t ORDER BY f2; +SELECT d3, f2, greatest(d3, f2) FROM t ORDER BY f2; +SELECT d4, f2, greatest(d4, f2) FROM t ORDER BY f2; DROP TABLE t; From 442a183ec2b041e82ff00f3ee9c7c8c9a26ecaff Mon Sep 17 00:00:00 2001 From: feng lv Date: Sat, 8 Jan 2022 14:16:39 +0000 Subject: [PATCH 04/31] fix fix --- src/Functions/FunctionBinaryArithmetic.h | 25 ++++++++++++------------ 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/Functions/FunctionBinaryArithmetic.h b/src/Functions/FunctionBinaryArithmetic.h index acb973b57d8..9d3c8b2cf89 100644 --- a/src/Functions/FunctionBinaryArithmetic.h +++ b/src/Functions/FunctionBinaryArithmetic.h @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -26,22 +27,18 @@ #include #include #include +#include +#include +#include +#include +#include +#include #include #include #include -#include #include #include #include -#include "Core/DecimalFunctions.h" -#include "DivisionUtils.h" -#include "FunctionFactory.h" -#include "FunctionHelpers.h" -#include "IFunction.h" -#include "IsOperation.h" -#include "castTypeToEither.h" - -#include #if USE_EMBEDDED_COMPILER # pragma GCC diagnostic push @@ -1484,14 +1481,16 @@ public: else // we can't avoid the else because otherwise the compiler may assume the ResultDataType may be Invalid // and that would produce the compile error. { - using T0 = std::conditional_t, Float64, typename LeftDataType::FieldType>; - using T1 = std::conditional_t, Float64, typename RightDataType::FieldType>; + constexpr bool decimal_with_float = (IsDataTypeDecimal && IsFloatingPoint) + || (IsFloatingPoint && IsDataTypeDecimal); + + using T0 = std::conditional_t; + using T1 = std::conditional_t; using ResultType = typename ResultDataType::FieldType; using ColVecT0 = ColumnVectorOrDecimal; using ColVecT1 = ColumnVectorOrDecimal; using ColVecResult = ColumnVectorOrDecimal; - const IColumn * col_left_raw; const IColumn * col_right_raw; From b06211fb03c36cc6f866f88f95d401e68934e4a8 Mon Sep 17 00:00:00 2001 From: feng lv Date: Sun, 9 Jan 2022 16:29:47 +0000 Subject: [PATCH 05/31] fix fix --- src/Functions/FunctionBinaryArithmetic.h | 77 +++++-------------- .../01603_decimal_mult_float.reference | 8 +- 2 files changed, 22 insertions(+), 63 deletions(-) diff --git a/src/Functions/FunctionBinaryArithmetic.h b/src/Functions/FunctionBinaryArithmetic.h index 9d3c8b2cf89..1a05de46812 100644 --- a/src/Functions/FunctionBinaryArithmetic.h +++ b/src/Functions/FunctionBinaryArithmetic.h @@ -988,25 +988,16 @@ class FunctionBinaryArithmetic : public IFunction static constexpr const bool left_is_decimal = is_decimal; static constexpr const bool right_is_decimal = is_decimal; - static constexpr const bool result_is_decimal = IsDataTypeDecimal; typename ColVecResult::MutablePtr col_res = nullptr; - const ResultDataType type = [&] - { - if constexpr (left_is_decimal && IsFloatingPoint) - return DataTypeFloat64{}; - else if constexpr (right_is_decimal && IsFloatingPoint) - return DataTypeFloat64{}; - else - return decimalResultType(left, right); - }(); + const ResultDataType type = decimalResultType(left, right); const ResultType scale_a = [&] { if constexpr (IsDataTypeDecimal && is_division) return right.getScaleMultiplier(); // the division impl uses only the scale_a - else if constexpr (result_is_decimal) + else { if constexpr (is_multiply) // the decimal impl uses scales, but if the result is decimal, both of the arguments are decimal, @@ -1017,37 +1008,14 @@ class FunctionBinaryArithmetic : public IFunction else return type.scaleFactorFor(left, false); } - else if constexpr (left_is_decimal) - { - if (col_left_const) - // the column will be converted to native type later, no need to scale it twice. - // the explicit type is needed to specify lambda return type - return ResultType{1}; - - return 1 / DecimalUtils::convertTo(left.getScaleMultiplier(), 0); - } - else - return 1; // the default value which won't cause any re-scale }(); const ResultType scale_b = [&] { - if constexpr (result_is_decimal) - { if constexpr (is_multiply) return ResultType{1}; else return type.scaleFactorFor(right, is_division); - } - else if constexpr (right_is_decimal) - { - if (col_right_const) - return ResultType{1}; - - return 1 / DecimalUtils::convertTo(right.getScaleMultiplier(), 0); - } - else - return 1; }(); /// non-vector result @@ -1058,20 +1026,15 @@ class FunctionBinaryArithmetic : public IFunction ResultType res = {}; if (!right_nullmap || !(*right_nullmap)[0]) - res = check_decimal_overflow ? OpImplCheck::template process(const_a, const_b, scale_a, scale_b) + res = check_decimal_overflow + ? OpImplCheck::template process(const_a, const_b, scale_a, scale_b) : OpImpl::template process(const_a, const_b, scale_a, scale_b); - if constexpr (result_is_decimal) - return ResultDataType(type.getPrecision(), type.getScale()).createColumnConst( - col_left_const->size(), toField(res, type.getScale())); - else - return ResultDataType().createColumnConst(col_left_const->size(), toField(res)); + return ResultDataType(type.getPrecision(), type.getScale()) + .createColumnConst(col_left_const->size(), toField(res, type.getScale())); } - if constexpr (result_is_decimal) - col_res = ColVecResult::create(0, type.getScale()); - else - col_res = ColVecResult::create(0); + col_res = ColVecResult::create(0, type.getScale()); auto & vec_res = col_res->getData(); vec_res.resize(col_left_size); @@ -1491,23 +1454,23 @@ public: using ColVecT1 = ColumnVectorOrDecimal; using ColVecResult = ColumnVectorOrDecimal; - const IColumn * col_left_raw; - const IColumn * col_right_raw; + ColumnPtr left_col = nullptr; + ColumnPtr right_col = nullptr; /// When Decimal op Float32/64, convert both of them into Float64 - if constexpr ((IsDataTypeDecimal || IsDataTypeDecimal)&&IsFloatingPoint) + if constexpr (decimal_with_float) { const auto converted_type = std::make_shared(); - auto c0_converted = castColumn(arguments[0], converted_type); - auto c1_converted = castColumn(arguments[1], converted_type); - col_left_raw = c0_converted.get(); - col_right_raw = c1_converted.get(); + left_col = castColumn(std::move(arguments[0]), converted_type); + right_col = castColumn(std::move(arguments[1]), converted_type); } else { - col_left_raw = arguments[0].column.get(); - col_right_raw = arguments[1].column.get(); + left_col = std::move(arguments[0].column); + right_col = std::move(arguments[1].column); } + const auto * const col_left_raw = left_col.get(); + const auto * const col_right_raw = right_col.get(); const size_t col_left_size = col_left_raw->size(); @@ -1517,7 +1480,7 @@ public: const ColVecT0 * const col_left = checkAndGetColumn(col_left_raw); const ColVecT1 * const col_right = checkAndGetColumn(col_right_raw); - if constexpr ((IsDataTypeDecimal || IsDataTypeDecimal)&&!IsFloatingPoint) + if constexpr (IsDataTypeDecimal) { return executeNumericWithDecimal( left, right, @@ -1571,11 +1534,7 @@ public: const T1 value = col_right_const->template getValue(); OpImpl::template process( - col_left->getData().data(), - &value, - vec_res.data(), - vec_res.size(), - right_nullmap); + col_left->getData().data(), &value, vec_res.data(), vec_res.size(), right_nullmap); } else return nullptr; diff --git a/tests/queries/0_stateless/01603_decimal_mult_float.reference b/tests/queries/0_stateless/01603_decimal_mult_float.reference index ee1eeb525ba..c2917516e99 100644 --- a/tests/queries/0_stateless/01603_decimal_mult_float.reference +++ b/tests/queries/0_stateless/01603_decimal_mult_float.reference @@ -5,10 +5,10 @@ 7.775900000000001 56.62269 598.8376688440277 -299.41883723437786 +299.41883695311844 0.7485470860550345 -2.245641373854596 +2.2456412771483882 1.641386318314034 1.641386318314034 -1.641386334333447 -1.641386334333447 +1.6413863258732018 +1.6413863258732018 From 8346057be6a1f0e83628d18bd146d08e48d7a414 Mon Sep 17 00:00:00 2001 From: feng lv Date: Mon, 10 Jan 2022 03:05:45 +0000 Subject: [PATCH 06/31] fix --- src/Functions/FunctionBinaryArithmetic.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Functions/FunctionBinaryArithmetic.h b/src/Functions/FunctionBinaryArithmetic.h index 1a05de46812..c73fe156808 100644 --- a/src/Functions/FunctionBinaryArithmetic.h +++ b/src/Functions/FunctionBinaryArithmetic.h @@ -1461,13 +1461,13 @@ public: if constexpr (decimal_with_float) { const auto converted_type = std::make_shared(); - left_col = castColumn(std::move(arguments[0]), converted_type); - right_col = castColumn(std::move(arguments[1]), converted_type); + left_col = castColumn(arguments[0], converted_type); + right_col = castColumn(arguments[1], converted_type); } else { - left_col = std::move(arguments[0].column); - right_col = std::move(arguments[1].column); + left_col = arguments[0].column; + right_col = arguments[1].column; } const auto * const col_left_raw = left_col.get(); const auto * const col_right_raw = right_col.get(); From 30e0df1e8cf1ce163e7fbd5ab1069f5dad501fa2 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 10 Jan 2022 22:01:41 +0300 Subject: [PATCH 07/31] Forward declaration of IStorage in InterpreterWatchQuery --- src/Interpreters/IInterpreterUnionOrSelectQuery.cpp | 1 + src/Interpreters/InterpreterWatchQuery.cpp | 1 + src/Interpreters/InterpreterWatchQuery.h | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Interpreters/IInterpreterUnionOrSelectQuery.cpp b/src/Interpreters/IInterpreterUnionOrSelectQuery.cpp index 55c007e2713..fb02ba3dc5a 100644 --- a/src/Interpreters/IInterpreterUnionOrSelectQuery.cpp +++ b/src/Interpreters/IInterpreterUnionOrSelectQuery.cpp @@ -3,6 +3,7 @@ #include #include #include +#include namespace DB { diff --git a/src/Interpreters/InterpreterWatchQuery.cpp b/src/Interpreters/InterpreterWatchQuery.cpp index 9e23cdf2af2..8a079ee471d 100644 --- a/src/Interpreters/InterpreterWatchQuery.cpp +++ b/src/Interpreters/InterpreterWatchQuery.cpp @@ -16,6 +16,7 @@ limitations under the License. */ #include #include #include +#include namespace DB diff --git a/src/Interpreters/InterpreterWatchQuery.h b/src/Interpreters/InterpreterWatchQuery.h index ac167182a71..1ca8f18dd67 100644 --- a/src/Interpreters/InterpreterWatchQuery.h +++ b/src/Interpreters/InterpreterWatchQuery.h @@ -15,7 +15,7 @@ limitations under the License. */ #include #include #include -#include +#include #include namespace DB From aee034a5979ea2b4439aa25a0570b591604e65ad Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 10 Jan 2022 22:01:41 +0300 Subject: [PATCH 08/31] Use explicit template instantiation for SystemLog - Move some code into module part to avoid dependency from IStorage in SystemLog - Remove extra headers from SystemLog.h - Rewrite some code that was relying on headers that was included by SystemLog.h v2: rebase v3: squash move into module part with explicit template instantiation (to make each commit self compilable after rebase) --- base/daemon/BaseDaemon.cpp | 1 + base/loggers/OwnSplitChannel.cpp | 1 + programs/local/LocalServer.cpp | 1 + src/Common/ThreadStatus.cpp | 2 + src/Common/ZooKeeper/ZooKeeperCommon.cpp | 1 + src/Coordination/FourLetterCommand.cpp | 2 + src/Coordination/KeeperStorage.cpp | 14 +- src/Coordination/KeeperStorage.h | 1 + src/Dictionaries/SSDCacheDictionaryStorage.h | 1 + src/Interpreters/AsynchronousMetricLog.h | 2 + src/Interpreters/CrashLog.h | 2 + src/Interpreters/IInterpreter.cpp | 1 + src/Interpreters/InterpreterAlterQuery.cpp | 1 + src/Interpreters/InterpreterCreateQuery.cpp | 1 + src/Interpreters/InterpreterExplainQuery.cpp | 2 + src/Interpreters/MetricLog.h | 2 + src/Interpreters/OpenTelemetrySpanLog.cpp | 2 + src/Interpreters/OpenTelemetrySpanLog.h | 2 + src/Interpreters/PartLog.h | 2 + src/Interpreters/QueryLog.h | 2 + src/Interpreters/QueryThreadLog.h | 2 + src/Interpreters/QueryViewsLog.h | 2 + src/Interpreters/SessionLog.cpp | 1 + src/Interpreters/SessionLog.h | 3 + src/Interpreters/SystemLog.cpp | 444 ++++++++++++++++ src/Interpreters/SystemLog.h | 484 ++---------------- src/Interpreters/TextLog.cpp | 1 + src/Interpreters/TextLog.h | 4 + src/Interpreters/ThreadStatusExt.cpp | 1 + src/Interpreters/TraceLog.h | 2 + src/Interpreters/ZooKeeperLog.h | 1 + src/Interpreters/executeQuery.cpp | 2 + src/Storages/MergeTree/IMergeTreeDataPart.cpp | 2 + .../MergeTree/ReplicatedMergeTreeQueue.cpp | 5 +- src/Storages/ProjectionsDescription.cpp | 1 + src/Storages/StorageDistributed.cpp | 3 + src/Storages/StorageMemory.cpp | 1 + src/Storages/StorageMergeTree.cpp | 1 + src/Storages/StorageReplicatedMergeTree.cpp | 2 + src/Storages/WindowView/StorageWindowView.cpp | 4 + 40 files changed, 553 insertions(+), 456 deletions(-) diff --git a/base/daemon/BaseDaemon.cpp b/base/daemon/BaseDaemon.cpp index b92a68f104e..f3026d7c87a 100644 --- a/base/daemon/BaseDaemon.cpp +++ b/base/daemon/BaseDaemon.cpp @@ -51,6 +51,7 @@ #include #include #include +#include #include #include diff --git a/base/loggers/OwnSplitChannel.cpp b/base/loggers/OwnSplitChannel.cpp index 2ae1e65729c..854dcacdf6f 100644 --- a/base/loggers/OwnSplitChannel.cpp +++ b/base/loggers/OwnSplitChannel.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include diff --git a/programs/local/LocalServer.cpp b/programs/local/LocalServer.cpp index aa4747636c9..a294857ace8 100644 --- a/programs/local/LocalServer.cpp +++ b/programs/local/LocalServer.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include diff --git a/src/Common/ThreadStatus.cpp b/src/Common/ThreadStatus.cpp index 411f725f2db..ad42468de68 100644 --- a/src/Common/ThreadStatus.cpp +++ b/src/Common/ThreadStatus.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -11,6 +12,7 @@ #include #include +#include namespace DB diff --git a/src/Common/ZooKeeper/ZooKeeperCommon.cpp b/src/Common/ZooKeeper/ZooKeeperCommon.cpp index 6a449cf0122..f6c9a3d3ca0 100644 --- a/src/Common/ZooKeeper/ZooKeeperCommon.cpp +++ b/src/Common/ZooKeeper/ZooKeeperCommon.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include diff --git a/src/Coordination/FourLetterCommand.cpp b/src/Coordination/FourLetterCommand.cpp index 294e623d803..3d0ebe86bf3 100644 --- a/src/Coordination/FourLetterCommand.cpp +++ b/src/Coordination/FourLetterCommand.cpp @@ -9,6 +9,8 @@ #include #include #include +#include +#include #include diff --git a/src/Coordination/KeeperStorage.cpp b/src/Coordination/KeeperStorage.cpp index a64a7d425f6..4f174e4e803 100644 --- a/src/Coordination/KeeperStorage.cpp +++ b/src/Coordination/KeeperStorage.cpp @@ -1,16 +1,18 @@ #include #include #include -#include -#include -#include #include -#include -#include +#include +#include +#include #include #include #include -#include +#include +#include +#include +#include +#include namespace DB { diff --git a/src/Coordination/KeeperStorage.h b/src/Coordination/KeeperStorage.h index f61b17a88a6..11d191b7f50 100644 --- a/src/Coordination/KeeperStorage.h +++ b/src/Coordination/KeeperStorage.h @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include diff --git a/src/Dictionaries/SSDCacheDictionaryStorage.h b/src/Dictionaries/SSDCacheDictionaryStorage.h index e30b0a257d9..7c7dc838436 100644 --- a/src/Dictionaries/SSDCacheDictionaryStorage.h +++ b/src/Dictionaries/SSDCacheDictionaryStorage.h @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include diff --git a/src/Interpreters/AsynchronousMetricLog.h b/src/Interpreters/AsynchronousMetricLog.h index 6275572935c..d0f07a041b7 100644 --- a/src/Interpreters/AsynchronousMetricLog.h +++ b/src/Interpreters/AsynchronousMetricLog.h @@ -3,6 +3,8 @@ #include #include #include +#include +#include #include #include diff --git a/src/Interpreters/CrashLog.h b/src/Interpreters/CrashLog.h index ba27c1f513e..61503d95cee 100644 --- a/src/Interpreters/CrashLog.h +++ b/src/Interpreters/CrashLog.h @@ -1,6 +1,8 @@ #pragma once #include +#include +#include /// Call this function on crash. diff --git a/src/Interpreters/IInterpreter.cpp b/src/Interpreters/IInterpreter.cpp index 1b0e9738429..af0c06e7503 100644 --- a/src/Interpreters/IInterpreter.cpp +++ b/src/Interpreters/IInterpreter.cpp @@ -1,5 +1,6 @@ #include #include +#include namespace DB { diff --git a/src/Interpreters/InterpreterAlterQuery.cpp b/src/Interpreters/InterpreterAlterQuery.cpp index 2475d437acb..d01f2b05567 100644 --- a/src/Interpreters/InterpreterAlterQuery.cpp +++ b/src/Interpreters/InterpreterAlterQuery.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include diff --git a/src/Interpreters/InterpreterCreateQuery.cpp b/src/Interpreters/InterpreterCreateQuery.cpp index 7ddb0c8c26e..e90029ca59a 100644 --- a/src/Interpreters/InterpreterCreateQuery.cpp +++ b/src/Interpreters/InterpreterCreateQuery.cpp @@ -39,6 +39,7 @@ #include #include #include +#include #include #include diff --git a/src/Interpreters/InterpreterExplainQuery.cpp b/src/Interpreters/InterpreterExplainQuery.cpp index fdb35637a9a..37b944d72d6 100644 --- a/src/Interpreters/InterpreterExplainQuery.cpp +++ b/src/Interpreters/InterpreterExplainQuery.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -15,6 +16,7 @@ #include #include #include +#include #include #include diff --git a/src/Interpreters/MetricLog.h b/src/Interpreters/MetricLog.h index c43c2872788..579e741c479 100644 --- a/src/Interpreters/MetricLog.h +++ b/src/Interpreters/MetricLog.h @@ -3,6 +3,8 @@ #include #include #include +#include +#include #include #include diff --git a/src/Interpreters/OpenTelemetrySpanLog.cpp b/src/Interpreters/OpenTelemetrySpanLog.cpp index 89cce890555..4c415800a20 100644 --- a/src/Interpreters/OpenTelemetrySpanLog.cpp +++ b/src/Interpreters/OpenTelemetrySpanLog.cpp @@ -8,8 +8,10 @@ #include #include #include +#include #include +#include namespace DB diff --git a/src/Interpreters/OpenTelemetrySpanLog.h b/src/Interpreters/OpenTelemetrySpanLog.h index b287301325c..8dfc2eccc00 100644 --- a/src/Interpreters/OpenTelemetrySpanLog.h +++ b/src/Interpreters/OpenTelemetrySpanLog.h @@ -1,6 +1,8 @@ #pragma once #include +#include +#include namespace DB { diff --git a/src/Interpreters/PartLog.h b/src/Interpreters/PartLog.h index b2d18e4d40d..bdd1db4334a 100644 --- a/src/Interpreters/PartLog.h +++ b/src/Interpreters/PartLog.h @@ -1,6 +1,8 @@ #pragma once #include +#include +#include namespace DB diff --git a/src/Interpreters/QueryLog.h b/src/Interpreters/QueryLog.h index 49c38e7d2a9..f015afb9249 100644 --- a/src/Interpreters/QueryLog.h +++ b/src/Interpreters/QueryLog.h @@ -1,6 +1,8 @@ #pragma once +#include #include +#include #include #include diff --git a/src/Interpreters/QueryThreadLog.h b/src/Interpreters/QueryThreadLog.h index f826ebac4fd..3b260b71441 100644 --- a/src/Interpreters/QueryThreadLog.h +++ b/src/Interpreters/QueryThreadLog.h @@ -2,6 +2,8 @@ #include #include +#include +#include namespace ProfileEvents diff --git a/src/Interpreters/QueryViewsLog.h b/src/Interpreters/QueryViewsLog.h index 254cacd2387..c0936e52a1c 100644 --- a/src/Interpreters/QueryViewsLog.h +++ b/src/Interpreters/QueryViewsLog.h @@ -9,6 +9,8 @@ #include #include #include +#include +#include #include #include diff --git a/src/Interpreters/SessionLog.cpp b/src/Interpreters/SessionLog.cpp index f9419088df8..d9698be1a9b 100644 --- a/src/Interpreters/SessionLog.cpp +++ b/src/Interpreters/SessionLog.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include diff --git a/src/Interpreters/SessionLog.h b/src/Interpreters/SessionLog.h index 93766d685e0..26f137565cb 100644 --- a/src/Interpreters/SessionLog.h +++ b/src/Interpreters/SessionLog.h @@ -3,6 +3,9 @@ #include #include #include +#include +#include +#include namespace DB { diff --git a/src/Interpreters/SystemLog.cpp b/src/Interpreters/SystemLog.cpp index fc2a5b620e2..43f02abc7eb 100644 --- a/src/Interpreters/SystemLog.cpp +++ b/src/Interpreters/SystemLog.cpp @@ -10,17 +10,37 @@ #include #include #include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include #include #include +#include +#define DBMS_SYSTEM_LOG_QUEUE_SIZE 1048576 + namespace DB { namespace ErrorCodes { extern const int BAD_ARGUMENTS; + extern const int TIMEOUT_EXCEEDED; + extern const int LOGICAL_ERROR; } namespace @@ -96,6 +116,9 @@ std::shared_ptr createSystemLog( } +/// +/// ISystemLog +/// ASTPtr ISystemLog::getCreateTableQueryClean(const StorageID & table_id, ContextPtr context) { DatabasePtr database = DatabaseCatalog::instance().getDatabase(table_id.database_name); @@ -111,7 +134,40 @@ ASTPtr ISystemLog::getCreateTableQueryClean(const StorageID & table_id, ContextP return old_ast; } +void ISystemLog::stopFlushThread() +{ + { + std::lock_guard lock(mutex); + if (!saving_thread.joinable()) + { + return; + } + + if (is_shutdown) + { + return; + } + + is_shutdown = true; + + /// Tell thread to shutdown. + flush_event.notify_all(); + } + + saving_thread.join(); +} + +void ISystemLog::startup() +{ + std::lock_guard lock(mutex); + saving_thread = ThreadFromGlobalPool([this] { savingThreadFunction(); }); +} + + +/// +/// SystemLogs +/// SystemLogs::SystemLogs(ContextPtr global_context, const Poco::Util::AbstractConfiguration & config) { query_log = createSystemLog(global_context, "system", "query_log", config, "query_log"); @@ -193,4 +249,392 @@ void SystemLogs::shutdown() log->shutdown(); } +/// +/// SystemLog +/// +template +SystemLog::SystemLog( + ContextPtr context_, + const String & database_name_, + const String & table_name_, + const String & storage_def_, + size_t flush_interval_milliseconds_) + : WithContext(context_) + , table_id(database_name_, table_name_) + , storage_def(storage_def_) + , create_query(serializeAST(*getCreateTableQuery())) + , flush_interval_milliseconds(flush_interval_milliseconds_) +{ + assert(database_name_ == DatabaseCatalog::SYSTEM_DATABASE); + log = &Poco::Logger::get("SystemLog (" + database_name_ + "." + table_name_ + ")"); +} + + +static thread_local bool recursive_add_call = false; + +template +void SystemLog::add(const LogElement & element) +{ + /// It is possible that the method will be called recursively. + /// Better to drop these events to avoid complications. + if (recursive_add_call) + return; + recursive_add_call = true; + SCOPE_EXIT({ recursive_add_call = false; }); + + /// Memory can be allocated while resizing on queue.push_back. + /// The size of allocation can be in order of a few megabytes. + /// But this should not be accounted for query memory usage. + /// Otherwise the tests like 01017_uniqCombined_memory_usage.sql will be flacky. + MemoryTracker::BlockerInThread temporarily_disable_memory_tracker(VariableContext::Global); + + /// Should not log messages under mutex. + bool queue_is_half_full = false; + + { + std::unique_lock lock(mutex); + + if (is_shutdown) + return; + + if (queue.size() == DBMS_SYSTEM_LOG_QUEUE_SIZE / 2) + { + queue_is_half_full = true; + + // The queue more than half full, time to flush. + // We only check for strict equality, because messages are added one + // by one, under exclusive lock, so we will see each message count. + // It is enough to only wake the flushing thread once, after the message + // count increases past half available size. + const uint64_t queue_end = queue_front_index + queue.size(); + if (requested_flush_up_to < queue_end) + requested_flush_up_to = queue_end; + + flush_event.notify_all(); + } + + if (queue.size() >= DBMS_SYSTEM_LOG_QUEUE_SIZE) + { + // Ignore all further entries until the queue is flushed. + // Log a message about that. Don't spam it -- this might be especially + // problematic in case of trace log. Remember what the front index of the + // queue was when we last logged the message. If it changed, it means the + // queue was flushed, and we can log again. + if (queue_front_index != logged_queue_full_at_index) + { + logged_queue_full_at_index = queue_front_index; + + // TextLog sets its logger level to 0, so this log is a noop and + // there is no recursive logging. + lock.unlock(); + LOG_ERROR(log, "Queue is full for system log '{}' at {}", demangle(typeid(*this).name()), queue_front_index); + } + + return; + } + + queue.push_back(element); + } + + if (queue_is_half_full) + LOG_INFO(log, "Queue is half full for system log '{}'.", demangle(typeid(*this).name())); +} + +template +void SystemLog::shutdown() +{ + stopFlushThread(); + + auto table = DatabaseCatalog::instance().tryGetTable(table_id, getContext()); + if (table) + table->flushAndShutdown(); +} + +template +void SystemLog::flush(bool force) +{ + uint64_t this_thread_requested_offset; + + { + std::unique_lock lock(mutex); + + if (is_shutdown) + return; + + this_thread_requested_offset = queue_front_index + queue.size(); + + // Publish our flush request, taking care not to overwrite the requests + // made by other threads. + is_force_prepare_tables |= force; + requested_flush_up_to = std::max(requested_flush_up_to, + this_thread_requested_offset); + + flush_event.notify_all(); + } + + LOG_DEBUG(log, "Requested flush up to offset {}", + this_thread_requested_offset); + + // Use an arbitrary timeout to avoid endless waiting. 60s proved to be + // too fast for our parallel functional tests, probably because they + // heavily load the disk. + const int timeout_seconds = 180; + std::unique_lock lock(mutex); + bool result = flush_event.wait_for(lock, std::chrono::seconds(timeout_seconds), + [&] { return flushed_up_to >= this_thread_requested_offset + && !is_force_prepare_tables; }); + + if (!result) + { + throw Exception("Timeout exceeded (" + toString(timeout_seconds) + " s) while flushing system log '" + demangle(typeid(*this).name()) + "'.", + ErrorCodes::TIMEOUT_EXCEEDED); + } +} + + +template +void SystemLog::savingThreadFunction() +{ + setThreadName("SystemLogFlush"); + + std::vector to_flush; + bool exit_this_thread = false; + while (!exit_this_thread) + { + try + { + // The end index (exclusive, like std end()) of the messages we are + // going to flush. + uint64_t to_flush_end = 0; + // Should we prepare table even if there are no new messages. + bool should_prepare_tables_anyway = false; + + { + std::unique_lock lock(mutex); + flush_event.wait_for(lock, + std::chrono::milliseconds(flush_interval_milliseconds), + [&] () + { + return requested_flush_up_to > flushed_up_to || is_shutdown || is_force_prepare_tables; + } + ); + + queue_front_index += queue.size(); + to_flush_end = queue_front_index; + // Swap with existing array from previous flush, to save memory + // allocations. + to_flush.resize(0); + queue.swap(to_flush); + + should_prepare_tables_anyway = is_force_prepare_tables; + + exit_this_thread = is_shutdown; + } + + if (to_flush.empty()) + { + if (should_prepare_tables_anyway) + { + prepareTable(); + LOG_TRACE(log, "Table created (force)"); + + std::lock_guard lock(mutex); + is_force_prepare_tables = false; + flush_event.notify_all(); + } + } + else + { + flushImpl(to_flush, to_flush_end); + } + } + catch (...) + { + tryLogCurrentException(__PRETTY_FUNCTION__); + } + } + LOG_TRACE(log, "Terminating"); +} + + +template +void SystemLog::flushImpl(const std::vector & to_flush, uint64_t to_flush_end) +{ + try + { + LOG_TRACE(log, "Flushing system log, {} entries to flush up to offset {}", + to_flush.size(), to_flush_end); + + /// We check for existence of the table and create it as needed at every + /// flush. This is done to allow user to drop the table at any moment + /// (new empty table will be created automatically). BTW, flush method + /// is called from single thread. + prepareTable(); + + ColumnsWithTypeAndName log_element_columns; + auto log_element_names_and_types = LogElement::getNamesAndTypes(); + + for (const auto & name_and_type : log_element_names_and_types) + log_element_columns.emplace_back(name_and_type.type, name_and_type.name); + + Block block(std::move(log_element_columns)); + + MutableColumns columns = block.mutateColumns(); + for (const auto & elem : to_flush) + elem.appendToBlock(columns); + + block.setColumns(std::move(columns)); + + /// We write to table indirectly, using InterpreterInsertQuery. + /// This is needed to support DEFAULT-columns in table. + + std::unique_ptr insert = std::make_unique(); + insert->table_id = table_id; + ASTPtr query_ptr(insert.release()); + + // we need query context to do inserts to target table with MV containing subqueries or joins + auto insert_context = Context::createCopy(context); + insert_context->makeQueryContext(); + + InterpreterInsertQuery interpreter(query_ptr, insert_context); + BlockIO io = interpreter.execute(); + + PushingPipelineExecutor executor(io.pipeline); + + executor.start(); + executor.push(block); + executor.finish(); + } + catch (...) + { + tryLogCurrentException(__PRETTY_FUNCTION__); + } + + { + std::lock_guard lock(mutex); + flushed_up_to = to_flush_end; + is_force_prepare_tables = false; + flush_event.notify_all(); + } + + LOG_TRACE(log, "Flushed system log up to offset {}", to_flush_end); +} + + +template +void SystemLog::prepareTable() +{ + String description = table_id.getNameForLogs(); + + auto table = DatabaseCatalog::instance().tryGetTable(table_id, getContext()); + if (table) + { + if (old_create_query.empty()) + { + old_create_query = serializeAST(*getCreateTableQueryClean(table_id, getContext())); + if (old_create_query.empty()) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Empty CREATE QUERY for {}", backQuoteIfNeed(table_id.table_name)); + } + + if (old_create_query != create_query) + { + /// Rename the existing table. + int suffix = 0; + while (DatabaseCatalog::instance().isTableExist( + {table_id.database_name, table_id.table_name + "_" + toString(suffix)}, getContext())) + ++suffix; + + auto rename = std::make_shared(); + + ASTRenameQuery::Table from; + from.database = table_id.database_name; + from.table = table_id.table_name; + + ASTRenameQuery::Table to; + to.database = table_id.database_name; + to.table = table_id.table_name + "_" + toString(suffix); + + ASTRenameQuery::Element elem; + elem.from = from; + elem.to = to; + + rename->elements.emplace_back(elem); + + LOG_DEBUG( + log, + "Existing table {} for system log has obsolete or different structure. Renaming it to {}.\nOld: {}\nNew: {}\n.", + description, + backQuoteIfNeed(to.table), + old_create_query, + create_query); + + auto query_context = Context::createCopy(context); + query_context->makeQueryContext(); + InterpreterRenameQuery(rename, query_context).execute(); + + /// The required table will be created. + table = nullptr; + } + else if (!is_prepared) + LOG_DEBUG(log, "Will use existing table {} for {}", description, LogElement::name()); + } + + if (!table) + { + /// Create the table. + LOG_DEBUG(log, "Creating new table {} for {}", description, LogElement::name()); + + auto query_context = Context::createCopy(context); + query_context->makeQueryContext(); + + auto create_query_ast = getCreateTableQuery(); + InterpreterCreateQuery interpreter(create_query_ast, query_context); + interpreter.setInternal(true); + interpreter.execute(); + + table = DatabaseCatalog::instance().getTable(table_id, getContext()); + + old_create_query.clear(); + } + + is_prepared = true; +} + + +template +ASTPtr SystemLog::getCreateTableQuery() +{ + auto create = std::make_shared(); + + create->setDatabase(table_id.database_name); + create->setTable(table_id.table_name); + + auto ordinary_columns = LogElement::getNamesAndTypes(); + auto alias_columns = LogElement::getNamesAndAliases(); + auto new_columns_list = std::make_shared(); + new_columns_list->set(new_columns_list->columns, InterpreterCreateQuery::formatColumns(ordinary_columns, alias_columns)); + create->set(create->columns_list, new_columns_list); + + ParserStorage storage_parser; + ASTPtr storage_ast = parseQuery( + storage_parser, storage_def.data(), storage_def.data() + storage_def.size(), + "Storage to create table for " + LogElement::name(), 0, DBMS_DEFAULT_MAX_PARSER_DEPTH); + create->set(create->storage, storage_ast); + + return create; +} + +template class SystemLog; +template class SystemLog; +template class SystemLog; +template class SystemLog; +template class SystemLog; +template class SystemLog; +template class SystemLog; +template class SystemLog; +template class SystemLog; +template class SystemLog; +template class SystemLog; +template class SystemLog; + } diff --git a/src/Interpreters/SystemLog.h b/src/Interpreters/SystemLog.h index 46254d0c3a2..3209dd2e13e 100644 --- a/src/Interpreters/SystemLog.h +++ b/src/Interpreters/SystemLog.h @@ -4,32 +4,27 @@ #include #include #include - #include #include -#include -#include + #include #include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include +#include +#include +#include +#include #include -#include -#include +namespace Poco +{ +class Logger; +namespace Util +{ +class AbstractConfiguration; +} +} + namespace DB { @@ -58,14 +53,6 @@ namespace DB }; */ -namespace ErrorCodes -{ - extern const int TIMEOUT_EXCEEDED; - extern const int LOGICAL_ERROR; -} - -#define DBMS_SYSTEM_LOG_QUEUE_SIZE 1048576 - class QueryLog; class QueryThreadLog; class PartLog; @@ -87,15 +74,33 @@ public: //// force -- force table creation (used for SYSTEM FLUSH LOGS) virtual void flush(bool force = false) = 0; virtual void prepareTable() = 0; - virtual void startup() = 0; + + /// Start the background thread. + virtual void startup(); + + /// Stop the background flush thread before destructor. No more data will be written. virtual void shutdown() = 0; + virtual ~ISystemLog() = default; + virtual void savingThreadFunction() = 0; + /// returns CREATE TABLE query, but with removed: /// - UUID /// - SETTINGS (for MergeTree) /// That way it can be used to compare with the SystemLog::getCreateTableQuery() static ASTPtr getCreateTableQueryClean(const StorageID & table_id, ContextPtr context); + +protected: + ThreadFromGlobalPool saving_thread; + + /// Data shared between callers of add()/flush()/shutdown(), and the saving thread + std::mutex mutex; + + bool is_shutdown = false; + std::condition_variable flush_event; + + void stopFlushThread(); }; @@ -156,23 +161,10 @@ public: */ void add(const LogElement & element); - void stopFlushThread(); + void shutdown() override; /// Flush data in the buffer to disk - void flush(bool force = false) override; - - /// Start the background thread. - void startup() override; - - /// Stop the background flush thread before destructor. No more data will be written. - void shutdown() override - { - stopFlushThread(); - - auto table = DatabaseCatalog::instance().tryGetTable(table_id, getContext()); - if (table) - table->flushAndShutdown(); - } + void flush(bool force) override; String getName() override { @@ -192,10 +184,7 @@ private: String old_create_query; bool is_prepared = false; const size_t flush_interval_milliseconds; - ThreadFromGlobalPool saving_thread; - /* Data shared between callers of add()/flush()/shutdown(), and the saving thread */ - std::mutex mutex; // Queue is bounded. But its size is quite large to not block in all normal cases. std::vector queue; // An always-incrementing index of the first message currently in the queue. @@ -203,10 +192,8 @@ private: // can wait until a particular message is flushed. This is used to implement // synchronous log flushing for SYSTEM FLUSH LOGS. uint64_t queue_front_index = 0; - bool is_shutdown = false; // A flag that says we must create the tables even if the queue is empty. bool is_force_prepare_tables = false; - std::condition_variable flush_event; // Requested to flush logs up to this index, exclusive uint64_t requested_flush_up_to = 0; // Flushed log up to this index, exclusive @@ -214,7 +201,7 @@ private: // Logged overflow message at this queue front index uint64_t logged_queue_full_at_index = -1; - void savingThreadFunction(); + void savingThreadFunction() override; /** Creates new table if it does not exist. * Renames old table if its structure is not suitable. @@ -226,403 +213,4 @@ private: void flushImpl(const std::vector & to_flush, uint64_t to_flush_end); }; - -template -SystemLog::SystemLog( - ContextPtr context_, - const String & database_name_, - const String & table_name_, - const String & storage_def_, - size_t flush_interval_milliseconds_) - : WithContext(context_) - , table_id(database_name_, table_name_) - , storage_def(storage_def_) - , create_query(serializeAST(*getCreateTableQuery())) - , flush_interval_milliseconds(flush_interval_milliseconds_) -{ - assert(database_name_ == DatabaseCatalog::SYSTEM_DATABASE); - log = &Poco::Logger::get("SystemLog (" + database_name_ + "." + table_name_ + ")"); -} - - -template -void SystemLog::startup() -{ - std::lock_guard lock(mutex); - saving_thread = ThreadFromGlobalPool([this] { savingThreadFunction(); }); -} - - -static thread_local bool recursive_add_call = false; - -template -void SystemLog::add(const LogElement & element) -{ - /// It is possible that the method will be called recursively. - /// Better to drop these events to avoid complications. - if (recursive_add_call) - return; - recursive_add_call = true; - SCOPE_EXIT({ recursive_add_call = false; }); - - /// Memory can be allocated while resizing on queue.push_back. - /// The size of allocation can be in order of a few megabytes. - /// But this should not be accounted for query memory usage. - /// Otherwise the tests like 01017_uniqCombined_memory_usage.sql will be flacky. - MemoryTracker::BlockerInThread temporarily_disable_memory_tracker(VariableContext::Global); - - /// Should not log messages under mutex. - bool queue_is_half_full = false; - - { - std::unique_lock lock(mutex); - - if (is_shutdown) - return; - - if (queue.size() == DBMS_SYSTEM_LOG_QUEUE_SIZE / 2) - { - queue_is_half_full = true; - - // The queue more than half full, time to flush. - // We only check for strict equality, because messages are added one - // by one, under exclusive lock, so we will see each message count. - // It is enough to only wake the flushing thread once, after the message - // count increases past half available size. - const uint64_t queue_end = queue_front_index + queue.size(); - if (requested_flush_up_to < queue_end) - requested_flush_up_to = queue_end; - - flush_event.notify_all(); - } - - if (queue.size() >= DBMS_SYSTEM_LOG_QUEUE_SIZE) - { - // Ignore all further entries until the queue is flushed. - // Log a message about that. Don't spam it -- this might be especially - // problematic in case of trace log. Remember what the front index of the - // queue was when we last logged the message. If it changed, it means the - // queue was flushed, and we can log again. - if (queue_front_index != logged_queue_full_at_index) - { - logged_queue_full_at_index = queue_front_index; - - // TextLog sets its logger level to 0, so this log is a noop and - // there is no recursive logging. - lock.unlock(); - LOG_ERROR(log, "Queue is full for system log '{}' at {}", demangle(typeid(*this).name()), queue_front_index); - } - - return; - } - - queue.push_back(element); - } - - if (queue_is_half_full) - LOG_INFO(log, "Queue is half full for system log '{}'.", demangle(typeid(*this).name())); -} - - -template -void SystemLog::flush(bool force) -{ - uint64_t this_thread_requested_offset; - - { - std::unique_lock lock(mutex); - - if (is_shutdown) - return; - - this_thread_requested_offset = queue_front_index + queue.size(); - - // Publish our flush request, taking care not to overwrite the requests - // made by other threads. - is_force_prepare_tables |= force; - requested_flush_up_to = std::max(requested_flush_up_to, - this_thread_requested_offset); - - flush_event.notify_all(); - } - - LOG_DEBUG(log, "Requested flush up to offset {}", - this_thread_requested_offset); - - // Use an arbitrary timeout to avoid endless waiting. 60s proved to be - // too fast for our parallel functional tests, probably because they - // heavily load the disk. - const int timeout_seconds = 180; - std::unique_lock lock(mutex); - bool result = flush_event.wait_for(lock, std::chrono::seconds(timeout_seconds), - [&] { return flushed_up_to >= this_thread_requested_offset - && !is_force_prepare_tables; }); - - if (!result) - { - throw Exception("Timeout exceeded (" + toString(timeout_seconds) + " s) while flushing system log '" + demangle(typeid(*this).name()) + "'.", - ErrorCodes::TIMEOUT_EXCEEDED); - } -} - - -template -void SystemLog::stopFlushThread() -{ - { - std::lock_guard lock(mutex); - - if (!saving_thread.joinable()) - { - return; - } - - if (is_shutdown) - { - return; - } - - is_shutdown = true; - - /// Tell thread to shutdown. - flush_event.notify_all(); - } - - saving_thread.join(); -} - - -template -void SystemLog::savingThreadFunction() -{ - setThreadName("SystemLogFlush"); - - std::vector to_flush; - bool exit_this_thread = false; - while (!exit_this_thread) - { - try - { - // The end index (exclusive, like std end()) of the messages we are - // going to flush. - uint64_t to_flush_end = 0; - // Should we prepare table even if there are no new messages. - bool should_prepare_tables_anyway = false; - - { - std::unique_lock lock(mutex); - flush_event.wait_for(lock, - std::chrono::milliseconds(flush_interval_milliseconds), - [&] () - { - return requested_flush_up_to > flushed_up_to || is_shutdown || is_force_prepare_tables; - } - ); - - queue_front_index += queue.size(); - to_flush_end = queue_front_index; - // Swap with existing array from previous flush, to save memory - // allocations. - to_flush.resize(0); - queue.swap(to_flush); - - should_prepare_tables_anyway = is_force_prepare_tables; - - exit_this_thread = is_shutdown; - } - - if (to_flush.empty()) - { - if (should_prepare_tables_anyway) - { - prepareTable(); - LOG_TRACE(log, "Table created (force)"); - - std::lock_guard lock(mutex); - is_force_prepare_tables = false; - flush_event.notify_all(); - } - } - else - { - flushImpl(to_flush, to_flush_end); - } - } - catch (...) - { - tryLogCurrentException(__PRETTY_FUNCTION__); - } - } - LOG_TRACE(log, "Terminating"); -} - - -template -void SystemLog::flushImpl(const std::vector & to_flush, uint64_t to_flush_end) -{ - try - { - LOG_TRACE(log, "Flushing system log, {} entries to flush up to offset {}", - to_flush.size(), to_flush_end); - - /// We check for existence of the table and create it as needed at every - /// flush. This is done to allow user to drop the table at any moment - /// (new empty table will be created automatically). BTW, flush method - /// is called from single thread. - prepareTable(); - - ColumnsWithTypeAndName log_element_columns; - auto log_element_names_and_types = LogElement::getNamesAndTypes(); - - for (auto name_and_type : log_element_names_and_types) - log_element_columns.emplace_back(name_and_type.type, name_and_type.name); - - Block block(std::move(log_element_columns)); - - MutableColumns columns = block.mutateColumns(); - for (const auto & elem : to_flush) - elem.appendToBlock(columns); - - block.setColumns(std::move(columns)); - - /// We write to table indirectly, using InterpreterInsertQuery. - /// This is needed to support DEFAULT-columns in table. - - std::unique_ptr insert = std::make_unique(); - insert->table_id = table_id; - ASTPtr query_ptr(insert.release()); - - // we need query context to do inserts to target table with MV containing subqueries or joins - auto insert_context = Context::createCopy(context); - insert_context->makeQueryContext(); - - InterpreterInsertQuery interpreter(query_ptr, insert_context); - BlockIO io = interpreter.execute(); - - PushingPipelineExecutor executor(io.pipeline); - - executor.start(); - executor.push(block); - executor.finish(); - } - catch (...) - { - tryLogCurrentException(__PRETTY_FUNCTION__); - } - - { - std::lock_guard lock(mutex); - flushed_up_to = to_flush_end; - is_force_prepare_tables = false; - flush_event.notify_all(); - } - - LOG_TRACE(log, "Flushed system log up to offset {}", to_flush_end); -} - - -template -void SystemLog::prepareTable() -{ - String description = table_id.getNameForLogs(); - - auto table = DatabaseCatalog::instance().tryGetTable(table_id, getContext()); - - if (table) - { - if (old_create_query.empty()) - { - old_create_query = serializeAST(*getCreateTableQueryClean(table_id, getContext())); - if (old_create_query.empty()) - throw Exception(ErrorCodes::LOGICAL_ERROR, "Empty CREATE QUERY for {}", backQuoteIfNeed(table_id.table_name)); - } - - if (old_create_query != create_query) - { - /// Rename the existing table. - int suffix = 0; - while (DatabaseCatalog::instance().isTableExist( - {table_id.database_name, table_id.table_name + "_" + toString(suffix)}, getContext())) - ++suffix; - - auto rename = std::make_shared(); - - ASTRenameQuery::Table from; - from.database = table_id.database_name; - from.table = table_id.table_name; - - ASTRenameQuery::Table to; - to.database = table_id.database_name; - to.table = table_id.table_name + "_" + toString(suffix); - - ASTRenameQuery::Element elem; - elem.from = from; - elem.to = to; - - rename->elements.emplace_back(elem); - - LOG_DEBUG( - log, - "Existing table {} for system log has obsolete or different structure. Renaming it to {}.\nOld: {}\nNew: {}\n.", - description, - backQuoteIfNeed(to.table), - old_create_query, - create_query); - - auto query_context = Context::createCopy(context); - query_context->makeQueryContext(); - InterpreterRenameQuery(rename, query_context).execute(); - - /// The required table will be created. - table = nullptr; - } - else if (!is_prepared) - LOG_DEBUG(log, "Will use existing table {} for {}", description, LogElement::name()); - } - - if (!table) - { - /// Create the table. - LOG_DEBUG(log, "Creating new table {} for {}", description, LogElement::name()); - - auto query_context = Context::createCopy(context); - query_context->makeQueryContext(); - - auto create_query_ast = getCreateTableQuery(); - InterpreterCreateQuery interpreter(create_query_ast, query_context); - interpreter.setInternal(true); - interpreter.execute(); - - table = DatabaseCatalog::instance().getTable(table_id, getContext()); - - old_create_query.clear(); - } - - is_prepared = true; -} - - -template -ASTPtr SystemLog::getCreateTableQuery() -{ - auto create = std::make_shared(); - - create->setDatabase(table_id.database_name); - create->setTable(table_id.table_name); - - auto ordinary_columns = LogElement::getNamesAndTypes(); - auto alias_columns = LogElement::getNamesAndAliases(); - auto new_columns_list = std::make_shared(); - new_columns_list->set(new_columns_list->columns, InterpreterCreateQuery::formatColumns(ordinary_columns, alias_columns)); - create->set(create->columns_list, new_columns_list); - - ParserStorage storage_parser; - ASTPtr storage_ast = parseQuery( - storage_parser, storage_def.data(), storage_def.data() + storage_def.size(), - "Storage to create table for " + LogElement::name(), 0, DBMS_DEFAULT_MAX_PARSER_DEPTH); - create->set(create->storage, storage_ast); - - return create; -} - } diff --git a/src/Interpreters/TextLog.cpp b/src/Interpreters/TextLog.cpp index 51ffbdd66ee..9aa5b26508e 100644 --- a/src/Interpreters/TextLog.cpp +++ b/src/Interpreters/TextLog.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include diff --git a/src/Interpreters/TextLog.h b/src/Interpreters/TextLog.h index d2ddd23d1e9..3026452fcc3 100644 --- a/src/Interpreters/TextLog.h +++ b/src/Interpreters/TextLog.h @@ -1,5 +1,9 @@ #pragma once + #include +#include +#include +#include namespace DB { diff --git a/src/Interpreters/ThreadStatusExt.cpp b/src/Interpreters/ThreadStatusExt.cpp index b3720b89eaa..bb610c34aa2 100644 --- a/src/Interpreters/ThreadStatusExt.cpp +++ b/src/Interpreters/ThreadStatusExt.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #if defined(OS_LINUX) diff --git a/src/Interpreters/TraceLog.h b/src/Interpreters/TraceLog.h index 85400560a7b..b5668f365dd 100644 --- a/src/Interpreters/TraceLog.h +++ b/src/Interpreters/TraceLog.h @@ -5,6 +5,8 @@ #include #include #include +#include +#include namespace DB diff --git a/src/Interpreters/ZooKeeperLog.h b/src/Interpreters/ZooKeeperLog.h index d721081fdae..284675a7ff5 100644 --- a/src/Interpreters/ZooKeeperLog.h +++ b/src/Interpreters/ZooKeeperLog.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index 9770d1a988f..7dc81fb5ab6 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -38,6 +39,7 @@ #include #include #include +#include #include #include #include diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.cpp b/src/Storages/MergeTree/IMergeTreeDataPart.cpp index da412e4941e..8943641cc56 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.cpp +++ b/src/Storages/MergeTree/IMergeTreeDataPart.cpp @@ -18,7 +18,9 @@ #include #include #include +#include #include +#include #include #include diff --git a/src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp b/src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp index 1432728d00a..4d24f491551 100644 --- a/src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp +++ b/src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp @@ -1,13 +1,14 @@ #include #include -#include -#include #include #include #include #include +#include +#include #include #include +#include namespace DB diff --git a/src/Storages/ProjectionsDescription.cpp b/src/Storages/ProjectionsDescription.cpp index 791583e2495..e13895e60f1 100644 --- a/src/Storages/ProjectionsDescription.cpp +++ b/src/Storages/ProjectionsDescription.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include diff --git a/src/Storages/StorageDistributed.cpp b/src/Storages/StorageDistributed.cpp index 19869b77106..9d98d36f8e7 100644 --- a/src/Storages/StorageDistributed.cpp +++ b/src/Storages/StorageDistributed.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -44,6 +45,7 @@ #include #include #include +#include #include #include #include @@ -54,6 +56,7 @@ #include #include +#include #include #include #include diff --git a/src/Storages/StorageMemory.cpp b/src/Storages/StorageMemory.cpp index 37cb238ba0f..72851472b79 100644 --- a/src/Storages/StorageMemory.cpp +++ b/src/Storages/StorageMemory.cpp @@ -11,6 +11,7 @@ #include #include #include +#include namespace DB diff --git a/src/Storages/StorageMergeTree.cpp b/src/Storages/StorageMergeTree.cpp index 11815d9ceef..90440ed084f 100644 --- a/src/Storages/StorageMergeTree.cpp +++ b/src/Storages/StorageMergeTree.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index 91a9c8567ba..5af2ae8bffe 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -36,12 +36,14 @@ #include #include +#include #include #include #include #include #include #include +#include #include #include diff --git a/src/Storages/WindowView/StorageWindowView.cpp b/src/Storages/WindowView/StorageWindowView.cpp index a81a5a9649a..96943a886c1 100644 --- a/src/Storages/WindowView/StorageWindowView.cpp +++ b/src/Storages/WindowView/StorageWindowView.cpp @@ -9,6 +9,8 @@ #include #include #include +#include +#include #include #include #include @@ -21,7 +23,9 @@ #include #include #include +#include #include +#include #include #include #include From c1dea66907154b7532fd1d92491228f1594189c7 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 10 Jan 2022 22:35:42 +0300 Subject: [PATCH 09/31] Move TraceCollector into Interpreters Since now it relies on SystemLog that is in Interpreters, and it cannot be moved into Common, since it has lots of dependencies. --- src/Common/MemoryTracker.cpp | 4 ++-- src/Common/QueryProfiler.cpp | 2 +- src/Interpreters/Context.cpp | 2 +- src/Interpreters/ThreadStatusExt.cpp | 2 +- src/{Common => Interpreters}/TraceCollector.cpp | 0 src/{Common => Interpreters}/TraceCollector.h | 0 src/Interpreters/TraceLog.h | 2 +- 7 files changed, 6 insertions(+), 6 deletions(-) rename src/{Common => Interpreters}/TraceCollector.cpp (100%) rename src/{Common => Interpreters}/TraceCollector.h (100%) diff --git a/src/Common/MemoryTracker.cpp b/src/Common/MemoryTracker.cpp index 013005442be..bb29bdca1f4 100644 --- a/src/Common/MemoryTracker.cpp +++ b/src/Common/MemoryTracker.cpp @@ -1,12 +1,12 @@ #include "MemoryTracker.h" #include -#include "Common/TraceCollector.h" +#include #include #include -#include #include #include +#include #include #include diff --git a/src/Common/QueryProfiler.cpp b/src/Common/QueryProfiler.cpp index 0b2cd602b38..9718d15c072 100644 --- a/src/Common/QueryProfiler.cpp +++ b/src/Common/QueryProfiler.cpp @@ -1,9 +1,9 @@ #include "QueryProfiler.h" #include +#include #include #include -#include #include #include #include diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 14b0f65072a..1c71ab2cd6f 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -63,6 +63,7 @@ #include #include #include +#include #include #include #include @@ -74,7 +75,6 @@ #include #include #include -#include #include #include #include diff --git a/src/Interpreters/ThreadStatusExt.cpp b/src/Interpreters/ThreadStatusExt.cpp index bb610c34aa2..3770e4b7c9f 100644 --- a/src/Interpreters/ThreadStatusExt.cpp +++ b/src/Interpreters/ThreadStatusExt.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -14,7 +15,6 @@ #include #include #include -#include #include #include diff --git a/src/Common/TraceCollector.cpp b/src/Interpreters/TraceCollector.cpp similarity index 100% rename from src/Common/TraceCollector.cpp rename to src/Interpreters/TraceCollector.cpp diff --git a/src/Common/TraceCollector.h b/src/Interpreters/TraceCollector.h similarity index 100% rename from src/Common/TraceCollector.h rename to src/Interpreters/TraceCollector.h diff --git a/src/Interpreters/TraceLog.h b/src/Interpreters/TraceLog.h index b5668f365dd..e8836955d96 100644 --- a/src/Interpreters/TraceLog.h +++ b/src/Interpreters/TraceLog.h @@ -3,8 +3,8 @@ #include #include #include +#include #include -#include #include #include From cb70544dfea0a2dadfd4993f85113df2e84dab55 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 10 Jan 2022 22:39:10 +0300 Subject: [PATCH 10/31] Move LockMemoryExceptionInThread and MemoryTrackerBlockerInThread --- base/base/scope_guard_safe.h | 8 +-- base/loggers/OwnSplitChannel.cpp | 3 +- src/Common/Exception.cpp | 5 +- src/Common/LockMemoryExceptionInThread.cpp | 20 ++++++ src/Common/LockMemoryExceptionInThread.h | 39 ++++++++++++ src/Common/MemoryTracker.cpp | 61 +++++-------------- src/Common/MemoryTracker.h | 61 +------------------ src/Common/MemoryTrackerBlockerInThread.cpp | 16 +++++ src/Common/MemoryTrackerBlockerInThread.h | 25 ++++++++ src/IO/WriteBuffer.h | 4 +- src/Interpreters/SystemLog.cpp | 3 +- src/Interpreters/ThreadStatusExt.cpp | 3 +- src/Interpreters/executeQuery.cpp | 3 +- src/Storages/MergeTree/IMergeTreeDataPart.cpp | 3 +- src/Storages/MergeTree/MergeList.cpp | 3 +- .../MergeTreeDataPartWriterOnDisk.cpp | 3 +- .../MergeTree/MergeTreeMarksLoader.cpp | 3 +- src/Storages/StorageBuffer.cpp | 8 +-- 18 files changed, 145 insertions(+), 126 deletions(-) create mode 100644 src/Common/LockMemoryExceptionInThread.cpp create mode 100644 src/Common/LockMemoryExceptionInThread.h create mode 100644 src/Common/MemoryTrackerBlockerInThread.cpp create mode 100644 src/Common/MemoryTrackerBlockerInThread.h diff --git a/base/base/scope_guard_safe.h b/base/base/scope_guard_safe.h index 73c85b65b57..98521dd17c1 100644 --- a/base/base/scope_guard_safe.h +++ b/base/base/scope_guard_safe.h @@ -2,7 +2,7 @@ #include #include -#include +#include /// Same as SCOPE_EXIT() but block the MEMORY_LIMIT_EXCEEDED errors. /// @@ -12,8 +12,7 @@ /// /// NOTE: it should be used with caution. #define SCOPE_EXIT_MEMORY(...) SCOPE_EXIT( \ - MemoryTracker::LockExceptionInThread \ - lock_memory_tracker(VariableContext::Global); \ + LockMemoryExceptionInThread lock_memory_tracker(VariableContext::Global); \ __VA_ARGS__; \ ) @@ -57,8 +56,7 @@ #define SCOPE_EXIT_MEMORY_SAFE(...) SCOPE_EXIT( \ try \ { \ - MemoryTracker::LockExceptionInThread \ - lock_memory_tracker(VariableContext::Global); \ + LockMemoryExceptionInThread lock_memory_tracker(VariableContext::Global); \ __VA_ARGS__; \ } \ catch (...) \ diff --git a/base/loggers/OwnSplitChannel.cpp b/base/loggers/OwnSplitChannel.cpp index 854dcacdf6f..2267b8f425d 100644 --- a/base/loggers/OwnSplitChannel.cpp +++ b/base/loggers/OwnSplitChannel.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -58,7 +59,7 @@ void OwnSplitChannel::tryLogSplit(const Poco::Message & msg) /// but let's log it into the stderr at least. catch (...) { - MemoryTracker::LockExceptionInThread lock_memory_tracker(VariableContext::Global); + LockMemoryExceptionInThread lock_memory_tracker(VariableContext::Global); const std::string & exception_message = getCurrentExceptionMessage(true); const std::string & message = msg.getText(); diff --git a/src/Common/Exception.cpp b/src/Common/Exception.cpp index cdf5816237c..b2987cbd73f 100644 --- a/src/Common/Exception.cpp +++ b/src/Common/Exception.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -175,7 +176,7 @@ void tryLogCurrentException(const char * log_name, const std::string & start_of_ /// /// And in this case the exception will not be logged, so let's block the /// MemoryTracker until the exception will be logged. - MemoryTracker::LockExceptionInThread lock_memory_tracker(VariableContext::Global); + LockMemoryExceptionInThread lock_memory_tracker(VariableContext::Global); /// Poco::Logger::get can allocate memory too tryLogCurrentExceptionImpl(&Poco::Logger::get(log_name), start_of_message); @@ -188,7 +189,7 @@ void tryLogCurrentException(Poco::Logger * logger, const std::string & start_of_ /// /// And in this case the exception will not be logged, so let's block the /// MemoryTracker until the exception will be logged. - MemoryTracker::LockExceptionInThread lock_memory_tracker(VariableContext::Global); + LockMemoryExceptionInThread lock_memory_tracker(VariableContext::Global); tryLogCurrentExceptionImpl(logger, start_of_message); } diff --git a/src/Common/LockMemoryExceptionInThread.cpp b/src/Common/LockMemoryExceptionInThread.cpp new file mode 100644 index 00000000000..606f02abcb0 --- /dev/null +++ b/src/Common/LockMemoryExceptionInThread.cpp @@ -0,0 +1,20 @@ +#include + +/// LockMemoryExceptionInThread +thread_local uint64_t LockMemoryExceptionInThread::counter = 0; +thread_local VariableContext LockMemoryExceptionInThread::level = VariableContext::Global; +thread_local bool LockMemoryExceptionInThread::block_fault_injections = false; +LockMemoryExceptionInThread::LockMemoryExceptionInThread(VariableContext level_, bool block_fault_injections_) + : previous_level(level) + , previous_block_fault_injections(block_fault_injections) +{ + ++counter; + level = level_; + block_fault_injections = block_fault_injections_; +} +LockMemoryExceptionInThread::~LockMemoryExceptionInThread() +{ + --counter; + level = previous_level; + block_fault_injections = previous_block_fault_injections; +} diff --git a/src/Common/LockMemoryExceptionInThread.h b/src/Common/LockMemoryExceptionInThread.h new file mode 100644 index 00000000000..dc2bccf257b --- /dev/null +++ b/src/Common/LockMemoryExceptionInThread.h @@ -0,0 +1,39 @@ +#pragma once + +#include + +/// To be able to avoid MEMORY_LIMIT_EXCEEDED Exception in destructors: +/// - either configured memory limit reached +/// - or fault injected +/// +/// So this will simply ignore the configured memory limit (and avoid fault injection). +/// +/// NOTE: exception will be silently ignored, no message in log +/// (since logging from MemoryTracker::alloc() is tricky) +/// +/// NOTE: MEMORY_LIMIT_EXCEEDED Exception implicitly blocked if +/// stack unwinding is currently in progress in this thread (to avoid +/// std::terminate()), so you don't need to use it in this case explicitly. +struct LockMemoryExceptionInThread +{ +private: + static thread_local uint64_t counter; + static thread_local VariableContext level; + static thread_local bool block_fault_injections; + + VariableContext previous_level; + bool previous_block_fault_injections; +public: + /// level_ - block in level and above + /// block_fault_injections_ - block in fault injection too + explicit LockMemoryExceptionInThread(VariableContext level_ = VariableContext::User, bool block_fault_injections_ = true); + ~LockMemoryExceptionInThread(); + + LockMemoryExceptionInThread(const LockMemoryExceptionInThread &) = delete; + LockMemoryExceptionInThread & operator=(const LockMemoryExceptionInThread &) = delete; + + static bool isBlocked(VariableContext current_level, bool fault_injection) + { + return counter > 0 && current_level >= level && (!fault_injection || block_fault_injections); + } +}; diff --git a/src/Common/MemoryTracker.cpp b/src/Common/MemoryTracker.cpp index bb29bdca1f4..ba98ede221a 100644 --- a/src/Common/MemoryTracker.cpp +++ b/src/Common/MemoryTracker.cpp @@ -3,6 +3,8 @@ #include #include #include +#include +#include #include #include #include @@ -34,7 +36,7 @@ namespace /// noexcept(false)) will cause std::terminate() bool inline memoryTrackerCanThrow(VariableContext level, bool fault_injection) { - return !MemoryTracker::LockExceptionInThread::isBlocked(level, fault_injection) && !std::uncaught_exceptions(); + return !LockMemoryExceptionInThread::isBlocked(level, fault_injection) && !std::uncaught_exceptions(); } } @@ -55,41 +57,6 @@ namespace ProfileEvents static constexpr size_t log_peak_memory_usage_every = 1ULL << 30; -// BlockerInThread -thread_local uint64_t MemoryTracker::BlockerInThread::counter = 0; -thread_local VariableContext MemoryTracker::BlockerInThread::level = VariableContext::Global; -MemoryTracker::BlockerInThread::BlockerInThread(VariableContext level_) - : previous_level(level) -{ - ++counter; - level = level_; -} -MemoryTracker::BlockerInThread::~BlockerInThread() -{ - --counter; - level = previous_level; -} - -/// LockExceptionInThread -thread_local uint64_t MemoryTracker::LockExceptionInThread::counter = 0; -thread_local VariableContext MemoryTracker::LockExceptionInThread::level = VariableContext::Global; -thread_local bool MemoryTracker::LockExceptionInThread::block_fault_injections = false; -MemoryTracker::LockExceptionInThread::LockExceptionInThread(VariableContext level_, bool block_fault_injections_) - : previous_level(level) - , previous_block_fault_injections(block_fault_injections) -{ - ++counter; - level = level_; - block_fault_injections = block_fault_injections_; -} -MemoryTracker::LockExceptionInThread::~LockExceptionInThread() -{ - --counter; - level = previous_level; - block_fault_injections = previous_block_fault_injections; -} - - MemoryTracker total_memory_tracker(nullptr, VariableContext::Global); @@ -133,9 +100,9 @@ void MemoryTracker::allocImpl(Int64 size, bool throw_if_memory_exceeded) if (size < 0) throw DB::Exception(DB::ErrorCodes::LOGICAL_ERROR, "Negative size ({}) is passed to MemoryTracker. It is a bug.", size); - if (BlockerInThread::isBlocked(level)) + if (MemoryTrackerBlockerInThread::isBlocked(level)) { - /// Since the BlockerInThread should respect the level, we should go to the next parent. + /// Since the MemoryTrackerBlockerInThread should respect the level, we should go to the next parent. if (auto * loaded_next = parent.load(std::memory_order_relaxed)) loaded_next->allocImpl(size, throw_if_memory_exceeded); return; @@ -184,7 +151,7 @@ void MemoryTracker::allocImpl(Int64 size, bool throw_if_memory_exceeded) if (unlikely(fault_probability && fault(thread_local_rng)) && memoryTrackerCanThrow(level, true) && throw_if_memory_exceeded) { /// Prevent recursion. Exception::ctor -> std::string -> new[] -> MemoryTracker::alloc - BlockerInThread untrack_lock(VariableContext::Global); + MemoryTrackerBlockerInThread untrack_lock(VariableContext::Global); ProfileEvents::increment(ProfileEvents::QueryMemoryLimitExceeded); const auto * description = description_ptr.load(std::memory_order_relaxed); @@ -203,7 +170,7 @@ void MemoryTracker::allocImpl(Int64 size, bool throw_if_memory_exceeded) bool allocation_traced = false; if (unlikely(current_profiler_limit && will_be > current_profiler_limit)) { - BlockerInThread untrack_lock(VariableContext::Global); + MemoryTrackerBlockerInThread untrack_lock(VariableContext::Global); DB::TraceCollector::collect(DB::TraceType::Memory, StackTrace(), size); setOrRaiseProfilerLimit((will_be + profiler_step - 1) / profiler_step * profiler_step); allocation_traced = true; @@ -212,7 +179,7 @@ void MemoryTracker::allocImpl(Int64 size, bool throw_if_memory_exceeded) std::bernoulli_distribution sample(sample_probability); if (unlikely(sample_probability && sample(thread_local_rng))) { - BlockerInThread untrack_lock(VariableContext::Global); + MemoryTrackerBlockerInThread untrack_lock(VariableContext::Global); DB::TraceCollector::collect(DB::TraceType::MemorySample, StackTrace(), size); allocation_traced = true; } @@ -220,7 +187,7 @@ void MemoryTracker::allocImpl(Int64 size, bool throw_if_memory_exceeded) if (unlikely(current_hard_limit && will_be > current_hard_limit) && memoryTrackerCanThrow(level, false) && throw_if_memory_exceeded) { /// Prevent recursion. Exception::ctor -> std::string -> new[] -> MemoryTracker::alloc - BlockerInThread untrack_lock(VariableContext::Global); + MemoryTrackerBlockerInThread untrack_lock(VariableContext::Global); ProfileEvents::increment(ProfileEvents::QueryMemoryLimitExceeded); const auto * description = description_ptr.load(std::memory_order_relaxed); throw DB::Exception( @@ -237,7 +204,7 @@ void MemoryTracker::allocImpl(Int64 size, bool throw_if_memory_exceeded) if (throw_if_memory_exceeded) { /// Prevent recursion. Exception::ctor -> std::string -> new[] -> MemoryTracker::alloc - BlockerInThread untrack_lock(VariableContext::Global); + MemoryTrackerBlockerInThread untrack_lock(VariableContext::Global); bool log_memory_usage = true; peak_updated = updatePeak(will_be, log_memory_usage); } @@ -249,7 +216,7 @@ void MemoryTracker::allocImpl(Int64 size, bool throw_if_memory_exceeded) if (peak_updated && allocation_traced) { - BlockerInThread untrack_lock(VariableContext::Global); + MemoryTrackerBlockerInThread untrack_lock(VariableContext::Global); DB::TraceCollector::collect(DB::TraceType::MemoryPeak, StackTrace(), will_be); } @@ -288,9 +255,9 @@ bool MemoryTracker::updatePeak(Int64 will_be, bool log_memory_usage) void MemoryTracker::free(Int64 size) { - if (BlockerInThread::isBlocked(level)) + if (MemoryTrackerBlockerInThread::isBlocked(level)) { - /// Since the BlockerInThread should respect the level, we should go to the next parent. + /// Since the MemoryTrackerBlockerInThread should respect the level, we should go to the next parent. if (auto * loaded_next = parent.load(std::memory_order_relaxed)) loaded_next->free(size); return; @@ -299,7 +266,7 @@ void MemoryTracker::free(Int64 size) std::bernoulli_distribution sample(sample_probability); if (unlikely(sample_probability && sample(thread_local_rng))) { - BlockerInThread untrack_lock(VariableContext::Global); + MemoryTrackerBlockerInThread untrack_lock(VariableContext::Global); DB::TraceCollector::collect(DB::TraceType::MemorySample, StackTrace(), -size); } diff --git a/src/Common/MemoryTracker.h b/src/Common/MemoryTracker.h index ce0eef52e17..a0138b25b5f 100644 --- a/src/Common/MemoryTracker.h +++ b/src/Common/MemoryTracker.h @@ -31,6 +31,9 @@ extern thread_local bool memory_tracker_always_throw_logical_error_on_allocation /** Tracks memory consumption. * It throws an exception if amount of consumed memory become greater than certain limit. * The same memory tracker could be simultaneously used in different threads. + * + * @see LockMemoryExceptionInThread + * @see MemoryTrackerBlockerInThread */ class MemoryTracker { @@ -167,64 +170,6 @@ public: /// Prints info about peak memory consumption into log. void logPeakMemoryUsage() const; - - /// To be able to temporarily stop memory tracking from current thread. - struct BlockerInThread - { - private: - static thread_local uint64_t counter; - static thread_local VariableContext level; - - VariableContext previous_level; - public: - /// level_ - block in level and above - explicit BlockerInThread(VariableContext level_ = VariableContext::User); - ~BlockerInThread(); - - BlockerInThread(const BlockerInThread &) = delete; - BlockerInThread & operator=(const BlockerInThread &) = delete; - - static bool isBlocked(VariableContext current_level) - { - return counter > 0 && current_level >= level; - } - }; - - /// To be able to avoid MEMORY_LIMIT_EXCEEDED Exception in destructors: - /// - either configured memory limit reached - /// - or fault injected - /// - /// So this will simply ignore the configured memory limit (and avoid fault injection). - /// - /// NOTE: exception will be silently ignored, no message in log - /// (since logging from MemoryTracker::alloc() is tricky) - /// - /// NOTE: MEMORY_LIMIT_EXCEEDED Exception implicitly blocked if - /// stack unwinding is currently in progress in this thread (to avoid - /// std::terminate()), so you don't need to use it in this case explicitly. - struct LockExceptionInThread - { - private: - static thread_local uint64_t counter; - static thread_local VariableContext level; - static thread_local bool block_fault_injections; - - VariableContext previous_level; - bool previous_block_fault_injections; - public: - /// level_ - block in level and above - /// block_fault_injections_ - block in fault injection too - explicit LockExceptionInThread(VariableContext level_ = VariableContext::User, bool block_fault_injections_ = true); - ~LockExceptionInThread(); - - LockExceptionInThread(const LockExceptionInThread &) = delete; - LockExceptionInThread & operator=(const LockExceptionInThread &) = delete; - - static bool isBlocked(VariableContext current_level, bool fault_injection) - { - return counter > 0 && current_level >= level && (!fault_injection || block_fault_injections); - } - }; }; extern MemoryTracker total_memory_tracker; diff --git a/src/Common/MemoryTrackerBlockerInThread.cpp b/src/Common/MemoryTrackerBlockerInThread.cpp new file mode 100644 index 00000000000..8eb119b2fe5 --- /dev/null +++ b/src/Common/MemoryTrackerBlockerInThread.cpp @@ -0,0 +1,16 @@ +#include + +// MemoryTrackerBlockerInThread +thread_local uint64_t MemoryTrackerBlockerInThread::counter = 0; +thread_local VariableContext MemoryTrackerBlockerInThread::level = VariableContext::Global; +MemoryTrackerBlockerInThread::MemoryTrackerBlockerInThread(VariableContext level_) + : previous_level(level) +{ + ++counter; + level = level_; +} +MemoryTrackerBlockerInThread::~MemoryTrackerBlockerInThread() +{ + --counter; + level = previous_level; +} diff --git a/src/Common/MemoryTrackerBlockerInThread.h b/src/Common/MemoryTrackerBlockerInThread.h new file mode 100644 index 00000000000..caad28f636e --- /dev/null +++ b/src/Common/MemoryTrackerBlockerInThread.h @@ -0,0 +1,25 @@ +#pragma once + +#include + +/// To be able to temporarily stop memory tracking from current thread. +struct MemoryTrackerBlockerInThread +{ +private: + static thread_local uint64_t counter; + static thread_local VariableContext level; + + VariableContext previous_level; +public: + /// level_ - block in level and above + explicit MemoryTrackerBlockerInThread(VariableContext level_ = VariableContext::User); + ~MemoryTrackerBlockerInThread(); + + MemoryTrackerBlockerInThread(const MemoryTrackerBlockerInThread &) = delete; + MemoryTrackerBlockerInThread & operator=(const MemoryTrackerBlockerInThread &) = delete; + + static bool isBlocked(VariableContext current_level) + { + return counter > 0 && current_level >= level; + } +}; diff --git a/src/IO/WriteBuffer.h b/src/IO/WriteBuffer.h index 4d7f300a504..9440ac0a855 100644 --- a/src/IO/WriteBuffer.h +++ b/src/IO/WriteBuffer.h @@ -8,7 +8,7 @@ #include #include -#include +#include #include @@ -116,7 +116,7 @@ public: return; /// finalize() is often called from destructors. - MemoryTracker::LockExceptionInThread lock(VariableContext::Global); + LockMemoryExceptionInThread lock(VariableContext::Global); try { finalizeImpl(); diff --git a/src/Interpreters/SystemLog.cpp b/src/Interpreters/SystemLog.cpp index 43f02abc7eb..66e28678ce6 100644 --- a/src/Interpreters/SystemLog.cpp +++ b/src/Interpreters/SystemLog.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -286,7 +287,7 @@ void SystemLog::add(const LogElement & element) /// The size of allocation can be in order of a few megabytes. /// But this should not be accounted for query memory usage. /// Otherwise the tests like 01017_uniqCombined_memory_usage.sql will be flacky. - MemoryTracker::BlockerInThread temporarily_disable_memory_tracker(VariableContext::Global); + MemoryTrackerBlockerInThread temporarily_disable_memory_tracker(VariableContext::Global); /// Should not log messages under mutex. bool queue_is_half_full = false; diff --git a/src/Interpreters/ThreadStatusExt.cpp b/src/Interpreters/ThreadStatusExt.cpp index 3770e4b7c9f..2ea371d3d03 100644 --- a/src/Interpreters/ThreadStatusExt.cpp +++ b/src/Interpreters/ThreadStatusExt.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #if defined(OS_LINUX) @@ -342,7 +343,7 @@ void ThreadStatus::finalizeQueryProfiler() void ThreadStatus::detachQuery(bool exit_if_already_detached, bool thread_exits) { - MemoryTracker::LockExceptionInThread lock(VariableContext::Global); + LockMemoryExceptionInThread lock(VariableContext::Global); if (exit_if_already_detached && thread_state == ThreadState::DetachedFromQuery) { diff --git a/src/Interpreters/executeQuery.cpp b/src/Interpreters/executeQuery.cpp index 7dc81fb5ab6..870e01d3b5c 100644 --- a/src/Interpreters/executeQuery.cpp +++ b/src/Interpreters/executeQuery.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -195,7 +196,7 @@ static void setExceptionStackTrace(QueryLogElement & elem) { /// Disable memory tracker for stack trace. /// Because if exception is "Memory limit (for query) exceed", then we probably can't allocate another one string. - MemoryTracker::BlockerInThread temporarily_disable_memory_tracker(VariableContext::Global); + MemoryTrackerBlockerInThread temporarily_disable_memory_tracker(VariableContext::Global); try { diff --git a/src/Storages/MergeTree/IMergeTreeDataPart.cpp b/src/Storages/MergeTree/IMergeTreeDataPart.cpp index 8943641cc56..826589361e9 100644 --- a/src/Storages/MergeTree/IMergeTreeDataPart.cpp +++ b/src/Storages/MergeTree/IMergeTreeDataPart.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -617,7 +618,7 @@ void IMergeTreeDataPart::loadColumnsChecksumsIndexes(bool require_columns_checks /// Memory should not be limited during ATTACH TABLE query. /// This is already true at the server startup but must be also ensured for manual table ATTACH. /// Motivation: memory for index is shared between queries - not belong to the query itself. - MemoryTracker::BlockerInThread temporarily_disable_memory_tracker(VariableContext::Global); + MemoryTrackerBlockerInThread temporarily_disable_memory_tracker(VariableContext::Global); loadUUID(); loadColumns(require_columns_checksums); diff --git a/src/Storages/MergeTree/MergeList.cpp b/src/Storages/MergeTree/MergeList.cpp index 366834b8f09..a03cb053e1f 100644 --- a/src/Storages/MergeTree/MergeList.cpp +++ b/src/Storages/MergeTree/MergeList.cpp @@ -2,8 +2,9 @@ #include #include #include -#include #include +#include +#include namespace DB diff --git a/src/Storages/MergeTree/MergeTreeDataPartWriterOnDisk.cpp b/src/Storages/MergeTree/MergeTreeDataPartWriterOnDisk.cpp index 03ae6688beb..2e299bb2447 100644 --- a/src/Storages/MergeTree/MergeTreeDataPartWriterOnDisk.cpp +++ b/src/Storages/MergeTree/MergeTreeDataPartWriterOnDisk.cpp @@ -1,4 +1,5 @@ #include +#include #include @@ -184,7 +185,7 @@ void MergeTreeDataPartWriterOnDisk::calculateAndSerializePrimaryIndex(const Bloc * And otherwise it will look like excessively growing memory consumption in context of query. * (observed in long INSERT SELECTs) */ - MemoryTracker::BlockerInThread temporarily_disable_memory_tracker; + MemoryTrackerBlockerInThread temporarily_disable_memory_tracker; /// Write index. The index contains Primary Key value for each `index_granularity` row. for (const auto & granule : granules_to_write) diff --git a/src/Storages/MergeTree/MergeTreeMarksLoader.cpp b/src/Storages/MergeTree/MergeTreeMarksLoader.cpp index 446ae9b97a1..e7ead4dc8bb 100644 --- a/src/Storages/MergeTree/MergeTreeMarksLoader.cpp +++ b/src/Storages/MergeTree/MergeTreeMarksLoader.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -47,7 +48,7 @@ const MarkInCompressedFile & MergeTreeMarksLoader::getMark(size_t row_index, siz MarkCache::MappedPtr MergeTreeMarksLoader::loadMarksImpl() { /// Memory for marks must not be accounted as memory usage for query, because they are stored in shared cache. - MemoryTracker::BlockerInThread temporarily_disable_memory_tracker; + MemoryTrackerBlockerInThread temporarily_disable_memory_tracker; size_t file_size = disk->getFileSize(mrk_path); size_t mark_size = index_granularity_info.getMarkSizeInBytes(columns_in_mark); diff --git a/src/Storages/StorageBuffer.cpp b/src/Storages/StorageBuffer.cpp index 0cc401aa93c..29772a14752 100644 --- a/src/Storages/StorageBuffer.cpp +++ b/src/Storages/StorageBuffer.cpp @@ -13,7 +13,7 @@ #include #include #include -#include +#include #include #include #include @@ -467,7 +467,7 @@ static void appendBlock(const Block & from, Block & to) MutableColumnPtr last_col; try { - MemoryTracker::BlockerInThread temporarily_disable_memory_tracker; + MemoryTrackerBlockerInThread temporarily_disable_memory_tracker; if (to.rows() == 0) { @@ -496,7 +496,7 @@ static void appendBlock(const Block & from, Block & to) /// In case of rollback, it is better to ignore memory limits instead of abnormal server termination. /// So ignore any memory limits, even global (since memory tracking has drift). - MemoryTracker::BlockerInThread temporarily_ignore_any_memory_limits(VariableContext::Global); + MemoryTrackerBlockerInThread temporarily_ignore_any_memory_limits(VariableContext::Global); try { @@ -924,7 +924,7 @@ void StorageBuffer::writeBlockToDestination(const Block & block, StoragePtr tabl } auto destination_metadata_snapshot = table->getInMemoryMetadataPtr(); - MemoryTracker::BlockerInThread temporarily_disable_memory_tracker; + MemoryTrackerBlockerInThread temporarily_disable_memory_tracker; auto insert = std::make_shared(); insert->table_id = destination_id; From 655cf6c3155c8828cad6d9185d5e5873652e87ac Mon Sep 17 00:00:00 2001 From: feng lv Date: Tue, 11 Jan 2022 12:27:21 +0000 Subject: [PATCH 11/31] fix --- src/Functions/FunctionBinaryArithmetic.h | 36 ++---------------------- 1 file changed, 3 insertions(+), 33 deletions(-) diff --git a/src/Functions/FunctionBinaryArithmetic.h b/src/Functions/FunctionBinaryArithmetic.h index c73fe156808..a1c3320f88a 100644 --- a/src/Functions/FunctionBinaryArithmetic.h +++ b/src/Functions/FunctionBinaryArithmetic.h @@ -133,40 +133,10 @@ public: Case && IsIntegralOrExtended, LeftDataType>, Case && IsIntegralOrExtended, RightDataType>, - /// e.g Decimal + Float64 = Float64 - Case::plus && IsDataTypeDecimal && IsFloatingPoint, + /// e.g Decimal +-*/ Float, least(Decimal, Float), greatest(Decimal, Float) = Float64 + Case::allow_decimal && IsDataTypeDecimal && IsFloatingPoint, DataTypeFloat64>, - Case::plus && IsDataTypeDecimal && IsFloatingPoint, - DataTypeFloat64>, - - /// e.g Decimal - Float64 = Float64 - Case::minus && IsDataTypeDecimal && IsFloatingPoint, - DataTypeFloat64>, - Case::minus && IsDataTypeDecimal && IsFloatingPoint, - DataTypeFloat64>, - - /// e.g Decimal * Float64 = Float64 - Case::multiply && IsDataTypeDecimal && IsFloatingPoint, - DataTypeFloat64>, - Case::multiply && IsDataTypeDecimal && IsFloatingPoint, - DataTypeFloat64>, - - /// e.g Decimal / Float64 = Float64 - Case::division && IsDataTypeDecimal && IsFloatingPoint, - DataTypeFloat64>, - Case::division && IsDataTypeDecimal && IsFloatingPoint, - DataTypeFloat64>, - - /// e.g least(Decimal, Float64) = Float64 - Case::least && IsDataTypeDecimal && IsFloatingPoint, - DataTypeFloat64>, - Case::least && IsDataTypeDecimal && IsFloatingPoint, - DataTypeFloat64>, - - /// e.g greatest(Decimal, Float64) = Float64 - Case::greatest && IsDataTypeDecimal && IsFloatingPoint, - DataTypeFloat64>, - Case::greatest && IsDataTypeDecimal && IsFloatingPoint, + Case::allow_decimal && IsDataTypeDecimal && IsFloatingPoint, DataTypeFloat64>, /// Decimal Real is not supported (traditional DBs convert Decimal Real to Real) From 47d8b076811ea6b6cb5c5721fd217066e310d8b2 Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Tue, 11 Jan 2022 18:53:45 +0300 Subject: [PATCH 12/31] Dictionary rename fix --- src/Dictionaries/IDictionary.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Dictionaries/IDictionary.h b/src/Dictionaries/IDictionary.h index b1923306003..991b45e8e8f 100644 --- a/src/Dictionaries/IDictionary.h +++ b/src/Dictionaries/IDictionary.h @@ -56,7 +56,7 @@ public: { } - const std::string & getFullName() const{ return full_name; } + const std::string & getFullName() const { return full_name; } StorageID getDictionaryID() const { @@ -69,6 +69,7 @@ public: std::lock_guard lock{name_mutex}; assert(new_name.uuid == dictionary_id.uuid && dictionary_id.uuid != UUIDHelpers::Nil); dictionary_id = new_name; + full_name = dictionary_id.getInternalDictionaryName(); } const std::string & getLoadableName() const override final { return getFullName(); } @@ -234,7 +235,7 @@ private: mutable StorageID dictionary_id; protected: - const String full_name; + mutable String full_name; String dictionary_comment; }; From 248b879fd5766ef3579fe9eac12d16e879490fc0 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 11 Jan 2022 22:30:55 +0300 Subject: [PATCH 13/31] Split TraceCollector between Common and Interpreters This is required to leave TraceCollector::collect() inside main for MemoryTrackier. --- src/Common/TraceSender.cpp | 78 ++++++++++++++++++++++++++ src/Common/TraceSender.h | 36 ++++++++++++ src/Interpreters/TraceCollector.cpp | 85 ++--------------------------- src/Interpreters/TraceCollector.h | 19 ++----- 4 files changed, 125 insertions(+), 93 deletions(-) create mode 100644 src/Common/TraceSender.cpp create mode 100644 src/Common/TraceSender.h diff --git a/src/Common/TraceSender.cpp b/src/Common/TraceSender.cpp new file mode 100644 index 00000000000..57ab3df8f96 --- /dev/null +++ b/src/Common/TraceSender.cpp @@ -0,0 +1,78 @@ +#include + +#include +#include +#include +#include + +namespace +{ + /// Normally query_id is a UUID (string with a fixed length) but user can provide custom query_id. + /// Thus upper bound on query_id length should be introduced to avoid buffer overflow in signal handler. + /// + /// And it cannot be large, since otherwise it will not fit into PIPE_BUF. + /// The performance test query ids can be surprisingly long like + /// `aggregating_merge_tree_simple_aggregate_function_string.query100.profile100`, + /// so make some allowance for them as well. + constexpr size_t QUERY_ID_MAX_LEN = 128; + static_assert(QUERY_ID_MAX_LEN <= std::numeric_limits::max()); +} + +namespace DB +{ + +LazyPipeFDs TraceSender::pipe; + +void TraceSender::send(TraceType trace_type, const StackTrace & stack_trace, Int64 size) +{ + constexpr size_t buf_size = sizeof(char) /// TraceCollector stop flag + + sizeof(UInt8) /// String size + + QUERY_ID_MAX_LEN /// Maximum query_id length + + sizeof(UInt8) /// Number of stack frames + + sizeof(StackTrace::FramePointers) /// Collected stack trace, maximum capacity + + sizeof(TraceType) /// trace type + + sizeof(UInt64) /// thread_id + + sizeof(Int64); /// size + + /// Write should be atomic to avoid overlaps + /// (since recursive collect() is possible) + static_assert(PIPE_BUF >= 512); + static_assert(buf_size <= 512, "Only write of PIPE_BUF to pipe is atomic and the minimal known PIPE_BUF across supported platforms is 512"); + + char buffer[buf_size]; + WriteBufferFromFileDescriptorDiscardOnFailure out(pipe.fds_rw[1], buf_size, buffer); + + StringRef query_id; + UInt64 thread_id; + + if (CurrentThread::isInitialized()) + { + query_id = CurrentThread::getQueryId(); + query_id.size = std::min(query_id.size, QUERY_ID_MAX_LEN); + + thread_id = CurrentThread::get().thread_id; + } + else + { + thread_id = MainThreadStatus::get()->thread_id; + } + + writeChar(false, out); /// true if requested to stop the collecting thread. + + writeBinary(static_cast(query_id.size), out); + out.write(query_id.data, query_id.size); + + size_t stack_trace_size = stack_trace.getSize(); + size_t stack_trace_offset = stack_trace.getOffset(); + writeIntBinary(UInt8(stack_trace_size - stack_trace_offset), out); + for (size_t i = stack_trace_offset; i < stack_trace_size; ++i) + writePODBinary(stack_trace.getFramePointers()[i], out); + + writePODBinary(trace_type, out); + writePODBinary(thread_id, out); + writePODBinary(size, out); + + out.next(); +} + +} diff --git a/src/Common/TraceSender.h b/src/Common/TraceSender.h new file mode 100644 index 00000000000..04c9286ad39 --- /dev/null +++ b/src/Common/TraceSender.h @@ -0,0 +1,36 @@ +#pragma once + +#include +#include + +class StackTrace; +class TraceCollector; + +namespace DB +{ + +enum class TraceType : uint8_t +{ + Real, + CPU, + Memory, + MemorySample, + MemoryPeak, +}; + +/// This is the second part of TraceCollector, that sends stacktrace to the pipe. +/// It has been split out to avoid dependency from interpreters part. +class TraceSender +{ +public: + /// Collect a stack trace. This method is signal safe. + /// Precondition: the TraceCollector object must be created. + /// size - for memory tracing is the amount of memory allocated; for other trace types it is 0. + static void send(TraceType trace_type, const StackTrace & stack_trace, Int64 size); + +private: + friend class TraceCollector; + static LazyPipeFDs pipe; +}; + +} diff --git a/src/Interpreters/TraceCollector.cpp b/src/Interpreters/TraceCollector.cpp index 523251fa2a2..84d2e25f70a 100644 --- a/src/Interpreters/TraceCollector.cpp +++ b/src/Interpreters/TraceCollector.cpp @@ -4,13 +4,9 @@ #include #include #include -#include #include #include #include -#include -#include -#include #include #include @@ -18,32 +14,16 @@ namespace DB { -namespace -{ - /// Normally query_id is a UUID (string with a fixed length) but user can provide custom query_id. - /// Thus upper bound on query_id length should be introduced to avoid buffer overflow in signal handler. - /// - /// And it cannot be large, since otherwise it will not fit into PIPE_BUF. - /// The performance test query ids can be surprisingly long like - /// `aggregating_merge_tree_simple_aggregate_function_string.query100.profile100`, - /// so make some allowance for them as well. - constexpr size_t QUERY_ID_MAX_LEN = 128; - static_assert(QUERY_ID_MAX_LEN <= std::numeric_limits::max()); -} - -LazyPipeFDs pipe; - - TraceCollector::TraceCollector(std::shared_ptr trace_log_) : trace_log(std::move(trace_log_)) { - pipe.open(); + TraceSender::pipe.open(); /** Turn write end of pipe to non-blocking mode to avoid deadlocks * when QueryProfiler is invoked under locks and TraceCollector cannot pull data from pipe. */ - pipe.setNonBlockingWrite(); - pipe.tryIncreaseSize(1 << 20); + TraceSender::pipe.setNonBlockingWrite(); + TraceSender::pipe.tryIncreaseSize(1 << 20); thread = ThreadFromGlobalPool(&TraceCollector::run, this); } @@ -56,60 +36,7 @@ TraceCollector::~TraceCollector() else stop(); - pipe.close(); -} - - -void TraceCollector::collect(TraceType trace_type, const StackTrace & stack_trace, Int64 size) -{ - constexpr size_t buf_size = sizeof(char) /// TraceCollector stop flag - + sizeof(UInt8) /// String size - + QUERY_ID_MAX_LEN /// Maximum query_id length - + sizeof(UInt8) /// Number of stack frames - + sizeof(StackTrace::FramePointers) /// Collected stack trace, maximum capacity - + sizeof(TraceType) /// trace type - + sizeof(UInt64) /// thread_id - + sizeof(Int64); /// size - - /// Write should be atomic to avoid overlaps - /// (since recursive collect() is possible) - static_assert(PIPE_BUF >= 512); - static_assert(buf_size <= 512, "Only write of PIPE_BUF to pipe is atomic and the minimal known PIPE_BUF across supported platforms is 512"); - - char buffer[buf_size]; - WriteBufferFromFileDescriptorDiscardOnFailure out(pipe.fds_rw[1], buf_size, buffer); - - StringRef query_id; - UInt64 thread_id; - - if (CurrentThread::isInitialized()) - { - query_id = CurrentThread::getQueryId(); - query_id.size = std::min(query_id.size, QUERY_ID_MAX_LEN); - - thread_id = CurrentThread::get().thread_id; - } - else - { - thread_id = MainThreadStatus::get()->thread_id; - } - - writeChar(false, out); /// true if requested to stop the collecting thread. - - writeBinary(static_cast(query_id.size), out); - out.write(query_id.data, query_id.size); - - size_t stack_trace_size = stack_trace.getSize(); - size_t stack_trace_offset = stack_trace.getOffset(); - writeIntBinary(UInt8(stack_trace_size - stack_trace_offset), out); - for (size_t i = stack_trace_offset; i < stack_trace_size; ++i) - writePODBinary(stack_trace.getFramePointers()[i], out); - - writePODBinary(trace_type, out); - writePODBinary(thread_id, out); - writePODBinary(size, out); - - out.next(); + TraceSender::pipe.close(); } @@ -121,7 +48,7 @@ void TraceCollector::collect(TraceType trace_type, const StackTrace & stack_trac */ void TraceCollector::stop() { - WriteBufferFromFileDescriptor out(pipe.fds_rw[1]); + WriteBufferFromFileDescriptor out(TraceSender::pipe.fds_rw[1]); writeChar(true, out); out.next(); thread.join(); @@ -132,7 +59,7 @@ void TraceCollector::run() { setThreadName("TraceCollector"); - ReadBufferFromFileDescriptor in(pipe.fds_rw[0]); + ReadBufferFromFileDescriptor in(TraceSender::pipe.fds_rw[0]); while (true) { diff --git a/src/Interpreters/TraceCollector.h b/src/Interpreters/TraceCollector.h index d3bbc74726e..3a9edf676be 100644 --- a/src/Interpreters/TraceCollector.h +++ b/src/Interpreters/TraceCollector.h @@ -1,7 +1,7 @@ #pragma once -#include "Common/PipeFDs.h" #include +#include class StackTrace; @@ -15,25 +15,16 @@ namespace DB class TraceLog; -enum class TraceType : uint8_t -{ - Real, - CPU, - Memory, - MemorySample, - MemoryPeak, -}; - class TraceCollector { public: TraceCollector(std::shared_ptr trace_log_); ~TraceCollector(); - /// Collect a stack trace. This method is signal safe. - /// Precondition: the TraceCollector object must be created. - /// size - for memory tracing is the amount of memory allocated; for other trace types it is 0. - static void collect(TraceType trace_type, const StackTrace & stack_trace, Int64 size); + static inline void collect(TraceType trace_type, const StackTrace & stack_trace, Int64 size) + { + return TraceSender::send(trace_type, stack_trace, size); + } private: std::shared_ptr trace_log; From 4a2bc64989c8e08f37ba4a89103ced27bee893fd Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 11 Jan 2022 23:51:41 +0300 Subject: [PATCH 14/31] Introduce clickhouse_common_zookeeper_no_log (w/o extra deps for examples) Signed-off-by: Azat Khuzhin --- src/Common/Config/CMakeLists.txt | 10 ++++---- src/Common/ZooKeeper/CMakeLists.txt | 25 ++++++++++++++++++- src/Common/ZooKeeper/ZooKeeperImpl.cpp | 5 ++++ src/Common/ZooKeeper/examples/CMakeLists.txt | 10 ++++---- utils/keeper-bench/CMakeLists.txt | 2 +- utils/zookeeper-cli/CMakeLists.txt | 2 +- utils/zookeeper-dump-tree/CMakeLists.txt | 2 +- utils/zookeeper-remove-by-list/CMakeLists.txt | 2 +- utils/zookeeper-test/CMakeLists.txt | 2 +- 9 files changed, 44 insertions(+), 16 deletions(-) diff --git a/src/Common/Config/CMakeLists.txt b/src/Common/Config/CMakeLists.txt index 4d72960f727..5825e64bc4f 100644 --- a/src/Common/Config/CMakeLists.txt +++ b/src/Common/Config/CMakeLists.txt @@ -11,7 +11,7 @@ add_library(clickhouse_common_config ${SRCS}) target_link_libraries(clickhouse_common_config PUBLIC - clickhouse_common_zookeeper + clickhouse_common_zookeeper_no_log common Poco::XML PRIVATE @@ -19,8 +19,8 @@ target_link_libraries(clickhouse_common_config ) if (USE_YAML_CPP) -target_link_libraries(clickhouse_common_config - PRIVATE - yaml-cpp -) + target_link_libraries(clickhouse_common_config + PRIVATE + yaml-cpp + ) endif() diff --git a/src/Common/ZooKeeper/CMakeLists.txt b/src/Common/ZooKeeper/CMakeLists.txt index d29fba53277..7e0558dd575 100644 --- a/src/Common/ZooKeeper/CMakeLists.txt +++ b/src/Common/ZooKeeper/CMakeLists.txt @@ -2,9 +2,32 @@ include("${ClickHouse_SOURCE_DIR}/cmake/dbms_glob_sources.cmake") add_headers_and_sources(clickhouse_common_zookeeper .) +# for clickhouse server add_library(clickhouse_common_zookeeper ${clickhouse_common_zookeeper_headers} ${clickhouse_common_zookeeper_sources}) +target_compile_definitions (clickhouse_common_zookeeper PRIVATE -DZOOKEEPER_LOG) +target_link_libraries (clickhouse_common_zookeeper + PUBLIC + clickhouse_common_io + common + PRIVATE + string_utils +) +# To avoid circular dependency from interpreters. +if (OS_DARWIN) + target_link_libraries (clickhouse_common_zookeeper PRIVATE -Wl,-undefined,dynamic_lookup) +else() + target_link_libraries (clickhouse_common_zookeeper PRIVATE -Wl,--unresolved-symbols=ignore-all) +endif() -target_link_libraries (clickhouse_common_zookeeper PUBLIC clickhouse_common_io common PRIVATE string_utils) +# for examples -- no logging (to avoid extra dependencies) +add_library(clickhouse_common_zookeeper_no_log ${clickhouse_common_zookeeper_headers} ${clickhouse_common_zookeeper_sources}) +target_link_libraries (clickhouse_common_zookeeper_no_log + PUBLIC + clickhouse_common_io + common + PRIVATE + string_utils +) if (ENABLE_EXAMPLES) add_subdirectory(examples) diff --git a/src/Common/ZooKeeper/ZooKeeperImpl.cpp b/src/Common/ZooKeeper/ZooKeeperImpl.cpp index 09b2548f2f4..759484cb497 100644 --- a/src/Common/ZooKeeper/ZooKeeperImpl.cpp +++ b/src/Common/ZooKeeper/ZooKeeperImpl.cpp @@ -1222,6 +1222,7 @@ void ZooKeeper::setZooKeeperLog(std::shared_ptr zk_log_) std::atomic_store(&zk_log, std::move(zk_log_)); } +#ifdef ZOOKEEPER_LOG void ZooKeeper::logOperationIfNeeded(const ZooKeeperRequestPtr & request, const ZooKeeperResponsePtr & response, bool finalize) { auto maybe_zk_log = std::atomic_load(&zk_log); @@ -1263,5 +1264,9 @@ void ZooKeeper::logOperationIfNeeded(const ZooKeeperRequestPtr & request, const maybe_zk_log->add(elem); } } +#else +void ZooKeeper::logOperationIfNeeded(const ZooKeeperRequestPtr &, const ZooKeeperResponsePtr &, bool) +{} +#endif } diff --git a/src/Common/ZooKeeper/examples/CMakeLists.txt b/src/Common/ZooKeeper/examples/CMakeLists.txt index bbfa3e1f137..8bec951e24f 100644 --- a/src/Common/ZooKeeper/examples/CMakeLists.txt +++ b/src/Common/ZooKeeper/examples/CMakeLists.txt @@ -1,14 +1,14 @@ add_executable(zkutil_test_commands zkutil_test_commands.cpp) -target_link_libraries(zkutil_test_commands PRIVATE clickhouse_common_zookeeper) +target_link_libraries(zkutil_test_commands PRIVATE clickhouse_common_zookeeper_no_log) add_executable(zkutil_test_commands_new_lib zkutil_test_commands_new_lib.cpp) -target_link_libraries(zkutil_test_commands_new_lib PRIVATE clickhouse_common_zookeeper string_utils) +target_link_libraries(zkutil_test_commands_new_lib PRIVATE clickhouse_common_zookeeper_no_log string_utils) add_executable(zkutil_test_async zkutil_test_async.cpp) -target_link_libraries(zkutil_test_async PRIVATE clickhouse_common_zookeeper) +target_link_libraries(zkutil_test_async PRIVATE clickhouse_common_zookeeper_no_log) add_executable (zk_many_watches_reconnect zk_many_watches_reconnect.cpp) -target_link_libraries (zk_many_watches_reconnect PRIVATE clickhouse_common_zookeeper clickhouse_common_config) +target_link_libraries (zk_many_watches_reconnect PRIVATE clickhouse_common_zookeeper_no_log clickhouse_common_config) add_executable (zookeeper_impl zookeeper_impl.cpp) -target_link_libraries (zookeeper_impl PRIVATE clickhouse_common_zookeeper) +target_link_libraries (zookeeper_impl PRIVATE clickhouse_common_zookeeper_no_log) diff --git a/utils/keeper-bench/CMakeLists.txt b/utils/keeper-bench/CMakeLists.txt index 2f12194d1b7..cd65152db87 100644 --- a/utils/keeper-bench/CMakeLists.txt +++ b/utils/keeper-bench/CMakeLists.txt @@ -1,2 +1,2 @@ add_executable(keeper-bench Generator.cpp Runner.cpp Stats.cpp main.cpp) -target_link_libraries(keeper-bench PRIVATE clickhouse_common_zookeeper) +target_link_libraries(keeper-bench PRIVATE clickhouse_common_zookeeper_no_log) diff --git a/utils/zookeeper-cli/CMakeLists.txt b/utils/zookeeper-cli/CMakeLists.txt index 2199a1b38ff..0258b0d695c 100644 --- a/utils/zookeeper-cli/CMakeLists.txt +++ b/utils/zookeeper-cli/CMakeLists.txt @@ -1,2 +1,2 @@ add_executable(clickhouse-zookeeper-cli zookeeper-cli.cpp) -target_link_libraries(clickhouse-zookeeper-cli PRIVATE clickhouse_common_zookeeper) +target_link_libraries(clickhouse-zookeeper-cli PRIVATE clickhouse_common_zookeeper_no_log) diff --git a/utils/zookeeper-dump-tree/CMakeLists.txt b/utils/zookeeper-dump-tree/CMakeLists.txt index 9f5da351068..db86c7005b2 100644 --- a/utils/zookeeper-dump-tree/CMakeLists.txt +++ b/utils/zookeeper-dump-tree/CMakeLists.txt @@ -1,2 +1,2 @@ add_executable (zookeeper-dump-tree main.cpp ${SRCS}) -target_link_libraries(zookeeper-dump-tree PRIVATE clickhouse_common_zookeeper clickhouse_common_io boost::program_options) +target_link_libraries(zookeeper-dump-tree PRIVATE clickhouse_common_zookeeper_no_log clickhouse_common_io boost::program_options) diff --git a/utils/zookeeper-remove-by-list/CMakeLists.txt b/utils/zookeeper-remove-by-list/CMakeLists.txt index c31b1ec3388..f24b794c629 100644 --- a/utils/zookeeper-remove-by-list/CMakeLists.txt +++ b/utils/zookeeper-remove-by-list/CMakeLists.txt @@ -1,2 +1,2 @@ add_executable (zookeeper-remove-by-list main.cpp ${SRCS}) -target_link_libraries(zookeeper-remove-by-list PRIVATE clickhouse_common_zookeeper boost::program_options) +target_link_libraries(zookeeper-remove-by-list PRIVATE clickhouse_common_zookeeper_no_log boost::program_options) diff --git a/utils/zookeeper-test/CMakeLists.txt b/utils/zookeeper-test/CMakeLists.txt index 56a1d3e380b..f99539fed79 100644 --- a/utils/zookeeper-test/CMakeLists.txt +++ b/utils/zookeeper-test/CMakeLists.txt @@ -1,2 +1,2 @@ add_executable(zk-test main.cpp) -target_link_libraries(zk-test PRIVATE clickhouse_common_zookeeper) +target_link_libraries(zk-test PRIVATE clickhouse_common_zookeeper_no_log) From 003cdbcf60cf80231276964cc26e97b00e80781a Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 12 Jan 2022 07:17:56 +0300 Subject: [PATCH 15/31] Introduce clickhouse_common_config_no_zookeeper_log Otherwise incorrect zookeeper (w/o log) will be used in clickhouse server itself. --- src/Common/Config/CMakeLists.txt | 17 ++++++++++++----- utils/config-processor/CMakeLists.txt | 2 +- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/Common/Config/CMakeLists.txt b/src/Common/Config/CMakeLists.txt index 5825e64bc4f..cc41a8b2bb2 100644 --- a/src/Common/Config/CMakeLists.txt +++ b/src/Common/Config/CMakeLists.txt @@ -8,8 +8,17 @@ set (SRCS ) add_library(clickhouse_common_config ${SRCS}) - target_link_libraries(clickhouse_common_config + PUBLIC + clickhouse_common_zookeeper + common + Poco::XML + PRIVATE + string_utils +) + +add_library(clickhouse_common_config_no_zookeeper_log ${SRCS}) +target_link_libraries(clickhouse_common_config_no_zookeeper_log PUBLIC clickhouse_common_zookeeper_no_log common @@ -19,8 +28,6 @@ target_link_libraries(clickhouse_common_config ) if (USE_YAML_CPP) - target_link_libraries(clickhouse_common_config - PRIVATE - yaml-cpp - ) + target_link_libraries(clickhouse_common_config PRIVATE yaml-cpp) + target_link_libraries(clickhouse_common_config_no_zookeeper_log PRIVATE yaml-cpp) endif() diff --git a/utils/config-processor/CMakeLists.txt b/utils/config-processor/CMakeLists.txt index a378d66a3d3..76c10b5f2fd 100644 --- a/utils/config-processor/CMakeLists.txt +++ b/utils/config-processor/CMakeLists.txt @@ -1,2 +1,2 @@ add_executable (config-processor config-processor.cpp) -target_link_libraries(config-processor PRIVATE clickhouse_common_config) +target_link_libraries(config-processor PRIVATE clickhouse_common_config_no_zookeeper_log) From 001db90cd4f514143fd51462b29b73c4a53cfac3 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 12 Jan 2022 10:30:09 +0300 Subject: [PATCH 16/31] Remove extra clickhouse_common_config for unit_tests_dbms --- src/CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index c9e9f736e0d..2d63d3b7d48 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -568,7 +568,6 @@ if (ENABLE_TESTS AND USE_GTEST) dbms clickhouse_common_config clickhouse_common_zookeeper - clickhouse_common_config string_utils) add_check(unit_tests_dbms) From bf803472b187dc0b2fff971a4214cd51ee304f9a Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 14 Jan 2022 18:44:10 +0300 Subject: [PATCH 17/31] Add a tests for a TTL bug in zero copy replication --- .../MergeTree/MergeTreePartsMover.cpp | 2 +- src/Storages/StorageReplicatedMergeTree.cpp | 27 ++++---- src/Storages/StorageReplicatedMergeTree.h | 2 +- .../test_s3_zero_copy_ttl/__init__.py | 1 + .../test_s3_zero_copy_ttl/configs/s3.xml | 26 +++++++ .../integration/test_s3_zero_copy_ttl/test.py | 69 +++++++++++++++++++ 6 files changed, 113 insertions(+), 14 deletions(-) create mode 100644 tests/integration/test_s3_zero_copy_ttl/__init__.py create mode 100644 tests/integration/test_s3_zero_copy_ttl/configs/s3.xml create mode 100644 tests/integration/test_s3_zero_copy_ttl/test.py diff --git a/src/Storages/MergeTree/MergeTreePartsMover.cpp b/src/Storages/MergeTree/MergeTreePartsMover.cpp index 5a889ea5e8b..190fc0d30a0 100644 --- a/src/Storages/MergeTree/MergeTreePartsMover.cpp +++ b/src/Storages/MergeTree/MergeTreePartsMover.cpp @@ -200,7 +200,7 @@ MergeTreeData::DataPartPtr MergeTreePartsMover::clonePart(const MergeTreeMoveEnt auto settings = data->getSettings(); auto part = moving_part.part; auto disk = moving_part.reserved_space->getDisk(); - LOG_DEBUG(log, "Cloning part {} from {} to {}", part->name, part->volume->getDisk()->getName(), disk->getName()); + LOG_DEBUG(log, "Cloning part {} from '{}' to '{}'", part->name, part->volume->getDisk()->getName(), disk->getName()); const String directory_to_move = "moving"; if (disk->supportZeroCopyReplication() && settings->allow_remote_fs_zero_copy_replication) diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index 91a9c8567ba..7cd646531da 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -7128,11 +7128,11 @@ bool StorageReplicatedMergeTree::unlockSharedData(const IMergeTreeDataPart & par } -bool StorageReplicatedMergeTree::unlockSharedDataByID(String id, const String & table_uuid, const String & part_name, +bool StorageReplicatedMergeTree::unlockSharedDataByID(String part_id, const String & table_uuid, const String & part_name, const String & replica_name_, DiskPtr disk, zkutil::ZooKeeperPtr zookeeper_ptr, const MergeTreeSettings & settings, Poco::Logger * logger, const String & zookeeper_path_old) { - boost::replace_all(id, "/", "_"); + boost::replace_all(part_id, "/", "_"); Strings zc_zookeeper_paths = getZeroCopyPartPath(settings, disk->getType(), table_uuid, part_name, zookeeper_path_old); @@ -7140,13 +7140,16 @@ bool StorageReplicatedMergeTree::unlockSharedDataByID(String id, const String & for (const auto & zc_zookeeper_path : zc_zookeeper_paths) { - String zookeeper_part_uniq_node = fs::path(zc_zookeeper_path) / id; - String zookeeper_node = fs::path(zookeeper_part_uniq_node) / replica_name_; + String zookeeper_part_uniq_node = fs::path(zc_zookeeper_path) / part_id; - LOG_TRACE(logger, "Remove zookeeper lock {}", zookeeper_node); + /// Delete our replica node for part from zookeeper (we are not interested in it anymore) + String zookeeper_part_replica_node = fs::path(zookeeper_part_uniq_node) / replica_name_; - zookeeper_ptr->tryRemove(zookeeper_node); + LOG_TRACE(logger, "Remove zookeeper lock {}", zookeeper_part_replica_node); + zookeeper_ptr->tryRemove(zookeeper_part_replica_node); + + /// Check, maybe we were the last replica and can remove part forever Strings children; zookeeper_ptr->tryGetChildren(zookeeper_part_uniq_node, children); @@ -7157,9 +7160,9 @@ bool StorageReplicatedMergeTree::unlockSharedDataByID(String id, const String & continue; } - auto e = zookeeper_ptr->tryRemove(zookeeper_part_uniq_node); + auto error_code = zookeeper_ptr->tryRemove(zookeeper_part_uniq_node); - LOG_TRACE(logger, "Remove parent zookeeper lock {} : {}", zookeeper_part_uniq_node, e != Coordination::Error::ZNOTEMPTY); + LOG_TRACE(logger, "Remove parent zookeeper lock {} : {}", zookeeper_part_uniq_node, error_code != Coordination::Error::ZNOTEMPTY); /// Even when we have lock with same part name, but with different uniq, we can remove files on S3 children.clear(); @@ -7168,9 +7171,9 @@ bool StorageReplicatedMergeTree::unlockSharedDataByID(String id, const String & if (children.empty()) { /// Cleanup after last uniq removing - e = zookeeper_ptr->tryRemove(zookeeper_part_node); + error_code = zookeeper_ptr->tryRemove(zookeeper_part_node); - LOG_TRACE(logger, "Remove parent zookeeper lock {} : {}", zookeeper_part_node, e != Coordination::Error::ZNOTEMPTY); + LOG_TRACE(logger, "Remove parent zookeeper lock {} : {}", zookeeper_part_node, error_code != Coordination::Error::ZNOTEMPTY); } else { @@ -7213,7 +7216,7 @@ String StorageReplicatedMergeTree::getSharedDataReplica( zkutil::ZooKeeperPtr zookeeper = tryGetZooKeeper(); if (!zookeeper) - return best_replica; + return ""; Strings zc_zookeeper_paths = getZeroCopyPartPath(*getSettings(), disk_type, getTableSharedID(), part.name, zookeeper_path); @@ -7251,7 +7254,7 @@ String StorageReplicatedMergeTree::getSharedDataReplica( LOG_TRACE(log, "Found zookeper active replicas for part {}: {}", part.name, active_replicas.size()); if (active_replicas.empty()) - return best_replica; + return ""; /** You must select the best (most relevant) replica. * This is a replica with the maximum `log_pointer`, then with the minimum `queue` size. diff --git a/src/Storages/StorageReplicatedMergeTree.h b/src/Storages/StorageReplicatedMergeTree.h index e390a0bcea4..2babd01d11d 100644 --- a/src/Storages/StorageReplicatedMergeTree.h +++ b/src/Storages/StorageReplicatedMergeTree.h @@ -243,7 +243,7 @@ public: /// Unlock shared data part in zookeeper by part id /// Return true if data unlocked /// Return false if data is still used by another node - static bool unlockSharedDataByID(String id, const String & table_uuid, const String & part_name, const String & replica_name_, + static bool unlockSharedDataByID(String part_id, const String & table_uuid, const String & part_name, const String & replica_name_, DiskPtr disk, zkutil::ZooKeeperPtr zookeeper_, const MergeTreeSettings & settings, Poco::Logger * logger, const String & zookeeper_path_old); diff --git a/tests/integration/test_s3_zero_copy_ttl/__init__.py b/tests/integration/test_s3_zero_copy_ttl/__init__.py new file mode 100644 index 00000000000..e5a0d9b4834 --- /dev/null +++ b/tests/integration/test_s3_zero_copy_ttl/__init__.py @@ -0,0 +1 @@ +#!/usr/bin/env python3 diff --git a/tests/integration/test_s3_zero_copy_ttl/configs/s3.xml b/tests/integration/test_s3_zero_copy_ttl/configs/s3.xml new file mode 100644 index 00000000000..c4889186e38 --- /dev/null +++ b/tests/integration/test_s3_zero_copy_ttl/configs/s3.xml @@ -0,0 +1,26 @@ + + + + + s3 + http://minio1:9001/root/data/ + minio + minio123 + + + + + + +
+ default +
+ + s3_disk + + + + + + + diff --git a/tests/integration/test_s3_zero_copy_ttl/test.py b/tests/integration/test_s3_zero_copy_ttl/test.py new file mode 100644 index 00000000000..5f63bfbfdff --- /dev/null +++ b/tests/integration/test_s3_zero_copy_ttl/test.py @@ -0,0 +1,69 @@ +#!/usr/bin/env python3 +import time + +import pytest +from helpers.cluster import ClickHouseCluster + +cluster = ClickHouseCluster(__file__) +node1 = cluster.add_instance("node1", main_configs=["configs/s3.xml"], with_minio=True, with_zookeeper=True) +node2 = cluster.add_instance("node2", main_configs=["configs/s3.xml"], with_minio=True, with_zookeeper=True) +node3 = cluster.add_instance("node3", main_configs=["configs/s3.xml"], with_minio=True, with_zookeeper=True) + +@pytest.fixture(scope="module") +def started_cluster(): + try: + cluster.start() + + yield cluster + finally: + cluster.shutdown() + +def test_ttl_move_and_s3(started_cluster): + for i, node in enumerate([node1, node2, node3]): + node.query( + """ + CREATE TABLE s3_test_with_ttl (date DateTime, id UInt32, value String) + ENGINE=ReplicatedMergeTree('/clickhouse/tables/s3_test', '{}') + ORDER BY id + PARTITION BY id + TTL date TO DISK 's3_disk' + SETTINGS storage_policy='s3_and_default' + """.format(i)) + + node1.query("SYSTEM STOP MOVES s3_test_with_ttl") + + node2.query("SYSTEM STOP MOVES s3_test_with_ttl") + + for i in range(30): + if i % 2 == 0: + node = node1 + else: + node = node2 + + node.query(f"INSERT INTO s3_test_with_ttl SELECT now() + 5, {i}, randomPrintableASCII(1048570)") + + node1.query("SYSTEM SYNC REPLICA s3_test_with_ttl") + node2.query("SYSTEM SYNC REPLICA s3_test_with_ttl") + node3.query("SYSTEM SYNC REPLICA s3_test_with_ttl") + + assert node1.query("SELECT COUNT() FROM s3_test_with_ttl") == "30\n" + assert node2.query("SELECT COUNT() FROM s3_test_with_ttl") == "30\n" + + node1.query("SYSTEM START MOVES s3_test_with_ttl") + node2.query("SYSTEM START MOVES s3_test_with_ttl") + + assert node1.query("SELECT COUNT() FROM s3_test_with_ttl") == "30\n" + assert node2.query("SELECT COUNT() FROM s3_test_with_ttl") == "30\n" + + time.sleep(5) + + print(node1.query("SELECT * FROM system.parts WHERE table = 's3_test_with_ttl' FORMAT Vertical")) + + minio = cluster.minio_client + objects = minio.list_objects(cluster.minio_bucket, 'data/', recursive=True) + counter = 0 + for obj in objects: + print("Objectname:", obj.object_name, "metadata:", obj.metadata) + counter += 1 + print("Total objects", counter) + assert counter == 300 From a0ad7a1014deed80814622032b456da8075ccc35 Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Sun, 16 Jan 2022 00:06:10 +0000 Subject: [PATCH 18/31] Update IExternalLoadable interface --- src/Dictionaries/CacheDictionary.cpp | 2 +- src/Dictionaries/DirectDictionary.cpp | 2 +- src/Dictionaries/FlatDictionary.cpp | 4 ++-- src/Dictionaries/HashedArrayDictionary.cpp | 2 +- src/Dictionaries/HashedDictionary.cpp | 2 +- src/Dictionaries/IDictionary.h | 15 ++++++++++----- src/Dictionaries/IPAddressDictionary.cpp | 8 ++++---- src/Interpreters/CatBoostModel.h | 2 +- src/Interpreters/IExternalLoadable.h | 2 +- src/Interpreters/UserDefinedExecutableFunction.h | 2 +- 10 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/Dictionaries/CacheDictionary.cpp b/src/Dictionaries/CacheDictionary.cpp index 7da25298cf2..901f982987b 100644 --- a/src/Dictionaries/CacheDictionary.cpp +++ b/src/Dictionaries/CacheDictionary.cpp @@ -69,7 +69,7 @@ CacheDictionary::CacheDictionary( , rnd_engine(randomSeed()) { if (!source_ptr->supportsSelectiveLoad()) - throw Exception(ErrorCodes::UNSUPPORTED_METHOD, "{}: source cannot be used with CacheDictionary", full_name); + throw Exception(ErrorCodes::UNSUPPORTED_METHOD, "{}: source cannot be used with CacheDictionary", getFullName()); } template diff --git a/src/Dictionaries/DirectDictionary.cpp b/src/Dictionaries/DirectDictionary.cpp index 19bbcb6ca98..9a65d916022 100644 --- a/src/Dictionaries/DirectDictionary.cpp +++ b/src/Dictionaries/DirectDictionary.cpp @@ -28,7 +28,7 @@ DirectDictionary::DirectDictionary( , source_ptr{std::move(source_ptr_)} { if (!source_ptr->supportsSelectiveLoad()) - throw Exception(ErrorCodes::UNSUPPORTED_METHOD, "{}: source cannot be used with DirectDictionary", full_name); + throw Exception(ErrorCodes::UNSUPPORTED_METHOD, "{}: source cannot be used with DirectDictionary", getFullName()); } template diff --git a/src/Dictionaries/FlatDictionary.cpp b/src/Dictionaries/FlatDictionary.cpp index c260924a82b..87f9c2c5950 100644 --- a/src/Dictionaries/FlatDictionary.cpp +++ b/src/Dictionaries/FlatDictionary.cpp @@ -370,7 +370,7 @@ void FlatDictionary::loadData() updateData(); if (configuration.require_nonempty && 0 == element_count) - throw Exception(ErrorCodes::DICTIONARY_IS_EMPTY, "{}: dictionary source is empty and 'require_nonempty' property is set.", full_name); + throw Exception(ErrorCodes::DICTIONARY_IS_EMPTY, "{}: dictionary source is empty and 'require_nonempty' property is set.", getFullName()); } void FlatDictionary::calculateBytesAllocated() @@ -478,7 +478,7 @@ void FlatDictionary::resize(Attribute & attribute, UInt64 key) if (key >= configuration.max_array_size) throw Exception(ErrorCodes::ARGUMENT_OUT_OF_BOUND, "{}: identifier should be less than {}", - full_name, + getFullName(), toString(configuration.max_array_size)); auto & container = std::get>(attribute.container); diff --git a/src/Dictionaries/HashedArrayDictionary.cpp b/src/Dictionaries/HashedArrayDictionary.cpp index 062620fb25b..99a9d4d41f3 100644 --- a/src/Dictionaries/HashedArrayDictionary.cpp +++ b/src/Dictionaries/HashedArrayDictionary.cpp @@ -695,7 +695,7 @@ void HashedArrayDictionary::loadData() if (configuration.require_nonempty && 0 == element_count) throw Exception(ErrorCodes::DICTIONARY_IS_EMPTY, "{}: dictionary source is empty and 'require_nonempty' property is set.", - full_name); + getFullName()); } template diff --git a/src/Dictionaries/HashedDictionary.cpp b/src/Dictionaries/HashedDictionary.cpp index b4f3aece174..61f1ae0b4dd 100644 --- a/src/Dictionaries/HashedDictionary.cpp +++ b/src/Dictionaries/HashedDictionary.cpp @@ -583,7 +583,7 @@ void HashedDictionary::loadData() if (configuration.require_nonempty && 0 == element_count) throw Exception(ErrorCodes::DICTIONARY_IS_EMPTY, "{}: dictionary source is empty and 'require_nonempty' property is set.", - full_name); + getFullName()); } template diff --git a/src/Dictionaries/IDictionary.h b/src/Dictionaries/IDictionary.h index 991b45e8e8f..16cd75ea42a 100644 --- a/src/Dictionaries/IDictionary.h +++ b/src/Dictionaries/IDictionary.h @@ -52,11 +52,14 @@ class IDictionary : public IExternalLoadable public: explicit IDictionary(const StorageID & dictionary_id_) : dictionary_id(dictionary_id_) - , full_name(dictionary_id.getInternalDictionaryName()) { } - const std::string & getFullName() const { return full_name; } + std::string getFullName() const + { + std::lock_guard lock{name_mutex}; + return dictionary_id.getFullNameNotQuoted(); + } StorageID getDictionaryID() const { @@ -69,10 +72,13 @@ public: std::lock_guard lock{name_mutex}; assert(new_name.uuid == dictionary_id.uuid && dictionary_id.uuid != UUIDHelpers::Nil); dictionary_id = new_name; - full_name = dictionary_id.getInternalDictionaryName(); } - const std::string & getLoadableName() const override final { return getFullName(); } + std::string getLoadableName() const override final + { + std::lock_guard lock{name_mutex}; + return dictionary_id.getInternalDictionaryName(); + } /// Specifies that no database is used. /// Sometimes we cannot simply use an empty string for that because an empty string is @@ -235,7 +241,6 @@ private: mutable StorageID dictionary_id; protected: - mutable String full_name; String dictionary_comment; }; diff --git a/src/Dictionaries/IPAddressDictionary.cpp b/src/Dictionaries/IPAddressDictionary.cpp index 266f8a7415c..bba4b05d4d9 100644 --- a/src/Dictionaries/IPAddressDictionary.cpp +++ b/src/Dictionaries/IPAddressDictionary.cpp @@ -331,7 +331,7 @@ void IPAddressDictionary::createAttributes() if (attribute.is_nullable) throw Exception(ErrorCodes::UNSUPPORTED_METHOD, "{}: array or nullable attributes not supported for dictionary of type {}", - full_name, + getFullName(), getTypeName()); attribute_index_by_name.emplace(attribute.name, attributes.size()); @@ -340,7 +340,7 @@ void IPAddressDictionary::createAttributes() if (attribute.hierarchical) throw Exception(ErrorCodes::TYPE_MISMATCH, "{}: hierarchical attributes not supported for dictionary of type {}", - full_name, + getFullName(), getTypeName()); } }; @@ -515,7 +515,7 @@ void IPAddressDictionary::loadData() LOG_TRACE(logger, "{} ip records are read", ip_records.size()); if (require_nonempty && 0 == element_count) - throw Exception(ErrorCodes::DICTIONARY_IS_EMPTY, "{}: dictionary source is empty and 'require_nonempty' property is set.", full_name); + throw Exception(ErrorCodes::DICTIONARY_IS_EMPTY, "{}: dictionary source is empty and 'require_nonempty' property is set.", getFullName()); } template @@ -776,7 +776,7 @@ const IPAddressDictionary::Attribute & IPAddressDictionary::getAttribute(const s { const auto it = attribute_index_by_name.find(attribute_name); if (it == std::end(attribute_index_by_name)) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "{}: no such attribute '{}'", full_name, attribute_name); + throw Exception(ErrorCodes::BAD_ARGUMENTS, "{}: no such attribute '{}'", getFullName(), attribute_name); return attributes[it->second]; } diff --git a/src/Interpreters/CatBoostModel.h b/src/Interpreters/CatBoostModel.h index eb599b43ef2..51bf0ba94f5 100644 --- a/src/Interpreters/CatBoostModel.h +++ b/src/Interpreters/CatBoostModel.h @@ -61,7 +61,7 @@ public: const ExternalLoadableLifetime & getLifetime() const override; - const std::string & getLoadableName() const override { return name; } + std::string getLoadableName() const override { return name; } bool supportUpdates() const override { return true; } diff --git a/src/Interpreters/IExternalLoadable.h b/src/Interpreters/IExternalLoadable.h index f49e8e86b3a..3c004508b0a 100644 --- a/src/Interpreters/IExternalLoadable.h +++ b/src/Interpreters/IExternalLoadable.h @@ -37,7 +37,7 @@ public: virtual const ExternalLoadableLifetime & getLifetime() const = 0; - virtual const std::string & getLoadableName() const = 0; + virtual std::string getLoadableName() const = 0; /// True if object can be updated when lifetime exceeded. virtual bool supportUpdates() const = 0; /// If lifetime exceeded and isModified(), ExternalLoader replace current object with the result of clone(). diff --git a/src/Interpreters/UserDefinedExecutableFunction.h b/src/Interpreters/UserDefinedExecutableFunction.h index a4fad8ceb7b..80d6b85ad90 100644 --- a/src/Interpreters/UserDefinedExecutableFunction.h +++ b/src/Interpreters/UserDefinedExecutableFunction.h @@ -33,7 +33,7 @@ public: return lifetime; } - const std::string & getLoadableName() const override + std::string getLoadableName() const override { return configuration.name; } From 0e6b90f513f827cb9c59e35bda46b6883fb291aa Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Sun, 16 Jan 2022 12:31:51 +0000 Subject: [PATCH 19/31] Fix tests --- src/Dictionaries/IDictionary.h | 2 +- src/Storages/StorageDictionary.cpp | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/Dictionaries/IDictionary.h b/src/Dictionaries/IDictionary.h index 16cd75ea42a..cbfb3133970 100644 --- a/src/Dictionaries/IDictionary.h +++ b/src/Dictionaries/IDictionary.h @@ -58,7 +58,7 @@ public: std::string getFullName() const { std::lock_guard lock{name_mutex}; - return dictionary_id.getFullNameNotQuoted(); + return dictionary_id.getNameForLogs(); } StorageID getDictionaryID() const diff --git a/src/Storages/StorageDictionary.cpp b/src/Storages/StorageDictionary.cpp index da8c5f115b2..6c907e85cda 100644 --- a/src/Storages/StorageDictionary.cpp +++ b/src/Storages/StorageDictionary.cpp @@ -231,10 +231,6 @@ void StorageDictionary::renameInMemory(const StorageID & new_table_id) configuration->setString("dictionary.database", new_table_id.database_name); configuration->setString("dictionary.name", new_table_id.table_name); - if (move_to_atomic) - configuration->setString("dictionary.uuid", toString(new_table_id.uuid)); - else if (move_to_ordinary) - configuration->remove("dictionary.uuid"); } /// Dictionary is moving between databases of different engines or is renaming inside Ordinary database From 886512694c65f35bbab0e7477fbb53106606e2cc Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Sun, 16 Jan 2022 15:30:23 +0000 Subject: [PATCH 20/31] Fixed build --- src/Storages/StorageDictionary.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Storages/StorageDictionary.cpp b/src/Storages/StorageDictionary.cpp index 6c907e85cda..c531edaa8d3 100644 --- a/src/Storages/StorageDictionary.cpp +++ b/src/Storages/StorageDictionary.cpp @@ -222,8 +222,8 @@ void StorageDictionary::renameInMemory(const StorageID & new_table_id) /// It's DDL dictionary, need to update configuration and reload - bool move_to_atomic = old_table_id.uuid == UUIDHelpers::Nil && new_table_id.uuid != UUIDHelpers::Nil; - bool move_to_ordinary = old_table_id.uuid != UUIDHelpers::Nil && new_table_id.uuid == UUIDHelpers::Nil; + [[maybe_unused]] bool move_to_atomic = old_table_id.uuid == UUIDHelpers::Nil && new_table_id.uuid != UUIDHelpers::Nil; + [[maybe_unused]] bool move_to_ordinary = old_table_id.uuid != UUIDHelpers::Nil && new_table_id.uuid == UUIDHelpers::Nil; assert(old_table_id.uuid == new_table_id.uuid || move_to_atomic || move_to_ordinary); { From 4caef03e6a4109c47ac19385f0854c8c66058153 Mon Sep 17 00:00:00 2001 From: alesapin Date: Mon, 17 Jan 2022 14:52:51 +0300 Subject: [PATCH 21/31] Add ephemeral lock in zookeeper --- src/Common/ZooKeeper/ZooKeeperLock.cpp | 95 +++++++++++++++++ src/Common/ZooKeeper/ZooKeeperLock.h | 54 ++++++++++ src/Interpreters/DDLWorker.cpp | 112 +------------------- src/Interpreters/DDLWorker.h | 8 +- src/Storages/MergeTree/MergeTreeData.cpp | 38 ++++++- src/Storages/MergeTree/MergeTreeData.h | 7 ++ src/Storages/MergeTree/ZeroCopyLock.cpp | 9 ++ src/Storages/MergeTree/ZeroCopyLock.h | 21 ++++ src/Storages/StorageReplicatedMergeTree.cpp | 25 +++++ src/Storages/StorageReplicatedMergeTree.h | 4 + 10 files changed, 260 insertions(+), 113 deletions(-) create mode 100644 src/Common/ZooKeeper/ZooKeeperLock.cpp create mode 100644 src/Common/ZooKeeper/ZooKeeperLock.h create mode 100644 src/Storages/MergeTree/ZeroCopyLock.cpp create mode 100644 src/Storages/MergeTree/ZeroCopyLock.h diff --git a/src/Common/ZooKeeper/ZooKeeperLock.cpp b/src/Common/ZooKeeper/ZooKeeperLock.cpp new file mode 100644 index 00000000000..56be8d59cb2 --- /dev/null +++ b/src/Common/ZooKeeper/ZooKeeperLock.cpp @@ -0,0 +1,95 @@ +#include +#include + +namespace DB +{ + +namespace ErrorCodes +{ + extern const int LOGICAL_ERROR; +} + +} + +namespace fs = std::filesystem; + +namespace zkutil +{ + +ZooKeeperLock::ZooKeeperLock( + const ZooKeeperPtr & zookeeper_, + const std::string & lock_prefix_, + const std::string & lock_name_, + const std::string & lock_message_) + : zookeeper(zookeeper_) + , lock_path(fs::path(lock_prefix_) / lock_name_) + , lock_message(lock_message_) + , log(&Poco::Logger::get("zkutil::Lock")) +{ + zookeeper->createIfNotExists(lock_prefix_, ""); +} + +ZooKeeperLock::~ZooKeeperLock() +{ + try + { + unlock(); + } + catch (...) + { + DB::tryLogCurrentException(__PRETTY_FUNCTION__); + } +} + +void ZooKeeperLock::unlock() +{ + if (!locked) + return; + + locked = false; + + if (zookeeper->expired()) + { + LOG_WARNING(log, "Lock is lost, because session was expired. Path: {}, message: {}", lock_path, lock_message); + return; + } + + Coordination::Stat stat; + std::string dummy; + /// NOTE It will throw if session expired after we checked it above + bool result = zookeeper->tryGet(lock_path, dummy, &stat); + + if (result && stat.ephemeralOwner == zookeeper->getClientID()) + zookeeper->remove(lock_path, -1); + else if (result) + throw DB::Exception(DB::ErrorCodes::LOGICAL_ERROR, "Lock is lost, it has another owner. Path: {}, message: {}, owner: {}, our id: {}", + lock_path, lock_message, stat.ephemeralOwner, zookeeper->getClientID()); + else + throw DB::Exception(DB::ErrorCodes::LOGICAL_ERROR, "Lock is lost, node does not exist. Path: {}, message: {}", lock_path, lock_message); +} + +bool ZooKeeperLock::tryLock() +{ + std::string dummy; + Coordination::Error code = zookeeper->tryCreate(lock_path, lock_message, zkutil::CreateMode::Ephemeral, dummy); + + if (code == Coordination::Error::ZOK) + { + locked = true; + } + else if (code != Coordination::Error::ZNODEEXISTS) + { + throw Coordination::Exception(code); + } + + return locked; +} + +std::unique_ptr createSimpleZooKeeperLock( + const ZooKeeperPtr & zookeeper, const String & lock_prefix, const String & lock_name, const String & lock_message) +{ + return std::make_unique(zookeeper, lock_prefix, lock_name, lock_message); +} + + +} diff --git a/src/Common/ZooKeeper/ZooKeeperLock.h b/src/Common/ZooKeeper/ZooKeeperLock.h new file mode 100644 index 00000000000..218f14ef132 --- /dev/null +++ b/src/Common/ZooKeeper/ZooKeeperLock.h @@ -0,0 +1,54 @@ +#pragma once +#include +#include +#include +#include +#include + +namespace zkutil +{ + +/** Caveats: usage of locks in ZooKeeper is incorrect in 99% of cases, + * and highlights your poor understanding of distributed systems. + * + * It's only correct if all the operations that are performed under lock + * are atomically checking that the lock still holds + * or if we ensure that these operations will be undone if lock is lost + * (due to ZooKeeper session loss) that's very difficult to achieve. + * + * It's Ok if every operation that we perform under lock is actually operation in ZooKeeper. + * + * In 1% of cases when you can correctly use Lock, the logic is complex enough, so you don't need this class. + * + * TLDR: Don't use this code if you are not sure. We only have a few cases of it's usage. + */ +class ZooKeeperLock +{ +public: + /// lock_prefix - path where the ephemeral lock node will be created + /// lock_name - the name of the ephemeral lock node + ZooKeeperLock( + const ZooKeeperPtr & zookeeper_, + const std::string & lock_prefix_, + const std::string & lock_name_, + const std::string & lock_message_ = ""); + + ~ZooKeeperLock(); + + void unlock(); + bool tryLock(); + +private: + zkutil::ZooKeeperPtr zookeeper; + + std::string lock_path; + std::string lock_message; + Poco::Logger * log; + bool locked = false; + +}; + +std::unique_ptr createSimpleZooKeeperLock( + const ZooKeeperPtr & zookeeper, const String & lock_prefix, const String & lock_name, const String & lock_message); + +} diff --git a/src/Interpreters/DDLWorker.cpp b/src/Interpreters/DDLWorker.cpp index ee5dc4deebb..3eeb817cbab 100644 --- a/src/Interpreters/DDLWorker.cpp +++ b/src/Interpreters/DDLWorker.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -54,113 +55,6 @@ namespace ErrorCodes constexpr const char * TASK_PROCESSED_OUT_REASON = "Task has been already processed"; -/** Caveats: usage of locks in ZooKeeper is incorrect in 99% of cases, - * and highlights your poor understanding of distributed systems. - * - * It's only correct if all the operations that are performed under lock - * are atomically checking that the lock still holds - * or if we ensure that these operations will be undone if lock is lost - * (due to ZooKeeper session loss) that's very difficult to achieve. - * - * It's Ok if every operation that we perform under lock is actually operation in ZooKeeper. - * - * In 1% of cases when you can correctly use Lock, the logic is complex enough, so you don't need this class. - * - * TLDR: Don't use this code. - * We only have a few cases of it's usage and it will be removed. - */ -class ZooKeeperLock -{ -public: - /// lock_prefix - path where the ephemeral lock node will be created - /// lock_name - the name of the ephemeral lock node - ZooKeeperLock( - const zkutil::ZooKeeperPtr & zookeeper_, - const std::string & lock_prefix_, - const std::string & lock_name_, - const std::string & lock_message_ = "") - : - zookeeper(zookeeper_), - lock_path(fs::path(lock_prefix_) / lock_name_), - lock_message(lock_message_), - log(&Poco::Logger::get("zkutil::Lock")) - { - zookeeper->createIfNotExists(lock_prefix_, ""); - } - - ~ZooKeeperLock() - { - try - { - unlock(); - } - catch (...) - { - DB::tryLogCurrentException(__PRETTY_FUNCTION__); - } - } - - void unlock() - { - if (!locked) - return; - - locked = false; - - if (zookeeper->expired()) - { - LOG_WARNING(log, "Lock is lost, because session was expired. Path: {}, message: {}", lock_path, lock_message); - return; - } - - Coordination::Stat stat; - std::string dummy; - /// NOTE It will throw if session expired after we checked it above - bool result = zookeeper->tryGet(lock_path, dummy, &stat); - - if (result && stat.ephemeralOwner == zookeeper->getClientID()) - zookeeper->remove(lock_path, -1); - else if (result) - throw Exception(ErrorCodes::LOGICAL_ERROR, "Lock is lost, it has another owner. Path: {}, message: {}, owner: {}, our id: {}", - lock_path, lock_message, stat.ephemeralOwner, zookeeper->getClientID()); - else - throw Exception(ErrorCodes::LOGICAL_ERROR, "Lock is lost, node does not exist. Path: {}, message: {}", lock_path, lock_message); - } - - bool tryLock() - { - std::string dummy; - Coordination::Error code = zookeeper->tryCreate(lock_path, lock_message, zkutil::CreateMode::Ephemeral, dummy); - - if (code == Coordination::Error::ZOK) - { - locked = true; - } - else if (code != Coordination::Error::ZNODEEXISTS) - { - throw Coordination::Exception(code); - } - - return locked; - } - -private: - zkutil::ZooKeeperPtr zookeeper; - - std::string lock_path; - std::string lock_message; - Poco::Logger * log; - bool locked = false; - -}; - -std::unique_ptr createSimpleZooKeeperLock( - const zkutil::ZooKeeperPtr & zookeeper, const String & lock_prefix, const String & lock_name, const String & lock_message) -{ - return std::make_unique(zookeeper, lock_prefix, lock_name, lock_message); -} - - DDLWorker::DDLWorker( int pool_size_, const std::string & zk_root_dir, @@ -656,7 +550,7 @@ void DDLWorker::processTask(DDLTaskBase & task, const ZooKeeperPtr & zookeeper) /// We must hold the lock until task execution status is committed to ZooKeeper, /// otherwise another replica may try to execute query again. - std::unique_ptr execute_on_leader_lock; + std::unique_ptr execute_on_leader_lock; /// Step 2: Execute query from the task. if (!task.was_executed) @@ -776,7 +670,7 @@ bool DDLWorker::tryExecuteQueryOnLeaderReplica( const String & rewritten_query, const String & /*node_path*/, const ZooKeeperPtr & zookeeper, - std::unique_ptr & execute_on_leader_lock) + std::unique_ptr & execute_on_leader_lock) { StorageReplicatedMergeTree * replicated_storage = dynamic_cast(storage.get()); diff --git a/src/Interpreters/DDLWorker.h b/src/Interpreters/DDLWorker.h index 0b8b0a4a4d8..dbdf0e94f06 100644 --- a/src/Interpreters/DDLWorker.h +++ b/src/Interpreters/DDLWorker.h @@ -30,6 +30,11 @@ namespace Coordination struct Stat; } +namespace zkutil +{ + class ZooKeeperLock; +} + namespace DB { class ASTAlterQuery; @@ -38,7 +43,6 @@ struct DDLTaskBase; using DDLTaskPtr = std::unique_ptr; using ZooKeeperPtr = std::shared_ptr; class AccessRightsElements; -class ZooKeeperLock; class DDLWorker { @@ -95,7 +99,7 @@ protected: const String & rewritten_query, const String & node_path, const ZooKeeperPtr & zookeeper, - std::unique_ptr & execute_on_leader_lock); + std::unique_ptr & execute_on_leader_lock); bool tryExecuteQuery(const String & query, DDLTaskBase & task, const ZooKeeperPtr & zookeeper); diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index b38a0112116..3e5f7058373 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -5516,6 +5516,8 @@ bool MergeTreeData::moveParts(const CurrentlyMovingPartsTaggerPtr & moving_tagge { LOG_INFO(log, "Got {} parts to move.", moving_tagger->parts_to_move.size()); + const auto settings = getSettings(); + for (const auto & moving_part : moving_tagger->parts_to_move) { Stopwatch stopwatch; @@ -5535,8 +5537,40 @@ bool MergeTreeData::moveParts(const CurrentlyMovingPartsTaggerPtr & moving_tagge try { - cloned_part = parts_mover.clonePart(moving_part); - parts_mover.swapClonedPart(cloned_part); + /// If zero-copy replication enabled than replicas shouldn't try to + /// move parts to another disk simultaneously. For this purpose we + /// use shared lock across replicas. NOTE: it's not 100% reliable, + /// because we are not checking lock while finishing part move. + /// However it's not dangerous at all, we will just have very rare + /// copies of some part. + /// + /// FIXME: this code is related to Replicated merge tree, and not + /// common for ordinary merge tree. So it's a bad design and should + /// be fixed. + auto disk = moving_part.reserved_space->getDisk(); + if (disk->supportZeroCopyReplication() && settings->allow_remote_fs_zero_copy_replication) + { + /// If we acuqired lock than let's try to move. After one + /// replica will actually move the part from disk to some + /// zero-copy storage other replicas will just fetch + /// metainformation. + if (auto lock = tryCreateZeroCopyExclusiveLock(moving_part.part, disk); lock) + { + cloned_part = parts_mover.clonePart(moving_part); + parts_mover.swapClonedPart(cloned_part); + } + else + { + /// Move will be retried but with backoff. + LOG_DEBUG(log, "Move of part {} postponed, because zero copy mode enabled and someone other moving this part right now", moving_part.part->name); + return false; + } + } + else /// Ordinary move as it should be + { + cloned_part = parts_mover.clonePart(moving_part); + parts_mover.swapClonedPart(cloned_part); + } write_part_log({}); } catch (...) diff --git a/src/Storages/MergeTree/MergeTreeData.h b/src/Storages/MergeTree/MergeTreeData.h index f1d0abffc7a..4c58a53f368 100644 --- a/src/Storages/MergeTree/MergeTreeData.h +++ b/src/Storages/MergeTree/MergeTreeData.h @@ -27,6 +27,8 @@ #include #include #include +#include + #include #include @@ -43,6 +45,7 @@ class MergeTreeDataMergerMutator; class MutationCommands; class Context; struct JobAndPool; +struct ZeroCopyLock; /// Auxiliary struct holding information about the future merged or mutated part. struct EmergingPartInfo @@ -1189,6 +1192,10 @@ private: DataPartsVector & duplicate_parts_to_remove, MutableDataPartsVector & parts_from_wal, DataPartsLock & part_lock); + + /// Create zero-copy exclusive lock for part and disk. Useful for coordination of + /// distributed operations which can lead to data duplication. Implemented only in ReplicatedMergeTree. + virtual std::optional tryCreateZeroCopyExclusiveLock(const DataPartPtr &, const DiskPtr &) { return std::nullopt; } }; /// RAII struct to record big parts that are submerging or emerging. diff --git a/src/Storages/MergeTree/ZeroCopyLock.cpp b/src/Storages/MergeTree/ZeroCopyLock.cpp new file mode 100644 index 00000000000..dbb12d0d610 --- /dev/null +++ b/src/Storages/MergeTree/ZeroCopyLock.cpp @@ -0,0 +1,9 @@ +#include "ZeroCopyLock.h" + +namespace DB +{ + ZeroCopyLock::ZeroCopyLock(const zkutil::ZooKeeperPtr & zookeeper, const std::string & lock_path) + : lock(zkutil::createSimpleZooKeeperLock(zookeeper, lock_path, "part_exclusive_lock", "")) + { + } +} diff --git a/src/Storages/MergeTree/ZeroCopyLock.h b/src/Storages/MergeTree/ZeroCopyLock.h new file mode 100644 index 00000000000..f99afc8256f --- /dev/null +++ b/src/Storages/MergeTree/ZeroCopyLock.h @@ -0,0 +1,21 @@ +#pragma once +#include +#include +#include +#include +#include + +namespace DB +{ + +/// Very simple wrapper for zookeeper ephemeral lock. It's better to have it +/// because due to bad abstraction we use in MergeTreeData. +struct ZeroCopyLock +{ + ZeroCopyLock(const zkutil::ZooKeeperPtr & zookeeper, const std::string & lock_path); + + /// Actual lock + std::unique_ptr lock; +}; + +} diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index 7cd646531da..ac52a8f298b 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include @@ -7308,6 +7309,30 @@ Strings StorageReplicatedMergeTree::getZeroCopyPartPath(const MergeTreeSettings } +std::optional StorageReplicatedMergeTree::tryCreateZeroCopyExclusiveLock(const DataPartPtr & part, const DiskPtr & disk) +{ + if (!disk || !disk->supportZeroCopyReplication()) + return std::nullopt; + + zkutil::ZooKeeperPtr zookeeper = tryGetZooKeeper(); + if (!zookeeper) + return std::nullopt; + + String zc_zookeeper_path = getZeroCopyPartPath(*getSettings(), disk->getType(), getTableSharedID(), + part->name, zookeeper_path)[0]; + + /// Just recursively create ancestors for lock with retries + createZeroCopyLockNode(zookeeper, zc_zookeeper_path); + + String zookeeper_node = fs::path(zc_zookeeper_path); + /// Create actual lock + ZeroCopyLock lock(zookeeper, zookeeper_node); + if (lock.lock->tryLock()) + return lock; + else + return std::nullopt; +} + String StorageReplicatedMergeTree::findReplicaHavingPart( const String & part_name, const String & zookeeper_path_, zkutil::ZooKeeper::Ptr zookeeper_ptr) { diff --git a/src/Storages/StorageReplicatedMergeTree.h b/src/Storages/StorageReplicatedMergeTree.h index 2babd01d11d..c91152ca0f3 100644 --- a/src/Storages/StorageReplicatedMergeTree.h +++ b/src/Storages/StorageReplicatedMergeTree.h @@ -758,6 +758,10 @@ private: // Create table id if needed void createTableSharedID(); + /// Create ephemeral lock in zookeeper for part and disk which support zero copy replication. + /// If somebody already holding the lock -- return std::nullopt. + std::optional tryCreateZeroCopyExclusiveLock(const DataPartPtr & part, const DiskPtr & disk) override; + protected: /** If not 'attach', either creates a new table in ZK, or adds a replica to an existing table. */ From 31fb93330e6bb801c6e90ab677ede913e576b44e Mon Sep 17 00:00:00 2001 From: alesapin Date: Mon, 17 Jan 2022 14:54:51 +0300 Subject: [PATCH 22/31] Fix comment --- src/Storages/MergeTree/ZeroCopyLock.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/ZeroCopyLock.h b/src/Storages/MergeTree/ZeroCopyLock.h index f99afc8256f..96709fb01c9 100644 --- a/src/Storages/MergeTree/ZeroCopyLock.h +++ b/src/Storages/MergeTree/ZeroCopyLock.h @@ -9,7 +9,7 @@ namespace DB { /// Very simple wrapper for zookeeper ephemeral lock. It's better to have it -/// because due to bad abstraction we use in MergeTreeData. +/// because due to bad abstraction we use it in MergeTreeData. struct ZeroCopyLock { ZeroCopyLock(const zkutil::ZooKeeperPtr & zookeeper, const std::string & lock_path); From 6d62060e16969fcd31dd180f64fdbab24fa9667c Mon Sep 17 00:00:00 2001 From: Amos Bird Date: Mon, 17 Jan 2022 12:33:47 +0800 Subject: [PATCH 23/31] Build improvement --- CMakeLists.txt | 7 ++++--- base/base/CMakeLists.txt | 7 +++++-- base/base/UUID.h | 4 ++-- base/daemon/BaseDaemon.cpp | 1 + base/loggers/Loggers.cpp | 2 ++ base/loggers/Loggers.h | 5 ++++- base/loggers/OwnSplitChannel.h | 7 ++++++- cmake/linux/toolchain-aarch64.cmake | 6 +++--- cmake/linux/toolchain-ppc64le.cmake | 6 +++--- cmake/linux/toolchain-riscv64.cmake | 6 +++--- cmake/linux/toolchain-x86_64-musl.cmake | 6 +++--- contrib/arrow-cmake/CMakeLists.txt | 21 +++++++++---------- contrib/aws-s3-cmake/CMakeLists.txt | 4 ++-- contrib/boringssl-cmake/CMakeLists.txt | 2 -- contrib/cctz-cmake/CMakeLists.txt | 2 +- contrib/cityhash102/CMakeLists.txt | 4 ++-- contrib/consistent-hashing/CMakeLists.txt | 2 +- contrib/curl-cmake/CMakeLists.txt | 2 +- contrib/lemmagen-c-cmake/CMakeLists.txt | 2 +- contrib/libpq-cmake/CMakeLists.txt | 6 +++--- contrib/libstemmer-c-cmake/CMakeLists.txt | 2 +- contrib/magic-enum-cmake/CMakeLists.txt | 2 +- contrib/snappy-cmake/CMakeLists.txt | 2 +- contrib/unixodbc-cmake/CMakeLists.txt | 2 ++ contrib/wordnet-blast-cmake/CMakeLists.txt | 2 +- contrib/xz-cmake/CMakeLists.txt | 2 +- contrib/zlib-ng-cmake/CMakeLists.txt | 2 +- programs/local/LocalServer.cpp | 1 + src/CMakeLists.txt | 11 ++++++---- src/Common/Config/ConfigHelper.h | 2 ++ src/Common/FieldVisitorConvertToNumber.h | 2 +- src/Interpreters/TreeCNFConverter.cpp | 1 + src/Interpreters/TreeCNFConverter.h | 1 - src/Parsers/CMakeLists.txt | 7 +++++-- .../fuzzers/codegen_fuzzer/CMakeLists.txt | 6 +++--- src/Storages/Distributed/DirectoryMonitor.cpp | 2 +- src/Storages/MergeTree/MergeTask.h | 1 + src/Storages/MergeTree/MergeTreeData.cpp | 11 ++++++++++ src/Storages/MergeTree/MergeTreeData.h | 13 ++---------- .../MergeTree/MergeTreeIndexFullText.cpp | 1 + .../MergeTree/MergeTreeRangeReader.cpp | 1 + src/Storages/StorageMergeTree.h | 1 - src/Storages/System/StorageSystemColumns.cpp | 2 ++ 43 files changed, 104 insertions(+), 75 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 023b1339f04..8aeb7b20c5f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -415,13 +415,14 @@ else () endif () if (WERROR) - add_warning(error) + # Don't pollute CMAKE_CXX_FLAGS with -Werror as it will break some CMake checks. + # Instead, adopt modern cmake usage requirement. + target_compile_options(global-libs INTERFACE "-Werror") endif () # Make this extra-checks for correct library dependencies. if (OS_LINUX AND NOT SANITIZE) - set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,--no-undefined") - set (CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,--no-undefined") + target_link_options(global-libs INTERFACE "-Wl,--no-undefined") endif () # Increase stack size on Musl. We need big stack for our recursive-descend parser. diff --git a/base/base/CMakeLists.txt b/base/base/CMakeLists.txt index e62299f3d06..bc82e502e79 100644 --- a/base/base/CMakeLists.txt +++ b/base/base/CMakeLists.txt @@ -25,8 +25,11 @@ endif () if (USE_DEBUG_HELPERS) get_target_property(MAGIC_ENUM_INCLUDE_DIR magic_enum INTERFACE_INCLUDE_DIRECTORIES) - set (INCLUDE_DEBUG_HELPERS "-I\"${MAGIC_ENUM_INCLUDE_DIR}\" -include \"${ClickHouse_SOURCE_DIR}/base/base/iostream_debug_helpers.h\"") - set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${INCLUDE_DEBUG_HELPERS}") + # CMake generator expression will do insane quoting when it encounters special character like quotes, spaces, etc. + # Prefixing "SHELL:" will force it to use the original text. + set (INCLUDE_DEBUG_HELPERS "SHELL:-I\"${MAGIC_ENUM_INCLUDE_DIR}\" -include \"${ClickHouse_SOURCE_DIR}/base/base/iostream_debug_helpers.h\"") + # Use generator expression as we don't want to pollute CMAKE_CXX_FLAGS, which will interfere with CMake check system. + add_compile_options($<$:${INCLUDE_DEBUG_HELPERS}>) endif () add_library (common ${SRCS}) diff --git a/base/base/UUID.h b/base/base/UUID.h index d6982b55faa..fdc687657ed 100644 --- a/base/base/UUID.h +++ b/base/base/UUID.h @@ -1,7 +1,7 @@ #pragma once -#include "strong_typedef.h" -#include "extended_types.h" +#include +#include namespace DB { diff --git a/base/daemon/BaseDaemon.cpp b/base/daemon/BaseDaemon.cpp index b92a68f104e..3870b65a25c 100644 --- a/base/daemon/BaseDaemon.cpp +++ b/base/daemon/BaseDaemon.cpp @@ -50,6 +50,7 @@ #include #include #include +#include #include #include diff --git a/base/loggers/Loggers.cpp b/base/loggers/Loggers.cpp index 5eb9ef95176..3c350c834e5 100644 --- a/base/loggers/Loggers.cpp +++ b/base/loggers/Loggers.cpp @@ -5,9 +5,11 @@ #include #include "OwnFormattingChannel.h" #include "OwnPatternFormatter.h" +#include "OwnSplitChannel.h" #include #include #include +#include #include namespace fs = std::filesystem; diff --git a/base/loggers/Loggers.h b/base/loggers/Loggers.h index e8afd749534..a859c32fa89 100644 --- a/base/loggers/Loggers.h +++ b/base/loggers/Loggers.h @@ -5,9 +5,12 @@ #include #include #include -#include #include "OwnSplitChannel.h" +namespace DB +{ + class TextLog; +} namespace Poco::Util { diff --git a/base/loggers/OwnSplitChannel.h b/base/loggers/OwnSplitChannel.h index fdc580e65f8..364a6346ede 100644 --- a/base/loggers/OwnSplitChannel.h +++ b/base/loggers/OwnSplitChannel.h @@ -1,11 +1,16 @@ #pragma once #include #include +#include +#include #include #include #include "ExtendedLogChannel.h" -#include +namespace DB +{ + class TextLog; +} namespace DB { diff --git a/cmake/linux/toolchain-aarch64.cmake b/cmake/linux/toolchain-aarch64.cmake index 5b1e41dc871..5db71aecf9a 100644 --- a/cmake/linux/toolchain-aarch64.cmake +++ b/cmake/linux/toolchain-aarch64.cmake @@ -14,9 +14,9 @@ set (TOOLCHAIN_PATH "${CMAKE_CURRENT_LIST_DIR}/../../contrib/sysroot/linux-aarch set (CMAKE_SYSROOT "${TOOLCHAIN_PATH}/aarch64-linux-gnu/libc") -set (CMAKE_C_FLAGS_INIT "${CMAKE_C_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") -set (CMAKE_CXX_FLAGS_INIT "${CMAKE_CXX_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") -set (CMAKE_ASM_FLAGS_INIT "${CMAKE_ASM_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_ASM_FLAGS "${CMAKE_ASM_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") set (HAS_PRE_1970_EXITCODE "0" CACHE STRING "Result from TRY_RUN" FORCE) set (HAS_PRE_1970_EXITCODE__TRYRUN_OUTPUT "" CACHE STRING "Output from TRY_RUN" FORCE) diff --git a/cmake/linux/toolchain-ppc64le.cmake b/cmake/linux/toolchain-ppc64le.cmake index 70915401046..345de208234 100644 --- a/cmake/linux/toolchain-ppc64le.cmake +++ b/cmake/linux/toolchain-ppc64le.cmake @@ -14,9 +14,9 @@ set (TOOLCHAIN_PATH "${CMAKE_CURRENT_LIST_DIR}/../../contrib/sysroot/linux-power set (CMAKE_SYSROOT "${TOOLCHAIN_PATH}/powerpc64le-linux-gnu/libc") -set (CMAKE_C_FLAGS_INIT "${CMAKE_C_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") -set (CMAKE_CXX_FLAGS_INIT "${CMAKE_CXX_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") -set (CMAKE_ASM_FLAGS_INIT "${CMAKE_ASM_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_ASM_FLAGS "${CMAKE_ASM_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") set (HAS_PRE_1970_EXITCODE "0" CACHE STRING "Result from TRY_RUN" FORCE) set (HAS_PRE_1970_EXITCODE__TRYRUN_OUTPUT "" CACHE STRING "Output from TRY_RUN" FORCE) diff --git a/cmake/linux/toolchain-riscv64.cmake b/cmake/linux/toolchain-riscv64.cmake index 7ce6ff43a49..cb0a9482a72 100644 --- a/cmake/linux/toolchain-riscv64.cmake +++ b/cmake/linux/toolchain-riscv64.cmake @@ -14,9 +14,9 @@ set (TOOLCHAIN_PATH "${CMAKE_CURRENT_LIST_DIR}/../../contrib/sysroot/linux-riscv set (CMAKE_SYSROOT "${TOOLCHAIN_PATH}") -set (CMAKE_C_FLAGS_INIT "${CMAKE_C_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") -set (CMAKE_CXX_FLAGS_INIT "${CMAKE_CXX_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") -set (CMAKE_ASM_FLAGS_INIT "${CMAKE_ASM_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_ASM_FLAGS "${CMAKE_ASM_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") set (CMAKE_EXE_LINKER_FLAGS_INIT "-fuse-ld=bfd") set (CMAKE_SHARED_LINKER_FLAGS_INIT "-fuse-ld=bfd") diff --git a/cmake/linux/toolchain-x86_64-musl.cmake b/cmake/linux/toolchain-x86_64-musl.cmake index 14dba6843b9..3eb2077db2b 100644 --- a/cmake/linux/toolchain-x86_64-musl.cmake +++ b/cmake/linux/toolchain-x86_64-musl.cmake @@ -14,9 +14,9 @@ set (TOOLCHAIN_PATH "${CMAKE_CURRENT_LIST_DIR}/../../contrib/sysroot/linux-x86_6 set (CMAKE_SYSROOT "${TOOLCHAIN_PATH}") -set (CMAKE_C_FLAGS_INIT "${CMAKE_C_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") -set (CMAKE_CXX_FLAGS_INIT "${CMAKE_CXX_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") -set (CMAKE_ASM_FLAGS_INIT "${CMAKE_ASM_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") +set (CMAKE_ASM_FLAGS "${CMAKE_ASM_FLAGS} --gcc-toolchain=${TOOLCHAIN_PATH}") set (HAS_PRE_1970_EXITCODE "0" CACHE STRING "Result from TRY_RUN" FORCE) set (HAS_PRE_1970_EXITCODE__TRYRUN_OUTPUT "" CACHE STRING "Output from TRY_RUN" FORCE) diff --git a/contrib/arrow-cmake/CMakeLists.txt b/contrib/arrow-cmake/CMakeLists.txt index b27056d9c55..c768e397e30 100644 --- a/contrib/arrow-cmake/CMakeLists.txt +++ b/contrib/arrow-cmake/CMakeLists.txt @@ -345,7 +345,7 @@ if (USE_INTERNAL_PROTOBUF_LIBRARY) add_dependencies(${ARROW_LIBRARY} protoc) endif () -target_include_directories(${ARROW_LIBRARY} SYSTEM PUBLIC "${ClickHouse_SOURCE_DIR}/contrib/arrow/cpp/src") +target_include_directories(${ARROW_LIBRARY} SYSTEM PUBLIC ${ARROW_SRC_DIR}) target_include_directories(${ARROW_LIBRARY} SYSTEM PUBLIC "${CMAKE_CURRENT_BINARY_DIR}/cpp/src") target_link_libraries(${ARROW_LIBRARY} PRIVATE ${DOUBLE_CONVERSION_LIBRARIES} ${Protobuf_LIBRARY}) target_link_libraries(${ARROW_LIBRARY} PRIVATE lz4) @@ -360,16 +360,15 @@ if (ARROW_WITH_ZSTD) target_include_directories(${ARROW_LIBRARY} SYSTEM BEFORE PRIVATE ${ZLIB_INCLUDE_DIR}) endif () -target_include_directories(${ARROW_LIBRARY} PRIVATE SYSTEM ${ORC_INCLUDE_DIR}) -target_include_directories(${ARROW_LIBRARY} PRIVATE SYSTEM ${ORC_SOURCE_SRC_DIR}) -target_include_directories(${ARROW_LIBRARY} PRIVATE SYSTEM ${ORC_SOURCE_WRAP_DIR}) -target_include_directories(${ARROW_LIBRARY} PRIVATE SYSTEM ${GOOGLE_PROTOBUF_DIR}) -target_include_directories(${ARROW_LIBRARY} PRIVATE SYSTEM ${ORC_BUILD_SRC_DIR}) -target_include_directories(${ARROW_LIBRARY} PRIVATE SYSTEM ${ORC_BUILD_INCLUDE_DIR}) -target_include_directories(${ARROW_LIBRARY} PRIVATE SYSTEM ${ORC_ADDITION_SOURCE_DIR}) -target_include_directories(${ARROW_LIBRARY} PRIVATE SYSTEM ${ARROW_SRC_DIR}) -target_include_directories(${ARROW_LIBRARY} PRIVATE SYSTEM ${FLATBUFFERS_INCLUDE_DIR}) -target_include_directories(${ARROW_LIBRARY} PRIVATE SYSTEM ${HDFS_INCLUDE_DIR}) +target_include_directories(${ARROW_LIBRARY} SYSTEM PRIVATE ${ORC_INCLUDE_DIR}) +target_include_directories(${ARROW_LIBRARY} SYSTEM PRIVATE ${ORC_SOURCE_SRC_DIR}) +target_include_directories(${ARROW_LIBRARY} SYSTEM PRIVATE ${ORC_SOURCE_WRAP_DIR}) +target_include_directories(${ARROW_LIBRARY} SYSTEM PRIVATE ${GOOGLE_PROTOBUF_DIR}) +target_include_directories(${ARROW_LIBRARY} SYSTEM PRIVATE ${ORC_BUILD_SRC_DIR}) +target_include_directories(${ARROW_LIBRARY} SYSTEM PRIVATE ${ORC_BUILD_INCLUDE_DIR}) +target_include_directories(${ARROW_LIBRARY} SYSTEM PRIVATE ${ORC_ADDITION_SOURCE_DIR}) +target_include_directories(${ARROW_LIBRARY} SYSTEM PRIVATE ${FLATBUFFERS_INCLUDE_DIR}) +target_include_directories(${ARROW_LIBRARY} SYSTEM PRIVATE ${HDFS_INCLUDE_DIR}) # === parquet diff --git a/contrib/aws-s3-cmake/CMakeLists.txt b/contrib/aws-s3-cmake/CMakeLists.txt index 723ceac3991..50f9482ef54 100644 --- a/contrib/aws-s3-cmake/CMakeLists.txt +++ b/contrib/aws-s3-cmake/CMakeLists.txt @@ -81,7 +81,7 @@ set(S3_INCLUDES ) add_library(aws_s3_checksums ${AWS_CHECKSUMS_SOURCES}) -target_include_directories(aws_s3_checksums PUBLIC "${AWS_CHECKSUMS_LIBRARY_DIR}/include/") +target_include_directories(aws_s3_checksums SYSTEM PUBLIC "${AWS_CHECKSUMS_LIBRARY_DIR}/include/") if(CMAKE_BUILD_TYPE_UC STREQUAL "DEBUG") target_compile_definitions(aws_s3_checksums PRIVATE "-DDEBUG_BUILD") endif() @@ -93,7 +93,7 @@ add_library(aws_s3 ${S3_UNIFIED_SRC}) target_compile_definitions(aws_s3 PUBLIC "AWS_SDK_VERSION_MAJOR=1") target_compile_definitions(aws_s3 PUBLIC "AWS_SDK_VERSION_MINOR=7") target_compile_definitions(aws_s3 PUBLIC "AWS_SDK_VERSION_PATCH=231") -target_include_directories(aws_s3 PUBLIC ${S3_INCLUDES}) +target_include_directories(aws_s3 SYSTEM PUBLIC ${S3_INCLUDES}) if (OPENSSL_FOUND) target_compile_definitions(aws_s3 PUBLIC -DENABLE_OPENSSL_ENCRYPTION) diff --git a/contrib/boringssl-cmake/CMakeLists.txt b/contrib/boringssl-cmake/CMakeLists.txt index fb32df8cd79..d599351fd5c 100644 --- a/contrib/boringssl-cmake/CMakeLists.txt +++ b/contrib/boringssl-cmake/CMakeLists.txt @@ -131,8 +131,6 @@ if(BUILD_SHARED_LIBS) set(CMAKE_POSITION_INDEPENDENT_CODE TRUE) endif() -include_directories("${BORINGSSL_SOURCE_DIR}/include") - set( CRYPTO_ios_aarch64_SOURCES diff --git a/contrib/cctz-cmake/CMakeLists.txt b/contrib/cctz-cmake/CMakeLists.txt index f0f5c103631..2248ba8b612 100644 --- a/contrib/cctz-cmake/CMakeLists.txt +++ b/contrib/cctz-cmake/CMakeLists.txt @@ -57,7 +57,7 @@ if (NOT EXTERNAL_CCTZ_LIBRARY_FOUND OR NOT EXTERNAL_CCTZ_LIBRARY_WORKS) ) add_library (cctz ${SRCS}) - target_include_directories (cctz PUBLIC "${LIBRARY_DIR}/include") + target_include_directories (cctz SYSTEM PUBLIC "${LIBRARY_DIR}/include") if (OS_FREEBSD) # yes, need linux, because bsd check inside linux in time_zone_libc.cc:24 diff --git a/contrib/cityhash102/CMakeLists.txt b/contrib/cityhash102/CMakeLists.txt index c3f53a8f878..f40a6d2408b 100644 --- a/contrib/cityhash102/CMakeLists.txt +++ b/contrib/cityhash102/CMakeLists.txt @@ -4,5 +4,5 @@ add_library(cityhash include/city.h src/config.h) -target_include_directories(cityhash BEFORE PUBLIC include) -target_include_directories(cityhash PRIVATE src) +target_include_directories(cityhash SYSTEM BEFORE PUBLIC include) +target_include_directories(cityhash SYSTEM PRIVATE src) diff --git a/contrib/consistent-hashing/CMakeLists.txt b/contrib/consistent-hashing/CMakeLists.txt index 4457fe6e2db..7543022df46 100644 --- a/contrib/consistent-hashing/CMakeLists.txt +++ b/contrib/consistent-hashing/CMakeLists.txt @@ -1,2 +1,2 @@ add_library(consistent-hashing consistent_hashing.cpp popcount.cpp) -target_include_directories(consistent-hashing PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) +target_include_directories(consistent-hashing SYSTEM PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) diff --git a/contrib/curl-cmake/CMakeLists.txt b/contrib/curl-cmake/CMakeLists.txt index 1f7449af914..63ac8da24b1 100644 --- a/contrib/curl-cmake/CMakeLists.txt +++ b/contrib/curl-cmake/CMakeLists.txt @@ -153,7 +153,7 @@ target_compile_definitions (curl PRIVATE libcurl_EXPORTS OS="${CMAKE_SYSTEM_NAME}" ) -target_include_directories (curl PUBLIC +target_include_directories (curl SYSTEM PUBLIC "${LIBRARY_DIR}/include" "${LIBRARY_DIR}/lib" . # curl_config.h diff --git a/contrib/lemmagen-c-cmake/CMakeLists.txt b/contrib/lemmagen-c-cmake/CMakeLists.txt index b5b92b774e1..3a067916bf6 100644 --- a/contrib/lemmagen-c-cmake/CMakeLists.txt +++ b/contrib/lemmagen-c-cmake/CMakeLists.txt @@ -6,4 +6,4 @@ set(SRCS ) add_library(lemmagen STATIC ${SRCS}) -target_include_directories(lemmagen PUBLIC "${LEMMAGEN_INCLUDE_DIR}") +target_include_directories(lemmagen SYSTEM PUBLIC "${LEMMAGEN_INCLUDE_DIR}") diff --git a/contrib/libpq-cmake/CMakeLists.txt b/contrib/libpq-cmake/CMakeLists.txt index 4f6a1554d10..2d2e0c428fe 100644 --- a/contrib/libpq-cmake/CMakeLists.txt +++ b/contrib/libpq-cmake/CMakeLists.txt @@ -55,8 +55,8 @@ set(SRCS add_library(libpq ${SRCS}) -target_include_directories (libpq PUBLIC ${LIBPQ_SOURCE_DIR}) -target_include_directories (libpq PUBLIC "${LIBPQ_SOURCE_DIR}/include") -target_include_directories (libpq PRIVATE "${LIBPQ_SOURCE_DIR}/configs") +target_include_directories (libpq SYSTEM PUBLIC ${LIBPQ_SOURCE_DIR}) +target_include_directories (libpq SYSTEM PUBLIC "${LIBPQ_SOURCE_DIR}/include") +target_include_directories (libpq SYSTEM PRIVATE "${LIBPQ_SOURCE_DIR}/configs") target_link_libraries (libpq PRIVATE ssl) diff --git a/contrib/libstemmer-c-cmake/CMakeLists.txt b/contrib/libstemmer-c-cmake/CMakeLists.txt index 2d38e5f3612..b5cd59e4633 100644 --- a/contrib/libstemmer-c-cmake/CMakeLists.txt +++ b/contrib/libstemmer-c-cmake/CMakeLists.txt @@ -28,4 +28,4 @@ endforeach () # all the sources parsed. Now just add the lib add_library ( stemmer STATIC ${_SOURCES} ${_HEADERS} ) -target_include_directories (stemmer PUBLIC "${STEMMER_INCLUDE_DIR}") +target_include_directories (stemmer SYSTEM PUBLIC "${STEMMER_INCLUDE_DIR}") diff --git a/contrib/magic-enum-cmake/CMakeLists.txt b/contrib/magic-enum-cmake/CMakeLists.txt index 142f9c7c755..fae2c9c2d05 100644 --- a/contrib/magic-enum-cmake/CMakeLists.txt +++ b/contrib/magic-enum-cmake/CMakeLists.txt @@ -1,3 +1,3 @@ set (LIBRARY_DIR "${ClickHouse_SOURCE_DIR}/contrib/magic_enum") add_library (magic_enum INTERFACE) -target_include_directories(magic_enum INTERFACE ${LIBRARY_DIR}/include) +target_include_directories(magic_enum SYSTEM INTERFACE ${LIBRARY_DIR}/include) diff --git a/contrib/snappy-cmake/CMakeLists.txt b/contrib/snappy-cmake/CMakeLists.txt index 750060db93f..289f8908436 100644 --- a/contrib/snappy-cmake/CMakeLists.txt +++ b/contrib/snappy-cmake/CMakeLists.txt @@ -40,5 +40,5 @@ target_sources(snappy "${SOURCE_DIR}/snappy-stubs-internal.cc" "${SOURCE_DIR}/snappy.cc") -target_include_directories(snappy PUBLIC ${SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR}) +target_include_directories(snappy SYSTEM PUBLIC ${SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR}) target_compile_definitions(snappy PRIVATE -DHAVE_CONFIG_H) diff --git a/contrib/unixodbc-cmake/CMakeLists.txt b/contrib/unixodbc-cmake/CMakeLists.txt index c154533739c..e03f6313a31 100644 --- a/contrib/unixodbc-cmake/CMakeLists.txt +++ b/contrib/unixodbc-cmake/CMakeLists.txt @@ -23,6 +23,7 @@ set (SRCS_LTDL add_library (ltdl ${SRCS_LTDL}) target_include_directories(ltdl + SYSTEM PRIVATE linux_x86_64/libltdl PUBLIC @@ -276,6 +277,7 @@ target_link_libraries (unixodbc PRIVATE ltdl) # SYSTEM_FILE_PATH was changed to /etc target_include_directories (unixodbc + SYSTEM PRIVATE linux_x86_64/private PUBLIC diff --git a/contrib/wordnet-blast-cmake/CMakeLists.txt b/contrib/wordnet-blast-cmake/CMakeLists.txt index 8d59c312664..37e4e9825ca 100644 --- a/contrib/wordnet-blast-cmake/CMakeLists.txt +++ b/contrib/wordnet-blast-cmake/CMakeLists.txt @@ -10,4 +10,4 @@ add_library(wnb ${SRCS}) target_link_libraries(wnb PRIVATE boost::headers_only boost::graph) -target_include_directories(wnb PUBLIC "${LIBRARY_DIR}") \ No newline at end of file +target_include_directories(wnb SYSTEM PUBLIC "${LIBRARY_DIR}") diff --git a/contrib/xz-cmake/CMakeLists.txt b/contrib/xz-cmake/CMakeLists.txt index af8ebb0ebc1..5d70199413f 100644 --- a/contrib/xz-cmake/CMakeLists.txt +++ b/contrib/xz-cmake/CMakeLists.txt @@ -241,7 +241,7 @@ add_library(liblzma ${SRC_DIR}/src/liblzma/simple/x86.c ) -target_include_directories(liblzma PRIVATE +target_include_directories(liblzma SYSTEM PUBLIC ${SRC_DIR}/src/liblzma/api ${SRC_DIR}/src/liblzma/common ${SRC_DIR}/src/liblzma/check diff --git a/contrib/zlib-ng-cmake/CMakeLists.txt b/contrib/zlib-ng-cmake/CMakeLists.txt index 96c29a09a28..bf5bc0d7f1c 100644 --- a/contrib/zlib-ng-cmake/CMakeLists.txt +++ b/contrib/zlib-ng-cmake/CMakeLists.txt @@ -158,4 +158,4 @@ if (ARCH_AMD64 OR ARCH_AARCH64) target_compile_definitions (zlib PUBLIC X86_64 UNALIGNED_OK) endif () -target_include_directories(zlib PUBLIC ${SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR}) +target_include_directories(zlib SYSTEM PUBLIC ${SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR}) diff --git a/programs/local/LocalServer.cpp b/programs/local/LocalServer.cpp index aa4747636c9..a294857ace8 100644 --- a/programs/local/LocalServer.cpp +++ b/programs/local/LocalServer.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 62767e02a64..41717f348a2 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -29,8 +29,11 @@ configure_file (Core/config_core.h.in "${CMAKE_CURRENT_BINARY_DIR}/Core/include/ if (USE_DEBUG_HELPERS) get_target_property(MAGIC_ENUM_INCLUDE_DIR magic_enum INTERFACE_INCLUDE_DIRECTORIES) - set (INCLUDE_DEBUG_HELPERS "-I\"${ClickHouse_SOURCE_DIR}/base\" -I\"${MAGIC_ENUM_INCLUDE_DIR}\" -include \"${ClickHouse_SOURCE_DIR}/src/Core/iostream_debug_helpers.h\"") - set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${INCLUDE_DEBUG_HELPERS}") + # CMake generator expression will do insane quoting when it encounters special character like quotes, spaces, etc. + # Prefixing "SHELL:" will force it to use the original text. + set (INCLUDE_DEBUG_HELPERS "SHELL:-I\"${ClickHouse_SOURCE_DIR}/base\" -I\"${MAGIC_ENUM_INCLUDE_DIR}\" -include \"${ClickHouse_SOURCE_DIR}/src/Core/iostream_debug_helpers.h\"") + # Use generator expression as we don't want to pollute CMAKE_CXX_FLAGS, which will interfere with CMake check system. + add_compile_options($<$:${INCLUDE_DEBUG_HELPERS}>) endif () if (COMPILER_GCC) @@ -385,9 +388,9 @@ dbms_target_link_libraries ( target_include_directories(clickhouse_common_io PUBLIC "${CMAKE_CURRENT_BINARY_DIR}/Core/include") # uses some includes from core dbms_target_include_directories(PUBLIC "${CMAKE_CURRENT_BINARY_DIR}/Core/include") -target_include_directories(clickhouse_common_io BEFORE PUBLIC ${PDQSORT_INCLUDE_DIR}) +target_include_directories(clickhouse_common_io SYSTEM BEFORE PUBLIC ${PDQSORT_INCLUDE_DIR}) dbms_target_include_directories(SYSTEM BEFORE PUBLIC ${PDQSORT_INCLUDE_DIR}) -target_include_directories(clickhouse_common_io BEFORE PUBLIC ${MINISELECT_INCLUDE_DIR}) +target_include_directories(clickhouse_common_io SYSTEM BEFORE PUBLIC ${MINISELECT_INCLUDE_DIR}) dbms_target_include_directories(SYSTEM BEFORE PUBLIC ${MINISELECT_INCLUDE_DIR}) if (ZSTD_LIBRARY) diff --git a/src/Common/Config/ConfigHelper.h b/src/Common/Config/ConfigHelper.h index 62271bbaf0a..b168be6ca9c 100644 --- a/src/Common/Config/ConfigHelper.h +++ b/src/Common/Config/ConfigHelper.h @@ -1,5 +1,7 @@ #pragma once +#include + namespace Poco { namespace Util diff --git a/src/Common/FieldVisitorConvertToNumber.h b/src/Common/FieldVisitorConvertToNumber.h index 6db65fe63ea..025fd667609 100644 --- a/src/Common/FieldVisitorConvertToNumber.h +++ b/src/Common/FieldVisitorConvertToNumber.h @@ -64,7 +64,7 @@ public: /// Conversion of infinite values to integer is undefined. throw Exception("Cannot convert infinite value to integer type", ErrorCodes::CANNOT_CONVERT_TYPE); } - else if (x > std::numeric_limits::max() || x < std::numeric_limits::lowest()) + else if (x > Float64(std::numeric_limits::max()) || x < Float64(std::numeric_limits::lowest())) { throw Exception("Cannot convert out of range floating point value to integer type", ErrorCodes::CANNOT_CONVERT_TYPE); } diff --git a/src/Interpreters/TreeCNFConverter.cpp b/src/Interpreters/TreeCNFConverter.cpp index a55bacee5fa..e8e6ce09804 100644 --- a/src/Interpreters/TreeCNFConverter.cpp +++ b/src/Interpreters/TreeCNFConverter.cpp @@ -3,6 +3,7 @@ #include #include #include +#include namespace DB diff --git a/src/Interpreters/TreeCNFConverter.h b/src/Interpreters/TreeCNFConverter.h index 7ea5fa54680..0c090c8d56b 100644 --- a/src/Interpreters/TreeCNFConverter.h +++ b/src/Interpreters/TreeCNFConverter.h @@ -3,7 +3,6 @@ #include #include #include -#include #include #include diff --git a/src/Parsers/CMakeLists.txt b/src/Parsers/CMakeLists.txt index d945e63589a..b2c31366929 100644 --- a/src/Parsers/CMakeLists.txt +++ b/src/Parsers/CMakeLists.txt @@ -7,8 +7,11 @@ add_library(clickhouse_parsers ${clickhouse_parsers_headers} ${clickhouse_parser target_link_libraries(clickhouse_parsers PUBLIC clickhouse_common_io clickhouse_common_access) if (USE_DEBUG_HELPERS) - set (INCLUDE_DEBUG_HELPERS "-I\"${ClickHouse_SOURCE_DIR}/base\" -include \"${ClickHouse_SOURCE_DIR}/src/Parsers/iostream_debug_helpers.h\"") - set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${INCLUDE_DEBUG_HELPERS}") + # CMake generator expression will do insane quoting when it encounters special character like quotes, spaces, etc. + # Prefixing "SHELL:" will force it to use the original text. + set (INCLUDE_DEBUG_HELPERS "SHELL:-I\"${ClickHouse_SOURCE_DIR}/base\" -include \"${ClickHouse_SOURCE_DIR}/src/Parsers/iostream_debug_helpers.h\"") + # Use generator expression as we don't want to pollute CMAKE_CXX_FLAGS, which will interfere with CMake check system. + add_compile_options($<$:${INCLUDE_DEBUG_HELPERS}>) endif () if(ENABLE_EXAMPLES) diff --git a/src/Parsers/fuzzers/codegen_fuzzer/CMakeLists.txt b/src/Parsers/fuzzers/codegen_fuzzer/CMakeLists.txt index 9cb7e79c42c..99bbd453144 100644 --- a/src/Parsers/fuzzers/codegen_fuzzer/CMakeLists.txt +++ b/src/Parsers/fuzzers/codegen_fuzzer/CMakeLists.txt @@ -40,7 +40,7 @@ add_executable(codegen_select_fuzzer ${FUZZER_SRCS}) set_source_files_properties("${PROTO_SRCS}" "out.cpp" PROPERTIES COMPILE_FLAGS "-Wno-reserved-identifier") -target_include_directories(codegen_select_fuzzer BEFORE PRIVATE "${Protobuf_INCLUDE_DIR}" "${CMAKE_CURRENT_BINARY_DIR}") -target_include_directories(codegen_select_fuzzer BEFORE PRIVATE "${LibProtobufMutator_SOURCE_DIR}") -target_include_directories(codegen_select_fuzzer BEFORE PRIVATE "${LibProtobufMutator_SOURCE_DIR}/src") +target_include_directories(codegen_select_fuzzer SYSTEM BEFORE PRIVATE "${Protobuf_INCLUDE_DIR}" "${CMAKE_CURRENT_BINARY_DIR}") +target_include_directories(codegen_select_fuzzer SYSTEM BEFORE PRIVATE "${LibProtobufMutator_SOURCE_DIR}") +target_include_directories(codegen_select_fuzzer SYSTEM BEFORE PRIVATE "${LibProtobufMutator_SOURCE_DIR}/src") target_link_libraries(codegen_select_fuzzer PRIVATE protobuf-mutator ${Protobuf_LIBRARY} ${Protobuf_PROTOC_LIBRARY} dbms ${LIB_FUZZING_ENGINE}) diff --git a/src/Storages/Distributed/DirectoryMonitor.cpp b/src/Storages/Distributed/DirectoryMonitor.cpp index a5fa8ab366b..0c41cf71386 100644 --- a/src/Storages/Distributed/DirectoryMonitor.cpp +++ b/src/Storages/Distributed/DirectoryMonitor.cpp @@ -332,7 +332,7 @@ namespace uint64_t doubleToUInt64(double d) { - if (d >= std::numeric_limits::max()) + if (d >= double(std::numeric_limits::max())) return std::numeric_limits::max(); return static_cast(d); } diff --git a/src/Storages/MergeTree/MergeTask.h b/src/Storages/MergeTree/MergeTask.h index 80c8e7165f8..d8e48c1794a 100644 --- a/src/Storages/MergeTree/MergeTask.h +++ b/src/Storages/MergeTree/MergeTask.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index b38a0112116..bcb393f7f68 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -340,6 +341,16 @@ StoragePolicyPtr MergeTreeData::getStoragePolicy() const return getContext()->getStoragePolicy(getSettings()->storage_policy); } +bool MergeTreeData::supportsFinal() const +{ + return merging_params.mode == MergingParams::Collapsing + || merging_params.mode == MergingParams::Summing + || merging_params.mode == MergingParams::Aggregating + || merging_params.mode == MergingParams::Replacing + || merging_params.mode == MergingParams::Graphite + || merging_params.mode == MergingParams::VersionedCollapsing; +} + static void checkKeyExpression(const ExpressionActions & expr, const Block & sample_block, const String & key_name, bool allow_nullable_key) { if (expr.hasArrayJoin()) diff --git a/src/Storages/MergeTree/MergeTreeData.h b/src/Storages/MergeTree/MergeTreeData.h index f1d0abffc7a..cbd24f12cb5 100644 --- a/src/Storages/MergeTree/MergeTreeData.h +++ b/src/Storages/MergeTree/MergeTreeData.h @@ -24,7 +24,6 @@ #include #include #include -#include #include #include @@ -408,15 +407,7 @@ public: bool supportsPrewhere() const override { return true; } - bool supportsFinal() const override - { - return merging_params.mode == MergingParams::Collapsing - || merging_params.mode == MergingParams::Summing - || merging_params.mode == MergingParams::Aggregating - || merging_params.mode == MergingParams::Replacing - || merging_params.mode == MergingParams::Graphite - || merging_params.mode == MergingParams::VersionedCollapsing; - } + bool supportsFinal() const override; bool supportsSubcolumns() const override { return true; } @@ -447,7 +438,7 @@ public: static void validateDetachedPartName(const String & name); - void dropDetached(const ASTPtr & partition, bool part, ContextPtr context); + void dropDetached(const ASTPtr & partition, bool part, ContextPtr local_context); MutableDataPartsVector tryLoadPartsToAttach(const ASTPtr & partition, bool attach_part, ContextPtr context, PartsTemporaryRename & renamed_parts); diff --git a/src/Storages/MergeTree/MergeTreeIndexFullText.cpp b/src/Storages/MergeTree/MergeTreeIndexFullText.cpp index 9332f4fd442..f87584c9cd6 100644 --- a/src/Storages/MergeTree/MergeTreeIndexFullText.cpp +++ b/src/Storages/MergeTree/MergeTreeIndexFullText.cpp @@ -1,5 +1,6 @@ #include +#include #include #include #include diff --git a/src/Storages/MergeTree/MergeTreeRangeReader.cpp b/src/Storages/MergeTree/MergeTreeRangeReader.cpp index 8481cee0f86..98b8de6a0ea 100644 --- a/src/Storages/MergeTree/MergeTreeRangeReader.cpp +++ b/src/Storages/MergeTree/MergeTreeRangeReader.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include diff --git a/src/Storages/StorageMergeTree.h b/src/Storages/StorageMergeTree.h index ee99b412f59..af7e112462f 100644 --- a/src/Storages/StorageMergeTree.h +++ b/src/Storages/StorageMergeTree.h @@ -141,7 +141,6 @@ private: std::atomic shutdown_called {false}; -private: void loadMutations(); /// Load and initialize deduplication logs. Even if deduplication setting diff --git a/src/Storages/System/StorageSystemColumns.cpp b/src/Storages/System/StorageSystemColumns.cpp index 25634ad3aa4..d847c00846c 100644 --- a/src/Storages/System/StorageSystemColumns.cpp +++ b/src/Storages/System/StorageSystemColumns.cpp @@ -3,10 +3,12 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include From 3ca17afa0097aa9eec4b1d44b10f4b8bb24b3a72 Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Mon, 17 Jan 2022 20:35:52 +0000 Subject: [PATCH 24/31] Fixed build --- src/Dictionaries/IDictionary.h | 2 +- src/Storages/StorageDictionary.cpp | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Dictionaries/IDictionary.h b/src/Dictionaries/IDictionary.h index cbfb3133970..3bf55504fc7 100644 --- a/src/Dictionaries/IDictionary.h +++ b/src/Dictionaries/IDictionary.h @@ -58,7 +58,7 @@ public: std::string getFullName() const { std::lock_guard lock{name_mutex}; - return dictionary_id.getNameForLogs(); + return dictionary_id.getInternalDictionaryName(); } StorageID getDictionaryID() const diff --git a/src/Storages/StorageDictionary.cpp b/src/Storages/StorageDictionary.cpp index c531edaa8d3..da8c5f115b2 100644 --- a/src/Storages/StorageDictionary.cpp +++ b/src/Storages/StorageDictionary.cpp @@ -222,8 +222,8 @@ void StorageDictionary::renameInMemory(const StorageID & new_table_id) /// It's DDL dictionary, need to update configuration and reload - [[maybe_unused]] bool move_to_atomic = old_table_id.uuid == UUIDHelpers::Nil && new_table_id.uuid != UUIDHelpers::Nil; - [[maybe_unused]] bool move_to_ordinary = old_table_id.uuid != UUIDHelpers::Nil && new_table_id.uuid == UUIDHelpers::Nil; + bool move_to_atomic = old_table_id.uuid == UUIDHelpers::Nil && new_table_id.uuid != UUIDHelpers::Nil; + bool move_to_ordinary = old_table_id.uuid != UUIDHelpers::Nil && new_table_id.uuid == UUIDHelpers::Nil; assert(old_table_id.uuid == new_table_id.uuid || move_to_atomic || move_to_ordinary); { @@ -231,6 +231,10 @@ void StorageDictionary::renameInMemory(const StorageID & new_table_id) configuration->setString("dictionary.database", new_table_id.database_name); configuration->setString("dictionary.name", new_table_id.table_name); + if (move_to_atomic) + configuration->setString("dictionary.uuid", toString(new_table_id.uuid)); + else if (move_to_ordinary) + configuration->remove("dictionary.uuid"); } /// Dictionary is moving between databases of different engines or is renaming inside Ordinary database From fc38cffd33487e73d76c369692d364dbbc15ae66 Mon Sep 17 00:00:00 2001 From: cnmade Date: Tue, 18 Jan 2022 11:06:41 +0800 Subject: [PATCH 25/31] Translate zh/sql/statements index and use document --- docs/en/sql-reference/statements/use.md | 8 +++--- docs/zh/sql-reference/statements/index.md | 30 ++++++++++++++++++++--- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/docs/en/sql-reference/statements/use.md b/docs/en/sql-reference/statements/use.md index 841c23d333d..dae499fb7ab 100644 --- a/docs/en/sql-reference/statements/use.md +++ b/docs/en/sql-reference/statements/use.md @@ -3,14 +3,14 @@ toc_priority: 53 toc_title: USE --- -# USE Statement {#use} +# USE 语句 {#use} ``` sql USE db ``` -Lets you set the current database for the session. +用于设置会话的当前数据库。 -The current database is used for searching for tables if the database is not explicitly defined in the query with a dot before the table name. +如果数据库未在查询中显式定义，则当前数据库用于搜索表，并在表名前加上一个点。 -This query can’t be made when using the HTTP protocol, since there is no concept of a session. +使用 HTTP 协议时无法进行此查询，因为没有会话的概念。 diff --git a/docs/zh/sql-reference/statements/index.md b/docs/zh/sql-reference/statements/index.md index ab080584c66..385639fde0b 100644 --- a/docs/zh/sql-reference/statements/index.md +++ b/docs/zh/sql-reference/statements/index.md @@ -1,8 +1,32 @@ --- -machine_translated: true -machine_translated_rev: 72537a2d527c63c07aa5d2361a8829f3895cf2bd -toc_folder_title: "\u8BED\u53E5" +toc_folder_title: SQL 语句 +toc_hidden: true toc_priority: 31 --- +# ClickHouse SQL 语句 {#clickhouse-sql-statements} +语句表示可以使用 SQL 查询执行的各种操作。每种类型的语句都有自己的语法和用法详细信息，这些语法和用法详细信息单独描述如下所示: + +- [SELECT](../../sql-reference/statements/select/index.md) +- [INSERT INTO](../../sql-reference/statements/insert-into.md) +- [CREATE](../../sql-reference/statements/create/index.md) +- [ALTER](../../sql-reference/statements/alter/index.md) +- [SYSTEM](../../sql-reference/statements/system.md) +- [SHOW](../../sql-reference/statements/show.md) +- [GRANT](../../sql-reference/statements/grant.md) +- [REVOKE](../../sql-reference/statements/revoke.md) +- [ATTACH](../../sql-reference/statements/attach.md) +- [CHECK TABLE](../../sql-reference/statements/check-table.md) +- [DESCRIBE TABLE](../../sql-reference/statements/describe-table.md) +- [DETACH](../../sql-reference/statements/detach.md) +- [DROP](../../sql-reference/statements/drop.md) +- [EXISTS](../../sql-reference/statements/exists.md) +- [KILL](../../sql-reference/statements/kill.md) +- [OPTIMIZE](../../sql-reference/statements/optimize.md) +- [RENAME](../../sql-reference/statements/rename.md) +- [SET](../../sql-reference/statements/set.md) +- [SET ROLE](../../sql-reference/statements/set-role.md) +- [TRUNCATE](../../sql-reference/statements/truncate.md) +- [USE](../../sql-reference/statements/use.md) +- [EXPLAIN](../../sql-reference/statements/explain.md) From 40435293004b983d0d4523a533dae31626aa14be Mon Sep 17 00:00:00 2001 From: cnmade Date: Tue, 18 Jan 2022 11:40:29 +0800 Subject: [PATCH 26/31] =?UTF-8?q?Translate=20zh/sql/statements=EF=BC=9Arev?= =?UTF-8?q?=20translate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/en/sql-reference/statements/use.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/sql-reference/statements/use.md b/docs/en/sql-reference/statements/use.md index dae499fb7ab..41cba58bb9d 100644 --- a/docs/en/sql-reference/statements/use.md +++ b/docs/en/sql-reference/statements/use.md @@ -11,6 +11,6 @@ USE db 用于设置会话的当前数据库。 -如果数据库未在查询中显式定义，则当前数据库用于搜索表，并在表名前加上一个点。 +如果查询语句中没有在表名前面以加点的方式指明数据库名， 则用当前数据库进行搜索。 使用 HTTP 协议时无法进行此查询，因为没有会话的概念。 From eff85d3352dac091bc7d04e7a143a107cdedf4e5 Mon Sep 17 00:00:00 2001 From: alesapin Date: Tue, 18 Jan 2022 11:27:01 +0300 Subject: [PATCH 27/31] Review fixes --- src/Common/ZooKeeper/ZooKeeperLock.cpp | 6 ++---- src/Storages/MergeTree/MergeTreeData.cpp | 8 +++++--- src/Storages/StorageReplicatedMergeTree.cpp | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/Common/ZooKeeper/ZooKeeperLock.cpp b/src/Common/ZooKeeper/ZooKeeperLock.cpp index 56be8d59cb2..1200dcdb533 100644 --- a/src/Common/ZooKeeper/ZooKeeperLock.cpp +++ b/src/Common/ZooKeeper/ZooKeeperLock.cpp @@ -55,9 +55,8 @@ void ZooKeeperLock::unlock() } Coordination::Stat stat; - std::string dummy; /// NOTE It will throw if session expired after we checked it above - bool result = zookeeper->tryGet(lock_path, dummy, &stat); + bool result = zookeeper->exists(lock_path, &stat); if (result && stat.ephemeralOwner == zookeeper->getClientID()) zookeeper->remove(lock_path, -1); @@ -70,8 +69,7 @@ void ZooKeeperLock::unlock() bool ZooKeeperLock::tryLock() { - std::string dummy; - Coordination::Error code = zookeeper->tryCreate(lock_path, lock_message, zkutil::CreateMode::Ephemeral, dummy); + Coordination::Error code = zookeeper->tryCreate(lock_path, lock_message, zkutil::CreateMode::Ephemeral); if (code == Coordination::Error::ZOK) { diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index 3e5f7058373..54705a3c405 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -5518,6 +5518,7 @@ bool MergeTreeData::moveParts(const CurrentlyMovingPartsTaggerPtr & moving_tagge const auto settings = getSettings(); + bool result = true; for (const auto & moving_part : moving_tagger->parts_to_move) { Stopwatch stopwatch; @@ -5548,7 +5549,7 @@ bool MergeTreeData::moveParts(const CurrentlyMovingPartsTaggerPtr & moving_tagge /// common for ordinary merge tree. So it's a bad design and should /// be fixed. auto disk = moving_part.reserved_space->getDisk(); - if (disk->supportZeroCopyReplication() && settings->allow_remote_fs_zero_copy_replication) + if (supportsReplication() && disk->supportZeroCopyReplication() && settings->allow_remote_fs_zero_copy_replication) { /// If we acuqired lock than let's try to move. After one /// replica will actually move the part from disk to some @@ -5563,7 +5564,8 @@ bool MergeTreeData::moveParts(const CurrentlyMovingPartsTaggerPtr & moving_tagge { /// Move will be retried but with backoff. LOG_DEBUG(log, "Move of part {} postponed, because zero copy mode enabled and someone other moving this part right now", moving_part.part->name); - return false; + result = false; + continue; } } else /// Ordinary move as it should be @@ -5582,7 +5584,7 @@ bool MergeTreeData::moveParts(const CurrentlyMovingPartsTaggerPtr & moving_tagge throw; } } - return true; + return result; } bool MergeTreeData::partsContainSameProjections(const DataPartPtr & left, const DataPartPtr & right) diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index ac52a8f298b..8d2e15357a0 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -7321,12 +7321,12 @@ std::optional StorageReplicatedMergeTree::tryCreateZeroCopyExclusi String zc_zookeeper_path = getZeroCopyPartPath(*getSettings(), disk->getType(), getTableSharedID(), part->name, zookeeper_path)[0]; - /// Just recursively create ancestors for lock with retries - createZeroCopyLockNode(zookeeper, zc_zookeeper_path); + /// Just recursively create ancestors for lock + zookeeper->createAncestors(zc_zookeeper_path); + zookeeper->createIfNotExists(zc_zookeeper_path, ""); - String zookeeper_node = fs::path(zc_zookeeper_path); /// Create actual lock - ZeroCopyLock lock(zookeeper, zookeeper_node); + ZeroCopyLock lock(zookeeper, zc_zookeeper_path); if (lock.lock->tryLock()) return lock; else From 98d036e2c069a186fb214a99b89b342883cf7623 Mon Sep 17 00:00:00 2001 From: vdimir Date: Tue, 18 Jan 2022 16:27:34 +0300 Subject: [PATCH 28/31] Add test, close issue #31009 --- .../0_stateless/02177_issue_31009.reference | 0 .../queries/0_stateless/02177_issue_31009.sql | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 tests/queries/0_stateless/02177_issue_31009.reference create mode 100644 tests/queries/0_stateless/02177_issue_31009.sql diff --git a/tests/queries/0_stateless/02177_issue_31009.reference b/tests/queries/0_stateless/02177_issue_31009.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/02177_issue_31009.sql b/tests/queries/0_stateless/02177_issue_31009.sql new file mode 100644 index 00000000000..f4a65e3a3a0 --- /dev/null +++ b/tests/queries/0_stateless/02177_issue_31009.sql @@ -0,0 +1,19 @@ +-- Tags: long + +CREATE TABLE left ( key UInt32, value String ) ENGINE = MergeTree ORDER BY key; +CREATE TABLE right ( key UInt32, value String ) ENGINE = MergeTree ORDER BY tuple(); + +INSERT INTO left SELECT number, toString(number) FROM numbers(25367182); +INSERT INTO right SELECT number, toString(number) FROM numbers(23124707); + +SET join_algorithm = 'partial_merge'; + +SELECT key, count(1) AS cnt +FROM ( + SELECT data.key + FROM ( SELECT key FROM left AS s ) AS data + LEFT JOIN ( SELECT key FROM right GROUP BY key ) AS promo ON promo.key = data.key +) GROUP BY key HAVING count(1) > 1; + +DROP TABLE IF EXISTS left; +DROP TABLE IF EXISTS right; From 9d87ca003fea52a720c766157c53736cd10f19ae Mon Sep 17 00:00:00 2001 From: Denny Crane Date: Tue, 18 Jan 2022 09:59:46 -0400 Subject: [PATCH 29/31] uniqState / uniqHLL12State / uniqCombinedState / uniqCombined64State Backward Incompatible from 21.6 --- docs/en/whats-new/changelog/2021.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/en/whats-new/changelog/2021.md b/docs/en/whats-new/changelog/2021.md index 1c3ceca08a7..2e81d981990 100644 --- a/docs/en/whats-new/changelog/2021.md +++ b/docs/en/whats-new/changelog/2021.md @@ -1018,6 +1018,9 @@ toc_title: '2021' ### ClickHouse release 21.6, 2021-06-05 +#### Backward Incompatible Change +* uniqState / uniqHLL12State / uniqCombinedState / uniqCombined64State produce incompatible states with `UUID` type. [#33607](https://github.com/ClickHouse/ClickHouse/issues/33607). + #### Upgrade Notes * `zstd` compression library is updated to v1.5.0. You may get messages about "checksum does not match" in replication. These messages are expected due to update of compression algorithm and you can ignore them. These messages are informational and do not indicate any kinds of undesired behaviour. From 105b50eccf84952e39664abf73785655b82e075a Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Tue, 18 Jan 2022 17:34:05 +0100 Subject: [PATCH 30/31] Fix release_branches workflow for some cases --- .github/workflows/release_branches.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/release_branches.yml b/.github/workflows/release_branches.yml index dc60c9df3a6..eb5dcc45327 100644 --- a/.github/workflows/release_branches.yml +++ b/.github/workflows/release_branches.yml @@ -7,10 +7,10 @@ env: on: # yamllint disable-line rule:truthy push: branches: - - '21.[1-9][1-9]' - - '22.[1-9][1-9]' - - '23.[1-9][1-9]' - - '24.[1-9][1-9]' + # 22.1 and 22.10 + - '2[1-9].[1-9][0-9]' + - '2[1-9].[1-9]' + jobs: DockerHubPushAarch64: runs-on: [self-hosted, func-tester-aarch64] From b1a35c934fad66dce256c9de39b685a94d2b4f9a Mon Sep 17 00:00:00 2001 From: Yohann Jardin Date: Tue, 18 Jan 2022 20:38:30 +0100 Subject: [PATCH 31/31] fix/doc: close code highlight before tuplePlus --- docs/en/sql-reference/functions/tuple-functions.md | 1 + docs/ru/sql-reference/functions/tuple-functions.md | 1 + 2 files changed, 2 insertions(+) diff --git a/docs/en/sql-reference/functions/tuple-functions.md b/docs/en/sql-reference/functions/tuple-functions.md index 8d06e8ea1cc..0ddd628d9c2 100644 --- a/docs/en/sql-reference/functions/tuple-functions.md +++ b/docs/en/sql-reference/functions/tuple-functions.md @@ -240,6 +240,7 @@ Result: ┌─tupleToNameValuePairs(tuple(3, 2, 1))─┐ │ [('1',3),('2',2),('3',1)] │ └───────────────────────────────────────┘ +``` ## tuplePlus {#tupleplus} diff --git a/docs/ru/sql-reference/functions/tuple-functions.md b/docs/ru/sql-reference/functions/tuple-functions.md index 006557d859f..57cff17913b 100644 --- a/docs/ru/sql-reference/functions/tuple-functions.md +++ b/docs/ru/sql-reference/functions/tuple-functions.md @@ -237,6 +237,7 @@ SELECT tupleToNameValuePairs(tuple(3, 2, 1)); ┌─tupleToNameValuePairs(tuple(3, 2, 1))─┐ │ [('1',3),('2',2),('3',1)] │ └───────────────────────────────────────┘ +``` ## tuplePlus {#tupleplus}