From ba582dd74c551a5844c7c63d4be4c617dd33a944 Mon Sep 17 00:00:00 2001 From: kssenii Date: Wed, 29 Mar 2023 15:27:00 +0200 Subject: [PATCH] Move assertions to a better place --- src/Interpreters/Cache/FileCache.cpp | 46 ++----------------- src/Interpreters/Cache/FileSegment.cpp | 23 +++++++++- src/Interpreters/Cache/FileSegment.h | 4 +- src/Interpreters/Cache/IFileCachePriority.h | 5 +- src/Interpreters/Cache/LRUFileCachePriority.h | 6 ++- .../Cache/LockedFileCachePriority.h | 4 +- src/Interpreters/Cache/Metadata.cpp | 2 +- 7 files changed, 38 insertions(+), 52 deletions(-) diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index 34d9000d0ec..c007f45d350 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/src/Interpreters/Cache/FileCache.cpp @@ -1131,48 +1131,12 @@ size_t FileCache::getFileSegmentsNum() const void FileCache::assertCacheCorrectness() { + auto lock = metadata.lock(); + iterateCacheMetadata(lock, [&](KeyMetadata & key_metadata) { - auto lock = metadata.lock(); - iterateCacheMetadata(lock, [&](KeyMetadata & key_metadata) - { - for (auto & [offset, file_segment_metadata] : key_metadata) - { - file_segment_metadata.file_segment->assertCorrectness(); - if (file_segment_metadata.size()) - chassert(file_segment_metadata.queue_iterator); - } - }); - } - - { - auto lock = cache_guard.lock(); - - LockedCachePriority queue(lock, *main_priority); - [[maybe_unused]] size_t total_size = 0; - - using QueueEntry = IFileCachePriority::Entry; - using IterationResult = IFileCachePriority::IterationResult; - - queue.iterate([&](const QueueEntry & entry) -> IterationResult - { - auto locked_key = lockKeyMetadata(entry.key, entry.getKeyMetadata()); - auto * file_segment_metadata = locked_key->getKeyMetadata()->getByOffset(entry.offset); - - if (file_segment_metadata->size() != entry.size) - { - throw Exception( - ErrorCodes::LOGICAL_ERROR, "Expected {} == {} size ({})", - file_segment_metadata->size(), entry.size, file_segment_metadata->file_segment->getInfoForLog()); - } - - total_size += entry.size; - return IterationResult::CONTINUE; - }); - - assert(queue.getSize() == total_size); - assert(queue.getSize() <= queue.getSizeLimit()); - assert(queue.getElementsCount() <= queue.getElementsLimit()); - } + for (auto & [offset, file_segment_metadata] : key_metadata) + file_segment_metadata.file_segment->assertCorrectness(); + }); } FileCache::QueryContextHolder::QueryContextHolder( diff --git a/src/Interpreters/Cache/FileSegment.cpp b/src/Interpreters/Cache/FileSegment.cpp index b18a5c6d776..580f4cfa670 100644 --- a/src/Interpreters/Cache/FileSegment.cpp +++ b/src/Interpreters/Cache/FileSegment.cpp @@ -466,6 +466,7 @@ bool FileSegment::reserve(size_t size_to_reserve) /// It is made atomic because of getInfoForLog. reserved_size += size_to_reserve; } + chassert(assertCorrectness()); } return reserved; @@ -689,18 +690,36 @@ String FileSegment::stateToString(FileSegment::State state) UNREACHABLE(); } -void FileSegment::assertCorrectness() const +bool FileSegment::assertCorrectness() const { auto lock = segment_guard.lock(); assertCorrectnessUnlocked(lock); + return true; } -void FileSegment::assertCorrectnessUnlocked(const FileSegmentGuard::Lock & lock) const +bool FileSegment::assertCorrectnessUnlocked(const FileSegmentGuard::Lock & lock) const { auto current_downloader = getDownloaderUnlocked(lock); chassert(current_downloader.empty() == (download_state != FileSegment::State::DOWNLOADING)); chassert(!current_downloader.empty() == (download_state == FileSegment::State::DOWNLOADING)); chassert(download_state != FileSegment::State::DOWNLOADED || std::filesystem::file_size(file_path) > 0); + + auto locked_key = lockKeyMetadata(true); + + const auto & file_segment_metadata = locked_key->getKeyMetadata()->tryGetByOffset(offset()); + chassert(reserved_size == 0 || file_segment_metadata->queue_iterator); + + if (file_segment_metadata->queue_iterator) + { + const auto & entry = *file_segment_metadata->queue_iterator; + if (isCompleted(false)) + chassert(reserved_size == entry.getEntry().size); + else + /// We cannot check == here because reserved_size is not + /// guarded by any mutex, it is just an atomic. + chassert(reserved_size <= entry.getEntry().size); + } + return true; } void FileSegment::throwIfDetachedUnlocked(const FileSegmentGuard::Lock & lock) const diff --git a/src/Interpreters/Cache/FileSegment.h b/src/Interpreters/Cache/FileSegment.h index 139a09f5939..fcba974ef87 100644 --- a/src/Interpreters/Cache/FileSegment.h +++ b/src/Interpreters/Cache/FileSegment.h @@ -222,7 +222,7 @@ public: /// Completed states: DOWNALODED, DETACHED. bool isCompleted(bool sync = false) const; - void assertCorrectness() const; + bool assertCorrectness() const; /** * ========== Methods for _only_ file segment's `downloader` ================== @@ -282,7 +282,7 @@ private: void assertNotDetached() const; void assertNotDetachedUnlocked(const FileSegmentGuard::Lock &) const; void assertIsDownloaderUnlocked(const std::string & operation, const FileSegmentGuard::Lock &) const; - void assertCorrectnessUnlocked(const FileSegmentGuard::Lock &) const; + bool assertCorrectnessUnlocked(const FileSegmentGuard::Lock &) const; LockedKeyMetadataPtr lockKeyMetadata(bool assert_exists = true) const; KeyMetadataPtr getKeyMetadata() const; diff --git a/src/Interpreters/Cache/IFileCachePriority.h b/src/Interpreters/Cache/IFileCachePriority.h index e27656d7958..d0c369f8853 100644 --- a/src/Interpreters/Cache/IFileCachePriority.h +++ b/src/Interpreters/Cache/IFileCachePriority.h @@ -61,9 +61,10 @@ public: public: virtual ~IIterator() = default; + virtual const Entry & getEntry() const = 0; + protected: - virtual Entry & operator *() = 0; - virtual const Entry & operator *() const = 0; + virtual Entry & getEntry() = 0; virtual size_t use() = 0; diff --git a/src/Interpreters/Cache/LRUFileCachePriority.h b/src/Interpreters/Cache/LRUFileCachePriority.h index 990b7660860..469238d10ab 100644 --- a/src/Interpreters/Cache/LRUFileCachePriority.h +++ b/src/Interpreters/Cache/LRUFileCachePriority.h @@ -48,8 +48,10 @@ public: LRUFileCachePriority * cache_priority_, LRUFileCachePriority::LRUQueueIterator queue_iter_); - Entry & operator *() override { return *queue_iter; } - const Entry & operator *() const override { return *queue_iter; } + const Entry & getEntry() const override { return *queue_iter; } + +protected: + Entry & getEntry() override { return *queue_iter; } size_t use() override; diff --git a/src/Interpreters/Cache/LockedFileCachePriority.h b/src/Interpreters/Cache/LockedFileCachePriority.h index 0185e8dd9c4..3cc6e2165cb 100644 --- a/src/Interpreters/Cache/LockedFileCachePriority.h +++ b/src/Interpreters/Cache/LockedFileCachePriority.h @@ -41,8 +41,8 @@ public: LockedCachePriorityIterator(const CacheGuard::Lock & lock_, IFileCachePriority::Iterator & iterator_) : lock(lock_), iterator(iterator_) {} - IFileCachePriority::Entry & operator *() { return **iterator; } - const IFileCachePriority::Entry & operator *() const { return **iterator; } + IFileCachePriority::Entry & getEntry() { return iterator->getEntry(); } + const IFileCachePriority::Entry & getEntry() const { return iterator->getEntry(); } size_t use() { return iterator->use(); } diff --git a/src/Interpreters/Cache/Metadata.cpp b/src/Interpreters/Cache/Metadata.cpp index ff7fcd96dc0..4f6af172253 100644 --- a/src/Interpreters/Cache/Metadata.cpp +++ b/src/Interpreters/Cache/Metadata.cpp @@ -247,7 +247,7 @@ void LockedKeyMetadata::shrinkFileSegmentToDownloadedSize( file_segment->getInfoForLogUnlocked(segment_lock)); } - auto & entry = *LockedCachePriorityIterator(cache_lock, file_segment_metadata->queue_iterator); + auto & entry = LockedCachePriorityIterator(cache_lock, file_segment_metadata->queue_iterator).getEntry(); assert(file_segment->downloaded_size <= file_segment->reserved_size); assert(entry.size == file_segment->reserved_size); assert(entry.size >= file_segment->downloaded_size);