From eef27d00486c45a52eaaa2b628415ade5603c6c9 Mon Sep 17 00:00:00 2001 From: Akanksha Mahajan Date: Wed, 21 Oct 2020 20:16:30 -0700 Subject: [PATCH] Bug fix to remove function calling in assert statement (#7581) Summary: Remove function calling in assert statement as assert is a no op in opt build and that function might not be called. This causes hang in closing RocksDB when refit level is set. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7581 Test Plan: make check -j64 Reviewed By: riversand963 Differential Revision: D24466420 Pulled By: akankshamahajan15 fbshipit-source-id: 97db4ec5a95ae693c3290e176a3c12a9b1ad2f6d --- HISTORY.md | 1 + db/db_impl/db_impl.cc | 3 ++- db/db_impl/db_impl_compaction_flush.cc | 3 ++- db/forward_iterator.cc | 14 ++++++-------- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 07c8035e1..2851f5373 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -6,6 +6,7 @@ * Since 6.12, memtable lookup should report unrecognized value_type as corruption (#7121). * Since 6.14, fix false positive flush/compaction `Status::Corruption` failure when `paranoid_file_checks == true` and range tombstones were written to the compaction output files. * Since 6.14, fix a bug that could cause a stalled write to crash with mixed of slowdown and no_slowdown writes (`WriteOptions.no_slowdown=true`). +* Fixed a bug which causes hang in closing DB when refit level is set in opt build. It was because ContinueBackgroundWork() was called in assert statement which is a no op. It was introduced in 6.14. ### Public API Change * Deprecate `BlockBasedTableOptions::pin_l0_filter_and_index_blocks_in_cache` and `BlockBasedTableOptions::pin_top_level_index_and_filter`. These options still take effect until users migrate to the replacement APIs in `BlockBasedTableOptions::metadata_cache_options`. Migration guidance can be found in the API comments on the deprecated options. diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 50e8d711a..38c476c1e 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -4720,7 +4720,8 @@ Status DBImpl::CreateColumnFamilyWithImport( temp_s.ToString().c_str()); } // Always returns Status::OK() - assert(DestroyColumnFamilyHandle(*handle).ok()); + temp_s = DestroyColumnFamilyHandle(*handle); + assert(temp_s.ok()); *handle = nullptr; } return status; diff --git a/db/db_impl/db_impl_compaction_flush.cc b/db/db_impl/db_impl_compaction_flush.cc index 0ea4d7d0d..95855e4c7 100644 --- a/db/db_impl/db_impl_compaction_flush.cc +++ b/db/db_impl/db_impl_compaction_flush.cc @@ -950,7 +950,8 @@ Status DBImpl::CompactRange(const CompactRangeOptions& options, s = ReFitLevel(cfd, final_output_level, options.target_level); TEST_SYNC_POINT("DBImpl::CompactRange:PostRefitLevel"); // ContinueBackgroundWork always return Status::OK(). - assert(ContinueBackgroundWork().ok()); + Status temp_s = ContinueBackgroundWork(); + assert(temp_s.ok()); } EnableManualCompaction(); } diff --git a/db/forward_iterator.cc b/db/forward_iterator.cc index 4eb48cde4..013af04e9 100644 --- a/db/forward_iterator.cc +++ b/db/forward_iterator.cc @@ -662,10 +662,9 @@ void ForwardIterator::RebuildIterators(bool refresh_sv) { read_options_, sv_->current->version_set()->LastSequence())); range_del_agg.AddTombstones(std::move(range_del_iter)); // Always return Status::OK(). - assert( - sv_->imm - ->AddRangeTombstoneIterators(read_options_, &arena_, &range_del_agg) - .ok()); + Status temp_s = sv_->imm->AddRangeTombstoneIterators(read_options_, &arena_, + &range_del_agg); + assert(temp_s.ok()); } has_iter_trimmed_for_upper_bound_ = false; @@ -728,10 +727,9 @@ void ForwardIterator::RenewIterators() { read_options_, sv_->current->version_set()->LastSequence())); range_del_agg.AddTombstones(std::move(range_del_iter)); // Always return Status::OK(). - assert( - svnew->imm - ->AddRangeTombstoneIterators(read_options_, &arena_, &range_del_agg) - .ok()); + Status temp_s = svnew->imm->AddRangeTombstoneIterators( + read_options_, &arena_, &range_del_agg); + assert(temp_s.ok()); } const auto* vstorage = sv_->current->storage_info();