BaseDeltaIterator: always check valid() before accessing key() #4702

miasantreble · 2018-11-20T22:27:38Z

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
Also recommit #4656 (It was reverted earlier due to bugs)

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ajkr

lgtm

ajkr · 2018-11-20T23:51:31Z

utilities/write_batch_with_index/write_batch_with_index.cc

@@ -262,6 +260,7 @@ class BaseDeltaIterator : public Iterator {
  }
  bool BaseValid() const { return base_iterator_->Valid(); }
  bool DeltaValid() const { return delta_iterator_->Valid(); }
+  bool valid() const { return (current_at_base_ ? BaseValid() : DeltaValid()); }


how about a name that differs other than capitalization, like BaseDeltaValid?

siying · 2018-11-20T23:59:53Z

As discussed offline, I suggest we revert the original change for now. We only check in the fix if we add the coverage of upper bound in WriteBatchWithIndexTest.TestRandomIteraratorWithBase

miasantreble · 2018-11-21T00:30:48Z

accidentally hit land to master button but looks like land failed due to the network sev. Closing for now, will reopen once test coverage has been added

facebook-github-bot · 2018-11-21T22:39:26Z

@miasantreble has updated the pull request. Re-import the pull request

miasantreble · 2018-11-21T22:45:19Z

Added unit test for iterate_upper_bound in NewIteratorWithBase. Specifically for the case that crashed mysql test: seek to a key outside of upper bound.
Also tried to add test coverage in TestRandomIteraratorWithBase, but the problem is that the iterator KVIter used for validating the result does not take ReadOptions or iterate_upper_bound, making it hard to verify the correctness of the BaseDeltaIterator

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2018-11-29T05:55:44Z

@miasantreble has updated the pull request. Re-import the pull request

miasantreble · 2018-11-29T05:56:53Z

@siying randomized test added, also fixed a bug uncovered by randomized test.
PTAL

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying

SeekForPrev() using a key not smaller than upper bound is undefined behavior. I think we don't have to complicate the code to handle this case. Rather, we can avoid this in randomize test.

siying · 2018-11-29T18:34:12Z

utilities/write_batch_with_index/write_batch_with_index_test.cc

