Add more comments

src/Interpreters/Cache/EvictionCandidates.cpp

@@ -17,6 +17,11 @@ namespace ErrorCodes
     extern const int LOGICAL_ERROR;
 }
 
+EvictionCandidates::EvictionCandidates()
+    : log(getLogger("EvictionCandidates"))
+{
+}
+
 EvictionCandidates::~EvictionCandidates()
 {
     /// Here `queue_entries_to_invalidate` contains queue entries
@@ -64,8 +69,11 @@ void EvictionCandidates::add(
 
 void EvictionCandidates::removeQueueEntries(const CachePriorityGuard::Lock & lock)
 {
-    auto log = getLogger("EvictionCandidates");
+    /// Remove queue entries of eviction candidates.
+    /// This will release space we consider to be hold for them.
+
     LOG_TEST(log, "Will remove {} eviction candidates", size());
+
     for (const auto & [key, key_candidates] : candidates)
     {
         for (const auto & candidate : key_candidates.candidates)
@@ -87,6 +95,7 @@ void EvictionCandidates::evict()
 
     auto timer = DB::CurrentThread::getProfileEvents().timer(ProfileEvents::FilesystemCacheEvictMicroseconds);
 
+    /// If queue entries are already removed, then nothing to invalidate.
     if (!removed_queue_entries)
         queue_entries_to_invalidate.reserve(candidates_size);
 
@@ -184,6 +193,12 @@ void EvictionCandidates::finalize(
     on_finalize.clear();
 }
 
+bool EvictionCandidates::needFinalize() const
+{
+    /// Do we need to call finalize()?
+    return !on_finalize.empty() || !queue_entries_to_invalidate.empty();
+}
+
 void EvictionCandidates::setSpaceHolder(
     size_t size,
     size_t elements,
@@ -196,9 +211,4 @@ void EvictionCandidates::setSpaceHolder(
         hold_space = std::make_unique<IFileCachePriority::HoldSpace>(size, elements, priority, lock);
 }
 
-void EvictionCandidates::insert(EvictionCandidates && other, const CachePriorityGuard::Lock &)
-{
-    candidates.insert(make_move_iterator(other.candidates.begin()), make_move_iterator(other.candidates.end()));
-}
-
 }

src/Interpreters/Cache/EvictionCandidates.h

@@ -9,7 +9,7 @@ class EvictionCandidates : private boost::noncopyable
 public:
     using FinalizeEvictionFunc = std::function<void(const CachePriorityGuard::Lock & lk)>;
 
-    EvictionCandidates() = default;
+    EvictionCandidates();
     ~EvictionCandidates();
 
     void add(
@@ -17,8 +17,6 @@ public:
         LockedKey & locked_key,
         const CachePriorityGuard::Lock &);
 
-    void insert(EvictionCandidates && other, const CachePriorityGuard::Lock &);
-
     void evict();
 
     void removeQueueEntries(const CachePriorityGuard::Lock &);
@@ -29,6 +27,8 @@ public:
         FileCacheQueryLimit::QueryContext * query_context,
         const CachePriorityGuard::Lock &);
 
+    bool needFinalize() const;
+
     size_t size() const { return candidates_size; }
 
     auto begin() const { return candidates.begin(); }
@@ -57,6 +57,8 @@ private:
     bool removed_queue_entries = false;
 
     IFileCachePriority::HoldSpacePtr hold_space;
+
+    LoggerPtr log;
 };
 
 using EvictionCandidatesPtr = std::unique_ptr<EvictionCandidates>;

src/Interpreters/Cache/FileCache.cpp

@@ -1389,7 +1389,18 @@ void FileCache::applySettingsIfPossible(const FileCacheSettings & new_settings,
         || new_settings.max_elements != actual_settings.max_elements)
     {
         EvictionCandidates eviction_candidates;
-        bool limits_satisfied = false;
+        bool modified_size_limit = false;
+
+        /// In order to not block cache for the duration of cache resize,
+        /// we do:
+        /// a. Take a cache lock.
+        ///     1. Collect eviction candidates,
+        ///     2. Remove queue entries of eviction candidates.
+        ///        This will release space we consider to be hold for them,
+        ///        so that we can safely modify size limits.
+        ///     3. Modify size limits of cache.
+        /// b. Release a cache lock.
+        ///     1. Do actual eviction from filesystem.
         {
             cache_is_being_resized.store(true, std::memory_order_relaxed);
             SCOPE_EXIT({
@@ -1399,38 +1410,45 @@ void FileCache::applySettingsIfPossible(const FileCacheSettings & new_settings,
             auto cache_lock = lockCache();
 
             FileCacheReserveStat stat;
-            limits_satisfied = main_priority->collectCandidatesForEviction(
+            if (main_priority->collectCandidatesForEviction(
-                new_settings.max_size, new_settings.max_elements, 0/* max_candidates_to_evict */, stat, eviction_candidates, cache_lock);
+                    new_settings.max_size, new_settings.max_elements, 0/* max_candidates_to_evict */,
-
+                    stat, eviction_candidates, cache_lock))
-            eviction_candidates.removeQueueEntries(cache_lock);
-
-            if (limits_satisfied)
             {
+                /// Remove only queue entries of eviction candidates.
+                eviction_candidates.removeQueueEntries(cache_lock);
+                /// Note that (in-memory) metadata about corresponding file segments
+                /// (e.g. file segment info in CacheMetadata) will be removed
+                /// only after eviction from filesystem. This is needed to avoid
+                /// a race on removal of file from filesystsem and
+                /// addition of the same file as part of a newly cached file segment.
+
+                /// Modify cache size limits.
+                /// From this point cache eviction will follow them.
                 main_priority->modifySizeLimits(
-                    new_settings.max_size, new_settings.max_elements, new_settings.slru_size_ratio, cache_lock);
+                    new_settings.max_size, new_settings.max_elements,
+                    new_settings.slru_size_ratio, cache_lock);
+
+                modified_size_limit = true;
             }
-            else
+        }
+
+        if (modified_size_limit)
+        {
+            try
             {
-                LOG_WARNING(log, "Unable to modify size limit from {} to {}, "
+                /// Do actual eviction from filesystem.
-                            "elements limit from {} to {}",
+                eviction_candidates.evict();
-                            actual_settings.max_size, new_settings.max_size,
+            }
-                            actual_settings.max_elements, new_settings.max_elements);
+            catch (...)
+            {
+                if (eviction_candidates.needFinalize())
+                    eviction_candidates.finalize(nullptr, lockCache());
+                throw;
             }
-        }
 
-        try
+            if (eviction_candidates.needFinalize())
-        {
+                eviction_candidates.finalize(nullptr, lockCache());
-            eviction_candidates.evict();
-        }
-        catch (...)
-        {
-            auto cache_lock = lockCache();
-            eviction_candidates.finalize(nullptr, cache_lock);
-            throw;
-        }
 
-        if (limits_satisfied)
-        {
             LOG_INFO(log, "Changed max_size from {} to {}, max_elements from {} to {}",
                     actual_settings.max_size, new_settings.max_size,
                     actual_settings.max_elements, new_settings.max_elements);
@@ -1438,6 +1456,13 @@ void FileCache::applySettingsIfPossible(const FileCacheSettings & new_settings,
             actual_settings.max_size = new_settings.max_size;
             actual_settings.max_elements = new_settings.max_elements;
         }
+        else
+        {
+            LOG_WARNING(log, "Unable to modify size limit from {} to {}, "
+                        "elements limit from {} to {}",
+                        actual_settings.max_size, new_settings.max_size,
+                        actual_settings.max_elements, new_settings.max_elements);
+        }
     }
 
     if (new_settings.max_file_segment_size != actual_settings.max_file_segment_size)

src/Interpreters/Cache/FileCacheFactory.cpp

@@ -142,10 +142,8 @@ void FileCacheFactory::updateSettingsFromConfig(const Poco::Util::AbstractConfig
         caches_by_name_copy = caches_by_name;
     }
 
-    auto * log = &Poco::Logger::get("FileCacheFactory");
-
     std::unordered_set<std::string> checked_paths;
-    for (const auto & [cache_name, cache_info] : caches_by_name_copy)
+    for (const auto & [_, cache_info] : caches_by_name_copy)
     {
         if (cache_info->config_path.empty() || checked_paths.contains(cache_info->config_path))
             continue;
@@ -158,10 +156,12 @@ void FileCacheFactory::updateSettingsFromConfig(const Poco::Util::AbstractConfig
         FileCacheSettings old_settings = cache_info->getSettings();
         if (old_settings == new_settings)
         {
-            LOG_TRACE(log, "No settings changes for cache: {}", cache_name);
             continue;
         }
 
+        /// FIXME: registerDiskCache modifies `path` setting of FileCacheSettings if path is relative.
+        /// This can lead to calling applySettingsIfPossible even though nothing changed, which is avoidable.
+
         // LOG_TRACE(log, "Will apply settings changes for cache {}. "
         //           "Settings changes: {} (new settings: {}, old_settings: {})",
         //           cache_name, fmt::join(new_settings.getSettingsDiff(old_settings), ", "),

src/Interpreters/Cache/LRUFileCachePriority.cpp

@@ -280,7 +280,7 @@ bool LRUFileCachePriority::collectCandidatesForEviction(
 
     auto can_fit = [&]
     {
-        return canFit(size, 1, stat.total_stat.releasable_size, stat.total_stat.releasable_count, lock);
+        return canFit(size, elements, stat.total_stat.releasable_size, stat.total_stat.releasable_count, lock);
     };
 
     iterateForEviction(res, stat, can_fit, lock);

src/Interpreters/Cache/Metadata.cpp

@@ -941,49 +941,47 @@ KeyMetadata::iterator LockedKey::removeFileSegmentImpl(
 
     file_segment->detach(segment_lock, *this);
 
+    try
     {
-        try
+        const auto path = key_metadata->getFileSegmentPath(*file_segment);
+        if (file_segment->segment_kind == FileSegmentKind::Temporary)
         {
-            const auto path = key_metadata->getFileSegmentPath(*file_segment);
+            /// FIXME: For temporary file segment the requirement is not as strong because
-            if (file_segment->segment_kind == FileSegmentKind::Temporary)
+            /// the implementation of "temporary data in cache" creates files in advance.
-            {
+            if (fs::exists(path))
-                /// FIXME: For temporary file segment the requirement is not as strong because
-                /// the implementation of "temporary data in cache" creates files in advance.
-                if (fs::exists(path))
-                    fs::remove(path);
-            }
-            else if (file_segment->downloaded_size == 0)
-            {
-                chassert(!fs::exists(path));
-            }
-            else if (fs::exists(path))
-            {
                 fs::remove(path);
-
-                /// Clear OpenedFileCache to avoid reading from incorrect file descriptor.
-                int flags = file_segment->getFlagsForLocalRead();
-                /// Files are created with flags from file_segment->getFlagsForLocalRead()
-                /// plus optionally O_DIRECT is added, depends on query setting, so remove both.
-                OpenedFileCache::instance().remove(path, flags);
-                OpenedFileCache::instance().remove(path, flags | O_DIRECT);
-
-                LOG_TEST(key_metadata->logger(), "Removed file segment at path: {}", path);
-            }
-            else if (!can_be_broken)
-            {
-#ifdef ABORT_ON_LOGICAL_ERROR
-                throw Exception(ErrorCodes::LOGICAL_ERROR, "Expected path {} to exist", path);
-#else
-                LOG_WARNING(key_metadata->logger(), "Expected path {} to exist, while removing {}:{}",
-                            path, getKey(), file_segment->offset());
-#endif
-            }
         }
-        catch (...)
+        else if (file_segment->downloaded_size == 0)
         {
-            tryLogCurrentException(__PRETTY_FUNCTION__);
+            chassert(!fs::exists(path));
-            chassert(false);
         }
+        else if (fs::exists(path))
+        {
+            fs::remove(path);
+
+            /// Clear OpenedFileCache to avoid reading from incorrect file descriptor.
+            int flags = file_segment->getFlagsForLocalRead();
+            /// Files are created with flags from file_segment->getFlagsForLocalRead()
+            /// plus optionally O_DIRECT is added, depends on query setting, so remove both.
+            OpenedFileCache::instance().remove(path, flags);
+            OpenedFileCache::instance().remove(path, flags | O_DIRECT);
+
+            LOG_TEST(key_metadata->logger(), "Removed file segment at path: {}", path);
+        }
+        else if (!can_be_broken)
+        {
+#ifdef ABORT_ON_LOGICAL_ERROR
+            throw Exception(ErrorCodes::LOGICAL_ERROR, "Expected path {} to exist", path);
+#else
+            LOG_WARNING(key_metadata->logger(), "Expected path {} to exist, while removing {}:{}",
+                        path, getKey(), file_segment->offset());
+#endif
+        }
+    }
+    catch (...)
+    {
+        tryLogCurrentException(__PRETTY_FUNCTION__);
+        chassert(false);
     }
 
     return key_metadata->erase(it);