andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

stop populating unused/invalid MergingIterator heaps (#8975)

Browse Source 
Summary:
I was looking at https://github.com/facebook/rocksdb/issues/2636 and got very confused that `MergingIterator::AddIterator()` is populating `min_heap_` with dangling pointers. There is justification in the comments that `min_heap_` will be cleared before it's used, but it'd be cleaner to not populate it with dangling pointers in the first place. Also made similar change in the constructor for consistency, although the pointers there would not be dangling, just unused.

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

Test Plan: rely on existing tests

Reviewed By: pdillinger, hx235

Differential Revision: D31273767

Pulled By: ajkr

fbshipit-source-id: 127ca9dd1f82f77f55dd0c3f19511de3282fc229
This commit is contained in:
Andrew Kryczka 2021-10-07 15:23:14 -07:00 committed by Facebook GitHub Bot
parent fcaa7ff638
commit c0ec58ecb9
1 changed files with 3 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
table/merging_iterator.cc
View File

@ -50,10 +50,6 @@ class MergingIterator : public InternalIterator {
for (int i = 0; i < n; i++) {
children_[i].Set(children[i]);
}
for (auto& child : children_) {
AddToMinHeapOrCheckStatus(&child);
}
current_ = CurrentForward();
}
void considerStatus(Status s) {
@ -63,16 +59,13 @@ class MergingIterator : public InternalIterator {
}
virtual void AddIterator(InternalIterator* iter) {
assert(direction_ == kForward);
children_.emplace_back(iter);
if (pinned_iters_mgr_) {
iter->SetPinnedItersMgr(pinned_iters_mgr_);
}
auto new_wrapper = children_.back();
AddToMinHeapOrCheckStatus(&new_wrapper);
if (new_wrapper.Valid()) {
current_ = CurrentForward();
}
// Invalidate to ensure `Seek*()` is called to construct the heaps before
// use.
current_ = nullptr;
}
~MergingIterator() override {