Repository navigation
Streaming Compression support for RDB - #3531
Conversation
c77cc98 to
616d763
Compare
Codecov Reportâ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #3531 +/- ##
============================================
+ Coverage 78.51% 78.75% +0.23%
============================================
Files 166 170 +4
Lines 88381 89782 +1401
============================================
+ Hits 69390 70705 +1315
- Misses 18991 19077 +86
ð New features to boost your workflow:
|
sarthakaggarwal97
changed the title
sarthakaggarwal97
changed the title
a241fd7 to
6af049a
Compare
â Provenance Check AlertPotential code similarities detected with upstream repository.
This check was performed automatically by the Provenance Guard Action. |
xdk-amz
left a comment
xdk-amz
left a comment
There was a problem hiding this comment.
Overall looks good but recommend separating RDB checksum concerns from compression checksum concerns and letting operators decide what combination they want instead of mixing the handling between the two modules. I would like to see some exploration on the lz4 configs I commented on as well as some defensive fixes to potentially unsafe code + other nits.
|
Benchmarks with using RDB checksum as is over logical RDB Stream: LZ4@0 Save (ms): CRC over logical stream vs LZ4 Block Checksum only over 1M Keys:
LZ4@0 Load (ms):
|
b9a5055 to
750a667
Compare
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ð WalkthroughWalkthroughAdds streaming LZ4 compression for RDB files, including vendored codec sources, VCS framing, RIO integration, save/load and checker support, configuration changes, build wiring, and unit/integration tests for normal and corrupted streams. ChangesStreaming LZ4 RDB support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RDBSave
participant rio
participant streamWriter
participant LZ4
participant streamReader
participant RDBLoad
RDBSave->>rio: write RDB bytes
rio->>streamWriter: transform payload
streamWriter->>LZ4: emit framed LZ4 data
streamReader->>LZ4: decode framed data
streamReader->>rio: provide decoded bytes
rio->>RDBLoad: load RDB data
ðĨ Pre-merge checks | â 4 | â 1â Failed checks (1 warning)
â Passed checks (4 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: 2
Caution
Some comments are outside the diff and canât be posted inline due to platform limitations.
â ïļ Outside diff range comments (2)
src/aof.c (1)
1009-1054:â ïļ Potential issue | ð Major | ⥠Quick winPlease request
@core-teamarchitectural review for thisaof.cbehavior change.This changes AOF/RDB sync-restart behavior (including fallback policy), which should get explicit core-team architectural sign-off before merge.
As per coding guidelines, "Request
@core-teamarchitectural review for changes to cluster*.c, replication.c, rdb.c, or aof.c".ðĪ Prompt for 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. In `@src/aof.c` around lines 1009 - 1054, The change in aof.c modifies AOF/RDB sync-restart behavior (in functions restartAOFWithSyncRdb and rdbFileUsesStreamingCompression) and the fallback policy when a streamed-compressed RDB is detected; request an explicit architectural review from `@core-team` before merging by adding a review request or TODO in the PR and code comments referencing restartAOFWithSyncRdb and rdbFileUsesStreamingCompression so core-team can approve the behavioral change, and ensure the PR description summarizes the new fallback logic (fall back to BGREWRITEAOF when streaming compression is detected) for their sign-off.src/rdb.c (1)
3689-3695:â ïļ Potential issue | ðĄ Minor | ⥠Quick winDifferentiate VKCS loads from checksum-disabled RDBs in this notice.
RIO_FLAG_SKIP_RDB_CHECKSUMnow also covers the streaming-compressed path, so this branch will log that checksums were disabled even when integrity is coming from the codec layer. That makes load diagnostics misleading.Suggested fix
- if (rdb->flags & RIO_FLAG_SKIP_RDB_CHECKSUM) { - serverLog(LL_NOTICE, "RDB file was saved with checksum disabled: skipped checksum for this transfer"); + if ((rdb->flags & RIO_FLAG_STREAMING_COMPRESSION) && + (rdb->flags & RIO_FLAG_SKIP_RDB_CHECKSUM)) { + serverLog(LL_NOTICE, + "Skipping logical RDB checksum for streaming-compressed input"); + } else if (rdb->flags & RIO_FLAG_SKIP_RDB_CHECKSUM) { + serverLog(LL_NOTICE, + "RDB file was saved with checksum disabled: " + "skipped checksum for this transfer"); } else if (cksum == 0) {ðĪ Prompt for 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. In `@src/rdb.c` around lines 3689 - 3695, The notice incorrectly attributes checksum skipping to RIO_FLAG_SKIP_RDB_CHECKSUM even for streaming-compressed (VKCS) loads; update the branch in rdb.c (where server.rdb_checksum, server.skip_checksum_validation, memrev64ifbe, rdb->flags and RIO_FLAG_SKIP_RDB_CHECKSUM are handled) to detect when the input is coming from a streaming-compressed/codec-backed path (e.g., check the RIO or codec metadata on rdb to see if a VKCS/streaming codec provided integrity) and emit a distinct serverLog message for that case versus a true checksum-disabled RDB; keep the existing behavior for cksum==0 and cksum!=expected, but split the RIO_FLAG_SKIP_RDB_CHECKSUM branch into two paths (codec-backed integrity vs. checksum-disabled file) with appropriate, different log text.
ð§đ Nitpick comments (3)
src/compression_lz4.h (1)
12-28: ⥠Quick winAdd API contract comments for exported LZ4 entry points.
These declarations are public-facing but currently undocumented at function level (ownership, error model, buffer/input-consumed guarantees). Please add concise contract comments to prevent misuse.
As per coding guidelines "Document why code exists, not just what it does; document all functions in C code".
ðĪ Prompt for 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. In `@src/compression_lz4.h` around lines 12 - 28, Add concise API contract comments above each exported LZ4 function declaration (compressionLz4CompressorInit, compressionLz4CompressorDestroy, compressionLz4DecompressorInit, compressionLz4DecompressorDestroy, compressionLz4OutputBound, compressionLz4CompressFeed, compressionLz4DecompressFeed) that describe ownership semantics (who allocates/frees streamCompressor/streamDecompressor and buffer pointers), the error model and possible return values (success codes vs negative error codes), guarantees about how much output will be produced or bounded (for compressionLz4OutputBound and CompressFeed), and for decompression functions explicitly state how input_consumed is set and when callers must retry vs drop input; keep each comment short, consistent, and focused on why the function contract exists (caller responsibilities, lifetime, and failure behavior).deps/lz4/Makefile (1)
26-28: ⥠Quick winDeclare
cleanas phony.If a file named
cleanappears in this directory,make cleancan silently no-op. Add.PHONY: cleanfor reliable cleanup.ðĪ Prompt for 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. In `@deps/lz4/Makefile` around lines 26 - 28, The Makefile target "clean" should be declared phony to avoid being skipped if a file named "clean" exists; update the Makefile by adding a .PHONY declaration for the clean target (e.g., add ".PHONY: clean") so that the clean rule (which removes *.o and *.a) always runs.src/rdb.c (1)
3150-3197: ⥠Quick winMake the new
rdbInputStream*helpers file-local.These helpers are only referenced inside
src/rdb.c, so exporting them widens the symbol surface for no gain.Suggested fix
-void rdbInputStreamInit(rdbInputStream *input, rio *raw_rio) { +static void rdbInputStreamInit(rdbInputStream *input, rio *raw_rio) { @@ -decompressRioInitResult rdbInputStreamPrepare(rdbInputStream *input) { +static decompressRioInitResult rdbInputStreamPrepare(rdbInputStream *input) { @@ -void rdbInputStreamDestroy(rdbInputStream *input) { +static void rdbInputStreamDestroy(rdbInputStream *input) { @@ -int rdbInputStreamValidateEnd(rdbInputStream *input) { +static int rdbInputStreamValidateEnd(rdbInputStream *input) { @@ -bool rdbRioHasCorruptCompressedInput(const rio *rdb) { +static bool rdbRioHasCorruptCompressedInput(const rio *rdb) {As per coding guidelines, "Use static keyword for file-local functions in C code"
ðĪ Prompt for 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. In `@src/rdb.c` around lines 3150 - 3197, Mark the helper functions as file-local by adding the static storage specifier to their definitions: rdbInputStreamInit, rdbInputStreamPrepare, rdbInputStreamDestroy, rdbInputStreamValidateEnd, and rdbRioHasCorruptCompressedInput. Leave the rdbInputStream type and any external declarations untouched; only change the function declarations/definitions to static so these helpers are not exported from the object file.
ðĪ 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 `@deps/lz4/lz4frame.c`:
- Around line 1619-1621: LZ4F_decompress performs pointer arithmetic on
srcBuffer without checking for NULL; add a guard at the start of LZ4F_decompress
to handle srcBuffer == NULL (or *srcSizePtr == 0) before computing
srcStart/srcEnd/srcPtr so you never do NULL + offset arithmetic; if srcBuffer is
NULL or *srcSizePtr == 0, initialize srcStart/srcEnd/srcPtr to a safe
non-dereferenced value (or skip computing srcEnd) and ensure subsequent logic
(e.g., the frame parsing path exercised by LZ4F_getFrameInfo calling
LZ4F_decompress with NULL/0) treats this as an empty input rather than
performing pointer arithmetic or dereferencing.
In `@src/compression_rio.c`:
- Around line 89-100: compressRioFlush and compressRioFinish currently mark the
stream writer errored but do not set the rio-level write error flag, so update
both functions (compressRioFlush and compressRioFinish) to set
RIO_FLAG_WRITE_ERROR on the rio when any flush/finalize failure occurs;
specifically, when streamWriterIsErrored(cr->writer) is detected, when
streamWriterFlush(cr->writer) != 0, and when
cr->inner->flush/cr->inner->finalize returns 0, set the rio flag (e.g., r->flags
|= RIO_FLAG_WRITE_ERROR or call the existing helper to mark rio write error) in
addition to calling streamWriterSetError(cr->writer) so callers relying on the
rio flag see the error.
---
Outside diff comments:
In `@src/aof.c`:
- Around line 1009-1054: The change in aof.c modifies AOF/RDB sync-restart
behavior (in functions restartAOFWithSyncRdb and
rdbFileUsesStreamingCompression) and the fallback policy when a
streamed-compressed RDB is detected; request an explicit architectural review
from `@core-team` before merging by adding a review request or TODO in the PR and
code comments referencing restartAOFWithSyncRdb and
rdbFileUsesStreamingCompression so core-team can approve the behavioral change,
and ensure the PR description summarizes the new fallback logic (fall back to
BGREWRITEAOF when streaming compression is detected) for their sign-off.
In `@src/rdb.c`:
- Around line 3689-3695: The notice incorrectly attributes checksum skipping to
RIO_FLAG_SKIP_RDB_CHECKSUM even for streaming-compressed (VKCS) loads; update
the branch in rdb.c (where server.rdb_checksum, server.skip_checksum_validation,
memrev64ifbe, rdb->flags and RIO_FLAG_SKIP_RDB_CHECKSUM are handled) to detect
when the input is coming from a streaming-compressed/codec-backed path (e.g.,
check the RIO or codec metadata on rdb to see if a VKCS/streaming codec provided
integrity) and emit a distinct serverLog message for that case versus a true
checksum-disabled RDB; keep the existing behavior for cksum==0 and
cksum!=expected, but split the RIO_FLAG_SKIP_RDB_CHECKSUM branch into two paths
(codec-backed integrity vs. checksum-disabled file) with appropriate, different
log text.
---
Nitpick comments:
In `@deps/lz4/Makefile`:
- Around line 26-28: The Makefile target "clean" should be declared phony to
avoid being skipped if a file named "clean" exists; update the Makefile by
adding a .PHONY declaration for the clean target (e.g., add ".PHONY: clean") so
that the clean rule (which removes *.o and *.a) always runs.
In `@src/compression_lz4.h`:
- Around line 12-28: Add concise API contract comments above each exported LZ4
function declaration (compressionLz4CompressorInit,
compressionLz4CompressorDestroy, compressionLz4DecompressorInit,
compressionLz4DecompressorDestroy, compressionLz4OutputBound,
compressionLz4CompressFeed, compressionLz4DecompressFeed) that describe
ownership semantics (who allocates/frees streamCompressor/streamDecompressor and
buffer pointers), the error model and possible return values (success codes vs
negative error codes), guarantees about how much output will be produced or
bounded (for compressionLz4OutputBound and CompressFeed), and for decompression
functions explicitly state how input_consumed is set and when callers must retry
vs drop input; keep each comment short, consistent, and focused on why the
function contract exists (caller responsibilities, lifetime, and failure
behavior).
In `@src/rdb.c`:
- Around line 3150-3197: Mark the helper functions as file-local by adding the
static storage specifier to their definitions: rdbInputStreamInit,
rdbInputStreamPrepare, rdbInputStreamDestroy, rdbInputStreamValidateEnd, and
rdbRioHasCorruptCompressedInput. Leave the rdbInputStream type and any external
declarations untouched; only change the function declarations/definitions to
static so these helpers are not exported from the object file.
ðŠ Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a47a8b43-59a8-4016-a872-af1fde59642c
ð Files selected for processing (40)
cmake/Modules/SourceFiles.cmakecmake/Modules/ValkeySetup.cmakedeps/CMakeLists.txtdeps/Makefiledeps/lz4/CMakeLists.txtdeps/lz4/LICENSEdeps/lz4/Makefiledeps/lz4/lz4.cdeps/lz4/lz4.hdeps/lz4/lz4frame.cdeps/lz4/lz4frame.hdeps/lz4/lz4hc.cdeps/lz4/lz4hc.hdeps/lz4/xxhash.cdeps/lz4/xxhash.hsrc/CMakeLists.txtsrc/Makefilesrc/aof.csrc/compression.csrc/compression.hsrc/compression_lz4.csrc/compression_lz4.hsrc/compression_rio.csrc/compression_rio.hsrc/compression_stream.csrc/compression_stream.hsrc/config.csrc/rdb.csrc/rdb.hsrc/rio.csrc/rio.hsrc/server.hsrc/unit/CMakeLists.txtsrc/unit/Makefilesrc/unit/test_compression.cppsrc/valkey-check-rdb.ctests/integration/rdb-compression.tcltests/integration/replication-aof-sync.tcltests/integration/valkey-check-rdb.tclvalkey.conf
2d6bfae to
b494e19
Compare
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 `@deps/lz4/Makefile`:
- Around line 26-28: The Makefile's clean target (target name "clean") should be
declared phony to avoid being skipped if a file named "clean" exists; add a
.PHONY declaration for the clean target (e.g., add a line declaring .PHONY:
clean) near the top or before the clean target so make always runs the clean
recipe (rm -f *.o and rm -f *.a).
ðŠ Autofix (Beta)
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: ebf9c0e3-08ee-4c87-a906-6a8e31eb4a87
ð Files selected for processing (40)
cmake/Modules/SourceFiles.cmakecmake/Modules/ValkeySetup.cmakedeps/CMakeLists.txtdeps/Makefiledeps/lz4/CMakeLists.txtdeps/lz4/LICENSEdeps/lz4/Makefiledeps/lz4/lz4.cdeps/lz4/lz4.hdeps/lz4/lz4frame.cdeps/lz4/lz4frame.hdeps/lz4/lz4hc.cdeps/lz4/lz4hc.hdeps/lz4/xxhash.cdeps/lz4/xxhash.hsrc/CMakeLists.txtsrc/Makefilesrc/aof.csrc/compression.csrc/compression.hsrc/compression_lz4.csrc/compression_lz4.hsrc/compression_rio.csrc/compression_rio.hsrc/compression_stream.csrc/compression_stream.hsrc/config.csrc/rdb.csrc/rdb.hsrc/rio.csrc/rio.hsrc/server.hsrc/unit/CMakeLists.txtsrc/unit/Makefilesrc/unit/test_compression.cppsrc/valkey-check-rdb.ctests/integration/rdb-compression.tcltests/integration/replication-aof-sync.tcltests/integration/valkey-check-rdb.tclvalkey.conf
â Files skipped from review due to trivial changes (2)
- cmake/Modules/ValkeySetup.cmake
- deps/lz4/LICENSE
ð§ Files skipped from review as they are similar to previous changes (33)
- src/CMakeLists.txt
- deps/CMakeLists.txt
- src/unit/CMakeLists.txt
- tests/integration/valkey-check-rdb.tcl
- valkey.conf
- cmake/Modules/SourceFiles.cmake
- src/config.c
- src/compression_rio.h
- src/rio.h
- tests/integration/replication-aof-sync.tcl
- deps/lz4/CMakeLists.txt
- src/aof.c
- src/server.h
- src/rdb.h
- deps/Makefile
- src/compression_lz4.c
- src/Makefile
- src/compression_lz4.h
- src/compression.h
- src/unit/Makefile
- src/compression_rio.c
- src/compression_stream.h
- deps/lz4/xxhash.c
- deps/lz4/xxhash.h
- tests/integration/rdb-compression.tcl
- src/rdb.c
- src/valkey-check-rdb.c
- deps/lz4/lz4frame.h
- src/compression.c
- src/compression_stream.c
- deps/lz4/lz4hc.c
- deps/lz4/lz4.h
- deps/lz4/lz4hc.h
There was a problem hiding this comment.
ð§đ Nitpick comments (2)
src/compression.h (1)
47-58: ⥠Quick winDocument public function contracts consistently.
The public declarations here are missing per-function API docs (ownership, nullability, error behavior). Please add short contract comments for each declaration to keep this interface self-describing and safe to use.
Proposed doc-style pattern
+/* Returns non-zero if the algorithm has streaming codec support. */ bool compressionAlgoSupportsStreaming(compressionAlgo algo); + +/* Returns a stable, human-readable algorithm name for logs/config output. */ const char *compressionAlgoName(compressionAlgo algo); +/* Initializes compressor state for the selected algorithm and level. + * Returns 0 on success, -1 on failure. */ int streamCompressorInit(streamCompressor *sc, compressionAlgo algo, int level); +/* Releases compressor resources. Safe to call after failed init. */ void streamCompressorDestroy(streamCompressor *sc); +/* Initializes decompressor state for the selected algorithm. + * Returns 0 on success, -1 on failure. */ int streamDecompressorInit(streamDecompressor *sd, compressionAlgo algo); +/* Releases decompressor resources. */ void streamDecompressorDestroy(streamDecompressor *sd);As per coding guidelines, "Document why code exists, not just what it does; document all functions in C code."
ðĪ Prompt for 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. In `@src/compression.h` around lines 47 - 58, Add short per-function API contract comments above each public declaration (compressionAlgoSupportsStreaming, compressionAlgoName, streamCompressorInit, streamCompressorDestroy, streamDecompressorInit, streamDecompressorDestroy, streamDecompressorFrameDone, streamDecompressorInputHint) describing purpose, ownership/ownership transfer (who allocates/frees streamCompressor/streamDecompressor), nullability of pointer parameters and return values, error semantics and return codes (e.g. 0 on success, negative on error), thread-safety expectations, and any lifetime guarantees (when the returned const char * from compressionAlgoName is valid). Use the existing project doc-style pattern for C comments so each function is self-describing and follows the codebase guideline of âwhyâ and âcontractâ information.src/compression_lz4.h (1)
12-30: ⥠Quick winAdd function-level docs for the LZ4 public API surface.
This header exposes public entry points but none are documented. Add concise contract comments for init/destroy/feed/output-bound so callers know expected invariants and error semantics.
Suggested documentation skeleton
+/* Initializes LZ4 compressor state in `sc`. Returns 0 on success, -1 on error. */ int compressionLz4CompressorInit(streamCompressor *sc); +/* Releases LZ4 compressor resources. */ void compressionLz4CompressorDestroy(streamCompressor *sc); +/* Initializes LZ4 decompressor state in `sd`. Returns 0 on success, -1 on error. */ int compressionLz4DecompressorInit(streamDecompressor *sd); +/* Releases LZ4 decompressor resources. */ void compressionLz4DecompressorDestroy(streamDecompressor *sd); +/* Conservative upper bound for compressed output size for `input_len`. */ size_t compressionLz4OutputBound(size_t input_len);As per coding guidelines, "Document why code exists, not just what it does; document all functions in C code."
ðĪ Prompt for 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. In `@src/compression_lz4.h` around lines 12 - 30, Add concise function-level comments above each public symbol in src/compression_lz4.h (compressionLz4CompressorInit, compressionLz4CompressorDestroy, compressionLz4DecompressorInit, compressionLz4DecompressorDestroy, compressionLz4OutputBound, compressionLz4CompressFeed, compressionLz4DecompressFeed) describing the contract: purpose, ownership/lifetime expectations (who allocates/frees streamCompressor/streamDecompressor), required preconditions (valid pointers, output_capacity >= needed, input may be NULL for flush), thread-safety, what each parameter means (including compressFlushMode and input_consumed), precise return/error semantics (success values, what negative return codes mean, when 0 is returned), how partial consumption is reported for feed functions and when callers must call Destroy, and that compressionLz4OutputBound returns an upper bound for worst-case expansion; keep each comment short and actionable so callers know invariants and error handling.
ðĪ 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.
Nitpick comments:
In `@src/compression_lz4.h`:
- Around line 12-30: Add concise function-level comments above each public
symbol in src/compression_lz4.h (compressionLz4CompressorInit,
compressionLz4CompressorDestroy, compressionLz4DecompressorInit,
compressionLz4DecompressorDestroy, compressionLz4OutputBound,
compressionLz4CompressFeed, compressionLz4DecompressFeed) describing the
contract: purpose, ownership/lifetime expectations (who allocates/frees
streamCompressor/streamDecompressor), required preconditions (valid pointers,
output_capacity >= needed, input may be NULL for flush), thread-safety, what
each parameter means (including compressFlushMode and input_consumed), precise
return/error semantics (success values, what negative return codes mean, when 0
is returned), how partial consumption is reported for feed functions and when
callers must call Destroy, and that compressionLz4OutputBound returns an upper
bound for worst-case expansion; keep each comment short and actionable so
callers know invariants and error handling.
In `@src/compression.h`:
- Around line 47-58: Add short per-function API contract comments above each
public declaration (compressionAlgoSupportsStreaming, compressionAlgoName,
streamCompressorInit, streamCompressorDestroy, streamDecompressorInit,
streamDecompressorDestroy, streamDecompressorFrameDone,
streamDecompressorInputHint) describing purpose, ownership/ownership transfer
(who allocates/frees streamCompressor/streamDecompressor), nullability of
pointer parameters and return values, error semantics and return codes (e.g. 0
on success, negative on error), thread-safety expectations, and any lifetime
guarantees (when the returned const char * from compressionAlgoName is valid).
Use the existing project doc-style pattern for C comments so each function is
self-describing and follows the codebase guideline of âwhyâ and âcontractâ
information.
âđïļ Review info
âïļ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f9cc2d76-d6f8-4849-8ca9-1346437aa8c3
ð Files selected for processing (9)
src/compression.csrc/compression.hsrc/compression_lz4.csrc/compression_lz4.hsrc/compression_rio.csrc/compression_rio.hsrc/compression_stream.csrc/compression_stream.hsrc/rdb.c
ð§ Files skipped from review as they are similar to previous changes (7)
- src/compression_rio.h
- src/compression_rio.c
- src/compression_stream.h
- src/compression_lz4.c
- src/compression.c
- src/compression_stream.c
- src/rdb.c
dbf155f to
21ee8c4
Compare
roshkhatri
left a comment
roshkhatri
left a comment
There was a problem hiding this comment.
Looked a little deeper for optimization. Let me know what you think
nmvk
left a comment
nmvk
left a comment
There was a problem hiding this comment.
Looks mostly good to me. I havenât reviewed the RIO part yet. Two design questions on CRC validation over decoded content and moving VKCS after the RDB magic/version.
|
Thanks for the review folks. There are mostly two design discussions that we need to have consensus:
Also note, the current implementation is smart enough to differentiate between VKCS and Existing RDB and it's able to load in both case irrespective of configuration. We could also just provide a utility to decompress VKCS RDB to existing RDB. @valkey-io/core-team please share if you have any thoughts on this. |
hpatro
added
the
run-extra-tests
Yeah, we won't be able to depend on this. Would we be able to just change the algorithm for internal compression of data without users consent? or else to be transparent we have to introduce on the lines you suggested (maybe |
The contract we've talked about for downgrade is to allow it as long as no new features of the new version have been actively used. Even if there's an RDB bump, the old version should be able to read it with the relaxed RDB version check enabled. Opting it by changing a config is an active choice, but a changed default is not. I think we could soften this contract if we have some overlap of e.g. two major versions (so let's say 10.0 can read this format but doesn't produce it) and/or if there's a way to downgrade by changing the config before saving. Another possibility is to introduce a way to save RDB files in old versions (#3032) â that would need to include decoding lz4 values to either uncompressed or re-compress as lzf. Either way, it would be good to be able to change the defaults without being limited by downgrades forever. |
in-memory storage (compressed / not) could be decoupled from disk storage. The user would need to pay the cost of decompress and serialise to disk if the algo for list compression / rdb compression. So, I feel we could change the in-memory compression algorithm without user consent.
Yeah, the tradeoff is between maintaining lot of these config forever vs user choice. btw, thanks for filing #3032, I've some thoughts about it. will share over there. |
You mean we should decompress in-memory quicklist nodes when we write them to RDB, and they get re-compressed at the RDB stream level instead? Makes sense to me. How is that working in legacy with lzf-compressed quicklist nodes? |
ab5a94a to
e45f2d6
Compare
|
The failing tests are not due to this PR. |
Currently the nodes are stored as is, there is no explicit logic to decompress individual nodes during RDB serialisation |
Does that mean even with |
Yep. rdbcompression only dictates per string compression today. |
We can look into this independently what happens with recompression of data with lz4 stream approach over list nodes compressed with lzf. |
226a2d1 to
24e4244
Compare
|
The separation in two commits is clean. We should not squash these commits, right? Let's include the PR number in parentheses in each commit message title. (We don't get a chance to edit the commit messages when we click Rebase-merge.) |
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>
24e4244 to
9a54fba
Compare
|
Yes, the plan is to merge two commits as is. I added the PR number in parentheses. |
madolson
left a comment
madolson
left a comment
There was a problem hiding this comment.
Since I did review it, just putting my approval on it. I never ended up reviewing the final details all that closely, but all the high level decisions I'm aligned with.
roshkhatri
left a comment
roshkhatri
left a comment
There was a problem hiding this comment.
LGTM, for multiple algorithms we will need changes when we add ZSTD. Good to go
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>
|
Thanks for your hard work @sarthakaggarwal97! |
Related to #3195
Overview
Today,
rdbcompression yesapplies legacy LZF compression to individual string payloads inside an otherwise plain RDB stream. This PR adds Valkey Compressed Stream (VCS) support for on-disk RDB persistence, withlz4as the first supported whole-stream codec.Setting
rdbcompression lz4wraps the complete logical RDB in a smallVCSenvelope and compresses it as a single LZ4 frame through the existingrio. The default remainsyes, preserving the existing plain RDB format and per-string LZF behavior.During loading, the RDB path probes the input before normal parsing begins. Plain RDB input is replayed unchanged, while a valid
VCSenvelope attaches an LZ4 stream reader that transparently decodes the logical RDB. Malformed, incompatible, and truncated compressed streams are rejected.When
rdbchecksum yes, LZ4 frame checksums also detect corrupted content. As with legacy plain RDB files, bytes after the logical end of the RDB are ignored.Architecture
The stream writer or reader is attached directly to the existing
rio. Encoded I/O reaches the concrete file backend through rawriooperations, avoiding recursive stream dispatch while keeping normal RDB parsing unchanged.Key Design Decisions
rio, rather than as a separate RIO wrapper or part of object serialization.VCSadds a seven-byte envelope before the compressed payload so loaders can classify the stream before normal RDB parsing begins.lzfremains the default, preserving the existing plain RDB format and compatibility.stream_kindare validated before decoding.rdbLoad,valkey-check-rdb, and startup loading transparently support both plain andVCS-wrapped RDB files.Data Flow: Save Path
Important notes:
VCSenvelope stores magic, version, codec, a reserved byte, andstream_kind.rdbchecksum yesenables both LZ4 block and content XXH32 checksums,rdbchecksum noomits them.VCSinput.rdbchecksumcontrols CRC64 generation and validation for plain RDB files, and LZ4 checksum generation and validation for compressed RDB files.Data Flow: Load Path
The RDB parser continues to consume logical bytes through the same
rio. For compressed input, physical source progress is tracked separately from decoded output progress.This PR is intentionally scoped to on-disk RDB streaming compression and the persistence, loading, and validation paths that need to understand it. It does not add replication compression or diskless full-sync capability negotiation.
Benchmarks
Benchmarked on r7g.2xlarge (Graviton, 8 vCPUs, 61GB RAM, NVMe). All results averaged over 3 repeats.
Datasets
Notes: