revision-id: 5ccdc6c154d78ef450609a8237b9e84e7d79eeaf (fb-prod201903-141-g5ccdc6c154d) parent(s): 3066ae2ebd4147f6d98b3c9be810db53e7bbea2e author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2019-09-06 12:12:11 +0300 message: Issue #790: MultiGet-based MRR, Secondary Keys - Fix an issue where one would switch off MRR and then get wrong query results: if MRR code has set m_keyread_only, it should also clear it. - Make Index Condition Pushdown work. --- mysql-test/suite/rocksdb/r/rocksdb_mrr.result | 26 ++++++++++++++++++++++++++ mysql-test/suite/rocksdb/t/rocksdb_mrr.test | 13 ++++++------- storage/rocksdb/ha_rocksdb.cc | 26 ++++++++++++++++++-------- storage/rocksdb/ha_rocksdb.h | 3 +++ 4 files changed, 53 insertions(+), 15 deletions(-) diff --git a/mysql-test/suite/rocksdb/r/rocksdb_mrr.result b/mysql-test/suite/rocksdb/r/rocksdb_mrr.result index 47c3c7941a5..bf90d75e696 100644 --- a/mysql-test/suite/rocksdb/r/rocksdb_mrr.result +++ b/mysql-test/suite/rocksdb/r/rocksdb_mrr.result @@ -258,6 +258,32 @@ pk1 pk2 col1 filler 26 26 26 26 27 27 27 27 28 28 28 28 +# Check if Index Condition Pushdown works +explain +select * from t3 where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE t3 range col1 col1 5 NULL 2 Using index condition; Using MRR +select * from t3 where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; +pk1 pk2 col1 filler +20 20 20 20 +26 26 26 26 +28 28 28 28 +select * from t3 use index() where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; +pk1 pk2 col1 filler +20 20 20 20 +26 26 26 26 +28 28 28 28 +explain +select pk1,pk2,col1, filler,mod(t3.col1,2) from t3 +where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE t3 range col1 col1 5 NULL 2 Using index condition; Using MRR +select pk1,pk2,col1, filler,mod(t3.col1,2) from t3 +where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; +pk1 pk2 col1 filler mod(t3.col1,2) +20 20 20 20 0 +26 26 26 26 0 +28 28 28 28 0 drop table t0,t1,t2,t3,t4; # # Multi-keypart testcase diff --git a/mysql-test/suite/rocksdb/t/rocksdb_mrr.test b/mysql-test/suite/rocksdb/t/rocksdb_mrr.test index 5283ee9ed2a..77a78cff6d1 100644 --- a/mysql-test/suite/rocksdb/t/rocksdb_mrr.test +++ b/mysql-test/suite/rocksdb/t/rocksdb_mrr.test @@ -181,20 +181,19 @@ explain select * from t3 where t3.col1=20 or t3.col1 between 25 and 28; select * from t3 where t3.col1=20 or t3.col1 between 25 and 28; ---disable_parsing ---echo # Check if index condition pushdown works +--echo # Check if Index Condition Pushdown works explain select * from t3 where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; select * from t3 where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; select * from t3 use index() where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; -select * from t3 use index() where (t3.col1=20 or t3.col1 between 25 and 28); -set optimizer_switch='mrr=off'; explain -select pk1,pk2,col1, filler,mod(t3.col1,2) from t3 where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; -select pk1,pk2,col1, filler,mod(t3.col1,2) from t3 where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; ---enable_parsing +select pk1,pk2,col1, filler,mod(t3.col1,2) from t3 +where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; + +select pk1,pk2,col1, filler,mod(t3.col1,2) from t3 +where (t3.col1=20 or t3.col1 between 25 and 28) and mod(t3.col1,2)=0; drop table t0,t1,t2,t3,t4; diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc index 35c0da9ed17..18b0beae850 100644 --- a/storage/rocksdb/ha_rocksdb.cc +++ b/storage/rocksdb/ha_rocksdb.cc @@ -6168,6 +6168,7 @@ ha_rocksdb::ha_rocksdb(my_core::handlerton *const hton, m_dup_pk_found(false), mrr_rowid_reader(nullptr), mrr_n_elements(0), + mrr_enabled_keyread(false), m_in_rpl_delete_rows(false), m_in_rpl_update_rows(false), m_force_skip_unique_check(false) {} @@ -7918,14 +7919,17 @@ int ha_rocksdb::read_row_from_secondary_key(uchar *const buf, #endif if (covered_lookup && m_lock_rows == RDB_LOCK_NONE) { - pk_size = - kd.get_primary_key_tuple(table, *m_pk_descr, &rkey, m_pk_packed_tuple); - if (pk_size == RDB_INVALID_KEY_LEN) { - rc = HA_ERR_ROCKSDB_CORRUPT_DATA; - } else { - rc = kd.unpack_record(table, buf, &rkey, &value, - m_converter->get_verify_row_debug_checksums()); - global_stats.covered_secondary_key_lookups.inc(); + // Due to MRR, we can have ICP enabled with covered_lookup == true + if (!(rc = find_icp_matching_index_rec(move_forward, buf))) { + pk_size = + kd.get_primary_key_tuple(table, *m_pk_descr, &rkey, m_pk_packed_tuple); + if (pk_size == RDB_INVALID_KEY_LEN) { + rc = HA_ERR_ROCKSDB_CORRUPT_DATA; + } else { + rc = kd.unpack_record(table, buf, &rkey, &value, + m_converter->get_verify_row_debug_checksums()); + global_stats.covered_secondary_key_lookups.inc(); + } } } else { if (kd.m_is_reverse_cf) move_forward = !move_forward; @@ -14901,6 +14905,7 @@ class Mrr_sec_key_rowid_source : public Mrr_rowid_source { int init(RANGE_SEQ_IF *seq, void *seq_init_param, uint n_ranges, uint mode) { self->m_keyread_only = true; // TODO: is this ugly or fine? + self->mrr_enabled_keyread = true; return self->handler::multi_range_read_init(seq, seq_init_param, n_ranges, mode, nullptr); } @@ -14942,6 +14947,7 @@ int ha_rocksdb::multi_range_read_init(RANGE_SEQ_IF *seq, void *seq_init_param, mrr_uses_default_impl = false; mrr_n_elements = 0; // nothing to cleanup, yet. + mrr_enabled_keyread = false; mrr_rowid_reader = nullptr; mrr_funcs = *seq; @@ -15057,6 +15063,10 @@ int ha_rocksdb::mrr_fill_buffer() { void ha_rocksdb::mrr_free() { // Free everything + if (mrr_enabled_keyread) { + m_keyread_only = false; + mrr_enabled_keyread = false; + } mrr_free_rows(); delete mrr_rowid_reader; mrr_rowid_reader = nullptr; diff --git a/storage/rocksdb/ha_rocksdb.h b/storage/rocksdb/ha_rocksdb.h index 40e4024a67d..a38cb02a7d1 100644 --- a/storage/rocksdb/ha_rocksdb.h +++ b/storage/rocksdb/ha_rocksdb.h @@ -693,6 +693,9 @@ class ha_rocksdb : public my_core::handler { ssize_t mrr_n_elements; // Number of elements in the above arrays ssize_t mrr_read_index; // Number of the element we will return next + // if true, MRR code has enabled keyread (and should disable it back) + bool mrr_enabled_keyread; + // Expected number of rowids that are left to scan. // This number is used to avoid allocating huge arrays in mrr_fill_buffer ssize_t mrr_n_rowids;