Add kvstore type and decouple kvstore from its metadata#12
Conversation
WalkthroughThe PR refactors the kvstore metadata system to support per-type customizable callbacks and removes the in-memory cluster slot statistics array. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/cluster_slot_stats.c`:
- Around line 218-224: The code risks a use-after-free because
kvstoreFreeDictIfNeeded(server.db->keys, slot) may free the dict backing the
slot and thus invalidate meta; to fix, write the reset assignments
(meta->cpu_usec = 0; meta->network_bytes_in = 0; meta->network_bytes_out = 0;)
before calling kvstoreFreeDictIfNeeded so meta is updated while still valid
(locate the block using getSlotMeta and kvstoreFreeDictIfNeeded in
cluster_slot_stats.c and move the three meta resets to precede the free call).
In `@src/server.c`:
- Around line 352-361: In dictDestructorKV detect that calling decrRefCount(kv)
can free kv and make the subsequent kvobjAllocSize(kv) a use-after-free; fix by
computing and caching size_t alloc_size = kvobjAllocSize(kv) immediately after
the NULL check and before calling decrRefCount(kv), then call decrRefCount(kv)
and, if server.memory_tracking_per_slot is set, use the cached alloc_size for
the debugServerAssert and to subtract from meta->alloc_size (obtained from
dictMetadata(d)); keep the early NULL return and otherwise preserve existing
logic and ordering.
🧹 Nitpick comments (2)
src/server.c (1)
6702-6703: Hoist kvstore metadata lookup outside the type loop.Minor optimization/readability:
kvstoreGetMetadatais invariant per-DB; fetch it once before the inner loop.♻️ Proposed refactor
- for (int type = 0; type < OBJ_TYPE_BASIC_MAX; type++) { - kvstoreMetadata *meta = kvstoreGetMetadata(server.db[dbnum].keys); - int64_t *kvstoreHist = meta->keysizes_hist[type]; + kvstoreMetadata *meta = kvstoreGetMetadata(server.db[dbnum].keys); + for (int type = 0; type < OBJ_TYPE_BASIC_MAX; type++) { + int64_t *kvstoreHist = meta->keysizes_hist[type];src/kvstore.c (1)
197-212: Consider adding NULL check fortypeparameter.The function dereferences
typeat line 201 (type->dictMetadataBytes) and line 206 (type->kvstoreMetadataBytes) without first verifying thattypeis non-NULL. While current callers appear to always pass valid types, a defensive NULL check would prevent crashes from misuse.Suggested fix
kvstore *kvstoreCreate(kvstoreType *type, dictType *dtype, int num_dicts_bits, int flags) { /* We can't support more than 2^16 dicts because we want to save 48 bits * for the dict cursor, see kvstoreScan */ assert(num_dicts_bits <= 16); + assert(type != NULL); assert(!type->dictMetadataBytes || type->dictMetadataBytes(NULL) >= sizeof(kvstoreDictMetaBase));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/cluster.csrc/cluster.hsrc/cluster_asm.csrc/cluster_slot_stats.csrc/db.csrc/kvstore.csrc/kvstore.hsrc/lazyfree.csrc/object.csrc/quicklist.csrc/server.csrc/server.hsrc/t_hash.csrc/t_stream.csrc/t_zset.ctests/unit/memefficiency.tcl
💤 Files with no reviewable changes (2)
- src/cluster.h
- src/cluster.c
🧰 Additional context used
🧬 Code graph analysis (12)
src/t_hash.c (2)
src/dict.c (1)
dictEmpty(1778-1793)src/server.h (1)
htGetMetadataSize(3622-3624)
src/object.c (2)
src/dict.c (1)
dictEmpty(1778-1793)src/server.h (1)
htGetMetadataSize(3622-3624)
src/t_stream.c (1)
src/zmalloc.c (1)
zmalloc_usable_size(463-465)
src/cluster_slot_stats.c (2)
src/kvstore.c (2)
kvstoreGetDictMeta(934-941)kvstoreFreeDictIfNeeded(134-136)src/cluster.c (1)
countKeysInSlot(820-822)
src/lazyfree.c (1)
src/kvstore.c (1)
kvstoreCreate(197-243)
src/t_zset.c (1)
src/zmalloc.c (1)
zmalloc_usable_size(463-465)
src/cluster_asm.c (1)
src/kvstore.c (1)
kvstoreCreate(197-243)
src/kvstore.h (1)
src/kvstore.c (4)
kvstoreCreate(197-243)kvstoreFreeDictIfNeeded(134-136)kvstoreGetDictMeta(934-941)kvstoreGetMetadata(943-945)
src/quicklist.c (1)
src/zmalloc.c (1)
zmalloc_usable_size(463-465)
src/db.c (1)
src/kvstore.c (3)
kvstoreGetDictMeta(934-941)kvstoreGetMetadata(943-945)kvstoreCreate(197-243)
src/kvstore.c (2)
src/dict.c (1)
dictRelease(737-754)src/zmalloc.c (1)
zcalloc(313-317)
src/server.c (3)
src/kvstore.c (4)
kvstoreGetDictMeta(934-941)kvstoreGetMetadata(943-945)kvstoreGetDict(51-53)kvstoreCreate(197-243)src/cluster.c (1)
clusterIsMySlot(1722-1724)src/dict.c (2)
dictEmpty(1778-1793)dictCreate(191-200)
🔇 Additional comments (37)
src/quicklist.c (1)
229-229: LGTM!Good change to broaden the allocation accounting check from test-only builds to all debug builds. The invariant is correct: after freeing all nodes and bookmarks,
alloc_sizeshould equal exactly the usable size of the quicklist struct itself.src/t_zset.c (1)
116-116: LGTM — debug-only invariant is appropriate.The debugServerAssert keeps the alloc-size invariant check in debug builds without affecting release behavior.
src/t_stream.c (1)
86-86: Debug assertion looks good.This strengthens alloc tracking consistency checks in debug builds without affecting release behavior.
src/server.h (2)
1142-1160: LGTM: kvstore metadata types look clean and well-scoped.
2844-2845: LGTM: kvstore type externs are clear and appropriately exposed.src/object.c (1)
535-539: LGTM: debug-only assertion guard is appropriate.src/t_hash.c (1)
2038-2041: LGTM!The change from
REDIS_TESTtoDEBUG_ASSERTIONSand fromserverAsserttodebugServerAssertis consistent with the broader debug assertion strategy. This ensures the metadata size check runs in debug builds rather than being tied to test builds specifically.tests/unit/memefficiency.tcl (3)
258-261: LGTM!The cluster-specific
config resetstatafter key deletions ensures accurate fragmentation measurements when per-slot statistics are being tracked in cluster mode.
485-489: LGTM!Consistent cluster-specific cleanup matching the pattern established in other tests.
708-710: LGTM!Correctly uses the synchronous
rclient for the resetstat since the deferred clients have already been closed.src/lazyfree.c (1)
210-211: KV store recreation update looks consistent.Switching to the new
kvstoreCreate(type, dtype, ...)signature and usingkvstoreExTypefor keys aligns with the metadata split.src/cluster_slot_stats.c (3)
49-61: Slot-stat read path now consistently uses dict metadata.The helper and read-side wiring look consistent with the kvstore metadata migration.
Also applies to: 110-121
154-172: Write-path counters correctly target per-slot metadata.Network and CPU aggregations now update the metadata in a coherent way.
Also applies to: 199-201, 249-251, 285-287
227-229: Reset-all loop aligns with per-slot metadata model.Iterating via
clusterSlotStatReset()is the right shape for metadata-backed stats.src/kvstore.h (1)
53-87: New kvstoreType + metadata helpers look good.The new type hooks and metadata accessors align with the refactor and make the API clearer.
Also applies to: 144-146
src/cluster_asm.c (1)
2887-2892: No issue found with kvstore type usage.The temp kvstores created at these lines use
kvstoreBaseType(with 0 metadata bytes), whiledb->keysuseskvstoreExType(with LRU/LFU metadata bytes). However, this is safe because:
- The dictionaries selected from these temp kvstores are only used for iteration within the function scope
- Data is transferred to
db->keysdirectly via themovingSlotSetoperation, not through dict structure merging- The temp kvstores are freed after use and never merged into
db->keys- The
dictMetadataBytesdifference does not impact this read-only iteration usage patternUsing
kvstoreBaseTypefor temporary kvstores is correct and more efficient thankvstoreExTypesince no metadata tracking is needed.src/db.c (5)
76-125: Good switch tokvstoreGetDictMetawith NULL guard.
The null-check ensures slot hist updates won’t crash when the slot dict is freed, while still updating the global histogram.
128-135: LGTM for per-slot alloc tracking update path.
UsingkvstoreGetDictMeta(..., 0)with an early return aligns with on-demand dict allocation semantics.
141-157: Debug histogram assertion remains correct with new metadata accessor.
Pulling the kvstore metadata viakvstoreGetMetadatakeeps the logic intact.
183-204: Per-slot alloc-size debug assertion updated correctly.
The NULL-safe metadata access matches the new dict metadata API and keeps the panic condition accurate.
968-983: Temp DB kvstore creation updates look consistent with new kvstore types.
UsingkvstoreExTypefor keys andkvstoreBaseTypefor expires aligns with the new type-based constructor.src/server.c (4)
790-804: LGTM — clear base vs. extended kvstore type wiring.
2953-2955: LGTM — db keys vs expires types are aligned with metadata needs.
2969-2975: LGTM — pubsub kvstores use base type consistently.
524-564: The NULL pointer dereference concern is not valid—callback invocation sites guarantee dict existence.
kvstoreCanFreeDictis only invoked fromfreeDictIfNeeded()(kvstore.c:126) after an explicit check thatkvstoreGetDict(kvs, didx)is non-NULL (line 120). Similarly,kvstoreOnDictEmptyis only called fromkvstoreEmpty()(line 254) andkvstoreRelease()(line 279) within loops that skip null dicts (if (!d) continue;). When these callbacks execute, the dict is guaranteed to exist, sokvstoreGetDictMeta(kvs, didx, 0)will always return a valid pointer to metadata rather than NULL. No changes needed.src/kvstore.c (12)
28-44: LGTM!The
kvstoreType *typefield is properly added to store the type-specific callbacks, and the flexible array membermetadata[]remains correctly positioned at the end of the struct for conditional allocation.
118-136: LGTM!The
canFreeDictcallback integration is well-implemented with proper NULL check before invocation. The new public wrapperkvstoreFreeDictIfNeededcleanly exposes the internal functionality for external callers needing to trigger dict cleanup after modifications.
185-188: LGTM!The renamed
kvstoreDictBaseMetaSizecorrectly returns the base metadata size and serves as the default when no customdictMetadataBytescallback is provided by the type.
214-225: LGTM!The assertions at lines 216-219 correctly enforce that the kvstore exclusively manages these dict callbacks, preventing configuration conflicts. The fallback to
kvstoreDictBaseMetaSizewhen no custom metadata size callback is provided ensures consistent behavior.
245-269: LGTM!The callback invocations are well-placed:
onDictEmptyis called per-dict after emptying but before potential deallocation, andonKvstoreEmptyis called after all dicts are processed, allowing type-specific cleanup logic access to kvstore state before internal counters are reset.
271-289: Verify: MissingonKvstoreEmptycallback inkvstoreRelease.
kvstoreEmptyinvokesonKvstoreEmptyafter processing all dicts (line 258), butkvstoreReleaseonly invokesonDictEmptyper-dict without a correspondingonKvstoreEmptycall. If the callback performs aggregate cleanup (e.g., resetting shared statistics), this inconsistency could cause resource leaks or incomplete cleanup when kvstores are released rather than emptied.Is this intentional given the kvstore is being destroyed, or should
onKvstoreEmptybe called beforezfree(kvs)?
305-317: LGTM!The memory usage calculation correctly uses the type-aware
dictMetadataBytescallback to account for potentially variable per-dict metadata sizes.
554-558: LGTM!The assertions properly validate preconditions for the dict move operation: valid index, matching kvstore dimensions, and empty destination slot.
934-941: LGTM!The
createIfNeededparameter provides a clean API for callers to control whether accessing metadata should lazily create the underlying dict. This is a useful abstraction for the cluster slot stats migration.
943-945: LGTM!Returns a pointer to the kvstore-level metadata stored in the flexible array member, enabling type-specific data to be associated with the kvstore itself.
991-997: LGTM!The test type with all-NULL callbacks appropriately tests the default behavior paths and verifies that the kvstore functions correctly when no custom callbacks are provided.
1029-1034: LGTM!Test calls are correctly updated to use the new
kvstoreCreatesignature, covering both allocation strategies and theFREE_EMPTY_DICTSflag combinations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| kvstoreDictMetadata *meta = getSlotMeta(slot, 0); | ||
| if (!meta) return; | ||
| kvstoreFreeDictIfNeeded(server.db->keys, slot); | ||
| meta->cpu_usec = 0; | ||
| meta->network_bytes_in = 0; | ||
| meta->network_bytes_out = 0; | ||
| } |
There was a problem hiding this comment.
Avoid use-after-free when resetting slot stats.
kvstoreFreeDictIfNeeded() can free the dict, invalidating meta before it’s written. This is a UAF risk.
🐛 Proposed fix (reset before potential free)
void clusterSlotStatReset(int slot) {
kvstoreDictMetadata *meta = getSlotMeta(slot, 0);
if (!meta) return;
- kvstoreFreeDictIfNeeded(server.db->keys, slot);
meta->cpu_usec = 0;
meta->network_bytes_in = 0;
meta->network_bytes_out = 0;
+ kvstoreFreeDictIfNeeded(server.db->keys, slot);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kvstoreDictMetadata *meta = getSlotMeta(slot, 0); | |
| if (!meta) return; | |
| kvstoreFreeDictIfNeeded(server.db->keys, slot); | |
| meta->cpu_usec = 0; | |
| meta->network_bytes_in = 0; | |
| meta->network_bytes_out = 0; | |
| } | |
| kvstoreDictMetadata *meta = getSlotMeta(slot, 0); | |
| if (!meta) return; | |
| meta->cpu_usec = 0; | |
| meta->network_bytes_in = 0; | |
| meta->network_bytes_out = 0; | |
| kvstoreFreeDictIfNeeded(server.db->keys, slot); | |
| } |
🤖 Prompt for AI Agents
In `@src/cluster_slot_stats.c` around lines 218 - 224, The code risks a
use-after-free because kvstoreFreeDictIfNeeded(server.db->keys, slot) may free
the dict backing the slot and thus invalidate meta; to fix, write the reset
assignments (meta->cpu_usec = 0; meta->network_bytes_in = 0;
meta->network_bytes_out = 0;) before calling kvstoreFreeDictIfNeeded so meta is
updated while still valid (locate the block using getSlotMeta and
kvstoreFreeDictIfNeeded in cluster_slot_stats.c and move the three meta resets
to precede the free call).
| static void dictDestructorKV(dict *d, void *kv) { | ||
| UNUSED(d); | ||
| if (kv == NULL) return; | ||
| kvstoreTrackDeallocation(d, kv); | ||
| decrRefCount(kv); | ||
| if (server.memory_tracking_per_slot) { | ||
| kvstoreDictMetadata *meta = (kvstoreDictMetadata *)dictMetadata(d); | ||
| size_t alloc_size = kvobjAllocSize(kv); | ||
| debugServerAssert(alloc_size <= meta->alloc_size); | ||
| meta->alloc_size -= alloc_size; | ||
| } |
There was a problem hiding this comment.
Fix use-after-free in per-slot accounting.
decrRefCount(kv) may free kv, so kvobjAllocSize(kv) can read freed memory. Capture the size before decrementing and use the cached value.
🐛 Proposed fix
static void dictDestructorKV(dict *d, void *kv) {
UNUSED(d);
if (kv == NULL) return;
- decrRefCount(kv);
- if (server.memory_tracking_per_slot) {
+ size_t alloc_size = 0;
+ if (server.memory_tracking_per_slot) {
kvstoreDictMetadata *meta = (kvstoreDictMetadata *)dictMetadata(d);
- size_t alloc_size = kvobjAllocSize(kv);
+ alloc_size = kvobjAllocSize(kv);
debugServerAssert(alloc_size <= meta->alloc_size);
meta->alloc_size -= alloc_size;
}
+ decrRefCount(kv);
}🤖 Prompt for AI Agents
In `@src/server.c` around lines 352 - 361, In dictDestructorKV detect that calling
decrRefCount(kv) can free kv and make the subsequent kvobjAllocSize(kv) a
use-after-free; fix by computing and caching size_t alloc_size =
kvobjAllocSize(kv) immediately after the NULL check and before calling
decrRefCount(kv), then call decrRefCount(kv) and, if
server.memory_tracking_per_slot is set, use the cached alloc_size for the
debugServerAssert and to subtract from meta->alloc_size (obtained from
dictMetadata(d)); keep the early NULL return and otherwise preserve existing
logic and ordering.
2 participants
Benchmark PR from qodo-benchmark#24
Summary by CodeRabbit
Release Notes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.