Summary:
For every merge operand encountered for a key in the read path we now have the ability to decide whether to look further (to retrieve more merge operands for the key) or stop and invoke the merge operator to return the value. The user needs to override `ShouldMerge()` method with a condition to terminate search when true to avail this facility.
This has a couple of advantages:
1. It helps in limiting the number of merge operands that are looked at to compute a value as part of a user Get operation.
2. It allows to peek at a merge key-value to see if further merge operands need to look at.
Example: Limiting the number of merge operands that are looked at: Lets say you have 10 merge operands for a key spread over various levels. If you only want RocksDB to look at the latest two merge operands instead of all 10 to compute the value, it is now possible with this PR. You can set the condition in `ShouldMerge()` to return true when the size of the operand list is 2. Look at the example implementation in the unit test. Without this PR, a Get might look at all the 10 merge operands in different levels before invoking the merge-operator.
Added a new unit test.
Made sure that there is no perf regression by running benchmarks.
Command line to Load data:
```
TEST_TMPDIR=/dev/shm ./db_bench --benchmarks="mergerandom" --merge_operator="uint64add" --num=10000000
...
mergerandom : 12.861 micros/op 77757 ops/sec; 8.6 MB/s ( updates:10000000)
```
**ReadRandomMergeRandom bechmark results:**
Command line:
```
TEST_TMPDIR=/dev/shm ./db_bench --benchmarks="readrandommergerandom" --merge_operator="uint64add" --num=10000000
```
Base -- Without this code change (on commit fc7476b):
```
readrandommergerandom : 38.586 micros/op 25916 ops/sec; (reads:3001599 merges:6998401 total:10000000 hits:842235 maxlength:8)
```
With this code change:
```
readrandommergerandom : 38.653 micros/op 25870 ops/sec; (reads:3001599 merges:6998401 total:10000000 hits:842235 maxlength:8)
```
Closes https://github.com/facebook/rocksdb/pull/2923
Differential Revision: D5898239
Pulled By: sagar0
fbshipit-source-id: daefa325019f77968639a75c851d46352c2303ef
Summary:
There is no need for smart pointers in cf_info_map, so use RAII. This should also placate valgrind.
Closes https://github.com/facebook/rocksdb/pull/2943
Differential Revision: D5932941
Pulled By: asandryh
fbshipit-source-id: 2c37df88573a9df2557880a31193926e4425e054
Summary:
A user encountered segfault on the call to `CacheDependencies()`, probably because `NewIndexIterator()` failed before populating `*index_entry`. Let's avoid the call in that case.
Closes https://github.com/facebook/rocksdb/pull/2939
Differential Revision: D5928611
Pulled By: ajkr
fbshipit-source-id: 484be453dbb00e5e160e9c6a1bc933df7d80f574
Summary:
SUMMARY
Moves the bytes_per_sync and wal_bytes_per_sync options from immutableoptions to mutable options. Also if wal_bytes_per_sync is changed, the wal file and memtables are flushed.
TEST PLAN
ran make check
all passed
Two new tests SetBytesPerSync, SetWalBytesPerSync check that after issuing setoptions with a new value for the var, the db options have the new value.
Closes https://github.com/facebook/rocksdb/pull/2893
Reviewed By: yiwu-arbug
Differential Revision: D5845814
Pulled By: TheRushingWookie
fbshipit-source-id: 93b52d779ce623691b546679dcd984a06d2ad1bd
Summary:
Looks like the API is simply missing. Adding it.
Closes https://github.com/facebook/rocksdb/pull/2937
Differential Revision: D5919955
Pulled By: yiwu-arbug
fbshipit-source-id: 6e2e9c96c29882b0bb4113d1f8efb72bffc57878
Summary:
Context/problem:
- CFs may be flushed at different times
- A WAL can only be deleted after all CFs have flushed beyond end of that WAL.
- Point-in-time recovery might stop upon reaching the first corruption.
- Some CFs may have already flushed beyond that point, while others haven't. We should fail the Open() instead of proceeding with inconsistent CFs.
Closes https://github.com/facebook/rocksdb/pull/2900
Differential Revision: D5863281
Pulled By: miasantreble
fbshipit-source-id: 180dbaf83d96c804cff49b3c406312a4ae61313e
Summary:
Problem:
- `DB::SanitizeOptions` strips trailing slash from `wal_dir` but not `dbname`
- We check whether `wal_dir` and `dbname` refer to the same directory using string equality: https://github.com/facebook/rocksdb/blob/master/db/repair.cc#L258
- Providing `dbname` with trailing slash causes default `wal_dir` to be misidentified as a separate directory.
- Then the repair tries to add all SST files to the `VersionEdit` twice (once for `dbname` dir, once for `wal_dir`) and fails with coredump.
Solution:
- Add a new `Env` function, `AreFilesSame`, which uses device and inode number to check whether files are the same. It's currently only implemented in `PosixEnv`.
- Migrate repair to use `AreFilesSame` to check whether `dbname` and `wal_dir` are same. If unsupported, falls back to string comparison.
Closes https://github.com/facebook/rocksdb/pull/2827
Differential Revision: D5761349
Pulled By: ajkr
fbshipit-source-id: c839d548678b742af1166d60b09abd94e5476238
Summary:
Problem:
During RocksJava performance testing we found that the rocksdb jni library is not built with jemalloc; instead it was getting built with the default glibc malloc. We saw quite a bit of memory bloat due to this.
Addressed this by installing jemalloc-devel package in the vm that we use to build release jars.
Closes https://github.com/facebook/rocksdb/pull/2916
Differential Revision: D5887018
Pulled By: sagar0
fbshipit-source-id: ace0b5d60234b3a30dcd5d39633e7827a5982a50
Summary:
Fix for [2461](https://github.com/facebook/rocksdb/issues/2461).
Problem: When using multiple db_paths setting with RocksDB, RocksDB incorrectly calculates the size of L1 in LevelCompactionBuilder::GetPathId.
max_bytes_for_level_base is used as L0 size and L1 size is calculated as (L0 size * max_bytes_for_level_multiplier). However, L1 size should be max_bytes_for_level_base.
Solution: Use max_bytes_for_level_base as L1 size. Also, use L1 size as the estimated size of L0.
Closes https://github.com/facebook/rocksdb/pull/2903
Differential Revision: D5885442
Pulled By: maysamyabandeh
fbshipit-source-id: 036da1c9298d173b9b80479cc6661ee4b7a951f6
Summary:
it's unsupported in options file, so the flag should be respected by db_bench even when an options file is provided.
Closes https://github.com/facebook/rocksdb/pull/2910
Differential Revision: D5869836
Pulled By: ajkr
fbshipit-source-id: f67f591ae083e95e989f86b6fad50765d2e3d855
Summary:
The test didn't delete txn before creating a new one.
Closes https://github.com/facebook/rocksdb/pull/2913
Differential Revision: D5880236
Pulled By: yiwu-arbug
fbshipit-source-id: 7a4fcaada3d86332292754502cd8f4341143bf4f
Summary:
This option was introduced in the C++ API in RocksDB 5.6 in bb01c1880c . Now, exposing it through RocksJava API.
Closes https://github.com/facebook/rocksdb/pull/2908
Differential Revision: D5864224
Pulled By: sagar0
fbshipit-source-id: 140aa55dcf74b14e4d11219d996735c7fdddf513
Summary:
Hello,
current master branch declares cmake_minimum_required (VERSION 2.8.11)
but cmake gives the following error:
[ 6%] CMake Error at CMakeLists.txt:658 (install):
install TARGETS given unknown argument "INCLUDES".
CMake Error at src/CMakeLists.txt:658 (install): install TARGETS given unknown argument "INCLUDES".
because this argument not supported on CMake versions prior 2.8.12
Closes https://github.com/facebook/rocksdb/pull/2904
Differential Revision: D5863430
Pulled By: yiwu-arbug
fbshipit-source-id: 0f7230e080add472ad4b87836b3104ea0b971a38
Summary:
This is caught when I was testing #2886.
Closes https://github.com/facebook/rocksdb/pull/2907
Differential Revision: D5863153
Pulled By: yiwu-arbug
fbshipit-source-id: 8c54759ba1a0dc101f24ab50423e35731300612d
Summary:
snprintf is defined as _snprintf, which doesn't exist in the std
namespace.
Closes https://github.com/facebook/rocksdb/pull/2298
Differential Revision: D5070457
Pulled By: yiwu-arbug
fbshipit-source-id: 6e1659ac3e86170653b174578da5a8ed16812cbb
Summary:
In our testing cluster, we found large amount tombstone has been promoted to kValue type from kMerge after reaching the top level of compaction. Since we used to only collecting tombstone in merge operator, those tombstones can never be collected.
This PR addresses the issue by adding a GC step in compaction filter, which is only for kValue type records. Since those record already reached the top of compaction (no earlier data exists) we can safely remove them in compaction filter without worrying old data appears.
This PR also removes an old optimization in cassandra merge operator for single merge operands. We need to do GC even on a single operand, so the optimation does not make sense anymore.
Closes https://github.com/facebook/rocksdb/pull/2855
Reviewed By: sagar0
Differential Revision: D5806445
Pulled By: wpc
fbshipit-source-id: 6eb25629d4ce917eb5e8b489f64a6aa78c7d270b
Summary:
By default the seq number in DB is increased once per written key. WritePrepared txns requires the seq to be increased once per the entire batch so that the seq would be used as the prepare timestamp by which the transaction is identified. Also we need to increase seq for the commit marker since it would give a unique id to the commit timestamp of transactions.
Two unit tests are added to verify our understanding of how the seq should be increased. The recovery path requires much more work and is left to another patch.
Closes https://github.com/facebook/rocksdb/pull/2885
Differential Revision: D5837843
Pulled By: maysamyabandeh
fbshipit-source-id: a08960b93d727e1cf438c254d0c2636fb133cc1c
Summary:
Our current implementation of (semi-)copy constructor of DBOptions and ColumnFamilyOptions seems to intend value by value copy, which is what the default copy constructor does anyway. Moreover not using the default constructor has the risk of forgetting to add newly added options.
As an example, allow_2pc seems to be forgotten in the copy constructor which was causing one of the unit tests not seeing its effect.
Closes https://github.com/facebook/rocksdb/pull/2888
Differential Revision: D5846368
Pulled By: maysamyabandeh
fbshipit-source-id: 1ee92a2aeae93886754b7bc039c3411ea2458683
Summary:
After 7f6c02dda1, the same get_perf_context() is called both of internally and externally. However, I found internally this is not got inlined. I don't know why this is the case, but directly referencing perf_context is the logical way to do.
Closes https://github.com/facebook/rocksdb/pull/2892
Differential Revision: D5843789
Pulled By: siying
fbshipit-source-id: b49777d8809f35847699291bb7f8ea2754c3af49
Summary:
Plumbed ReadOptions::iterate_upper_bound through JNI.
Made the following design choices:
* Used Slice instead of AbstractSlice due to the anticipated usecase (key / key prefix). Can change this if anyone disagrees.
* Used Slice instead of raw byte[] which seemed cleaner but necessitated the package-private handle-based Slice constructor. Followed WriteBatch as an example.
* We need a copy constructor for ReadOptions, as we create one base ReadOptions for a particular usecase and clone -> change the iterate_upper_bound on each slice operation. Shallow copy seemed cleanest.
* Hold a reference to the upper bound slice on ReadOptions, in contrast to Snapshot.
Signed a Facebook CLA this morning.
Closes https://github.com/facebook/rocksdb/pull/2872
Differential Revision: D5824446
Pulled By: sagar0
fbshipit-source-id: 74fc51313a10a81ecd348625e2a50ca5b7766888
Summary:
Three small optimizations:
(1) iter_->IsKeyPinned() shouldn't be called if read_options.pin_data is not true. This may trigger function call all the way down the iterator tree.
(2) reuse the iterator key object in DBIter::FindNextUserEntryInternal(). The constructor of the class has some overheads.
(3) Move the switching direction logic in MergingIterator::Next() to a separate function.
These three in total improves readseq performance by about 3% in my benchmark setting.
Closes https://github.com/facebook/rocksdb/pull/2880
Differential Revision: D5829252
Pulled By: siying
fbshipit-source-id: 991aea10c6d6c3b43769cb4db168db62954ad1e3
Summary:
Move uncommon code paths in RangeDelAggregator::ShouldDelete() and IterKey::EnlargeBufferIfNeeded() to a separate function, so that the inlined strcuture can be more optimized.
Optimize it because these places show up in CPU profiling, though minimum. The performance is really hard measure. I ran db_bench with readseq benchmark against in-memory DB many times. The variation is big, but it seems to show 1% improvements.
Closes https://github.com/facebook/rocksdb/pull/2877
Differential Revision: D5828123
Pulled By: siying
fbshipit-source-id: 41a49e229f91e9f8409f85cc6f0dc70e31334e4b
Summary:
Added possibility to set save points in transactions and then rollback to them
Closes https://github.com/facebook/rocksdb/pull/2876
Differential Revision: D5825829
Pulled By: yiwu-arbug
fbshipit-source-id: 62168992340bbcddecdaea3baa2a678475d1429d
Summary:
Remove misleading 'experimental' comment around `level_compaction_dynamic_level_bytes` option. This is not experimental anymore and is ready for wider adoption. MyRocks is already using it in production.
Closes https://github.com/facebook/rocksdb/pull/2878
Differential Revision: D5828890
Pulled By: sagar0
fbshipit-source-id: fffb45f4999f689b7eca326e4f4caf472d40c5a9
Summary:
Bug report: https://www.facebook.com/groups/rocksdb.dev/permalink/1389452781153232/
Non-empty `level0_compactions_in_progress_` was aborting `CompactFiles` after incrementing `bg_compaction_scheduled_`, and in that case we never decremented it. This blocked future compactions and prevented DB close as we wait for scheduled compactions to finish/abort during close.
I eliminated `CompactFiles`'s dependency on `level0_compactions_in_progress_`. Since it takes a contiguous span of L0 files -- through the last L0 file if any L1+ files are included -- it's fine to run in parallel with other compactions involving L0. We make the same assumption in intra-L0 compaction.
Closes https://github.com/facebook/rocksdb/pull/2849
Differential Revision: D5780440
Pulled By: ajkr
fbshipit-source-id: 15b15d3faf5a699aed4b82a58352d4a7bb23e027
Summary:
We had two proposals for lock-free commit maps. This patch implements the latter one that was simpler. We can later experiment with both proposals.
In this impl each entry is an std::atomic of uint64_t, which are accessed via memory_order_acquire/release. In x86_64 arch this is compiled to simple reads and writes from memory.
Closes https://github.com/facebook/rocksdb/pull/2861
Differential Revision: D5800724
Pulled By: maysamyabandeh
fbshipit-source-id: 41abae9a4a5df050a8eb696c43de11c2770afdda
Summary:
Added some bindings for `OptimisticTransactionDB` in C API
Closes https://github.com/facebook/rocksdb/pull/2823
Differential Revision: D5820672
Pulled By: yiwu-arbug
fbshipit-source-id: 7efd17f619cc0741feddd2050b8fc856f9288350
Summary:
test the `DBOptions(const Options&)` and `ColumnFamilyOptions(const Options&)` constructors. Actually this'll work better once we refactor `RandomInitDBOptions` / `RandomInitCFOptions` to use the authoritative sources of struct members: `db_options_type_info` / `cf_options_type_info` (internal task T21804189 for this).
Closes https://github.com/facebook/rocksdb/pull/2873
Differential Revision: D5817141
Pulled By: ajkr
fbshipit-source-id: 8567c20feced9d1751fdf1f4383e2af30f7e3591
Summary:
currently `ImmutableDBOptions::Dump` use default value for `concurrent_prepare` and `manual_wal_flush`, because DBOptions ctor does not init those member variables.
so in LOG file, it will be
```
Options.concurrent_prepare: 0
Options.manual_wal_flush: 0
```
Closes https://github.com/facebook/rocksdb/pull/2864
Differential Revision: D5816240
Pulled By: ajkr
fbshipit-source-id: 82335e8bcae3dceedc6a99224e7998de5fad1e50
Summary:
- Switched all instances of SetMinPossibleForUserKey and SetMaxPossibleForUserKey in accordance to InternalKeyComparator's comparison logic
Closes https://github.com/facebook/rocksdb/pull/2868
Differential Revision: D5804152
Pulled By: axxufb
fbshipit-source-id: 80be35e04f2e8abc35cc64abe1fecb03af24e183
Summary:
because it is not only platform independent
but also allows to override the build date
This helps to make ceph builds reproducible (that includes a fork of rockdb in a submodule)
Also adds UTC flag, to be independent of timezone.
Requires cmake-2.8.11+ from 2013
Closes https://github.com/facebook/rocksdb/pull/2848
Differential Revision: D5820189
Pulled By: yiwu-arbug
fbshipit-source-id: e3e8c1550e10e238c173f6c5d9ba15f71ad3ce28
Summary:
options.enable_pipelined_write and options.concurrent_prepare are incompatible and should not be set together.
Closes https://github.com/facebook/rocksdb/pull/2875
Differential Revision: D5818358
Pulled By: maysamyabandeh
fbshipit-source-id: dad862508f00817ab302f8b61729accf38315fb8
Summary:
There are internal users who open BackupEngine for writing new backups only, and they don't care whether old backups can be read or not. The condition `BackupableDBOptions::max_valid_backups_to_open == 0` should be supported (previously in df74b775e6 I made the mistake of choosing 0 as a special value to disable the limit).
Closes https://github.com/facebook/rocksdb/pull/2819
Differential Revision: D5751599
Pulled By: ajkr
fbshipit-source-id: e73ac19eb5d756d6b68601eae8e43407ee4f2752
Summary:
Changes:
* added check for value before merge is called on code path that should check if key exists
Closes https://github.com/facebook/rocksdb/pull/2814
Reviewed By: IslamAbdelRahman
Differential Revision: D5743966
Pulled By: armishra
fbshipit-source-id: 6ac4283bc510c8ca50827d87ef0ba631f2b33b18
Summary:
it sometimes takes more than 10 minutes (i.e., times out) on our internal CI. mainly because bzip is super slow. so I reduced the amount of work it tries to do.
Closes https://github.com/facebook/rocksdb/pull/2856
Differential Revision: D5795883
Pulled By: ajkr
fbshipit-source-id: e69f986ae60b44ecc26b6b024abd0f13bdf3a3c5
Summary:
As discussed in #2742 , this pull-requests brings the iterator's [SeekForPrev()](https://github.com/facebook/rocksdb/wiki/SeekForPrev) functionality to the java-api. It affects all locations in the code where previously only Seek() was supported.
All code changes are essentially a copy & paste of the already existing implementations for Seek().
**Please Note**: the changes to the C++ code were applied without fully understanding its effect, so please take a closer look. However, since Seek() and SeekForPrev() provide exactly the same signature, I do not expect any mistake here.
The java-tests are extended by new tests for the additional functionality.
Compilation (`make rocksdbjavastatic`) and test (`java/make test`) run without errors.
Closes https://github.com/facebook/rocksdb/pull/2747
Differential Revision: D5721011
Pulled By: sagar0
fbshipit-source-id: c1f951cddc321592c70dd2d32bc04892f3f119f8
Summary:
Merging iterator invokes InternalKeyComparator.Compare() frequently to heap merge. By making InternalKeyComparator final and merging iterator to directly use InternalKeyComparator rather than through Iterator interface, we can give compiler a choice to avoid one more virtual function call if possible. I ran readseq benchmark in memory-only use case to make sure the performance at least doesn't regress.
I have to disable the final key word in debug build, as a hack test class depends on overriding the class.
Closes https://github.com/facebook/rocksdb/pull/2860
Differential Revision: D5800461
Pulled By: siying
fbshipit-source-id: ab876f22a09bb5c560740911412336e0e25ccb53
Summary:
DBIter is referenced in ArenaWrappedDBIter, which is a simple wrapper. If DBIter is final, some virtual function call can be avoided. Some functions can even be inlined, like DBIter.value() to ArenaWrappedDBIter.value() and DBIter.key() to ArenaWrappedDBIter.key(). The performance gain is hard to measure. I just ran the memory-only benchmark for readseq and saw it didn't regress. There shouldn't be any harm doing it. Just give compiler more choices.
Closes https://github.com/facebook/rocksdb/pull/2859
Differential Revision: D5799888
Pulled By: siying
fbshipit-source-id: 829788f91310c40282dcfb7e412e6ef489931143
