check upper and lower bound for writebatchwithindex

Summary:
rocksdb use Writebatchwithindex(WBWI) to support read your own data. But there are two issues for implementations:
1.  facebook/rocksdb#11606: Rocksdb may return deleted row or out of range row during iterating WBWI, see code https://github.com/facebook/rocksdb/blob/main/utilities/write_batch_with_index/write_batch_with_index_internal.cc#L311, when it return due out of range(bound), its DeltaIterator may still valid but point to out of range row.
2. facebook/rocksdb#11607: it doesn't check lower_bound_ even  lower_bound_ values is passed to rocksdb with readoptions. see https://github.com/facebook/rocksdb/blob/main/utilities/write_batch_with_index/write_batch_with_index_internal.cc#L306

To workaround these issue,  add a variable rocksdb_check_iterate_bounds to control whether we should check iterate bounds and check these bounds inside myrocks if rocksdb_check_iterate_bounds is true.

Differential Revision: D46908478

mysql-test/r/mysqld--help-notwin.result

@@ -1952,6 +1952,10 @@ The following options may be given as the first argument:
  --rocksdb-charge-memory 
  For experiment only. Turn on memory charging feature of
  RocksDB
+ --rocksdb-check-iterate-bounds 
+ Check rocksdb iterator upper/lower bounds during
+ iterating.
+ (Defaults to on; use --skip-rocksdb-check-iterate-bounds to disable.)
  --rocksdb-checksums-pct=# 
  How many percentages of rows to be checksummed
  --rocksdb-clone-checkpoint-max-age=# 
@@ -3628,6 +3632,7 @@ rocksdb-cancel-manual-compactions FALSE
 rocksdb-cf-options ON
 rocksdb-cfstats ON
 rocksdb-charge-memory FALSE
+rocksdb-check-iterate-bounds TRUE
 rocksdb-checksums-pct 100
 rocksdb-clone-checkpoint-max-age 600
 rocksdb-clone-checkpoint-max-count 90

mysql-test/suite/rocksdb/my.cnf

@@ -12,3 +12,5 @@ explicit-defaults-for-timestamp=1
 rocksdb_lock_wait_timeout=1
 rocksdb_strict_collation_check=0
 rocksdb_force_compute_memtable_stats_cachetime=0
+rocksdb_check_iterate_bounds=1
+

mysql-test/suite/rocksdb/r/iterator_bounds.result

@@ -13,3 +13,43 @@ select j from t order by j desc;
 j
 1
 drop table t;
+create table t(
+a int unsigned not null,
+b int unsigned not null,
+c varchar(64) not null COLLATE utf8_bin,
+primary key(a),
+key(b,c)
+) DEFAULT CHARSET=utf8 COLLATE=utf8_bin;
+Warnings:
+Warning	3778	'utf8mb3_bin' is a collation of the deprecated character set UTF8MB3. Please consider using UTF8MB4 with an appropriate collation instead.
+Warning	3719	'utf8' is currently an alias for the character set UTF8MB3, but will be an alias for UTF8MB4 in a future release. Please consider using UTF8MB4 in order to be unambiguous.
+Warning	3778	'utf8mb3_bin' is a collation of the deprecated character set UTF8MB3. Please consider using UTF8MB4 with an appropriate collation instead.
+begin;
+insert into t values(101, 101, 'view_routine_usage');
+update t set b = 100 where b = 101 and c like 'view_routine_usage';
+update t set b = 101 where b = 100 and c like 'view_routine_usage';
+select a from t where b = 101 and c like 'view_routine_usage';
+a
+101
+rollback;
+drop table t;
+create table t(
+a int unsigned not null,
+b int unsigned not null,
+c varchar(64) not null COLLATE utf8_bin,
+primary key(a),
+key(b,c) comment 'rev:bc'
+) DEFAULT CHARSET=utf8 COLLATE=utf8_bin;
+Warnings:
+Warning	3778	'utf8mb3_bin' is a collation of the deprecated character set UTF8MB3. Please consider using UTF8MB4 with an appropriate collation instead.
+Warning	3719	'utf8' is currently an alias for the character set UTF8MB3, but will be an alias for UTF8MB4 in a future release. Please consider using UTF8MB4 in order to be unambiguous.
+Warning	3778	'utf8mb3_bin' is a collation of the deprecated character set UTF8MB3. Please consider using UTF8MB4 with an appropriate collation instead.
+begin;
+insert into t values(110, 110, 'view_routine_usage');
+insert into t values(100, 100, 'view_routine_usage');
+update t set b = 101 where b = 100 and c like 'view_routine_usage';
+update t set b = 100 where b = 101 and c like 'view_routine_usage';
+select a from t where b = 101 and c like 'view_routine_usage';
+a
+rollback;
+drop table t;

mysql-test/suite/rocksdb/r/rocksdb.result

@@ -928,6 +928,7 @@ rocksdb_cache_index_and_filter_blocks	ON
 rocksdb_cache_index_and_filter_with_high_priority	ON
 rocksdb_cancel_manual_compactions	OFF
 rocksdb_charge_memory	OFF
+rocksdb_check_iterate_bounds	ON
 rocksdb_checksums_pct	100
 rocksdb_clone_checkpoint_max_age	600
 rocksdb_clone_checkpoint_max_count	90

mysql-test/suite/rocksdb/t/iterator_bounds.test

@@ -27,3 +27,56 @@ select j from t order by j asc;
 select j from t order by j desc;
 
 drop table t;
+
+#
+# check bounds for writebatch(forward cf), all data changes are in writebatch
+#
+create table t(
+  a int unsigned not null,
+  b int unsigned not null,
+  c varchar(64) not null COLLATE utf8_bin,
+  primary key(a),
+  key(b,c)
+) DEFAULT CHARSET=utf8 COLLATE=utf8_bin;
+
+begin;
+insert into t values(101, 101, 'view_routine_usage');
+# SD(101, 'view_routine_usage',101)
+update t set b = 100 where b = 101 and c like 'view_routine_usage';
+# dring iterating, writebatchwithindex may return "out of range" record after 
+# checking with upper bounds. sometimes the "out of range" record is a SD record.
+# For SD record, its value slice will be empty. Try to decode a key slice
+# which contains varchar with empty value slice, decoder will crash due missing
+# upack_info in value slice
+update t set b = 101 where b = 100 and c like 'view_routine_usage';
+select a from t where b = 101 and c like 'view_routine_usage';
+rollback;
+
+drop table t;
+
+
+#
+# check bounds for writebatch(rev cf), all data changes are in writebatch
+#
+create table t(
+  a int unsigned not null,
+  b int unsigned not null,
+  c varchar(64) not null COLLATE utf8_bin,
+  primary key(a),
+  key(b,c) comment 'rev:bc'
+) DEFAULT CHARSET=utf8 COLLATE=utf8_bin;
+
+begin;
+insert into t values(110, 110, 'view_routine_usage');
+insert into t values(100, 100, 'view_routine_usage');
+# SD(100, 'view_routine_usage',100)
+update t set b = 101 where b = 100 and c like 'view_routine_usage';
+# during iterating, we don't check with lower bound in writebatchwithindex
+# in rev cf,
+update t set b = 100 where b = 101 and c like 'view_routine_usage';
+select a from t where b = 101 and c like 'view_routine_usage';
+rollback;
+
+drop table t;
+
+

mysql-test/suite/rocksdb_sys_vars/r/rocksdb_check_iterate_bounds_basic.result

