diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index 95af90a947a..1bb0d826671 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/src/Interpreters/Cache/FileCache.cpp @@ -589,6 +589,16 @@ void FileCache::iterateCacheAndCollectKeyLocks( }); } +void FileCache::removeFileSegment(LockedKey & locked_key, FileSegmentPtr file_segment, const CacheGuard::Lock & cache_lock) +{ + /// FIXME: + /// We must hold pointer to file segment while removing it (because we remove file segment under file segment lock). + /// But this should not be obligatory. + + chassert(file_segment->key() == locked_key.getKey()); + locked_key.removeFileSegment(file_segment->offset(), file_segment->lock(), cache_lock); +} + bool FileCache::tryReserveImpl( IFileCachePriority & priority_queue, const Key & key, @@ -682,7 +692,7 @@ bool FileCache::tryReserveImpl( { remove_current_it = true; file_segment_metadata->queue_iterator = {}; - current_locked_key.remove(file_segment, cache_lock); + removeFileSegment(current_locked_key, file_segment, cache_lock); break; } } @@ -708,7 +718,7 @@ bool FileCache::tryReserveImpl( for (const auto & offset_to_delete : offsets_to_delete) { auto * file_segment_metadata = current_locked_key->getKeyMetadata().getByOffset(offset_to_delete); - current_locked_key->remove(file_segment_metadata->file_segment, cache_lock); + removeFileSegment(*current_locked_key, file_segment_metadata->file_segment, cache_lock); if (query_context) query_context->remove(key, offset); } @@ -780,7 +790,7 @@ void FileCache::removeKeyIfExists(const Key & key) if (!file_segment_metadata->releasable()) continue; - locked_key->remove(file_segment_metadata->file_segment, lock); + removeFileSegment(*locked_key, file_segment_metadata->file_segment, lock); } } } @@ -809,7 +819,7 @@ void FileCache::removeAllReleasable() if (file_segment_metadata->releasable()) { file_segment_metadata->queue_iterator = {}; - locked_key->remove(file_segment_metadata->file_segment, lock); + removeFileSegment(*locked_key, file_segment_metadata->file_segment, lock); return IterationResult::REMOVE_AND_CONTINUE; } return IterationResult::CONTINUE; @@ -1026,10 +1036,10 @@ LockedKeyPtr FileCache::createLockedKey(const Key & key, KeyNotFoundPolicy key_n return nullptr; it = metadata.emplace(key, std::make_shared()).first; - return std::make_unique(key, *it->second, it->second->lock(), cleanup_keys_metadata_queue, getPathInLocalCache(key)); + return std::make_unique(key, *it->second, it->second->lock(), getPathInLocalCache(key), cleanup_keys_metadata_queue); } - return std::make_unique(key, *key_metadata, std::move(key_lock), cleanup_keys_metadata_queue, getPathInLocalCache(key)); + return std::make_unique(key, *key_metadata, std::move(key_lock), getPathInLocalCache(key), cleanup_keys_metadata_queue); } LockedKeyPtr FileCache::createLockedKey(const Key & key, KeyMetadata & key_metadata) const @@ -1037,7 +1047,7 @@ LockedKeyPtr FileCache::createLockedKey(const Key & key, KeyMetadata & key_metad auto key_lock = key_metadata.lock(); if (key_metadata.removed) throw Exception(ErrorCodes::LOGICAL_ERROR, "Cannot lock key: it was removed from cache"); - return std::make_unique(key, key_metadata, std::move(key_lock), cleanup_keys_metadata_queue, getPathInLocalCache(key)); + return std::make_unique(key, key_metadata, std::move(key_lock), getPathInLocalCache(key), cleanup_keys_metadata_queue); } void FileCache::iterateCacheMetadata(const CacheMetadataGuard::Lock & lock, std::function && func) diff --git a/src/Interpreters/Cache/FileCache.h b/src/Interpreters/Cache/FileCache.h index 1ba56af4e9d..e7632b791c9 100644 --- a/src/Interpreters/Cache/FileCache.h +++ b/src/Interpreters/Cache/FileCache.h @@ -245,6 +245,8 @@ private: void assertCacheCorrectness(); void assertCacheCorrectness(const CacheGuard::Lock & cache_lock, const CacheMetadataGuard::Lock & metadata_lock); + + static void removeFileSegment(LockedKey & locked_key, FileSegmentPtr file_segment, const CacheGuard::Lock &); }; } diff --git a/src/Interpreters/Cache/FileSegment.cpp b/src/Interpreters/Cache/FileSegment.cpp index 061c075d142..0861f396e7d 100644 --- a/src/Interpreters/Cache/FileSegment.cpp +++ b/src/Interpreters/Cache/FileSegment.cpp @@ -56,7 +56,7 @@ FileSegment::FileSegment( break; } /// DOWNLOADED is used either on initial cache metadata load into memory on server startup - /// or on reduceSizeToDownloaded() -- when file segment object is updated. + /// or on shrinkFileSegmentToDownloadedSize() -- when file segment object is updated. case (State::DOWNLOADED): { reserved_size = downloaded_size = size_; @@ -254,7 +254,7 @@ FileSegment::RemoteFileReaderPtr FileSegment::extractRemoteFileReader() assert(download_state != State::DETACHED); - bool is_last_holder = locked_key->isLastHolder(offset()); + bool is_last_holder = locked_key->isLastOwnerOfFileSegment(offset()); if (!downloader_id.empty() || !is_last_holder) return nullptr; @@ -445,7 +445,7 @@ bool FileSegment::reserve(size_t size_to_reserve) { /// No lock is required because reserved size is always /// mananaged (read/modified) by the downloader only - /// or in isLastHolder() case. + /// or in isLastOwnerOfFileSegment() case. /// It is made atomic because of getInfoForLog. reserved_size += size_to_reserve; } @@ -534,7 +534,7 @@ void FileSegment::completeUnlocked(LockedKey & locked_key, const CacheGuard::Loc return; const bool is_downloader = isDownloaderUnlocked(segment_lock); - const bool is_last_holder = locked_key.isLastHolder(offset()); + const bool is_last_holder = locked_key.isLastOwnerOfFileSegment(offset()); const size_t current_downloaded_size = getDownloadedSize(true); SCOPE_EXIT({ @@ -567,7 +567,7 @@ void FileSegment::completeUnlocked(LockedKey & locked_key, const CacheGuard::Loc LOG_TEST(log, "Removing temporary file segment: {}", getInfoForLogUnlocked(segment_lock)); detach(segment_lock, locked_key); setDownloadState(State::DETACHED, segment_lock); - locked_key.remove(offset(), segment_lock, cache_lock); + locked_key.removeFileSegment(offset(), segment_lock, cache_lock); return; } @@ -598,7 +598,7 @@ void FileSegment::completeUnlocked(LockedKey & locked_key, const CacheGuard::Loc LOG_TEST(log, "Remove cell {} (nothing downloaded)", range().toString()); setDownloadState(State::DETACHED, segment_lock); - locked_key.remove(offset(), segment_lock, cache_lock); + locked_key.removeFileSegment(offset(), segment_lock, cache_lock); } else { @@ -616,7 +616,7 @@ void FileSegment::completeUnlocked(LockedKey & locked_key, const CacheGuard::Loc /// but current file segment should remain PARRTIALLY_DOWNLOADED_NO_CONTINUATION and with detached state, /// because otherwise an invariant that getOrSet() returns a contiguous range of file segments will be broken /// (this will be crucial for other file segment holder, not for current one). - locked_key.reduceSizeToDownloaded(offset(), segment_lock, cache_lock); + locked_key.shrinkFileSegmentToDownloadedSize(offset(), segment_lock, cache_lock); } detachAssumeStateFinalized(segment_lock); diff --git a/src/Interpreters/Cache/LockedKey.cpp b/src/Interpreters/Cache/LockedKey.cpp index d6cffb5844b..87856d542c1 100644 --- a/src/Interpreters/Cache/LockedKey.cpp +++ b/src/Interpreters/Cache/LockedKey.cpp @@ -18,8 +18,8 @@ LockedKey::LockedKey( const FileCacheKey & key_, KeyMetadata & key_metadata_, KeyGuard::Lock && lock_, - KeysQueuePtr cleanup_keys_metadata_queue_, - const std::string & key_path_) + const std::string & key_path_, + KeysQueuePtr cleanup_keys_metadata_queue_) : key(key_) , key_path(key_path_) , lock(std::move(lock_)) @@ -34,20 +34,13 @@ LockedKey::~LockedKey() removeKeyIfEmpty(); } -void LockedKey::remove(FileSegmentPtr file_segment, const CacheGuard::Lock & cache_lock) -{ - /// We must hold pointer to file segment while removing it. - chassert(file_segment->key() == key); - remove(file_segment->offset(), file_segment->lock(), cache_lock); -} - -bool LockedKey::isLastHolder(size_t offset) const +bool LockedKey::isLastOwnerOfFileSegment(size_t offset) const { const auto * file_segment_metadata = key_metadata.getByOffset(offset); return file_segment_metadata->file_segment.use_count() == 2; } -void LockedKey::remove( +void LockedKey::removeFileSegment( size_t offset, const FileSegmentGuard::Lock & segment_lock, const CacheGuard::Lock & cache_lock) @@ -82,7 +75,7 @@ void LockedKey::remove( } } -void LockedKey::reduceSizeToDownloaded( +void LockedKey::shrinkFileSegmentToDownloadedSize( size_t offset, const FileSegmentGuard::Lock & segment_lock, const CacheGuard::Lock & cache_lock) diff --git a/src/Interpreters/Cache/LockedKey.h b/src/Interpreters/Cache/LockedKey.h index efee692f987..bc6440f3016 100644 --- a/src/Interpreters/Cache/LockedKey.h +++ b/src/Interpreters/Cache/LockedKey.h @@ -27,23 +27,21 @@ struct LockedKey : private boost::noncopyable const FileCacheKey & key_, KeyMetadata & key_metadata_, KeyGuard::Lock && key_lock_, - KeysQueuePtr cleanup_keys_metadata_queue_, - const std::string & key_path_); + const std::string & key_path_, + KeysQueuePtr cleanup_keys_metadata_queue_); ~LockedKey(); - void reduceSizeToDownloaded(size_t offset, const FileSegmentGuard::Lock &, const CacheGuard::Lock &); - - void remove(FileSegmentPtr file_segment, const CacheGuard::Lock &); - - void remove(size_t offset, const FileSegmentGuard::Lock &, const CacheGuard::Lock &); - - bool isLastHolder(size_t offset) const; + const FileCacheKey & getKey() const { return key; } const KeyMetadata & getKeyMetadata() const { return key_metadata; } KeyMetadata & getKeyMetadata() { return key_metadata; } - const FileCacheKey & getKey() const { return key; } + void removeFileSegment(size_t offset, const FileSegmentGuard::Lock &, const CacheGuard::Lock &); + + void shrinkFileSegmentToDownloadedSize(size_t offset, const FileSegmentGuard::Lock &, const CacheGuard::Lock &); + + bool isLastOwnerOfFileSegment(size_t offset) const; private: void removeKeyIfEmpty() const;