From 106b9717cd32833926a9bc1115fa51a3d35209f0 Mon Sep 17 00:00:00 2001 From: alesapin Date: Tue, 27 Aug 2019 16:14:19 +0300 Subject: [PATCH 01/35] Refactoring of immutable settings --- .../performance-test/PerformanceTestInfo.cpp | 2 +- dbms/src/Core/SettingsCommon.h | 91 +++++++++---------- dbms/src/Interpreters/Context.cpp | 2 +- dbms/src/Storages/Kafka/KafkaSettings.cpp | 2 +- dbms/src/Storages/MergeTree/MergeTreeData.cpp | 2 +- .../Storages/MergeTree/MergeTreeSettings.cpp | 2 +- .../MergeTree/registerStorageMergeTree.cpp | 3 + 7 files changed, 51 insertions(+), 53 deletions(-) diff --git a/dbms/programs/performance-test/PerformanceTestInfo.cpp b/dbms/programs/performance-test/PerformanceTestInfo.cpp index f335a16e0ff..40a066aa0a7 100644 --- a/dbms/programs/performance-test/PerformanceTestInfo.cpp +++ b/dbms/programs/performance-test/PerformanceTestInfo.cpp @@ -97,7 +97,7 @@ void PerformanceTestInfo::applySettings(XMLConfigurationPtr config) } extractSettings(config, "settings", config_settings, settings_to_apply); - settings.loadFromChanges(settings_to_apply); + settings.applyChanges(settings_to_apply); if (settings_contain("average_rows_speed_precision")) TestStats::avg_rows_speed_precision = diff --git a/dbms/src/Core/SettingsCommon.h b/dbms/src/Core/SettingsCommon.h index b8c56d50caa..de30f43c7c8 100644 --- a/dbms/src/Core/SettingsCommon.h +++ b/dbms/src/Core/SettingsCommon.h @@ -34,6 +34,7 @@ struct SettingNumber { Type value; bool changed = false; + bool immutable = false; SettingNumber(Type x = 0) : value(x) {} @@ -76,6 +77,7 @@ struct SettingMaxThreads UInt64 value; bool is_auto; bool changed = false; + bool immutable = false; SettingMaxThreads(UInt64 x = 0) : value(x ? x : getAutoValue()), is_auto(x == 0) {} @@ -104,6 +106,7 @@ struct SettingTimespan { Poco::Timespan value; bool changed = false; + bool immutable = false; SettingTimespan(UInt64 x = 0) : value(x * microseconds_per_io_unit) {} @@ -136,6 +139,7 @@ struct SettingString { String value; bool changed = false; + bool immutable = false; SettingString(const String & x = String{}) : value(x) {} @@ -158,6 +162,7 @@ struct SettingChar public: char value; bool changed = false; + bool immutable = false; SettingChar(char x = '\0') : value(x) {} @@ -182,6 +187,7 @@ struct SettingEnum { EnumType value; bool changed = false; + bool immutable = false; SettingEnum(EnumType x) : value(x) {} @@ -316,16 +322,17 @@ private: using SerializeFunction = void (*)(const Derived &, WriteBuffer & buf); using DeserializeFunction = void (*)(Derived &, ReadBuffer & buf); using CastValueWithoutApplyingFunction = Field (*)(const Field &); + using SetImmutable = void(*)(Derived &); + using IsImmutable = bool(*)(const Derived &); + struct MemberInfo { IsChangedFunction is_changed; StringRef name; StringRef description; - /// Can be updated after first load for config/definition. - /// Non updatable settings can be `changed`, - /// if they were overwritten in config/definition. - const bool updateable; + /// At one moment this setting can became immutable + const bool can_be_immutable; GetStringFunction get_string; GetFieldFunction get_field; SetStringFunction set_string; @@ -333,6 +340,8 @@ private: SerializeFunction serialize; DeserializeFunction deserialize; CastValueWithoutApplyingFunction cast_value_without_applying; + SetImmutable set_immutable; + IsImmutable is_immutable; bool isChanged(const Derived & collection) const { return is_changed(collection); } }; @@ -405,7 +414,6 @@ public: const_reference(const const_reference & src) = default; const StringRef & getName() const { return member->name; } const StringRef & getDescription() const { return member->description; } - bool isUpdateable() const { return member->updateable; } bool isChanged() const { return member->isChanged(*collection); } Field getValue() const { return member->get_field(*collection); } String getValueAsString() const { return member->get_string(*collection); } @@ -423,20 +431,20 @@ public: public: reference(Derived & collection_, const MemberInfo & member_) : const_reference(collection_, member_) {} reference(const const_reference & src) : const_reference(src) {} - void setValue(const Field & value) { this->member->set_field(*const_cast(this->collection), value); } - void setValue(const String & value) { this->member->set_string(*const_cast(this->collection), value); } - void updateValue(const Field & value) + void setValue(const Field & value) { - if (!this->member->updateable) + if (this->member->is_immutable(*this->collection)) throw Exception("Setting '" + this->member->name.toString() + "' is restricted for updates.", ErrorCodes::IMMUTABLE_SETTING); - setValue(value); + this->member->set_field(*const_cast(this->collection), value); } - void updateValue(const String & value) + void setValue(const String & value) { - if (!this->member->updateable) + if (this->member->is_immutable(*this->collection)) throw Exception("Setting '" + this->member->name.toString() + "' is restricted for updates.", ErrorCodes::IMMUTABLE_SETTING); - setValue(value); + this->member->set_string(*const_cast(this->collection), value); } + bool canBeImmutable() const { return this->member->can_be_immutable; } + void makeImmutableForever() { this->member->set_immutable(*const_cast(this->collection)); } }; /// Iterator to iterating through all the settings. @@ -519,15 +527,6 @@ public: void set(size_t index, const String & value) { (*this)[index].setValue(value); } void set(const String & name, const String & value) { (*this)[name].setValue(value); } - /// Updates setting's value. Checks it' mutability. - void update(size_t index, const Field & value) { (*this)[index].updateValue(value); } - - void update(const String & name, const Field & value) { (*this)[name].updateValue(value); } - - void update(size_t index, const String & value) { (*this)[index].updateValue(value); } - - void update(const String & name, const String & value) { (*this)[name].updateValue(value); } - /// Returns value of a setting. Field get(size_t index) const { return (*this)[index].getValue(); } Field get(const String & name) const { return (*this)[name].getValue(); } @@ -591,35 +590,19 @@ public: return found_changes; } - /// Applies change to the settings. Doesn't check settings mutability. - void loadFromChange(const SettingChange & change) + /// Applies change to concrete setting. + void applyChange(const SettingChange & change) { set(change.name, change.value); } - /// Applies changes to the settings. Should be used in initial settings loading. - /// (on table creation or loading from config) - void loadFromChanges(const SettingsChanges & changes) + /// Applies changes to the settings. + void applyChanges(const SettingsChanges & changes) { for (const SettingChange & change : changes) - loadFromChange(change); + applyChange(change); } - /// Applies change to the settings, checks settings mutability. - void updateFromChange(const SettingChange & change) - { - update(change.name, change.value); - } - - /// Applies changes to the settings. Should be used for settigns update. - /// (ALTER MODIFY SETTINGS) - void updateFromChanges(const SettingsChanges & changes) - { - for (const SettingChange & change : changes) - updateFromChange(change); - } - - void copyChangesFrom(const Derived & src) { for (const auto & member : members()) @@ -632,6 +615,14 @@ public: dest.copyChangesFrom(castToDerived()); } + /// Make all possible immutable settings (can_be_immutable) immutable forever + void finishSettingsInitialization() + { + for (auto & member : *this) + if (member.canBeImmutable()) + member.makeImmutableForever(); + } + /// Writes the settings to buffer (e.g. to be sent to remote server). /// Only changed settings are written. They are written as list of contiguous name-value pairs, /// finished with empty name. @@ -690,22 +681,26 @@ public: static void NAME##_setField(Derived & collection, const Field & value) { collection.NAME.set(value); } \ static void NAME##_serialize(const Derived & collection, WriteBuffer & buf) { collection.NAME.serialize(buf); } \ static void NAME##_deserialize(Derived & collection, ReadBuffer & buf) { collection.NAME.deserialize(buf); } \ - static Field NAME##_castValueWithoutApplying(const Field & value) { TYPE temp{DEFAULT}; temp.set(value); return temp.toField(); } + static Field NAME##_castValueWithoutApplying(const Field & value) { TYPE temp{DEFAULT}; temp.set(value); return temp.toField(); } \ + static void NAME##_setImmutable(Derived & collection) { collection.NAME.immutable = true; } \ + static bool NAME##_isImmutable(const Derived & collection) { return collection.NAME.immutable; } #define IMPLEMENT_SETTINGS_COLLECTION_ADD_MUTABLE_MEMBER_INFO_HELPER_(TYPE, NAME, DEFAULT, DESCRIPTION) \ add({[](const Derived & d) { return d.NAME.changed; }, \ - StringRef(#NAME, strlen(#NAME)), StringRef(#DESCRIPTION, strlen(#DESCRIPTION)), true, \ + StringRef(#NAME, strlen(#NAME)), StringRef(#DESCRIPTION, strlen(#DESCRIPTION)), false, \ &Functions::NAME##_getString, &Functions::NAME##_getField, \ &Functions::NAME##_setString, &Functions::NAME##_setField, \ &Functions::NAME##_serialize, &Functions::NAME##_deserialize, \ - &Functions::NAME##_castValueWithoutApplying }); + &Functions::NAME##_castValueWithoutApplying, \ + &Functions::NAME##_setImmutable, &Functions::NAME##_isImmutable }); #define IMPLEMENT_SETTINGS_COLLECTION_ADD_IMMUTABLE_MEMBER_INFO_HELPER_(TYPE, NAME, DEFAULT, DESCRIPTION) \ add({[](const Derived & d) { return d.NAME.changed; }, \ - StringRef(#NAME, strlen(#NAME)), StringRef(#DESCRIPTION, strlen(#DESCRIPTION)), false, \ + StringRef(#NAME, strlen(#NAME)), StringRef(#DESCRIPTION, strlen(#DESCRIPTION)), true, \ &Functions::NAME##_getString, &Functions::NAME##_getField, \ &Functions::NAME##_setString, &Functions::NAME##_setField, \ &Functions::NAME##_serialize, &Functions::NAME##_deserialize, \ - &Functions::NAME##_castValueWithoutApplying }); + &Functions::NAME##_castValueWithoutApplying, \ + &Functions::NAME##_setImmutable, &Functions::NAME##_isImmutable }); } diff --git a/dbms/src/Interpreters/Context.cpp b/dbms/src/Interpreters/Context.cpp index 016f1fa0e49..9aad83b4623 100644 --- a/dbms/src/Interpreters/Context.cpp +++ b/dbms/src/Interpreters/Context.cpp @@ -1132,7 +1132,7 @@ void Context::updateSettingsChanges(const SettingsChanges & changes) if (change.name == "profile") setProfile(change.value.safeGet()); else - settings.updateFromChange(change); + settings.applyChange(change); } } diff --git a/dbms/src/Storages/Kafka/KafkaSettings.cpp b/dbms/src/Storages/Kafka/KafkaSettings.cpp index b08d45780bb..d08282a9794 100644 --- a/dbms/src/Storages/Kafka/KafkaSettings.cpp +++ b/dbms/src/Storages/Kafka/KafkaSettings.cpp @@ -22,7 +22,7 @@ void KafkaSettings::loadFromQuery(ASTStorage & storage_def) { try { - loadFromChanges(storage_def.settings->changes); + applyChanges(storage_def.settings->changes); } catch (Exception & e) { diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.cpp b/dbms/src/Storages/MergeTree/MergeTreeData.cpp index b2d4a4b9d73..f8f915cadef 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeData.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreeData.cpp @@ -1652,7 +1652,7 @@ void MergeTreeData::changeSettings( if (!new_changes.empty()) { MergeTreeSettings copy = *getSettings(); - copy.updateFromChanges(new_changes); + copy.applyChanges(new_changes); storage_settings.set(std::make_unique(copy)); } } diff --git a/dbms/src/Storages/MergeTree/MergeTreeSettings.cpp b/dbms/src/Storages/MergeTree/MergeTreeSettings.cpp index 9eee33554ab..224c094dbfb 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeSettings.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreeSettings.cpp @@ -46,7 +46,7 @@ void MergeTreeSettings::loadFromQuery(ASTStorage & storage_def) { try { - loadFromChanges(storage_def.settings->changes); + applyChanges(storage_def.settings->changes); } catch (Exception & e) { diff --git a/dbms/src/Storages/MergeTree/registerStorageMergeTree.cpp b/dbms/src/Storages/MergeTree/registerStorageMergeTree.cpp index 674116a54bc..596ea4f5d0b 100644 --- a/dbms/src/Storages/MergeTree/registerStorageMergeTree.cpp +++ b/dbms/src/Storages/MergeTree/registerStorageMergeTree.cpp @@ -637,6 +637,9 @@ static StoragePtr create(const StorageFactory::Arguments & args) throw Exception("You must set the setting `allow_experimental_data_skipping_indices` to 1 " \ "before using data skipping indices.", ErrorCodes::BAD_ARGUMENTS); + /// Finalize settings and disable updates + storage_settings->finishSettingsInitialization(); + if (replicated) return StorageReplicatedMergeTree::create( zookeeper_path, replica_name, args.attach, args.data_path, args.database_name, args.table_name, From 39d50b5144b7d3f11eed2fb9b883e0f4c38d8253 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Wed, 28 Aug 2019 22:01:52 +0300 Subject: [PATCH 02/35] Remove mimalloc --- .gitmodules | 3 - CMakeLists.txt | 1 - cmake/find_mimalloc.cmake | 17 --- dbms/CMakeLists.txt | 5 - dbms/src/Common/MiAllocator.cpp | 70 ------------ dbms/src/Common/MiAllocator.h | 27 ----- dbms/src/Common/config.h.in | 1 - dbms/src/Common/tests/CMakeLists.txt | 3 - dbms/src/Common/tests/mi_malloc_test.cpp | 118 -------------------- dbms/src/DataStreams/MarkInCompressedFile.h | 9 +- dbms/src/IO/UncompressedCache.h | 9 -- 11 files changed, 1 insertion(+), 262 deletions(-) delete mode 100644 cmake/find_mimalloc.cmake delete mode 100644 dbms/src/Common/MiAllocator.cpp delete mode 100644 dbms/src/Common/MiAllocator.h delete mode 100644 dbms/src/Common/tests/mi_malloc_test.cpp diff --git a/.gitmodules b/.gitmodules index f6990fed41f..e5be5438cc7 100644 --- a/.gitmodules +++ b/.gitmodules @@ -97,9 +97,6 @@ [submodule "contrib/rapidjson"] path = contrib/rapidjson url = https://github.com/Tencent/rapidjson -[submodule "contrib/mimalloc"] - path = contrib/mimalloc - url = https://github.com/ClickHouse-Extras/mimalloc [submodule "contrib/fastops"] path = contrib/fastops url = https://github.com/ClickHouse-Extras/fastops diff --git a/CMakeLists.txt b/CMakeLists.txt index f84a181a39c..e181bdbc2af 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -476,7 +476,6 @@ include (cmake/find_consistent-hashing.cmake) include (cmake/find_base64.cmake) include (cmake/find_parquet.cmake) include (cmake/find_hyperscan.cmake) -include (cmake/find_mimalloc.cmake) include (cmake/find_simdjson.cmake) include (cmake/find_rapidjson.cmake) include (cmake/find_fastops.cmake) diff --git a/cmake/find_mimalloc.cmake b/cmake/find_mimalloc.cmake deleted file mode 100644 index 1820421379f..00000000000 --- a/cmake/find_mimalloc.cmake +++ /dev/null @@ -1,17 +0,0 @@ -if (OS_LINUX AND NOT SANITIZE AND NOT ARCH_ARM AND NOT ARCH_32 AND NOT ARCH_PPC64LE) - option (ENABLE_MIMALLOC "Set to FALSE to disable usage of mimalloc for internal ClickHouse caches" FALSE) -endif () - -if (NOT EXISTS "${ClickHouse_SOURCE_DIR}/contrib/mimalloc/include/mimalloc.h") - message (WARNING "submodule contrib/mimalloc is missing. to fix try run: \n git submodule update --init --recursive") - return() -endif () - -if (ENABLE_MIMALLOC) - message (FATAL_ERROR "Mimalloc is not production ready. (Disable with cmake -D ENABLE_MIMALLOC=0). If you want to use mimalloc, you must manually remove this message.") - - set (MIMALLOC_INCLUDE_DIR ${ClickHouse_SOURCE_DIR}/contrib/mimalloc/include) - set (USE_MIMALLOC 1) - set (MIMALLOC_LIBRARY mimalloc-static) - message (STATUS "Using mimalloc: ${MIMALLOC_INCLUDE_DIR} : ${MIMALLOC_LIBRARY}") -endif () diff --git a/dbms/CMakeLists.txt b/dbms/CMakeLists.txt index 355c66902a8..af59af7642b 100644 --- a/dbms/CMakeLists.txt +++ b/dbms/CMakeLists.txt @@ -266,11 +266,6 @@ if(RE2_INCLUDE_DIR) target_include_directories(clickhouse_common_io SYSTEM BEFORE PUBLIC ${RE2_INCLUDE_DIR}) endif() -if (USE_MIMALLOC) - target_include_directories (clickhouse_common_io SYSTEM BEFORE PUBLIC ${MIMALLOC_INCLUDE_DIR}) - target_link_libraries (clickhouse_common_io PRIVATE ${MIMALLOC_LIBRARY}) -endif () - if(CPUID_LIBRARY) target_link_libraries(clickhouse_common_io PRIVATE ${CPUID_LIBRARY}) endif() diff --git a/dbms/src/Common/MiAllocator.cpp b/dbms/src/Common/MiAllocator.cpp deleted file mode 100644 index 04e61a5de16..00000000000 --- a/dbms/src/Common/MiAllocator.cpp +++ /dev/null @@ -1,70 +0,0 @@ -#include "MiAllocator.h" - -#if USE_MIMALLOC -#include - -#include -#include -#include - -namespace DB -{ -namespace ErrorCodes -{ - extern const int CANNOT_ALLOCATE_MEMORY; -} - -void * MiAllocator::alloc(size_t size, size_t alignment) -{ - void * ptr; - if (alignment == 0) - { - ptr = mi_malloc(size); - if (!ptr) - DB::throwFromErrno("MiAllocator: Cannot allocate in mimalloc " + formatReadableSizeWithBinarySuffix(size) + ".", DB::ErrorCodes::CANNOT_ALLOCATE_MEMORY); - } - else - { - ptr = mi_malloc_aligned(size, alignment); - if (!ptr) - DB::throwFromErrno("MiAllocator: Cannot allocate in mimalloc (mi_malloc_aligned) " + formatReadableSizeWithBinarySuffix(size) + " with alignment " + toString(alignment) + ".", DB::ErrorCodes::CANNOT_ALLOCATE_MEMORY); - } - return ptr; -} - -void MiAllocator::free(void * buf, size_t) -{ - mi_free(buf); -} - -void * MiAllocator::realloc(void * old_ptr, size_t, size_t new_size, size_t alignment) -{ - if (old_ptr == nullptr) - return alloc(new_size, alignment); - - if (new_size == 0) - { - mi_free(old_ptr); - return nullptr; - } - - void * ptr; - - if (alignment == 0) - { - ptr = mi_realloc(old_ptr, alignment); - if (!ptr) - DB::throwFromErrno("MiAllocator: Cannot reallocate in mimalloc " + formatReadableSizeWithBinarySuffix(size) + ".", DB::ErrorCodes::CANNOT_ALLOCATE_MEMORY); - } - else - { - ptr = mi_realloc_aligned(old_ptr, new_size, alignment); - if (!ptr) - DB::throwFromErrno("MiAllocator: Cannot reallocate in mimalloc (mi_realloc_aligned) " + formatReadableSizeWithBinarySuffix(size) + " with alignment " + toString(alignment) + ".", DB::ErrorCodes::CANNOT_ALLOCATE_MEMORY); - } - return ptr; -} - -} - -#endif diff --git a/dbms/src/Common/MiAllocator.h b/dbms/src/Common/MiAllocator.h deleted file mode 100644 index 127be82434b..00000000000 --- a/dbms/src/Common/MiAllocator.h +++ /dev/null @@ -1,27 +0,0 @@ -#pragma once - -#include - -#if USE_MIMALLOC -#include - -namespace DB -{ - -/* - * This is a different allocator that is based on mimalloc (Microsoft malloc). - * It can be used separately from main allocator to catch heap corruptions and vulnerabilities (for example, for caches). - * We use MI_SECURE mode in mimalloc to achieve such behaviour. - */ -struct MiAllocator -{ - static void * alloc(size_t size, size_t alignment = 0); - - static void free(void * buf, size_t); - - static void * realloc(void * old_ptr, size_t, size_t new_size, size_t alignment = 0); -}; - -} - -#endif diff --git a/dbms/src/Common/config.h.in b/dbms/src/Common/config.h.in index b6a5b6de2b8..7804068e5c4 100644 --- a/dbms/src/Common/config.h.in +++ b/dbms/src/Common/config.h.in @@ -8,6 +8,5 @@ #cmakedefine01 USE_CPUID #cmakedefine01 USE_CPUINFO #cmakedefine01 USE_BROTLI -#cmakedefine01 USE_MIMALLOC #cmakedefine01 USE_UNWIND #cmakedefine01 CLICKHOUSE_SPLIT_BINARY diff --git a/dbms/src/Common/tests/CMakeLists.txt b/dbms/src/Common/tests/CMakeLists.txt index 2c99c85baec..67c0e376f74 100644 --- a/dbms/src/Common/tests/CMakeLists.txt +++ b/dbms/src/Common/tests/CMakeLists.txt @@ -76,8 +76,5 @@ target_link_libraries (cow_compositions PRIVATE clickhouse_common_io) add_executable (stopwatch stopwatch.cpp) target_link_libraries (stopwatch PRIVATE clickhouse_common_io) -add_executable (mi_malloc_test mi_malloc_test.cpp) -target_link_libraries (mi_malloc_test PRIVATE clickhouse_common_io) - add_executable (symbol_index symbol_index.cpp) target_link_libraries (symbol_index PRIVATE clickhouse_common_io) diff --git a/dbms/src/Common/tests/mi_malloc_test.cpp b/dbms/src/Common/tests/mi_malloc_test.cpp deleted file mode 100644 index ce1e4a3a770..00000000000 --- a/dbms/src/Common/tests/mi_malloc_test.cpp +++ /dev/null @@ -1,118 +0,0 @@ -/** In addition to ClickHouse (Apache 2) license, this file can be also used under MIT license: - -MIT License - -Copyright (c) 2019 Yandex LLC, Alexey Milovidov - -Permission is hereby granted, free of charge, to any person obtaining a copy -of this software and associated documentation files (the "Software"), to deal -in the Software without restriction, including without limitation the rights -to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -copies of the Software, and to permit persons to whom the Software is -furnished to do so, subject to the following conditions: - -The above copyright notice and this permission notice shall be included in all -copies or substantial portions of the Software. - -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -SOFTWARE. - -*/ - -#include -#include -#include -#include -#include -#include - -#include - -//#undef USE_MIMALLOC -//#define USE_MIMALLOC 0 - -#if USE_MIMALLOC - -#include -#define malloc mi_malloc -#define free mi_free - -#else - -#include - -#endif - - -size_t total_size{0}; - -struct Allocation -{ - void * ptr = nullptr; - size_t size = 0; - - Allocation() {} - - Allocation(size_t size_) - : size(size_) - { - ptr = malloc(size); - if (!ptr) - throw std::runtime_error("Cannot allocate memory"); - total_size += size; - } - - ~Allocation() - { - if (ptr) - { - free(ptr); - total_size -= size; - } - ptr = nullptr; - } - - Allocation(const Allocation &) = delete; - - Allocation(Allocation && rhs) - { - ptr = rhs.ptr; - size = rhs.size; - rhs.ptr = nullptr; - rhs.size = 0; - } -}; - - -int main(int, char **) -{ - std::vector allocations; - - constexpr size_t limit = 100000000; - constexpr size_t min_alloc_size = 65536; - constexpr size_t max_alloc_size = 10000000; - - std::mt19937 rng; - auto distribution = std::uniform_int_distribution(min_alloc_size, max_alloc_size); - - size_t total_allocations = 0; - - while (true) - { - size_t size = distribution(rng); - - while (total_size + size > limit) - allocations.pop_back(); - - allocations.emplace_back(size); - - ++total_allocations; - if (total_allocations % (1ULL << 20) == 0) - std::cerr << "Total allocations: " << total_allocations << "\n"; - } -} diff --git a/dbms/src/DataStreams/MarkInCompressedFile.h b/dbms/src/DataStreams/MarkInCompressedFile.h index a5970a89738..62fe8eedf76 100644 --- a/dbms/src/DataStreams/MarkInCompressedFile.h +++ b/dbms/src/DataStreams/MarkInCompressedFile.h @@ -6,10 +6,6 @@ #include #include -#include -#if USE_MIMALLOC -#include -#endif namespace DB { @@ -43,9 +39,6 @@ struct MarkInCompressedFile } }; -#if USE_MIMALLOC -using MarksInCompressedFile = PODArray; -#else + using MarksInCompressedFile = PODArray; -#endif } diff --git a/dbms/src/IO/UncompressedCache.h b/dbms/src/IO/UncompressedCache.h index 1f17c5e61b6..86f1530e5b3 100644 --- a/dbms/src/IO/UncompressedCache.h +++ b/dbms/src/IO/UncompressedCache.h @@ -6,11 +6,6 @@ #include #include -#include -#if USE_MIMALLOC -#include -#endif - namespace ProfileEvents { @@ -25,11 +20,7 @@ namespace DB struct UncompressedCacheCell { -#if USE_MIMALLOC - Memory data; -#else Memory<> data; -#endif size_t compressed_size; UInt32 additional_bytes; }; From 549c61cad9c9df3566ce70d5a02b209c6af51856 Mon Sep 17 00:00:00 2001 From: alesapin Date: Thu, 29 Aug 2019 15:38:20 +0300 Subject: [PATCH 03/35] Update MarkInCompressedFile.h --- dbms/src/DataStreams/MarkInCompressedFile.h | 1 + 1 file changed, 1 insertion(+) diff --git a/dbms/src/DataStreams/MarkInCompressedFile.h b/dbms/src/DataStreams/MarkInCompressedFile.h index 62fe8eedf76..46d078f2b76 100644 --- a/dbms/src/DataStreams/MarkInCompressedFile.h +++ b/dbms/src/DataStreams/MarkInCompressedFile.h @@ -41,4 +41,5 @@ struct MarkInCompressedFile }; using MarksInCompressedFile = PODArray; + } From 260b8c7fa7baa21f4283206cf59dd5670b233759 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Thu, 29 Aug 2019 18:32:25 +0300 Subject: [PATCH 04/35] Removed obsolete directory --- contrib/mimalloc | 1 - 1 file changed, 1 deletion(-) delete mode 160000 contrib/mimalloc diff --git a/contrib/mimalloc b/contrib/mimalloc deleted file mode 160000 index a787bdebce9..00000000000 --- a/contrib/mimalloc +++ /dev/null @@ -1 +0,0 @@ -Subproject commit a787bdebce94bf3776dc0d1ad597917f479ab8d5 From a894288fa059ad06c325780aa780c49ae57b49da Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Thu, 29 Aug 2019 18:48:00 +0300 Subject: [PATCH 05/35] Fallback from O_DIRECT. --- dbms/src/Common/ProfileEvents.cpp | 2 + dbms/src/IO/createReadBufferFromFileBase.cpp | 40 ++++++++++-------- dbms/src/IO/createWriteBufferFromFileBase.cpp | 41 ++++++++++--------- 3 files changed, 46 insertions(+), 37 deletions(-) diff --git a/dbms/src/Common/ProfileEvents.cpp b/dbms/src/Common/ProfileEvents.cpp index c73e5422439..67303b085f4 100644 --- a/dbms/src/Common/ProfileEvents.cpp +++ b/dbms/src/Common/ProfileEvents.cpp @@ -36,8 +36,10 @@ M(MarkCacheMisses, "") \ M(CreatedReadBufferOrdinary, "") \ M(CreatedReadBufferAIO, "") \ + M(CreatedReadBufferAIOFailed, "") \ M(CreatedWriteBufferOrdinary, "") \ M(CreatedWriteBufferAIO, "") \ + M(CreatedWriteBufferAIOFailed, "") \ M(DiskReadElapsedMicroseconds, "Total time spent waiting for read syscall. This include reads from page cache.") \ M(DiskWriteElapsedMicroseconds, "Total time spent waiting for write syscall. This include writes to page cache.") \ M(NetworkReceiveElapsedMicroseconds, "") \ diff --git a/dbms/src/IO/createReadBufferFromFileBase.cpp b/dbms/src/IO/createReadBufferFromFileBase.cpp index 512f3dfd3ed..8fc5923e6ff 100644 --- a/dbms/src/IO/createReadBufferFromFileBase.cpp +++ b/dbms/src/IO/createReadBufferFromFileBase.cpp @@ -10,34 +10,38 @@ namespace ProfileEvents { extern const Event CreatedReadBufferOrdinary; extern const Event CreatedReadBufferAIO; + extern const Event CreatedReadBufferAIOFailed; } namespace DB { -#if !defined(__linux__) -namespace ErrorCodes -{ - extern const int NOT_IMPLEMENTED; -} -#endif std::unique_ptr createReadBufferFromFileBase(const std::string & filename_, size_t estimated_size, size_t aio_threshold, size_t buffer_size_, int flags_, char * existing_memory_, size_t alignment) { - if ((aio_threshold == 0) || (estimated_size < aio_threshold)) - { - ProfileEvents::increment(ProfileEvents::CreatedReadBufferOrdinary); - return std::make_unique(filename_, buffer_size_, flags_, existing_memory_, alignment); - } - else - { #if defined(__linux__) || defined(__FreeBSD__) - ProfileEvents::increment(ProfileEvents::CreatedReadBufferAIO); - return std::make_unique(filename_, buffer_size_, flags_, existing_memory_); -#else - throw Exception("AIO is implemented only on Linux and FreeBSD", ErrorCodes::NOT_IMPLEMENTED); -#endif + if (aio_threshold && estimated_size >= aio_threshold) + { + /// Attempt to open a file with O_DIRECT + try + { + auto res = std::make_unique(filename_, buffer_size_, flags_, existing_memory_); + ProfileEvents::increment(ProfileEvents::CreatedReadBufferAIO); + return res; + } + catch (const ErrnoException &) + { + /// Fallback to cached IO if O_DIRECT is not supported. + ProfileEvents::increment(ProfileEvents::CreatedReadBufferAIOFailed); + } } +#else + (void)aio_threshold; + (void)estimated_size; +#endif + + ProfileEvents::increment(ProfileEvents::CreatedReadBufferOrdinary); + return std::make_unique(filename_, buffer_size_, flags_, existing_memory_, alignment); } } diff --git a/dbms/src/IO/createWriteBufferFromFileBase.cpp b/dbms/src/IO/createWriteBufferFromFileBase.cpp index 32cbcb1b12e..d20af3ede76 100644 --- a/dbms/src/IO/createWriteBufferFromFileBase.cpp +++ b/dbms/src/IO/createWriteBufferFromFileBase.cpp @@ -10,36 +10,39 @@ namespace ProfileEvents { extern const Event CreatedWriteBufferOrdinary; extern const Event CreatedWriteBufferAIO; + extern const Event CreatedWriteBufferAIOFailed; } namespace DB { -#if !defined(__linux__) -namespace ErrorCodes -{ - extern const int NOT_IMPLEMENTED; -} -#endif - std::unique_ptr createWriteBufferFromFileBase(const std::string & filename_, size_t estimated_size, size_t aio_threshold, size_t buffer_size_, int flags_, mode_t mode, char * existing_memory_, size_t alignment) { - if ((aio_threshold == 0) || (estimated_size < aio_threshold)) - { - ProfileEvents::increment(ProfileEvents::CreatedWriteBufferOrdinary); - return std::make_unique(filename_, buffer_size_, flags_, mode, existing_memory_, alignment); - } - else - { #if defined(__linux__) || defined(__FreeBSD__) - ProfileEvents::increment(ProfileEvents::CreatedWriteBufferAIO); - return std::make_unique(filename_, buffer_size_, flags_, mode, existing_memory_); -#else - throw Exception("AIO is implemented only on Linux and FreeBSD", ErrorCodes::NOT_IMPLEMENTED); -#endif + if (aio_threshold && estimated_size >= aio_threshold) + { + /// Attempt to open a file with O_DIRECT + try + { + auto res = std::make_unique(filename_, buffer_size_, flags_, mode, existing_memory_); + ProfileEvents::increment(ProfileEvents::CreatedWriteBufferAIO); + return res; + } + catch (const ErrnoException &) + { + /// Fallback to cached IO if O_DIRECT is not supported. + ProfileEvents::increment(ProfileEvents::CreatedWriteBufferAIOFailed); + } } +#else + (void)aio_threshold; + (void)estimated_size; +#endif + + ProfileEvents::increment(ProfileEvents::CreatedWriteBufferOrdinary); + return std::make_unique(filename_, buffer_size_, flags_, mode, existing_memory_, alignment); } } From 0bca68e50b540608832b17202ceec6a2d7a5cba4 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Thu, 29 Aug 2019 21:55:20 +0300 Subject: [PATCH 06/35] Style --- dbms/src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp | 3 ++- dbms/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dbms/src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp b/dbms/src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp index 7079996af80..fdeb8c3ea96 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp @@ -561,7 +561,8 @@ MergeTreeData::MutableDataPartPtr MergeTreeDataMergerMutator::mergePartsToTempor NamesAndTypesList all_columns = data.getColumns().getAllPhysical(); const auto data_settings = data.getSettings(); - NamesAndTypesList gathering_columns, merging_columns; + NamesAndTypesList gathering_columns; + NamesAndTypesList merging_columns; Names gathering_column_names, merging_column_names; extractMergingAndGatheringColumns( all_columns, data.sorting_key_expr, data.skip_indices, diff --git a/dbms/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp b/dbms/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp index 3c15bd54df2..0a852e68b74 100644 --- a/dbms/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp +++ b/dbms/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp @@ -103,7 +103,6 @@ MergeTreeData::DataPart::Checksums MergedColumnOnlyOutputStream::writeSuffixAndG serialize_settings.getter = createStreamGetter(column.name, already_written_offset_columns, skip_offsets); column.type->serializeBinaryBulkStateSuffix(serialize_settings, serialization_states[i]); - if (with_final_mark) writeFinalMark(column.name, column.type, offset_columns, skip_offsets, serialize_settings.path); } From b66485a1d2e6f3bf76086ba99876fce58f37016e Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Fri, 30 Aug 2019 11:09:13 +0300 Subject: [PATCH 07/35] Reduced children_mutex lock scope in IBlockInputStream This is to fix TSan warning 'lock-order-inversion'. Thread locks IBlockInputStream::children_mutex (A) and then subsequently locks MergeTreeDataPart::columns_lock mutex (B), holding it for extended period of time, surviving the unlock of the A. Then, while B is still locked, A is locked again, causing a TSan warning. --- dbms/src/DataStreams/IBlockInputStream.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dbms/src/DataStreams/IBlockInputStream.h b/dbms/src/DataStreams/IBlockInputStream.h index 309eecbeebf..f33c4534a3f 100644 --- a/dbms/src/DataStreams/IBlockInputStream.h +++ b/dbms/src/DataStreams/IBlockInputStream.h @@ -314,7 +314,11 @@ private: /// NOTE: Acquire a read lock, therefore f() should be thread safe std::shared_lock lock(children_mutex); - for (auto & child : children) + // Reduce lock scope and avoid recursive locking since that is undefined for shared_mutex. + const auto children_copy = children; + lock.unlock(); + + for (auto & child : children_copy) if (f(*child)) return; } From da8f67123f8661dd33eb324ed9163b37f8719516 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Fri, 30 Aug 2019 12:50:38 +0300 Subject: [PATCH 08/35] Improve error handling in cache dictionaries: allow using expired values while the source of a cache dictionary doesn't respond; clone the source after an error to reset connections; don't ask the source for a little time after error; show the last exception in system.dictionaries for a cache dictionary too. --- dbms/src/Dictionaries/CacheDictionary.cpp | 7 + dbms/src/Dictionaries/CacheDictionary.h | 10 +- dbms/src/Dictionaries/CacheDictionary.inc.h | 165 +++++++++++------- dbms/src/Dictionaries/IDictionary.h | 2 + dbms/src/Interpreters/ExternalLoader.cpp | 18 +- dbms/src/Interpreters/ExternalLoader.h | 15 +- dbms/src/Interpreters/IExternalLoadable.cpp | 11 +- dbms/src/Interpreters/IExternalLoadable.h | 12 ++ .../System/StorageSystemDictionaries.cpp | 14 +- 9 files changed, 163 insertions(+), 91 deletions(-) diff --git a/dbms/src/Dictionaries/CacheDictionary.cpp b/dbms/src/Dictionaries/CacheDictionary.cpp index 53fc746e565..c3a78150f05 100644 --- a/dbms/src/Dictionaries/CacheDictionary.cpp +++ b/dbms/src/Dictionaries/CacheDictionary.cpp @@ -70,6 +70,7 @@ CacheDictionary::CacheDictionary( , dict_struct(dict_struct_) , source_ptr{std::move(source_ptr_)} , dict_lifetime(dict_lifetime_) + , log(&Logger::get("ExternalDictionaries")) , size{roundUpToPowerOfTwoOrZero(std::max(size_, size_t(max_collision_length)))} , size_overlap_mask{this->size - 1} , cells{this->size} @@ -575,6 +576,12 @@ BlockInputStreamPtr CacheDictionary::getBlockInputStream(const Names & column_na return std::make_shared(shared_from_this(), max_block_size, getCachedIds(), column_names); } +std::exception_ptr CacheDictionary::getLastException() const +{ + const ProfilingScopedReadRWLock read_lock{rw_lock, ProfileEvents::DictCacheLockReadNs}; + return last_exception; +} + void registerDictionaryCache(DictionaryFactory & factory) { auto create_layout = [=](const std::string & name, diff --git a/dbms/src/Dictionaries/CacheDictionary.h b/dbms/src/Dictionaries/CacheDictionary.h index 7e1cec6ffe9..750c51a7cf3 100644 --- a/dbms/src/Dictionaries/CacheDictionary.h +++ b/dbms/src/Dictionaries/CacheDictionary.h @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -74,6 +75,8 @@ public: void isInVectorConstant(const PaddedPODArray & child_ids, const Key ancestor_id, PaddedPODArray & out) const override; void isInConstantVector(const Key child_id, const PaddedPODArray & ancestor_ids, PaddedPODArray & out) const override; + std::exception_ptr getLastException() const override; + template using ResultArrayType = std::conditional_t, DecimalPaddedPODArray, PaddedPODArray>; @@ -253,8 +256,9 @@ private: const std::string name; const DictionaryStructure dict_struct; - const DictionarySourcePtr source_ptr; + mutable DictionarySourcePtr source_ptr; const DictionaryLifetime dict_lifetime; + Logger * const log; mutable std::shared_mutex rw_lock; @@ -274,6 +278,10 @@ private: Attribute * hierarchical_attribute = nullptr; std::unique_ptr string_arena; + mutable std::exception_ptr last_exception; + mutable size_t error_count = 0; + mutable std::chrono::system_clock::time_point backoff_end_time; + mutable pcg64 rnd_engine; mutable size_t bytes_allocated = 0; diff --git a/dbms/src/Dictionaries/CacheDictionary.inc.h b/dbms/src/Dictionaries/CacheDictionary.inc.h index 39e1a4b00df..51d515a63dd 100644 --- a/dbms/src/Dictionaries/CacheDictionary.inc.h +++ b/dbms/src/Dictionaries/CacheDictionary.inc.h @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -243,77 +244,102 @@ template void CacheDictionary::update( const std::vector & requested_ids, PresentIdHandler && on_cell_updated, AbsentIdHandler && on_id_not_found) const { + CurrentMetrics::Increment metric_increment{CurrentMetrics::DictCacheRequests}; + ProfileEvents::increment(ProfileEvents::DictCacheKeysRequested, requested_ids.size()); + std::unordered_map remaining_ids{requested_ids.size()}; for (const auto id : requested_ids) remaining_ids.insert({id, 0}); - std::uniform_int_distribution distribution{dict_lifetime.min_sec, dict_lifetime.max_sec}; + const auto now = std::chrono::system_clock::now(); const ProfilingScopedWriteRWLock write_lock{rw_lock, ProfileEvents::DictCacheLockWriteNs}; + if (now > backoff_end_time) { - CurrentMetrics::Increment metric_increment{CurrentMetrics::DictCacheRequests}; - Stopwatch watch; - auto stream = source_ptr->loadIds(requested_ids); - stream->readPrefix(); - - const auto now = std::chrono::system_clock::now(); - - while (const auto block = stream->read()) + try { - const auto id_column = typeid_cast(block.safeGetByPosition(0).column.get()); - if (!id_column) - throw Exception{name + ": id column has type different from UInt64.", ErrorCodes::TYPE_MISMATCH}; - - const auto & ids = id_column->getData(); - - /// cache column pointers - const auto column_ptrs = ext::map( - ext::range(0, attributes.size()), [&block](size_t i) { return block.safeGetByPosition(i + 1).column.get(); }); - - for (const auto i : ext::range(0, ids.size())) + if (error_count) { - const auto id = ids[i]; - - const auto find_result = findCellIdx(id, now); - const auto & cell_idx = find_result.cell_idx; - - auto & cell = cells[cell_idx]; - - for (const auto attribute_idx : ext::range(0, attributes.size())) - { - const auto & attribute_column = *column_ptrs[attribute_idx]; - auto & attribute = attributes[attribute_idx]; - - setAttributeValue(attribute, cell_idx, attribute_column[i]); - } - - /// if cell id is zero and zero does not map to this cell, then the cell is unused - if (cell.id == 0 && cell_idx != zero_cell_idx) - element_count.fetch_add(1, std::memory_order_relaxed); - - cell.id = id; - if (dict_lifetime.min_sec != 0 && dict_lifetime.max_sec != 0) - cell.setExpiresAt(std::chrono::system_clock::now() + std::chrono::seconds{distribution(rnd_engine)}); - else - cell.setExpiresAt(std::chrono::time_point::max()); - - /// inform caller - on_cell_updated(id, cell_idx); - /// mark corresponding id as found - remaining_ids[id] = 1; + /// Recover after error: we have to clone the source here because + /// it could keep connections which should be reset after error. + source_ptr = source_ptr->clone(); } + + Stopwatch watch; + auto stream = source_ptr->loadIds(requested_ids); + stream->readPrefix(); + + while (const auto block = stream->read()) + { + const auto id_column = typeid_cast(block.safeGetByPosition(0).column.get()); + if (!id_column) + throw Exception{name + ": id column has type different from UInt64.", ErrorCodes::TYPE_MISMATCH}; + + const auto & ids = id_column->getData(); + + /// cache column pointers + const auto column_ptrs = ext::map( + ext::range(0, attributes.size()), [&block](size_t i) { return block.safeGetByPosition(i + 1).column.get(); }); + + for (const auto i : ext::range(0, ids.size())) + { + const auto id = ids[i]; + + const auto find_result = findCellIdx(id, now); + const auto & cell_idx = find_result.cell_idx; + + auto & cell = cells[cell_idx]; + + for (const auto attribute_idx : ext::range(0, attributes.size())) + { + const auto & attribute_column = *column_ptrs[attribute_idx]; + auto & attribute = attributes[attribute_idx]; + + setAttributeValue(attribute, cell_idx, attribute_column[i]); + } + + /// if cell id is zero and zero does not map to this cell, then the cell is unused + if (cell.id == 0 && cell_idx != zero_cell_idx) + element_count.fetch_add(1, std::memory_order_relaxed); + + cell.id = id; + if (dict_lifetime.min_sec != 0 && dict_lifetime.max_sec != 0) + { + std::uniform_int_distribution distribution{dict_lifetime.min_sec, dict_lifetime.max_sec}; + cell.setExpiresAt(now + std::chrono::seconds{distribution(rnd_engine)}); + } + else + cell.setExpiresAt(std::chrono::time_point::max()); + + /// inform caller + on_cell_updated(id, cell_idx); + /// mark corresponding id as found + remaining_ids[id] = 1; + } + } + + stream->readSuffix(); + + error_count = 0; + last_exception = std::exception_ptr{}; + backoff_end_time = std::chrono::system_clock::time_point{}; + + ProfileEvents::increment(ProfileEvents::DictCacheRequestTimeNs, watch.elapsed()); } + catch (...) + { + ++error_count; + last_exception = std::current_exception(); + backoff_end_time = now + std::chrono::seconds(ExternalLoadableBackoff{}.calculateDuration(rnd_engine, error_count)); - stream->readSuffix(); - - ProfileEvents::increment(ProfileEvents::DictCacheKeysRequested, requested_ids.size()); - ProfileEvents::increment(ProfileEvents::DictCacheRequestTimeNs, watch.elapsed()); + tryLogException(last_exception, log, "Could not update cache dictionary '" + getName() + + "', next update is scheduled at " + DateLUT::instance().timeToString(std::chrono::system_clock::to_time_t(backoff_end_time))); + } } size_t not_found_num = 0, found_num = 0; - const auto now = std::chrono::system_clock::now(); /// Check which ids have not been found and require setting null_value for (const auto & id_found_pair : remaining_ids) { @@ -328,24 +354,45 @@ void CacheDictionary::update( const auto find_result = findCellIdx(id, now); const auto & cell_idx = find_result.cell_idx; - auto & cell = cells[cell_idx]; - /// Set null_value for each attribute - for (auto & attribute : attributes) - setDefaultAttributeValue(attribute, cell_idx); + if (error_count) + { + if (find_result.outdated) + { + /// We have expired data for that `id` so we can continue using it. + bool was_default = cell.isDefault(); + cell.setExpiresAt(backoff_end_time); + if (was_default) + cell.setDefault(); + if (was_default) + on_id_not_found(id, cell_idx); + else + on_cell_updated(id, cell_idx); + continue; + } + /// We don't have expired data for that `id` so all we can do is to rethrow `last_exception`. + std::rethrow_exception(last_exception); + } /// Check if cell had not been occupied before and increment element counter if it hadn't if (cell.id == 0 && cell_idx != zero_cell_idx) element_count.fetch_add(1, std::memory_order_relaxed); cell.id = id; + if (dict_lifetime.min_sec != 0 && dict_lifetime.max_sec != 0) - cell.setExpiresAt(std::chrono::system_clock::now() + std::chrono::seconds{distribution(rnd_engine)}); + { + std::uniform_int_distribution distribution{dict_lifetime.min_sec, dict_lifetime.max_sec}; + cell.setExpiresAt(now + std::chrono::seconds{distribution(rnd_engine)}); + } else cell.setExpiresAt(std::chrono::time_point::max()); + /// Set null_value for each attribute cell.setDefault(); + for (auto & attribute : attributes) + setDefaultAttributeValue(attribute, cell_idx); /// inform caller that the cell has not been found on_id_not_found(id, cell_idx); diff --git a/dbms/src/Dictionaries/IDictionary.h b/dbms/src/Dictionaries/IDictionary.h index 05f9806dc04..a1c080ca6eb 100644 --- a/dbms/src/Dictionaries/IDictionary.h +++ b/dbms/src/Dictionaries/IDictionary.h @@ -56,6 +56,8 @@ struct IDictionaryBase : public IExternalLoadable return source && source->isModified(); } + virtual std::exception_ptr getLastException() const { return {}; } + std::shared_ptr shared_from_this() { return std::static_pointer_cast(IExternalLoadable::shared_from_this()); diff --git a/dbms/src/Interpreters/ExternalLoader.cpp b/dbms/src/Interpreters/ExternalLoader.cpp index cc8466bebc1..6e16fd37cba 100644 --- a/dbms/src/Interpreters/ExternalLoader.cpp +++ b/dbms/src/Interpreters/ExternalLoader.cpp @@ -1,6 +1,5 @@ #include "ExternalLoader.h" -#include #include #include #include @@ -933,6 +932,8 @@ private: class ExternalLoader::PeriodicUpdater : private boost::noncopyable { public: + static constexpr UInt64 check_period_sec = 5; + PeriodicUpdater(ConfigFilesReader & config_files_reader_, LoadingDispatcher & loading_dispatcher_) : config_files_reader(config_files_reader_), loading_dispatcher(loading_dispatcher_) { @@ -940,11 +941,10 @@ public: ~PeriodicUpdater() { enable(false); } - void enable(bool enable_, const ExternalLoaderUpdateSettings & settings_ = {}) + void enable(bool enable_) { std::unique_lock lock{mutex}; enabled = enable_; - settings = settings_; if (enable_) { @@ -985,9 +985,7 @@ public: return std::chrono::system_clock::now() + std::chrono::seconds{distribution(rnd_engine)}; } - std::uniform_int_distribution distribution(0, static_cast(std::exp2(error_count - 1))); - std::chrono::seconds delay(std::min(settings.backoff_max_sec, settings.backoff_initial_sec + distribution(rnd_engine))); - return std::chrono::system_clock::now() + delay; + return std::chrono::system_clock::now() + std::chrono::seconds(ExternalLoadableBackoff{}.calculateDuration(rnd_engine, error_count)); } private: @@ -996,9 +994,8 @@ private: setThreadName("ExterLdrReload"); std::unique_lock lock{mutex}; - auto timeout = [this] { return std::chrono::seconds(settings.check_period_sec); }; auto pred = [this] { return !enabled; }; - while (!event.wait_for(lock, timeout(), pred)) + while (!event.wait_for(lock, std::chrono::seconds(check_period_sec), pred)) { lock.unlock(); loading_dispatcher.setConfiguration(config_files_reader.read()); @@ -1012,7 +1009,6 @@ private: mutable std::mutex mutex; bool enabled = false; - ExternalLoaderUpdateSettings settings; ThreadFromGlobalPool thread; std::condition_variable event; mutable pcg64 rnd_engine{randomSeed()}; @@ -1051,9 +1047,9 @@ void ExternalLoader::enableAsyncLoading(bool enable) loading_dispatcher->enableAsyncLoading(enable); } -void ExternalLoader::enablePeriodicUpdates(bool enable_, const ExternalLoaderUpdateSettings & settings_) +void ExternalLoader::enablePeriodicUpdates(bool enable_) { - periodic_updater->enable(enable_, settings_); + periodic_updater->enable(enable_); } bool ExternalLoader::hasCurrentlyLoadedObjects() const diff --git a/dbms/src/Interpreters/ExternalLoader.h b/dbms/src/Interpreters/ExternalLoader.h index 8a52d991759..ecfc43c2dd9 100644 --- a/dbms/src/Interpreters/ExternalLoader.h +++ b/dbms/src/Interpreters/ExternalLoader.h @@ -11,19 +11,6 @@ namespace DB { -struct ExternalLoaderUpdateSettings -{ - UInt64 check_period_sec = 5; - UInt64 backoff_initial_sec = 5; - /// 10 minutes - UInt64 backoff_max_sec = 10 * 60; - - ExternalLoaderUpdateSettings() = default; - ExternalLoaderUpdateSettings(UInt64 check_period_sec_, UInt64 backoff_initial_sec_, UInt64 backoff_max_sec_) - : check_period_sec(check_period_sec_), backoff_initial_sec(backoff_initial_sec_), backoff_max_sec(backoff_max_sec_) {} -}; - - /* External configuration structure. * * @@ -105,7 +92,7 @@ public: void enableAsyncLoading(bool enable); /// Sets settings for periodic updates. - void enablePeriodicUpdates(bool enable, const ExternalLoaderUpdateSettings & settings = {}); + void enablePeriodicUpdates(bool enable); /// Returns the status of the object. /// If the object has not been loaded yet then the function returns Status::NOT_LOADED. diff --git a/dbms/src/Interpreters/IExternalLoadable.cpp b/dbms/src/Interpreters/IExternalLoadable.cpp index 37855490a9f..e8bf8cbaf3c 100644 --- a/dbms/src/Interpreters/IExternalLoadable.cpp +++ b/dbms/src/Interpreters/IExternalLoadable.cpp @@ -1,7 +1,7 @@ #include #include - +#include namespace DB { @@ -16,4 +16,13 @@ ExternalLoadableLifetime::ExternalLoadableLifetime(const Poco::Util::AbstractCon max_sec = has_min ? config.getUInt64(config_prefix + ".max") : min_sec; } + +UInt64 ExternalLoadableBackoff::calculateDuration(pcg64 & rnd_engine, size_t error_count) const +{ + if (error_count < 1) + error_count = 1; + std::uniform_int_distribution distribution(0, static_cast(std::exp2(error_count - 1))); + return std::min(backoff_max_sec, backoff_initial_sec + distribution(rnd_engine)); +} + } diff --git a/dbms/src/Interpreters/IExternalLoadable.h b/dbms/src/Interpreters/IExternalLoadable.h index f8725a67989..e842fdb8573 100644 --- a/dbms/src/Interpreters/IExternalLoadable.h +++ b/dbms/src/Interpreters/IExternalLoadable.h @@ -3,6 +3,7 @@ #include #include #include +#include #include @@ -25,6 +26,17 @@ struct ExternalLoadableLifetime }; +/// Delay before trying to load again after error. +struct ExternalLoadableBackoff +{ + UInt64 backoff_initial_sec = 5; + UInt64 backoff_max_sec = 10 * 60; /// 10 minutes + + /// Calculates time to try loading again after error. + UInt64 calculateDuration(pcg64 & rnd_engine, size_t error_count = 1) const; +}; + + /// Basic interface for external loadable objects. Is used in ExternalLoader. class IExternalLoadable : public std::enable_shared_from_this, private boost::noncopyable { diff --git a/dbms/src/Storages/System/StorageSystemDictionaries.cpp b/dbms/src/Storages/System/StorageSystemDictionaries.cpp index 6f85e1ea84b..826bb601609 100644 --- a/dbms/src/Storages/System/StorageSystemDictionaries.cpp +++ b/dbms/src/Storages/System/StorageSystemDictionaries.cpp @@ -50,10 +50,11 @@ void StorageSystemDictionaries::fillData(MutableColumns & res_columns, const Con res_columns[i++]->insert(static_cast(load_result.status)); res_columns[i++]->insert(load_result.origin); - if (load_result.object) - { - const auto dict_ptr = std::static_pointer_cast(load_result.object); + std::exception_ptr last_exception = load_result.exception; + const auto dict_ptr = std::dynamic_pointer_cast(load_result.object); + if (dict_ptr) + { res_columns[i++]->insert(dict_ptr->getTypeName()); const auto & dict_struct = dict_ptr->getStructure(); @@ -66,6 +67,9 @@ void StorageSystemDictionaries::fillData(MutableColumns & res_columns, const Con res_columns[i++]->insert(dict_ptr->getElementCount()); res_columns[i++]->insert(dict_ptr->getLoadFactor()); res_columns[i++]->insert(dict_ptr->getSource()->toString()); + + if (!last_exception) + last_exception = dict_ptr->getLastException(); } else { @@ -76,8 +80,8 @@ void StorageSystemDictionaries::fillData(MutableColumns & res_columns, const Con res_columns[i++]->insert(static_cast(std::chrono::system_clock::to_time_t(load_result.loading_start_time))); res_columns[i++]->insert(std::chrono::duration_cast>(load_result.loading_duration).count()); - if (load_result.exception) - res_columns[i++]->insert(getExceptionMessage(load_result.exception, false)); + if (last_exception) + res_columns[i++]->insert(getExceptionMessage(last_exception, false)); else res_columns[i++]->insertDefault(); } From f6120558dfd4aa9d61b2beff7c7513fc168d909a Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 30 Aug 2019 17:29:08 +0300 Subject: [PATCH 09/35] Fix bad size of marks --- .../MergeTree/IMergedBlockOutputStream.cpp | 12 ++++---- .../MergeTree/IMergedBlockOutputStream.h | 5 +++- .../MergeTree/MergedBlockOutputStream.cpp | 4 ++- .../MergedColumnOnlyOutputStream.cpp | 5 +++- ...01000_bad_size_of_marks_skip_idx.reference | 2 ++ .../01000_bad_size_of_marks_skip_idx.sql | 28 +++++++++++++++++++ 6 files changed, 47 insertions(+), 9 deletions(-) create mode 100644 dbms/tests/queries/0_stateless/01000_bad_size_of_marks_skip_idx.reference create mode 100644 dbms/tests/queries/0_stateless/01000_bad_size_of_marks_skip_idx.sql diff --git a/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.cpp b/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.cpp index e673fd4759a..71e55015d77 100644 --- a/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.cpp +++ b/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.cpp @@ -333,7 +333,7 @@ void IMergedBlockOutputStream::calculateAndSerializeSkipIndices( { /// Creating block for update Block indices_update_block(skip_indexes_columns); - size_t skip_index_current_mark = 0; + size_t skip_index_current_data_mark = 0; /// Filling and writing skip indices like in IMergedBlockOutputStream::writeColumn for (size_t i = 0; i < skip_indices.size(); ++i) @@ -341,7 +341,7 @@ void IMergedBlockOutputStream::calculateAndSerializeSkipIndices( const auto index = skip_indices[i]; auto & stream = *skip_indices_streams[i]; size_t prev_pos = 0; - skip_index_current_mark = skip_index_mark; + skip_index_current_data_mark = skip_index_data_mark; while (prev_pos < rows) { UInt64 limit = 0; @@ -351,7 +351,7 @@ void IMergedBlockOutputStream::calculateAndSerializeSkipIndices( } else { - limit = index_granularity.getMarkRows(skip_index_current_mark); + limit = index_granularity.getMarkRows(skip_index_current_data_mark); if (skip_indices_aggregators[i]->empty()) { skip_indices_aggregators[i] = index->createIndexAggregator(); @@ -366,9 +366,9 @@ void IMergedBlockOutputStream::calculateAndSerializeSkipIndices( /// to be compatible with normal .mrk2 file format if (can_use_adaptive_granularity) writeIntBinary(1UL, stream.marks); - - ++skip_index_current_mark; } + /// this mark is aggregated, go to the next one + skip_index_current_data_mark++; } size_t pos = prev_pos; @@ -388,7 +388,7 @@ void IMergedBlockOutputStream::calculateAndSerializeSkipIndices( prev_pos = pos; } } - skip_index_mark = skip_index_current_mark; + skip_index_data_mark = skip_index_current_data_mark; } void IMergedBlockOutputStream::finishSkipIndicesSerialization( diff --git a/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h b/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h index 97c7922042d..6b58f95020f 100644 --- a/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h +++ b/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h @@ -141,7 +141,10 @@ protected: size_t aio_threshold; size_t current_mark = 0; - size_t skip_index_mark = 0; + + /// Number of mark in data from which skip indicies have to start + /// aggregation. I.e. it's data mark number, not skip indices mark. + size_t skip_index_data_mark = 0; const bool can_use_adaptive_granularity; const std::string marks_file_extension; diff --git a/dbms/src/Storages/MergeTree/MergedBlockOutputStream.cpp b/dbms/src/Storages/MergeTree/MergedBlockOutputStream.cpp index b923337d71a..9e33b4594f3 100644 --- a/dbms/src/Storages/MergeTree/MergedBlockOutputStream.cpp +++ b/dbms/src/Storages/MergeTree/MergedBlockOutputStream.cpp @@ -332,7 +332,7 @@ void MergedBlockOutputStream::writeImpl(const Block & block, const IColumn::Perm else if (skip_indexes_column_name_to_position.end() != skip_index_column_it) { const auto & index_column = *skip_indexes_columns[skip_index_column_it->second].column; - writeColumn(column.name, *column.type, index_column, offset_columns, false, serialization_states[i], current_mark); + std::tie(std::ignore, new_index_offset) = writeColumn(column.name, *column.type, index_column, offset_columns, false, serialization_states[i], current_mark); } else { @@ -349,6 +349,8 @@ void MergedBlockOutputStream::writeImpl(const Block & block, const IColumn::Perm rows_count += rows; + /// Should be written before index offset update, because we calculate, + /// indices of currently written granules calculateAndSerializeSkipIndices(skip_indexes_columns, rows); { diff --git a/dbms/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp b/dbms/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp index 3c15bd54df2..931cdc67afe 100644 --- a/dbms/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp +++ b/dbms/src/Storages/MergeTree/MergedColumnOnlyOutputStream.cpp @@ -68,7 +68,6 @@ void MergedColumnOnlyOutputStream::write(const Block & block) if (!rows) return; - calculateAndSerializeSkipIndices(skip_indexes_columns, rows); size_t new_index_offset = 0; size_t new_current_mark = 0; @@ -79,6 +78,10 @@ void MergedColumnOnlyOutputStream::write(const Block & block) std::tie(new_current_mark, new_index_offset) = writeColumn(column.name, *column.type, *column.column, offset_columns, skip_offsets, serialization_states[i], current_mark); } + /// Should be written before index offset update, because we calculate, + /// indices of currently written granules + calculateAndSerializeSkipIndices(skip_indexes_columns, rows); + index_offset = new_index_offset; current_mark = new_current_mark; } diff --git a/dbms/tests/queries/0_stateless/01000_bad_size_of_marks_skip_idx.reference b/dbms/tests/queries/0_stateless/01000_bad_size_of_marks_skip_idx.reference new file mode 100644 index 00000000000..6ed281c757a --- /dev/null +++ b/dbms/tests/queries/0_stateless/01000_bad_size_of_marks_skip_idx.reference @@ -0,0 +1,2 @@ +1 +1 diff --git a/dbms/tests/queries/0_stateless/01000_bad_size_of_marks_skip_idx.sql b/dbms/tests/queries/0_stateless/01000_bad_size_of_marks_skip_idx.sql new file mode 100644 index 00000000000..7af19fec695 --- /dev/null +++ b/dbms/tests/queries/0_stateless/01000_bad_size_of_marks_skip_idx.sql @@ -0,0 +1,28 @@ +SET allow_experimental_data_skipping_indices=1; + +DROP TABLE IF EXISTS bad_skip_idx; + +CREATE TABLE bad_skip_idx +( + id UInt64, + value String +) ENGINE MergeTree() +ORDER BY id SETTINGS index_granularity_bytes = 64, vertical_merge_algorithm_min_rows_to_activate = 0, vertical_merge_algorithm_min_columns_to_activate = 0; -- actually vertical merge is not required condition for this bug, but it's more easy to reproduce (becuse we don't recalc granularities) + +-- 7 rows per granule +INSERT INTO bad_skip_idx SELECT number, concat('x', toString(number)) FROM numbers(1000); + +-- 3 rows per granule +INSERT INTO bad_skip_idx SELECT number, concat('xxxxxxxxxx', toString(number)) FROM numbers(1000,1000); + +SELECT COUNT(*) from bad_skip_idx WHERE value = 'xxxxxxxxxx1015'; -- check no exception + +INSERT INTO bad_skip_idx SELECT number, concat('x', toString(number)) FROM numbers(1000); + +ALTER TABLE bad_skip_idx ADD INDEX idx value TYPE bloom_filter(0.01) GRANULARITY 4; + +OPTIMIZE TABLE bad_skip_idx FINAL; + +SELECT COUNT(*) from bad_skip_idx WHERE value = 'xxxxxxxxxx1015'; -- check no exception + +DROP TABLE IF EXISTS bad_skip_idx; From d4ea6a52343fc741f7c3d8c4b2e765be867ed09c Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 30 Aug 2019 17:30:28 +0300 Subject: [PATCH 10/35] Fix comment --- dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h b/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h index 6b58f95020f..37aa2203a72 100644 --- a/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h +++ b/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h @@ -142,7 +142,7 @@ protected: size_t current_mark = 0; - /// Number of mark in data from which skip indicies have to start + /// Number of mark in data from which skip indices have to start /// aggregation. I.e. it's data mark number, not skip indices mark. size_t skip_index_data_mark = 0; From 83c75ca2adfa34c256628077232f8131199accf5 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Fri, 30 Aug 2019 19:21:05 +0300 Subject: [PATCH 11/35] Added a test (but it doesn't reproduce the issue #6746) --- ...er_nullable_adaptive_granularity.reference | 0 ...002_alter_nullable_adaptive_granularity.sh | 58 +++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 dbms/tests/queries/0_stateless/01002_alter_nullable_adaptive_granularity.reference create mode 100755 dbms/tests/queries/0_stateless/01002_alter_nullable_adaptive_granularity.sh diff --git a/dbms/tests/queries/0_stateless/01002_alter_nullable_adaptive_granularity.reference b/dbms/tests/queries/0_stateless/01002_alter_nullable_adaptive_granularity.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/dbms/tests/queries/0_stateless/01002_alter_nullable_adaptive_granularity.sh b/dbms/tests/queries/0_stateless/01002_alter_nullable_adaptive_granularity.sh new file mode 100755 index 00000000000..85fc847f3f3 --- /dev/null +++ b/dbms/tests/queries/0_stateless/01002_alter_nullable_adaptive_granularity.sh @@ -0,0 +1,58 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +. $CURDIR/../shell_config.sh + +set -e + +$CLICKHOUSE_CLIENT --query "DROP TABLE IF EXISTS test"; +$CLICKHOUSE_CLIENT --query "CREATE TABLE test (x UInt8, s String MATERIALIZED toString(rand64())) ENGINE = MergeTree ORDER BY s"; + +function thread1() +{ + while true; do + $CLICKHOUSE_CLIENT --query "INSERT INTO test SELECT rand() FROM numbers(1000)"; + done +} + +function thread2() +{ + while true; do + $CLICKHOUSE_CLIENT -n --query "ALTER TABLE test MODIFY COLUMN x Nullable(UInt8);"; + sleep 0.0$RANDOM + $CLICKHOUSE_CLIENT -n --query "ALTER TABLE test MODIFY COLUMN x UInt8;"; + sleep 0.0$RANDOM + done +} + +function thread3() +{ + while true; do + $CLICKHOUSE_CLIENT -n --query "SELECT count() FROM test FORMAT Null"; + done +} + +function thread4() +{ + while true; do + $CLICKHOUSE_CLIENT -n --query "OPTIMIZE TABLE test FINAL"; + sleep 0.1$RANDOM + done +} + +# https://stackoverflow.com/questions/9954794/execute-a-shell-function-with-timeout +export -f thread1; +export -f thread2; +export -f thread3; +export -f thread4; + +TIMEOUT=10 + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & +timeout $TIMEOUT bash -c thread4 2> /dev/null & + +wait + +$CLICKHOUSE_CLIENT -q "DROP TABLE test" From 808f4d0b8a9c290f5b794b1f33f00db4b46bd769 Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 30 Aug 2019 19:50:59 +0300 Subject: [PATCH 12/35] Intermediate refactoring --- dbms/src/Core/Settings.h | 3 +- dbms/src/Core/SettingsCommon.h | 55 +------------------ dbms/src/Storages/AlterCommands.cpp | 9 +-- dbms/src/Storages/IStorage.cpp | 18 +++--- dbms/src/Storages/IStorage.h | 6 +- dbms/src/Storages/Kafka/KafkaSettings.h | 23 ++++---- dbms/src/Storages/Kafka/StorageKafka.cpp | 45 ++++++++++----- dbms/src/Storages/Kafka/StorageKafka.h | 4 +- dbms/src/Storages/MergeTree/MergeTreeData.cpp | 14 +++-- dbms/src/Storages/MergeTree/MergeTreeData.h | 2 +- .../Storages/MergeTree/MergeTreeSettings.h | 14 +++-- .../MergeTree/registerStorageMergeTree.cpp | 3 - 12 files changed, 78 insertions(+), 118 deletions(-) diff --git a/dbms/src/Core/Settings.h b/dbms/src/Core/Settings.h index cb12a969b76..e03efa3f56f 100644 --- a/dbms/src/Core/Settings.h +++ b/dbms/src/Core/Settings.h @@ -42,8 +42,7 @@ struct Settings : public SettingsCollection * but we are not going to do it, because settings is used everywhere as static struct fields. */ -/// M (mutable) for normal settings, IM (immutable) for not updateable settings. -#define LIST_OF_SETTINGS(M, IM) \ +#define LIST_OF_SETTINGS(M) \ M(SettingUInt64, min_compress_block_size, 65536, "The actual size of the block to compress, if the uncompressed data less than max_compress_block_size is no less than this value and no less than the volume of data for one mark.") \ M(SettingUInt64, max_compress_block_size, 1048576, "The maximum size of blocks of uncompressed data before compressing for writing to a table.") \ M(SettingUInt64, max_block_size, DEFAULT_BLOCK_SIZE, "Maximum block size for reading") \ diff --git a/dbms/src/Core/SettingsCommon.h b/dbms/src/Core/SettingsCommon.h index de30f43c7c8..07badbdddc0 100644 --- a/dbms/src/Core/SettingsCommon.h +++ b/dbms/src/Core/SettingsCommon.h @@ -17,11 +17,6 @@ class Field; class ReadBuffer; class WriteBuffer; -namespace ErrorCodes -{ - extern const int IMMUTABLE_SETTING; -} - /** One setting for any type. * Stores a value within itself, as well as a flag - whether the value was changed. * This is done so that you can send to the remote servers only changed settings (or explicitly specified in the config) values. @@ -34,7 +29,6 @@ struct SettingNumber { Type value; bool changed = false; - bool immutable = false; SettingNumber(Type x = 0) : value(x) {} @@ -77,7 +71,6 @@ struct SettingMaxThreads UInt64 value; bool is_auto; bool changed = false; - bool immutable = false; SettingMaxThreads(UInt64 x = 0) : value(x ? x : getAutoValue()), is_auto(x == 0) {} @@ -106,7 +99,6 @@ struct SettingTimespan { Poco::Timespan value; bool changed = false; - bool immutable = false; SettingTimespan(UInt64 x = 0) : value(x * microseconds_per_io_unit) {} @@ -139,7 +131,6 @@ struct SettingString { String value; bool changed = false; - bool immutable = false; SettingString(const String & x = String{}) : value(x) {} @@ -162,7 +153,6 @@ struct SettingChar public: char value; bool changed = false; - bool immutable = false; SettingChar(char x = '\0') : value(x) {} @@ -187,7 +177,6 @@ struct SettingEnum { EnumType value; bool changed = false; - bool immutable = false; SettingEnum(EnumType x) : value(x) {} @@ -322,8 +311,6 @@ private: using SerializeFunction = void (*)(const Derived &, WriteBuffer & buf); using DeserializeFunction = void (*)(Derived &, ReadBuffer & buf); using CastValueWithoutApplyingFunction = Field (*)(const Field &); - using SetImmutable = void(*)(Derived &); - using IsImmutable = bool(*)(const Derived &); struct MemberInfo @@ -331,8 +318,6 @@ private: IsChangedFunction is_changed; StringRef name; StringRef description; - /// At one moment this setting can became immutable - const bool can_be_immutable; GetStringFunction get_string; GetFieldFunction get_field; SetStringFunction set_string; @@ -340,8 +325,6 @@ private: SerializeFunction serialize; DeserializeFunction deserialize; CastValueWithoutApplyingFunction cast_value_without_applying; - SetImmutable set_immutable; - IsImmutable is_immutable; bool isChanged(const Derived & collection) const { return is_changed(collection); } }; @@ -431,20 +414,8 @@ public: public: reference(Derived & collection_, const MemberInfo & member_) : const_reference(collection_, member_) {} reference(const const_reference & src) : const_reference(src) {} - void setValue(const Field & value) - { - if (this->member->is_immutable(*this->collection)) - throw Exception("Setting '" + this->member->name.toString() + "' is restricted for updates.", ErrorCodes::IMMUTABLE_SETTING); - this->member->set_field(*const_cast(this->collection), value); - } - void setValue(const String & value) - { - if (this->member->is_immutable(*this->collection)) - throw Exception("Setting '" + this->member->name.toString() + "' is restricted for updates.", ErrorCodes::IMMUTABLE_SETTING); - this->member->set_string(*const_cast(this->collection), value); - } - bool canBeImmutable() const { return this->member->can_be_immutable; } - void makeImmutableForever() { this->member->set_immutable(*const_cast(this->collection)); } + void setValue(const Field & value) { this->member->set_field(*const_cast(this->collection), value); } + void setValue(const String & value) { this->member->set_string(*const_cast(this->collection), value); } }; /// Iterator to iterating through all the settings. @@ -615,14 +586,6 @@ public: dest.copyChangesFrom(castToDerived()); } - /// Make all possible immutable settings (can_be_immutable) immutable forever - void finishSettingsInitialization() - { - for (auto & member : *this) - if (member.canBeImmutable()) - member.makeImmutableForever(); - } - /// Writes the settings to buffer (e.g. to be sent to remote server). /// Only changed settings are written. They are written as list of contiguous name-value pairs, /// finished with empty name. @@ -666,7 +629,7 @@ public: { \ LIST_OF_SETTINGS_MACRO(IMPLEMENT_SETTINGS_COLLECTION_DEFINE_FUNCTIONS_HELPER_, IMPLEMENT_SETTINGS_COLLECTION_DEFINE_FUNCTIONS_HELPER_) \ }; \ - LIST_OF_SETTINGS_MACRO(IMPLEMENT_SETTINGS_COLLECTION_ADD_MUTABLE_MEMBER_INFO_HELPER_, IMPLEMENT_SETTINGS_COLLECTION_ADD_IMMUTABLE_MEMBER_INFO_HELPER_) \ + LIST_OF_SETTINGS_MACRO(IMPLEMENT_SETTINGS_COLLECTION_ADD_MUTABLE_MEMBER_INFO_HELPER_) \ } @@ -682,8 +645,6 @@ public: static void NAME##_serialize(const Derived & collection, WriteBuffer & buf) { collection.NAME.serialize(buf); } \ static void NAME##_deserialize(Derived & collection, ReadBuffer & buf) { collection.NAME.deserialize(buf); } \ static Field NAME##_castValueWithoutApplying(const Field & value) { TYPE temp{DEFAULT}; temp.set(value); return temp.toField(); } \ - static void NAME##_setImmutable(Derived & collection) { collection.NAME.immutable = true; } \ - static bool NAME##_isImmutable(const Derived & collection) { return collection.NAME.immutable; } #define IMPLEMENT_SETTINGS_COLLECTION_ADD_MUTABLE_MEMBER_INFO_HELPER_(TYPE, NAME, DEFAULT, DESCRIPTION) \ @@ -693,14 +654,4 @@ public: &Functions::NAME##_setString, &Functions::NAME##_setField, \ &Functions::NAME##_serialize, &Functions::NAME##_deserialize, \ &Functions::NAME##_castValueWithoutApplying, \ - &Functions::NAME##_setImmutable, &Functions::NAME##_isImmutable }); - -#define IMPLEMENT_SETTINGS_COLLECTION_ADD_IMMUTABLE_MEMBER_INFO_HELPER_(TYPE, NAME, DEFAULT, DESCRIPTION) \ - add({[](const Derived & d) { return d.NAME.changed; }, \ - StringRef(#NAME, strlen(#NAME)), StringRef(#DESCRIPTION, strlen(#DESCRIPTION)), true, \ - &Functions::NAME##_getString, &Functions::NAME##_getField, \ - &Functions::NAME##_setString, &Functions::NAME##_setField, \ - &Functions::NAME##_serialize, &Functions::NAME##_deserialize, \ - &Functions::NAME##_castValueWithoutApplying, \ - &Functions::NAME##_setImmutable, &Functions::NAME##_isImmutable }); } diff --git a/dbms/src/Storages/AlterCommands.cpp b/dbms/src/Storages/AlterCommands.cpp index edac6b3b88b..2a311461f00 100644 --- a/dbms/src/Storages/AlterCommands.cpp +++ b/dbms/src/Storages/AlterCommands.cpp @@ -543,15 +543,8 @@ void AlterCommands::validate(const IStorage & table, const Context & context) } } else if (command.type == AlterCommand::MODIFY_SETTING) - { for (const auto & change : command.settings_changes) - { - if (!table.hasSetting(change.name)) - { - throw Exception{"Storage '" + table.getName() + "' doesn't have setting '" + change.name + "'", ErrorCodes::UNKNOWN_SETTING}; - } - } - } + table.checkSetting(change.name); } /** Existing defaulted columns may require default expression extensions with a type conversion, diff --git a/dbms/src/Storages/IStorage.cpp b/dbms/src/Storages/IStorage.cpp index 2f3a48d90b6..a26af1637fa 100644 --- a/dbms/src/Storages/IStorage.cpp +++ b/dbms/src/Storages/IStorage.cpp @@ -308,11 +308,10 @@ bool IStorage::isVirtualColumn(const String & column_name) const return getColumns().get(column_name).is_virtual; } -bool IStorage::hasSetting(const String & /* setting_name */) const +void IStorage::checkSetting(const String & /* setting_name */) const { if (!supportsSettings()) throw Exception("Storage '" + getName() + "' doesn't support settings.", ErrorCodes::SETTINGS_ARE_NOT_SUPPORTED); - return false; } TableStructureReadLockHolder IStorage::lockStructureForShare(bool will_add_new_data, const String & query_id) @@ -380,16 +379,13 @@ IDatabase::ASTModifier IStorage::getSettingsModifier(const SettingsChanges & new /// Make storage settings unique for (const auto & change : new_changes) { - if (hasSetting(change.name)) - { - auto finder = [&change] (const SettingChange & c) { return c.name == change.name; }; - if (auto it = std::find_if(storage_changes.begin(), storage_changes.end(), finder); it != storage_changes.end()) - it->value = change.value; - else - storage_changes.push_back(change); - } + checkSetting(change.name); + + auto finder = [&change] (const SettingChange & c) { return c.name == change.name; }; + if (auto it = std::find_if(storage_changes.begin(), storage_changes.end(), finder); it != storage_changes.end()) + it->value = change.value; else - throw Exception{"Storage '" + getName() + "' doesn't have setting '" + change.name + "'", ErrorCodes::UNKNOWN_SETTING}; + storage_changes.push_back(change); } } }; diff --git a/dbms/src/Storages/IStorage.h b/dbms/src/Storages/IStorage.h index 6c23a638ddf..dc3f4905310 100644 --- a/dbms/src/Storages/IStorage.h +++ b/dbms/src/Storages/IStorage.h @@ -138,8 +138,8 @@ public: /// thread-unsafe part. lockStructure must be acquired /// If |need_all| is set, then checks that all the columns of the table are in the block. void check(const Block & block, bool need_all = false) const; - /// Check storage has setting. Exception will be thrown if it doesn't support settings at all. - virtual bool hasSetting(const String & setting_name) const; + /// Check storage has setting and setting can be modified. + virtual bool checkSetting(const String & setting_name) const; protected: /// still thread-unsafe part. void setIndices(IndicesDescription indices_); @@ -149,7 +149,7 @@ protected: /// still thread-unsafe part. virtual bool isVirtualColumn(const String & column_name) const; /// Returns modifier of settings in storage definition - virtual IDatabase::ASTModifier getSettingsModifier(const SettingsChanges & new_changes) const; + IDatabase::ASTModifier getSettingsModifier(const SettingsChanges & new_changes) const; private: ColumnsDescription columns; /// combined real and virtual columns diff --git a/dbms/src/Storages/Kafka/KafkaSettings.h b/dbms/src/Storages/Kafka/KafkaSettings.h index bc453238b51..6ff62f30411 100644 --- a/dbms/src/Storages/Kafka/KafkaSettings.h +++ b/dbms/src/Storages/Kafka/KafkaSettings.h @@ -15,18 +15,17 @@ struct KafkaSettings : public SettingsCollection { -/// M (mutable) for normal settings, IM (immutable) for not updateable settings. -#define LIST_OF_KAFKA_SETTINGS(M, IM) \ - IM(SettingString, kafka_broker_list, "", "A comma-separated list of brokers for Kafka engine.") \ - IM(SettingString, kafka_topic_list, "", "A list of Kafka topics.") \ - IM(SettingString, kafka_group_name, "", "A group of Kafka consumers.") \ - IM(SettingString, kafka_format, "", "The message format for Kafka engine.") \ - IM(SettingChar, kafka_row_delimiter, '\0', "The character to be considered as a delimiter in Kafka message.") \ - IM(SettingString, kafka_schema, "", "Schema identifier (used by schema-based formats) for Kafka engine") \ - IM(SettingUInt64, kafka_num_consumers, 1, "The number of consumers per table for Kafka engine.") \ - IM(SettingUInt64, kafka_max_block_size, 0, "The maximum block size per table for Kafka engine.") \ - IM(SettingUInt64, kafka_skip_broken_messages, 0, "Skip at least this number of broken messages from Kafka topic per block") \ - IM(SettingUInt64, kafka_commit_every_batch, 0, "Commit every consumed and handled batch instead of a single commit after writing a whole block") +#define LIST_OF_KAFKA_SETTINGS(M) \ + M(SettingString, kafka_broker_list, "", "A comma-separated list of brokers for Kafka engine.") \ + M(SettingString, kafka_topic_list, "", "A list of Kafka topics.") \ + M(SettingString, kafka_group_name, "", "A group of Kafka consumers.") \ + M(SettingString, kafka_format, "", "The message format for Kafka engine.") \ + M(SettingChar, kafka_row_delimiter, '\0', "The character to be considered as a delimiter in Kafka message.") \ + M(SettingString, kafka_schema, "", "Schema identifier (used by schema-based formats) for Kafka engine") \ + M(SettingUInt64, kafka_num_consumers, 1, "The number of consumers per table for Kafka engine.") \ + M(SettingUInt64, kafka_max_block_size, 0, "The maximum block size per table for Kafka engine.") \ + M(SettingUInt64, kafka_skip_broken_messages, 0, "Skip at least this number of broken messages from Kafka topic per block") \ + M(SettingUInt64, kafka_commit_every_batch, 0, "Commit every consumed and handled batch instead of a single commit after writing a whole block") DECLARE_SETTINGS_COLLECTION(LIST_OF_KAFKA_SETTINGS) diff --git a/dbms/src/Storages/Kafka/StorageKafka.cpp b/dbms/src/Storages/Kafka/StorageKafka.cpp index 835ce43b1a4..956a19a8ba8 100644 --- a/dbms/src/Storages/Kafka/StorageKafka.cpp +++ b/dbms/src/Storages/Kafka/StorageKafka.cpp @@ -34,16 +34,37 @@ namespace DB { +namespace ProfileEvents +{ + extern const Event RejectedInserts; + extern const Event DelayedInserts; + extern const Event DelayedInsertsMilliseconds; +} + +namespace CurrentMetrics +{ + extern const Metric DelayedInserts; +} + namespace ErrorCodes { - extern const int INCORRECT_DATA; - extern const int UNKNOWN_EXCEPTION; - extern const int CANNOT_READ_FROM_ISTREAM; - extern const int INVALID_CONFIG_PARAMETER; - extern const int LOGICAL_ERROR; extern const int BAD_ARGUMENTS; - extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH; - extern const int UNSUPPORTED_METHOD; + extern const int MEMORY_LIMIT_EXCEEDED; + extern const int SYNTAX_ERROR; + extern const int INVALID_PARTITION_VALUE; + extern const int METADATA_MISMATCH; + extern const int PART_IS_TEMPORARILY_LOCKED; + extern const int TOO_MANY_PARTS; + extern const int INCOMPATIBLE_COLUMNS; + extern const int CANNOT_UPDATE_COLUMN; + extern const int CANNOT_ALLOCATE_MEMORY; + extern const int CANNOT_MUNMAP; + extern const int CANNOT_MREMAP; + extern const int BAD_TTL_EXPRESSION; + extern const int INCORRECT_FILE_NAME; + extern const int BAD_DATA_PART_NAME; + extern const int UNKNOWN_SETTING; + extern const int IMMUTABLE_SETTING; } namespace @@ -407,14 +428,12 @@ bool StorageKafka::streamToViews() } -bool StorageKafka::hasSetting(const String & setting_name) const +void StorageKafka::checkSetting(const String & setting_name) const { - return KafkaSettings::findIndex(setting_name) != KafkaSettings::npos; -} + if (KafkaSettings::findIndex(setting_name) == KafkaSettings::npos) + throw Exception{"Storage '" + getName() + "' doesn't have setting '" + setting_name + "'", ErrorCodes::UNKNOWN_SETTING}; -IDatabase::ASTModifier StorageKafka::getSettingsModifier(const SettingsChanges & /* new_changes */) const -{ - throw Exception("Storage '" + getName() + "' doesn't support settings alter", ErrorCodes::UNSUPPORTED_METHOD); + throw Exception{"Setting '" + setting_name + "' is immutable for storage '" + getName() + "'", ErrorCodes::IMMUTABLE_SETTING}; } void registerStorageKafka(StorageFactory & factory) diff --git a/dbms/src/Storages/Kafka/StorageKafka.h b/dbms/src/Storages/Kafka/StorageKafka.h index b1eac57dca1..fc336eabafb 100644 --- a/dbms/src/Storages/Kafka/StorageKafka.h +++ b/dbms/src/Storages/Kafka/StorageKafka.h @@ -57,8 +57,7 @@ public: const auto & getSchemaName() const { return schema_name; } const auto & skipBroken() const { return skip_broken; } - bool hasSetting(const String & setting_name) const override; - + void checkSetting(const String & setting_name) const override; protected: StorageKafka( @@ -71,7 +70,6 @@ protected: size_t num_consumers_, UInt64 max_block_size_, size_t skip_broken, bool intermediate_commit_); - IDatabase::ASTModifier getSettingsModifier(const SettingsChanges & new_changes) const override; private: // Configuration and state String table_name; diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.cpp b/dbms/src/Storages/MergeTree/MergeTreeData.cpp index f8f915cadef..aceedec78bc 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeData.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreeData.cpp @@ -91,6 +91,7 @@ namespace ErrorCodes extern const int INCORRECT_FILE_NAME; extern const int BAD_DATA_PART_NAME; extern const int UNKNOWN_SETTING; + extern const int IMMUTABLE_SETTING; } @@ -1324,10 +1325,7 @@ void MergeTreeData::checkAlter(const AlterCommands & commands, const Context & c setTTLExpressions(new_columns.getColumnTTLs(), new_ttl_table_ast, /* only_check = */ true); for (const auto & setting : new_changes) - { - if (!hasSetting(setting.name)) - throw Exception{"Storage '" + getName() + "' doesn't have setting '" + setting.name + "'", ErrorCodes::UNKNOWN_SETTING}; - } + checkSetting(setting.name); /// Check that type conversions are possible. ExpressionActionsPtr unused_expression; @@ -1657,9 +1655,13 @@ void MergeTreeData::changeSettings( } } -bool MergeTreeData::hasSetting(const String & setting_name) const +void MergeTreeData::checkSetting(const String & setting_name) const { - return MergeTreeSettings::findIndex(setting_name) != MergeTreeSettings::npos; + if (MergeTreeSettings::findIndex(setting_name) == MergeTreeSettings::npos) + throw Exception{"Storage '" + getName() + "' doesn't have setting '" + setting_name + "'", ErrorCodes::UNKNOWN_SETTING}; + if (MergeTreeSettings::isImmutableSetting(setting_name)) + throw Exception{"Setting '" + setting_name + "' is immutable for storage '" + getName() + "'", ErrorCodes::IMMUTABLE_SETTING}; + } void MergeTreeData::removeEmptyColumnsFromPart(MergeTreeData::MutableDataPartPtr & data_part) diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.h b/dbms/src/Storages/MergeTree/MergeTreeData.h index 0440a3181c8..5a82ee407a4 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeData.h +++ b/dbms/src/Storages/MergeTree/MergeTreeData.h @@ -543,7 +543,7 @@ public: TableStructureWriteLockHolder & table_lock_holder); /// All MergeTreeData children have settings. - bool hasSetting(const String & setting_name) const override; + void checkSetting(const String & setting_name) const override; /// Remove columns, that have been markedd as empty after zeroing values with expired ttl void removeEmptyColumnsFromPart(MergeTreeData::MutableDataPartPtr & data_part); diff --git a/dbms/src/Storages/MergeTree/MergeTreeSettings.h b/dbms/src/Storages/MergeTree/MergeTreeSettings.h index 6ba08fed5da..20d81771f5e 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeSettings.h +++ b/dbms/src/Storages/MergeTree/MergeTreeSettings.h @@ -2,6 +2,7 @@ #include #include +#include namespace Poco @@ -24,9 +25,8 @@ class ASTStorage; struct MergeTreeSettings : public SettingsCollection { -/// M (mutable) for normal settings, IM (immutable) for not updateable settings. -#define LIST_OF_MERGE_TREE_SETTINGS(M, IM) \ - IM(SettingUInt64, index_granularity, 8192, "How many rows correspond to one primary key value.") \ +#define LIST_OF_MERGE_TREE_SETTINGS(M) \ + M(SettingUInt64, index_granularity, 8192, "How many rows correspond to one primary key value.") \ \ /** Merge settings. */ \ M(SettingUInt64, max_bytes_to_merge_at_max_space_in_pool, 150ULL * 1024 * 1024 * 1024, "Maximum in total size of parts to merge, when there are maximum free threads in background pool (or entries in replication queue).") \ @@ -80,7 +80,7 @@ struct MergeTreeSettings : public SettingsCollection M(SettingBool, use_minimalistic_part_header_in_zookeeper, false, "Store part header (checksums and columns) in a compact format and a single part znode instead of separate znodes (/columns and /checksums). This can dramatically reduce snapshot size in ZooKeeper. Before enabling check that all replicas support new format.") \ M(SettingUInt64, finished_mutations_to_keep, 100, "How many records about mutations that are done to keep. If zero, then keep all of them.") \ M(SettingUInt64, min_merge_bytes_to_use_direct_io, 10ULL * 1024 * 1024 * 1024, "Minimal amount of bytes to enable O_DIRECT in merge (0 - disabled).") \ - IM(SettingUInt64, index_granularity_bytes, 10 * 1024 * 1024, "Approximate amount of bytes in single granule (0 - disabled).") \ + M(SettingUInt64, index_granularity_bytes, 10 * 1024 * 1024, "Approximate amount of bytes in single granule (0 - disabled).") \ M(SettingInt64, merge_with_ttl_timeout, 3600 * 24, "Minimal time in seconds, when merge with TTL can be repeated.") \ M(SettingBool, write_final_mark, 1, "Write final mark after end of column (0 - disabled, do nothing if index_granularity_bytes=0)") \ M(SettingBool, enable_mixed_granularity_parts, 0, "Enable parts with adaptive and non adaptive granularity") \ @@ -98,6 +98,12 @@ struct MergeTreeSettings : public SettingsCollection /// NOTE: will rewrite the AST to add immutable settings. void loadFromQuery(ASTStorage & storage_def); + + /// We check settings after storage creation + static bool isImmutableSetting(const String & name) + { + return name == "index_granularity" || name == "index_granularity_bytes"; + } }; using MergeTreeSettingsPtr = std::shared_ptr; diff --git a/dbms/src/Storages/MergeTree/registerStorageMergeTree.cpp b/dbms/src/Storages/MergeTree/registerStorageMergeTree.cpp index 596ea4f5d0b..674116a54bc 100644 --- a/dbms/src/Storages/MergeTree/registerStorageMergeTree.cpp +++ b/dbms/src/Storages/MergeTree/registerStorageMergeTree.cpp @@ -637,9 +637,6 @@ static StoragePtr create(const StorageFactory::Arguments & args) throw Exception("You must set the setting `allow_experimental_data_skipping_indices` to 1 " \ "before using data skipping indices.", ErrorCodes::BAD_ARGUMENTS); - /// Finalize settings and disable updates - storage_settings->finishSettingsInitialization(); - if (replicated) return StorageReplicatedMergeTree::create( zookeeper_path, replica_name, args.attach, args.data_path, args.database_name, args.table_name, From cd5c0fc9ac29af25632ab50f902497db737ddfa7 Mon Sep 17 00:00:00 2001 From: Ivan <5627721+abyss7@users.noreply.github.com> Date: Fri, 30 Aug 2019 20:40:27 +0300 Subject: [PATCH 13/35] Fix build issues (#6744) * libcxxabi uses exception handling library as public * Don't set -stdlib for internal libc++ - it poisons the checks. * Enable capnproto in unbundled build back --- cmake/find_ccache.cmake | 21 ++++++++++--------- cmake/find_cxx.cmake | 3 ++- contrib/libcxx-cmake/CMakeLists.txt | 2 +- contrib/libcxxabi-cmake/CMakeLists.txt | 7 +------ .../integration/test_storage_kafka/test.py | 2 +- docker/packager/packager | 2 +- 6 files changed, 17 insertions(+), 20 deletions(-) diff --git a/cmake/find_ccache.cmake b/cmake/find_ccache.cmake index c2d467ca6ba..95d6b208cfa 100644 --- a/cmake/find_ccache.cmake +++ b/cmake/find_ccache.cmake @@ -1,13 +1,14 @@ find_program (CCACHE_FOUND ccache) -if (CCACHE_FOUND AND NOT CMAKE_CXX_COMPILER_LAUNCHER MATCHES "ccache" AND NOT CMAKE_CXX_COMPILER MATCHES "ccache") - execute_process(COMMAND ${CCACHE_FOUND} "-V" OUTPUT_VARIABLE CCACHE_VERSION) - string(REGEX REPLACE "ccache version ([0-9\\.]+).*" "\\1" CCACHE_VERSION ${CCACHE_VERSION}) - if (CCACHE_VERSION VERSION_GREATER "3.2.0" OR NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang") - #message(STATUS "Using ${CCACHE_FOUND} ${CCACHE_VERSION}") - set_property (GLOBAL PROPERTY RULE_LAUNCH_COMPILE ${CCACHE_FOUND}) - set_property (GLOBAL PROPERTY RULE_LAUNCH_LINK ${CCACHE_FOUND}) - else () - message(STATUS "Not using ${CCACHE_FOUND} ${CCACHE_VERSION} bug: https://bugzilla.samba.org/show_bug.cgi?id=8118") - endif () +if (CCACHE_FOUND AND NOT CMAKE_CXX_COMPILER_LAUNCHER MATCHES "ccache" AND NOT CMAKE_CXX_COMPILER MATCHES "ccache") + execute_process(COMMAND ${CCACHE_FOUND} "-V" OUTPUT_VARIABLE CCACHE_VERSION) + string(REGEX REPLACE "ccache version ([0-9\\.]+).*" "\\1" CCACHE_VERSION ${CCACHE_VERSION}) + + if (CCACHE_VERSION VERSION_GREATER "3.2.0" OR NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang") + #message(STATUS "Using ${CCACHE_FOUND} ${CCACHE_VERSION}") + set_property (GLOBAL PROPERTY RULE_LAUNCH_COMPILE ${CCACHE_FOUND}) + set_property (GLOBAL PROPERTY RULE_LAUNCH_LINK ${CCACHE_FOUND}) + else () + message(STATUS "Not using ${CCACHE_FOUND} ${CCACHE_VERSION} bug: https://bugzilla.samba.org/show_bug.cgi?id=8118") + endif () endif () diff --git a/cmake/find_cxx.cmake b/cmake/find_cxx.cmake index da6e3d48cab..f84a76183ec 100644 --- a/cmake/find_cxx.cmake +++ b/cmake/find_cxx.cmake @@ -25,6 +25,8 @@ if (USE_LIBCXX) find_library (LIBCXXFS_LIBRARY c++fs) find_library (LIBCXXABI_LIBRARY c++abi) + set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libc++") + target_link_libraries(global-libs INTERFACE ${EXCEPTION_HANDLING_LIBRARY}) else () set (LIBCXX_LIBRARY cxx) @@ -38,7 +40,6 @@ if (USE_LIBCXX) target_link_libraries(global-libs INTERFACE ${LIBCXX_LIBRARY} ${LIBCXXABI_LIBRARY} ${LIBCXXFS_LIBRARY}) set (HAVE_LIBCXX 1) - set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -stdlib=libc++") message (STATUS "Using libcxx: ${LIBCXX_LIBRARY}") message (STATUS "Using libcxxfs: ${LIBCXXFS_LIBRARY}") diff --git a/contrib/libcxx-cmake/CMakeLists.txt b/contrib/libcxx-cmake/CMakeLists.txt index 8759786c3e8..9609c7ca9e7 100644 --- a/contrib/libcxx-cmake/CMakeLists.txt +++ b/contrib/libcxx-cmake/CMakeLists.txt @@ -40,7 +40,7 @@ ${LIBCXX_SOURCE_DIR}/src/random.cpp add_library(cxx ${SRCS}) -target_include_directories(cxx BEFORE PUBLIC $) +target_include_directories(cxx SYSTEM BEFORE PUBLIC $) target_compile_definitions(cxx PRIVATE -D_LIBCPP_BUILDING_LIBRARY -DLIBCXX_BUILDING_LIBCXXABI) target_compile_options(cxx PUBLIC -nostdinc++ -Wno-reserved-id-macro) target_link_libraries(cxx PUBLIC cxxabi) diff --git a/contrib/libcxxabi-cmake/CMakeLists.txt b/contrib/libcxxabi-cmake/CMakeLists.txt index 2abe5702132..68bb5606689 100644 --- a/contrib/libcxxabi-cmake/CMakeLists.txt +++ b/contrib/libcxxabi-cmake/CMakeLists.txt @@ -30,12 +30,7 @@ target_include_directories(cxxabi SYSTEM BEFORE ) target_compile_definitions(cxxabi PRIVATE -D_LIBCPP_BUILDING_LIBRARY) target_compile_options(cxxabi PRIVATE -nostdinc++ -fno-sanitize=undefined -Wno-macro-redefined) # If we don't disable UBSan, infinite recursion happens in dynamic_cast. - -if (USE_UNWIND) - target_link_libraries(cxxabi PRIVATE ${UNWIND_LIBRARIES}) -else () - target_link_libraries(cxxabi PRIVATE gcc_eh) -endif () +target_link_libraries(cxxabi PUBLIC ${EXCEPTION_HANDLING_LIBRARY}) install( TARGETS cxxabi diff --git a/dbms/tests/integration/test_storage_kafka/test.py b/dbms/tests/integration/test_storage_kafka/test.py index 47ba957cdef..7d17c1cf362 100644 --- a/dbms/tests/integration/test_storage_kafka/test.py +++ b/dbms/tests/integration/test_storage_kafka/test.py @@ -605,7 +605,7 @@ def test_kafka_produce_consume(kafka_cluster): assert int(result) == messages_num * threads_num, 'ClickHouse lost some messages: {}'.format(result) -@pytest.mark.timeout(120) +@pytest.mark.timeout(180) def test_kafka_commit_on_block_write(kafka_cluster): instance.query(''' DROP TABLE IF EXISTS test.view; diff --git a/docker/packager/packager b/docker/packager/packager index c132f514569..d0d46fd5f51 100755 --- a/docker/packager/packager +++ b/docker/packager/packager @@ -143,7 +143,7 @@ def parse_env_variables(build_type, compiler, sanitizer, package_type, cache, di result.append("ALIEN_PKGS='" + ' '.join(['--' + pkg for pkg in alien_pkgs]) + "'") if unbundled: - cmake_flags.append('-DUNBUNDLED=1 -DENABLE_MYSQL=0 -DENABLE_POCO_ODBC=0 -DENABLE_ODBC=0 -DUSE_CAPNP=0') + cmake_flags.append('-DUNBUNDLED=1 -DENABLE_MYSQL=0 -DENABLE_POCO_ODBC=0 -DENABLE_ODBC=0') if split_binary: cmake_flags.append('-DUSE_STATIC_LIBRARIES=0 -DSPLIT_SHARED_LIBRARIES=1 -DCLICKHOUSE_SPLIT_BINARY=1') From 2647d4ca158b56546b335757fd39fb1297e253d1 Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 30 Aug 2019 23:12:26 +0300 Subject: [PATCH 14/35] Rename immutable to readonly --- dbms/src/Common/ErrorCodes.cpp | 2 +- dbms/src/Storages/Kafka/StorageKafka.cpp | 4 ++-- dbms/src/Storages/MergeTree/MergeTreeData.cpp | 6 +++--- dbms/src/Storages/MergeTree/MergeTreeSettings.h | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/dbms/src/Common/ErrorCodes.cpp b/dbms/src/Common/ErrorCodes.cpp index 87ab252c583..76622cfa43b 100644 --- a/dbms/src/Common/ErrorCodes.cpp +++ b/dbms/src/Common/ErrorCodes.cpp @@ -446,7 +446,7 @@ namespace ErrorCodes extern const int VIOLATED_CONSTRAINT = 469; extern const int QUERY_IS_NOT_SUPPORTED_IN_LIVE_VIEW = 470; extern const int SETTINGS_ARE_NOT_SUPPORTED = 471; - extern const int IMMUTABLE_SETTING = 472; + extern const int READONLY_SETTING = 472; extern const int KEEPER_EXCEPTION = 999; extern const int POCO_EXCEPTION = 1000; diff --git a/dbms/src/Storages/Kafka/StorageKafka.cpp b/dbms/src/Storages/Kafka/StorageKafka.cpp index 22238a0a15e..fc43e7046a7 100644 --- a/dbms/src/Storages/Kafka/StorageKafka.cpp +++ b/dbms/src/Storages/Kafka/StorageKafka.cpp @@ -45,7 +45,7 @@ namespace ErrorCodes extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH; extern const int UNSUPPORTED_METHOD; extern const int UNKNOWN_SETTING; - extern const int IMMUTABLE_SETTING; + extern const int READONLY_SETTING; } namespace @@ -417,7 +417,7 @@ void StorageKafka::checkSetting(const String & setting_name) const if (KafkaSettings::findIndex(setting_name) == KafkaSettings::npos) throw Exception{"Storage '" + getName() + "' doesn't have setting '" + setting_name + "'", ErrorCodes::UNKNOWN_SETTING}; - throw Exception{"Setting '" + setting_name + "' is immutable for storage '" + getName() + "'", ErrorCodes::IMMUTABLE_SETTING}; + throw Exception{"Setting '" + setting_name + "' is readonly for storage '" + getName() + "'", ErrorCodes::READONLY_SETTING}; } void registerStorageKafka(StorageFactory & factory) diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.cpp b/dbms/src/Storages/MergeTree/MergeTreeData.cpp index cf2dfc63ac8..a95c526bf20 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeData.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreeData.cpp @@ -91,7 +91,7 @@ namespace ErrorCodes extern const int INCORRECT_FILE_NAME; extern const int BAD_DATA_PART_NAME; extern const int UNKNOWN_SETTING; - extern const int IMMUTABLE_SETTING; + extern const int READONLY_SETTING; } @@ -1661,8 +1661,8 @@ void MergeTreeData::checkSetting(const String & setting_name) const { if (MergeTreeSettings::findIndex(setting_name) == MergeTreeSettings::npos) throw Exception{"Storage '" + getName() + "' doesn't have setting '" + setting_name + "'", ErrorCodes::UNKNOWN_SETTING}; - if (MergeTreeSettings::isImmutableSetting(setting_name)) - throw Exception{"Setting '" + setting_name + "' is immutable for storage '" + getName() + "'", ErrorCodes::IMMUTABLE_SETTING}; + if (MergeTreeSettings::isReadonlySetting(setting_name)) + throw Exception{"Setting '" + setting_name + "' is readonly for storage '" + getName() + "'", ErrorCodes::READONLY_SETTING}; } diff --git a/dbms/src/Storages/MergeTree/MergeTreeSettings.h b/dbms/src/Storages/MergeTree/MergeTreeSettings.h index 6d6157bcc25..8ab7965de04 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeSettings.h +++ b/dbms/src/Storages/MergeTree/MergeTreeSettings.h @@ -101,7 +101,7 @@ struct MergeTreeSettings : public SettingsCollection void loadFromQuery(ASTStorage & storage_def); /// We check settings after storage creation - static bool isImmutableSetting(const String & name) + static bool isReadonlySetting(const String & name) { return name == "index_granularity" || name == "index_granularity_bytes"; } From cc118b3fed1e8e6cb172aabb1503d84c4a8bfa8a Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 01:43:15 +0300 Subject: [PATCH 15/35] Added a test for RENAME / Merge table race condition --- ...1001_rename_merge_race_condition.reference | 0 .../01001_rename_merge_race_condition.sh | 39 +++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 dbms/tests/queries/0_stateless/01001_rename_merge_race_condition.reference create mode 100755 dbms/tests/queries/0_stateless/01001_rename_merge_race_condition.sh diff --git a/dbms/tests/queries/0_stateless/01001_rename_merge_race_condition.reference b/dbms/tests/queries/0_stateless/01001_rename_merge_race_condition.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/dbms/tests/queries/0_stateless/01001_rename_merge_race_condition.sh b/dbms/tests/queries/0_stateless/01001_rename_merge_race_condition.sh new file mode 100755 index 00000000000..b0f1dda7c45 --- /dev/null +++ b/dbms/tests/queries/0_stateless/01001_rename_merge_race_condition.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +. $CURDIR/../shell_config.sh + +set -e + +$CLICKHOUSE_CLIENT --query "DROP TABLE IF EXISTS test1"; +$CLICKHOUSE_CLIENT --query "DROP TABLE IF EXISTS test2"; +$CLICKHOUSE_CLIENT --query "CREATE TABLE test1 (x UInt64) ENGINE = Memory"; + + +function thread1() +{ + while true; do + seq 1 1000 | sed -r -e 's/.+/RENAME TABLE test1 TO test2; RENAME TABLE test2 TO test1;/' | $CLICKHOUSE_CLIENT -n + done +} + +function thread2() +{ + while true; do + $CLICKHOUSE_CLIENT --query "SELECT * FROM merge(currentDatabase(), '^test[12]$')" + done +} + +# https://stackoverflow.com/questions/9954794/execute-a-shell-function-with-timeout +export -f thread1; +export -f thread2; + +TIMEOUT=10 + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & + +wait + +$CLICKHOUSE_CLIENT --query "DROP TABLE IF EXISTS test1"; +$CLICKHOUSE_CLIENT --query "DROP TABLE IF EXISTS test2"; From 2fd8f5c32495da2cc20851a2ad6b68f22c381316 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 02:09:08 +0300 Subject: [PATCH 16/35] Removed code that I don't understand and that has no comments --- dbms/src/Interpreters/InterpreterKillQueryQuery.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/dbms/src/Interpreters/InterpreterKillQueryQuery.cpp b/dbms/src/Interpreters/InterpreterKillQueryQuery.cpp index 4e4120c5580..89339668088 100644 --- a/dbms/src/Interpreters/InterpreterKillQueryQuery.cpp +++ b/dbms/src/Interpreters/InterpreterKillQueryQuery.cpp @@ -265,11 +265,6 @@ Block InterpreterKillQueryQuery::getSelectResult(const String & columns, const S if (where_expression) select_query += " WHERE " + queryToString(where_expression); - auto use_processors = context.getSettingsRef().experimental_use_processors; - context.getSettingsRef().experimental_use_processors = false; - - SCOPE_EXIT(context.getSettingsRef().experimental_use_processors = use_processors); - BlockIO block_io = executeQuery(select_query, context, true); Block res = block_io.in->read(); From fd85a862f039c659d9d699c6ea42aa463658147b Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 02:19:05 +0300 Subject: [PATCH 17/35] Added a test just in case --- .../01003_kill_query_race_condition.reference | 0 .../01003_kill_query_race_condition.sh | 50 +++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 dbms/tests/queries/0_stateless/01003_kill_query_race_condition.reference create mode 100755 dbms/tests/queries/0_stateless/01003_kill_query_race_condition.sh diff --git a/dbms/tests/queries/0_stateless/01003_kill_query_race_condition.reference b/dbms/tests/queries/0_stateless/01003_kill_query_race_condition.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/dbms/tests/queries/0_stateless/01003_kill_query_race_condition.sh b/dbms/tests/queries/0_stateless/01003_kill_query_race_condition.sh new file mode 100755 index 00000000000..9eeafaa8442 --- /dev/null +++ b/dbms/tests/queries/0_stateless/01003_kill_query_race_condition.sh @@ -0,0 +1,50 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +. $CURDIR/../shell_config.sh + +set -e + +function thread1() +{ + while true; do + $CLICKHOUSE_CLIENT --query_id=hello --query "SELECT count() FROM system.numbers"; + done +} + +function thread2() +{ + while true; do + $CLICKHOUSE_CLIENT --query "KILL QUERY WHERE query_id = 'hello'" --format Null; + sleep 0.$RANDOM + done +} + +function thread3() +{ + while true; do + $CLICKHOUSE_CLIENT --query "SHOW PROCESSLIST" --format Null; + $CLICKHOUSE_CLIENT --query "SELECT * FROM system.processes" --format Null; + done +} + +# https://stackoverflow.com/questions/9954794/execute-a-shell-function-with-timeout +export -f thread1; +export -f thread2; +export -f thread3; + +TIMEOUT=10 + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread1 2> /dev/null & + +timeout $TIMEOUT bash -c thread2 2> /dev/null & + +timeout $TIMEOUT bash -c thread3 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & + +wait From bb0ca310ab9406ab6a110657554b9a53cf6307a8 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 02:38:03 +0300 Subject: [PATCH 18/35] Allow to ATTACH live views --- dbms/src/Storages/LiveView/StorageLiveView.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Storages/LiveView/StorageLiveView.cpp b/dbms/src/Storages/LiveView/StorageLiveView.cpp index 1726fe0fba1..b6877c52752 100644 --- a/dbms/src/Storages/LiveView/StorageLiveView.cpp +++ b/dbms/src/Storages/LiveView/StorageLiveView.cpp @@ -593,7 +593,7 @@ void registerStorageLiveView(StorageFactory & factory) { factory.registerStorage("LiveView", [](const StorageFactory::Arguments & args) { - if (!args.local_context.getSettingsRef().allow_experimental_live_view) + if (!args.attach && !args.local_context.getSettingsRef().allow_experimental_live_view) throw Exception("Experimental LIVE VIEW feature is not enabled (the setting 'allow_experimental_live_view')", ErrorCodes::SUPPORT_IS_DISABLED); return StorageLiveView::create(args.table_name, args.database_name, args.local_context, args.query, args.columns); From 5fcdd6f20b58ad2b0b5da4d4ab0373e4ceefe8eb Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 03:19:10 +0300 Subject: [PATCH 19/35] Added stress test variant that is as simple to run as ./stress --- dbms/tests/stress | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100755 dbms/tests/stress diff --git a/dbms/tests/stress b/dbms/tests/stress new file mode 100755 index 00000000000..9099b5a9b91 --- /dev/null +++ b/dbms/tests/stress @@ -0,0 +1,23 @@ +#!/usr/bin/env bash + +# https://stackoverflow.com/questions/360201/how-do-i-kill-background-processes-jobs-when-my-shell-script-exits +trap 'kill -9 $(jobs -p)' EXIT + +function thread() +{ + while true; do + ./clickhouse-test --order random 2>&1 | awk '{ printf "'$1' " }' + done +} + +# https://stackoverflow.com/questions/9954794/execute-a-shell-function-with-timeout +export -f thread; + +NUM_THREADS=16 +TIMEOUT=300 + +for i in $(seq 1 $NUM_THREADS); do + timeout $TIMEOUT bash -c "thread $i" 2> /dev/null & +done + +wait From 57c8091e5b680ab1d9e32b147e4abf0f2b17a273 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 03:30:12 +0300 Subject: [PATCH 20/35] Better stress test script --- dbms/tests/stress | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dbms/tests/stress b/dbms/tests/stress index 9099b5a9b91..90728f7e990 100755 --- a/dbms/tests/stress +++ b/dbms/tests/stress @@ -13,8 +13,8 @@ function thread() # https://stackoverflow.com/questions/9954794/execute-a-shell-function-with-timeout export -f thread; -NUM_THREADS=16 -TIMEOUT=300 +NUM_THREADS=${1:-"16"} +TIMEOUT=${2:-"300"} for i in $(seq 1 $NUM_THREADS); do timeout $TIMEOUT bash -c "thread $i" 2> /dev/null & From fbd616b6a4ae49108ddf3dc54aea769b1dbac786 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Sat, 31 Aug 2019 03:39:38 +0300 Subject: [PATCH 21/35] Add integration test for handling errors by a cache dictionary. --- .../dictionary_preset_cache_xypairs.xml | 31 ++++++++++ .../integration/test_dictionaries/test.py | 59 +++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 dbms/tests/integration/test_dictionaries/configs/dictionaries/dictionary_preset_cache_xypairs.xml diff --git a/dbms/tests/integration/test_dictionaries/configs/dictionaries/dictionary_preset_cache_xypairs.xml b/dbms/tests/integration/test_dictionaries/configs/dictionaries/dictionary_preset_cache_xypairs.xml new file mode 100644 index 00000000000..4142706259a --- /dev/null +++ b/dbms/tests/integration/test_dictionaries/configs/dictionaries/dictionary_preset_cache_xypairs.xml @@ -0,0 +1,31 @@ + + + cache_xypairs + + + localhost + 9000 + default + + test + xypairs
+ + + 1 + + + 5 + + + + + x + + + y + UInt64 + 0 + + + + diff --git a/dbms/tests/integration/test_dictionaries/test.py b/dbms/tests/integration/test_dictionaries/test.py index 251fe7f31ee..95f82f65c0d 100644 --- a/dbms/tests/integration/test_dictionaries/test.py +++ b/dbms/tests/integration/test_dictionaries/test.py @@ -17,6 +17,10 @@ def get_status(dictionary_name): return instance.query("SELECT status FROM system.dictionaries WHERE name='" + dictionary_name + "'").rstrip("\n") +def get_last_exception(dictionary_name): + return instance.query("SELECT last_exception FROM system.dictionaries WHERE name='" + dictionary_name + "'").rstrip("\n").replace("\\'", "'") + + def get_loading_start_time(dictionary_name): s = instance.query("SELECT loading_start_time FROM system.dictionaries WHERE name='" + dictionary_name + "'").rstrip("\n") if s == "0000-00-00 00:00:00": @@ -350,3 +354,58 @@ def test_reload_after_fail_by_timer(started_cluster): time.sleep(6); query("SELECT dictGetInt32('no_file_2', 'a', toUInt64(9))") == "10\n" assert get_status("no_file_2") == "LOADED" + + +def test_reload_after_fail_in_cache_dictionary(started_cluster): + query = instance.query + query_and_get_error = instance.query_and_get_error + + # Can't get a value from the cache dictionary because the source (table `test.xypairs`) doesn't respond. + expected_error = "Table test.xypairs doesn't exist" + assert expected_error in query_and_get_error("SELECT dictGetUInt64('cache_xypairs', 'y', toUInt64(1))") + assert get_status("cache_xypairs") == "LOADED" + assert expected_error in get_last_exception("cache_xypairs") + + # Create table `test.xypairs`. + query(''' + drop table if exists test.xypairs; + create table test.xypairs (x UInt64, y UInt64) engine=Log; + insert into test.xypairs values (1, 56), (3, 78); + ''') + + # Cache dictionary now works. + assert_eq_with_retry(instance, "SELECT dictGet('cache_xypairs', 'y', toUInt64(1))", "56", ignore_error=True) + query("SELECT dictGet('cache_xypairs', 'y', toUInt64(2))") == "0" + assert get_last_exception("cache_xypairs") == "" + + # Drop table `test.xypairs`. + query('drop table if exists test.xypairs') + + # Values are cached so we can get them. + query("SELECT dictGet('cache_xypairs', 'y', toUInt64(1))") == "56" + query("SELECT dictGet('cache_xypairs', 'y', toUInt64(2))") == "0" + assert get_last_exception("cache_xypairs") == "" + + # But we can't get a value from the source table which isn't cached. + assert expected_error in query_and_get_error("SELECT dictGetUInt64('cache_xypairs', 'y', toUInt64(3))") + assert expected_error in get_last_exception("cache_xypairs") + + # Passed time should not spoil the cache. + time.sleep(5); + query("SELECT dictGet('cache_xypairs', 'y', toUInt64(1))") == "56" + query("SELECT dictGet('cache_xypairs', 'y', toUInt64(2))") == "0" + assert expected_error in query_and_get_error("SELECT dictGetUInt64('cache_xypairs', 'y', toUInt64(3))") + assert expected_error in get_last_exception("cache_xypairs") + + # Create table `test.xypairs` again with changed values. + query(''' + drop table if exists test.xypairs; + create table test.xypairs (x UInt64, y UInt64) engine=Log; + insert into test.xypairs values (1, 57), (3, 79); + ''') + + # The cache dictionary returns new values now. + assert_eq_with_retry(instance, "SELECT dictGet('cache_xypairs', 'y', toUInt64(1))", "57") + query("SELECT dictGet('cache_xypairs', 'y', toUInt64(2))") == "0" + query("SELECT dictGet('cache_xypairs', 'y', toUInt64(3))") == "79" + assert get_last_exception("cache_xypairs") == "" From e4376a3f6f42ed2a9069f5f8242695461b8d9bf4 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 04:21:10 +0300 Subject: [PATCH 22/35] Addition to prev. revision --- .../queries/0_stateless/01003_kill_query_race_condition.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/tests/queries/0_stateless/01003_kill_query_race_condition.sh b/dbms/tests/queries/0_stateless/01003_kill_query_race_condition.sh index 9eeafaa8442..d8a73ac24a4 100755 --- a/dbms/tests/queries/0_stateless/01003_kill_query_race_condition.sh +++ b/dbms/tests/queries/0_stateless/01003_kill_query_race_condition.sh @@ -8,7 +8,7 @@ set -e function thread1() { while true; do - $CLICKHOUSE_CLIENT --query_id=hello --query "SELECT count() FROM system.numbers"; + $CLICKHOUSE_CLIENT --query_id=hello --query "SELECT count() FROM numbers(1000000000)" --format Null; done } From d879bcb010fdf27f5d467e2f658daeed2fd67b1e Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 05:32:42 +0300 Subject: [PATCH 23/35] Added a test for deadlock in RENAME TABLE --- .../01004_rename_deadlock.reference | 0 .../0_stateless/01004_rename_deadlock.sh | 60 +++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 dbms/tests/queries/0_stateless/01004_rename_deadlock.reference create mode 100755 dbms/tests/queries/0_stateless/01004_rename_deadlock.sh diff --git a/dbms/tests/queries/0_stateless/01004_rename_deadlock.reference b/dbms/tests/queries/0_stateless/01004_rename_deadlock.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh b/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh new file mode 100755 index 00000000000..b09fabe3d9e --- /dev/null +++ b/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +. $CURDIR/../shell_config.sh + +set -e + +$CLICKHOUSE_CLIENT --query "DROP TABLE IF EXISTS test1"; +$CLICKHOUSE_CLIENT --query "DROP TABLE IF EXISTS test2"; +$CLICKHOUSE_CLIENT --query "CREATE TABLE test1 (x UInt8) ENGINE = MergeTree ORDER BY x"; +$CLICKHOUSE_CLIENT --query "CREATE TABLE test2 (x UInt8) ENGINE = MergeTree ORDER BY x"; + +function thread1() +{ + while true; do + $CLICKHOUSE_CLIENT --query "RENAME TABLE test1 TO test_tmp, test2 TO test1, test_tmp TO test2" + done +} + +function thread2() +{ + while true; do + $CLICKHOUSE_CLIENT --query "SELECT * FROM test1 UNION ALL SELECT * FROM test2" --format Null + done +} + +function thread3() +{ + while true; do + $CLICKHOUSE_CLIENT --query "SELECT * FROM system.tables" --format Null + done +} + +# https://stackoverflow.com/questions/9954794/execute-a-shell-function-with-timeout +export -f thread1; +export -f thread2; +export -f thread3; + +TIMEOUT=1000 + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & + +wait + +$CLICKHOUSE_CLIENT -q "DROP TABLE test1" +$CLICKHOUSE_CLIENT -q "DROP TABLE test2" From 202673e3bd40c57dd738bb4a2adf19f973bd53e7 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 13:37:28 +0300 Subject: [PATCH 24/35] Avoid deadlock in multiple tables RENAME --- .../Interpreters/InterpreterRenameQuery.cpp | 38 ++++++------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/dbms/src/Interpreters/InterpreterRenameQuery.cpp b/dbms/src/Interpreters/InterpreterRenameQuery.cpp index e763c002209..3e8bc3a88fb 100644 --- a/dbms/src/Interpreters/InterpreterRenameQuery.cpp +++ b/dbms/src/Interpreters/InterpreterRenameQuery.cpp @@ -26,8 +26,6 @@ struct RenameDescription to_table_name(elem.to.table) {} - TableStructureWriteLockHolder from_table_lock; - String from_database_name; String from_table_name; @@ -77,8 +75,6 @@ BlockIO InterpreterRenameQuery::execute() } }; - std::map tables_from_locks; - /// Don't allow to drop tables (that we are renaming); don't allow to create tables in places where tables will be renamed. std::map> table_guards; @@ -89,36 +85,26 @@ BlockIO InterpreterRenameQuery::execute() UniqueTableName from(descriptions.back().from_database_name, descriptions.back().from_table_name); UniqueTableName to(descriptions.back().to_database_name, descriptions.back().to_table_name); - if (!tables_from_locks.count(from)) - if (auto table = context.tryGetTable(from.database_name, from.table_name)) - tables_from_locks.emplace(from, table->lockExclusively(context.getCurrentQueryId())); - - descriptions.back().from_table_lock = tables_from_locks[from]; - - if (!table_guards.count(from)) - table_guards.emplace(from, context.getDDLGuard(from.database_name, from.table_name)); - - if (!table_guards.count(to)) - table_guards.emplace(to, context.getDDLGuard(to.database_name, to.table_name)); + table_guards[from]; + table_guards[to]; } - /** All tables are locked. If there are more than one rename in chain, - * we need to hold global lock while doing all renames. Order matters to avoid deadlocks. - * It provides atomicity of all RENAME chain as a whole, from the point of view of DBMS client, - * but only in cases when there was no exceptions during this process and server does not fall. - */ - - decltype(context.getLock()) lock; - - if (descriptions.size() > 1) - lock = context.getLock(); + /// Must do it in consistent order. + for (auto & table_guard : table_guards) + table_guard.second = context.getDDLGuard(table_guard.first.database_name, table_guard.first.table_name); for (auto & elem : descriptions) { context.assertTableDoesntExist(elem.to_database_name, elem.to_table_name); + auto from_table = context.getTable(elem.from_database_name, elem.from_table_name); + auto from_table_lock = from_table->lockExclusively(context.getCurrentQueryId()); context.getDatabase(elem.from_database_name)->renameTable( - context, elem.from_table_name, *context.getDatabase(elem.to_database_name), elem.to_table_name, elem.from_table_lock); + context, + elem.from_table_name, + *context.getDatabase(elem.to_database_name), + elem.to_table_name, + from_table_lock); } return {}; From 3568d3d890c6d3e7f657e397b6882d7aad1985ba Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 13:38:20 +0300 Subject: [PATCH 25/35] Updated test --- dbms/tests/queries/0_stateless/01004_rename_deadlock.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh b/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh index b09fabe3d9e..5d5726bb001 100755 --- a/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh +++ b/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh @@ -36,7 +36,7 @@ export -f thread1; export -f thread2; export -f thread3; -TIMEOUT=1000 +TIMEOUT=10 timeout $TIMEOUT bash -c thread1 2> /dev/null & timeout $TIMEOUT bash -c thread2 2> /dev/null & From c0e465f9f00116ce636f5e52932767c8290fcd65 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 14:20:09 +0300 Subject: [PATCH 26/35] Stress test: more beautiful --- dbms/tests/stress | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/tests/stress b/dbms/tests/stress index 90728f7e990..4a6ad411298 100755 --- a/dbms/tests/stress +++ b/dbms/tests/stress @@ -6,7 +6,7 @@ trap 'kill -9 $(jobs -p)' EXIT function thread() { while true; do - ./clickhouse-test --order random 2>&1 | awk '{ printf "'$1' " }' + ./clickhouse-test --order random 2>&1 | awk '/^\w+:/ { printf("\033[0;%s%sm \033[0m", ('$1' % 2 ? "4" : "10"), (int('$1' / 2) % 8)) }' done } From 783df7a5c541d1ceec50cd562cd99ffee0e16726 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 14:32:14 +0300 Subject: [PATCH 27/35] Added a test that prooves that our locking model is non-viable --- .../01005_rwr_shard_deadlock.reference | 0 .../0_stateless/01005_rwr_shard_deadlock.sh | 46 +++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 dbms/tests/queries/0_stateless/01005_rwr_shard_deadlock.reference create mode 100755 dbms/tests/queries/0_stateless/01005_rwr_shard_deadlock.sh diff --git a/dbms/tests/queries/0_stateless/01005_rwr_shard_deadlock.reference b/dbms/tests/queries/0_stateless/01005_rwr_shard_deadlock.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/dbms/tests/queries/0_stateless/01005_rwr_shard_deadlock.sh b/dbms/tests/queries/0_stateless/01005_rwr_shard_deadlock.sh new file mode 100755 index 00000000000..1afd3acd324 --- /dev/null +++ b/dbms/tests/queries/0_stateless/01005_rwr_shard_deadlock.sh @@ -0,0 +1,46 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +. $CURDIR/../shell_config.sh + +set -e + +$CLICKHOUSE_CLIENT --query "DROP TABLE IF EXISTS test1"; +$CLICKHOUSE_CLIENT --query "CREATE TABLE test1 (x UInt8) ENGINE = MergeTree ORDER BY tuple()"; + +function thread1() +{ + while true; do + $CLICKHOUSE_CLIENT --query "ALTER TABLE test1 MODIFY COLUMN x Nullable(UInt8)" + $CLICKHOUSE_CLIENT --query "ALTER TABLE test1 MODIFY COLUMN x UInt8" + done +} + +function thread2() +{ + while true; do + $CLICKHOUSE_CLIENT --query "SELECT x FROM test1 WHERE x IN (SELECT x FROM remote('127.0.0.2', currentDatabase(), test1))" --format Null + done +} + +# https://stackoverflow.com/questions/9954794/execute-a-shell-function-with-timeout +export -f thread1; +export -f thread2; + +TIMEOUT=10 + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & + +wait + +$CLICKHOUSE_CLIENT -q "DROP TABLE test1" From aac0b27daaca3af5bb163d8b6fc2eb07ee6867fc Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 15:18:14 +0300 Subject: [PATCH 28/35] Fixed possible deadlock in distributed queries --- dbms/src/DataStreams/PushingToViewsBlockOutputStream.cpp | 2 +- dbms/src/Functions/FunctionJoinGet.cpp | 2 +- dbms/src/Interpreters/Context.cpp | 6 ++++++ dbms/src/Interpreters/Context.h | 4 ++++ dbms/src/Interpreters/InterpreterDescribeQuery.cpp | 2 +- dbms/src/Interpreters/InterpreterInsertQuery.cpp | 2 +- dbms/src/Interpreters/InterpreterSelectQuery.cpp | 2 +- 7 files changed, 15 insertions(+), 5 deletions(-) diff --git a/dbms/src/DataStreams/PushingToViewsBlockOutputStream.cpp b/dbms/src/DataStreams/PushingToViewsBlockOutputStream.cpp index 807a9129a75..6c3012a481e 100644 --- a/dbms/src/DataStreams/PushingToViewsBlockOutputStream.cpp +++ b/dbms/src/DataStreams/PushingToViewsBlockOutputStream.cpp @@ -26,7 +26,7 @@ PushingToViewsBlockOutputStream::PushingToViewsBlockOutputStream( * Although now any insertion into the table is done via PushingToViewsBlockOutputStream, * but it's clear that here is not the best place for this functionality. */ - addTableLock(storage->lockStructureForShare(true, context.getCurrentQueryId())); + addTableLock(storage->lockStructureForShare(true, context.getInitialQueryId())); /// If the "root" table deduplactes blocks, there are no need to make deduplication for children /// Moreover, deduplication for AggregatingMergeTree children could produce false positives due to low size of inserting blocks diff --git a/dbms/src/Functions/FunctionJoinGet.cpp b/dbms/src/Functions/FunctionJoinGet.cpp index 5201a0ba5c2..0aad01c62f3 100644 --- a/dbms/src/Functions/FunctionJoinGet.cpp +++ b/dbms/src/Functions/FunctionJoinGet.cpp @@ -65,7 +65,7 @@ FunctionBasePtr FunctionBuilderJoinGet::buildImpl(const ColumnsWithTypeAndName & auto join = storage_join->getJoin(); DataTypes data_types(arguments.size()); - auto table_lock = storage_join->lockStructureForShare(false, context.getCurrentQueryId()); + auto table_lock = storage_join->lockStructureForShare(false, context.getInitialQueryId()); for (size_t i = 0; i < arguments.size(); ++i) data_types[i] = arguments[i].type; diff --git a/dbms/src/Interpreters/Context.cpp b/dbms/src/Interpreters/Context.cpp index 016f1fa0e49..d7c713abb3d 100644 --- a/dbms/src/Interpreters/Context.cpp +++ b/dbms/src/Interpreters/Context.cpp @@ -1162,6 +1162,12 @@ String Context::getCurrentQueryId() const } +String Context::getInitialQueryId() const +{ + return client_info.initial_query_id; +} + + void Context::setCurrentDatabase(const String & name) { auto lock = getLock(); diff --git a/dbms/src/Interpreters/Context.h b/dbms/src/Interpreters/Context.h index f60f16e5b29..f7ba0a7dbaa 100644 --- a/dbms/src/Interpreters/Context.h +++ b/dbms/src/Interpreters/Context.h @@ -264,6 +264,10 @@ public: String getCurrentDatabase() const; String getCurrentQueryId() const; + + /// Id of initiating query for distributed queries; or current query id if it's not a distributed query. + String getInitialQueryId() const; + void setCurrentDatabase(const String & name); void setCurrentQueryId(const String & query_id); diff --git a/dbms/src/Interpreters/InterpreterDescribeQuery.cpp b/dbms/src/Interpreters/InterpreterDescribeQuery.cpp index faec6f96045..b7f51674a75 100644 --- a/dbms/src/Interpreters/InterpreterDescribeQuery.cpp +++ b/dbms/src/Interpreters/InterpreterDescribeQuery.cpp @@ -92,7 +92,7 @@ BlockInputStreamPtr InterpreterDescribeQuery::executeImpl() table = context.getTable(database_name, table_name); } - auto table_lock = table->lockStructureForShare(false, context.getCurrentQueryId()); + auto table_lock = table->lockStructureForShare(false, context.getInitialQueryId()); columns = table->getColumns(); } diff --git a/dbms/src/Interpreters/InterpreterInsertQuery.cpp b/dbms/src/Interpreters/InterpreterInsertQuery.cpp index f7edca14089..6fa55b2d8e0 100644 --- a/dbms/src/Interpreters/InterpreterInsertQuery.cpp +++ b/dbms/src/Interpreters/InterpreterInsertQuery.cpp @@ -100,7 +100,7 @@ BlockIO InterpreterInsertQuery::execute() checkAccess(query); StoragePtr table = getTable(query); - auto table_lock = table->lockStructureForShare(true, context.getCurrentQueryId()); + auto table_lock = table->lockStructureForShare(true, context.getInitialQueryId()); /// We create a pipeline of several streams, into which we will write data. BlockOutputStreamPtr out; diff --git a/dbms/src/Interpreters/InterpreterSelectQuery.cpp b/dbms/src/Interpreters/InterpreterSelectQuery.cpp index 79fbcf44323..69613c73705 100644 --- a/dbms/src/Interpreters/InterpreterSelectQuery.cpp +++ b/dbms/src/Interpreters/InterpreterSelectQuery.cpp @@ -294,7 +294,7 @@ InterpreterSelectQuery::InterpreterSelectQuery( } if (storage) - table_lock = storage->lockStructureForShare(false, context.getCurrentQueryId()); + table_lock = storage->lockStructureForShare(false, context.getInitialQueryId()); syntax_analyzer_result = SyntaxAnalyzer(context, options).analyze( query_ptr, source_header.getNamesAndTypesList(), required_result_column_names, storage, NamesAndTypesList()); From 7382a9f3c3b5502f440cf5be1788c36dfd597e94 Mon Sep 17 00:00:00 2001 From: alesapin Date: Sun, 1 Sep 2019 00:15:40 +0300 Subject: [PATCH 29/35] Rename method for settings check --- dbms/src/Storages/AlterCommands.cpp | 2 +- dbms/src/Storages/IStorage.cpp | 4 ++-- dbms/src/Storages/IStorage.h | 2 +- dbms/src/Storages/Kafka/StorageKafka.cpp | 2 +- dbms/src/Storages/Kafka/StorageKafka.h | 2 +- dbms/src/Storages/MergeTree/MergeTreeData.cpp | 4 ++-- dbms/src/Storages/MergeTree/MergeTreeData.h | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/dbms/src/Storages/AlterCommands.cpp b/dbms/src/Storages/AlterCommands.cpp index 2a311461f00..bcfd852a628 100644 --- a/dbms/src/Storages/AlterCommands.cpp +++ b/dbms/src/Storages/AlterCommands.cpp @@ -544,7 +544,7 @@ void AlterCommands::validate(const IStorage & table, const Context & context) } else if (command.type == AlterCommand::MODIFY_SETTING) for (const auto & change : command.settings_changes) - table.checkSetting(change.name); + table.checkSettingCanBeChanged(change.name); } /** Existing defaulted columns may require default expression extensions with a type conversion, diff --git a/dbms/src/Storages/IStorage.cpp b/dbms/src/Storages/IStorage.cpp index 2fb737de71b..cbd14666006 100644 --- a/dbms/src/Storages/IStorage.cpp +++ b/dbms/src/Storages/IStorage.cpp @@ -309,7 +309,7 @@ bool IStorage::isVirtualColumn(const String & column_name) const return getColumns().get(column_name).is_virtual; } -void IStorage::checkSetting(const String & /* setting_name */) const +void IStorage::checkSettingCanBeChanged(const String & /* setting_name */) const { if (!supportsSettings()) throw Exception("Storage '" + getName() + "' doesn't support settings.", ErrorCodes::SETTINGS_ARE_NOT_SUPPORTED); @@ -380,7 +380,7 @@ IDatabase::ASTModifier IStorage::getSettingsModifier(const SettingsChanges & new /// Make storage settings unique for (const auto & change : new_changes) { - checkSetting(change.name); + checkSettingCanBeChanged(change.name); auto finder = [&change] (const SettingChange & c) { return c.name == change.name; }; if (auto it = std::find_if(storage_changes.begin(), storage_changes.end(), finder); it != storage_changes.end()) diff --git a/dbms/src/Storages/IStorage.h b/dbms/src/Storages/IStorage.h index f0dcef1f629..d92b06029d8 100644 --- a/dbms/src/Storages/IStorage.h +++ b/dbms/src/Storages/IStorage.h @@ -140,7 +140,7 @@ public: /// thread-unsafe part. lockStructure must be acquired void check(const Block & block, bool need_all = false) const; /// Check storage has setting and setting can be modified. - virtual void checkSetting(const String & setting_name) const; + virtual void checkSettingCanBeChanged(const String & setting_name) const; protected: /// still thread-unsafe part. void setIndices(IndicesDescription indices_); diff --git a/dbms/src/Storages/Kafka/StorageKafka.cpp b/dbms/src/Storages/Kafka/StorageKafka.cpp index fc43e7046a7..10b4381dd2d 100644 --- a/dbms/src/Storages/Kafka/StorageKafka.cpp +++ b/dbms/src/Storages/Kafka/StorageKafka.cpp @@ -412,7 +412,7 @@ bool StorageKafka::streamToViews() } -void StorageKafka::checkSetting(const String & setting_name) const +void StorageKafka::checkSettingCanBeChanged(const String & setting_name) const { if (KafkaSettings::findIndex(setting_name) == KafkaSettings::npos) throw Exception{"Storage '" + getName() + "' doesn't have setting '" + setting_name + "'", ErrorCodes::UNKNOWN_SETTING}; diff --git a/dbms/src/Storages/Kafka/StorageKafka.h b/dbms/src/Storages/Kafka/StorageKafka.h index 76983ac4b9e..e8799983705 100644 --- a/dbms/src/Storages/Kafka/StorageKafka.h +++ b/dbms/src/Storages/Kafka/StorageKafka.h @@ -57,7 +57,7 @@ public: const auto & getSchemaName() const { return schema_name; } const auto & skipBroken() const { return skip_broken; } - void checkSetting(const String & setting_name) const override; + void checkSettingCanBeChanged(const String & setting_name) const override; protected: StorageKafka( diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.cpp b/dbms/src/Storages/MergeTree/MergeTreeData.cpp index a95c526bf20..e5550ce91b4 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeData.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreeData.cpp @@ -1325,7 +1325,7 @@ void MergeTreeData::checkAlter(const AlterCommands & commands, const Context & c setTTLExpressions(new_columns.getColumnTTLs(), new_ttl_table_ast, /* only_check = */ true); for (const auto & setting : new_changes) - checkSetting(setting.name); + checkSettingCanBeChanged(setting.name); /// Check that type conversions are possible. ExpressionActionsPtr unused_expression; @@ -1657,7 +1657,7 @@ void MergeTreeData::changeSettings( } } -void MergeTreeData::checkSetting(const String & setting_name) const +void MergeTreeData::checkSettingCanBeChanged(const String & setting_name) const { if (MergeTreeSettings::findIndex(setting_name) == MergeTreeSettings::npos) throw Exception{"Storage '" + getName() + "' doesn't have setting '" + setting_name + "'", ErrorCodes::UNKNOWN_SETTING}; diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.h b/dbms/src/Storages/MergeTree/MergeTreeData.h index 69c5449a751..ffca5de1a16 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeData.h +++ b/dbms/src/Storages/MergeTree/MergeTreeData.h @@ -543,7 +543,7 @@ public: TableStructureWriteLockHolder & table_lock_holder); /// All MergeTreeData children have settings. - void checkSetting(const String & setting_name) const override; + void checkSettingCanBeChanged(const String & setting_name) const override; /// Remove columns, that have been markedd as empty after zeroing values with expired ttl void removeEmptyColumnsFromPart(MergeTreeData::MutableDataPartPtr & data_part); From dcc6163d326965811f759b42032d9735afd3f998 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sun, 1 Sep 2019 00:39:17 +0300 Subject: [PATCH 30/35] Added function "trap" --- .../registerFunctionsIntrospection.cpp | 2 + dbms/src/Functions/trap.cpp | 143 ++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 dbms/src/Functions/trap.cpp diff --git a/dbms/src/Functions/registerFunctionsIntrospection.cpp b/dbms/src/Functions/registerFunctionsIntrospection.cpp index 448400b37ab..700a568d822 100644 --- a/dbms/src/Functions/registerFunctionsIntrospection.cpp +++ b/dbms/src/Functions/registerFunctionsIntrospection.cpp @@ -6,12 +6,14 @@ class FunctionFactory; void registerFunctionAddressToSymbol(FunctionFactory & factory); void registerFunctionDemangle(FunctionFactory & factory); void registerFunctionAddressToLine(FunctionFactory & factory); +void registerFunctionTrap(FunctionFactory & factory); void registerFunctionsIntrospection(FunctionFactory & factory) { registerFunctionAddressToSymbol(factory); registerFunctionDemangle(factory); registerFunctionAddressToLine(factory); + registerFunctionTrap(factory); } } diff --git a/dbms/src/Functions/trap.cpp b/dbms/src/Functions/trap.cpp new file mode 100644 index 00000000000..e05d5efa4f7 --- /dev/null +++ b/dbms/src/Functions/trap.cpp @@ -0,0 +1,143 @@ +#if 0 + +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include + + +namespace DB +{ + +namespace ErrorCodes +{ + extern const int ILLEGAL_COLUMN; + extern const int ILLEGAL_TYPE_OF_ARGUMENT; + extern const int BAD_ARGUMENTS; +} + + +/// Various illegal actions to test diagnostic features of ClickHouse itself. Should not be enabled in production builds. +class FunctionTrap : public IFunction +{ +public: + static constexpr auto name = "trap"; + static FunctionPtr create(const Context &) + { + return std::make_shared(); + } + + String getName() const override + { + return name; + } + + size_t getNumberOfArguments() const override + { + return 1; + } + + DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override + { + if (!isString(arguments[0])) + throw Exception("The only argument for function " + getName() + " must be constant String", ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); + + return std::make_shared(); + } + + void executeImpl(Block & block, const ColumnNumbers & arguments, size_t result, size_t input_rows_count) override + { + if (const ColumnConst * column = checkAndGetColumnConst(block.getByPosition(arguments[0]).column.get())) + { + String mode = column->getValue(); + + if (mode == "read nullptr c++") + { + volatile int x = *reinterpret_cast(0); + (void)x; + } + else if (mode == "read nullptr asm") + { + __asm__ volatile ("movq $0, %rax"); + __asm__ volatile ("movq (%rax), %rax"); + } + else if (mode == "illegal instruction") + { + __asm__ volatile ("ud2a"); + } + else if (mode == "abort") + { + abort(); + } + else if (mode == "use after free") + { + int * x_ptr; + { + auto x = std::make_unique(); + x_ptr = x.get(); + } + *x_ptr = 1; + (void)x_ptr; + } + else if (mode == "use after scope") + { + volatile int * x_ptr; + [&]{ + volatile int x = 0; + x_ptr = &x; + (void)x; + }(); + [&]{ + volatile int y = 1; + *x_ptr = 2; + (void)y; + }(); + (void)x_ptr; + } + else if (mode == "uninitialized memory") + { + int x; + (void)write(2, &x, sizeof(x)); + } + else if (mode == "data race") + { + int x = 0; + std::thread t1([&]{ ++x; }); + std::thread t2([&]{ ++x; }); + t1.join(); + t2.join(); + } + else + throw Exception("Unknown trap mode", ErrorCodes::BAD_ARGUMENTS); + } + else + throw Exception("The only argument for function " + getName() + " must be constant String", ErrorCodes::ILLEGAL_COLUMN); + + block.getByPosition(result).column = block.getByPosition(result).type->createColumnConst(input_rows_count, 0ULL); + } +}; + + +void registerFunctionTrap(FunctionFactory & factory) +{ + factory.registerFunction(); +} + +} + +#else + +namespace DB +{ + class FunctionFactory; + void registerFunctionTrap(FunctionFactory &) {} +} + +#endif From cb79e2371ed1899b7ee0fa3c9647dddada218993 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sun, 1 Sep 2019 00:47:15 +0300 Subject: [PATCH 31/35] Add "os_thread_ids" column to system tables --- dbms/src/Common/ThreadStatus.h | 1 + dbms/src/Interpreters/ProcessList.cpp | 1 + dbms/src/Interpreters/ProcessList.h | 1 + dbms/src/Interpreters/QueryLog.cpp | 9 +++++++++ dbms/src/Interpreters/QueryLog.h | 1 + dbms/src/Interpreters/ThreadStatusExt.cpp | 2 ++ dbms/src/Interpreters/executeQuery.cpp | 2 ++ dbms/src/Storages/System/StorageSystemProcesses.cpp | 9 +++++++++ 8 files changed, 26 insertions(+) diff --git a/dbms/src/Common/ThreadStatus.h b/dbms/src/Common/ThreadStatus.h index f175db74771..2ba55fa07d0 100644 --- a/dbms/src/Common/ThreadStatus.h +++ b/dbms/src/Common/ThreadStatus.h @@ -61,6 +61,7 @@ public: InternalTextLogsQueueWeakPtr logs_queue_ptr; std::vector thread_numbers; + std::vector os_thread_ids; /// The first thread created this thread group UInt32 master_thread_number = 0; diff --git a/dbms/src/Interpreters/ProcessList.cpp b/dbms/src/Interpreters/ProcessList.cpp index 71376c6d129..100ecc00dc1 100644 --- a/dbms/src/Interpreters/ProcessList.cpp +++ b/dbms/src/Interpreters/ProcessList.cpp @@ -444,6 +444,7 @@ QueryStatusInfo QueryStatus::getInfo(bool get_thread_list, bool get_profile_even { std::lock_guard lock(thread_group->mutex); res.thread_numbers = thread_group->thread_numbers; + res.os_thread_ids = thread_group->os_thread_ids; } if (get_profile_events) diff --git a/dbms/src/Interpreters/ProcessList.h b/dbms/src/Interpreters/ProcessList.h index 4cdf7c18fea..d5631abdb0c 100644 --- a/dbms/src/Interpreters/ProcessList.h +++ b/dbms/src/Interpreters/ProcessList.h @@ -66,6 +66,7 @@ struct QueryStatusInfo /// Optional fields, filled by request std::vector thread_numbers; + std::vector os_thread_ids; std::shared_ptr profile_counters; std::shared_ptr query_settings; }; diff --git a/dbms/src/Interpreters/QueryLog.cpp b/dbms/src/Interpreters/QueryLog.cpp index 52e552d833f..7cca320b04b 100644 --- a/dbms/src/Interpreters/QueryLog.cpp +++ b/dbms/src/Interpreters/QueryLog.cpp @@ -78,6 +78,7 @@ Block QueryLogElement::createBlock() {std::make_shared(), "revision"}, {std::make_shared(std::make_shared()), "thread_numbers"}, + {std::make_shared(std::make_shared()), "os_thread_ids"}, {std::make_shared(std::make_shared()), "ProfileEvents.Names"}, {std::make_shared(std::make_shared()), "ProfileEvents.Values"}, {std::make_shared(std::make_shared()), "Settings.Names"}, @@ -123,6 +124,14 @@ void QueryLogElement::appendToBlock(Block & block) const columns[i++]->insert(threads_array); } + { + Array threads_array; + threads_array.reserve(os_thread_ids.size()); + for (const UInt32 thread_number : os_thread_ids) + threads_array.emplace_back(UInt64(thread_number)); + columns[i++]->insert(threads_array); + } + if (profile_counters) { auto column_names = columns[i++].get(); diff --git a/dbms/src/Interpreters/QueryLog.h b/dbms/src/Interpreters/QueryLog.h index 95d563fd21e..2b8f8120050 100644 --- a/dbms/src/Interpreters/QueryLog.h +++ b/dbms/src/Interpreters/QueryLog.h @@ -60,6 +60,7 @@ struct QueryLogElement ClientInfo client_info; std::vector thread_numbers; + std::vector os_thread_ids; std::shared_ptr profile_counters; std::shared_ptr query_settings; diff --git a/dbms/src/Interpreters/ThreadStatusExt.cpp b/dbms/src/Interpreters/ThreadStatusExt.cpp index 28740417b71..1407d0d2073 100644 --- a/dbms/src/Interpreters/ThreadStatusExt.cpp +++ b/dbms/src/Interpreters/ThreadStatusExt.cpp @@ -61,6 +61,7 @@ void ThreadStatus::initializeQuery() thread_group->memory_tracker.setDescription("(for query)"); thread_group->thread_numbers.emplace_back(thread_number); + thread_group->os_thread_ids.emplace_back(os_thread_id); thread_group->master_thread_number = thread_number; thread_group->master_thread_os_id = os_thread_id; @@ -99,6 +100,7 @@ void ThreadStatus::attachQuery(const ThreadGroupStatusPtr & thread_group_, bool /// NOTE: A thread may be attached multiple times if it is reused from a thread pool. thread_group->thread_numbers.emplace_back(thread_number); + thread_group->os_thread_ids.emplace_back(os_thread_id); } if (query_context) diff --git a/dbms/src/Interpreters/executeQuery.cpp b/dbms/src/Interpreters/executeQuery.cpp index 36bdcc27634..07445aac646 100644 --- a/dbms/src/Interpreters/executeQuery.cpp +++ b/dbms/src/Interpreters/executeQuery.cpp @@ -400,6 +400,7 @@ static std::tuple executeQueryImpl( } elem.thread_numbers = std::move(info.thread_numbers); + elem.os_thread_ids = std::move(info.os_thread_ids); elem.profile_counters = std::move(info.profile_counters); if (log_queries) @@ -437,6 +438,7 @@ static std::tuple executeQueryImpl( elem.memory_usage = info.peak_memory_usage > 0 ? info.peak_memory_usage : 0; elem.thread_numbers = std::move(info.thread_numbers); + elem.os_thread_ids = std::move(info.os_thread_ids); elem.profile_counters = std::move(info.profile_counters); } diff --git a/dbms/src/Storages/System/StorageSystemProcesses.cpp b/dbms/src/Storages/System/StorageSystemProcesses.cpp index 2450ec9296e..56905b29349 100644 --- a/dbms/src/Storages/System/StorageSystemProcesses.cpp +++ b/dbms/src/Storages/System/StorageSystemProcesses.cpp @@ -58,6 +58,7 @@ NamesAndTypesList StorageSystemProcesses::getNamesAndTypes() {"query", std::make_shared()}, {"thread_numbers", std::make_shared(std::make_shared())}, + {"os_thread_ids", std::make_shared(std::make_shared())}, {"ProfileEvents.Names", std::make_shared(std::make_shared())}, {"ProfileEvents.Values", std::make_shared(std::make_shared())}, {"Settings.Names", std::make_shared(std::make_shared())}, @@ -120,6 +121,14 @@ void StorageSystemProcesses::fillData(MutableColumns & res_columns, const Contex res_columns[i++]->insert(threads_array); } + { + Array threads_array; + threads_array.reserve(process.os_thread_ids.size()); + for (const UInt32 thread_number : process.os_thread_ids) + threads_array.emplace_back(thread_number); + res_columns[i++]->insert(threads_array); + } + { IColumn * column_profile_events_names = res_columns[i++].get(); IColumn * column_profile_events_values = res_columns[i++].get(); From 4d21f97d6aeefeafa92b76370e06d7a3ff48f9db Mon Sep 17 00:00:00 2001 From: Ivan <5627721+abyss7@users.noreply.github.com> Date: Sun, 1 Sep 2019 16:03:38 +0300 Subject: [PATCH 32/35] Increase test timeout --- dbms/tests/integration/test_storage_kafka/test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/tests/integration/test_storage_kafka/test.py b/dbms/tests/integration/test_storage_kafka/test.py index 7d17c1cf362..24c5a3ac3bf 100644 --- a/dbms/tests/integration/test_storage_kafka/test.py +++ b/dbms/tests/integration/test_storage_kafka/test.py @@ -605,7 +605,7 @@ def test_kafka_produce_consume(kafka_cluster): assert int(result) == messages_num * threads_num, 'ClickHouse lost some messages: {}'.format(result) -@pytest.mark.timeout(180) +@pytest.mark.timeout(300) def test_kafka_commit_on_block_write(kafka_cluster): instance.query(''' DROP TABLE IF EXISTS test.view; From 55f11b367575bb9f174649afbf0180c4172a0883 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sun, 1 Sep 2019 18:25:46 +0300 Subject: [PATCH 33/35] Speed up test for "text_log" in cases when text_log is large (such as on production servers) --- .../tests/queries/0_stateless/00974_text_log_table_not_empty.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/tests/queries/0_stateless/00974_text_log_table_not_empty.sh b/dbms/tests/queries/0_stateless/00974_text_log_table_not_empty.sh index 149f0668bd1..c3cde4c08bb 100755 --- a/dbms/tests/queries/0_stateless/00974_text_log_table_not_empty.sh +++ b/dbms/tests/queries/0_stateless/00974_text_log_table_not_empty.sh @@ -10,7 +10,7 @@ do ${CLICKHOUSE_CLIENT} --query="SYSTEM FLUSH LOGS" sleep 0.1; -if [[ $($CLICKHOUSE_CURL -sS "$CLICKHOUSE_URL" -d "SELECT count() > 0 FROM system.text_log WHERE position(system.text_log.message, 'SELECT 6103') > 0") == 1 ]]; then echo 1; exit; fi; +if [[ $($CLICKHOUSE_CURL -sS "$CLICKHOUSE_URL" -d "SELECT count() > 0 FROM system.text_log WHERE position(system.text_log.message, 'SELECT 6103') > 0 AND event_date >= yesterday()") == 1 ]]; then echo 1; exit; fi; done; From 8917572087db020a2c9c4ec180fb34c28b4714de Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sun, 1 Sep 2019 18:28:43 +0300 Subject: [PATCH 34/35] Speed up another test in cases when query_log is large (such as on production servers) --- .../00933_test_fix_extra_seek_on_compressed_cache.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/tests/queries/0_stateless/00933_test_fix_extra_seek_on_compressed_cache.sh b/dbms/tests/queries/0_stateless/00933_test_fix_extra_seek_on_compressed_cache.sh index b0fd9a70bd4..1f7571a2404 100755 --- a/dbms/tests/queries/0_stateless/00933_test_fix_extra_seek_on_compressed_cache.sh +++ b/dbms/tests/queries/0_stateless/00933_test_fix_extra_seek_on_compressed_cache.sh @@ -19,7 +19,7 @@ $CLICKHOUSE_CLIENT --use_uncompressed_cache=1 --query_id="test-query-uncompresse sleep 1 $CLICKHOUSE_CLIENT --query="SYSTEM FLUSH LOGS" -$CLICKHOUSE_CLIENT --query="SELECT ProfileEvents.Values[indexOf(ProfileEvents.Names, 'Seek')], ProfileEvents.Values[indexOf(ProfileEvents.Names, 'ReadCompressedBytes')], ProfileEvents.Values[indexOf(ProfileEvents.Names, 'UncompressedCacheHits')] AS hit FROM system.query_log WHERE (query_id = 'test-query-uncompressed-cache') AND (type = 2) ORDER BY event_time DESC LIMIT 1" +$CLICKHOUSE_CLIENT --query="SELECT ProfileEvents.Values[indexOf(ProfileEvents.Names, 'Seek')], ProfileEvents.Values[indexOf(ProfileEvents.Names, 'ReadCompressedBytes')], ProfileEvents.Values[indexOf(ProfileEvents.Names, 'UncompressedCacheHits')] AS hit FROM system.query_log WHERE (query_id = 'test-query-uncompressed-cache') AND (type = 2) AND event_date >= yesterday() ORDER BY event_time DESC LIMIT 1" $CLICKHOUSE_CLIENT --query="DROP TABLE IF EXISTS small_table" From 4ebf610808c2bbc1bf15ffd56a4eff914163e929 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sun, 1 Sep 2019 19:21:54 +0300 Subject: [PATCH 35/35] Disable query profiler with sanitizers --- dbms/programs/server/Server.cpp | 13 ++++++++++++- dbms/src/Core/Defines.h | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/dbms/programs/server/Server.cpp b/dbms/programs/server/Server.cpp index 5f5e464eb01..f10dc07ab56 100644 --- a/dbms/programs/server/Server.cpp +++ b/dbms/programs/server/Server.cpp @@ -520,7 +520,18 @@ int Server::main(const std::vector & /*args*/) /// Init trace collector only after trace_log system table was created /// Disable it if we collect test coverage information, because it will work extremely slow. -#if USE_UNWIND && !WITH_COVERAGE + /// + /// It also cannot work with sanitizers. + /// Sanitizers are using quick "frame walking" stack unwinding (this implies -fno-omit-frame-pointer) + /// And they do unwiding frequently (on every malloc/free, thread/mutex operations, etc). + /// They change %rbp during unwinding and it confuses libunwind if signal comes during sanitizer unwiding + /// and query profiler decide to unwind stack with libunwind at this moment. + /// + /// Symptoms: you'll get silent Segmentation Fault - without sanitizer message and without usual ClickHouse diagnostics. + /// + /// Look at compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.h + /// +#if USE_UNWIND && !WITH_COVERAGE && !defined(SANITIZER) /// QueryProfiler cannot work reliably with any other libunwind or without PHDR cache. if (hasPHDRCache()) global_context->initializeTraceCollector(); diff --git a/dbms/src/Core/Defines.h b/dbms/src/Core/Defines.h index a172cf6e243..33c65081e40 100644 --- a/dbms/src/Core/Defines.h +++ b/dbms/src/Core/Defines.h @@ -124,6 +124,8 @@ #endif #endif +/// TODO Strange enough, there is no way to detect UB sanitizer. + /// Explicitly allow undefined behaviour for certain functions. Use it as a function attribute. /// It is useful in case when compiler cannot see (and exploit) it, but UBSan can. /// Example: multiplication of signed integers with possibility of overflow when both sides are from user input.