@@ -506,7 +507,17 @@ typedef std::map<std::string, std::string> KVMap;
 class KVIter : public Iterator {
 public:
  explicit KVIter(const KVMap* map) : map_(map), iter_(map_->end()) {}
-  virtual bool Valid() const { return iter_ != map_->end(); }
+  explicit KVIter(const KVMap* map, const Slice* iterate_upper_bound) : map_(map), iter_(map_->end()), iterate_upper_bound_(iterate_upper_bound) {}


siying · 2018-11-29T18:34:28Z

utilities/write_batch_with_index/write_batch_with_index_test.cc

+    if (iterate_upper_bound_ == nullptr) {
+      return iter_ != map_->end();
+    }
+    else {


facebook-github-bot · 2018-11-30T06:15:57Z

@miasantreble has updated the pull request. Re-import the pull request

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2018-11-30T18:02:20Z

@miasantreble has updated the pull request. Re-import the pull request

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2018-11-30T18:10:55Z

utilities/write_batch_with_index/write_batch_with_index.cc

@@ -293,6 +293,10 @@ class BaseDeltaIterator : public Iterator {
        if (delta_entry.type == kDeleteRecord ||
            delta_entry.type == kSingleDeleteRecord) {
          AdvanceDelta();
+          current_over_upper_bound_ = BaseDeltaValid() && IsOverUpperBound();
+          if (current_over_upper_bound_) {
+            break;


Why not just return from the function here?

siying · 2018-11-30T18:19:00Z

utilities/write_batch_with_index/write_batch_with_index.cc

@@ -293,6 +293,10 @@ class BaseDeltaIterator : public Iterator {
        if (delta_entry.type == kDeleteRecord ||
            delta_entry.type == kSingleDeleteRecord) {
          AdvanceDelta();
+          current_over_upper_bound_ = BaseDeltaValid() && IsOverUpperBound();


The use of BaseDeltaValid() here is somehow confusing. We are advancing delta only in the line above, without updating current_at_base_, we use BaseDeltaValid(). We know it has under if (!BaseValid()), current_at_base_ won't change, but still, someone trying to understand the code needs to make efforts to understand what's going on here.
It will be slightly better to assert(!current_at_base_) above this line, and add a comment to explain why.

facebook-github-bot · 2018-11-30T18:53:01Z

@miasantreble has updated the pull request. Re-import the pull request

facebook-github-bot

@miasantreble has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…iterator… (facebook#4705)" This reverts commit a21cb22. Recommit a21cb22 as a fix is now available

facebook-github-bot · 2018-11-30T21:16:33Z

@miasantreble has updated the pull request. Re-import the pull request

facebook-github-bot

@miasantreble is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook#4702)" This reverts commit 3a18bb3.

#4744) Summary: …) (#4702)" This reverts commit 3a18bb3. Pull Request resolved: #4744 Differential Revision: D13311869 Pulled By: miasantreble fbshipit-source-id: 6300b12cc34828d8b9274e907a3aef1506d5d553

…#4702 again)

facebook-github-bot added the CLA Signed label Nov 20, 2018

facebook-github-bot reviewed Nov 20, 2018

View reviewed changes

ajkr approved these changes Nov 20, 2018

View reviewed changes

miasantreble closed this Nov 21, 2018

miasantreble reopened this Nov 21, 2018

facebook-github-bot reviewed Nov 22, 2018

View reviewed changes

miasantreble force-pushed the fix-mysql-crash branch from 7c9d5ff to e816822 Compare November 29, 2018 05:55

miasantreble requested a review from siying November 29, 2018 05:56

facebook-github-bot reviewed Nov 29, 2018

View reviewed changes

siying approved these changes Nov 29, 2018

View reviewed changes

facebook-github-bot reviewed Nov 30, 2018

View reviewed changes

siying reviewed Nov 30, 2018

View reviewed changes

siying approved these changes Nov 30, 2018

View reviewed changes

facebook-github-bot reviewed Nov 30, 2018

View reviewed changes

miasantreble added 6 commits November 30, 2018 13:16

Revert "Revert "apply ReadOptions.iterate_upper_bound to transaction …

4de1bc9

…iterator… (facebook#4705)" This reverts commit a21cb22. Recommit a21cb22 as a fix is now available

BaseDeltaIterator: always check valid() before accessing key()

4abb8a6

use better names for valid()

3e3a324

add test coverage for seek to a key outside of upper bound

81cff5a

add randomised test for iterate_upper_bound

b113fe7

fix a hidden bug found by randomized test

b399bd9

miasantreble added 3 commits November 30, 2018 13:16

fix formatting and break from loop once for delete keys

cc2d825

trivial fix

0b00ce3

address review comments

b257d8f

miasantreble force-pushed the fix-mysql-crash branch from 868bbb2 to b257d8f Compare November 30, 2018 21:16

facebook-github-bot reviewed Nov 30, 2018

View reviewed changes

facebook-github-bot closed this in 3a18bb3 Nov 30, 2018

miasantreble deleted the fix-mysql-crash branch November 30, 2018 23:37

miasantreble added a commit to miasantreble/rocksdb that referenced this pull request Dec 3, 2018

Revert "BaseDeltaIterator: always check valid() before accessing key() (

e7e2627

facebook#4702)" This reverts commit 3a18bb3.

miasantreble mentioned this pull request Dec 3, 2018

Revert "BaseDeltaIterator: always check valid() before accessing key(… #4744

Closed

miasantreble added a commit to miasantreble/rocksdb that referenced this pull request Mar 26, 2019

implement iterate_upper_bound for txn iterators (try to land facebook…

14436a2

…#4702 again)

miasantreble mentioned this pull request Mar 26, 2019

implement iterate_upper_bound for txn iterators #5105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BaseDeltaIterator: always check valid() before accessing key() #4702

BaseDeltaIterator: always check valid() before accessing key() #4702

miasantreble commented Nov 20, 2018 •

edited

Loading

facebook-github-bot left a comment

ajkr left a comment

ajkr Nov 20, 2018

siying commented Nov 20, 2018

miasantreble commented Nov 21, 2018

facebook-github-bot commented Nov 21, 2018

miasantreble commented Nov 21, 2018

facebook-github-bot left a comment

facebook-github-bot commented Nov 29, 2018

miasantreble commented Nov 29, 2018

facebook-github-bot left a comment

siying left a comment

siying Nov 29, 2018

siying Nov 29, 2018

facebook-github-bot commented Nov 30, 2018

facebook-github-bot left a comment

facebook-github-bot commented Nov 30, 2018

facebook-github-bot left a comment

siying Nov 30, 2018

siying Nov 30, 2018

facebook-github-bot commented Nov 30, 2018

facebook-github-bot left a comment

facebook-github-bot commented Nov 30, 2018

facebook-github-bot left a comment

BaseDeltaIterator: always check valid() before accessing key() #4702

BaseDeltaIterator: always check valid() before accessing key() #4702

Conversation

miasantreble commented Nov 20, 2018 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

ajkr Nov 20, 2018

Choose a reason for hiding this comment

siying commented Nov 20, 2018

miasantreble commented Nov 21, 2018

facebook-github-bot commented Nov 21, 2018

miasantreble commented Nov 21, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 29, 2018

miasantreble commented Nov 29, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

siying Nov 29, 2018

Choose a reason for hiding this comment

siying Nov 29, 2018

Choose a reason for hiding this comment

facebook-github-bot commented Nov 30, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 30, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying Nov 30, 2018

Choose a reason for hiding this comment

siying Nov 30, 2018

Choose a reason for hiding this comment

facebook-github-bot commented Nov 30, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 30, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

miasantreble commented Nov 20, 2018 •

edited

Loading