refactor(lz4): switch from frame format to block format #73
Conversation
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
The lz4 library v4.1.23 added frame concatenation support, which peeks ahead after reading a frame to check for another concatenated frame. This broke LTX because each page is an independent LZ4 frame with a PageHeader in between. This change adds a new PageHeaderFlagCompressedSize flag and writes a 4-byte compressed size prefix after each page header. The decoder uses this size to create an exact LimitedReader, preventing lz4 from peeking into the next page. For backward compatibility, the decoder handles both formats: - New format (flag set): reads compressed size, uses exact LimitedReader - Old format (flag=0): uses LimitedReader workaround with lz4 frame footer size Fixes #70 Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace LZ4 frame compression with block compression to eliminate ~15-23 bytes of overhead per page (magic bytes, descriptor, EndMark, content checksum). Changes: - Add PageHeaderFlagUncompressed for incompressible data - Use lz4.Compressor.CompressBlock() in encoder - Use lz4.UncompressBlock() in decoder for new format - Maintain backward compatibility with old frame format - Add validation for uncompressed data size and buffer bounds This addresses feedback from @ncruces and @benbjohnson that the LZ4 frame format adds unnecessary overhead when we already have file-level checksums. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Collaborator
|
@corylanou Can you add a test to verify that this continues to work for SQLite databases with 64KB sized pages? |
Add tests to verify LZ4 block compression works correctly with 64KB pages (SQLite's maximum page size): - Compressible test: repetitive data that compresses well - Incompressible test: random data stored uncompressed Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
benbjohnson
approved these changes
Jan 15, 2026
ltx.go
Outdated
Comment on lines
410
to
413
| // PageHeaderFlagCompressedSize indicates that a 4-byte compressed size | ||
| // field follows the page header. When set, data uses LZ4 block format | ||
| // (not frame format). | ||
| PageHeaderFlagCompressedSize = uint16(1 << 0) |
Collaborator
There was a problem hiding this comment.
This should probably be named PageHeaderFlagSize since it exists for both compressed and uncompressed data in the new format.
encoder.go
Outdated
Comment on lines
243
to
251
| // Determine what data to write based on compression result. | ||
| var writeData []byte | ||
| if n == 0 || n >= len(data) { | ||
| // Incompressible or compression didn't help - store uncompressed. | ||
| hdr.Flags |= PageHeaderFlagUncompressed | ||
| writeData = data | ||
| } else { | ||
| writeData = enc.compressBuf[:n] | ||
| } |
Collaborator
There was a problem hiding this comment.
I'm not sure it's worth having a mix of compressed and uncompressed blocks. I can't imagine a time when a SQLite page won't compress except for encrypted pages (but we will handle that without compression in the future when we implement it).
There was a problem hiding this comment.
Also, the maximum overhead for a block is 273 bytes (0.4%) for 64K pages, 32 bytes (0.8%) for 4K pages, and at most 3.5% on (rather uncommon) even smaller pages:
https://github.com/pierrec/lz4/blob/v4.1.23/internal/lz4block/block.go#L40-L42
Note that, since lz4.CompressBlockBound was used, it is an error if n == 0 is true.
Address PR review feedback: - Rename PageHeaderFlagCompressedSize to PageHeaderFlagSize since it applies to both compressed and uncompressed data - Remove PageHeaderFlagUncompressed and always compress data, as SQLite pages reliably compress and max block overhead is only 0.8% for 4K pages Co-Authored-By: Claude Opus 4.5 <[email protected]>
Collaborator
Author
|
Addressed review feedback in df2a1e2:
All tests pass. |
This was referenced Jan 15, 2026
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.
Summary
PageHeaderFlagCompressedSizetoPageHeaderFlagSize(applies to all block format data)This addresses feedback from @ncruces and @benbjohnson on PR #72:
Changes
ltx.goPageHeaderFlagSizeconstant (renamed fromPageHeaderFlagCompressedSize)encoder.golz4.Compressor.CompressBlock()instead of frame writerdecoder.golz4.UncompressBlock()for new format, keep frame fallback*_test.goFormat Comparison
Backward Compatibility
Review Feedback Addressed
PageHeaderFlagCompressedSize→PageHeaderFlagSize(per @benbjohnson)Test plan
🤖 Generated with Claude Code