fix(database): apply the database's column family options to late-created column families - #721
fix(database): apply the database's column family options to late-created column families#721Minh-Ng wants to merge 3 commits into
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 ensures that dynamically created column families inherit the same ColumnFamilyOptions (such as writeBufferSize) that the database was originally opened with, rather than falling back to RocksDB defaults. This is achieved by storing cfOptions in DBDescriptor and passing it to createRocksDBColumnFamily. A corresponding integration test has been added to verify this behavior. I have no feedback to provide.
d9dd882 to
ee88116
Compare
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Nice, good catch.
Additional inline comments:
test/db-options.test.ts:80: The production change also routes a missing CF through the already-open descriptor inDBRegistry::OpenDB, but these two fresh paths only exercise creation insideDBDescriptor::open. Could we add a same-path case that opens the default CF first with a 64 KiB buffer, then opens a missing named CF and verifies that it flushes? That would protect the retainedcfOptionsmember and the registry wiring, which this test can currently pass without.
|
|
||
| // A named CF is created after DB::Open, so it misses the open options. | ||
| // Observed via flush behavior: RocksDB exposes no property for the size. | ||
| it('should apply writeBufferSize to a named column family', () => |
There was a problem hiding this comment.
The production change also routes a missing CF through the already-open descriptor in DBRegistry::OpenDB, but these two fresh paths only exercise creation inside DBDescriptor::open. Could we add a same-path case that opens the default CF first with a 64 KiB buffer, then opens a missing named CF and verifies that it flushes? That would protect the retained cfOptions member and the registry wiring, which this test can currently pass without.
There was a problem hiding this comment.
Added test coverage. Used a 64 MiB buffer for the first handle and 64 KiB for the late named CF because these memory settings are per-CF and the late handle’s value should win. The test verifies that the late CF automatically flushes through the DBRegistry::OpenDB path.
| 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( | ||
| entry.descriptor->db, name, entry.descriptor->cfOptions |
There was a problem hiding this comment.
Low: the already-open-DB late-CF path is untested
The new test opens two separate databases (each via generateDBPath()), so it only exercises DBDescriptor::open's late-CF branch. This branch — adding a named CF to an already-open DB using the freshly-stored descriptor->cfOptions — is the round-trip unique to this fix (options captured at first DB::Open, reused on a later open() for a new CF name) and has no coverage.
Worth a targeted test: open a DB with writeBufferSize: 64 * 1024, then open a second named CF on the same path, and assert it flushes (rocksdb.total-sst-files-size > 0). Note this path also means a writeBufferSize / noBlockCache passed on the later open call is intentionally dropped in favor of the first open's options — consistent with the PR's stated design, but a test would pin that behavior down.
—
Generated by Barber AI
There was a problem hiding this comment.
Covered by the new test discussed above
| } | ||
|
|
||
| // Flushes run in the background. | ||
| const deadline = Date.now() + 5000; |
There was a problem hiding this comment.
Would it be possible to force a await db.flush() and reduce the amount of time we need to wait? 5 seconds seems arbitrary, so I wonder if we could get this down to 1 second?
There was a problem hiding this comment.
Yes 5 seconds is arbitrary, agreed polling is not ideal. Calling flush() would remove the behavior the test is checking. Instead, we limit the CF to two write buffers. When both are full, RocksDB pauses later writes until it automatically flushes one. Since the test awaits every put(), it cannot reach the assertion until that flush completes, so no separate timeout or polling is needed.
…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.
ee88116 to
f2698db
Compare
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Maybe an improvement could be made to centralizing construction of ColumnFamilyOptions? Otherwise LGTM.
🤖 Reviewed with Codex
| auto column = rocksdb_js::createRocksDBColumnFamily(entry.descriptor->db, name); | ||
| // Preserve the retained table/blob settings, but apply the current | ||
| // handle's per-CF memory options to the family it is creating. | ||
| auto cfOptions = entry.descriptor->cfOptions; |
There was a problem hiding this comment.
The follow-up correctly takes the three memtable fields from the handle creating this CF, but noBlockCache is also a per-CF setting: it is encoded in ColumnFamilyOptions::table_factory from that handle on the first-open path. Copying the descriptor's factory here makes the cache policy depend on whichever handle opened the path first. For example, opening the default family normally and then opening late with noBlockCache: true still lets a post-flush read populate the cache; reversing those options leaves late uncached even though it requested the default cache. Could we centralize construction of the complete ColumnFamilyOptions from the current DBOptions for both creation paths, or at least rebuild the table factory from options.noBlockCache here, and add a same-path regression with opposing cache policies?
There was a problem hiding this comment.
Will centralize construction in buildColumnFamilyOptions() and use it from both creation path, thanks
4 participants
Summary
A named column family does not exist when a database is first created, so it is added immediately
after
DB::Open. That path built its own options instead of reusing the ones the database wasopened with, so RocksDB's defaults applied. Values passed to
open()were silently dropped forthose column families, as were this library's own tuned defaults:
optimistic transaction conflict detection
noBlockCache: ignoredDatabases using only the default column family are unaffected. Stored data is unaffected either
way; these govern how memtables are sized and flushed, not what gets written.
The descriptor now saves the column family options used by
DB::Open. When a handle creates acolumn family later, it starts with those saved options, then applies
writeBufferSize,maxWriteBufferNumber, andmaxWriteBufferSizeToMaintainfrom that handle.Named column families now use the configured write buffer, 16MB by default rather than 64MB, so
they will flush more frequently than they do today.
Test Plan
Added a test that opens two named column families, identical apart from write buffer size (64KB and
64MB), and writes 512KB to each. The smaller one flushes to disk unprompted; the larger one does
not. Both measured zero before this change, so the test fails without the fix.
Added a test that gives the default column family a 64MB write buffer and a named column family a
64KB write buffer. After writing 512KB to the named column family, the test confirms that it flushes
instead of inheriting the default column family's 64MB setting.
noBlockCachewas confirmed manually: a named column family now reports no block cache, against557KB of usage when caching is enabled. The remaining two options are corrected by the same change
but have no direct assertion, as I could not find a way to observe them through the JavaScript API.