Apply *-max-listpack-value symmetrically on RDB load - #4371
Conversation
The listpack-encoded hash/set/zset load paths in rdb.c only checked the entries threshold (O(1) via lpLength) and skipped the value-size threshold when deciding whether to convert to hashtable/skiplist. The runtime insertion path applies both (t_hash.c, t_set.c, t_zset.c), so a key loaded from RDB could end up listpack-encoded even though its current config says it should be hashtable -- but only if the offending dimension was value size, not count. Lowering hash-max-listpack-value (or the set/zset equivalents) and reloading a snapshot produced under a larger threshold left oversized listpacks in place. Issue valkey-io#4203. Add lpMaxElementLength(lp, step) which scans a listpack once and returns the maximum byte length of its string entries; integer-encoded entries are ignored. step=1 inspects every entry (sets; hashes, where both field and value matter); step=2 skips every second entry (zsets, where only the element length is checked at runtime, scores are numbers). Wire it into the three listpack load branches alongside the existing entries check, so the load path mirrors the runtime conversion in all dimensions. The scan is O(n) in the listpack length, same order as the existing RDB_TYPE_HASH_ZIPLIST branch (rdb.c:2392) which already does this for the legacy hash format, and as the field-by-field RDB_TYPE_HASH branch. The entries check short-circuits first, so the scan only runs when the entries threshold alone does not already force a conversion. Add regression tests in tests/unit/type/{hash,set,zset}.tcl that write an oversized value under a permissive threshold, tighten the threshold, reload through DEBUG RELOAD, and assert the encoding flips. The three tests fail before this change and pass after. Signed-off-by: quanyeyang <quanyemostima@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRDB loading now checks listpack element lengths against configured thresholds for sets, sorted sets, and hashes. Debug-only tests verify conversion to hashtable or skiplist encoding after reload under stricter limits. ChangesRDB listpack conversion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rdb.c`:
- Around line 2575-2576: Update the legacy RDB conversion checks in src/rdb.c
lines 2575-2576 and 2620-2624: extend the RDB_TYPE_ZSET_ZIPLIST condition to
check lpMaxElementLength(..., 2), and extend the RDB_TYPE_HASH_ZIPLIST condition
to check lpMaxElementLength(..., 1), alongside the existing entry-count checks.
Add regression coverage confirming oversized zset members, hash fields, and hash
values convert legacy payloads appropriately.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8474b29d-eff0-42ab-b282-bee4a5b85c1d
📒 Files selected for processing (4)
src/rdb.ctests/unit/type/hash.tcltests/unit/type/set.tcltests/unit/type/zset.tcl
The listpack fix in the previous commit left RDB_TYPE_ZSET_ZIPLIST and RDB_TYPE_HASH_ZIPLIST applying the same entries-only check. Apply lpMaxElementLength there too so the load path is consistent across all five hash/set/zset branches. No new tests: current builds cannot emit these legacy types (rdbGetObjectType only outputs listpack types), so they are unreachable via DEBUG RELOAD; the shared helper is already covered by the listpack regression tests. Per coderabbitai review on valkey-io#4371. Signed-off-by: quanyeyang <quanyemostima@gmail.com>
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #4371 +/- ##
============================================
- Coverage 77.01% 77.00% -0.01%
============================================
Files 162 162
Lines 81784 81804 +20
============================================
+ Hits 62988 62997 +9
- Misses 18796 18807 +11
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
The three *_LISTPACK branches plus the two legacy ziplist branches now apply the value threshold on load, which does close the gap from #4203 for string entries. Two things on the code and one on the tests below.
Separately: the two ziplist branches added in 1a02dc4 (RDB_TYPE_ZSET_ZIPLIST, RDB_TYPE_HASH_ZIPLIST) have no coverage — tests/integration/convert-ziplist-{hash,zset}-on-load.tcl already load the fixtures with a *-max-ziplist-entries 1 override, so a *-max-ziplist-value 1 variant next to it would exercise them the same way convert-zipmap-hash-on-load.tcl:29 does for the zipmap path.
| /* Return the maximum byte length of string entries in a listpack. | ||
| * step=1 inspects every entry (sets, hashes); step=2 skips every second | ||
| * entry (zsets: skip scores). Integer-encoded entries are ignored. | ||
| * Used by the RDB load path to enforce *-max-listpack-value symmetrically | ||
| * with the runtime conversion. */ | ||
| static unsigned int lpMaxElementLength(unsigned char *lp, int step) { | ||
| unsigned int maxlen = 0, slen; | ||
| long long lval; | ||
| unsigned char *p = lpFirst(lp); | ||
| int idx = 0; | ||
| while (p) { | ||
| if (step != 2 || (idx % 2 == 0)) { | ||
| if (lpGetValue(p, &slen, &lval) != NULL) { | ||
| if (slen > maxlen) maxlen = slen; | ||
| } | ||
| } | ||
| p = lpNext(lp, p); | ||
| idx++; | ||
| } | ||
| return maxlen; |
There was a problem hiding this comment.
lpGetValue returns NULL for integer-encoded entries (src/listpack.c:587-598), so the scan skips them and the load path still disagrees with the runtime for numeric fields/values/members. At runtime the digit length is what counts: setTypeAddAux measures the string form of an integer member (src/t_set.c:137-139) before comparing at src/t_set.c:164, and src/t_hash.c:375 / src/t_zset.c:1538 use sdslen unconditionally. The sds-based RDB branches in this same file count them too — rdb.c:2045, rdb.c:2127, rdb.c:2199, and the zipmap path at rdb.c:2418-2419.
Concretely: with hash-max-listpack-value 64, HSET h f 1234567890123456789 stays a listpack with the value int-encoded; lower the threshold to 8 and DEBUG RELOAD and it stays a listpack, while any further HSET on that key flips it to a hashtable. Passing an int buffer to lpGet makes both cases report the string length:
| /* Return the maximum byte length of string entries in a listpack. | |
| * step=1 inspects every entry (sets, hashes); step=2 skips every second | |
| * entry (zsets: skip scores). Integer-encoded entries are ignored. | |
| * Used by the RDB load path to enforce *-max-listpack-value symmetrically | |
| * with the runtime conversion. */ | |
| static unsigned int lpMaxElementLength(unsigned char *lp, int step) { | |
| unsigned int maxlen = 0, slen; | |
| long long lval; | |
| unsigned char *p = lpFirst(lp); | |
| int idx = 0; | |
| while (p) { | |
| if (step != 2 || (idx % 2 == 0)) { | |
| if (lpGetValue(p, &slen, &lval) != NULL) { | |
| if (slen > maxlen) maxlen = slen; | |
| } | |
| } | |
| p = lpNext(lp, p); | |
| idx++; | |
| } | |
| return maxlen; | |
| /* Return the maximum byte length of the entries in a listpack, measured as | |
| * the length of their string form so that integer-encoded entries count the | |
| * same as they do at runtime. step=1 inspects every entry (sets, hashes); | |
| * step=2 skips every second entry (zsets: skip scores). | |
| * Used by the RDB load path to enforce *-max-listpack-value symmetrically | |
| * with the runtime conversion. */ | |
| static unsigned int lpMaxElementLength(unsigned char *lp, int step) { | |
| unsigned char intbuf[LP_INTBUF_SIZE]; | |
| unsigned int maxlen = 0; | |
| int64_t slen; | |
| unsigned char *p = lpFirst(lp); | |
| int idx = 0; | |
| while (p) { | |
| if (step != 2 || (idx % 2 == 0)) { | |
| lpGet(p, &slen, intbuf); | |
| if ((unsigned int)slen > maxlen) maxlen = (unsigned int)slen; | |
| } | |
| p = lpNext(lp, p); | |
| idx++; | |
| } | |
| return maxlen; | |
| } |
|
|
||
| if (hashTypeLength(o) > server.hash_max_listpack_entries) hashTypeConvert(o, OBJ_ENCODING_HASHTABLE); | ||
| /* Apply both the entries and the value threshold, symmetric with | ||
| * the runtime conversion. */ |
There was a problem hiding this comment.
The comment that introduces this whole switch (src/rdb.c:2393-2395) still states the opposite rule: "Note that we only check the length and not max element size as this is an O(N) scan. Eventually everything will get converted." Five branches under it now do check max element size, so that paragraph is stale and its justification no longer describes the code — a reader landing on the switch will take it as the convention and re-open this question. Update it in the same commit.
| r config set hash-max-listpack-entries 512 | ||
| r config set hash-max-listpack-value 256 | ||
| r del myhash | ||
| r hset myhash f [string repeat a 200] |
There was a problem hiding this comment.
Nothing pins the pre-reload encoding, so if a future change makes this HSET leave the hash as a hashtable (the enclosing file already leaks hash-max-listpack-entries 2 from the test at line 903), the assertion after DEBUG RELOAD passes without the load path ever converting anything. Same shape in the set and zset copies.
| r hset myhash f [string repeat a 200] | |
| r hset myhash f [string repeat a 200] | |
| assert_encoding listpack myhash |
1 participant
Closes #4203
The listpack-encoded hash/set/zset load paths in rdb.c only checked the entries threshold (O(1) via lpLength) and skipped the value-size threshold when deciding whether to convert to hashtable/skiplist. The runtime insertion path applies both (t_hash.c, t_set.c, t_zset.c), so a key loaded from RDB could end up listpack-encoded even though its current config says it should be hashtable -- but only if the offending dimension was value size, not count. Lowering hash-max-listpack-value (or the set/zset equivalents) and reloading a snapshot produced under a larger threshold left oversized listpacks in place. Issue #4203.
Add lpMaxElementLength(lp, step) which scans a listpack once and returns the maximum byte length of its string entries; integer-encoded entries are ignored. step=1 inspects every entry (sets; hashes, where both field and value matter); step=2 skips every second entry (zsets, where only the element length is checked at runtime, scores are numbers). Wire it into the three listpack load branches alongside the existing entries check, so the load path mirrors the runtime conversion in all dimensions.
The scan is O(n) in the listpack length, same order as the existing RDB_TYPE_HASH_ZIPLIST branch (rdb.c:2392) which already does this for the legacy hash format, and as the field-by-field RDB_TYPE_HASH branch. The entries check short-circuits first, so the scan only runs when the entries threshold alone does not already force a conversion.
Add regression tests in tests/unit/type/{hash,set,zset}.tcl that write an oversized value under a permissive threshold, tighten the threshold, reload through DEBUG RELOAD, and assert the encoding flips. The three tests fail before this change and pass after.