diff --git a/db/db_impl.cc b/db/db_impl.cc index cd727b6c3..8c8ababb6 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -894,6 +894,7 @@ InternalIterator* DBImpl::NewInternalIterator( range_del_agg); } } + TEST_SYNC_POINT_CALLBACK("DBImpl::NewInternalIterator:StatusCallback", &s); if (s.ok()) { // Collect iterators for files in L0 - Ln if (read_options.read_tier != kMemtableTier) { @@ -907,8 +908,10 @@ InternalIterator* DBImpl::NewInternalIterator( internal_iter->RegisterCleanup(CleanupIteratorState, cleanup, nullptr); return internal_iter; + } else { + CleanupSuperVersion(super_version); } - return NewErrorInternalIterator(s); + return NewErrorInternalIterator(s, arena); } ColumnFamilyHandle* DBImpl::DefaultColumnFamily() const { @@ -1819,21 +1822,23 @@ SuperVersion* DBImpl::GetAndRefSuperVersion(uint32_t column_family_id) { return GetAndRefSuperVersion(cfd); } +void DBImpl::CleanupSuperVersion(SuperVersion* sv) { + // Release SuperVersion + if (sv->Unref()) { + { + InstrumentedMutexLock l(&mutex_); + sv->Cleanup(); + } + delete sv; + RecordTick(stats_, NUMBER_SUPERVERSION_CLEANUPS); + } + RecordTick(stats_, NUMBER_SUPERVERSION_RELEASES); +} + void DBImpl::ReturnAndCleanupSuperVersion(ColumnFamilyData* cfd, SuperVersion* sv) { - bool unref_sv = !cfd->ReturnThreadLocalSuperVersion(sv); - - if (unref_sv) { - // Release SuperVersion - if (sv->Unref()) { - { - InstrumentedMutexLock l(&mutex_); - sv->Cleanup(); - } - delete sv; - RecordTick(stats_, NUMBER_SUPERVERSION_CLEANUPS); - } - RecordTick(stats_, NUMBER_SUPERVERSION_RELEASES); + if (!cfd->ReturnThreadLocalSuperVersion(sv)) { + CleanupSuperVersion(sv); } } diff --git a/db/db_impl.h b/db/db_impl.h index 067a2f591..30866fba1 100644 --- a/db/db_impl.h +++ b/db/db_impl.h @@ -477,6 +477,9 @@ class DBImpl : public DB { // mutex is held. SuperVersion* GetAndRefSuperVersion(uint32_t column_family_id); + // Un-reference the super version and clean it up if it is the last reference. + void CleanupSuperVersion(SuperVersion* sv); + // Un-reference the super version and return it to thread local cache if // needed. If it is the last reference of the super version. Clean it up // after un-referencing it. diff --git a/db/db_iterator_test.cc b/db/db_iterator_test.cc index 7828d39b2..e7a3534e3 100644 --- a/db/db_iterator_test.cc +++ b/db/db_iterator_test.cc @@ -1997,6 +1997,19 @@ TEST_F(DBIteratorTest, Refresh) { iter.reset(); } +TEST_F(DBIteratorTest, CreationFailure) { + SyncPoint::GetInstance()->SetCallBack( + "DBImpl::NewInternalIterator:StatusCallback", [](void* arg) { + *(reinterpret_cast(arg)) = Status::Corruption("test status"); + }); + SyncPoint::GetInstance()->EnableProcessing(); + + Iterator* iter = db_->NewIterator(ReadOptions()); + ASSERT_FALSE(iter->Valid()); + ASSERT_TRUE(iter->status().IsCorruption()); + delete iter; +} + } // namespace rocksdb int main(int argc, char** argv) { diff --git a/table/merging_iterator.cc b/table/merging_iterator.cc index d48f8ebc9..99a867fe7 100644 --- a/table/merging_iterator.cc +++ b/table/merging_iterator.cc @@ -387,10 +387,20 @@ MergeIteratorBuilder::MergeIteratorBuilder( new (mem) MergingIterator(comparator, nullptr, 0, true, prefix_seek_mode); } +MergeIteratorBuilder::~MergeIteratorBuilder() { + if (first_iter != nullptr) { + first_iter->~InternalIterator(); + } + if (merge_iter != nullptr) { + merge_iter->~MergingIterator(); + } +} + void MergeIteratorBuilder::AddIterator(InternalIterator* iter) { if (!use_merging_iter && first_iter != nullptr) { merge_iter->AddIterator(first_iter); use_merging_iter = true; + first_iter = nullptr; } if (use_merging_iter) { merge_iter->AddIterator(iter); @@ -400,13 +410,15 @@ void MergeIteratorBuilder::AddIterator(InternalIterator* iter) { } InternalIterator* MergeIteratorBuilder::Finish() { + InternalIterator* ret = nullptr; if (!use_merging_iter) { - return first_iter; + ret = first_iter; + first_iter = nullptr; } else { - auto ret = merge_iter; + ret = merge_iter; merge_iter = nullptr; - return ret; } + return ret; } } // namespace rocksdb diff --git a/table/merging_iterator.h b/table/merging_iterator.h index 0509d1dce..04fcf421d 100644 --- a/table/merging_iterator.h +++ b/table/merging_iterator.h @@ -40,7 +40,7 @@ class MergeIteratorBuilder { // arena: where the merging iterator needs to be allocated from. explicit MergeIteratorBuilder(const InternalKeyComparator* comparator, Arena* arena, bool prefix_seek_mode = false); - ~MergeIteratorBuilder() {} + ~MergeIteratorBuilder(); // Add iter to the merging iterator. void AddIterator(InternalIterator* iter);