Replace fragile dict stored-key API with getKeyId callback#14
Conversation
WalkthroughThis PR refactors Redis's stored-key handling mechanism by introducing a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/defrag.c (1)
274-282: Fix build break and use the derived key for lookup.CI is failing with an undefined reference to
dictStoredKey2Key, anddictFindLinkis still called with the stored-key pointer even though you now compute the real key. After removingdictUseStoredKeyApi, this lookup should use the derived key. Consider inlining the conversion (or export the helper) and pass the derived key todictFindLink.🛠️ Suggested fix (avoids the undefined reference and uses the key)
- const void *key = dictStoredKey2Key(d, ptr); - link = dictFindLink(d, ptr, NULL); + const void *key = d->type->keyFromStoredKey ? d->type->keyFromStoredKey(ptr) : ptr; + link = dictFindLink(d, key, NULL);
🤖 Fix all issues with AI agents
In `@src/dict.c`:
- Around line 97-108: The function validateStoredKeyConversion is defined but
unused, causing -Werror=unused-function; either remove the function entirely or
invoke it in appropriate places (e.g., call validateStoredKeyConversion(d, key)
from dictAddRaw during debug/test builds or wrap its use under REDIS_TEST)
and/or replace it with an assertion that calls dictStoredKey2Key(d, key) to
validate behavior; update references to dictStoredKey2Key and ensure any added
call is only in non-production builds (DEBUG/REDIS_TEST) to avoid runtime
overhead.
🧹 Nitpick comments (1)
src/server.h (1)
3658-3658: Minor: Parameter name inconsistency between declaration and definition.The declaration uses
keybut the implementation insrc/t_hash.cusesfield. For hash operations,fieldis more semantically accurate since this function deletes hash fields, not keys (which refer to the hash object itself in Redis terminology).Suggested fix
-int hashTypeDelete(robj *o, void *key); +int hashTypeDelete(robj *o, void *field);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/defrag.csrc/dict.csrc/dict.hsrc/kvstore.csrc/memory_prefetch.csrc/module.csrc/rdb.csrc/server.csrc/server.hsrc/t_hash.c
💤 Files with no reviewable changes (2)
- src/rdb.c
- src/kvstore.c
🧰 Additional context used
🧬 Code graph analysis (5)
src/server.h (1)
src/t_hash.c (1)
hashTypeDelete(1257-1297)
src/module.c (1)
src/t_hash.c (1)
hashTypeDelete(1257-1297)
src/memory_prefetch.c (2)
src/object.c (1)
kvobjGetKey(234-246)src/dict.c (2)
dictGetNext(1081-1085)dictCompareKeys(502-506)
src/defrag.c (1)
src/dict.c (1)
dictStoredKey2Key(97-99)
src/server.c (2)
src/object.c (1)
kvobjGetKey(234-246)src/sds.h (1)
sdslen(98-111)
🪛 Cppcheck (2.19.0)
src/dict.c
[error] 341-341: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
🪛 GitHub Actions: CI
src/dict.c
[error] 102-102: Build failed due to -Werror: unused function 'validateStoredKeyConversion' in src/dict.c. Command: make REDIS_CFLAGS='-Werror' BUILD_TLS=yes.
🪛 GitHub Actions: Codecov
src/t_hash.c
[error] 1-1: Command failed: make -C src lcov (lcov step failed due to the above link error).
src/server.h
[error] 1-1: Command failed: make -C src lcov (lcov step failed due to the above link error).
src/module.c
[error] 1-1: Command failed: make -C src lcov (lcov step failed due to the above link error).
src/memory_prefetch.c
[error] 1-1: Command failed: make -C src lcov (lcov step failed due to the above link error).
src/defrag.c
[error] 280-280: Linking failed: undefined reference to 'dictStoredKey2Key' in activeDefragHfieldAndUpdateRef. This function may be missing implementation or prototype/configuration in the dict subsystem.
[error] 1-1: Command failed: make -C src lcov (lcov step failed due to the above link error).
src/dict.h
[error] 1-1: Command failed: make -C src lcov (lcov step failed due to the above link error).
src/dict.c
[error] 1-1: Command failed: make -C src lcov (lcov step failed due to the above link error).
src/server.c
[error] 1-1: Command failed: make -C src lcov (lcov step failed due to the above link error).
🪛 GitHub Actions: CodeQL
src/defrag.c
[error] 280-280: Undefined reference to 'dictStoredKey2Key' during linking, indicating missing implementation or missing object file for this function.
🪛 GitHub Actions: External Server Tests
src/dict.c
[error] 102-102: validateStoredKeyConversion defined but not used. (-Werror=unused-function)
🔇 Additional comments (27)
src/t_hash.c (7)
1256-1259: Function signature simplification looks correct.The removal of the
isSdsFieldparameter is consistent with the PR objective. All call sites in this file pass SDS strings, and the implementation correctly casts thevoid *fieldtosdsat line 1259.The comment accurately documents the expected type. Consider whether the parameter type could be changed from
void *fieldtosds fieldto make the contract explicit at the type level, though the current approach may be intentional for API consistency.
537-537: LGTM!Call site correctly updated to the new two-parameter signature.
764-764: LGTM!Call site correctly updated with assertion on the return value.
1112-1112: LGTM!Call site correctly updated to the new signature.
2723-2723: LGTM!Call site correctly updated;
c->argv[i]->ptris an SDS for string objects.
2928-2928: LGTM!Call site correctly updated to the new two-parameter signature.
3503-3503: LGTM!Call site correctly updated;
fieldis obtained fromentryGetField(e)which returns SDS.src/memory_prefetch.c (1)
233-238: The code change correctly extracts the key before comparison. WhilekvobjGetKey()technically can return NULL whenkv->iskvobjis false, all 30+ uses of this function throughout the codebase proceed without NULL checks, indicating this is a design invariant for dictionary entries—iskvobjis always set to true for kvobj entries stored in the main dictionary. Adding a defensive NULL check here would be inconsistent with the established pattern across the codebase and unnecessary given this guarantee. No changes required.src/module.c (1)
5454-5456: LGTM — updated call matches new hashTypeDelete signature.
No issues found with the new two-parameter usage.src/defrag.c (1)
486-492: LGTM: correct use ofplinkwhen updating hash dict keys.src/server.c (3)
321-333: dictSdsCompareKV update looks consistent with key extraction.The signature and sds-to-sds compare logic line up with the new
keyFromStoredKeyflow.
604-615: LGTM: keyFromStoredKey wired for both db and expires dicts.This aligns the dict types with the new stored-key extraction path and keeps lookups consistent.
Also applies to: 618-629
316-319: Add a defensive guard or assert the kvobj invariant.
kvGetKeyreturns NULL whenkv->iskvobjis false, which would causedictSdsHash/sdslento crash. Confirm that all entries stored indbDictType/dbExpiresDictTypeare guaranteed to haveiskvobj=true, or add an assertion to catch violations:Defensive guard
static const void *kvGetKey(const void *kv) { sds sdsKey = kvobjGetKey((kvobj *) kv); + serverAssert(sdsKey != NULL); return sdsKey; }src/dict.h (5)
55-66: LGTM! Clear documentation of the stored-key concept.The documentation clearly explains the distinction between lookup keys and stored-keys, and when the
keyFromStoredKeycallback is needed. The__stored_keymarker macro provides good self-documenting annotations for API consumers.
94-97: LGTM! Consistent annotation of callback signatures.The
__stored_keyannotation onkeyDupandkeyDestructorcorrectly indicates these callbacks receive stored-key objects rather than lookup keys.
128-134: LGTM! Well-designed callback for stored-key extraction.The
keyFromStoredKeycallback placement after the flags and the accompanying documentation clearly explains its purpose forno_value=1dictionaries.
157-157: Type change from bit-field toint16_taffects struct layout.This changes
pauseAutoResizefrom a bit-field to a signedint16_t. The comment mentions negative values indicate coding errors, which is a reasonable defensive check. The padding optimization note in the struct comment (line 155) supports this change.
239-242: LGTM! Consistent__stored_keyannotation across public API.All insertion and mutation APIs (
dictAdd,dictAddRaw,dictAddOrFind,dictReplace,dictSetKeyAtLink,dictSetKey) are correctly annotated to indicate they accept stored-key objects.Also applies to: 288-288, 291-291
src/dict.c (9)
124-126: LGTM! Minor formatting adjustment.The spacing change in
siphashcall is cosmetic.
339-364: LGTM! Correct stored-key handling during rehashing.The rehashing logic properly extracts the lookup key via
dictStoredKey2Keybefore computing the hash for the new bucket, while preserving the originalstoredKeyfor storage in the destination bucket.
526-536: LGTM! Stored-key extraction before lookup.
dictAddRawcorrectly extracts the lookup key viadictStoredKey2KeyfordictFindLinkForInsert, then passes the original stored-key todictInsertKeyAtLinkafter optional duplication.
636-650: LGTM! Consistent stored-key extraction in delete path.
dictGenericDeleteproperly extracts the lookup key from each entry viadictStoredKey2Keyfor comparison against the search key.
779-802: LGTM! Stored-key extraction in lookup internals.
dictFindLinkInternalcorrectly extracts the lookup key from visited entries for comparison while iterating the hash chain.
887-914: LGTM! Stored-key handling indictSetKeyAtLink.The function correctly uses
dictStoredKey2Keywhen looking up the bucket after table reallocation (line 902) and when finding an existing link (line 914).
964-973: LGTM! Stored-key extraction in two-phase unlink.
dictTwoPhaseUnlinkFindproperly extracts keys for comparison during the search loop.
1748-1769: LGTM! Stored-key extraction in insert path lookup.
dictFindLinkForInsertcorrectly extracts keys from existing entries viadictStoredKey2Keyfor duplicate detection.
1807-1809: LGTM! CleandictGetHashimplementation.The inline comment about compiler optimization is helpful, and the implementation correctly delegates to the type's hash function.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) { | ||
| return (d->type->keyFromStoredKey) ? d->type->keyFromStoredKey(key) : key; | ||
| } | ||
|
|
||
| /* Validate that stored-key to key conversion works correctly */ | ||
| static int validateStoredKeyConversion(dict *d, const void *key __stored_key) { | ||
| const void *extracted = dictStoredKey2Key(d, key); | ||
| if (d->type->keyFromStoredKey) { | ||
| return extracted != NULL; | ||
| } | ||
| return extracted == key; | ||
| } |
There was a problem hiding this comment.
Remove or use validateStoredKeyConversion to fix build failure.
The function validateStoredKeyConversion is defined but never called, causing the build to fail with -Werror=unused-function. Either add calls to this validation function where appropriate (e.g., in dictAddRaw for debug builds) or remove it entirely.
Option 1: Remove the unused function
static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) {
return (d->type->keyFromStoredKey) ? d->type->keyFromStoredKey(key) : key;
}
-
-/* Validate that stored-key to key conversion works correctly */
-static int validateStoredKeyConversion(dict *d, const void *key __stored_key) {
- const void *extracted = dictStoredKey2Key(d, key);
- if (d->type->keyFromStoredKey) {
- return extracted != NULL;
- }
- return extracted == key;
-}Option 2: Guard with REDIS_TEST or use in assertions
+#ifdef REDIS_TEST
/* Validate that stored-key to key conversion works correctly */
static int validateStoredKeyConversion(dict *d, const void *key __stored_key) {
const void *extracted = dictStoredKey2Key(d, key);
if (d->type->keyFromStoredKey) {
return extracted != NULL;
}
return extracted == key;
}
+#endif📝 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.
| static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) { | |
| return (d->type->keyFromStoredKey) ? d->type->keyFromStoredKey(key) : key; | |
| } | |
| /* Validate that stored-key to key conversion works correctly */ | |
| static int validateStoredKeyConversion(dict *d, const void *key __stored_key) { | |
| const void *extracted = dictStoredKey2Key(d, key); | |
| if (d->type->keyFromStoredKey) { | |
| return extracted != NULL; | |
| } | |
| return extracted == key; | |
| } | |
| static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) { | |
| return (d->type->keyFromStoredKey) ? d->type->keyFromStoredKey(key) : key; | |
| } |
| static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) { | |
| return (d->type->keyFromStoredKey) ? d->type->keyFromStoredKey(key) : key; | |
| } | |
| /* Validate that stored-key to key conversion works correctly */ | |
| static int validateStoredKeyConversion(dict *d, const void *key __stored_key) { | |
| const void *extracted = dictStoredKey2Key(d, key); | |
| if (d->type->keyFromStoredKey) { | |
| return extracted != NULL; | |
| } | |
| return extracted == key; | |
| } | |
| static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) { | |
| return (d->type->keyFromStoredKey) ? d->type->keyFromStoredKey(key) : key; | |
| } | |
| `#ifdef` REDIS_TEST | |
| /* Validate that stored-key to key conversion works correctly */ | |
| static int validateStoredKeyConversion(dict *d, const void *key __stored_key) { | |
| const void *extracted = dictStoredKey2Key(d, key); | |
| if (d->type->keyFromStoredKey) { | |
| return extracted != NULL; | |
| } | |
| return extracted == key; | |
| } | |
| `#endif` |
🧰 Tools
🪛 GitHub Actions: CI
[error] 102-102: Build failed due to -Werror: unused function 'validateStoredKeyConversion' in src/dict.c. Command: make REDIS_CFLAGS='-Werror' BUILD_TLS=yes.
🪛 GitHub Actions: External Server Tests
[error] 102-102: validateStoredKeyConversion defined but not used. (-Werror=unused-function)
🤖 Prompt for AI Agents
In `@src/dict.c` around lines 97 - 108, The function validateStoredKeyConversion
is defined but unused, causing -Werror=unused-function; either remove the
function entirely or invoke it in appropriate places (e.g., call
validateStoredKeyConversion(d, key) from dictAddRaw during debug/test builds or
wrap its use under REDIS_TEST) and/or replace it with an assertion that calls
dictStoredKey2Key(d, key) to validate behavior; update references to
dictStoredKey2Key and ensure any added call is only in non-production builds
(DEBUG/REDIS_TEST) to avoid runtime overhead.
1 participant
Benchmark PR from qodo-benchmark#90
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.