From 67f071faded6738fc970d9fb8c965f632b5ee0a5 Mon Sep 17 00:00:00 2001 From: Bo Wang Date: Fri, 18 Feb 2022 14:10:25 -0800 Subject: [PATCH] Fixes #9565 (#9586) Summary: [Compaction::IsTrivialMove](https://github.com/facebook/rocksdb/blob/a2b9be42b6d5ac4d44bcc6a9451a825440000769/db/compaction/compaction.cc#L318) checks whether allow_trivial_move is set, and if so it returns the value of is_trivial_move_. The allow_trivial_move option is there for universal compaction. So when this is set and leveled compaction is enabled, then useful code that follows this block never gets a chance to run. A check that [compaction_style == kCompactionStyleUniversal](https://github.com/facebook/rocksdb/blob/320d9a8e8a1b6998f92934f87fc71ad8bd6d4596/db/db_impl/db_impl_compaction_flush.cc#L1030) should be added to avoid doing the wrong thing for leveled. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9586 Test Plan: To reproduce this: First edit db/compaction/compaction.cc with ``` diff --git a/db/compaction/compaction.cc b/db/compaction/compaction.cc index 7ae50b91e..52dd489b1 100644 --- a/db/compaction/compaction.cc +++ b/db/compaction/compaction.cc @@ -319,6 +319,8 @@ bool Compaction::IsTrivialMove() const { // input files are non overlapping if ((mutable_cf_options_.compaction_options_universal.allow_trivial_move) && (output_level_ != 0)) { + printf("IsTrivialMove:: return %d because universal allow_trivial_move\n", (int) is_trivial_move_); + // abort(); return is_trivial_move_; } ``` And then run ``` ./db_bench --benchmarks=fillseq --allow_concurrent_memtable_write=false --level0_file_num_compaction_trigger=4 --level0_slowdown_writes_trigger=20 --level0_stop_writes_trigger=30 --max_background_jobs=8 --max_write_buffer_number=8 --db=/data/m/rx --wal_dir=/data/m/rx --num=800000000 --num_levels=8 --key_size=20 --value_size=400 --block_size=8192 --cache_size=51539607552 --cache_numshardbits=6 --compression_max_dict_bytes=0 --compression_ratio=0.5 --compression_type=lz4 --bytes_per_sync=8388608 --cache_index_and_filter_blocks=1 --cache_high_pri_pool_ratio=0.5 --benchmark_write_rate_limit=0 --write_buffer_size=16777216 --target_file_size_base=16777216 --max_bytes_for_level_base=67108864 --verify_checksum=1 --delete_obsolete_files_period_micros=62914560 --max_bytes_for_level_multiplier=8 --statistics=0 --stats_per_interval=1 --stats_interval_seconds=20 --histogram=1 --memtablerep=skip_list --bloom_bits=10 --open_files=-1 --subcompactions=1 --compaction_style=0 --min_level_to_compress=3 --level_compaction_dynamic_level_bytes=true --pin_l0_filter_and_index_blocks_in_cache=1 --soft_pending_compaction_bytes_limit=167503724544 --hard_pending_compaction_bytes_limit=335007449088 --min_level_to_compress=0 --use_existing_db=0 --sync=0 --threads=1 --memtablerep=vector --allow_concurrent_memtable_write=false --disable_wal=1 --seed=1641328309 --universal_allow_trivial_move=1 ``` Example output with the debug code added ``` IsTrivialMove:: return 0 because universal allow_trivial_move IsTrivialMove:: return 0 because universal allow_trivial_move ``` After this PR, the bug is fixed. Reviewed By: ajkr Differential Revision: D34350451 Pulled By: gitbw95 fbshipit-source-id: 3232005cc47c40a7e75d316cfc7960beb5bdff3a --- db/compaction/compaction.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/db/compaction/compaction.cc b/db/compaction/compaction.cc index 7ae50b91e..20361d3f9 100644 --- a/db/compaction/compaction.cc +++ b/db/compaction/compaction.cc @@ -318,7 +318,8 @@ bool Compaction::IsTrivialMove() const { // Used in universal compaction, where trivial move can be done if the // input files are non overlapping if ((mutable_cf_options_.compaction_options_universal.allow_trivial_move) && - (output_level_ != 0)) { + (output_level_ != 0) && + (cfd_->ioptions()->compaction_style == kCompactionStyleUniversal)) { return is_trivial_move_; }