Repository navigation
fix: collate per-key cap must count real bytes, not fixed record slots - #58
Merged
Merged
Conversation
ldb_collate_add_variable_record() caps a key's list at the input sector size (max_key_bytes) as an OOM guard: a key's records are a subset of its sector, so they can never legitimately exceed it. The check compared that byte size against collate->data_ptr, which is not a byte count of the data: it advances by a fixed rec_width slot per record (key_ln + MAX_RECORD + 4 = 2060 B for KEY_SIZE=8 / MAX_RECORD=2048) whatever the record's real size. The cap therefore fired after sector_size/rec_width records instead of at sector_size worth of data - over 10x too early for typical records, and sooner still because the sector file size is dominated by the fixed 256^3 pointer map (~84 MB), which makes the threshold nearly constant at ~41k-165k records per key regardless of the data. Every further record of that key was dropped and the import still reported success. The cap is also applied while records are read, i.e. before ldb_eliminate_duplicates(), so a duplicate-heavy key was truncated on its raw count and the survivors deduplicated down to a fraction of the real data. Observed on daily_vulnerability/vulnerability (KEYS=1, FIELDS=10, KEY_SIZE=8): sector c3 imported 1695299 records with 0 skipped, then collated to 1444796; key c3fc2c156d152b64 held 38002 distinct records and only 15438 survived. Variable-record tables such as attribution and copyright are affected the same way. - collate.c: account the current key in real bytes (LDB_KEY_LN + subkey_ln + size + 4) in the new collate->key_bytes, and compare that against max_key_bytes. Reset per key alongside data_ptr. - collate.c / import.c: count truncated keys. Discarding records is now logged as E078 instead of a plain "collate completed", and import_collate_sector() returns LDB_ERROR_COLLATE_TRUNCATED rather than success. - collate.c: key_rec_count was only incremented in the else branch, so it missed the first record of every key. It feeds the diagnostic message, so fix it. - test_kb_crc64: two regression tests. test_14 imports 60000 distinct records under one key (kept 41770 before); test_14b covers the cap-before-dedup path with 20000 distinct records repeated 6 times (kept 7516 before). Note: restoring the dropped records restores their memory cost too. Peak RSS for 418k records on a single key measured 945 MB against 228 MB while truncating - the fixed 2060-byte slot per record, not the cap, is what drives it. Lowering MAX_RECORD to fit the table's real record size scales rec_width down proportionally. Also bump LDB_VERSION to 4.3.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
ldb_collate_add_variable_record() caps a key's list at the input sector size (max_key_bytes) as an OOM guard: a key's records are a subset of its sector, so they can never legitimately exceed it. The check compared that byte size against collate->data_ptr, which is not a byte count of the data: it advances by a fixed rec_width slot per record (key_ln + MAX_RECORD + 4 = 2060 B for KEY_SIZE=8 / MAX_RECORD=2048) whatever the record's real size.
The cap therefore fired after sector_size/rec_width records instead of at sector_size worth of data - over 10x too early for typical records, and sooner still because the sector file size is dominated by the fixed 256^3 pointer map (~84 MB), which makes the threshold nearly constant at ~41k-165k records per key regardless of the data. Every further record of that key was dropped and the import still reported success. The cap is also applied while records are read, i.e. before ldb_eliminate_duplicates(), so a duplicate-heavy key was truncated on its raw count and the survivors deduplicated down to a fraction of the real data.
Observed on daily_vulnerability/vulnerability (KEYS=1, FIELDS=10, KEY_SIZE=8): sector c3 imported 1695299 records with 0 skipped, then collated to 1444796; key c3fc2c156d152b64 held 38002 distinct records and only 15438 survived. Variable-record tables such as attribution and copyright are affected the same way.
Note: restoring the dropped records restores their memory cost too. Peak RSS for 418k records on a single key measured 945 MB against 228 MB while truncating - the fixed 2060-byte slot per record, not the cap, is what drives it. Lowering MAX_RECORD to fit the table's real record size scales rec_width down proportionally.
Also bump LDB_VERSION to 4.3.0.