Repository navigation
Conversation
Infrastructure for the cache refactor, plus the first family migrated. - `common/cache`: S3-FIFO with ghost queue. - `storages/common/cache`: `CacheKind` (23 kinds, 3 pools), `CachedItem`, `CacheRef`/`Reservation`, `MemCache` (single-flight, read-only view, optional disk tier), `KindStore` with `LegacyStore` (pre-refactor capacity rules) and `SharedStore` (pooled S3-FIFO), `CacheManager` with `system.caches` rows and `set_capacity_by_name`. - `mem_bytes` for all cached types; the nine that reported 0 now report real sizes, verified against a counting allocator. - Snapshot family moves to `FuseCaches::snapshots` + `SnapshotReader` / `SnapshotStatsReader`. The old `CacheManager` builds the new manager for migrated kinds and routes `set_cache_capacity` and `system.caches` to it. - Benches: hit path (current vs legacy vs shared backend), pressure shrink, S3-FIFO vs LRU hit rate.
github-actions
Bot
added
the
pr-refactor
bohutang
added
the
ci-benchmark-cloud
⏱ CI timing · Devrun 41219395591 · attempt 2 (wall time includes re-run gap) · 58.6 min (main p50 63.3 min, −4.6 min)
Minutes unless stated; durations exclude queue time unless stated; "max" = longest job in the group. Baseline = median of the last 10 successful runs (Dev: |
Docker Image for PR
|
Move segments, column-oriented segments, segment block metas and segment statistics onto the kind-based cache, still on the legacy capacity rules. - `SegmentCache` in `FuseCaches` + `SegmentInfoReader` / `SegmentStatsReader` replace `MetaReaders::segment_*`; call sites hold the cache and reader. - The two duplicated `extract_block_metas` paths share `SegmentCache::block_metas`. - RowFetch and replace into look up `segment_block_metas` before decoding a segment; the per-block `BlockMeta` cache is removed (disabled by default, `block_meta_count` no longer has an effect). - The old `CacheManager` drops these slots; `set_cache_capacity` and `system.caches` for them are served by the new manager.
bohutang
changed the title
bohutang
added
ci-benchmark-cloud
⏱ CI timing · Cloud (PR image)run 41009288689 · to image 35.4 min (recent p50 48.4 min, −13.0 min)
Minutes unless stated; durations exclude queue time unless stated; "max" = longest job in the group. Baseline = median of the last 10 successful runs (Dev: |
Docker Image for PR
|
…e and prune result caches Move BloomIndexMeta, BloomFilter, VectorIndexMeta, SpatialIndexMeta, VirtualColumnMeta, SpatialIndex, VectorRefine and PruneResult to the kind-based store. IndexMetaReader<T> replaces MetaReaders. Disk tiers keep their directories and sizes; the license toggle also hides them from system.caches.
bohutang
changed the title
…g caches Move the remaining kinds to the kind-based store: inverted index meta / lookup / payload / full payload, decoded HNSW indexes, column data and arrays, parquet file metas and iceberg tables. The old CacheManager now only assembles the store and the disk tiers; HybridCache, the CacheReader / Loader readers, CachedObject and VectorIndexCache are removed. A kind with its memory tier off keeps using its disk tier, as the hybrid caches did.
bohutang
changed the title
Add shared_memory_enabled (off by default) and the meta / index / data pool ratios. With it on, caches live in three S3-FIFO pools sized by ratio of the memory limit, the legacy capacity options are ignored with a warning, and set_cache_capacity accepts pool names. QueriesMemoryManager::set_reclaim_handle lets a waiting allocation shrink caches before a query is killed when the global limit is exceeded. It is registered only for the shared backend, so the legacy backend behaves as before. CI runs the base, query and standalone sqllogictests on the shared backend.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83daa30fd8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # src/query/storages/fuse/src/operations/read/parquet_rows_fetcher.rs
…tale queue positions - LegacyStore: replacing an entry counts the old value as room, so growing the least recently used entry stays within the byte capacity; a value larger than the whole capacity is not cached. - SingleFlight: a cancelled load with no other waiter removes its entry. - S3Fifo: removals compact stale queue positions once they outnumber live entries. - Vector index reader tests write separate files, so cached index metas of one test are not read by the other.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a22b8fbdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ntries - SharedStore reserves the kind limit and the pool capacity with compare-and-swap before inserting, and gives them back when the insert is rejected, so concurrent fills cannot together exceed either. - TTLReader: a background refresh replaces the entry with put; if the cache rejects the new value, or the refresh fails, the stale entry is dropped so a later read reloads it instead of serving it forever.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9c60d9c5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rejected victims - LegacyStore charges an admitted value's reservation before releasing the kind's LRU lock, so concurrent VectorIndex inserts, which check resident bytes under that lock, cannot both pass the check. - Entries evicted by an insert that is then rejected are accounted for, instead of staying in the item and in-cache counters.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54fdbbbbfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff6de590de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| shard.pop_evictable(Entry::is_shared, CHECKS_PER_POP) | ||
| else { | ||
| break; |
There was a problem hiding this comment.
With shared caching enabled, a shard whose main queue starts with 32 pinned entries followed by unpinned entries still stops eviction prematurely: pop_evictable returns None after keeping 32 entries, and this break abandons the remaining entries. If the other shards cannot yield a victim, pressure reclamation stops despite reclaimable memory, and capacity-driven insertion incorrectly returns PoolFullPinned. Fresh evidence beyond the earlier aging-only case: a reproduction using the current S3Fifo with 32 pinned entries followed by one unpinned entry returns None on its first scan and evicts the unpinned entry on its second. Distinguish scan-budget exhaustion from a completed search before abandoning the shard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9540010: evict_batch no longer abandons a shard after one empty pop_evictable. Kept entries rotate to the back, so it keeps scanning until the checks reach the shard's entry count (a full turn, as quick_cache does), bumping the lock between scans. Test: test_eviction_scans_past_held_entries (96 held entries in front of one free entry, one shard).
- evict_batch gave up a shard once pop_evictable had kept 32 held entries, so held entries at the front of a shard hid evictable ones behind them: pressure reclamation stopped and inserts were refused as PoolFullPinned with memory to free. Kept entries rotate to the back, so the shard is now done only once as many checks as it has entries came back empty, as quick_cache gives up only after a full turn of its ring; the lock is bumped between two scans. - pop_evictable_where restarted from the queue front for each victim and left passed entries in place: a victim cost a scan, and entries at the front lost an access on every eviction. evict_where evicts a batch in one scan and moves aged and kept entries to the back of the main queue, as pop_evictable does; other kinds keep their place. Tests: test_eviction_scans_past_held_entries, evict_where order, target, kept entries, clock rotation matching pop_evictable, bounded queues.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The queues were VecDeques of (key, sequence number): a removal or a move left a stale position behind, skipped when reached and dropped by compaction, and a scan over one kind could not take an entry out of the middle. Entries now live in a slab of slots linked into the small and the main queue by index, as quick_cache does (foyer uses an intrusive list): removing or moving an entry is O(1) and leaves nothing behind, and the key is stored once, found through a HashTable of slot indexes. Free slots are reused; once they outnumber the entries the slab is rebuilt in queue order and released, so it stays within twice the entries plus a constant after the entries shrink. Eviction order and every result are unchanged. PoolStructure reports slots instead of queue positions. Tests, in s3fifo.rs: a plain reference S3-FIFO (a map and two lists of keys) run against the slab on random gets, inserts, admissions, removals, pop_evictable with held values and check budgets, evict_where, remove_if and bulk fills that make the slab compact; each result, both queues in order, the byte counts and the slab's links, table and free list are compared after every operation, with admission on first and on second access. test_compaction_keeps_order checks the order after a rebuild against the reference.
The ghost of each shard remembered a fixed 16Ki keys whatever the pool size: for a data pool of large values that is many times the data the pool holds, so a value read twice within a long window was admitted and the second-read admission let scans through; for small values it could be too short. As libCacheSim (ghost-size-ratio 0.9) and foyer size ghosts in bytes, a pool's ghosts now also stop at 0.9 times its cap in bytes, split over the shards, and follow the cap when it changes; a pool turned off forgets them. The record bound stays, so the ghost's memory does not grow. A shard keeps its newest record even when larger than its share, so that any admissible value can still be admitted on a prompt second read. S3Fifo::admission takes the value's bytes; PoolStructure reports ghost bytes and their bound. Tests: ghost byte bound, oldest dropped first, newest kept when large, size updates and bound changes, byte count against the live records on random operations; pool ghosts following the cap and a large value admitted on its second read; the model comparison in s3fifo.rs now runs with a byte-bounded ghost and compares ghost counts and bytes; the soak checks ghost bytes against the bound.
16 threads fill a kind limited to 50 values: it never passes its limit, refuses nothing, and every value but the last 50 is counted as evicted for capacity.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 288ff3ca92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| break; | ||
| } | ||
| // Let readers in between two scans of a shard full of held entries. | ||
| RwLockWriteGuard::bump(&mut shard); |
There was a problem hiding this comment.
With shared caching enabled, scanning more than 32 pinned entries can reach this call during pressure reclamation, even when blocking is false. RwLockWriteGuard::bump yields the lock and then reacquires it with a blocking write. A queued cache writer can acquire the yielded shard, allocate during insertion, and enter QueriesMemoryManager::reclaim, where it waits on the reclaiming mutex held by the original reclaimer. The reclaimer then waits for that writer's shard lock inside bump, permanently deadlocking both threads under memory pressure. Avoid bump on the nonblocking path, or release the guard and reacquire only with try_write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 10dbeee: evict_batch bumps the shard lock only when blocking. Non-blocking eviction (reclaim) takes each shard with try_write once and keeps it until its scan is done, so it never waits for a shard lock. No deterministic test: the hang needs a writer to take the yielded lock and allocate inside it; the property is stated on evict_batch.
Scanning a shard past held entries bumped its lock between two scans, also when reclaiming. bump takes the lock back by waiting, and the writer it let in could allocate, enter reclaim and wait for the reclaim in progress: both threads hung. Only blocking eviction bumps now; reclaim tries each shard once and keeps it until its scan ends.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… memory limit - Data on first read. The data pool cached a column value only on its second read; caching data once read matters more than filtering scans out, which probation in the small queue still does. Every pool now admits on first read, the ghost still sends a key evicted from probation and read again to the main queue. Deferral is gone with it: InsertMode, InsertOutcome::Deferred, PoolStore::admission and the cache_admission_deferred_count metric. - Loads shared per kind. A MemCache made its own single-flight, so the parquet meta, parquet column data and iceberg table caches, which make a handle per call, loaded one key once per concurrent caller. CacheManager keeps one single-flight per kind for every handle. - A memory limit for the shared backend. Its pools were sized from 80% of system memory when max_server_memory_usage was 0, and only reclaimed when an enforced limit was reached, which max_memory_limit_enabled (off by default) decides: up to 48% of memory as cache that nothing would shrink. Startup now fails unless both are set; the legacy backend is unchanged. CI's standalone_shared_cache job sets them to 80% of the runner memory. - Reclaim outside queries. An allocation over the global limit on a thread outside any query (background work, cache fills) was let through without reclaiming; it now reclaims cache memory first, and is still never refused. Without a reclaim handler (legacy) nothing is done, not even flushing the stat buffers. An empty pool is not scanned by a reclaim. Tests: every kind through both backends loads once for concurrent misses through two handles (test_handles_share_loads) and is cached on its first read; an allocation outside queries over the limit reclaims, under it or without a handler does not (test_allocation_outside_queries_reclaims); the shared backend refuses a config without a memory limit, the legacy one accepts it; a hot set of vector indexes keeps a hit rate of at least 0.95 while bloom filters scan a quarter of their shared pool every round (test_hot_index_survives_scans_of_another_kind); the pool's ghost bytes follow its cap with first-read admission.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Every pool caches a value on its first read since 1453dd6, so the admit_after knob of S3Fifo was always 1 and nothing called admit or admission any more. Removed: admit_after, Admission, Placement, admit, admission and insert_at; insert is the one way in, sending a key the ghost remembers to the main queue as before. The ghost no longer keeps access counts, only which keys were evicted from probation and their bytes. Behavior is unchanged. Tests: the reference model in s3fifo.rs drops admission too and runs with the default and with a byte-bounded ghost; the it tests use insert.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
09_0043 expected one row from system.caches for memory_cache_parquet_meta_data; in a cluster there is one per node, and only the nodes that read the stage files cache their footers, so cluster (base, 2c, 2) got 0, 0, 1. Sum hits and items over the nodes.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
- S3-FIFO allocates before it mutates; removals and evictions do not allocate, and the ghost drops a record rather than fail. Per-class lists let a kind evict or be removed without walking other kinds. - Budgets are RAII charges taken one at a time; in-cache counts come from the queues (shared) or the LRU (legacy) instead of atomics. - Memory-pressure reclaim looks at a bounded number of entries over all pools, skips busy shards, never compacts, and no longer serializes reclaiming threads behind a mutex. - A per-pool bounds lock orders inserts against cap and limit changes; kind limits are enforced on resident bytes. - Loads are shared only for kinds whose files are written once (CacheDef::SHARE_LOADS); a joined flight checks memory first. - Pruning results size parts by PartInfo::mem_bytes; iceberg values count their engine options.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3d1831be3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let state = self.kind(kind); | ||
| let capacity = *state.capacity.lock(); | ||
| if capacity == LegacyCapacity::Disabled { | ||
| return InsertOutcome::Rejected(RejectReason::Disabled); | ||
| } |
There was a problem hiding this comment.
On the default legacy backend, a concurrent capacity reduction can be bypassed because insertion snapshots capacity before acquiring the LRU lock. An insertion can read the old nonzero capacity, pause while set_limit updates the capacity and finishes evicting entries, and then insert using the old budget. This also allows an entry to remain resident after the cache has been disabled, since subsequent fills are rejected without removing it. Previously, InMemoryLruCache changed capacity under the same lock used for insertion. Protect the capacity check and publication with the same synchronization as resizing so an insertion cannot publish against bounds that have already been replaced.
Useful? React with 👍 / 👎.
An insert read the capacity before locking the LRU, so it could insert against a capacity that set_limit had already lowered and evicted down to, leaving an entry in a disabled kind. set_limit now changes the capacity under the LRU lock and inserts read it there.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
format! grows its string by doubling, so the sample's JSON allocation was about twice its length; the estimate counts the allocation and passed the test's bound. Shrink the sample to its length.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
Refactors the cache subsystem onto one kind-based infrastructure, migrates every cache onto it, and adds a shared-memory backend behind
cache.shared_memory_enabled(off by default). The default backend keeps main's capacity rules and accounting.Why. Each of the 24
CacheManagerslots is its own LRU whosegettakes a write lock; capacities are per cache and can sum past node memory; memory pressure never reclaims cache; IO and caching are mixed in one reader.Changes
common/cache,storages/common/cache):CacheKindregistry (23 kinds, 3 pools, legacy names kept),MemCache<T>(single-flight shared per kind, read-only view, optional disk tier),Reader(IO + decode only),CacheRef(values held by readers stay charged),mem_bytesfor every cached type.LegacyStore(default): one LRU per kind with main's capacity rules and byte formulas.SharedStore(shared_memory_enabled): three S3-FIFO pools (meta / index / data, 10 / 40 / 10 % ofmax_server_memory_usage), queues in a linked slab, ghost bounded by 0.9× the pool cap. Values are cached on first read; a kind with[cache.kind_max_bytes]evicts its own coldest entries. Under memory pressure, caches shrink before a query is killed, also for allocations outside queries. Requiresmax_memory_limit_enabledandmax_server_memory_usage, and refuses to start without them.HybridCache,CacheReader/Loader, the per-cache type aliases and the per-blockBlockMetacache.standalone_shared_cacherunsbase,queryandstandalonesqllogictests on the shared backend.Known differences vs main (default backend)
memory_cache_block_metais gone:set_cache_capacityon it errors; RowFetch and replace into reusesegment_block_metas.system.cachesreports real sizes for snapshots, and for byte-bounded caches counts values evicted but still held; a cache set to 0 at runtime has no row.Performance (db0: 16 vCPU aarch64, 30 GiB, release, local fs; shared backend vs main)
* Q78 sits at the 8 GiB limit and succeeded once on each side (22 s vs 32 s); the other queries are within 0.5 %.
Hit path per lookup, 10k keys: 1 thread 194 ns (main) / 228 ns (legacy) / 304 ns (shared); 64 threads 48.1 µs / 55.3 µs / 2.39 µs.
Correctness and stability (db0)
Tests: unit and integration tests for both backends over all 23 kinds, S3-FIFO against a reference model, kind turnover under 16 threads, scan resistance of a shared pool, reclaim / scrape / traffic deadlock tests, the soak;
storages::fusesuites; sqllogictestsbase,query,no_table_meta_cache,standalone(both backends).Not in this PR: turning the shared backend on by default;
system.cachescolumns for pools and eviction counters (Prometheus metrics for now).Tests
Type of change
AI assistance
This change is