Support Valkey 9.1 rollback compatibility with Redis 6.0#4216
Conversation
…key 7.2/8.0 Signed-off-by: Satheesha Chattenahalli Hanume Gowda <satheesha@apple.com>
Extend the Redis 6.0 compatibility loader introduced by valkey-io#2549 to read Valkey RDB v80 data that can be represented safely. Consume newer stream and slot metadata, preserve stream entries, groups, and pending-entry ownership, and reject hash-field expiration, function libraries, and active slot migration state instead of silently dropping semantics. Add Valkey 9.1 fixtures and integration coverage. Signed-off-by: yusheng.chen <yusheng.chen@uber.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
No need to merge/approve, it is to provide rollback option from valkey 9.1 to redis 6.0 |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| rdbver = (footer[1] << 8) | footer[0]; | ||
| if (rdbver > RDB_VERSION) return C_ERR; | ||
| if ((rdbver >= RDB_FOREIGN_VERSION_MIN && rdbver <= RDB_FOREIGN_VERSION_MAX) || | ||
| (rdbver > RDB_VERSION && server.rdb_version_check == RDB_VERSION_CHECK_STRICT)) { |
There was a problem hiding this comment.
Relaxing the footer check here only updates the first half of RESTORE. The decode path still goes through rdbLoadObjectType(&payload) / rdbLoadObject(type, &payload, key->ptr) in src/cluster.c:5080-5082, and rdbLoadObject() still rejects the new encoded object types with Unknown RDB encoding type in src/rdb.c:2031-2033. In relaxed mode that means future-version payloads only work for object types 6.0 already knew; listpack-backed hash/zset/set/stream dumps will still fail with Bad data format.
Thread the validated footer version into restoreCommand() and call rdbLoadObjectCompat() there, the same way rdbLoadRio() does.
There was a problem hiding this comment.
Fixed in 6e5d24f.
RESTORE now captures the validated footer RDB version and routes object decoding through rdbLoadObjectCompat(). The regression test restores authentic Valkey 9.1 hash, zset, list, set, and stream DUMP payloads in relaxed mode.
[addressed by agent]
|
|
||
| case RDB_TYPE_SET_LISTPACK: | ||
| /* Set with listpack encoding - convert to regular set */ | ||
| o->type = OBJ_SET; |
There was a problem hiding this comment.
o->ptr is still the temporary ziplist here, but o->type / o->encoding have already been changed to OBJ_SET / OBJ_ENCODING_HT. Any decrRefCount(o) before o->ptr = set_dict—for example the duplicate-element error path below—will therefore run freeSetObject() and call dictRelease() on a ziplist pointer (src/object.c:295-299), turning the corrupt-input path into a crash.
Keep the object in its original string/ziplist state until the dict is fully built, or assign o->ptr = set_dict before any path that can decref it.
There was a problem hiding this comment.
Fixed in 6e5d24f.
Set conversion now installs the destination dictionary on the object before any error path can decrement it, while retaining the source ziplist separately until conversion completes. A duplicate-element payload now returns Bad data format and the server remains alive.
[addressed by agent]
| * ziplist and storing it at 'zl'. | ||
| * The function is safe to call on non-validated listpacks, it returns 0 | ||
| * when encounter an integrity validation issue. */ | ||
| int quicklistConvertAndValidateIntegrity(unsigned char *lp, unsigned char **zl) { |
There was a problem hiding this comment.
quicklistConvertAndValidateIntegrity() never reports a validation failure: src/rdb_downgrade_compat.c:968-974 just walks lpFirst() / lpNext() and unconditionally returns 1, so the failure branch at the call site is dead code. That matters here because the listpack helpers are documented as assuming already-valid input (src/listpack.c:487-490), so corrupt packed quicklist nodes will still be accepted or can walk invalid memory during conversion instead of being rejected.
This helper needs a real listpack validity check and should return 0 when that validation fails.
There was a problem hiding this comment.
Fixed in 6e5d24f.
Quicklist conversion now receives the serialized node length and runs a bounded deep listpack integrity validation before invoking conversion callbacks. A corrupt listpack header is rejected with Bad data format rather than being traversed.
[addressed by agent]
Use the compatibility object loader for relaxed RESTORE payloads, validate listpacks before quicklist conversion, and make set conversion cleanup safe on duplicate elements. Add RESTORE coverage for Valkey 9.1 listpack encodings and corrupt-input regressions. Signed-off-by: yusheng.chen <yusheng.chen@uber.com>
Valkey rewrites relative SET expirations to SET PXAT before propagation. Redis 6.0 rejects PXAT and EXAT during parsing, which can leave replicas permanently divergent. Backport EXAT and PXAT parsing with absolute expiration handling and overflow checks. Add direct command, AOF reload, and replication downgrade compatibility coverage. Signed-off-by: yusheng.chen <yusheng.chen@uber.com>
1 participant
Summary
VALKEY080).SLOT_INFOwhile preserving RDB alignment.SET ... EXAT/PXATon the Redis 6.0 downgrade target. Valkey rewrites relativeSETexpirations andSETEX/PSETEXto absoluteSET ... PXATbefore propagation; rejecting PXAT causes permanent replica divergence.This PR includes the compatibility commit from #2549 because that PR was not merged and provides the Redis 7.0/7.2 and Valkey 7.2/8.0 compatibility foundation.
Test plan
make -j4— passed.make test— all 61 test units passed.SET EX,SET PX,SETEX, andPSETEXpropagated successfully as absolute expirations; values and TTLs matched, the replication link remained up, and no unexpected error replies were recorded (6 checks passed, 0 failed).git diff --check— passed.