Skip to content

Add streaming compression for replication full sync - #20

Open
roshkhatri wants to merge 30 commits into
replication-streaming-compression-prfrom
repl-streaming-comression-fullsync-pr
Open

Add streaming compression for replication full sync#20
roshkhatri wants to merge 30 commits into
replication-streaming-compression-prfrom
repl-streaming-comression-fullsync-pr

Conversation

@roshkhatri

Copy link
Copy Markdown
Owner

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-compression config (no | lz4-stream); default off. Stacked on replication-streaming-compression-pr (the incremental-stream compression work).

Negotiation and the cohort rule

A replica advertises REPLCONF capa compression (REPLICA_CAPA_COMPRESSION) when repl-compression is enabled; the primary compresses only when it also has repl-compression enabled. 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 existing skip_rdb_checksum cohort-AND.

Disk-based

The cohort AND is computed in startBgsaveForReplication (the generic rdbSaveBackground is not replication-aware) and passed via RDBFLAGS_REPL_COMPRESSED_SYNC; rdbSaveInternal applies the repl-compression gate, decoupled from the rdbcompression persistence setting.

Diskless

The cohort AND is computed inside rdbSaveToReplicasSockets alongside skip_rdb_checksum. Only the RDB body is wrapped as a VCS frame; the $EOF framing stays plaintext and the frame's own checksum replaces the RDB CRC64. The replica decompresses inline when it loads from the socket (repl-diskless-load enabled), or via the existing rdbLoad auto-detect when it receives the stream to disk first (the default, and dual-channel). The dual-channel handshake now advertises the compression capability.

streamReaderValidateFrameEnd is 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 ONE teardown and resync, and the AOF-base BGREWRITEAOF fallback (a compression-capable replica's compressed disk sync RDB cannot be reused as the AOF base). valkey.conf documents the expanded repl-compression coverage.

Follow-up

A src/unit gtest for the streamReaderValidateFrameEnd split is tracked as a follow-up (currently covered by the integration tests).

@roshkhatri
roshkhatri force-pushed the repl-streaming-comression-fullsync-pr branch from 25ab585 to acabef9 Compare June 24, 2026 23:49
@sarthakaggarwal97

Copy link
Copy Markdown

tests are failing :P

@roshkhatri
roshkhatri force-pushed the repl-streaming-comression-fullsync-pr branch 6 times, most recently from efc525d to ef02d03 Compare June 30, 2026 23:09
@roshkhatri
roshkhatri force-pushed the replication-streaming-compression-pr branch from 850bd66 to dbe7581 Compare July 30, 2026 08:31
@roshkhatri
roshkhatri force-pushed the replication-streaming-compression-pr branch 5 times, most recently from bd80272 to fc58806 Compare August 4, 2026 02:13
@roshkhatri
roshkhatri force-pushed the repl-streaming-comression-fullsync-pr branch from ef02d03 to f96bf89 Compare August 5, 2026 07:57
@roshkhatri
roshkhatri force-pushed the replication-streaming-compression-pr branch from fc58806 to 1e7389a Compare August 5, 2026 07:58
@roshkhatri
roshkhatri force-pushed the repl-streaming-comression-fullsync-pr branch 8 times, most recently from 186d69c to 95621f7 Compare August 10, 2026 18:18
tjade273 and others added 4 commits August 11, 2026 18:18
## 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>
moko-poi and others added 2 commits August 12, 2026 15:24
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
roshkhatri force-pushed the repl-streaming-comression-fullsync-pr branch from 48bdd10 to 8a5389c Compare August 12, 2026 22:34
latent-9 and others added 3 commits August 12, 2026 16:46
…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
roshkhatri force-pushed the repl-streaming-comression-fullsync-pr branch from 8a5389c to c274d2d Compare August 13, 2026 16:54
roshkhatri and others added 3 commits August 13, 2026 10:03
…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
roshkhatri force-pushed the replication-streaming-compression-pr branch from 1e7389a to e36f2af Compare August 13, 2026 18:26
@roshkhatri
roshkhatri force-pushed the repl-streaming-comression-fullsync-pr branch from c274d2d to fb3c3cc Compare August 13, 2026 18:28
@roshkhatri
roshkhatri force-pushed the replication-streaming-compression-pr branch from e36f2af to 9814a6d Compare August 13, 2026 19:21
…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
roshkhatri force-pushed the replication-streaming-compression-pr branch from 9814a6d to a7a8cba Compare August 13, 2026 20:21
quanyeyang and others added 6 commits August 13, 2026 22:36
…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
roshkhatri force-pushed the repl-streaming-comression-fullsync-pr branch 7 times, most recently from 67ed2b5 to df8d1f6 Compare August 15, 2026 00:24
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
@roshkhatri
roshkhatri force-pushed the repl-streaming-comression-fullsync-pr branch from df8d1f6 to 68f4b3a Compare August 15, 2026 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.