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; }