Cleanup SuperVersion in Iterator::Refresh() (#10770)

Summary:
Fix a bug in Iterator::Refresh() where the local SV it obtained could be obsolete upon return, and should be cleaned up.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/10770

Test Plan: added a unit test to reproduce the issue.

Reviewed By: ajkr

Differential Revision: D40063809

Pulled By: ajkr

fbshipit-source-id: 619e728eb0f1ac9540b4d0ad38e43acc37a514b2
main
Changyu Bi 2 years ago committed by Facebook GitHub Bot
parent edda219fc3
commit ffde463a5f
  1. 1
      HISTORY.md
  2. 5
      db/arena_wrapped_db_iter.cc
  3. 22
      db/db_iterator_test.cc

@ -9,6 +9,7 @@
* Fixed an optimistic transaction validation bug caused by DBImpl::GetLatestSequenceForKey() returning non-latest seq for merge (#10724). * Fixed an optimistic transaction validation bug caused by DBImpl::GetLatestSequenceForKey() returning non-latest seq for merge (#10724).
* Fixed a bug in iterator refresh which could segfault for DeleteRange users (#10739). * Fixed a bug in iterator refresh which could segfault for DeleteRange users (#10739).
* Fixed a bug causing manual flush with `flush_opts.wait=false` to stall when database has stopped all writes (#10001). * Fixed a bug causing manual flush with `flush_opts.wait=false` to stall when database has stopped all writes (#10001).
* Fixed a bug in iterator refresh that was not freeing up SuperVersion, which could cause excessive resource pinniung (#10770).
### Performance Improvements ### Performance Improvements
* Try to align the compaction output file boundaries to the next level ones, which can reduce more than 10% compaction load for the default level compaction. The feature is enabled by default, to disable, set `AdvancedColumnFamilyOptions.level_compaction_dynamic_file_size` to false. As a side effect, it can create SSTs larger than the target_file_size (capped at 2x target_file_size) or smaller files. * Try to align the compaction output file boundaries to the next level ones, which can reduce more than 10% compaction load for the default level compaction. The feature is enabled by default, to disable, set `AdvancedColumnFamilyOptions.level_compaction_dynamic_file_size` to false. As a side effect, it can create SSTs larger than the target_file_size (capped at 2x target_file_size) or smaller files.

@ -90,6 +90,7 @@ Status ArenaWrappedDBIter::Refresh() {
// Refresh range-tombstones in MemTable // Refresh range-tombstones in MemTable
if (!read_options_.ignore_range_deletions) { if (!read_options_.ignore_range_deletions) {
SuperVersion* sv = cfd_->GetThreadLocalSuperVersion(db_impl_); SuperVersion* sv = cfd_->GetThreadLocalSuperVersion(db_impl_);
TEST_SYNC_POINT_CALLBACK("ArenaWrappedDBIter::Refresh:SV", nullptr);
auto t = sv->mem->NewRangeTombstoneIterator( auto t = sv->mem->NewRangeTombstoneIterator(
read_options_, latest_seq, false /* immutable_memtable */); read_options_, latest_seq, false /* immutable_memtable */);
if (!t || t->empty()) { if (!t || t->empty()) {
@ -107,7 +108,7 @@ Status ArenaWrappedDBIter::Refresh() {
} else { // current mutable memtable has range tombstones } else { // current mutable memtable has range tombstones
if (!memtable_range_tombstone_iter_) { if (!memtable_range_tombstone_iter_) {
delete t; delete t;
cfd_->ReturnThreadLocalSuperVersion(sv); db_impl_->ReturnAndCleanupSuperVersion(cfd_, sv);
// The memtable under DBIter did not have range tombstone before // The memtable under DBIter did not have range tombstone before
// refresh. // refresh.
reinit_internal_iter(); reinit_internal_iter();
@ -119,7 +120,7 @@ Status ArenaWrappedDBIter::Refresh() {
&cfd_->internal_comparator(), nullptr, nullptr); &cfd_->internal_comparator(), nullptr, nullptr);
} }
} }
cfd_->ReturnThreadLocalSuperVersion(sv); db_impl_->ReturnAndCleanupSuperVersion(cfd_, sv);
} }
// Refresh latest sequence number // Refresh latest sequence number
db_iter_->set_sequence(latest_seq); db_iter_->set_sequence(latest_seq);

@ -3232,6 +3232,28 @@ TEST_F(DBIteratorTest, BackwardIterationOnInplaceUpdateMemtable) {
} }
} }
TEST_F(DBIteratorTest, IteratorRefreshReturnSV) {
Options options = CurrentOptions();
options.disable_auto_compactions = true;
DestroyAndReopen(options);
ASSERT_OK(
db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(), "a", "z"));
std::unique_ptr<Iterator> iter{db_->NewIterator(ReadOptions())};
SyncPoint::GetInstance()->SetCallBack(
"ArenaWrappedDBIter::Refresh:SV", [&](void*) {
ASSERT_OK(db_->Put(WriteOptions(), "dummy", "new SV"));
// This makes the local SV obselete.
ASSERT_OK(Flush());
SyncPoint::GetInstance()->DisableProcessing();
});
SyncPoint::GetInstance()->EnableProcessing();
ASSERT_OK(iter->Refresh());
iter.reset();
// iter used to not cleanup SV, so the Close() below would hit an assertion
// error.
Close();
}
} // namespace ROCKSDB_NAMESPACE } // namespace ROCKSDB_NAMESPACE
int main(int argc, char** argv) { int main(int argc, char** argv) {

Loading…
Cancel
Save