@@ -0,0 +1,100 @@
+CREATE TABLE valid_values (value varchar(255)) ENGINE=myisam;
+INSERT INTO valid_values VALUES(1);
+INSERT INTO valid_values VALUES(0);
+INSERT INTO valid_values VALUES('on');
+CREATE TABLE invalid_values (value varchar(255)) ENGINE=myisam;
+INSERT INTO invalid_values VALUES('\'aaa\'');
+INSERT INTO invalid_values VALUES('\'bbb\'');
+SET @start_global_value = @@global.ROCKSDB_CHECK_ITERATE_BOUNDS;
+SELECT @start_global_value;
+@start_global_value
+1
+SET @start_session_value = @@session.ROCKSDB_CHECK_ITERATE_BOUNDS;
+SELECT @start_session_value;
+@start_session_value
+1
+'# Setting to valid values in global scope#'
+"Trying to set variable @@global.ROCKSDB_CHECK_ITERATE_BOUNDS to 1"
+SET @@global.ROCKSDB_CHECK_ITERATE_BOUNDS   = 1;
+SELECT @@global.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@global.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+"Setting the global scope variable back to default"
+SET @@global.ROCKSDB_CHECK_ITERATE_BOUNDS = DEFAULT;
+SELECT @@global.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@global.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+"Trying to set variable @@global.ROCKSDB_CHECK_ITERATE_BOUNDS to 0"
+SET @@global.ROCKSDB_CHECK_ITERATE_BOUNDS   = 0;
+SELECT @@global.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@global.ROCKSDB_CHECK_ITERATE_BOUNDS
+0
+"Setting the global scope variable back to default"
+SET @@global.ROCKSDB_CHECK_ITERATE_BOUNDS = DEFAULT;
+SELECT @@global.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@global.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+"Trying to set variable @@global.ROCKSDB_CHECK_ITERATE_BOUNDS to on"
+SET @@global.ROCKSDB_CHECK_ITERATE_BOUNDS   = on;
+SELECT @@global.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@global.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+"Setting the global scope variable back to default"
+SET @@global.ROCKSDB_CHECK_ITERATE_BOUNDS = DEFAULT;
+SELECT @@global.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@global.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+'# Setting to valid values in session scope#'
+"Trying to set variable @@session.ROCKSDB_CHECK_ITERATE_BOUNDS to 1"
+SET @@session.ROCKSDB_CHECK_ITERATE_BOUNDS   = 1;
+SELECT @@session.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@session.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+"Setting the session scope variable back to default"
+SET @@session.ROCKSDB_CHECK_ITERATE_BOUNDS = DEFAULT;
+SELECT @@session.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@session.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+"Trying to set variable @@session.ROCKSDB_CHECK_ITERATE_BOUNDS to 0"
+SET @@session.ROCKSDB_CHECK_ITERATE_BOUNDS   = 0;
+SELECT @@session.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@session.ROCKSDB_CHECK_ITERATE_BOUNDS
+0
+"Setting the session scope variable back to default"
+SET @@session.ROCKSDB_CHECK_ITERATE_BOUNDS = DEFAULT;
+SELECT @@session.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@session.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+"Trying to set variable @@session.ROCKSDB_CHECK_ITERATE_BOUNDS to on"
+SET @@session.ROCKSDB_CHECK_ITERATE_BOUNDS   = on;
+SELECT @@session.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@session.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+"Setting the session scope variable back to default"
+SET @@session.ROCKSDB_CHECK_ITERATE_BOUNDS = DEFAULT;
+SELECT @@session.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@session.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+'# Testing with invalid values in global scope #'
+"Trying to set variable @@global.ROCKSDB_CHECK_ITERATE_BOUNDS to 'aaa'"
+SET @@global.ROCKSDB_CHECK_ITERATE_BOUNDS   = 'aaa';
+Got one of the listed errors
+SELECT @@global.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@global.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+"Trying to set variable @@global.ROCKSDB_CHECK_ITERATE_BOUNDS to 'bbb'"
+SET @@global.ROCKSDB_CHECK_ITERATE_BOUNDS   = 'bbb';
+Got one of the listed errors
+SELECT @@global.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@global.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+SET @@global.ROCKSDB_CHECK_ITERATE_BOUNDS = @start_global_value;
+SELECT @@global.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@global.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+SET @@session.ROCKSDB_CHECK_ITERATE_BOUNDS = @start_session_value;
+SELECT @@session.ROCKSDB_CHECK_ITERATE_BOUNDS;
+@@session.ROCKSDB_CHECK_ITERATE_BOUNDS
+1
+DROP TABLE valid_values;
+DROP TABLE invalid_values;

