Add streaming compression for replication full sync - #20
Open
roshkhatri wants to merge 30 commits into
Open
Conversation
roshkhatri
force-pushed
the
repl-streaming-comression-fullsync-pr
branch
from
June 24, 2026 23:49
25ab585 to
acabef9
Compare
|
tests are failing :P |
roshkhatri
force-pushed
the
repl-streaming-comression-fullsync-pr
branch
6 times, most recently
from
June 30, 2026 23:09
efc525d to
ef02d03
Compare
roshkhatri
force-pushed
the
replication-streaming-compression-pr
branch
from
July 30, 2026 08:31
850bd66 to
dbe7581
Compare
roshkhatri
force-pushed
the
replication-streaming-compression-pr
branch
5 times, most recently
from
August 4, 2026 02:13
bd80272 to
fc58806
Compare
roshkhatri
force-pushed
the
repl-streaming-comression-fullsync-pr
branch
from
August 5, 2026 07:57
ef02d03 to
f96bf89
Compare
roshkhatri
force-pushed
the
replication-streaming-compression-pr
branch
from
August 5, 2026 07:58
fc58806 to
1e7389a
Compare
roshkhatri
force-pushed
the
repl-streaming-comression-fullsync-pr
branch
8 times, most recently
from
August 10, 2026 18:18
186d69c to
95621f7
Compare
## Problem `clearClientConnectionState()` resets the per-connection flags (tracking, db, auth, transactions, pub/sub, name, no-touch, no-evict) but leaves `c->flag.import_source` set. That flag is only toggled by `CLIENT IMPORT-SOURCE ON|OFF`. When set it makes the connection treat logically expired keys as live (see `objectIsExpired()` / `keyIsExpiredWithDictIndex()` in `db.c` and the `POLICY_IGNORE_EXPIRE` path in `expire.c`). `RESET` is `NO_AUTH` and is commonly issued by connection pools before handing a socket to another user. Because the flag survives RESET, a pooled connection that was placed in import mode keeps import-mode expiration semantics for whoever reuses it, reading expired data a fresh connection would see as missing. ## Fix Clear `c->flag.import_source` alongside the other flags in `clearClientConnectionState()` so RESET returns the connection to normal expiration semantics. A bare `AUTH` (without a preceding RESET) still does not clear the flag, since `AUTH` does not route through `clearClientConnectionState()`. The pooling pattern that exposes this always issues RESET before reuse, so it is covered; clearing on the auth path as well would be a reasonable follow-up. Signed-off-by: Tjaden Hess <tjade273@gmail.com>
When a Lua script or a transaction produces multiple write operations,
these operations are persisted to the AOF inside a MULTI-EXEC block.
In cases such as a machine power failure, only part of the block may be
persisted:
SET foo hello
MULTI
SET bar world
EX
With `aof-load-truncated yes`, the server previously removed only the
partial `EXEC`, leaving an incomplete `MULTI` block in the AOF:
SET foo hello
MULTI
SET bar world
New writes could then be lost after another restart.
This PR fixes the issue by truncating the AOF before `MULTI` and adds a
regression test for this case.
Signed-off-by: chzhoo <czawyx@163.com>
…key-io#4359) The sorted set skiplist is replaced with a B+ tree behind a new ordered-index abstraction. skiplist.c and skiplist.h are deleted, zskiplist and zskiplistNode are removed from server.h, and zset now holds a hashtable plus an OrderedIndex. OBJ_ENCODING_SKIPLIST becomes OBJ_ENCODING_BTREE (same value, 7), so OBJECT ENCODING on a large sorted set answers "btree" instead of "skiplist". The listpack encoding for small sorted sets is untouched. Two new layers replace it: * ordered_index.[ch] (802 lines) is the API the rest of the server speaks: insert, delete, update score, pop, seek, iterate, count and delete by score, lex and rank range, defrag, memory estimation. Every skiplist call site in t_zset.c, object.c, rdb.c, aof.c, geo.c, sort.c, db.c, debug.c, defrag.c, lazyfree.c, module.c and valkey-check-rdb.c now goes through it, so nothing outside this file knows what the backing structure is. t_zset.c loses 643 lines net. * fbtree.[ch] plus fbtree_internal.h (2,811 lines) is the tree itself. It is a general-purpose ordered collection of sds with no ZSET knowledge, depending only on sds for string operations. Score packing, lex range sentinels and positioned seeks all live in the adapter. Details worth knowing when reading it: Items are packed sds. Each entry is a single allocation holding [8-byte sortable score][element bytes], so plain sdscmp gives correct ZSET ordering. The score is a big-endian sign-flipped IEEE 754 double, with -0.0 normalized to 0.0 so equal scores map to one key. Node layout is 61-way fanout, with leaves at exactly 512 B and inner nodes at exactly 2048 B on 64-bit, enforced by static_assert so they land in jemalloc size classes. Inner nodes carry per-child subtree sizes for O(log N) rank arithmetic, and 4 bytes of discriminating "feature" per child in a cache-line-aligned row, searched with AVX2 or NEON (scalar fallback) to route a lookup without touching the child. Hashtable lookups need disambiguation. The hashtable stores packed items but is looked up with a plain sds, and hashtable has no heterogeneous lookup. Rather than allocating a temporary packed key per lookup, lookup keys are transiently marked via an sds aux bit (sdshdr8+) or a spare type value (sdshdr5), and zsetHashFunction/zsetKeyCompare check the mark to decide whether to skip the 8-byte score prefix. This is not pretty; the plan is to add real heterogeneous lookup to hashtable and delete the scheme. Delete does not merge. Empty nodes are freed and a single-child root collapses, but sibling leaves are never rebalanced, so leaves can stay sparse after heavy deletion. Background compaction is a follow-up, along with 32-bit node sizing (NODE_SIZE is tuned for 64-bit, so 32-bit nodes fit their size class but do not fill it). Defrag is rank-based. defragZsetSkiplist becomes defragZset and drops 52 lines. The cursor is a rank, resumed by O(log N) tree navigation, processing 4 full leaves (~244 items) per call. Compatibility: * No RDB or AOF format change. Saves still use RDB_TYPE_ZSET_2 and RDB_TYPE_ZSET_LISTPACK, and loading is unchanged apart from converting to OBJ_ENCODING_BTREE. * No config change and no module API change. valkeymodule.h is untouched. Module zset range iteration is reimplemented on the ordered-index iterator with the same semantics. * OBJECT ENCODING is the only user-visible reply change. * Two DEBUG outputs change: the crash report logs "Index height: N" instead of "Skiplist level: N", and DEBUG HTSTATS-KEY and DEBUG DIGEST follow the new encoding constant. Performance, measured on the branch in valkey-io#4206 and not re-run for this merge. ARM Graviton 3 (c7g.metal, 64 cores), io-threads=9, pipeline=10, 3M-member zset, 5 repetitions, 95% CI within 2% except ZRANDMEMBER (5%): Command Workload Skiplist fbtree Change ZADD Score update, all hits 205K 421K +105% ZREM 50/50 random add/remove 386K 680K +76% ZCOUNT Score-range count 720K 917K +27% ZRANK Point rank lookup 692K 825K +19% ZRANGE 100-element rank scan 174K 183K +6% ZRANDMEMBER 100 random samples 5.0K 5.1K +2% ZSCORE Point score lookup 1.44M 1.46M +2% ZRANGEBYSCORE 100-element score scan 95K 96K +1% ZPOPMIN Pop min, append new member 1.08M 1.08M 0% Mutations that reposition an element gain the most from cache locality. Range scans are bottlenecked on reply size. ZSCORE and ZRANDMEMBER are hashtable paths and do not involve the ordered index. Memory, 5M members, 20-byte elements, via jemalloc heap profiling. Per-member overhead excluding user data: Insertion pattern Skiplist fbtree Savings Sequential, dense packing 50.3 B 28.5 B -21.8 B (-43%) Random, fresh fill 50.3 B 32.0 B -18.2 B (-36%) fbtree overhead tracks leaf occupancy. The skiplist is flat at ~50.3 B regardless of insertion order. Since there is no compaction yet, delete-heavy workloads will land worse than either number here. Testing: * 302 new gtest cases: 205 in test_fbtree.cpp (tree operations, SIMD feature search, property and fuzz tests) and 97 in test_ordered_index.cpp, which exercise the interface contract and were written against the skiplist before fbtree existed. They passed unchanged against the new backend, which is the main evidence the swap is behavior preserving. * 21 new tcl tests, 18 in tests/unit/type/zset.tcl and 3 in tests/unit/sort.tcl, mostly forcing the non-listpack encoding on paths that previously only ran on small sets. * New with_config helper in tests/support/util.tcl that restores the old value even when the body fails. * assert_encoding skiplist becomes assert_encoding btree in 5 test files. * -Wframe-larger-than in src/unit/Makefile goes from 32K to 128K. The cap applies to the compiler-generated static initializer, whose frame grows with the number of gtest registrations under ASan, and test_fbtree.cpp has 205 of them. Also included beyond the three feature PRs: * valkey-io#4363, a strict-aliasing miscompilation of the fbtree iterator. fbtreeIterator is uint64_t[3] accessed through an internal struct, which is UB everywhere and got miscompiled on Alpine gcc 15.2 at -O3 -flto: the optimizer elided the seek stores, so ZRANGE and ZREVRANGE returned results as if the iterator had never been seeked. Fixed with __attribute__((may_alias)) on the internal struct. * fbtree.c included both assert.h and serverassert.h, and only produced server crash reports because serverassert.h happened to come last and reclaim the macro. assert.h is dropped and the include order is documented. * "Ba" and "Addd" added to the typos allowlist, both from fbtree test fixtures. --------- Signed-off-by: Rain Valentine <rsg000@gmail.com> Signed-off-by: Rain Valentine <rainval@amazon.com> Co-authored-by: Rain Valentine <rsg000@gmail.com>
## Problem `getKeysFromCommandWithSpecs()` (used by the ACL path and `COMMAND GETKEYS`) prefers a command's JSON key specs over its `get_keys_function` when the specs carry no VARIABLE_FLAGS. For GEORADIUS/GEORADIUSBYMEMBER the `STORE`/`STOREDIST` specs are keyword searches, and `getKeysUsingKeySpecs()` stops at the first matching keyword. The `geo.c` parser, however, is last-one-wins. So for `GEORADIUS ... STORE scratch:ok STORE protected:dst`, ACL only checks `scratch:ok`, while the command writes (or, when the source is missing, deletes) `protected:dst`, bypassing key-level write ACLs. A user with `+georadius`, read on the source pattern, and write on only one benign destination pattern can delete or overwrite a key outside their permitted patterns. ## Fix Flag the `STORE`/`STOREDIST` key specs in `georadius.json` / `georadiusbymember.json` with VARIABLE_FLAGS so `getKeysFromCommandWithSpecs()` falls through to `georadiusGetKeys`, which resolves the effective (last) destination. This mirrors how MIGRATE routes through its `getkeys_proc`. `commands.def` regenerated. `georadiusGetKeys` previously returned `flags = 0` for every key, since it was only used by cluster routing where read/write direction is irrelevant. Now that ACL uses it, set `CMD_KEY_RO | CMD_KEY_ACCESS` on the source and `CMD_KEY_OW | CMD_KEY_UPDATE` on the store key so `%R~` / `%W~` direction is enforced. Signed-off-by: Tjaden Hess <tjade273@gmail.com>
Grammatical errors are fixed in documentation Signed-off-by: takahashi shun <mokopoi44@gmail.com> Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
Fixes grammatical and spelling errors --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Co-authored-by: Sarthak Aggarwal <sarthagg@amazon.com>
roshkhatri
force-pushed
the
repl-streaming-comression-fullsync-pr
branch
from
August 12, 2026 22:34
48bdd10 to
8a5389c
Compare
…valkey-io#4368) In `generateStringArgValue`, the `argName` dispatch chain contains two identical `else if (strcmp(argName, "command") == 0)` branches. The earlier branch already handles every `argName == "command"` case, so the second one, whose body is identical, is unreachable dead code. This removes the duplicate branch; behavior is unchanged. Signed-off-by: latent-9 <296084221+latent-9@users.noreply.github.com>
Improves spelling and grammar across codebase --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Grammar corrections in comments --------- Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
roshkhatri
force-pushed
the
repl-streaming-comression-fullsync-pr
branch
from
August 13, 2026 16:54
8a5389c to
c274d2d
Compare
…ey-io#4381) Stream listpack master entries store separate live and deleted record counts, but integrity validation only verified their sum. A corrupted payload could therefore preserve the total while understating the live count, causing XLEN to disagree with the stored records and allowing XDEL to discard records not accounted for by the header. Count live and deleted records independently while validating stream listpacks and reject payloads when either count differs from its declared value. Signed-off-by: Roshan Khatri <roshanvkhatri@gmail.com>
Consolidating nonexistent spelling Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
…alkey-io#4402) `src/commands/config-info.json` and `src/commands/move.json` still carry `"since": "10.0.0"`. *This was generated by AI but verified, with love, by a human.* Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
roshkhatri
force-pushed
the
replication-streaming-compression-pr
branch
from
August 13, 2026 18:26
1e7389a to
e36f2af
Compare
roshkhatri
force-pushed
the
repl-streaming-comression-fullsync-pr
branch
from
August 13, 2026 18:28
c274d2d to
fb3c3cc
Compare
roshkhatri
force-pushed
the
replication-streaming-compression-pr
branch
from
August 13, 2026 19:21
e36f2af to
9814a6d
Compare
…load (valkey-io#4360) moduleUnregisterCleanup did not remove the module's cluster message receivers. The stale entries kept pointing at the freed ValkeyModule, so a later cluster message of that type dereferenced r->module in moduleCallClusterReceivers, causing a use-after-free. Added a MODULE UNLOAD test to verify the fix, and also added type=254 to allow us to verify the correctness of the loop logic. --------- Signed-off-by: Binbin <binloveplay1314@qq.com>
roshkhatri
force-pushed
the
replication-streaming-compression-pr
branch
from
August 13, 2026 20:21
9814a6d to
a7a8cba
Compare
…ndlers (valkey-io#4401) Follow-up of valkey-io#3611. Fixes a regression introduced by that PR. The May 27 On-Demand run on this PR (SET/GET, 96B, io-threads 2/10, pipeline 1/10) showed no significant RPS change. After `17ec23f` removed `post_read_done_postpone_mask`, `processClientIOReadsDone()` started postponing READ and returning `needs_post_read_update = 1` for every non-ACCEPTING completed read. That second phase (`lookupClientByID` + `processPendingCommandAndInputBuffer` + `connUpdateState`) is only required when `update_state` may synchronously invoke handlers. For other transports it is per-completion overhead. This matches the post-merge dashboard drop: small payloads, io-threads, **P1 worse than P10**. This PR restores the original gate without bringing `struct client` into the connection driver (the review concern that led to `17ec23f`): - `ConnectionType.sync_handlers_in_update_state` (0 by default) - Set only where `update_state` can sync-call handlers - Mask is still computed in `networking.c` from IO state - `connUpdateState()` still runs immediately, including ACCEPTING --------- Signed-off-by: quanyeyang <quanyemostima@gmail.com>
Log EXEC in commandlog. Catches cases where there's no individually slow command. --------- Signed-off-by: Michelle Lee <michellee.3104@gmail.com>
…lkey-io#4404) `unit/commandlog` fails on unstable at 917de6a: ``` *** [err]: COMMANDLOG slow - Redaction does not leak to later commands in a MULTI in tests/unit/commandlog.tcl Expected 'set foo bar' to be equal to 'exec' (context: type eval line 12 cmd {assert_equal {set foo bar} [lindex [lindex $slowlog_resp 0] 3]} proc ::test) ``` valkey-io#4267 dropped `SKIP_COMMANDLOG` from EXEC (`src/commands/exec.json:12`), so EXEC is now logged after the commands it ran, which puts it at entry 0 of the newest-first `COMMANDLOG GET`. The redaction test at `tests/unit/commandlog.tcl:212`, added by valkey-io#4323, still reads entry 0 and gets the EXEC instead of the SET. Neither PR was rebased on the other, so this only broke on merge and not in either PR's CI. The log after `MULTI; ACL SETUSER commandlog-test-user +get; SET foo bar; EXEC`: | Entry | Command | |---|---| | 0 | `exec` | | 1 | `set foo bar` | | 2 | `acl setuser (redacted) (redacted)` | Read the SET from entry 1 instead. The other tests valkey-io#4267 touched already assert entry 0 is `exec` and entry 1 is the inner command, so this matches. Also assert entries 0 and 2 rather than only the SET. The point of the test is that the ACL SETUSER redaction stops at the ACL SETUSER, and asserting entry 2 is redacted is what keeps it from passing if redaction breaks entirely. Asserting entry 0 is `exec` makes the index arithmetic fail loudly next time the entry order changes, instead of silently comparing against the wrong entry the way it just did. ## Testing `unit/commandlog` goes from 29 passed, 1 failed to 30 passed. `unit/slowlog` and `unit/multi` are unchanged at 100 passed. Reverting the `src/` hunks of 917de6a confirms that commit is the trigger. The new entry-0 assert fails, along with the two EXEC tests 917de6a added: ``` *** [err]: COMMANDLOG slow - Redaction does not leak to later commands in a MULTI in tests/unit/commandlog.tcl Expected 'exec' to be equal to 'set foo bar' (context: type eval line 13 cmd {assert_equal {exec} [lindex [lindex $slowlog_resp 0] 3]} proc ::test) *** [err]: COMMANDLOG slow - EXEC is logged alongside slow inner commands in tests/unit/commandlog.tcl Expected '1' to be equal to '2' (context: type eval line 8 cmd {assert_equal [r commandlog len slow] 2} proc ::test) *** [err]: COMMANDLOG slow - EXEC records total transaction time when inner commands are individually fast in tests/unit/commandlog.tcl Expected '0' to be equal to '1' (context: type eval line 9 cmd {assert_equal [r commandlog len slow] 1} proc ::test) ``` *This was generated by AI but verified, with love, by a human.* Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
The cluster message receiver array was sized UINT8_MAX (255), so the valid index range was 0..254. Two issues existed: - VM_RegisterClusterMessageReceiver allowed type 255, an out-of-bounds WRITE past the end of the array. - moduleCallClusterReceivers would perform an out-of-bounds READ when a cluster bus packet carried type 255 (UINT8_MAX), which can be sent by any cluster member. Grow the array to MAX_CLUSTER_MESSAGE_TYPES (UINT8_MAX + 1) so the full uint8_t type range (0..255) is covered, and use the explicit constant in both dispatch and unregister paths. type 255 is now a valid, dispatchable message type. Signed-off-by: Binbin <binloveplay1314@qq.com>
Vendor lz4, lz4hc, lz4frame and xxhash from https://github.com/lz4/lz4 (v1.10.0) under deps/lz4 with build wiring for make and cmake. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Add Valkey Compressed Stream (VCS) support for RDB persistence with LZ4 as the first whole-stream codec. The RDB is wrapped in a VCS envelope and compressed as a single stream at the rio layer. On load, the RDB path probes for the envelope and transparently decompresses when present, falling back to the plain RDB path otherwise. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
roshkhatri
force-pushed
the
repl-streaming-comression-fullsync-pr
branch
7 times, most recently
from
August 15, 2026 00:24
67ed2b5 to
df8d1f6
Compare
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
roshkhatri
force-pushed
the
repl-streaming-comression-fullsync-pr
branch
from
August 15, 2026 01:18
df8d1f6 to
68f4b3a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds streaming compression of the full-sync RDB payload for replicas that negotiate the compression capability, covering disk-based sync, diskless sync, and dual-channel sync. Builds on the streaming RDB codec (VCS/LZ4) and reuses the existing
repl-compressionconfig (no|lz4-stream); default off. Stacked onreplication-streaming-compression-pr(the incremental-stream compression work).Negotiation and the cohort rule
A replica advertises
REPLCONF capa compression(REPLICA_CAPA_COMPRESSION) whenrepl-compressionis enabled; the primary compresses only when it also hasrepl-compressionenabled. Because a single full sync (a shared disk file, or one connset/pipe) serves a whole cohort of replicas, the decision is the AND of the capability over every replica in the attaching cohort: if any replica is not capable, the payload is sent plaintext so all of them can load it. This mirrors the existingskip_rdb_checksumcohort-AND.Disk-based
The cohort AND is computed in
startBgsaveForReplication(the genericrdbSaveBackgroundis not replication-aware) and passed viaRDBFLAGS_REPL_COMPRESSED_SYNC;rdbSaveInternalapplies therepl-compressiongate, decoupled from therdbcompressionpersistence setting.Diskless
The cohort AND is computed inside
rdbSaveToReplicasSocketsalongsideskip_rdb_checksum. Only the RDB body is wrapped as a VCS frame; the$EOFframing stays plaintext and the frame's own checksum replaces the RDB CRC64. The replica decompresses inline when it loads from the socket (repl-diskless-loadenabled), or via the existingrdbLoadauto-detect when it receives the stream to disk first (the default, and dual-channel). The dual-channel handshake now advertises the compression capability.streamReaderValidateFrameEndis added so the replica can validate a closed compressed frame without consuming the caller-owned trailing EOF mark.Testing
Disk grouping (all/mixed/none capable), diskless and dual-channel sync, default disk-receive load, checksum on/off, the compressed-full-sync to compressed-incremental handoff,
REPLICAOF NO ONEteardown and resync, and the AOF-baseBGREWRITEAOFfallback (a compression-capable replica's compressed disk sync RDB cannot be reused as the AOF base).valkey.confdocuments the expandedrepl-compressioncoverage.Follow-up
A
src/unitgtest for thestreamReaderValidateFrameEndsplit is tracked as a follow-up (currently covered by the integration tests).