Summary:
and remove deprecated DB::MaxMemCompactionLevel(). In the process of pushing through a relatively clean refactoring of uses of the old functions, some other minor public APIs are also migrated from raw DB pointers to unique_ptr.
Claude did pretty much all the work, but requiring dozens of prompts to actually push through relatively clean phase out of raw DB pointers from what needed to be touched, and leaving that code in better shape. (Hundreds of `DB*` still remain all over the place even outside C and Java bindings.)
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14335
Test Plan: existing tests; no functional changes intended
Reviewed By: xingbowang, mszeszko-meta
Differential Revision: D93523820
Pulled By: pdillinger
fbshipit-source-id: e4ca22ad81cd2cfe91122d7507d7ca34fe03d043
Summary:
I'm implementing this intending it to be used for https://github.com/facebook/rocksdb/issues/14287
Refactor the data block footer encoding/decoding to use a struct-based Encode/Decode API (DataBlockFooter), reserving the top 4 bits of the footer for metadata:
- Bit 31: Hash index present (kDataBlockBinaryAndHash) - existing use
- Bits 28-30: Reserved for future features
Comments have some detail for why it is safe to assume no practical existing SST files would use these newly reserved bits. And for forward compatibility, existing versions detect (non-zero) use of these new bits as impossibly large num_restarts and report "bad block contents". Not perfect, but not bad.
Key changes:
- Replace PackIndexTypeAndNumRestarts/UnPackIndexTypeAndNumRestarts with DataBlockFooter::EncodeTo/DecodeFrom methods
- DecodeFrom returns a detailed error when reserved bits are set, enabling graceful failure on newer format versions
- Reduce kMaxNumRestarts from 2^31-1 to 2^28-1 (268M), which is adequate for the maximum possible restarts in a 4GiB block
- Add GetCorruptionStatus() to Block for detailed error messages (Note that we are sensitive to the size of Block objects, so have to avoid adding unnecessary new members.)
- Remove obsolete kMaxBlockSizeSupportedByHashIndex size checks
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14332
Test Plan:
- Existing unit tests and format compatibility test
- Add test for reserved bit detection (ReservedBitInDataBlockFooter)
Reviewed By: joshkang97
Differential Revision: D93293152
Pulled By: pdillinger
fbshipit-source-id: b65a83e96bb09a98fb9b8b2dd9f754653ca7ed4d
Summary:
Interpolation search is an alternative algorithm to binary search, which performs better on uniformly distributed keys. Instead of binary search always computing the mid point of the left and right boundaries, interpolation search "interpolates" the mid point based on the distance to the target. Fortunately, we can re-use existing block format to support interpolation search.
For a given block, we compute the shared_prefix length of the first and last key. Interpolation search is usually done with numerical target values, so for a variable binary length key, we calculate the "value" as the first 8 non-shared bytes. This also means interpolation search would only really be effective for bytewise comparator (guarded via options validations).
#### Fallback to binary search
- if the the val(left_key) == val(right_key) then we fallback to classic binary search (to avoid divide by 0)
- interpolation search is significantly more computationally expensive than binary search, so when the search distance is small, we also fallback to binary search.
- if interpolation search does not make significant progress (i.e. reduces search space by more than half each iteration), we can assume data is non-uniform and fallback.
Interpolation search also performs best when there is minimal shortening, especially shortening of the last block, as it can heavily skew the distribution of the actual keys.
Note that each search algorithm is guaranteed to make progress because at each iteration the search space is guaranteed to be reduce by at least 1.
For now this change only applies to index block seeks, as data block seeks and other blocks do not have as many entries and would not require significant number of search rounds, but it could be easily extended to include that support.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14247
Test Plan:
Updated unit tests and crash test with new search option
### Benchmark
The default benchmark sets up keys in generally uniform distribution, so it was a good way to test performance improvements.
Setup: `./db_bench -benchmarks=fillseq,compact -index_shortening_mode=1`
#### Before this change
```
./db_bench -use_existing_db=true -benchmarks=readrandom -seed=1
readrandom : 2.899 micros/op 344973 ops/sec 2.899 seconds 1000000 operations; 38.2 MB/s (1000000 of 1000000 found)
```
#### After this change
Notice how key comparison counts are the same between the two.
```
./db_bench -use_existing_db=true -benchmarks=readrandom -seed=1 -index_search_type=binary_search
readrandom : 2.881 micros/op 347128 ops/sec 2.881 seconds 1000000 operations; 38.4 MB/s (1000000 of 1000000 found)
```
```
./db_bench -use_existing_db=true -benchmarks=readrandom -seed=1 -index_search_type=interpolation_search
readrandom : 2.609 micros/op 383209 ops/sec 2.610 seconds 1000000 operations; 42.4 MB/s (1000000 of 1000000 found)
```
With a non-uniform distribution, `i.e. index_shortening_mode=2`
```
./db_bench -use_existing_db=true -benchmarks=readrandom -seed=1 -index_search_type=binary_search
readrandom : 2.958 micros/op 338075 ops/sec 2.958 seconds 1000000 operations; 37.4 MB/s (1000000 of 1000000 found)
```
```
./db_bench -use_existing_db=true -benchmarks=readrandom -seed=1 -index_search_type=interpolation_search
readrandom : 5.502 micros/op 181750 ops/sec 5.502 seconds 1000000 operations; 20.1 MB/s (1000000 of 1000000 found)
```
Reviewed By: pdillinger
Differential Revision: D91063163
Pulled By: joshkang97
fbshipit-source-id: 151d6aa76f8713740b714de6e406aff40d28ccbc
Summary:
... and remove some old code and tech debt in the process.
This is arguably a great milestone and precendent in RocksDB history as for the first time we are explicitly dropping support for the ability to read source-of-truth data in old formats. (We previously dropped support for reading some old bloom filters, but those are performance optimizers not source-of-truth. https://github.com/facebook/rocksdb/issues/10184) However, DBs written with default settings since release 4.6.0, which is very nearly 10 years ago, can still be read. And by using compaction with intermediate versions, there's an upgrade path going back to (AFAIK) early releases of LevelDB (from which RocksDB was forked).
Some detail:
* The magic number for LevelDB SST files (0xdb4775248b80fb57, most recently called kLegacyBlockBasedTableMagicNumber) now only exists in the code to provide a good error message and to test that good error message.
* There is some notable refactoring and renaming around format_version handling. This is a bit of a messy area of code because the footer code being shared between different table formats (block-based, plain, cuckoo) means format_version in the footer is in ways tied to all of them, but in other ways is just tied to block-based table where we have been making updates. Hopefully code comments keep this clear.
* Now that there are old format_versions we can't read (and can't write authoritatively in tests), I've needed to split out kMinSupportedFormatVersion into a constant for reads and for writes, currently the same at format_version=2. Comments describe how to update these in the future.
* The idea of versioning the compression format is basically going away, though we're keeping BuiltinV2 in places just because it's already there. There's lots of room in the BuiltinV2 schema to expand to new built-in compression types, or new ways of handling existing compression algorithms. CompressionManager with CompatibilityName gives users the power to customize compression without the need for versions tied to format_version.
Immediate follow-up:
* Clean up compression loose ends like OLD_Compress, OLD_Uncompress
Suggested follow-up:
* Update plain table builder to migrate to new footer version so that we can drop support for legacy footer. We have to be careful that the (likely untested) forward compatibility path I put in place a while back works (or fix it and wait a while) before dropping support for plain table with legacy footer.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14315
Test Plan:
* Some tests updated / added
* A couple tests are obsolete: removed
* Also updated format compatible test, which now doesn't need to dig as far back into history building RocksDB.
Reviewed By: hx235
Differential Revision: D92577766
Pulled By: pdillinger
fbshipit-source-id: a23be846189d901ce087af4ca9a99cef18445cb7
Summary:
* Address feedback from https://github.com/facebook/rocksdb/issues/14040
* Add additional test for MultiScan
* Fix a bug when del range and data are in same file for multi-scan
* Rewrite the cases need to be handled in SeekMultiScan
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14055
Test Plan: Unit test
Reviewed By: cbi42, anand1976
Differential Revision: D84851788
Pulled By: xingbowang
fbshipit-source-id: 0f69632733afb99685f6341badbf239681010c38
Summary:
Fixing a nullptr access in multiscan, under following situation.
```
Block Based Table: blk1:[k1,k2], blk2:[k3, k8], blk3:[k9]
Scan ranges: [k1, k4), [k5,k6), [k7, k10)
Prepared block ranges: [0,2], [2,2], [1,3]
```
1. Seek key k1 on the first range, read key k1, k2.
2. Seek key k4 on the 2nd range, blocks 0,1 would be unpinned.
3. Seek key k9, block 1 would be accessed, but it is unpinned, which trigger assert failure in debug mode and nullptr access on release build.
This fix changes how blocks are unpinned. It is now only unpinning the block, when the cur_data_block_idx has passed it.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14062
Test Plan:
Unit Test
rand_seed 304010984 on UserDefinedIndexStressTest
Reviewed By: cbi42
Differential Revision: D84976410
Pulled By: xingbowang
fbshipit-source-id: 6b99bf85fc9d4108c5267ae77be77ccfe08923cd
Summary:
The current seek key validation is too strict. This change relaxes it at block iterator level, and add additional check at DB iterator level. The new contract is that when MultiScan is used, after prepared is called, each following seek must seek the start key of the prepared scan range in order. Otherwise, the iterator is set with error status.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14040
Test Plan: Unit test
Reviewed By: anand1976
Differential Revision: D84292297
Pulled By: xingbowang
fbshipit-source-id: 7b31f727e67e7c0bfc53c2f9a6552e0c3d324869
Summary:
When there is an ingested SST file that only contains delete range operations, MultiScan may return error "Scan does not intersect with file". This is due to file selection during Prepare uses the file smallest and largest key without considering whether there is any key in the file. This is only a temporary fix.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14028
Test Plan: Unit test
Reviewed By: anand1976
Differential Revision: D83986964
Pulled By: xingbowang
fbshipit-source-id: e0961ca854e2062c2457be4324817ba073ae785d
Summary:
Pad block based table based on super block alignment
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13909
Test Plan:
Unit Test
No impact on perf observed due to change in the inner loop of flush.
upstream/main branch 202.15 MB/s
```
for i in `seq 1 10`; do ./db_bench --benchmarks=fillseq -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=1000 -fifo_compaction_allow_compaction=0 -disable_wal -write_buffer_size=12000000 -format_version=7 >> /tmp/x1 2>&1; grep fillseq /tmp/x1 | grep -Po "\d+\.\d+ MB/s" | grep -Po "\d+\.\d+" | awk '{sum+=$1} END {print sum/NR}'
```
After the change without super block alignment 203.44 MB/s
```
for i in `seq 1 10`; do ./db_bench --benchmarks=fillseq -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=1000 -fifo_compaction_allow_compaction=0 -disable_wal -write_buffer_size=12000000 -format_version=7 >> /tmp/x1 2>&1
```
After the change with super block alignment 204.47 MB/s
```
for i in `seq 1 10`; do ./db_bench --benchmarks=fillseq -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=1000 -fifo_compaction_allow_compaction=0 -disable_wal -write_buffer_size=12000000 -format_version=7 --super_block_alignment_size=131072 --super_block_alignment_max_padding_size=4096 >> /tmp/x1 2>&1;
```
Reviewed By: pdillinger
Differential Revision: D83068913
Pulled By: xingbowang
fbshipit-source-id: eecd65088ab3e9dbc7902aab8c2580f1bc8575df
Summary:
This PR fixes a bug in how MultiScan handled a scan range limit falling in the key range between files. The bug was in LevelIterator, where Prepare() relied on FindFile to determine the lower bound file for the range limit. FindFile returns the smallest file index with `range.limit < file.largest_key`. However, that doesn't guarantee that the range overlaps the file, as the `range.limit` could be smaller than `file.smallest_key`.
This also fixes a bug in BlockBasedTableIterator of Valid() returning true even if status() returned error. This was exposed by the previous bug.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14011
Test Plan: Add unit tests in db_iterator_test and table_test
Reviewed By: cbi42
Differential Revision: D83496439
Pulled By: anand1976
fbshipit-source-id: a9d2d138d69d0c816d9f4160a984b273d00d683f
Summary:
Pass the comparator to UDI interface for both reader and builder.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14001
Test Plan: Unit test
Reviewed By: anand1976
Differential Revision: D83339943
Pulled By: xingbowang
fbshipit-source-id: 7f6541776b0995260e28224329f0cca37f13b3d4
Summary:
Remove the restriction of only using BytewiseComparator(). In a follow on PR, the UDI interface will be updated to take the Comparator as a parameter.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13999
Test Plan: Add a unit test in table_test.cc
Reviewed By: cbi42
Differential Revision: D83179747
Pulled By: anand1976
fbshipit-source-id: 60222533c71022aa0701ac61c39268d36ca86338
Summary:
Return a failure status for multi scan if Prepare fails, or if the scan options are unsupported, instead of falling back on a regular scan. This PR also fixes a bug in LevelIterator that caused max_prefetch_size to be ignored.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13974
Test Plan: Add new test in db_iterator_test and table_test
Reviewed By: xingbowang
Differential Revision: D82843944
Pulled By: anand1976
fbshipit-source-id: f12756c40ebd38d8d4e4425e97438b6e766a4663
Summary:
Force caller of MultiScanArgs to pass comparator. Pass comparator from CF handle to MultiScanArgs in NewMultiScan.
Expand MultiScanArgs unit test with different comparator.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13970
Test Plan: unit test
Reviewed By: cbi42
Differential Revision: D82739270
Pulled By: xingbowang
fbshipit-source-id: e709f4a333ad547c0ba6d24d8fb2b22e50e8a12f
Summary:
when ingesting DB generated file with non-zero sequence number, we need smallest seqno of each file for file meta data. To avoid full table scan, we record this information in table property and use it during file ingestion.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13942
Test Plan: new unit test and updated existing unit test.
Reviewed By: hx235
Differential Revision: D82331802
Pulled By: cbi42
fbshipit-source-id: 3009a6801ca7092cd0fde33692db1a13567068a9
Summary:
This PR fixes a bug in BlockBasedTableIterator::Prepare in conjunction with a user defined index (UDI). If the UDI determines a scan range to be empty and thus returns the kOutOfBound iteration result during Seek, the iteration result is not propagated up and Prepare() assumes end of file and aborts the remaining scans. This results in incorrect behavior and unpredictable multi scan results.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13960
Test Plan: Add unit test to table_test.cc
Reviewed By: xingbowang
Differential Revision: D82590892
Pulled By: anand1976
fbshipit-source-id: 8cfaaae2bb1a9509ddf8ec967cb8a8801748413d
Summary:
If user_defined_index_factory in BlockBasedTableOptions is configured and we try to open an SST file without the corresponding UDI (either during DB open or file ingestion), ignore a failure to load the UDI by default. If fail_if_no_udi_on_open in BlockBasedTableOptions is true, then treat it as a fatal error.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13921
Test Plan: Update unit tests
Reviewed By: xingbowang
Differential Revision: D81826054
Pulled By: anand1976
fbshipit-source-id: f4fe0b13ccb02b9448622af487680131e349c52b
Summary:
Allow a user defined index to be configured from a string
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13880
Test Plan: Add a unit test in table_test.cc
Reviewed By: bikash-c
Differential Revision: D80237701
Pulled By: anand1976
fbshipit-source-id: 8b3d0bcdfbb4bb76803916ea1b1f940a4d985dfd
Summary:
The original intention of the User Defined Index interface was to use the user key. However, the implementation mixed user and internal key usage. This PR makes it consistent. It also clarifies the UDI contract.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13865
Test Plan: Update tests in table_test.cc
Reviewed By: pdillinger
Differential Revision: D80050344
Pulled By: anand1976
fbshipit-source-id: ace47737d21684ec19709640a09e198cee2d98bd
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13846
This diff addresses few issues that was identified during testing of the user defined index.
1. During the finishing of the index blocks, we run into an infinite loop because the user defined index wrapper returns
early on incomplete status. This happens because the wrapper blindly returns the status if it not OK. But, the status
could legitimately be `Incomplete()` for some indices like Partitioned Index (serving as the internal index for the UDI
wrapper). Fix is to exclude `Incomplete()` check from the status check early in the UDI wrapper's finish.
2. Once we fixed (1), we noticed that the meta blocks for the UDI-based index writer were not written out to the final
SST file. This is because the UDI's meta blocks are created after the internal index's meta blocks and the block-based
index builder didn't account for this. The fix is to finish the UDI wrapper first which will create the necessary meta blocks
and then finish the internal index. If the internal index is incomplete, the block-based index builder should still continue
to write out the meta blocks.
3. OnKeyAdded when delegating to the user-defined index should only pass the user key. The UDI builder doesn't
understand RocksDB's internal key format and while that poses interesting challenges when the UDI is used for non
last level SST files, our plan is to restrict the usage of the UDI to last level files only (for now).
Reviewed By: pdillinger
Differential Revision: D79781453
fbshipit-source-id: 2239c8fc016da55df5c24be6aacc8f6357cab029
Summary:
To better support future options, and changes, we need to convert the std::vector<ScanOptions> to something more malleable.
This diff introduces the MultiScanOptions structure and pipes it through the various points in the code in the Prepare path.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13837
Test Plan:
Ensure all associated tests pass
```
make check all
```
Reviewed By: cbi42
Differential Revision: D79655229
Pulled By: krhancoc
fbshipit-source-id: 3a90fb7420e9655021de85ed0158b866f8bfba05
Summary:
The main motivation for this change is to more flexibly and efficiently support compressing data without extra copies when we do not want to support saving compressed data that is LARGER than the uncompressed. We believe pretty strongly that for the various workloads served by RocksDB, it is well worth a single byte compression marker so that we have the flexibility to save compressed or uncompressed data when compression is attempted. Why? Compression algorithms can add tens of bytes in fixed overheads and percents of bytes in relative overheads. It is also an advantage for the reader when they can bypass decompression, including at least a buffer copy in most cases, after reading just one byte.
The block-based table format in RocksDB follows this model with a single-byte compression marker, and at least after https://github.com/facebook/rocksdb/pull/13797 so does CompressedSecondaryCache. (Notably, the blob file format DOES NOT. This is left to follow-up work.)
In particular, Compressor::CompressBlock now takes in a fixed size buffer for output rather than a `std::string*`. CompressBlock itself rejects the compression if the output would not fit in the provided buffer. This also works well with `max_compressed_bytes_per_kb` option to reject compression even sooner if its ratio is insufficient (implemented in this change). In the future we might use this functionality to reduce a buffer copy (in many cases) into the WritableFileWriter buffer of the block based table builder.
This is a large change because we needed to (or were compelled to)
* Update all the existing callers of CompressBlock, sometimes with substantial changes. This includes introducing GrowableBuffer to reuse between calls rather than std::string, which (at least in C++17) requires zeroing out data when allocating/growing a buffer.
* Re-implement built-in Compressors (V2; V1 is obsolete) to efficiently implement the new version of the API, no longer wrapping the `OLD_CompressData()` function. The new compressors appropriately leverage the CompressBlock virtual call required for the customization interface and no rely on `switch` on compression type for each block. The implementations are largely adaptations of the old implementations, except
* LZ4 and LZ4HC are notably upgraded to take advantage of WorkingArea (see performance tests). And for simplicity in the new implementation, we are dropping support for some super old versions of the library.
* Getting snappy to work with limited-size output buffer required using the Sink/Source interfaces, which appear to be well supported for a long time and efficient (see performance tests).
* Replace awkward old CompressionManager::GetDecompressorForCompressor with Compressor::GetOptimizedDecompressor (which is optional to implement)
* Small behavior change where we treat lack of support for compression closer to not configuring compression, such as incompatibility with block_align. This is motivated by giving CompressionManager the freedom of determining when compression can be excluded for an entire file despite the configured "compression" type, and thus only surfacing actual incompatibilities not hypothetical ones that might be irrelevant to the CompressionManager (or build configuration). Unit tests in `table_test` and `compact_files_test` required update.
* Some lingering clean up of CompressedSecondaryCache and a re-optimization made possible by compressing into an existing buffer.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13805
Test Plan:
for correctness, existing tests
## Performance Test
As I generally only modified compression paths, I'm using a db_bench write benchmark, with before & after configurations running at the same time. vc=1 means verify_compression=1
```
USE_CLANG=1 DEBUG_LEVEL=0 LIB_MODE=static make -j100 db_bench
SUFFIX=`tty | sed 's|/|_|g'`; for CT in zlib bzip2 none snappy zstd lz4 lz4hc none snappy zstd lz4 bzip2; do for VC in 0 1; do echo "$CT vc=$VC"; (for I in `seq 1 20`; do BIN=/dev/shm/dbbench${SUFFIX}.bin; rm -f $BIN; cp db_bench $BIN; $BIN -db=/dev/shm/dbbench$SUFFIX --benchmarks=fillseq -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=1000 -fifo_compaction_allow_compaction=0 -disable_wal -write_buffer_size=12000000 -format_version=7 -compression_type=$CT -verify_compression=$VC 2>&1 | grep micros/op; done) | awk '{n++; sum += $5;} END { print int(sum / n); }'; done; done
```
zlib vc=0 524198 -> 524904 (+0.1%)
zlib vc=1 430521 -> 430699 (+0.0%)
bzip2 vc=0 61841 -> 60835 (-1.6%)
bzip2 vc=1 49232 -> 48734 (-1.0%)
none vc=0 1802375 -> 1906227 (+5.8%)
none vc=1 1837181 -> 1950308 (+6.2%)
snappy vc=0 1783266 -> 1901461 (+6.6%)
snappy vc=1 1799703 -> 1879660 (+4.4%)
zstd vc=0 1216779 -> 1230507 (+1.1%)
zstd vc=1 996370 -> 1015415 (+1.9%)
lz4 vc=0 1801473 -> 1943095 (+7.9%)
lz4 vc=1 1799155 -> 1935242 (+7.6%)
lz4hc vc=0 349719 -> 1126909 (+222.2%)
lz4hc vc=1 348099 -> 1108933 (+218.6%)
(Repeating the most important ones)
none vc=0 1816878 -> 1952221 (+7.4%)
none vc=1 1813736 -> 1904622 (+5.0%)
snappy vc=0 1794816 -> 1875062 (+4.5%)
snappy vc=1 1789363 -> 1873771 (+4.7%)
zstd vc=0 1202592 -> 1225164 (+1.9%)
zstd vc=1 994322 -> 1016688 (+2.2%)
lz4 vc=0 1786959 -> 1971518 (+10.3%)
lz4 vc=1 1829483 -> 1935871 (+5.8%)
I confirmed manually that the new WorkingArea for LZ4HC makes the huge difference on that one, but not as much difference for LZ4, presumably because LZ4HC uses much larger buffers/structures/whatever for better compression ratios.
Reviewed By: hx235
Differential Revision: D79111736
Pulled By: pdillinger
fbshipit-source-id: 1ce1b14af9f15365f1b6da49906b5073a8cecc14
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13728
The Prepare interface allows the user defined index iterator to prefetch index entries, as well as take custom scan termination criteria specified in the property_bag into account.
Reviewed By: pdillinger
Differential Revision: D76165546
fbshipit-source-id: 83d628598924aa7a60dff7ed62a16ae575b2c8ec
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13727
Add UserDefinedIndexReader and UserDefinedIndexIterator. The BlockBasedTable reads the user defined index meta block during open, verifies the checksum, pins in cache or heap depending on configuration, and allocates a UserDefinedIndexReader object with the contents. Similar to the builder, an IndexReader wrapper is allocated. The wrapper forwards the calls to the native reader and/or user defined index reader as appropriate.
A new option, table_index_name, in ReadOptions specifies the index to use when creating a new Iterator.
Reviewed By: pdillinger
Differential Revision: D76165694
fbshipit-source-id: c30bde4c5ce91ea3dc9ad302e73fe4963c1ed457
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13726
Add UserDefinedIndexFactory and UserDefinedIndexBuilder interfaces to allow users to plugin custom index implementation into block based table. The factory is specified in BlockBasedTableOptions. If non-null, BlockBasedTableBuilder allocates a wrapper index builder encapsulating the native index and the custom index. The custom index is exposed to BlockBasedTableBuilder as a meta_block of type kUserDefinedIndex. This block type is not compressed.
The IndexBuilder OnKeyAdded interface is enhanced to accept the value in addition to the key. Only full values are supported, and parallel compression is not supported since we cannot obtain the value when calling OnKeyAdded.
Reviewed By: pdillinger
Differential Revision: D76165614
fbshipit-source-id: dfad9cbd6d0359987b7f4abe64cae58c472836f9
Summary:
Respect the scan upper bound/limit, if specified, in `MultiScan`. This applies to block based table and other native RocksDB SSTs. In order to properly support it, the `MultiScan` object caches the `ReadOptions` passed by the user and sets the `iterate_upper_bound` as appropriate. We optimize for the case of either all scans specifying the upper bound, or none of them. In case of mixed scans, we reallocate the DB iterator anytime `ReadOptions` has to be updated.
Tests:
New unit tests in `db_iterator_test`
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13723
Reviewed By: cbi42
Differential Revision: D77385049
Pulled By: anand1976
fbshipit-source-id: 9c02d125770cbedbe6e8c10767ba537e7f7540e1
Summary:
* Make new format_version=7 a supported setting.
* Fix a bug in compressed_secondary_cache.cc that is newly exercised by custom compression types and showing up in crash test with tiered secondary cache
* Small change to handling of disabled compression in fv=7: use empty compression manager compatibility name.
* Get rid of GetDefaultBuiltinCompressionManager() in public API because it could cause unexpected+unsafe schema change on a user's CompressionManager if built upon the default built-in manager and we add a new built-in schema. Now must be referenced by explicit compression schema version in the public API. (That notion was already exposed in compressed secondary cache API, for better or worse.)
* Improve some error messages for compression misconfiguration
* Improve testing with ObjectLibrary and CompressionManagers
* Improve testing of compression_name table property in BlockBasedTableTest.BlockBasedTableProperties2
* Improve some comments
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13713
Test Plan: existing and updated tests. Notably, the crash test has already been running with (unpublished) format_version=7
Reviewed By: mszeszko-meta, hx235
Differential Revision: D77035482
Pulled By: pdillinger
fbshipit-source-id: 95278de8734a79706a22361bff2184b1edb230ca
Summary:
The Object registry requires object to be allocated as std::unique_ptr. Hence we provide a new API for external table plugins to allocate and return a unique_ptr ExternalTableFactory wrapper.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13694
Reviewed By: jaykorean
Differential Revision: D76767974
Pulled By: anand1976
fbshipit-source-id: ac59c523a11679ca7c9f0b280325c7873c6b4c07
Summary:
This change builds on https://github.com/facebook/rocksdb/issues/13540 and https://github.com/facebook/rocksdb/issues/13626 in allowing a CompressionManager / Compressor / Decompressor to use a custom compression algorithm, with a distinct CompressionType. For background, review the API comments on CompressionManager and its CompatibilityName() function.
Highlights:
* Reserve and name 127 new CompressionTypes that can be used for custom compression algorithms / schemas. In many or most cases I expect the enumerators such as `kCustomCompression8F` to be used in user code rather than casting between integers and CompressionTypes, as I expect the supported custom compression algorithms to be identifiable / enumerable at compile time.
* When using these custom compression types, a CompressionManager must use a CompatibilityName() other than the built-in one AND new format_version=7 (see below).
* When building new SST files, track the full set of CompressionTypes actually used (usually just one aside from kNoCompression), using our efficient bitset SmallEnumSet, which supports fast iteration over the bits set to 1. Ideally, to support mixed or non-mixed compression algorithms in a file as efficiently as possible, we would know the set of CompressionTypes as SST file open time.
* New schema for `TableProperties::compression_name` in format_version=7 to represent the CompressionManager's CompatibilityName(), the set of CompressionTypes used, and potentially more in the future, while keeping the data relatively human-readable.
* It would be possible to do this without a new format_version, but then the only way to ensure incompatible versions fail is with an unsupported CompressionType tag, not with a compression_name property. Therefore, (a) I prefer not to put something misleading in the `compression_name` property (a built-in compression name) when there is nuance because of a CompressionManager, and (b) I prefer better, more consistent error messages that refer to either format_version or the CompressionManager's CompatibilityName(), rather than an unrecognized custom CompressionType value (which could have come from various CompressionManagers).
* The current configured CompressionManager is passed in to TableReaders so that it (or one it knows about) can be used if it matches the CompatibilityName() used for compression in the SST file. Until the connection with ObjectRegistry is implemented, the only way to read files generated with a particular CompressionManager using custom compression algorithms is to configure it (or a known relative; see FindCompatibleCompressionManager()) in the ColumnFamilyOptions.
* Optimized snappy compression with BuiltinDecompressorV2SnappyOnly, to offset some small added overheads with the new tracking. This is essentially an early part of the planned refactoring that will get rid of the old internal compression APIs.
* Another small optimization in eliminating an unnecessary key copy in flush (builder.cc).
* Fix some handling of named CompressionManagers in CompressionManager::CreateFromString() (problem seen in https://github.com/facebook/rocksdb/issues/13647)
Smaller things:
* Adds Name() and GetId() functions to Compressor for debugging/logging purposes. (Compressor and Decompressor are not expected to be Customizable because they are only instantiated by a CompressionManager.)
* When using an explicit compression_manager, the GetId() of the CompressionManager and the Compressor used to build the file are stored as bonus entries in the compression_options table property. This table property is not parsed anywhere, so it is currently for human reading, but still could be parsed with the new underscore-prefixed bonus entries. IMHO, this is preferable to additional table properties, which would increase memory fragmentation in the TableProperties objects and likely take slightly more CPU on SST open and slightly more storage.
* ReleaseWorkingArea() function from protected to public to make wrappers work, because of a quirk in C++ (vs. Java) in which you cannot access protected members of another instance of the same class (sigh)
* Added `CompressionManager:: SupportsCompressionType()` for early options sanity checking.
Follow-up before release:
* Make format_version=7 official / supported
* Stress test coverage
Sooner than later:
* Update tests for RoundRobinManager and SimpleMixedCompressionManager to take advantage of e.g. set of compression types in compression_name property
* ObjectRegistry stuff
* Refactor away old internal compression APIs
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13659
Test Plan:
Basic unit test added.
## Performance
### SST write performance
```
SUFFIX=`tty | sed 's|/|_|g'`; for ARGS in "-compression_type=none" "-compression_type=snappy" "-compression_type=zstd" "-compression_type=snappy -verify_compression=1" "-compression_type=zstd -verify_compression=1" "-compression_type=zstd -compression_max_dict_bytes=8180"; do echo $ARGS; (for I in `seq 1 20`; do BIN=/dev/shm/dbbench${SUFFIX}.bin; rm -f $BIN; cp db_bench $BIN; $BIN -db=/dev/shm/dbbench$SUFFIX --benchmarks=fillseq -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=1000 -fifo_compaction_allow_compaction=0 -disable_wal -write_buffer_size=12000000 -format_version=7 $ARGS 2>&1 | grep micros/op; done) | awk '{n++; sum += $5;} END { print int(sum / n); }'; done
```
Ops/sec, Before -> After, both fv=6:
-compression_type=none
1894386 -> 1858403 (-2.0%)
-compression_type=snappy
1859131 -> 1807469 (-2.8%)
-compression_type=zstd
1191428 -> 1214374 (+1.9%)
-compression_type=snappy -verify_compression=1
1861819 -> 1858342 (+0.2%)
-compression_type=zstd -verify_compression=1
979435 -> 995870 (+1.6%)
-compression_type=zstd -compression_max_dict_bytes=8180
905349 -> 940563 (+3.9%)
Ops/sec, Before fv=6 -> After fv=7:
-compression_type=none
1879365 -> 1836159 (-2.3%)
-compression_type=snappy
1865460 -> 1830916 (-1.9%)
-compression_type=zstd
1191428 -> 1210260 (+1.6%)
-compression_type=snappy -verify_compression=1
1866756 -> 1818989 (-2.6%)
-compression_type=zstd -verify_compression=1
982640 -> 997129 (+1.5%)
-compression_type=zstd -compression_max_dict_bytes=8180
912608 -> 937248 (+2.7%)
### SST read performance
Create DBs
```
for COMP in none snappy zstd; do echo $ARGS; ./db_bench -db=/dev/shm/dbbench-7-$COMP --benchmarks=fillseq,flush -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=1000 -fifo_compaction_allow_compaction=0 -disable_wal -write_buffer_size=12000000 -compression_type=$COMP -format_version=7; done
```
And test
```
for COMP in none
snappy zstd none; do echo $COMP; (for I in `seq 1 8`; do ./db_bench -readonly -db=/dev/shm/dbbench
-7-$COMP --benchmarks=readrandom -num=10000000 -duration=20 -threads=8 2>&1 | grep micros/op; done
) | awk '{n++; sum += $5;} END { print int(sum / n); }'; done
```
Ops/sec, Before -> After (both fv=6)
none
1491732 -> 1500209 (+0.6%)
snappy
1157216 -> 1169202 (+1.0%)
zstd
695414 -> 703719 (+1.2%)
none (again)
1491787 -> 1528789 (+2.4%)
Ops/sec, Before fv=6 -> After fv=7:
none
1492278 -> 1508668 (+1.1%)
snappy
1140769 -> 1152613 (+1.0%)
zstd
696437 -> 696511 (+0.0%)
none (again)
1500585 -> 1512037 (+0.7%)
Overall, I think we can take the read CPU improvement in exchange for the hit (in some cases) on background write CPU
Reviewed By: hx235
Differential Revision: D76520739
Pulled By: pdillinger
fbshipit-source-id: e73bd72502ff85c8779cba313f26f7d1fd50be3a
Summary:
Some tests were failing due to apparent missing include of iomanip. I suspect this was from a gtest upgrade, because in open source, the include iomanip comes from gtest.h. To ensure we maintain compatibility with older gtest as well as the newer one, I pulled the include iomanip out of the in-repo gtest.h. Note that other places in gtest code only instantiate floating-point related templates with `float` and `double` types.
Also, to avoid `make format` being insanely slow on gtest.h, I've excluded third-party from the formatting check.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13661
Test Plan: make check, internal CI, manually ensure formatting check works outside of third-party/
Reviewed By: jaykorean
Differential Revision: D75963897
Pulled By: pdillinger
fbshipit-source-id: ed5737dd456e74068185f1ac5d57046d7509df7a
Summary:
Also revamping test
GeneralTableTest::ApproximateOffsetOfCompressed so that it's not sensitive to adding new metadata to SST files
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13646
Test Plan: manually inspect new table property, which is not parsed anywhere, just for information to human reader
Reviewed By: hx235
Differential Revision: D75561241
Pulled By: pdillinger
fbshipit-source-id: c076c01a8b540bc4cb771964d48fa919c4c48ae4
Summary:
Remove the dependency on `allow_db_generated_files` option in `IngestExternalFile` to be set for ingesting external tables. The files are created by SstFileWriter, and we should be able to ingest them. We could make it work by having the external table implementation provide the version and global sequence number related properties, but its safer to have RocksDB generate the table properties block and store it as is in the file.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13608
Test Plan: Add unit test to test basic ingestion and ingestion with atomic_replace_range
Reviewed By: pdillinger
Differential Revision: D74830707
Pulled By: anand1976
fbshipit-source-id: 4a9bea4a4f38f7c24c584262095c5c98cd771ddc
Summary:
Adds new classes etc. in internal compression.h that are intended to become public APIs for supporting custom/pluggable compression. Some steps remain to allow for pluggable compression and to remove a lot of legacy code (e.g. now called `OLD_CompressData` and `OLD_UncompressData`), but this change refactors the key integration points of SST building and reading and compressed secondary cache over to the new APIs.
Compared with the proposed https://github.com/facebook/rocksdb/issues/7650, this fixes a number of issues including
* Making a clean divide between public and internal APIs (currently just indicated with comments)
* Enough generality that built-in compressions generally fit into the framework rather than needing special treatment
* Avoid exposing obnoxious idioms like `compress_format_version` to the user.
* Enough generality that a compressor mixing algorithms/strategies from other compressors is pretty well supported without an extra schema layer
* Explicit thread-safety contracts (carefully considered)
* Contract details around schema compatibility and extension with code changes (more detail in next PR)
* Customizable "working areas" (e.g. for ZSTD "context")
* Decompression into an arbitrary memory location (rather than involving the decompressor in memory allocation; should facilitate reducing number of objects in block cache)
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13540
Test Plan:
This is currently an internal refactor. More testing will come when the new API is migrated to the public API. A test in db_block_cache_test is updated to meaningfully cover a case (cache warming compression dictionary block) that was previously only covered in the crash test.
SST write performance test, like https://github.com/facebook/rocksdb/issues/13583. Compile with CLANG, run before & after simultaneously:
```
SUFFIX=`tty | sed 's|/|_|g'`; for ARGS in "-compression_parallel_threads=1 -compression_type=none" "-compression_parallel_threads=1 -compression_type=snappy" "-compression_parallel_threads=1 -compression_type=zstd" "-compression_parallel_threads=1 -compression_type=zstd -verify_compression=1" "-compression_parallel_threads=1 -compression_type=zstd -compression_max_dict_bytes=8180" "-compression_parallel_threads=4 -compression_type=snappy"; do echo $ARGS; (for I in `seq 1 20`; do ./db_bench -db=/dev/shm/dbbench$SUFFIX --benchmarks=fillseq -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=1000 -fifo_compaction_allow_compaction=0 -disable_wal -write_buffer_size=12000000 $ARGS 2>&1 | grep micros/op; done) | awk '{n++; sum += $5;} END { print int(sum / n); }'; done
```
Before (this PR and with https://github.com/facebook/rocksdb/issues/13583 reverted):
-compression_parallel_threads=1 -compression_type=none
1908372
-compression_parallel_threads=1 -compression_type=snappy
1926093
-compression_parallel_threads=1 -compression_type=zstd
1208259
-compression_parallel_threads=1 -compression_type=zstd -verify_compression=1
997583
-compression_parallel_threads=1 -compression_type=zstd -compression_max_dict_bytes=8180
934246
-compression_parallel_threads=4 -compression_type=snappy
1644849
After:
-compression_parallel_threads=1 -compression_type=none
1956054 (+2.5%)
-compression_parallel_threads=1 -compression_type=snappy
1911433 (-0.8%)
-compression_parallel_threads=1 -compression_type=zstd
1205668 (-0.3%)
-compression_parallel_threads=1 -compression_type=zstd -verify_compression=1
999263 (+0.2%)
-compression_parallel_threads=1 -compression_type=zstd -compression_max_dict_bytes=8180
934322 (+0.0%)
-compression_parallel_threads=4 -compression_type=snappy
1642519 (-0.2%)
Pretty neutral change(s) overall.
SST read performance test (related to https://github.com/facebook/rocksdb/issues/13583). Set up:
```
for COMP in none snappy zstd; do echo $ARGS; ./db_bench -db=/dev/shm/dbbench-$COMP --benchmarks=fillseq,flush -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=1000 -fifo_compaction_allow_compaction=0 -disable_wal -write_buffer_size=12000000 -compression_type=$COMP; done
```
Test (compile with CLANG, run before & after simultaneously):
```
for COMP in none snappy zstd; do echo $COMP; (for I in `seq 1 5`; do ./db_bench -readonly -db=/dev/shm/dbbench-$COMP --benchmarks=readrandom -num=10000000 -duration=20 -threads=8 2>&1 | grep micros/op; done) | awk '{n++; sum += $5;} END { print int(sum / n); }'; done
```
Before (this PR and with https://github.com/facebook/rocksdb/issues/13583 reverted):
none
1495646
snappy
1172443
zstd
706036
zstd (after constructing with -compression_max_dict_bytes=8180)
656182
After:
none
1494981 (-0.0%)
snappy
1171846 (-0.1%)
zstd
696363 (-1.4%)
zstd (after constructing with -compression_max_dict_bytes=8180)
667585 (+1.7%)
Pretty neutral.
Reviewed By: hx235
Differential Revision: D74626863
Pulled By: pdillinger
fbshipit-source-id: dc8ff3178da9b4eaa7c16aa1bb910c872afaf14a
Summary:
This PR fixes a bug where the file checksum for an external table file was not being calculated by SstFileWriter. The checksum is calculated in WritableFileWriter, so we need to pass that the the external table builder rather than the FSWritableFile pointer directly. However, WritableFileWriter is private to RocksDB, so wrap it in an FSWritableFile and pass it.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13591
Test Plan: Add a new test in table_test.cc
Reviewed By: jaykorean
Differential Revision: D74410563
Pulled By: anand1976
fbshipit-source-id: c7fa8142e20da8836589dee5fa50919951cf4046
Summary:
A multi scan API for users to pass a set of scan ranges and have the table readers determine the optimal strategy for performing the scans. This might include coalescing of IOs across scans, for example. The requested scans should be in increasing key order. The scan start keys and other info is passed to NewMultiScanIterator, which in turn uses the newly added Prepare() interface in Iterator to update the iterator. The Prepare() takes a vector of ScanOptions, which contain the start keys and optional upper bounds, as well as user defined parameters in the property_bag taht are passed through as is to external table readers.
The initial implementation plumbs this through to the ExternalTableReader. This PR also fixes an issue of premature destruction of the external table iterator after the first scan of the multi-scan. The `LevelIterator` treats an invalid iterator as a potential end of file and destroys the table iterator in order to move to the next file. To prevent that, this PR defines the `NextAndGetResult` interface that the external table iterator must implement. The result returned by `NextAndGetResult` differentiates between iterator invalidation due to out of bound vs end of file.
Eventually, I envision the `MultiScanIterator` to be built on top of a producer-consumer queue like container, with RocksDB (producer) enqueueing keys and values into the container and the application (consumer) dequeueing them. Unlike a traditional producer consumer queue, there is no concurrency here. The results will be buffered in the container, and when the buffer is empty a new batch will be read from the child iterators. This will allow the virtual function call overhead to be amortized over many entries.
TODO (in future PRs):
1. Update the internal implementation of Prepare to trim the ScanOptions range based on the intersection with the table key range, taking into consideration unbounded scans and opaque user defined bounds.
2. Long term, take advantage of Prepare in BlockBasedTableIterator, atleast for the upper bound case.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13473
Reviewed By: pdillinger
Differential Revision: D71447559
Pulled By: anand1976
fbshipit-source-id: 31668abb0c529aa1ac1738ae46c36cbddf9148f1
Summary:
In hopes of eventually removing some ugly and awkard code for compress_format_version < 2, users can no longer write files in that format and its read support is marked deprecated. For continuing to test that read support, there is a back door to writing the files in unit tests.
If format_version < 2 is specified, it is quietly sanitized to 2. (This is similar to other BlockBasedTableOptions.)
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13463
Test Plan: unit tests updated.
Reviewed By: hx235
Differential Revision: D71152916
Pulled By: pdillinger
fbshipit-source-id: 95be55e86f93f09fd898223578b9381385c3ccd8
Summary:
This PR adds the ability to use an ExternalTableBuilder through the SstFileWriter to create external tables. This is a counterpart to https://github.com/facebook/rocksdb/issues/13401 , which adds the ExternalTableReader. The support for external tables is confined to ingestion only DBs, with external table files ingested into the bottommost level only. https://github.com/facebook/rocksdb/issues/13431 enforces ingestion only DBs by adding a disallow_memtable_writes column family option.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13434
Test Plan: New unit tests in table_test.cc
Reviewed By: pdillinger
Differential Revision: D70532054
Pulled By: anand1976
fbshipit-source-id: a837487eadfabed9627a0eceb403bfc5fc2c427c
Summary:
This PR introduces an interface to plug in an external table file reader into RocksDB. The external table reader may support custom file formats that might work better for a specific use case compared to RocksDB native formats. This initial version allows the external table file to be loaded and queried using an `SstFileReader`. In the near future, we will allow it to be used with a limited RocksDB instance that allows bulkload but not live writes.
The model of a DB using an external table reader is a read only database allowing bulkload and atomic replace in the bottommost level only. Live writes, if supported in the future, are expected to use block based table files in higher levels. Tombstones, merge operands, and non-zero sequence numbers are expected to be present only in non-bottommost levels. External table files are assumed to have only Puts, and all keys implicitly have sequence number 0.
TODO (in future PRs) -
1. Add support for external file ingestion, with safety mechanisms to prevent accidental writes
2. Add support for atomic column family replace
3. Allow custom table file extensions
4. Add a TableBuilder interface for use with `SstFileWriter`
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13401
Reviewed By: pdillinger
Differential Revision: D69689351
Pulled By: anand1976
fbshipit-source-id: c5d5b92d56fd4d0fc43a77c4ceb0463d4f479bda
Summary:
add a new transaction option `TransactionOptions::commit_bypass_memtable` that will ingest the transaction into a DB as an immutable memtables, skipping memtable writes during transaction commit. This helps to reduce the blocking time of committing a large transaction, which is mostly spent on memtable writes. The ingestion is done by creating WBWIMemTable using transaction's underlying WBWI, and ingest it as the latest immutable memtable. The feature will be experimental.
Major changes are:
1. write path change to ingest the transaction, mostly in WriteImpl() and IngestWBWI() in db_impl_write.cc.
2. WBWI changes to track some per CF stats like entry count and overwritten single deletion count, and track which keys have overwritten single deletions (see 3.). Per CF stat is used to precompute the number of entries in each WBWIMemTable.
3. WBWIMemTable Iterator changes to emit overwritten single deletions. The motivation is explained in the comment above class WBWIMemTable definition. The rest of the changes in WBWIMemTable are moving the iterator definition around.
Some intended follow ups:
1. support for merge operations
2. stats/logging around this option
3. tests improvement, including stress test support for the more comprehensive no_batched_op_stress.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13144
Test Plan:
* added new unit tests
* enabled in multi_ops_txns_stress test
* Benchmark: applying the change in 8222c0cafc4c6eb3a0d05807f7014b44998acb7a, I tested txn size of 10k and check perf context for write_memtable_time, write_wal_time and key_lock_wait_time(repurposed for transaction unlock time). Though the benchmark result number can be flaky, this shows memtable write time improved a lot (more than 100 times). The benchmark also shows that the remaining commit latency is from transaction unlock.
```
./db_bench --benchmarks=fillrandom --seed=1727376962 --threads=1 --disable_auto_compactions=1 --max_write_buffer_number=100 --min_write_buffer_number_to_merge=100 --writes=100000 --batch_size=10000 --transaction_db=1 --perf_level=4 --enable_pipelined_write=false --commit_bypass_memtable=1
commit_bypass_memtable = false
fillrandom : 3.982 micros/op 251119 ops/sec 0.398 seconds 100000 operations; 27.8 MB/s PERF_CONTEXT:
write_memtable_time = 116950422
write_wal_time = 8535565
txn unlock time = 32979883
commit_bypass_memtable = true
fillrandom : 2.627 micros/op 380559 ops/sec 0.263 seconds 100000 operations; 42.1 MB/s PERF_CONTEXT:
write_memtable_time = 740784
write_wal_time = 11993119
txn unlock time = 21735685
```
Reviewed By: jowlyzhang
Differential Revision: D66307632
Pulled By: cbi42
fbshipit-source-id: 6619af58c4c537aed1f76c4a7e869fb3f5098999
Summary:
This PR adds support for reusing the file system provided buffer to avoid an extra `memcpy` into RockDB's buffer. This optimization has already been implemented for point lookups, as well as compaction and scan reads _when prefetching is disabled_.
This PR extends this optimization to work with synchronous prefetching (`num_buffers == 1`). Asynchronous prefetching can be addressed in a future PR (and probably should be to keep this PR from growing too large).
Remarks
- To handle the case where the main buffer only has part of the requested data, I used the existing `overlap_buf_` (currently used in the async prefetching case) instead of defining a separate buffer. This was discussed in https://github.com/facebook/rocksdb/pull/13118#discussion_r1842839360.
- We use `MultiRead` with a single request to take advantage of the file system buffer. This is consistent with previous work (e.g. https://github.com/facebook/rocksdb/pull/12266).
- Even without the tests I added, there was some code coverage inside in at least `DBIOCorruptionTest.IterReadCorruptionRetry`, since those tests were failing before I addressed a bug in my code for this PR. [Run with failed test](https://github.com/facebook/rocksdb/actions/runs/11708830448/job/32611508818?pr=13118).
- This prefetching code is not too easy to follow, so I added quite a bit of comments to both the code and test case to try to make it easier to understand the exact internal state of the prefetch buffer at every point in time.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13118
Test Plan:
I wrote pretty thorough unit tests that cover synchronous prefetching with file system buffer reuse. The flows for partial hits, complete hits, and complete misses are tested. I also parametrized the test to make sure the async prefetching (without file system buffer reuse) still work as expected.
Once we agree on the changes, I will run a long stress test before merging.
Reviewed By: anand1976
Differential Revision: D65559101
Pulled By: archang19
fbshipit-source-id: 1a56d846e918c20a009b83f1371c1791f69849ae
Summary:
This PR does two things:
1. Adds a new table property `newest_key_time`
2. Uses this property to improve TTL and temperature change compaction.
### Context
The current `creation_time` table property should really be named `oldest_ancestor_time`. For flush output files, this is the oldest key time in the file. For compaction output files, this is the minimum among all oldest key times in the input files.
The problem with using the oldest ancestor time for TTL compaction is that we may end up dropping files earlier than we should. What we really want is the newest (i.e. "youngest") key time. Right now we take a roundabout way to estimate this value -- we take the value of the _oldest_ key time for the _next_ (newer) SST file. This is also why the current code has checks for `index >= 1`.
Our new property `newest_key_time` is set to the file creation time during flushes, and the max over all input files for compactions.
There were some additional smaller changes that I had to make for testing purposes:
- Refactoring the mock table reader to support specifying my own table properties
- Refactoring out a test utility method `GetLevelFileMetadatas` that would otherwise be copy/pasted in 3 places
Credit to cbi42 for the problem explanation and proposed solution
### Testing
- Added a dedicated unit test to my `newest_key_time` logic in isolation (i.e. are we populating the property on flush and compaction)
- Updated the existing unit tests (for TTL/temperate change compaction), which were comprehensive enough to break when I first made my code changes. I removed the test setup code which set the file metadata `oldest_ancestor_time`, so we know we are actually only using the new table property instead.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13083
Reviewed By: cbi42
Differential Revision: D65298604
Pulled By: archang19
fbshipit-source-id: 898ef91b692ab33f5129a2a16b64ecadd4c32432
Summary:
This is setting up for a fix to a data race in SetOptions on BlockBasedTableOptions (BBTO), https://github.com/facebook/rocksdb/issues/10079
The race will be fixed by replacing `table_factory` with a modified copy whenever we want to modify a BBTO field.
An argument could be made that this change creates more entaglement between features (e.g. BlobSource <-> MutableCFOptions), rather than (conceptually) minimizing the dependencies of each feature, but
* Most of these things already depended on ImmutableOptions
* Historically there has been a lot of plumbing (and possible small CPU overhead) involved in adding features that need to reach a lot of places, like `block_protection_bytes_per_key`. Keeping those wrapped up in options simplifies that.
* SuperVersion management generally takes care of lifetime management of MutableCFOptions, so is not that difficult. (Crash test agrees so far.)
There are some FIXME places where it is known to be unsafe to replace `block_cache` unless/until we handle shared_ptr tracking properly. HOWEVER, replacing `block_cache` is generally dubious, at least while existing users of the old block cache (e.g. table readers) can continue indefinitely.
The change to cf_options.cc is essentially just moving code (not changing).
I'm not concerned about the performance of copying another shared_ptr with MutableCFOptions, but I left a note about considering an improvement if more shared_ptr are added to it.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13077
Test Plan:
existing tests, crash test.
Unit test DBOptionsTest.GetLatestCFOptions updated with some temporary logic. MemoryTest required some refactoring (simplification) for the change.
Reviewed By: cbi42
Differential Revision: D64546903
Pulled By: pdillinger
fbshipit-source-id: 69ae97ce5cf4c01b58edc4c5d4687eb1e5bf5855
Summary:
With some new use cases onboarding to prefix extractors/seek/filters, one of the risks is existing iterator code, e.g. for maintenance tasks, being unintentionally subject to prefix seek semantics. This is a longstanding known design flaw with prefix seek, and `prefix_same_as_start` and `auto_prefix_mode` were steps in the direction of making that obsolete. However, we can't just immediately set `total_order_seek` to true by default, because that would impact so much code instantly.
Here we add a new DB option, `prefix_seek_opt_in_only` that basically allows users to transition to the future behavior when they are ready. When set to true, all iterators will be treated as if `total_order_seek=true` and then the only ways to get prefix seek semantics are with `prefix_same_as_start` or `auto_prefix_mode`.
Related fixes / changes:
* Make sure that `prefix_same_as_start` and `auto_prefix_mode` are compatible with (or override) `total_order_seek` (depending on your interpretation).
* Fix a bug in which a new iterator after dynamically changing the prefix extractor might mix different prefix semantics between memtable and SSTs. Both should use the latest extractor semantics, which means iterators ignoring memtable prefix filters with an old extractor. And that means passing the latest prefix extractor to new memtable iterators that might use prefix seek. (Without the fix, the test added for this fails in many ways.)
Suggested follow-up:
* Investigate a FIXME where a MergeIteratorBuilder is created in db_impl.cc. No unit test detects a change in value that should impact correctness.
* Make memtable prefix bloom compatible with `auto_prefix_mode`, which might require involving the memtablereps because we don't know at iterator creation time (only seek time) whether an auto_prefix_mode seek will be a prefix seek.
* Add `prefix_same_as_start` testing to db_stress
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13026
Test Plan:
tests updated, added. Add combination of `total_order_seek=true` and `auto_prefix_mode=true` to stress test. Ran `make blackbox_crash_test` for a long while.
Manually ran tests with `prefix_seek_opt_in_only=true` as default, looking for unexpected issues. I inspected most of the results and migrated many tests to be ready for such a change (but not all).
Reviewed By: ltamasi
Differential Revision: D63147378
Pulled By: pdillinger
fbshipit-source-id: 1f4477b730683d43b4be7e933338583702d3c25e
Summary:
**Context/Summary:**
https://github.com/facebook/rocksdb/pull/12542 introduced a bug where wrong padded bytes used to generate file checksum if flush happens during padding. This PR fixed it along with an existing same bug for `perform_data_verification_=true`.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/12598
Test Plan:
- New UT that failed before this fix (`db->VerifyFileChecksums: ...Corruption: ...file checksum mismatch`) and passes after
- Benchmark
```
TEST_TMPDIR=/dev/shm ./db_bench --benchmarks=fillseq[-X300] --num=100000 --block_align=1 --compression_type=none
```
Pre-PR:
fillseq [AVG 300 runs] : 421334 (± 4126) ops/sec; 46.6 (± 0.5) MB/sec
Post-PR: (no regression observed but a slight improvement)
fillseq [AVG 300 runs] : 425768 (± 4309) ops/sec; 47.1 (± 0.5) MB/sec
Reviewed By: ajkr, anand1976
Differential Revision: D56725688
Pulled By: hx235
fbshipit-source-id: c1a700a95def8c65c0a21e44f8c1966164925ad5
Peter Dillinger