[Commits] d89e1604438: Reset iterator after point lookup

psergey sergey at mariadb.com
Mon May 17 17:30:04 EEST 2021


revision-id: d89e160443823d0f07691e99067e15b06d532ccc (percona-202102-50-gd89e1604438)
parent(s): 25ecd858d747918007eb17d9f46199592ccf664f
author: Sergei Petrunia
committer: Sergei Petrunia
timestamp: 2021-05-17 17:30:04 +0300
message:

Reset iterator after point lookup

Summary: Although I did not find an example of this happening today, it seems like the SQL layer can "index_next" after a point lookup. However, since we don't position our iterator for point lookups, we would be potentially returning incorrect results (eg. if we had a previous valid iterator from before the point lookup). It seems safer to just reset the iterator after a point lookup.

Test Plan: mtr

Reviewers: luqun, herman, yzha

Subscribers: pgl, vinaybhat

Differential Revision: https://phabricator.intern.facebook.com/D23399585

---
 storage/rocksdb/ha_rocksdb.cc | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc
index 3f50f0b6380..91791b337af 100644
--- a/storage/rocksdb/ha_rocksdb.cc
+++ b/storage/rocksdb/ha_rocksdb.cc
@@ -9078,6 +9078,15 @@ int ha_rocksdb::index_read_intern(uchar *const buf, const uchar *const key,
             stats.rows_index_first++; */
           update_row_stats(ROWS_READ);
         }
+        /*
+          If the SQL layer calls index_read_map, it expects the iterator to be
+          positioned accordingly, so that next/prev can work as expected. In
+          this case, we calling DB::Get directly without positioning an
+          iterator, so it is incorrect for the SQL layer to be calling
+          next/prev anyway. To avoid correctness issues, just free the
+          iterator.
+        */
+        release_scan_iterator();
         DBUG_RETURN(rc);
       } else {
         /*
@@ -9622,6 +9631,12 @@ int ha_rocksdb::index_next_with_direction_intern(uchar *const buf,
       break;
     }
 
+    DBUG_ASSERT(m_scan_it);
+    if (m_scan_it == nullptr) {
+      rc = HA_ERR_INTERNAL_ERROR;
+      break;
+    }
+
     if (skip_next) {
       skip_next = false;
     } else {
@@ -9632,15 +9647,7 @@ int ha_rocksdb::index_next_with_direction_intern(uchar *const buf,
       }
     }
 
-    if (!m_scan_it || !is_valid_iterator(m_scan_it)) {
-      /*
-        We can get here when SQL layer has called
-
-        h->index_init(PRIMARY);
-        h->index_read_map(full index tuple, HA_READ_KEY_EXACT);
-
-        In this case, we should return EOF.
-      */
+    if (!is_valid_iterator(m_scan_it)) {
       rc = HA_ERR_END_OF_FILE;
       break;
     }


More information about the commits mailing list