Add storage tiering: SST paths and blob file placement across volumes - #767
Draft
kriszyp wants to merge 2 commits into
Draft
Add storage tiering: SST paths and blob file placement across volumes#767kriszyp wants to merge 2 commits into
kriszyp wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces tiered storage support for RocksDB, allowing SST files to be distributed across multiple storage paths and large values to be decoupled into separate blob files. The changes span C++ bindings, TypeScript definitions, documentation, and tests. The review feedback correctly identifies a cross-platform path handling issue on Windows, where using std::filesystem in the C++ layer can corrupt non-ASCII paths due to ANSI code page round-tripping. It is recommended to resolve and normalize all paths in the JavaScript layer using node:path before passing them to the native binding.
Comment on lines
+1562
to
+1565
| // Resolve now: RocksDB interprets a relative path against the process | ||
| // working directory at open time, so storing it raw would let the same | ||
| // string resolve to a different volume on a later open. | ||
| storagePath.path = std::filesystem::absolute(storagePath.path).lexically_normal().string(); |
Contributor
There was a problem hiding this comment.
On Windows, constructing a std::filesystem::path from a UTF-8 std::string and converting it back via .string() round-trips through the active code page (ANSI), which can corrupt non-ASCII paths. To prevent this and ensure robust cross-platform path handling, resolve and normalize the paths in the JavaScript layer using path.resolve() before passing them to the native binding, and remove the std::filesystem conversions here.
Suggested change
| // Resolve now: RocksDB interprets a relative path against the process | |
| // working directory at open time, so storing it raw would let the same | |
| // string resolve to a different volume on a later open. | |
| storagePath.path = std::filesystem::absolute(storagePath.path).lexically_normal().string(); | |
| // Paths are resolved to absolute and normalized form in the JavaScript layer. |
References
- In C++ Node-API bindings, avoid round-tripping UTF-8 paths from N-API through 'std::filesystem::path' on Windows if they are subsequently converted to wide strings, as this re-encodes through the active code page and can corrupt non-ASCII paths. Use plain UTF-8 string concatenation instead, as '/' is a valid separator on all platforms.
Comment on lines
+1599
to
+1604
| if (!result.dir.empty()) { | ||
| // Same reason as paths[].path: the persisted value is compared as a | ||
| // string on reopen, so a relative directory would compare equal while | ||
| // resolving somewhere else. | ||
| result.dir = std::filesystem::absolute(result.dir).lexically_normal().string(); | ||
| } |
Contributor
There was a problem hiding this comment.
Similar to the paths resolution, calling std::filesystem::absolute on Windows round-trips the UTF-8 path through the active code page, corrupting non-ASCII paths. Since we can resolve and normalize the path in the JavaScript layer using path.resolve(), we should remove the std::filesystem conversion here.
// The directory is resolved to absolute and normalized form in the JavaScript layer.References
- In C++ Node-API bindings, avoid round-tripping UTF-8 paths from N-API through 'std::filesystem::path' on Windows if they are subsequently converted to wide strings, as this re-encodes through the active code page and can corrupt non-ASCII paths. Use plain UTF-8 string concatenation instead, as '/' is a valid separator on all platforms.
Comment on lines
16
to
25
| import { | ||
| constants, | ||
| NativeDatabase, | ||
| type NativeBlobOptions, | ||
| type NativeDatabaseOptions, | ||
| NativeIterator, | ||
| type NativeStoragePath, | ||
| NativeTransaction, | ||
| stats, | ||
| supportedCompression, |
Contributor
There was a problem hiding this comment.
Import resolve from node:path to resolve and normalize the storage paths and blob directory in the JavaScript layer, avoiding active code page corruption on Windows when using std::filesystem in C++.
Suggested change
| import { | |
| constants, | |
| NativeDatabase, | |
| type NativeBlobOptions, | |
| type NativeDatabaseOptions, | |
| NativeIterator, | |
| type NativeStoragePath, | |
| NativeTransaction, | |
| stats, | |
| supportedCompression, | |
| import { resolve } from 'node:path'; | |
| import { | |
| constants, | |
| NativeDatabase, | |
| type NativeBlobOptions, | |
| type NativeDatabaseOptions, | |
| NativeIterator, | |
| type NativeStoragePath, | |
| NativeTransaction, | |
| stats, | |
| supportedCompression, |
Comment on lines
+1132
to
+1133
| paths: this.paths, | ||
| blobs: this.blobs, |
Contributor
There was a problem hiding this comment.
Resolve and normalize the paths and blobs.dir to absolute paths in the JavaScript layer using resolve(). This ensures that they are correctly compared as strings on reopen and avoids the Windows active code page corruption issue in the C++ layer.
paths: this.paths?.map((p) => ({ ...p, path: resolve(p.path) })),
blobs: this.blobs
? {
...this.blobs,
dir: this.blobs.dir ? resolve(this.blobs.dir) : undefined,
}
: undefined,
Contributor
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit fc4b4ca |
Lets one database span volumes with different cost and performance — typically
a fast local NVMe plus a larger, slower attached volume.
Two knobs, covering different files:
- `paths`: an ordered list of { path, targetSize } mapped to RocksDB's
db_paths, distributing SST files across volumes by LSM level. Left on
db_paths rather than cf_paths so one policy covers every column family.
- `blobs.dir`: the directory blob files live in. This is the only way to tier
large values — db_paths does not affect blob files, whose paths RocksDB
derives from cf_paths.front(). Requires a native build carrying the blob_dir
patch (ROCKSDB_HAS_CF_BLOB_DIR); the option is rejected at open otherwise,
and the binding still compiles and behaves against an unpatched RocksDB.
The rest of the blob settings were previously hardcoded and are now
configurable: enabled, minSize, garbage collection and its age cutoff and force
threshold, plus a process-wide blobCacheSize on RocksDatabase.config(). That
last one matters more than it looks: RocksDB does not put blob values in the
block cache, so without it every blob read is real I/O — the dominant cost once
blob files are on slower storage.
All of it goes through buildColumnFamilyOptions, so both column-family creation
paths get it. An earlier revision set these only on the open path, which left
every named family — which is every Harper table — on the hardcoded defaults
while an open stamped the caller's directory on the families listed on disk;
the next reopen then threw on the mismatch. The three named-family tests here
fail against that revision.
Reopening with a different blobs.dir does not move the existing blob files, it
strands them (a blob file's directory is derived from the option every time,
unlike an SST's MANIFEST-recorded path index), making every value >= minSize
unreadable. Open compares the request against the directory persisted in
OPTIONS and refuses the mismatch; blobs.allowDirChange acknowledges a completed
offline relocation. A warm reopen asking for different paths or a different
blob directory is rejected the same way a conflicting compression request
already was, rather than being silently ignored.
Verified against a patched RocksDB (19/19, including blob files written to a
separate volume, read back across reopen, relocated, and applied to named
column families) and against the stock unpatched prebuild (blob_dir suite
skipped, everything else passing).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/tiered-storage
branch
from
August 12, 2026 21:03
2a9d78e to
03d58b4
Compare
The tiered-storage invariants were added as 11/12 while 11 was already taken by the transaction-log frame invariant, so `oxfmt --check` failed the Check job on the duplicate ordered-list numbering. Co-Authored-By: Claude Opus 5 <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
1 participant
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.
Lets one database span volumes with different cost and performance — typically a fast local NVMe plus a larger, slower attached volume.
Two knobs, covering different files:
paths— an ordered list of{ path, targetSize }mapped to RocksDB'sdb_paths, distributing SST files across volumes by LSM level. Kept ondb_pathsrather thancf_pathsso one policy covers every column family in the database.blobs.dir— the directory blob files live in. This is the only way to tier large values:db_pathsdoes not affect blob files, whose paths RocksDB derives fromcf_paths.front(). It needs a native build carrying the downstreamblob_dirpatch (Add RocksDB blob_dir patch so blob files can live on a separate volume rocksdb-prebuilds#13); the option is rejected at open otherwise, and the binding still compiles and behaves correctly against an unpatched RocksDB.The remaining blob settings were previously hardcoded and are now configurable (
enabled,minSize, garbage collection plus its age cutoff and force threshold), along with a process-wideblobCacheSizeonRocksDatabase.config(). That last one matters more than it looks: RocksDB does not put blob values in the block cache, so without it every blob read is real I/O — the dominant cost once blob files are on slower storage.Depends on HarperFast/rocksdb-prebuilds#13 — the
blobs.dirtests skip until a prebuild carries the patch. Everything else works against the current prebuild.Where to look
buildColumnFamilyOptionsinsrc/binding/database/db_descriptor.cpp, and the reopen guard about 60 lines below it.Putting the blob settings in that shared builder is load-bearing rather than tidiness. An earlier revision set them only on the open path, which left every named column family — which is every Harper table — on the hardcoded defaults, while an open stamped the caller's directory on the families listed on disk. The next reopen then threw on the mismatch, or with
allowDirChangepointed the family at a directory its blob files were not in. The three named-column-family tests fail against that revision; I verified that by restoring the old behavior and re-running, not by reasoning about it.The guard is the other thing worth reading. A blob file's directory is derived from the option every time it is opened — unlike an SST's MANIFEST-recorded path index — so reopening elsewhere strands the files rather than moving them, making every value at or above
minSizeunreadable.open()compares the request against theblob_dirpersisted in the OPTIONS file and refuses the mismatch;blobs.allowDirChangeacknowledges a completed offline relocation for the single open that performs the switch.Verification
blobs.dirsuite skips via capability detection, everything else passes.compression.test.ts > configure-rocksdb.mjs (build script), which shells out to the RocksDB provisioner; it fails only whendeps/rocksdbis symlinked to a local source tree and passes against the stock prebuild. Not related to this change.pnpm checkclean.Open items from cross-model review
blobs.diris a database-wide policy here, not per-column-family. RocksDB models it per-CF, and this applies one request to every family in the database — the same shape aspaths. A database with genuinely heterogeneous blob directories per family is not expressible, and the guard rejects one if it somehow exists. Flagged as the main judgment call.pathsappend-only invariant is documented but not enforced. A file's path index is what lands in the MANIFEST, so reordering or removing an entry points existing files at the wrong directory.db_paths/cf_pathssit in RocksDB's "not yet supported" serialization block, so there is nothing persisted to compare against without inventing a rocksdb-js-owned marker file — a new on-disk artifact I did not want to add unilaterally.docs/tiered-storage.md; not fixed.For the human reviewer
pathsand because Harper wants one policy per database. The cost is that per-table blob placement is unavailable, and widening it later means deciding what a mixed database means on reopen. Reversible now, awkward once databases exist.blobs.allowDirChangeis public API once shipped.allowDirChangeis open-scoped and covers all families — left in a config file it permanently disables the guard. A one-shot marker would be tighter; cheap to change now, awkward after adoption.targetSizeis required per entry — a default would be friendlier, but an omitted target silently means "spill immediately". Relaxing later is compatible; tightening is not.blobCacheSizedefaults to 0, matching RocksDB. Given blob files are on by default at a 2KB threshold, that means every large-value read is real I/O today — a live performance choice, not just an API one. Trivially reversible.Generated by Claude Opus 5.
Human-Review-Need: 4 @ 03d58b4