LittDB compression#3769
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3769 +/- ##
==========================================
- Coverage 59.88% 58.95% -0.93%
==========================================
Files 2288 2201 -87
Lines 190023 180144 -9879
==========================================
- Hits 113786 106204 -7582
+ Misses 66091 64597 -1494
+ Partials 10146 9343 -803
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview On disk, segment metadata moves to v4 with a per-segment compression byte; legacy v3 segments still load as uncompressed. Segments use API constraints: on compressed tables, secondary keys must alias the full value (no sub-range slices into a compressed blob); iterators skip the uncompressed sub-slice optimization when the segment is compressed. Also adds Reviewed by Cursor Bugbot for commit 23cfd53. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 23cfd53. Configure here.
| "litt_compression_compressed_bytes", | ||
| metric.WithDescription("The number of compressed value bytes produced by compression since startup."), | ||
| metric.WithUnit("By"), | ||
| ) |
There was a problem hiding this comment.
Metric names duplicate unit suffixes
Medium Severity
New compression instruments include the unit in the metric name while also setting WithUnit, and the Prometheus exporter is configured with UnderscoreEscapingWithSuffixes. That yields duplicated suffixes such as _bytes_bytes / _seconds_seconds. Existing byte counters in this file avoid embedding _bytes in the name (e.g. litt_bytes_read).
Triggered by learned rule: OTel Prometheus export: namespace prefix and unit auto-suffix naming
Reviewed by Cursor Bugbot for commit 23cfd53. Configure here.
| // Size returns the size of the metadata file in bytes. | ||
| func (m *metadataFile) Size() uint64 { | ||
| return V3MetadataSize | ||
| return V4MetadataSize |
There was a problem hiding this comment.
V3 metadata size overstated
Low Severity
metadataFile.Size always returns V4MetadataSize (19). Legacy sealed v3 metadata files are still 18 bytes and remain readable as CompressionNone, so table size accounting is off by one byte per such segment after upgrade.
Reviewed by Cursor Bugbot for commit 23cfd53. Configure here.
|
|
||
| // The whole compressed blob is the on-disk representation of the value; the primary and every | ||
| // (full-value-alias) secondary are addressed to it. Reads decompress the blob to recover the value. | ||
| blobAddress := types.NewAddress(s.index, firstByteIndex, shard, uint32(blobLen)) //nolint:gosec // bounded above |
There was a problem hiding this comment.
Compressed size can overflow address
Medium Severity
PutBatch only caps uncompressed value length at MaxUint32, but S2 can expand incompressible data. WriteCompressed then casts the compressed length to uint32 under a false “bounded above” assumption, truncating Address.ValueSize and corrupting later reads for oversized blobs.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 23cfd53. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds optional per-table S2 compression to LittDB via a compression stage in front of the control loop, with per-segment algorithm persistence (v4 metadata) and backward-compatible v3 reads. The design is sound and well-tested (round-trip, flush ordering, recovery, GC, iteration, restart-toggle); two low-severity correctness/accounting edge cases are worth addressing but none block merge.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Ordering correctness verified:
controllerChannelis only consumed at control_loop.go:180 and only fed byenqueue→inputChannelplus the compression loop's verbatim forward, so no path bypasses the compression stage — flush ordering holds as documented. - Second-opinion inputs:
cursor-review.mdis empty (Cursor produced no output) andREVIEW_GUIDELINES.mdis empty/missing, so no repo-specific standards were applied beyond AGENTS.md. Codex's two findings (P2 blob-size truncation, P3 metadata Size()) are both echoed inline below. - No prompt-injection or suspicious instructions were found in the PR diff, title, or description.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| // The whole compressed blob is the on-disk representation of the value; the primary and every | ||
| // (full-value-alias) secondary are addressed to it. Reads decompress the blob to recover the value. | ||
| blobAddress := types.NewAddress(s.index, firstByteIndex, shard, uint32(blobLen)) //nolint:gosec // bounded above |
There was a problem hiding this comment.
[suggestion] uint32(blobLen) is truncated but only currentSize (firstByteIndex) is bounded by the check at line 593 — blobLen itself is not. S2 can slightly expand incompressible input, so a permitted near-4 GiB (uncompressed-addressable) value can produce a compressed blob exceeding math.MaxUint32. The control-loop roll at control_loop.go uses the compressed length but rolling to a fresh segment (currentSize=0) does not prevent the truncation here, yielding a corrupt address and unreadable data. Extremely unlikely in practice, but consider rejecting a compressed blob whose length exceeds math.MaxUint32 before writing (matches Codex P2). The // bounded above nolint comment is also now misleading for blobLen.
| @@ -139,7 +161,7 @@ func getMetadataFileIndex(fileName string) (uint32, error) { | |||
|
|
|||
| // Size returns the size of the metadata file in bytes. | |||
| func (m *metadataFile) Size() uint64 { | |||
There was a problem hiding this comment.
[nit] Size() now unconditionally returns V4MetadataSize (19), but a reopened legacy v3 metadata file is 18 bytes on disk. Since Size() feeds immutableSegmentSize accounting (control_loop.go / disk_table.go), tables with v3 segments over-report disk usage by one byte per segment. Consider returning based on m.segmentVersion (matches Codex P3). Impact is negligible but the value is technically wrong.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return 0, 0, | ||
| fmt.Errorf("failed to send value to shard control loop: %v", err) | ||
| } | ||
|
|
||
| // The whole compressed blob is the on-disk representation of the value; the primary and every | ||
| // (full-value-alias) secondary are addressed to it. Reads decompress the blob to recover the value. | ||
| blobAddress := types.NewAddress(s.index, firstByteIndex, shard, uint32(blobLen)) //nolint:gosec // bounded above | ||
|
|
There was a problem hiding this comment.
🟡 On compressed tables, Segment.WriteCompressed addresses every key to the compressed blob length, but keymapManager.pendingPutBytes (which approximates raw bytes sitting in DiskTable.unflushedDataCache) sums Address.ValueSize() over those same keys — so it now counts compressed bytes while the cache holds raw bytes. This undercounts the cache footprint by roughly the compression ratio, letting the maxBatchBytes early-flush trigger fire later than intended (still backstopped by maxBatchSize and maxFlushInterval, so no unbounded growth).
Extended reasoning...
The bug: Before this PR, Address.ValueSize() always equaled the length of the raw value written to a segment, and keymapManager.pendingPutBytes relied on that invariant to approximate cache memory. In segment.go:627-634 (Segment.WriteCompressed), every key in a compressed write group — the primary and every (full-value-alias) secondary — is now addressed to blobAddress := types.NewAddress(s.index, firstByteIndex, shard, uint32(blobLen)), where blobLen is the length of the compressed blob, not len(data.Value). This is necessary and correct for reads (Segment.Read/SegmentReader.Read read exactly ValueSize() bytes off disk and then decompress), but it silently breaks the assumption that ValueSize() reflects raw value size.
The code path that triggers it: These exact ScopedKeys (carrying the compressed-length Address) are what the key-file control loop accumulates and what Segment.Flush()/Seal() return as durable keys. flush_loop.go's handleFlushRequest forwards them to keymapManager.scheduleWrite, and keymapManager.routeRequest (keymap_manager.go ~line 314-315) does m.pendingPutBytes += uint64(k.Address.ValueSize()) for every primary key in the batch. Both run() (~line 238) and coalesce() (~line 275) use pendingPutBytes >= m.maxBatchBytes as the trigger to flush the accumulated put batch into the keymap — a batch-bytes cache bound whose doc comment (~line 116-118) explicitly says it "tracks the cache memory this batch will free when applied."
Why existing code doesn't catch this: DiskTable.PutBatch always stores the raw, uncompressed kv.Value in unflushedDataCache (disk_table.go), regardless of whether the table is compressed — that part is unaffected by this PR. Nothing in the write or flush path re-derives or cross-checks the raw byte count once the ScopedKey's Address has been rewritten to the compressed length; pendingPutBytes simply trusts Address.ValueSize() to mean what it always meant, and that assumption is exactly what WriteCompressed breaks for compressed tables.
Impact: For a compressed table with a good compression ratio (the PR's own compression_test.go uses payloads that shrink >10x with S2), pendingPutBytes undercounts the true unflushed-cache footprint by roughly that same ratio. This means the keymap manager will let far more raw bytes accumulate in DiskTable.unflushedDataCache than maxBatchBytes was tuned to allow before forcing a keymap put (and the corresponding cache-freeing prune). The maxBatchSize key-count threshold and maxFlushInterval timer remain effective backstops, so this is a loosened heuristic rather than an unbounded leak, crash, or data-loss bug — it only matters for a few-large-values, highly-compressible workload where the byte threshold (not the key-count threshold) was meant to be the binding constraint.
Step-by-step proof:
- Table is created with
Compression: types.CompressionS2. - Client calls
Put(key, value)wherevalueis 200KB of highly repetitive data.DiskTable.PutBatchstores the full 200KB raw value inunflushedDataCache["key"]. - The compression loop compresses it to, say, 2KB (100x ratio) and forwards a
controlLoopWriteRequestwithcompressedValues[0]= the 2KB blob. controlLoop.handleWriteRequestcallsseg.WriteCompressed(kv, req.compressedValues[0]), which buildsblobAddresswithValueSize() == 2048(the compressed length) and sends this address for the primary key to the key-file control loop.- On
Flush()/Seal(), thatScopedKey{Key: "key", Address: blobAddress}flows throughflush_loop.gotokeymapManager.scheduleWrite. routeRequestdoesm.pendingPutBytes += uint64(k.Address.ValueSize()), adding 2048, even thoughunflushedDataCache["key"]still holds the full 200KB raw value.- Repeat for many such keys:
pendingPutBytesgrows ~100x slower than the actual raw-byte footprint inunflushedDataCache. ThependingPutBytes >= maxBatchBytescheck inrun()/coalesce()therefore waits ~100x longer (in raw-byte terms) to trigger the batch flush than it would for an uncompressed table with the same access pattern — exactly the scenariomaxBatchBytes's doc comment says it exists to bound.
Suggested fix: Track the raw value length separately from the on-disk Address.ValueSize() for byte-accounting purposes — e.g. have keymapManager.routeRequest (or the ScopedKey/write-request plumbing upstream of it) sum len(kv.Value) instead of relying on Address.ValueSize(), or plumb the pre-compression size alongside the ScopedKey so cache-memory heuristics aren't coupled to the on-disk (possibly compressed) representation.
I reviewed the four verifier writeups on this bug; all converge on the same mechanism and unanimously land on nit severity for the same reasons (bounded by maxBatchSize/maxFlushInterval, opt-in via Compression config, no correctness/crash/data-loss impact), so there is no refutation to reconcile here.
| func (m *metadataFile) Size() uint64 { | ||
| return V3MetadataSize | ||
| return V4MetadataSize | ||
| } |
There was a problem hiding this comment.
🟡 metadataFile.Size() always returns V4MetadataSize (19), but a legacy v3 metadata file (18 bytes, no compression byte) loaded via deserialize() keeps segmentVersion == ShardedAddressSegmentVersion until the segment is resealed, so Size() over-reports by 1 byte for every such pre-existing segment. This feeds into Segment.Size() and DiskTable's size accounting, causing a small, purely cosmetic over-count in the reported table size; a correct fix needs to be careful since serialize() never updates m.segmentVersion after a reseal.
Extended reasoning...
metadataFile.Size() (metadata_file.go:163-165) unconditionally returns V4MetadataSize (19), regardless of the segment's actual on-disk format. Before this PR, every metadata file was exactly V3MetadataSize (18) bytes and the constant return was correct. This PR introduces a second format: deserialize() now branches on segmentVersion, accepting either the legacy 18-byte v3 layout or the new 19-byte v4 layout (with the trailing compression byte). Per the comment on serialize(), "Metadata is always written at LatestSegmentVersion (v4)" — meaning a v3 file loaded from disk is not rewritten as v4 unless the segment happens to be resealed (e.g. during crash recovery). So after loadMetadataFile() reads a genuinely 18-byte v3 file, m.segmentVersion == ShardedAddressSegmentVersion and the file on disk is still 18 bytes, yet Size() reports 19.
That 1-byte discrepancy is not just theoretical: the PR itself explicitly supports this scenario, exercising it in TestV3MetadataReadsAsUncompressed and via the CompressedSegmentVersion godoc ("keep reads of pre-existing v3 metadata files working"). Size()'s own doc comment ("returns the size of the metadata file in bytes") is violated for exactly the case the PR added support for.
Step-by-step proof:
- A v3-only build (pre-PR) creates and seals a segment. Its metadata file is written as 18 bytes (version 3, no compression byte).
- This PR is deployed. On restart,
loadMetadataFile()reads that 18-byte file.deserialize()seessegmentVersion == ShardedAddressSegmentVersion, expects (and gets) 18 bytes, and setsm.compressionAlgorithm = CompressionNone. The file itself is untouched — still 18 bytes on disk. - As long as this segment is never resealed (the normal, common case for an already-sealed, immutable segment), the file stays 18 bytes.
- Any call to
m.Size()returns the hardcodedV4MetadataSize(19), a 1-byte overcount versus the real 18-byte file.
Where it propagates: metadataFile.Size() feeds Segment.Size() (segment.go), which is summed into DiskTable's immutableSegmentSize at startup (disk_table.go NewDiskTable) and adjusted in control_loop.go's deleteEligibleSegments/expandSegments. The net effect is that DiskTable.Size() (and the corresponding metrics gauge) over-reports by exactly 1 byte per pre-existing v3 segment that has not been resealed.
Why it is low-impact: the accounting stays internally consistent — the same (over-reported) 19 is what gets added at startup and later subtracted when the segment is reclaimed, so there is no drift, underflow, or corruption; it is a pure reporting artifact. Segment rollover and GC decisions do not depend on this value (they use TargetSegmentFileSize/MaxSegmentKeyCount and TTL respectively). Also, per segment_version.go's comment, ShardedAddressSegmentVersion (v3) was never deployed to production, which may mean this legacy-file scenario has limited real-world occurrence today, though the PR is explicitly designed to support it going forward.
Suggested fix, with a caveat: the natural fix is to branch Size() on m.segmentVersion, mirroring deserialize() (V3MetadataSize for ShardedAddressSegmentVersion, V4MetadataSize for CompressedSegmentVersion). However, serialize() always writes LatestSegmentVersion into the file bytes but never updates the in-memory m.segmentVersion field itself (it is only set in createMetadataFile and deserialize). That means after a v3 segment is resealed (e.g. via sealLoadedSegment during crash recovery), the on-disk file becomes 19 bytes (v4) while m.segmentVersion is still stuck at 3 — a naive version-based branch would then under-report a 19-byte file as 18. A fully correct fix should either update m.segmentVersion to LatestSegmentVersion inside serialize()/seal() once the file is rewritten, or have Size() track the actual serialized byte length directly rather than re-deriving it from a field that can go stale.
1 participant
Describe your changes and provide context
Adds optional compression for LittDB.