Add a comment, an actual fix for the bug

src/Interpreters/Cache/EvictionCandidates.cpp

@@ -157,54 +157,63 @@ void EvictionCandidates::evict()
     }
 }
 
+bool EvictionCandidates::needFinalize() const
+{
+    /// finalize() does the following:
+    /// 1. Release space holders in case they were added.
+    ///    (Space holder are created if some space needs to be hold
+    ///    while were are doing eviction from filesystem without which is done without a lock)
+    ///    Note: this step is not needed in case dynamic cache resize,
+    ///          because space holders are not used.
+    /// 2. Delete queue entries from IFileCachePriority queue.
+    ///    These queue entries were invalidated during lock-free eviction phase,
+    ///    so on finalize() we just remove them (not to let the queue grow too much).
+    ///    Note: this step can in fact be removed as we do this cleanup
+    ///    (removal of invalidated queue entries)
+    ///    when we iterate the queue and see such entries along the way.
+    ///    Note: this step is omitted in case dynamic cache resize,
+    ///          because we remove queue entries in advance, before actual eviction.
+    /// 3. Execute on_finalize functions.
+    ///    These functions are set only for SLRU eviction policy,
+    ///    where we need to do additional work after eviction.
+    ///    Note: this step is not needed in case dynamic cache resize even for SLRU.
+
+    return !on_finalize.empty() || !queue_entries_to_invalidate.empty() || !hold_space.empty();
+}
+
 void EvictionCandidates::finalize(
     FileCacheQueryLimit::QueryContext * query_context,
-    const CachePriorityGuard::Lock * lock)
+    const CachePriorityGuard::Lock & lock)
 {
+    chassert(lock.owns_lock());
+
     /// Release the hold space. It was hold only for the duration of evict() phase,
     /// now we can release. It might also be needed for on_finalize func,
     /// so release the space it firtst.
-    releaseSpaceHolders();
+    hold_space.clear();
 
-    if (requiresLockToFinalize())
+    while (!queue_entries_to_invalidate.empty())
     {
-        if (!lock || !lock->owns_lock())
-            throw Exception(
-                ErrorCodes::LOGICAL_ERROR, "Cannot finalize eviction without a cache lock");
+        auto iterator = queue_entries_to_invalidate.back();
+        iterator->invalidate();
+        queue_entries_to_invalidate.pop_back();
 
-        while (!queue_entries_to_invalidate.empty())
+        /// Remove entry from per query priority queue.
+        if (query_context)
         {
-            auto iterator = queue_entries_to_invalidate.back();
-            iterator->invalidate();
-            queue_entries_to_invalidate.pop_back();
-
-            /// Remove entry from per query priority queue.
-            if (query_context)
-            {
-                const auto & entry = iterator->getEntry();
-                query_context->remove(entry->key, entry->offset, *lock);
-            }
-            /// Remove entry from main priority queue.
-            iterator->remove(*lock);
+            const auto & entry = iterator->getEntry();
+            query_context->remove(entry->key, entry->offset, lock);
         }
-
-        for (auto & func : on_finalize)
-            func(*lock);
-
-        /// Finalize functions might hold something (like HoldSpace object),
-        /// so we need to clear them now.
-        on_finalize.clear();
+        /// Remove entry from main priority queue.
+        iterator->remove(lock);
     }
-    else
-    {
-        chassert(on_finalize.empty() && queue_entries_to_invalidate.empty());
-    }
-}
 
-bool EvictionCandidates::requiresLockToFinalize() const
-{
-    /// Do we need to call finalize()?
-    return !on_finalize.empty() || !queue_entries_to_invalidate.empty();
+    for (auto & func : on_finalize)
+        func(lock);
+
+    /// Finalize functions might hold something (like HoldSpace object),
+    /// so we need to clear them now.
+    on_finalize.clear();
 }
 
 void EvictionCandidates::addSpaceHolder(
@@ -217,10 +226,4 @@ void EvictionCandidates::addSpaceHolder(
     hold_space.emplace_back(std::make_unique<IFileCachePriority::HoldSpace>(size, elements, priority, lock));
 }
 
-void EvictionCandidates::releaseSpaceHolders()
-{
-    for (const auto & holder : hold_space)
-        holder->release();
-}
-
 }