mysql-test/suite/rocksdb_sys_vars/t/rocksdb_check_iterate_bounds_basic.test

@@ -0,0 +1,18 @@
+--source include/have_rocksdb.inc
+
+CREATE TABLE valid_values (value varchar(255)) ENGINE=myisam;
+INSERT INTO valid_values VALUES(1);
+INSERT INTO valid_values VALUES(0);
+INSERT INTO valid_values VALUES('on');
+
+CREATE TABLE invalid_values (value varchar(255)) ENGINE=myisam;
+INSERT INTO invalid_values VALUES('\'aaa\'');
+INSERT INTO invalid_values VALUES('\'bbb\'');
+
+--let $sys_var=ROCKSDB_CHECK_ITERATE_BOUNDS
+--let $read_only=0
+--let $session=1
+--source ../include/rocksdb_sys_var.inc
+
+DROP TABLE valid_values;
+DROP TABLE invalid_values;

storage/rocksdb/ha_rocksdb.cc

@@ -1566,6 +1566,11 @@ static MYSQL_THDVAR_BOOL(
     "Enable rocksdb iterator upper/lower bounds in read options.", nullptr,
     nullptr, true);
 
+static MYSQL_THDVAR_BOOL(
+    check_iterate_bounds, PLUGIN_VAR_OPCMDARG,
+    "Check rocksdb iterator upper/lower bounds during iterating.", nullptr,
+    nullptr, true);
+
 static const char *const DEFAULT_READ_FREE_RPL_TABLES = ".*";
 
 static int rocksdb_validate_read_free_rpl_tables(
@@ -2994,6 +2999,7 @@ static struct SYS_VAR *rocksdb_system_variables[] = {
     MYSQL_SYSVAR(commit_in_the_middle),
     MYSQL_SYSVAR(blind_delete_primary_key),
     MYSQL_SYSVAR(enable_iterate_bounds),
+    MYSQL_SYSVAR(check_iterate_bounds),
     MYSQL_SYSVAR(read_free_rpl_tables),
     MYSQL_SYSVAR(read_free_rpl),
     MYSQL_SYSVAR(bulk_load_size),
@@ -17627,11 +17633,13 @@ bool ha_rocksdb::can_assume_tracked(THD *thd) {
 bool ha_rocksdb::check_bloom_and_set_bounds(
     THD *thd, const Rdb_key_def &kd, const rocksdb::Slice &eq_cond,
     size_t bound_len, uchar *const lower_bound, uchar *const upper_bound,
-    rocksdb::Slice *lower_bound_slice, rocksdb::Slice *upper_bound_slice) {
+    rocksdb::Slice *lower_bound_slice, rocksdb::Slice *upper_bound_slice,
+    bool *check_iterate_bounds) {
   bool can_use_bloom = can_use_bloom_filter(thd, kd, eq_cond);
   if (!can_use_bloom && (THDVAR(thd, enable_iterate_bounds))) {
     setup_iterator_bounds(kd, eq_cond, bound_len, lower_bound, upper_bound,
                           lower_bound_slice, upper_bound_slice);
+    *check_iterate_bounds = THDVAR(thd, check_iterate_bounds);
   }
   return can_use_bloom;
 }

storage/rocksdb/ha_rocksdb.h

@@ -665,7 +665,8 @@ class ha_rocksdb : public my_core::handler, public blob_buffer {
   static bool check_bloom_and_set_bounds(
       THD *thd, const Rdb_key_def &kd, const rocksdb::Slice &eq_cond,
       size_t bound_len, uchar *const lower_bound, uchar *const upper_bound,
-      rocksdb::Slice *lower_bound_slice, rocksdb::Slice *upper_bound_slice);
+      rocksdb::Slice *lower_bound_slice, rocksdb::Slice *upper_bound_slice,
+      bool *check_iterate_bounds);
   static bool can_use_bloom_filter(THD *thd, const Rdb_key_def &kd,
                                    const rocksdb::Slice &eq_cond);
 

storage/rocksdb/rdb_iterator.cc

@@ -43,7 +43,8 @@ Rdb_iterator_base::Rdb_iterator_base(THD *thd, ha_rocksdb *rocksdb_handler,
       m_scan_it_upper_bound(nullptr),
       m_prefix_buf(nullptr),
       m_table_type(tbl_def->get_table_type()),
-      m_valid(false) {
+      m_valid(false),
+      m_check_iterate_bounds(false) {
   if (tbl_def->get_table_type() == INTRINSIC_TMP) {
     if (m_rocksdb_handler) {
       add_tmp_table_handler(m_thd, m_rocksdb_handler);
@@ -147,7 +148,8 @@ void Rdb_iterator_base::setup_scan_iterator(const rocksdb::Slice *const slice,
           m_thd, *m_kd, eq_cond,
           std::max(eq_cond_len, (uint)Rdb_key_def::INDEX_NUMBER_SIZE),
           m_scan_it_lower_bound, m_scan_it_upper_bound,
-          &m_scan_it_lower_bound_slice, &m_scan_it_upper_bound_slice)) {
+          &m_scan_it_lower_bound_slice, &m_scan_it_upper_bound_slice,
+          &m_check_iterate_bounds)) {
     skip_bloom = false;
   }
 
@@ -228,6 +230,7 @@ int Rdb_iterator_base::next_with_direction(bool move_forward, bool skip_next) {
   Rdb_transaction *const tx = get_tx_from_thd(m_thd);
 
   if (!m_valid) return HA_ERR_END_OF_FILE;
+  const rocksdb::Comparator *kd_comp = kd.get_cf()->GetComparator();
 
   for (;;) {
     DEBUG_SYNC(m_thd, "rocksdb.check_flags_nwd");
@@ -266,6 +269,23 @@ int Rdb_iterator_base::next_with_direction(bool move_forward, bool skip_next) {
       break;
     }
 
+    // Check specified lower/upper bounds
+    // For example, retrieved key is 00077
+    // in     cf, lower_bound: 0076 and uppper bound: 0078
+    //     cf->Compare(0077, 0078) > 0 ==> False
+    //     cf->Compare(0077, 0076) < 0 ==> False
+    // in rev cf, lower_bound: 0078 and uppper bound: 0076
+    //     revcf->Compare(0077, 0076) > 0 ==> False
+    //     revcf->Compare(0077, 0078) < 0 ==> False
+    if (m_check_iterate_bounds &&
+        ((!m_scan_it_upper_bound_slice.empty() &&
+          kd_comp->Compare(key, m_scan_it_upper_bound_slice) > 0) ||
+         (!m_scan_it_lower_bound_slice.empty() &&
+          kd_comp->Compare(key, m_scan_it_lower_bound_slice) < 0))) {
+      rc = HA_ERR_END_OF_FILE;
+      break;
+    }
+
     // Record is not visible due to TTL, move to next record.
     if (m_pkd->has_ttl() && rdb_should_hide_ttl_rec(kd, &value, tx)) {
       continue;

storage/rocksdb/rdb_iterator.h

@@ -147,6 +147,7 @@ class Rdb_iterator_base : public Rdb_iterator {
   rocksdb::Slice m_prefix_tuple;
   TABLE_TYPE m_table_type;
   bool m_valid;
+  bool m_check_iterate_bounds;
 };
 
 class Rdb_iterator_partial : public Rdb_iterator_base {