feat(database): add opt-in SST block compression - #722
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces SST block compression options for newly written table files, supporting both boolean shorthands and explicit algorithm names. It implements native C++ validation to ensure requested algorithms are linked into the RocksDB build, propagates configuration options to late-created column families, and prevents conflicting compression settings on already-open database paths. The review feedback highlights two valuable improvement opportunities: caching the supported compressions statically in 'isLinkedIn' to avoid repeated RocksDB queries, and normalizing the compression conflict check in 'DBRegistry' to resolve an asymmetric comparison behavior between omitted and explicit 'none' options.
2e63001 to
ededd17
Compare
| } | ||
| DEBUG_LOG("%p DBRegistry::OpenDB Creating column family \"%s\"\n", instance.get(), name.c_str()); | ||
| auto column = rocksdb_js::createRocksDBColumnFamily(entry.descriptor->db, name); | ||
| auto column = rocksdb_js::createRocksDBColumnFamily( |
There was a problem hiding this comment.
Could we overlay this handle's per-CF memory options before creating the family? Reusing the first opener's entire cfOptions means a late CF silently ignores its own writeBufferSize, maxWriteBufferNumber, and maxWriteBufferSizeToMaintain. For example, default opened at 64 MiB followed by a new named CF requesting 64 KiB still gives the named CF 64 MiB. The public docs describe these knobs as per-CF. I suggest copying the descriptor base, overlaying the current handle's per-CF values, and retaining descriptor compression because that setting is intentionally database-wide; the test at test/db-options.test.ts:81 should also exercise this already-open registry path.
There was a problem hiding this comment.
Done. Late CF creation now copies the descriptor’s base options, overlays the current handle’s writeBufferSize, maxWriteBufferNumber, and maxWriteBufferSizeToMaintain, and retains descriptor compression/table settings. I also added a regression test covering the already-open registry path.
| NAPI_STATUS_THROWS(rocksdb_js::getProperty(env, options, "compression", dbHandleOptions.compression)); | ||
| // Validated here, not in DBDescriptor::open: an open that reuses an | ||
| // already-open path never reaches it. | ||
| if (!dbHandleOptions.compression.empty()) { |
There was a problem hiding this comment.
An explicit compression: '' reaches this as an empty string, so it is indistinguishable from omission and skips validation; the registry conflict guard skips it for the same reason. Untyped JS then silently gets RocksDB's default even though tryResolveCompressionType("") is explicitly invalid. Could we track property presence separately (for example with std::optional) or reject an explicitly empty value, with an integration test beside the gzip case?
There was a problem hiding this comment.
Done. Compression is now parsed through std::optional, so an explicit '' is preserved and rejected by the resolver. Added an integration test beside the gzip case.
| TEST(CompressionTest, ZlibIsAvailable) { | ||
| rocksdb::CompressionType type = rocksdb::kNoCompression; | ||
| std::string error; | ||
| EXPECT_TRUE(tryResolveCompressionType("zlib", type, error)) << error; |
There was a problem hiding this comment.
This is a published-prebuild guarantee, not a property of every supported RocksDB build. With ROCKSDB_PATH pointing to a build without ZLIB, the resolver correctly rejects zlib but pnpm test:native fails. The integration assertion at test/db-options.test.ts:284 has the same portability issue in the other direction: a Snappy-enabled default correctly conflicts with explicit none. Could these checks be gated by the build profile/effective support, or moved to packaging CI?
There was a problem hiding this comment.
Done. The tests now adapt to what the build supports, while release builds still verify that zlib is included. I also tested against a build with compression support disabled.
| if (!tryResolveCompressionType(options.compression, requested, error)) { | ||
| throw rocksdb_js::DBException(error); | ||
| } | ||
| if (requested != entry.descriptor->cfOptions.compression) { |
There was a problem hiding this comment.
Low: conflict check compares against the build-dependent default compression
When the first open() of a path omits compression, entry.descriptor->cfOptions.compression holds RocksDB's default, which its ColumnFamilyOptions constructor sets to Snappy_Supported() ? kSnappyCompression : kNoCompression. On a build that links Snappy, opening a path first with omitted compression and then with explicit 'none' makes requested (kNoCompression) != kSnappyCompression and throws a spurious already open with compression 'default', requested 'none'. The published prebuilt links no Snappy, so this is benign there, but it means the should not treat an explicit none after an omitted compression as a conflict test only passes on builds without Snappy — the behavior is build-dependent.
Suggested fix: only enforce the mismatch when the first open was itself explicit — e.g. skip the throw when entry.descriptor->compression.empty() (opened as default), matching the stated intent that a conflict is between two explicit values. Alternatively gate the no-conflict tests on Snappy support.
—
Generated by Barber AI
There was a problem hiding this comment.
Fixed using second suggestion. The test now handles both types of build instead of assuming the default is always none. It expects a conflict when Snappy is available and no conflict when it isn’t.
| - `path: string` The path to write the database files to. This path does not need to exist, but the | ||
| parent directories do. | ||
| - `options: object` [optional] | ||
| - `compression: boolean | string` Enables SST block compression for newly written table files. |
There was a problem hiding this comment.
Low: doc omits that blob files (large values) are not compressed
This option maps to ColumnFamilyOptions::compression, which compresses only SST blocks. Values at or above min_blob_size (2048 bytes, hardcoded) are stored in blob files, whose blob_compression_type is left at its default (kNoCompression) and is unaffected here. Given Harper routinely stores large values as blobs, enabling compression may yield little space savings on real payloads, which is surprising from the current wording.
Suggested fix: add a sentence noting the option applies to SST blocks only and that values >= 2048 bytes (stored as blob files) are not compressed by it.
—
Generated by Barber AI
There was a problem hiding this comment.
Added to README.md:47 and src/store.ts:87: values ≥2,048 bytes go into blob files, remain uncompressed, and are unaffected by this SST option.
…ated CFs
Column families that don't exist when `DB::Open` runs are created afterwards
via `createRocksDBColumnFamily`, which built its own `ColumnFamilyOptions`
rather than reusing the database's. Every tunable the database configured
silently reverted to the RocksDB default for those families, which is every
named column family, since a named CF never exists on a fresh path.
Affected:
- writeBufferSize: 64MB (RocksDB default) instead of the configured 16MB
- maxWriteBufferNumber: 2 instead of 16
- maxWriteBufferSizeToMaintain: 0 instead of the derived value, leaving no
retained memtable history for OptimisticTransactionDB conflict checking
- noBlockCache: ignored, so a no-cache database still cached blocks for its
named column families
The descriptor now retains the options `DB::Open` was given and
`createRocksDBColumnFamily` takes them, so a future option propagates by
construction instead of needing a second edit here. The blob settings were
already duplicated across both sites, which is how the drift went unnoticed.
Behavior change: existing named column families move from 64MB to 16MB
memtables, which changes flush cadence.
Adds a `compression` option accepting a boolean shorthand (`true` -> `zlib`, `false` -> `none`) or an explicit algorithm name. The name is resolved and checked against the compressors actually linked into the build, because RocksDB silently degrades to no compression when asked for one it lacks. Without the check, requesting `zstd` on a build without it would quietly write uncompressed table files. Resolution lives in a Node-free translation unit so it is reachable from GoogleTest, and holds the only C++ list of algorithm names. Validation runs in `Database::Open` rather than at descriptor creation so an open that reuses an already-open path is checked too. Only zlib and none are usable in the current prebuilt; the error names what the running build supports.
Every handle on a path shares one RocksDB instance, so a second `open()` cannot change how table files are written. Previously the request was silently ignored and the caller believed it had compression it did not have. Compares resolved `CompressionType` values rather than the requested names. An omitted option leaves RocksDB's own default, which is snappy where linked and none otherwise, so comparing names would reject `compression: false` against a database already writing uncompressed files. Follows the uncoded `mode`-mismatch precedent in `DBRegistry::OpenDB` rather than the coded `ERR_DATABASE_READONLY` one.
ededd17 to
448b275
Compare
|
Thanks for the quick review. Addressed comments. Let me know if anything needs a second pass. |
3 participants
Summary
Adds a
compressionoption acceptingtrue(zlib),false(none), or an explicit algorithm name.Omitting it leaves RocksDB's own default untouched, so existing callers are unaffected. That
default is snappy on builds that link it and none otherwise; the published prebuilt has no snappy,
so omitting the option today means no compression. Values at or above 2,048 bytes are stored in
blob files, which remain uncompressed and are unaffected by this option.
Requesting an algorithm the RocksDB build does not include causes
open()to fail. This isdeliberate: RocksDB reports no error in that case and silently writes uncompressed files. The error
message lists what the running build supports.
The name resolver is in a translation unit free of Node dependencies so the GoogleTest suite can
link against it directly. Validation runs on every
open()rather than only the first, so a callthat reuses an already-open path is checked as well.
This also rejects an
open()requesting different compression on a path that is alreadyopen. All handles on a path share one RocksDB instance, so a later open cannot change how table
files are written; previously the request was ignored and the caller would believe compression was
active. The check compares resolved
CompressionTypevalues rather than the requested names, sincethe default varies by build as noted above. The error is uncoded, consistent with the existing
"already open in X mode" error rather than
ERR_DATABASE_READONLY.Depends on #721. Compression is a column family option and cannot reach named column families
without that fix; this diff includes that commit until #721 merges.
Test Plan
Compression is asserted against on-disk SST size rather than a round trip, since an option that has
no effect still reads and writes correctly. The same payload produces substantially smaller SST
files with zlib than without compression. Verified on the default column family, a named column
family, and one added to an already-open database, which exercise three separate code paths.
Tests adapt to the compression algorithms the running build supports. Also verified with
ROCKSDB_PATHpointing to a build with all optional compressors disabled. The packaging smoke testchecks that the published prebuilt includes zlib.
Local runs are macOS arm64 only.