diff --git a/src/Storages/MergeTree/BackgroundJobsAssignee.cpp b/src/Storages/MergeTree/BackgroundJobsAssignee.cpp index 598c43f2153..e104e188cc6 100644 --- a/src/Storages/MergeTree/BackgroundJobsAssignee.cpp +++ b/src/Storages/MergeTree/BackgroundJobsAssignee.cpp @@ -78,6 +78,7 @@ String BackgroundJobsAssignee::toString(Type type) case Type::Moving: return "Moving"; } + __builtin_unreachable(); } void BackgroundJobsAssignee::start() diff --git a/src/Storages/MergeTree/ExecutableTask.h b/src/Storages/MergeTree/IExecutableTask.h similarity index 83% rename from src/Storages/MergeTree/ExecutableTask.h rename to src/Storages/MergeTree/IExecutableTask.h index 2b89a98cac1..2e546e83035 100644 --- a/src/Storages/MergeTree/ExecutableTask.h +++ b/src/Storages/MergeTree/IExecutableTask.h @@ -9,19 +9,19 @@ namespace DB { -class ExecutableTask +class IExecutableTask { public: virtual bool execute() = 0; virtual void onCompleted() = 0; virtual StorageID getStorageID() = 0; - virtual ~ExecutableTask() = default; + virtual ~IExecutableTask() = default; }; -using ExecutableTaskPtr = std::shared_ptr; +using ExecutableTaskPtr = std::shared_ptr; -class LambdaAdapter : public shared_ptr_helper, public ExecutableTask +class LambdaAdapter : public shared_ptr_helper, public IExecutableTask { public: @@ -32,6 +32,7 @@ public: bool execute() override { res = inner(); + inner = {}; return false; } diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp index d2d547809e4..01431abad88 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.cpp @@ -33,7 +33,7 @@ void MergeTreeBackgroundExecutor::updateConfiguration() pending.set_capacity(new_max_tasks_count); active.set_capacity(new_max_tasks_count); - pool.setMaxFreeThreads(new_threads_count); + pool.setMaxFreeThreads(0); pool.setMaxThreads(new_threads_count); pool.setQueueSize(new_max_tasks_count); } @@ -130,13 +130,6 @@ void MergeTreeBackgroundExecutor::removeTasksCorrespondingToStorage(StorageID id { std::lock_guard lock(mutex); - - for (auto & item : tasks_to_wait) - { - assert(item.use_count() == 1); - item.reset(); - } - currently_deleting.erase(id); } } @@ -146,7 +139,7 @@ void MergeTreeBackgroundExecutor::routine(ItemPtr item) { setThreadName(name.c_str()); - auto erase_from_active = [&] + auto erase_from_active = [this, item] { active.erase(std::remove(active.begin(), active.end(), item), active.end()); }; @@ -177,6 +170,7 @@ void MergeTreeBackgroundExecutor::routine(ItemPtr item) /// because it may interact somehow with BackgroundSchedulePool, which may allocate memory /// But it is rather safe, because we have try...catch block here, and another one in ThreadPool. item->task->onCompleted(); + item->task.reset(); } catch (...) { @@ -192,8 +186,6 @@ void MergeTreeBackgroundExecutor::routine(ItemPtr item) void MergeTreeBackgroundExecutor::schedulerThreadFunction() { - DENY_ALLOCATIONS_IN_SCOPE; - while (true) { std::unique_lock lock(mutex); @@ -206,19 +198,15 @@ void MergeTreeBackgroundExecutor::schedulerThreadFunction() ItemPtr item = std::move(pending.front()); pending.pop_front(); - bool res = false; + /// Execute a piece of task + bool res = pool.trySchedule([this, item] () mutable { - ALLOW_ALLOCATIONS_IN_SCOPE; - /// Execute a piece of task - res = pool.trySchedule([this, item] - { - routine(item); - /// When storage shutdowns it will wait until all related background tasks - /// are finished, because they may want to interact with its fields - /// and this will cause segfault. - item->is_done.set(); - }); - } + routine(item); + /// When storage shutdowns it will wait until all related background tasks + /// are finished, because they may want to interact with its fields + /// and this will cause segfault. + item->is_done.set(); + }); if (!res) { diff --git a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h index bdeeecb7592..d64652a3b10 100644 --- a/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h +++ b/src/Storages/MergeTree/MergeTreeBackgroundExecutor.h @@ -14,7 +14,7 @@ #include #include #include -#include +#include namespace DB diff --git a/src/Storages/MergeTree/tests/gtest_executor.cpp b/src/Storages/MergeTree/tests/gtest_executor.cpp index 1377d9add9a..b3f8ad2a0ca 100644 --- a/src/Storages/MergeTree/tests/gtest_executor.cpp +++ b/src/Storages/MergeTree/tests/gtest_executor.cpp @@ -5,7 +5,7 @@ #include #include -#include +#include #include using namespace DB; @@ -17,7 +17,7 @@ namespace CurrentMetrics std::random_device device; -class FakeExecutableTask : public ExecutableTask +class FakeExecutableTask : public IExecutableTask { public: explicit FakeExecutableTask(String name_) : generator(device()), distribution(0, 5), name(name_)