From b39f8cc6ac2e470b01826458f3f0f286367da64e Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 8 Mar 2021 22:05:51 +0300 Subject: [PATCH 01/12] Move ErrorCodes::increment() into module part --- src/Common/ErrorCodes.cpp | 11 +++++++++++ src/Common/ErrorCodes.h | 11 +---------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index 40ce23fffb2..f6c15848553 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -584,6 +584,17 @@ namespace ErrorCodes } ErrorCode end() { return END + 1; } + + void increment(ErrorCode error_code) + { + if (error_code >= end()) + { + /// For everything outside the range, use END. + /// (end() is the pointer pass the end, while END is the last value that has an element in values array). + error_code = end() - 1; + } + values[error_code].fetch_add(1, std::memory_order_relaxed); + } } } diff --git a/src/Common/ErrorCodes.h b/src/Common/ErrorCodes.h index cc610c5d927..919a4afdabf 100644 --- a/src/Common/ErrorCodes.h +++ b/src/Common/ErrorCodes.h @@ -31,16 +31,7 @@ namespace ErrorCodes ErrorCode end(); /// Add value for specified error_code. - inline void increment(ErrorCode error_code) - { - if (error_code >= end()) - { - /// For everything outside the range, use END. - /// (end() is the pointer pass the end, while END is the last value that has an element in values array). - error_code = end() - 1; - } - values[error_code].fetch_add(1, std::memory_order_relaxed); - } + void increment(ErrorCode error_code); } } From 259e5ba88e35546279bb46511a5e6ad18b4457d6 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 8 Mar 2021 22:05:51 +0300 Subject: [PATCH 02/12] Separate accounting of remote exceptions in system.errors --- docs/en/operations/system-tables/errors.md | 3 ++- src/Common/ErrorCodes.cpp | 9 ++++--- src/Common/ErrorCodes.h | 9 +++++-- src/Common/Exception.cpp | 12 +++++----- src/Storages/System/StorageSystemErrors.cpp | 24 ++++++++++++------- .../0_stateless/01545_system_errors.reference | 3 ++- .../0_stateless/01545_system_errors.sh | 13 +++++++--- 7 files changed, 49 insertions(+), 24 deletions(-) diff --git a/docs/en/operations/system-tables/errors.md b/docs/en/operations/system-tables/errors.md index ec874efd711..bf3e33f5275 100644 --- a/docs/en/operations/system-tables/errors.md +++ b/docs/en/operations/system-tables/errors.md @@ -7,11 +7,12 @@ Columns: - `name` ([String](../../sql-reference/data-types/string.md)) — name of the error (`errorCodeToName`). - `code` ([Int32](../../sql-reference/data-types/int-uint.md)) — code number of the error. - `value` ([UInt64](../../sql-reference/data-types/int-uint.md)) — the number of times this error has been happened. +- `remote` ([UInt8](../../sql-reference/data-types/int-uint.md)) — remote exception (i.e. received during one of the distributed query). **Example** ``` sql -SELECT * +SELECT name, code, value FROM system.errors WHERE value > 0 ORDER BY code ASC diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index f6c15848553..ec3bf9c8917 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -563,7 +563,7 @@ namespace ErrorCodes #undef M constexpr Value END = 3000; - std::atomic values[END + 1]{}; + ValuePair values[END + 1]{}; struct ErrorCodesNames { @@ -585,7 +585,7 @@ namespace ErrorCodes ErrorCode end() { return END + 1; } - void increment(ErrorCode error_code) + void increment(ErrorCode error_code, bool remote) { if (error_code >= end()) { @@ -593,7 +593,10 @@ namespace ErrorCodes /// (end() is the pointer pass the end, while END is the last value that has an element in values array). error_code = end() - 1; } - values[error_code].fetch_add(1, std::memory_order_relaxed); + if (remote) + values[error_code].remote.fetch_add(1, std::memory_order_relaxed); + else + values[error_code].local.fetch_add(1, std::memory_order_relaxed); } } diff --git a/src/Common/ErrorCodes.h b/src/Common/ErrorCodes.h index 919a4afdabf..c4a9ae2907b 100644 --- a/src/Common/ErrorCodes.h +++ b/src/Common/ErrorCodes.h @@ -25,13 +25,18 @@ namespace ErrorCodes std::string_view getName(ErrorCode error_code); /// ErrorCode identifier -> current value of error_code. - extern std::atomic values[]; + struct ValuePair + { + std::atomic local; + std::atomic remote; + }; + extern ValuePair values[]; /// Get index just after last error_code identifier. ErrorCode end(); /// Add value for specified error_code. - void increment(ErrorCode error_code); + void increment(ErrorCode error_code, bool remote); } } diff --git a/src/Common/Exception.cpp b/src/Common/Exception.cpp index 16f15d4e6f2..1963c1513b9 100644 --- a/src/Common/Exception.cpp +++ b/src/Common/Exception.cpp @@ -34,9 +34,9 @@ namespace ErrorCodes extern const int CANNOT_MREMAP; } -/// Aborts the process if error code is LOGICAL_ERROR. -/// Increments error codes statistics. -void handle_error_code([[maybe_unused]] const std::string & msg, int code) +/// - Aborts the process if error code is LOGICAL_ERROR. +/// - Increments error codes statistics. +void handle_error_code([[maybe_unused]] const std::string & msg, int code, bool remote) { // In debug builds and builds with sanitizers, treat LOGICAL_ERROR as an assertion failure. // Log the message before we fail. @@ -47,20 +47,20 @@ void handle_error_code([[maybe_unused]] const std::string & msg, int code) abort(); } #endif - ErrorCodes::increment(code); + ErrorCodes::increment(code, remote); } Exception::Exception(const std::string & msg, int code, bool remote_) : Poco::Exception(msg, code) , remote(remote_) { - handle_error_code(msg, code); + handle_error_code(msg, code, remote); } Exception::Exception(const std::string & msg, const Exception & nested, int code) : Poco::Exception(msg, nested, code) { - handle_error_code(msg, code); + handle_error_code(msg, code, remote); } Exception::Exception(CreateFromPocoTag, const Poco::Exception & exc) diff --git a/src/Storages/System/StorageSystemErrors.cpp b/src/Storages/System/StorageSystemErrors.cpp index 89df058900b..1a29484e169 100644 --- a/src/Storages/System/StorageSystemErrors.cpp +++ b/src/Storages/System/StorageSystemErrors.cpp @@ -13,27 +13,35 @@ NamesAndTypesList StorageSystemErrors::getNamesAndTypes() { "name", std::make_shared() }, { "code", std::make_shared() }, { "value", std::make_shared() }, + { "remote", std::make_shared() }, }; } void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & context, const SelectQueryInfo &) const { + auto add_row = [&](std::string_view name, size_t code, size_t value, bool remote) + { + if (value || context.getSettingsRef().system_events_show_zero_values) + { + size_t col_num = 0; + res_columns[col_num++]->insert(name); + res_columns[col_num++]->insert(code); + res_columns[col_num++]->insert(value); + res_columns[col_num++]->insert(remote); + } + }; + for (size_t i = 0, end = ErrorCodes::end(); i < end; ++i) { - UInt64 value = ErrorCodes::values[i]; + const auto & error = ErrorCodes::values[i]; std::string_view name = ErrorCodes::getName(i); if (name.empty()) continue; - if (value || context.getSettingsRef().system_events_show_zero_values) - { - size_t col_num = 0; - res_columns[col_num++]->insert(name); - res_columns[col_num++]->insert(i); - res_columns[col_num++]->insert(value); - } + add_row(name, i, error.local, 0 /* remote=0 */); + add_row(name, i, error.remote, 1 /* remote=1 */); } } diff --git a/tests/queries/0_stateless/01545_system_errors.reference b/tests/queries/0_stateless/01545_system_errors.reference index d00491fd7e5..0e7f2447090 100644 --- a/tests/queries/0_stateless/01545_system_errors.reference +++ b/tests/queries/0_stateless/01545_system_errors.reference @@ -1 +1,2 @@ -1 +local=1 +remote=1 diff --git a/tests/queries/0_stateless/01545_system_errors.sh b/tests/queries/0_stateless/01545_system_errors.sh index 63af6bb8d43..970fd403866 100755 --- a/tests/queries/0_stateless/01545_system_errors.sh +++ b/tests/queries/0_stateless/01545_system_errors.sh @@ -4,7 +4,14 @@ CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) # shellcheck source=../shell_config.sh . "$CURDIR"/../shell_config.sh -prev="$(${CLICKHOUSE_CLIENT} -q "SELECT value FROM system.errors WHERE name = 'FUNCTION_THROW_IF_VALUE_IS_NON_ZERO'")" +# local +prev="$(${CLICKHOUSE_CLIENT} -q "SELECT value FROM system.errors WHERE name = 'FUNCTION_THROW_IF_VALUE_IS_NON_ZERO' AND NOT remote")" $CLICKHOUSE_CLIENT -q 'SELECT throwIf(1)' >& /dev/null -cur="$(${CLICKHOUSE_CLIENT} -q "SELECT value FROM system.errors WHERE name = 'FUNCTION_THROW_IF_VALUE_IS_NON_ZERO'")" -echo $((cur - prev)) +cur="$(${CLICKHOUSE_CLIENT} -q "SELECT value FROM system.errors WHERE name = 'FUNCTION_THROW_IF_VALUE_IS_NON_ZERO' AND NOT remote")" +echo local=$((cur - prev)) + +# remote +prev="$(${CLICKHOUSE_CLIENT} -q "SELECT value FROM system.errors WHERE name = 'FUNCTION_THROW_IF_VALUE_IS_NON_ZERO' AND remote")" +${CLICKHOUSE_CLIENT} -q "SELECT * FROM remote('127.2', system.one) where throwIf(not dummy)" >& /dev/null +cur="$(${CLICKHOUSE_CLIENT} -q "SELECT value FROM system.errors WHERE name = 'FUNCTION_THROW_IF_VALUE_IS_NON_ZERO' AND remote")" +echo remote=$((cur - prev)) From 7f73ac2b7a94ed809f4a2eaa59c13d7585756a6c Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 8 Mar 2021 22:05:51 +0300 Subject: [PATCH 03/12] Fix ErrorCodes::Value/ErrorCode types (sigh) Note, that system.errors already uses correct types --- src/Common/ErrorCodes.cpp | 2 +- src/Common/ErrorCodes.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index ec3bf9c8917..6f1ff8e4f3a 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -562,7 +562,7 @@ namespace ErrorCodes APPLY_FOR_ERROR_CODES(M) #undef M - constexpr Value END = 3000; + constexpr ErrorCode END = 3000; ValuePair values[END + 1]{}; struct ErrorCodesNames diff --git a/src/Common/ErrorCodes.h b/src/Common/ErrorCodes.h index c4a9ae2907b..6373ad6d0f9 100644 --- a/src/Common/ErrorCodes.h +++ b/src/Common/ErrorCodes.h @@ -17,8 +17,8 @@ namespace DB namespace ErrorCodes { /// ErrorCode identifier (index in array). - using ErrorCode = size_t; - using Value = int; + using ErrorCode = int; + using Value = size_t; /// Get name of error_code by identifier. /// Returns statically allocated string. From 0d01eaf94fd122213b3d31593248fe3f3e9c6a40 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 8 Mar 2021 23:08:04 +0300 Subject: [PATCH 04/12] Guard ErrorCodes with mutex over atomic --- src/Common/ErrorCodes.cpp | 30 +++++++++++++++++---- src/Common/ErrorCodes.h | 25 +++++++++++++---- src/Storages/System/StorageSystemErrors.cpp | 2 +- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index 6f1ff8e4f3a..32c9c4a452b 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -563,7 +563,7 @@ namespace ErrorCodes #undef M constexpr ErrorCode END = 3000; - ValuePair values[END + 1]{}; + ValuePairHolder values[END + 1]{}; struct ErrorCodesNames { @@ -593,10 +593,30 @@ namespace ErrorCodes /// (end() is the pointer pass the end, while END is the last value that has an element in values array). error_code = end() - 1; } - if (remote) - values[error_code].remote.fetch_add(1, std::memory_order_relaxed); - else - values[error_code].local.fetch_add(1, std::memory_order_relaxed); + + ValuePair inc_value{ + !remote, /* local */ + remote, /* remote */ + }; + values[error_code].increment(inc_value); + } + + ValuePair & ValuePair::operator+=(const ValuePair & value) + { + local += value.local; + remote += value.remote; + return *this; + } + + void ValuePairHolder::increment(const ValuePair & value_) + { + std::lock_guard lock(mutex); + value += value_; + } + ValuePair ValuePairHolder::get() + { + std::lock_guard lock(mutex); + return value; } } diff --git a/src/Common/ErrorCodes.h b/src/Common/ErrorCodes.h index 6373ad6d0f9..0db877db205 100644 --- a/src/Common/ErrorCodes.h +++ b/src/Common/ErrorCodes.h @@ -3,7 +3,7 @@ #include #include #include -#include +#include #include #include @@ -24,13 +24,28 @@ namespace ErrorCodes /// Returns statically allocated string. std::string_view getName(ErrorCode error_code); - /// ErrorCode identifier -> current value of error_code. struct ValuePair { - std::atomic local; - std::atomic remote; + Value local = 0; + Value remote = 0; + + ValuePair & operator+=(const ValuePair & value); }; - extern ValuePair values[]; + + /// Thread-safe + struct ValuePairHolder + { + public: + void increment(const ValuePair & value_); + ValuePair get(); + + private: + ValuePair value; + std::mutex mutex; + }; + + /// ErrorCode identifier -> current value of error_code. + extern ValuePairHolder values[]; /// Get index just after last error_code identifier. ErrorCode end(); diff --git a/src/Storages/System/StorageSystemErrors.cpp b/src/Storages/System/StorageSystemErrors.cpp index 1a29484e169..d57e8a0a670 100644 --- a/src/Storages/System/StorageSystemErrors.cpp +++ b/src/Storages/System/StorageSystemErrors.cpp @@ -34,7 +34,7 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & for (size_t i = 0, end = ErrorCodes::end(); i < end; ++i) { - const auto & error = ErrorCodes::values[i]; + const auto & error = ErrorCodes::values[i].get(); std::string_view name = ErrorCodes::getName(i); if (name.empty()) From c8852331a2f3356069155d6ab45ba79f1ecada57 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 8 Mar 2021 23:16:46 +0300 Subject: [PATCH 05/12] Add system.errors.last_error_time column --- docs/en/operations/system-tables/errors.md | 1 + src/Common/ErrorCodes.cpp | 5 +++++ src/Common/ErrorCodes.h | 1 + src/Storages/System/StorageSystemErrors.cpp | 9 ++++++--- 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/docs/en/operations/system-tables/errors.md b/docs/en/operations/system-tables/errors.md index bf3e33f5275..b440ae4d787 100644 --- a/docs/en/operations/system-tables/errors.md +++ b/docs/en/operations/system-tables/errors.md @@ -7,6 +7,7 @@ Columns: - `name` ([String](../../sql-reference/data-types/string.md)) — name of the error (`errorCodeToName`). - `code` ([Int32](../../sql-reference/data-types/int-uint.md)) — code number of the error. - `value` ([UInt64](../../sql-reference/data-types/int-uint.md)) — the number of times this error has been happened. +- `last_error_time` ([DateTime](../../sql-reference/data-types/datetime.md)) — time when the last error happened. - `remote` ([UInt8](../../sql-reference/data-types/int-uint.md)) — remote exception (i.e. received during one of the distributed query). **Example** diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index 32c9c4a452b..3532c063651 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -1,4 +1,5 @@ #include +#include /** Previously, these constants were located in one enum. * But in this case there is a problem: when you add a new constant, you need to recompile @@ -605,6 +606,10 @@ namespace ErrorCodes { local += value.local; remote += value.remote; + + const auto now = std::chrono::system_clock::now(); + last_error_time_ms = std::chrono::duration_cast(now.time_since_epoch()).count(); + return *this; } diff --git a/src/Common/ErrorCodes.h b/src/Common/ErrorCodes.h index 0db877db205..c8c454b51a7 100644 --- a/src/Common/ErrorCodes.h +++ b/src/Common/ErrorCodes.h @@ -28,6 +28,7 @@ namespace ErrorCodes { Value local = 0; Value remote = 0; + UInt64 last_error_time_ms = 0; ValuePair & operator+=(const ValuePair & value); }; diff --git a/src/Storages/System/StorageSystemErrors.cpp b/src/Storages/System/StorageSystemErrors.cpp index d57e8a0a670..a3d68ff5d86 100644 --- a/src/Storages/System/StorageSystemErrors.cpp +++ b/src/Storages/System/StorageSystemErrors.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -13,6 +14,7 @@ NamesAndTypesList StorageSystemErrors::getNamesAndTypes() { "name", std::make_shared() }, { "code", std::make_shared() }, { "value", std::make_shared() }, + { "last_error_time", std::make_shared() }, { "remote", std::make_shared() }, }; } @@ -20,7 +22,7 @@ NamesAndTypesList StorageSystemErrors::getNamesAndTypes() void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & context, const SelectQueryInfo &) const { - auto add_row = [&](std::string_view name, size_t code, size_t value, bool remote) + auto add_row = [&](std::string_view name, size_t code, size_t value, UInt64 last_error_time_ms, bool remote) { if (value || context.getSettingsRef().system_events_show_zero_values) { @@ -28,6 +30,7 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & res_columns[col_num++]->insert(name); res_columns[col_num++]->insert(code); res_columns[col_num++]->insert(value); + res_columns[col_num++]->insert(last_error_time_ms / 1000); res_columns[col_num++]->insert(remote); } }; @@ -40,8 +43,8 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & if (name.empty()) continue; - add_row(name, i, error.local, 0 /* remote=0 */); - add_row(name, i, error.remote, 1 /* remote=1 */); + add_row(name, i, error.local, error.last_error_time_ms, 0 /* remote=0 */); + add_row(name, i, error.remote, error.last_error_time_ms, 1 /* remote=1 */); } } From 775f8f76827378b98d0b679eaa564ac7697ee81f Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 8 Mar 2021 23:31:51 +0300 Subject: [PATCH 06/12] Add system.errors.last_error_message column --- docs/en/operations/system-tables/errors.md | 1 + src/Common/ErrorCodes.cpp | 5 ++++- src/Common/ErrorCodes.h | 3 ++- src/Common/Exception.cpp | 2 +- src/Storages/System/StorageSystemErrors.cpp | 8 +++++--- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/docs/en/operations/system-tables/errors.md b/docs/en/operations/system-tables/errors.md index b440ae4d787..f8ac1de29a8 100644 --- a/docs/en/operations/system-tables/errors.md +++ b/docs/en/operations/system-tables/errors.md @@ -8,6 +8,7 @@ Columns: - `code` ([Int32](../../sql-reference/data-types/int-uint.md)) — code number of the error. - `value` ([UInt64](../../sql-reference/data-types/int-uint.md)) — the number of times this error has been happened. - `last_error_time` ([DateTime](../../sql-reference/data-types/datetime.md)) — time when the last error happened. +- `last_error_message` ([String](../../sql-reference/data-types/string.md)) — message for the last error. - `remote` ([UInt8](../../sql-reference/data-types/int-uint.md)) — remote exception (i.e. received during one of the distributed query). **Example** diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index 3532c063651..6c9de122a26 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -586,7 +586,7 @@ namespace ErrorCodes ErrorCode end() { return END + 1; } - void increment(ErrorCode error_code, bool remote) + void increment(ErrorCode error_code, bool remote, const std::string & message) { if (error_code >= end()) { @@ -598,6 +598,8 @@ namespace ErrorCodes ValuePair inc_value{ !remote, /* local */ remote, /* remote */ + 0, /* last_error_time_ms */ + message, /* message */ }; values[error_code].increment(inc_value); } @@ -606,6 +608,7 @@ namespace ErrorCodes { local += value.local; remote += value.remote; + message = value.message; const auto now = std::chrono::system_clock::now(); last_error_time_ms = std::chrono::duration_cast(now.time_since_epoch()).count(); diff --git a/src/Common/ErrorCodes.h b/src/Common/ErrorCodes.h index c8c454b51a7..962b1f8a20a 100644 --- a/src/Common/ErrorCodes.h +++ b/src/Common/ErrorCodes.h @@ -29,6 +29,7 @@ namespace ErrorCodes Value local = 0; Value remote = 0; UInt64 last_error_time_ms = 0; + std::string message; ValuePair & operator+=(const ValuePair & value); }; @@ -52,7 +53,7 @@ namespace ErrorCodes ErrorCode end(); /// Add value for specified error_code. - void increment(ErrorCode error_code, bool remote); + void increment(ErrorCode error_code, bool remote, const std::string & message); } } diff --git a/src/Common/Exception.cpp b/src/Common/Exception.cpp index 1963c1513b9..1fe224edc6e 100644 --- a/src/Common/Exception.cpp +++ b/src/Common/Exception.cpp @@ -47,7 +47,7 @@ void handle_error_code([[maybe_unused]] const std::string & msg, int code, bool abort(); } #endif - ErrorCodes::increment(code, remote); + ErrorCodes::increment(code, remote, msg); } Exception::Exception(const std::string & msg, int code, bool remote_) diff --git a/src/Storages/System/StorageSystemErrors.cpp b/src/Storages/System/StorageSystemErrors.cpp index a3d68ff5d86..c06bb13beb6 100644 --- a/src/Storages/System/StorageSystemErrors.cpp +++ b/src/Storages/System/StorageSystemErrors.cpp @@ -15,6 +15,7 @@ NamesAndTypesList StorageSystemErrors::getNamesAndTypes() { "code", std::make_shared() }, { "value", std::make_shared() }, { "last_error_time", std::make_shared() }, + { "last_error_message",std::make_shared() }, { "remote", std::make_shared() }, }; } @@ -22,7 +23,7 @@ NamesAndTypesList StorageSystemErrors::getNamesAndTypes() void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & context, const SelectQueryInfo &) const { - auto add_row = [&](std::string_view name, size_t code, size_t value, UInt64 last_error_time_ms, bool remote) + auto add_row = [&](std::string_view name, size_t code, size_t value, UInt64 last_error_time_ms, const std::string & message, bool remote) { if (value || context.getSettingsRef().system_events_show_zero_values) { @@ -31,6 +32,7 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & res_columns[col_num++]->insert(code); res_columns[col_num++]->insert(value); res_columns[col_num++]->insert(last_error_time_ms / 1000); + res_columns[col_num++]->insert(message); res_columns[col_num++]->insert(remote); } }; @@ -43,8 +45,8 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & if (name.empty()) continue; - add_row(name, i, error.local, error.last_error_time_ms, 0 /* remote=0 */); - add_row(name, i, error.remote, error.last_error_time_ms, 1 /* remote=1 */); + add_row(name, i, error.local, error.last_error_time_ms, error.message, 0 /* remote=0 */); + add_row(name, i, error.remote, error.last_error_time_ms, error.message, 1 /* remote=1 */); } } From 44c9dc753da4add80dd0a92ce14152790138e85a Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 8 Mar 2021 23:39:19 +0300 Subject: [PATCH 07/12] Add system.errors.last_error_stacktrace column --- docs/en/operations/system-tables/errors.md | 1 + src/Common/ErrorCodes.cpp | 12 +++++++----- src/Common/ErrorCodes.h | 3 ++- src/Common/Exception.cpp | 8 ++++---- src/Storages/System/StorageSystemErrors.cpp | 20 +++++++++++--------- 5 files changed, 25 insertions(+), 19 deletions(-) diff --git a/docs/en/operations/system-tables/errors.md b/docs/en/operations/system-tables/errors.md index f8ac1de29a8..72a537f15b9 100644 --- a/docs/en/operations/system-tables/errors.md +++ b/docs/en/operations/system-tables/errors.md @@ -9,6 +9,7 @@ Columns: - `value` ([UInt64](../../sql-reference/data-types/int-uint.md)) — the number of times this error has been happened. - `last_error_time` ([DateTime](../../sql-reference/data-types/datetime.md)) — time when the last error happened. - `last_error_message` ([String](../../sql-reference/data-types/string.md)) — message for the last error. +- `last_error_stacktrace` ([String](../../sql-reference/data-types/string.md)) — stacktrace for the last error. - `remote` ([UInt8](../../sql-reference/data-types/int-uint.md)) — remote exception (i.e. received during one of the distributed query). **Example** diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index 6c9de122a26..14182467351 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -586,7 +586,7 @@ namespace ErrorCodes ErrorCode end() { return END + 1; } - void increment(ErrorCode error_code, bool remote, const std::string & message) + void increment(ErrorCode error_code, bool remote, const std::string & message, const std::string & stacktrace) { if (error_code >= end()) { @@ -596,10 +596,11 @@ namespace ErrorCodes } ValuePair inc_value{ - !remote, /* local */ - remote, /* remote */ - 0, /* last_error_time_ms */ - message, /* message */ + !remote, /* local */ + remote, /* remote */ + 0, /* last_error_time_ms */ + message, /* message */ + stacktrace, /* stacktrace */ }; values[error_code].increment(inc_value); } @@ -609,6 +610,7 @@ namespace ErrorCodes local += value.local; remote += value.remote; message = value.message; + stacktrace = value.stacktrace; const auto now = std::chrono::system_clock::now(); last_error_time_ms = std::chrono::duration_cast(now.time_since_epoch()).count(); diff --git a/src/Common/ErrorCodes.h b/src/Common/ErrorCodes.h index 962b1f8a20a..4c79614d55d 100644 --- a/src/Common/ErrorCodes.h +++ b/src/Common/ErrorCodes.h @@ -30,6 +30,7 @@ namespace ErrorCodes Value remote = 0; UInt64 last_error_time_ms = 0; std::string message; + std::string stacktrace; ValuePair & operator+=(const ValuePair & value); }; @@ -53,7 +54,7 @@ namespace ErrorCodes ErrorCode end(); /// Add value for specified error_code. - void increment(ErrorCode error_code, bool remote, const std::string & message); + void increment(ErrorCode error_code, bool remote, const std::string & message, const std::string & stacktrace); } } diff --git a/src/Common/Exception.cpp b/src/Common/Exception.cpp index 1fe224edc6e..08afd0397f5 100644 --- a/src/Common/Exception.cpp +++ b/src/Common/Exception.cpp @@ -36,7 +36,7 @@ namespace ErrorCodes /// - Aborts the process if error code is LOGICAL_ERROR. /// - Increments error codes statistics. -void handle_error_code([[maybe_unused]] const std::string & msg, int code, bool remote) +void handle_error_code([[maybe_unused]] const std::string & msg, const std::string & stacktrace, int code, bool remote) { // In debug builds and builds with sanitizers, treat LOGICAL_ERROR as an assertion failure. // Log the message before we fail. @@ -47,20 +47,20 @@ void handle_error_code([[maybe_unused]] const std::string & msg, int code, bool abort(); } #endif - ErrorCodes::increment(code, remote, msg); + ErrorCodes::increment(code, remote, msg, stacktrace); } Exception::Exception(const std::string & msg, int code, bool remote_) : Poco::Exception(msg, code) , remote(remote_) { - handle_error_code(msg, code, remote); + handle_error_code(msg, getStackTraceString(), code, remote); } Exception::Exception(const std::string & msg, const Exception & nested, int code) : Poco::Exception(msg, nested, code) { - handle_error_code(msg, code, remote); + handle_error_code(msg, getStackTraceString(), code, remote); } Exception::Exception(CreateFromPocoTag, const Poco::Exception & exc) diff --git a/src/Storages/System/StorageSystemErrors.cpp b/src/Storages/System/StorageSystemErrors.cpp index c06bb13beb6..87cf3f2f603 100644 --- a/src/Storages/System/StorageSystemErrors.cpp +++ b/src/Storages/System/StorageSystemErrors.cpp @@ -11,19 +11,20 @@ namespace DB NamesAndTypesList StorageSystemErrors::getNamesAndTypes() { return { - { "name", std::make_shared() }, - { "code", std::make_shared() }, - { "value", std::make_shared() }, - { "last_error_time", std::make_shared() }, - { "last_error_message",std::make_shared() }, - { "remote", std::make_shared() }, + { "name", std::make_shared() }, + { "code", std::make_shared() }, + { "value", std::make_shared() }, + { "last_error_time", std::make_shared() }, + { "last_error_message", std::make_shared() }, + { "last_error_stacktrace", std::make_shared() }, + { "remote", std::make_shared() }, }; } void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & context, const SelectQueryInfo &) const { - auto add_row = [&](std::string_view name, size_t code, size_t value, UInt64 last_error_time_ms, const std::string & message, bool remote) + auto add_row = [&](std::string_view name, size_t code, size_t value, UInt64 last_error_time_ms, const std::string & message, const std::string & stacktrace, bool remote) { if (value || context.getSettingsRef().system_events_show_zero_values) { @@ -33,6 +34,7 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & res_columns[col_num++]->insert(value); res_columns[col_num++]->insert(last_error_time_ms / 1000); res_columns[col_num++]->insert(message); + res_columns[col_num++]->insert(stacktrace); res_columns[col_num++]->insert(remote); } }; @@ -45,8 +47,8 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & if (name.empty()) continue; - add_row(name, i, error.local, error.last_error_time_ms, error.message, 0 /* remote=0 */); - add_row(name, i, error.remote, error.last_error_time_ms, error.message, 1 /* remote=1 */); + add_row(name, i, error.local, error.last_error_time_ms, error.message, error.stacktrace, 0 /* remote=0 */); + add_row(name, i, error.remote, error.last_error_time_ms, error.message, error.stacktrace, 1 /* remote=1 */); } } From efdd04c958960233fad73f480366d3cfcbffba0d Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 8 Mar 2021 23:43:58 +0300 Subject: [PATCH 08/12] Drop last_ prefix for ErrorCodes::ValuePair::error_time_ms --- src/Common/ErrorCodes.cpp | 4 ++-- src/Common/ErrorCodes.h | 2 +- src/Storages/System/StorageSystemErrors.cpp | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index 14182467351..d7e0d5fb16a 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -598,7 +598,7 @@ namespace ErrorCodes ValuePair inc_value{ !remote, /* local */ remote, /* remote */ - 0, /* last_error_time_ms */ + 0, /* error_time_ms */ message, /* message */ stacktrace, /* stacktrace */ }; @@ -613,7 +613,7 @@ namespace ErrorCodes stacktrace = value.stacktrace; const auto now = std::chrono::system_clock::now(); - last_error_time_ms = std::chrono::duration_cast(now.time_since_epoch()).count(); + error_time_ms = std::chrono::duration_cast(now.time_since_epoch()).count(); return *this; } diff --git a/src/Common/ErrorCodes.h b/src/Common/ErrorCodes.h index 4c79614d55d..1c8f0a58884 100644 --- a/src/Common/ErrorCodes.h +++ b/src/Common/ErrorCodes.h @@ -28,7 +28,7 @@ namespace ErrorCodes { Value local = 0; Value remote = 0; - UInt64 last_error_time_ms = 0; + UInt64 error_time_ms = 0; std::string message; std::string stacktrace; diff --git a/src/Storages/System/StorageSystemErrors.cpp b/src/Storages/System/StorageSystemErrors.cpp index 87cf3f2f603..c16eba6754b 100644 --- a/src/Storages/System/StorageSystemErrors.cpp +++ b/src/Storages/System/StorageSystemErrors.cpp @@ -24,7 +24,7 @@ NamesAndTypesList StorageSystemErrors::getNamesAndTypes() void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & context, const SelectQueryInfo &) const { - auto add_row = [&](std::string_view name, size_t code, size_t value, UInt64 last_error_time_ms, const std::string & message, const std::string & stacktrace, bool remote) + auto add_row = [&](std::string_view name, size_t code, size_t value, UInt64 error_time_ms, const std::string & message, const std::string & stacktrace, bool remote) { if (value || context.getSettingsRef().system_events_show_zero_values) { @@ -32,7 +32,7 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & res_columns[col_num++]->insert(name); res_columns[col_num++]->insert(code); res_columns[col_num++]->insert(value); - res_columns[col_num++]->insert(last_error_time_ms / 1000); + res_columns[col_num++]->insert(error_time_ms / 1000); res_columns[col_num++]->insert(message); res_columns[col_num++]->insert(stacktrace); res_columns[col_num++]->insert(remote); @@ -47,8 +47,8 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & if (name.empty()) continue; - add_row(name, i, error.local, error.last_error_time_ms, error.message, error.stacktrace, 0 /* remote=0 */); - add_row(name, i, error.remote, error.last_error_time_ms, error.message, error.stacktrace, 1 /* remote=1 */); + add_row(name, i, error.local, error.error_time_ms, error.message, error.stacktrace, 0 /* remote=0 */); + add_row(name, i, error.remote, error.error_time_ms, error.message, error.stacktrace, 1 /* remote=1 */); } } From cc87bcfb63fbca0a9024364156d60dc703521c63 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 9 Mar 2021 09:09:28 +0300 Subject: [PATCH 09/12] Fix errorCodeToName() for signed integers - https://clickhouse-test-reports.s3.yandex.net/21529/2ce2772d35eb3d81628f4d294d5799e9f05333fd/functional_stateless_tests_(address).html#fail1 - https://clickhouse-test-reports.s3.yandex.net/21529/2ce2772d35eb3d81628f4d294d5799e9f05333fd/functional_stateless_tests_(ubsan).html#fail1 - https://clickhouse-test-reports.s3.yandex.net/21529/2ce2772d35eb3d81628f4d294d5799e9f05333fd/stress_test_(address).html#fail1 --- src/Common/ErrorCodes.cpp | 2 +- tests/queries/0_stateless/01544_errorCodeToName.reference | 1 + tests/queries/0_stateless/01544_errorCodeToName.sql | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index d7e0d5fb16a..879784bb43a 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -579,7 +579,7 @@ namespace ErrorCodes std::string_view getName(ErrorCode error_code) { - if (error_code >= END) + if (error_code < 0 || error_code >= END) return std::string_view(); return error_codes_names.names[error_code]; } diff --git a/tests/queries/0_stateless/01544_errorCodeToName.reference b/tests/queries/0_stateless/01544_errorCodeToName.reference index ace588644e1..fefccf984be 100644 --- a/tests/queries/0_stateless/01544_errorCodeToName.reference +++ b/tests/queries/0_stateless/01544_errorCodeToName.reference @@ -1,4 +1,5 @@ + OK UNSUPPORTED_METHOD diff --git a/tests/queries/0_stateless/01544_errorCodeToName.sql b/tests/queries/0_stateless/01544_errorCodeToName.sql index 9e28ed1116c..aa32270f00b 100644 --- a/tests/queries/0_stateless/01544_errorCodeToName.sql +++ b/tests/queries/0_stateless/01544_errorCodeToName.sql @@ -1,4 +1,5 @@ SELECT errorCodeToName(toUInt32(-1)); +SELECT errorCodeToName(-1); SELECT errorCodeToName(600); /* gap in error codes */ SELECT errorCodeToName(0); SELECT errorCodeToName(1); From 9921e7ca284b7d274af540d13812881dd6a0e578 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 9 Mar 2021 09:12:08 +0300 Subject: [PATCH 10/12] Add 01545_system_errors into skip_list.parallel https://clickhouse-test-reports.s3.yandex.net/21529/2ce2772d35eb3d81628f4d294d5799e9f05333fd/functional_stateless_tests_flaky_check_(address).html#fail1 --- tests/queries/skip_list.json | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/queries/skip_list.json b/tests/queries/skip_list.json index bded0807db9..caab92636b3 100644 --- a/tests/queries/skip_list.json +++ b/tests/queries/skip_list.json @@ -739,6 +739,7 @@ "01541_max_memory_usage_for_user_long", "01542_dictionary_load_exception_race", "01560_optimize_on_insert_zookeeper", + "01545_system_errors", // looks at the difference of values in system.errors "01575_disable_detach_table_of_dictionary", "01593_concurrent_alter_mutations_kill", "01593_concurrent_alter_mutations_kill_many_replicas", From a337691b060e9ad2c2cf53e9762691352f732837 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 9 Mar 2021 10:05:56 +0300 Subject: [PATCH 11/12] Fix modernize-use-bool-literals clang-tidy warning in StorageSystemErrors --- src/Storages/System/StorageSystemErrors.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Storages/System/StorageSystemErrors.cpp b/src/Storages/System/StorageSystemErrors.cpp index c16eba6754b..c9aac9ce007 100644 --- a/src/Storages/System/StorageSystemErrors.cpp +++ b/src/Storages/System/StorageSystemErrors.cpp @@ -47,8 +47,8 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & if (name.empty()) continue; - add_row(name, i, error.local, error.error_time_ms, error.message, error.stacktrace, 0 /* remote=0 */); - add_row(name, i, error.remote, error.error_time_ms, error.message, error.stacktrace, 1 /* remote=1 */); + add_row(name, i, error.local, error.error_time_ms, error.message, error.stacktrace, false /* remote=0 */); + add_row(name, i, error.remote, error.error_time_ms, error.message, error.stacktrace, true /* remote=1 */); } } From 9dee842b6082a3e90c65a8a45e5a357de30106a6 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Tue, 16 Mar 2021 21:31:14 +0300 Subject: [PATCH 12/12] Distinguish remote and local error info --- src/Common/ErrorCodes.cpp | 34 +++++++-------------- src/Common/ErrorCodes.h | 26 ++++++++++------ src/Storages/System/StorageSystemErrors.cpp | 16 +++++----- 3 files changed, 35 insertions(+), 41 deletions(-) diff --git a/src/Common/ErrorCodes.cpp b/src/Common/ErrorCodes.cpp index 879784bb43a..a4e2f8742ca 100644 --- a/src/Common/ErrorCodes.cpp +++ b/src/Common/ErrorCodes.cpp @@ -564,7 +564,7 @@ namespace ErrorCodes #undef M constexpr ErrorCode END = 3000; - ValuePairHolder values[END + 1]{}; + ErrorPairHolder values[END + 1]{}; struct ErrorCodesNames { @@ -595,35 +595,23 @@ namespace ErrorCodes error_code = end() - 1; } - ValuePair inc_value{ - !remote, /* local */ - remote, /* remote */ - 0, /* error_time_ms */ - message, /* message */ - stacktrace, /* stacktrace */ - }; - values[error_code].increment(inc_value); + values[error_code].increment(remote, message, stacktrace); } - ValuePair & ValuePair::operator+=(const ValuePair & value) + void ErrorPairHolder::increment(bool remote, const std::string & message, const std::string & stacktrace) { - local += value.local; - remote += value.remote; - message = value.message; - stacktrace = value.stacktrace; - const auto now = std::chrono::system_clock::now(); - error_time_ms = std::chrono::duration_cast(now.time_since_epoch()).count(); - return *this; - } - - void ValuePairHolder::increment(const ValuePair & value_) - { std::lock_guard lock(mutex); - value += value_; + + auto & error = remote ? value.remote : value.local; + + ++error.count; + error.message = message; + error.stacktrace = stacktrace; + error.error_time_ms = std::chrono::duration_cast(now.time_since_epoch()).count(); } - ValuePair ValuePairHolder::get() + ErrorPair ErrorPairHolder::get() { std::lock_guard lock(mutex); return value; diff --git a/src/Common/ErrorCodes.h b/src/Common/ErrorCodes.h index 1c8f0a58884..edb9be9e0c0 100644 --- a/src/Common/ErrorCodes.h +++ b/src/Common/ErrorCodes.h @@ -24,31 +24,37 @@ namespace ErrorCodes /// Returns statically allocated string. std::string_view getName(ErrorCode error_code); - struct ValuePair + struct Error { - Value local = 0; - Value remote = 0; + /// Number of times Exception with this ErrorCode had been throw. + Value count; + /// Time of the last error. UInt64 error_time_ms = 0; + /// Message for the last error. std::string message; + /// Stacktrace for the last error. std::string stacktrace; - - ValuePair & operator+=(const ValuePair & value); + }; + struct ErrorPair + { + Error local; + Error remote; }; /// Thread-safe - struct ValuePairHolder + struct ErrorPairHolder { public: - void increment(const ValuePair & value_); - ValuePair get(); + ErrorPair get(); + void increment(bool remote, const std::string & message, const std::string & stacktrace); private: - ValuePair value; + ErrorPair value; std::mutex mutex; }; /// ErrorCode identifier -> current value of error_code. - extern ValuePairHolder values[]; + extern ErrorPairHolder values[]; /// Get index just after last error_code identifier. ErrorCode end(); diff --git a/src/Storages/System/StorageSystemErrors.cpp b/src/Storages/System/StorageSystemErrors.cpp index c9aac9ce007..5243cb11aa3 100644 --- a/src/Storages/System/StorageSystemErrors.cpp +++ b/src/Storages/System/StorageSystemErrors.cpp @@ -24,17 +24,17 @@ NamesAndTypesList StorageSystemErrors::getNamesAndTypes() void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & context, const SelectQueryInfo &) const { - auto add_row = [&](std::string_view name, size_t code, size_t value, UInt64 error_time_ms, const std::string & message, const std::string & stacktrace, bool remote) + auto add_row = [&](std::string_view name, size_t code, const auto & error, bool remote) { - if (value || context.getSettingsRef().system_events_show_zero_values) + if (error.count || context.getSettingsRef().system_events_show_zero_values) { size_t col_num = 0; res_columns[col_num++]->insert(name); res_columns[col_num++]->insert(code); - res_columns[col_num++]->insert(value); - res_columns[col_num++]->insert(error_time_ms / 1000); - res_columns[col_num++]->insert(message); - res_columns[col_num++]->insert(stacktrace); + res_columns[col_num++]->insert(error.count); + res_columns[col_num++]->insert(error.error_time_ms / 1000); + res_columns[col_num++]->insert(error.message); + res_columns[col_num++]->insert(error.stacktrace); res_columns[col_num++]->insert(remote); } }; @@ -47,8 +47,8 @@ void StorageSystemErrors::fillData(MutableColumns & res_columns, const Context & if (name.empty()) continue; - add_row(name, i, error.local, error.error_time_ms, error.message, error.stacktrace, false /* remote=0 */); - add_row(name, i, error.remote, error.error_time_ms, error.message, error.stacktrace, true /* remote=1 */); + add_row(name, i, error.local, /* remote= */ false); + add_row(name, i, error.remote, /* remote= */ true); } }