From 5b879e143f510c09c538de6d4aa9be88199e36de Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Sat, 20 Jul 2019 00:03:25 +0300 Subject: [PATCH 1/3] Fix segfault in ExternalLoader::reloadOutdated(). --- dbms/src/Interpreters/ExternalLoader.cpp | 25 ++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/dbms/src/Interpreters/ExternalLoader.cpp b/dbms/src/Interpreters/ExternalLoader.cpp index c7b3d202a28..3b6f9c21b7d 100644 --- a/dbms/src/Interpreters/ExternalLoader.cpp +++ b/dbms/src/Interpreters/ExternalLoader.cpp @@ -536,7 +536,7 @@ public: for (const auto & name_and_info : infos) { const auto & info = name_and_info.second; - if ((now >= info.next_update_time) && !info.loading() && info.was_loading()) + if ((now >= info.next_update_time) && !info.loading() && info.loaded()) is_modified_map.emplace(info.object, true); } } @@ -558,16 +558,21 @@ public: std::lock_guard lock{mutex}; TimePoint now = std::chrono::system_clock::now(); for (auto & [name, info] : infos) - if ((now >= info.next_update_time) && !info.loading() && info.was_loading()) + if ((now >= info.next_update_time) && !info.loading()) { - auto it = is_modified_map.find(info.object); - if (it == is_modified_map.end()) - continue; /// Object has been just added, it can be simply omitted from this update of outdated. - bool is_modified_flag = it->second; - if (info.loaded() && !is_modified_flag) - info.next_update_time = calculate_next_update_time(info.object, info.error_count); - else - startLoading(name, info); + if (info.loaded()) + { + auto it = is_modified_map.find(info.object); + if (it == is_modified_map.end()) + continue; /// Object has just been added, we can simply omit it from this update of outdated. + bool is_modified_flag = it->second; + if (!is_modified_flag) + { + info.next_update_time = calculate_next_update_time(info.object, info.error_count); + continue; + } + } + startLoading(name, info); } } } From f18e9592a1d3a422a56166dfc33cb9a714d3b5c9 Mon Sep 17 00:00:00 2001 From: alexey-milovidov Date: Fri, 19 Jul 2019 22:37:48 +0300 Subject: [PATCH 2/3] Update ExternalLoader.cpp --- dbms/src/Interpreters/ExternalLoader.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dbms/src/Interpreters/ExternalLoader.cpp b/dbms/src/Interpreters/ExternalLoader.cpp index 3b6f9c21b7d..a5863adb92d 100644 --- a/dbms/src/Interpreters/ExternalLoader.cpp +++ b/dbms/src/Interpreters/ExternalLoader.cpp @@ -511,12 +511,14 @@ public: { std::lock_guard lock{mutex}; for (auto & [name, info] : infos) + { if ((info.was_loading() || load_never_loading) && filter_by_name(name)) { cancelLoading(info); info.forced_to_reload = true; startLoading(name, info); } + } } /// Starts reloading of all the objects. @@ -558,6 +560,7 @@ public: std::lock_guard lock{mutex}; TimePoint now = std::chrono::system_clock::now(); for (auto & [name, info] : infos) + { if ((now >= info.next_update_time) && !info.loading()) { if (info.loaded()) @@ -574,6 +577,7 @@ public: } startLoading(name, info); } + } } } From e64f8e606d62dadbe5c9a9fd20a6607709317497 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Sat, 20 Jul 2019 00:01:20 +0300 Subject: [PATCH 3/3] Add comments. --- dbms/src/Interpreters/ExternalLoader.cpp | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/dbms/src/Interpreters/ExternalLoader.cpp b/dbms/src/Interpreters/ExternalLoader.cpp index a5863adb92d..31a69086f5c 100644 --- a/dbms/src/Interpreters/ExternalLoader.cpp +++ b/dbms/src/Interpreters/ExternalLoader.cpp @@ -530,8 +530,8 @@ public: /// The function doesn't touch the objects which were never tried to load. void reloadOutdated() { + /// Iterate through all the objects and find loaded ones which should be checked if they were modified. std::unordered_map is_modified_map; - { std::lock_guard lock{mutex}; TimePoint now = std::chrono::system_clock::now(); @@ -543,7 +543,9 @@ public: } } - /// The `mutex` should be unlocked while we're calling the function is_object_modified(). + /// Find out which of the loaded objects were modified. + /// We couldn't perform these checks while we were building `is_modified_map` because + /// the `mutex` should be unlocked while we're calling the function is_object_modified(). for (auto & [object, is_modified_flag] : is_modified_map) { try @@ -556,6 +558,7 @@ public: } } + /// Iterate through all the objects again and either start loading or just set `next_update_time`. { std::lock_guard lock{mutex}; TimePoint now = std::chrono::system_clock::now(); @@ -567,15 +570,24 @@ public: { auto it = is_modified_map.find(info.object); if (it == is_modified_map.end()) - continue; /// Object has just been added, we can simply omit it from this update of outdated. + continue; /// Object has been just loaded (it wasn't loaded while we were building the map `is_modified_map`), so we don't have to reload it right now. + bool is_modified_flag = it->second; if (!is_modified_flag) { + /// Object wasn't modified so we only have to set `next_update_time`. info.next_update_time = calculate_next_update_time(info.object, info.error_count); continue; } + + /// Object was modified and should be reloaded. + startLoading(name, info); + } + else if (info.failed()) + { + /// Object was never loaded successfully and should be reloaded. + startLoading(name, info); } - startLoading(name, info); } } }