search

facebook / rocksdb

A library that provides an embeddable, persistent key-value store for fast storage.
http://rocksdb.org
GNU General Public License v2.0
28.52k stars 6.3k forks source link

max_successive_merges and allow_concurrent_memtable_write compatibility #8502

Closed akrylysov closed 3 years ago

akrylysov commented 3 years ago

Currently RocksDB doesn't prevent setting max_successive_merges to a value above 0 and enabling allow_concurrent_memtable_write at the same time.

There is only a check for unordered_write and max_successive_merges: https://github.com/facebook/rocksdb/blob/9dc887ece05860f091ceef25c7f61c3359de0cda/db/column_family.cc#L1339

Is max_successive_merges not compatible with allow_concurrent_memtable_write and the options check is missing, or the following asserts are incorrect https://github.com/facebook/rocksdb/blob/d89483098ff4a77bd0ef12454a64554805984cc5/db/write_batch.cc#L1994?

ajkr commented 3 years ago

Should be incompatible. I expect two concurrent kTypeMerge updates could both read the key, do a full merge, and insert the merged result as a kTypeValue. Only the newer kTypeValue would be visible which excludes data from one of the kTypeMerge.

Unfortunately our stress test can't catch it since a thread locks a key while writing it. Also writing a unit test repro feels not that attractive since we wouldn't check it in for a disabled feature combination. So I'll probably just disable the feature combo.

ajkr commented 3 years ago

Actually, can you confirm whether you hit the assertion? It looks like this code should prevent creating a MemTableInserter for parallel insertion even if the DB config has allow_concurrent_memtable_write == true:

https://github.com/facebook/rocksdb/blob/0b75b223212ca19a07ad6259b37450c287ff34ae/db/db_impl/db_impl_write.cc#L256-L286

akrylysov commented 3 years ago

My unit tests couldn't hit the assertion, but the combination was never tested under a production load.

akrylysov commented 3 years ago

I double checked db_impl_write.cc, it seems to be handling the case. Thanks for the details! Closing the issue.

asad-awadia commented 2 years ago

@ajkr just to clarify @akrylysov comment of "it seems to be handling the case."

  1. I can't have both max_successive_merges > 0 and allow_concurrent_memtable_write enabled at the same time?

  2. What happens if I do specify it in the options? Will one get ignored all the time?