src/Interpreters/Cache/EvictionCandidates.h

@@ -25,9 +25,9 @@ public:
 
     void finalize(
         FileCacheQueryLimit::QueryContext * query_context,
-        const CachePriorityGuard::Lock * lock = nullptr);
+        const CachePriorityGuard::Lock & lock);
 
-    bool requiresLockToFinalize() const;
+    bool needFinalize() const;
 
     size_t size() const { return candidates_size; }
 
@@ -42,8 +42,6 @@ public:
         const CachePriorityGuard::Lock &);
 
 private:
-    void releaseSpaceHolders();
-
     struct KeyCandidates
     {
         KeyMetadataPtr key_metadata;

src/Interpreters/Cache/FileCache.cpp

@@ -890,14 +890,14 @@ bool FileCache::tryReserve(
         {
             cache_lock.lock();
             /// Invalidate queue entries if some succeeded to be removed.
-            eviction_candidates.finalize(query_context.get(), &cache_lock);
+            eviction_candidates.finalize(query_context.get(), cache_lock);
             throw;
         }
 
         cache_lock.lock();
 
         /// Invalidate and remove queue entries and execute finalize func.
-        eviction_candidates.finalize(query_context.get(), &cache_lock);
+        eviction_candidates.finalize(query_context.get(), cache_lock);
     }
     else if (!main_priority->canFit(size, required_elements_num, cache_lock, queue_iterator))
     {
@@ -1438,30 +1438,24 @@ void FileCache::applySettingsIfPossible(const FileCacheSettings & new_settings,
 
                     cache_lock.unlock();
 
-                    auto finalize_eviction = [&]()
-                    {
-                        if (eviction_candidates.requiresLockToFinalize())
+                    SCOPE_EXIT({
+                        try
                         {
-                            cache_lock.lock();
-                            eviction_candidates.finalize(nullptr, &cache_lock);
+                            if (eviction_candidates.needFinalize())
+                            {
+                                cache_lock.lock();
+                                eviction_candidates.finalize(nullptr, cache_lock);
+                            }
                         }
-                        else
-                            eviction_candidates.finalize(nullptr);
-                    };
+                        catch (...)
+                        {
+                            tryLogCurrentException(__PRETTY_FUNCTION__);
+                            chassert(false);
+                        }
+                    });
 
-                    try
-                    {
-                        /// Do actual eviction from filesystem.
-                        eviction_candidates.evict();
-                    }
-                    catch (...)
-                    {
-                        tryLogCurrentException(__PRETTY_FUNCTION__);
-                        finalize_eviction();
-                        throw;
-                    }
-
-                    finalize_eviction();
+                    /// Do actual eviction from filesystem.
+                    eviction_candidates.evict();
                 }
 
                 modified_size_limit = true;

src/Interpreters/Cache/SLRUFileCachePriority.cpp

@@ -283,8 +283,8 @@ bool SLRUFileCachePriority::collectCandidatesForEviction(
     if (max_candidates_to_evict && res.size() >= max_candidates_to_evict)
         return probationary_limit_satisfied;
 
-    const auto desired_protected_size = getRatio(max_size, size_ratio);
-    const auto desired_protected_elements_num = getRatio(max_elements, size_ratio);
+    const auto desired_protected_size = getRatio(desired_size, size_ratio);
+    const auto desired_protected_elements_num = getRatio(desired_elements_count, size_ratio);
 
     FileCacheReserveStat protected_stat;
     const bool protected_limit_satisfied = protected_queue.collectCandidatesForEviction(
@@ -387,7 +387,7 @@ void SLRUFileCachePriority::increasePriority(SLRUIterator & iterator, const Cach
         }
 
         eviction_candidates.evict();
-        eviction_candidates.finalize(nullptr, &lock);
+        eviction_candidates.finalize(nullptr, lock);
     }
     catch (...)
     {