implement iterate_upper_bound for txn iterators #5105
Conversation
miasantreble
changed the title
miasantreble
force-pushed
the
txn-iterator-upper-bound
branch
from
af5fbcd to
848036d
Compare
There was a problem hiding this comment.
@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@miasantreble has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Did we understand why the randomized tests failed to catch the bug exposed by MyRocks now? |
|
I think it's because inside the infinite loop, the update to |
|
@miasantreble the question is, why didn't the randomized test catch it? |
| @@ -264,32 +276,38 @@ class BaseDeltaIterator : public Iterator { | |||
| } else if (!delta_iterator_->status().ok()) { | |||
| // Expose the error status and stop. | |||
| current_at_base_ = false; | |||
| return; | |||
| break; | |||
There was a problem hiding this comment.
It's safer to just return here. I am worried about by mistake we access the data while status is not OK. It always makes things hard to reason about.
| } | ||
| equal_keys_ = false; | ||
| if (!BaseValid()) { | ||
| if (!base_iterator_->status().ok()) { | ||
| // Expose the error status and stop. | ||
| current_at_base_ = true; | ||
| return; | ||
| break; |
There was a problem hiding this comment.
Same here. Safer to just return.
| } | ||
|
|
||
| // Base has finished. | ||
| if (!DeltaValid()) { | ||
| // Finished | ||
| return; | ||
| break; |
There was a problem hiding this comment.
Both of delta and base are at the end. A good idea to return.
| DeltaValid() && IsOverUpperBound(delta_iterator_->Entry().key); | ||
| if (current_over_upper_bound_) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Do we consider to do the check inside AdvanceDelta() instead?
|
@miasantreble has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
ran mysqltest using latest commit 495e6c7 and no crash found |
|
@miasantreble I think I asked the question twice, but why didn't the randomized test catch the bug we found in MyRocks test? Or would it catch it? |
|
Do you consider to move the check to AdvanceDelta() and leaves UpdateCurrent() alone? |
Current implementation of current_over_upper_bound_ fails to take into consideration that keys might be invalid in either base iterator or delta iterator. Calling key() in such scenario will lead to assertion failure and runtime errors.
This PR addresses the bug by adding check for valid keys before calling IsOverUpperBound(), also added test coverage for iterate_upper_bound usage in BaseDeltaIterator
(trying to land PR #4702 once again)
Test plan: