From bdf640326bf36c4ec64f766a95e0ee1da27b6a46 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Tue, 2 Nov 2021 22:28:27 +0300 Subject: [PATCH 1/3] Clickhouse dictionary source no longer keep a session after creation. This fixes tsan alert on shutdown. --- .../ClickHouseDictionarySource.cpp | 22 +++++++++---------- src/Dictionaries/ClickHouseDictionarySource.h | 4 +--- src/Interpreters/Context.h | 1 - 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/Dictionaries/ClickHouseDictionarySource.cpp b/src/Dictionaries/ClickHouseDictionarySource.cpp index a5a04d277da..640371947a9 100644 --- a/src/Dictionaries/ClickHouseDictionarySource.cpp +++ b/src/Dictionaries/ClickHouseDictionarySource.cpp @@ -65,14 +65,12 @@ ClickHouseDictionarySource::ClickHouseDictionarySource( const DictionaryStructure & dict_struct_, const Configuration & configuration_, const Block & sample_block_, - ContextMutablePtr context_, - std::shared_ptr local_session_) + ContextMutablePtr context_) : update_time{std::chrono::system_clock::from_time_t(0)} , dict_struct{dict_struct_} , configuration{configuration_} , query_builder{dict_struct, configuration.db, "", configuration.table, configuration.query, configuration.where, IdentifierQuotingStyle::Backticks} , sample_block{sample_block_} - , local_session(local_session_) , context(context_) , pool{createPool(configuration)} , load_all_query{query_builder.composeLoadAllQuery()} @@ -86,7 +84,6 @@ ClickHouseDictionarySource::ClickHouseDictionarySource(const ClickHouseDictionar , invalidate_query_response{other.invalidate_query_response} , query_builder{dict_struct, configuration.db, "", configuration.table, configuration.query, configuration.where, IdentifierQuotingStyle::Backticks} , sample_block{other.sample_block} - , local_session(other.local_session) , context(Context::createCopy(other.context)) , pool{createPool(configuration)} , load_all_query{other.load_all_query} @@ -252,17 +249,18 @@ void registerDictionarySourceClickHouse(DictionarySourceFactory & factory) }; ContextMutablePtr context; - std::shared_ptr local_session; if (configuration.is_local) { - /// Start local session in case when the dictionary is loaded in-process (without TCP communication). - local_session = std::make_shared(global_context, ClientInfo::Interface::LOCAL); - local_session->authenticate(configuration.user, configuration.password, {}); - context = local_session->makeQueryContext(); - context->applySettingsChanges(readSettingsFromDictionaryConfig(config, config_prefix)); + /// We should set user info even for the case when the dictionary is loaded in-process (without TCP communication). + Session session(global_context, ClientInfo::Interface::LOCAL); + session.authenticate(configuration.user, configuration.password, {}); + context = session.makeQueryContext(); } else - context = copyContextAndApplySettingsFromDictionaryConfig(global_context, config, config_prefix); + { + context = Context::createCopy(global_context); + } + context->applySettingsChanges(readSettingsFromDictionaryConfig(config, config_prefix)); String dictionary_name = config.getString(".dictionary.name", ""); String dictionary_database = config.getString(".dictionary.database", ""); @@ -270,7 +268,7 @@ void registerDictionarySourceClickHouse(DictionarySourceFactory & factory) if (dictionary_name == configuration.table && dictionary_database == configuration.db) throw Exception(ErrorCodes::BAD_ARGUMENTS, "ClickHouseDictionarySource table cannot be dictionary table"); - return std::make_unique(dict_struct, configuration, sample_block, context, local_session); + return std::make_unique(dict_struct, configuration, sample_block, context); }; factory.registerSource("clickhouse", create_table_source); diff --git a/src/Dictionaries/ClickHouseDictionarySource.h b/src/Dictionaries/ClickHouseDictionarySource.h index 58243e43b15..be09fa415fd 100644 --- a/src/Dictionaries/ClickHouseDictionarySource.h +++ b/src/Dictionaries/ClickHouseDictionarySource.h @@ -39,8 +39,7 @@ public: const DictionaryStructure & dict_struct_, const Configuration & configuration_, const Block & sample_block_, - ContextMutablePtr context_, - std::shared_ptr local_session_); + ContextMutablePtr context_); /// copy-constructor is provided in order to support cloneability ClickHouseDictionarySource(const ClickHouseDictionarySource & other); @@ -82,7 +81,6 @@ private: mutable std::string invalidate_query_response; ExternalQueryBuilder query_builder; Block sample_block; - std::shared_ptr local_session; ContextMutablePtr context; ConnectionPoolWithFailoverPtr pool; const std::string load_all_query; diff --git a/src/Interpreters/Context.h b/src/Interpreters/Context.h index bc87f570366..7d31a8375d8 100644 --- a/src/Interpreters/Context.h +++ b/src/Interpreters/Context.h @@ -381,7 +381,6 @@ public: /// Sets the current user assuming that he/she is already authenticated. /// WARNING: This function doesn't check password! - /// Normally you shouldn't call this function. Use the Session class to do authentication instead. void setUser(const UUID & user_id_); UserPtr getUser() const; From 49d8360fc5406d59e6819a57b5392c7089a92f85 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 3 Nov 2021 16:14:40 +0300 Subject: [PATCH 2/3] Turn off sending events for the LOCAL interface to the system log. --- src/Interpreters/Session.cpp | 6 ++++++ src/Interpreters/Session.h | 1 + 2 files changed, 7 insertions(+) diff --git a/src/Interpreters/Session.cpp b/src/Interpreters/Session.cpp index 00be194f1eb..39d2abc9b43 100644 --- a/src/Interpreters/Session.cpp +++ b/src/Interpreters/Session.cpp @@ -246,6 +246,7 @@ void Session::shutdownNamedSessions() Session::Session(const ContextPtr & global_context_, ClientInfo::Interface interface_) : auth_id(UUIDHelpers::generateV4()), global_context(global_context_), + interface(interface_), log(&Poco::Logger::get(String{magic_enum::enum_name(interface_)} + "-Session")) { prepared_client_info.emplace(); @@ -418,6 +419,11 @@ ContextMutablePtr Session::makeQueryContext(ClientInfo && query_client_info) con std::shared_ptr Session::getSessionLog() const { + /// For the LOCAL interface we don't send events to the session log + /// because the LOCAL interface is internal, it does nothing with networking. + if (interface == ClientInfo::Interface::LOCAL) + return nullptr; + // take it from global context, since it outlives the Session and always available. // please note that server may have session_log disabled, hence this may return nullptr. return global_context->getSessionLog(); diff --git a/src/Interpreters/Session.h b/src/Interpreters/Session.h index 273ed88b9b5..71964130412 100644 --- a/src/Interpreters/Session.h +++ b/src/Interpreters/Session.h @@ -79,6 +79,7 @@ private: mutable bool notified_session_log_about_login = false; const UUID auth_id; const ContextPtr global_context; + const ClientInfo::Interface interface; /// ClientInfo that will be copied to a session context when it's created. std::optional prepared_client_info; From 15280df08d66345c5d064d47f1469e0ed0df25ca Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 3 Nov 2021 17:07:32 +0300 Subject: [PATCH 3/3] Fix integration test "test_dictionaries_dependency_xml/". --- .../configs/dictionaries/dep_z.xml | 2 +- .../test_dictionaries_dependency_xml/test.py | 19 +++++++++---------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/tests/integration/test_dictionaries_dependency_xml/configs/dictionaries/dep_z.xml b/tests/integration/test_dictionaries_dependency_xml/configs/dictionaries/dep_z.xml index 72cb43caf09..8e254d769ea 100644 --- a/tests/integration/test_dictionaries_dependency_xml/configs/dictionaries/dep_z.xml +++ b/tests/integration/test_dictionaries_dependency_xml/configs/dictionaries/dep_z.xml @@ -9,7 +9,7 @@ dict dep_y
- SELECT intDiv(count(), 5) from dict.dep_y + SELECT intDiv(count(), 4) from dict.dep_y diff --git a/tests/integration/test_dictionaries_dependency_xml/test.py b/tests/integration/test_dictionaries_dependency_xml/test.py index 849fdf57980..6b8a5dff133 100644 --- a/tests/integration/test_dictionaries_dependency_xml/test.py +++ b/tests/integration/test_dictionaries_dependency_xml/test.py @@ -59,20 +59,19 @@ def test_get_data(started_cluster): query("INSERT INTO test.elements VALUES (3, 'fire', 30, 8)") # Wait for dictionaries to be reloaded. - assert_eq_with_retry(instance, "SELECT dictHas('dep_y', toUInt64(3))", "1", sleep_time=2, retry_count=10) - assert query("SELECT dictGetString('dep_x', 'a', toUInt64(3))") == "XX\n" - assert query("SELECT dictGetString('dep_y', 'a', toUInt64(3))") == "fire\n" - assert query("SELECT dictGetString('dep_z', 'a', toUInt64(3))") == "ZZ\n" - - # dep_x and dep_z are updated only when there `intDiv(count(), 5)` is changed. - query("INSERT INTO test.elements VALUES (4, 'ether', 404, 0.001)") - assert_eq_with_retry(instance, "SELECT dictHas('dep_x', toUInt64(4))", "1", sleep_time=2, retry_count=10) + assert_eq_with_retry(instance, "SELECT dictHas('dep_x', toUInt64(3))", "1", sleep_time=2, retry_count=10) assert query("SELECT dictGetString('dep_x', 'a', toUInt64(3))") == "fire\n" assert query("SELECT dictGetString('dep_y', 'a', toUInt64(3))") == "fire\n" assert query("SELECT dictGetString('dep_z', 'a', toUInt64(3))") == "fire\n" - assert query("SELECT dictGetString('dep_x', 'a', toUInt64(4))") == "ether\n" + + # dep_z (and hence dep_x) are updated only when there `intDiv(count(), 4)` is changed, now `count()==4`, + # so dep_x and dep_z are not going to be updated after the following INSERT. + query("INSERT INTO test.elements VALUES (4, 'ether', 404, 0.001)") + assert_eq_with_retry(instance, "SELECT dictHas('dep_y', toUInt64(4))", "1", sleep_time=2, retry_count=10) + assert query("SELECT dictGetString('dep_x', 'a', toUInt64(4))") == "XX\n" assert query("SELECT dictGetString('dep_y', 'a', toUInt64(4))") == "ether\n" - assert query("SELECT dictGetString('dep_z', 'a', toUInt64(4))") == "ether\n" + assert query("SELECT dictGetString('dep_z', 'a', toUInt64(4))") == "ZZ\n" + def dependent_tables_assert(): res = instance.query("select database || '.' || name from system.tables")