From a07e1172caacdba36a05d16dd7b6202deebafcf6 Mon Sep 17 00:00:00 2001 From: Leonid Krugliak Date: Sat, 9 May 2026 23:16:37 +0300 Subject: [PATCH] Prevent dead node accumulation in eviction queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skip re-insertion into the eviction queue when a block's existing entry is still valid. Previously, every unpin unconditionally re-inserted, creating a dead node for each prior entry. In production with 70K databases this led to 224M queue entries (73% dead), causing Purge to block queries for seconds. The fix tracks whether a block needs queue insertion via an atomic flag: - Set true on first load (from disk) — block has no queue entry yet - Cleared after successful insertion — entry is valid - Re-set if eviction consumes the entry without unloading the block Also fixes TryGetBlockMemory to only check seq_num (not full CanUnload) so that pinned blocks are correctly retained during Purge instead of being discarded as dead. Co-Authored-By: Claude Opus 4.6 --- .../duckdb/storage/buffer/block_handle.hpp | 17 +++++++++++++ src/storage/buffer/block_handle.cpp | 1 + src/storage/buffer/buffer_pool.cpp | 25 ++++++++++++++----- 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/include/duckdb/storage/buffer/block_handle.hpp b/src/include/duckdb/storage/buffer/block_handle.hpp index e6eefe774385..9f8e3f748495 100644 --- a/src/include/duckdb/storage/buffer/block_handle.hpp +++ b/src/include/duckdb/storage/buffer/block_handle.hpp @@ -115,6 +115,20 @@ class BlockMemory : public enable_shared_from_this { idx_t NextEvictionSequenceNumber() { return ++eviction_seq_num; } + //! Returns true if the block needs to be added to the eviction queue. + //! A block needs insertion when it has no valid entry in the queue (freshly loaded from disk). + //! Once added, the entry remains valid across pin/unpin cycles until the block is evicted. + bool NeedsEvictionQueueInsertion() const { + return needs_eviction_queue_insert.load(std::memory_order_relaxed); + } + //! Marks the block as needing eviction queue insertion (called when loaded from disk). + void MarkNeedsEvictionQueueInsert() { + needs_eviction_queue_insert.store(true, std::memory_order_relaxed); + } + //! Clears the flag (called after successfully adding to the eviction queue). + void ClearNeedsEvictionQueueInsert() { + needs_eviction_queue_insert.store(false, std::memory_order_relaxed); + } //! Get the LRU timestamp. int64_t GetLRUTimestamp() const { return lru_timestamp_msec; @@ -213,6 +227,9 @@ class BlockMemory : public enable_shared_from_this { unique_ptr buffer; //! The internal eviction sequence number. atomic eviction_seq_num; + //! Whether the block needs to be added to the eviction queue. + //! True when block has no valid queue entry (initial state, or after eviction/unload). + atomic needs_eviction_queue_insert {true}; //! The LRU timestamp for age-based eviction. atomic lru_timestamp_msec; //! When to destroy the data buffer. diff --git a/src/storage/buffer/block_handle.cpp b/src/storage/buffer/block_handle.cpp index a652940a65dc..df70c86e3c8b 100644 --- a/src/storage/buffer/block_handle.cpp +++ b/src/storage/buffer/block_handle.cpp @@ -233,6 +233,7 @@ BufferHandle BlockHandle::Load(QueryContext context, unique_ptr reus } memory.SetState(BlockState::BLOCK_LOADED); memory.SetReaders(1); + memory.MarkNeedsEvictionQueueInsert(); return BufferHandle(shared_from_this(), memory.GetBuffer()); } diff --git a/src/storage/buffer/buffer_pool.cpp b/src/storage/buffer/buffer_pool.cpp index 104d3fba62eb..2ebf6b61d48d 100644 --- a/src/storage/buffer/buffer_pool.cpp +++ b/src/storage/buffer/buffer_pool.cpp @@ -58,11 +58,11 @@ shared_ptr BufferEvictionNode::TryGetBlockMemory() { // The block memory has been destroyed. return nullptr; } - if (!CanUnload(*shared_memory_p)) { - // The memory handle was used in between. + if (handle_sequence_number != shared_memory_p->GetEvictionSequenceNumber()) { + // A newer queue entry supersedes this one. return nullptr; } - // The node is the latest node in the queue with this memory. + // The node is the latest entry in the queue for this memory. return shared_memory_p; } @@ -273,6 +273,13 @@ bool BufferPool::AddToEvictionQueue(shared_ptr &handle) { // The block handle is locked during this operation (Unpin), // or the block handle is still a local variable (ConvertToPersistent) D_ASSERT(memory.GetReaders() == 0); + + // If the block was not pinned since it was last added to the eviction queue, + // its previous queue entry is still valid — skip re-insertion to avoid creating dead nodes. + if (!memory.NeedsEvictionQueueInsertion()) { + return false; + } + auto ts = memory.NextEvictionSequenceNumber(); if (track_eviction_timestamps) { memory.SetLRUTimestamp(std::chrono::time_point_cast(std::chrono::steady_clock::now()) @@ -287,6 +294,7 @@ bool BufferPool::AddToEvictionQueue(shared_ptr &handle) { // Get the eviction queue for the block and add it BufferEvictionNode node(handle->GetMemoryWeak(), ts); + memory.ClearNeedsEvictionQueueInsert(); return queue.AddToEvictionQueue(std::move(node)); } @@ -468,7 +476,7 @@ void EvictionQueue::IterateUnloadableBlocks(FN fn) { } // get a reference to the underlying block pointer - auto handle = node.TryGetBlockMemory(); + auto handle = node.memory_p.lock(); if (debug_sleep_micros > 0) { // Debug race conditions regarding the ownership of the BlockMemory. // Note that for this to trigger we need at least one purge iteration with the setting active. @@ -482,8 +490,13 @@ void EvictionQueue::IterateUnloadableBlocks(FN fn) { // we might be able to free this block: grab the mutex and check if we can free it auto lock = handle->GetLock(); if (!node.CanUnload(*handle)) { - // something changed in the mean-time, bail out - DecrementDeadNodes(); + if (node.handle_sequence_number == handle->GetEvictionSequenceNumber()) { + // The entry was valid (seq_num matches) but the block can't be evicted right now. + // Mark it for re-insertion on the next unpin since we consumed its queue entry. + handle->MarkNeedsEvictionQueueInsert(); + } else { + DecrementDeadNodes(); + } continue; }