Added error message when client sets an invalid rocksdb_update_cf_opt…

…ions

Summary: Modified the set_var's sql_set_variables to propagate errors set by my_error() in the update func. Added error printing for invalid rocksdb_update_cf_options and updated the corresponding unit test

Reviewed By: hermanlee

Differential Revision: D5970102

fbshipit-source-id: d39ba65

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

@@ -0,0 +1,38 @@
+CREATE TABLE t1 (a INT, PRIMARY KEY (a) COMMENT 'update_cf1') ENGINE=ROCKSDB;
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS='update_cf1={write_buffer_size=8m;target_file_size_base=2m};';
+SELECT @@global.rocksdb_update_cf_options;
+@@global.rocksdb_update_cf_options
+update_cf1={write_buffer_size=8m;target_file_size_base=2m};
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS=NULL;
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+Variable_name	Value
+rocksdb_update_cf_options	
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS=NULL;
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+Variable_name	Value
+rocksdb_update_cf_options	
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS="";
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+Variable_name	Value
+rocksdb_update_cf_options	
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS=NULL;
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+Variable_name	Value
+rocksdb_update_cf_options	
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS='update_cf1={write_buffer_size=8m;target_file_size_base=2m};';
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+Variable_name	Value
+rocksdb_update_cf_options	update_cf1={write_buffer_size=8m;target_file_size_base=2m};
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS='update_cf2={write_buffer_size=8m;target_file_size_base=2m};';
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+Variable_name	Value
+rocksdb_update_cf_options	update_cf2={write_buffer_size=8m;target_file_size_base=2m};
+DROP TABLE t1;
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS='update_cf1={write_buffer_size=8m;target_file_size_base=2m};';
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+Variable_name	Value
+rocksdb_update_cf_options	update_cf1={write_buffer_size=8m;target_file_size_base=2m};
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS=DEFAULT;
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+Variable_name	Value
+rocksdb_update_cf_options	

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

@@ -32,10 +32,19 @@ SET @@global.rocksdb_update_cf_options = NULL;
 SELECT @@global.rocksdb_update_cf_options;
 @@global.rocksdb_update_cf_options
 NULL
-SET @@global.rocksdb_update_cf_options = 'aaaaa';
+SET @@global.rocksdb_update_cf_options = NULL;
 SELECT @@global.rocksdb_update_cf_options;
 @@global.rocksdb_update_cf_options
 NULL
+SET @@global.rocksdb_update_cf_options = '';
+SELECT @@global.rocksdb_update_cf_options;
+@@global.rocksdb_update_cf_options
+
+SET @@global.rocksdb_update_cf_options = 'aaaaa';;
+ERROR 42000: Variable 'rocksdb_update_cf_options' can't be set to the value of 'aaaaa'
+SELECT @@global.rocksdb_update_cf_options;
+@@global.rocksdb_update_cf_options
+
 SELECT * FROM ROCKSDB_CF_OPTIONS WHERE CF_NAME='default' AND OPTION_TYPE='WRITE_BUFFER_SIZE';
 CF_NAME	OPTION_TYPE	VALUE
 default	WRITE_BUFFER_SIZE	67108864
@@ -100,7 +109,12 @@ cf1={target_file_size_base=24m};foo={max_bytes_for_level_multiplier=8};
 SELECT * FROM ROCKSDB_CF_OPTIONS WHERE CF_NAME='cf1' AND OPTION_TYPE='TARGET_FILE_SIZE_BASE';
 CF_NAME	OPTION_TYPE	VALUE
 cf1	TARGET_FILE_SIZE_BASE	25165824
-SET @@global.rocksdb_update_cf_options = 'default={foo=bar};';
+SET @@global.rocksdb_update_cf_options = 'default={foo=bar};';;
+ERROR 42000: Variable 'rocksdb_update_cf_options' can't be set to the value of 'default={foo=bar};'
+SELECT @@global.rocksdb_update_cf_options;
+@@global.rocksdb_update_cf_options
+cf1={target_file_size_base=24m};foo={max_bytes_for_level_multiplier=8};
+SET @@global.rocksdb_update_cf_options = NULL;
 SELECT @@global.rocksdb_update_cf_options;
 @@global.rocksdb_update_cf_options
 NULL

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

@@ -0,0 +1,22 @@
+--source include/have_rocksdb.inc
+
+CREATE TABLE t1 (a INT, PRIMARY KEY (a) COMMENT 'update_cf1') ENGINE=ROCKSDB;
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS='update_cf1={write_buffer_size=8m;target_file_size_base=2m};';
+SELECT @@global.rocksdb_update_cf_options;
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS=NULL;
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS=NULL;
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS="";
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS=NULL;
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS='update_cf1={write_buffer_size=8m;target_file_size_base=2m};';
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS='update_cf2={write_buffer_size=8m;target_file_size_base=2m};';
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+DROP TABLE t1;
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS='update_cf1={write_buffer_size=8m;target_file_size_base=2m};';
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';
+SET @@GLOBAL.ROCKSDB_UPDATE_CF_OPTIONS=DEFAULT;
+SHOW GLOBAL VARIABLES LIKE 'rocksdb_update_cf_options';

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

@@ -38,8 +38,17 @@ SELECT @@global.rocksdb_update_cf_options;
 SET @@global.rocksdb_update_cf_options = NULL;
 SELECT @@global.rocksdb_update_cf_options;
 
+# Make sure that we do not double free the NULL string
+SET @@global.rocksdb_update_cf_options = NULL;
+SELECT @@global.rocksdb_update_cf_options;
+
+# Attempt setting an empty string
+SET @@global.rocksdb_update_cf_options = '';
+SELECT @@global.rocksdb_update_cf_options;
+
 # Will fail to parse. Value not updated.
-SET @@global.rocksdb_update_cf_options = 'aaaaa';
+--Error ER_WRONG_VALUE_FOR_VAR
+--eval SET @@global.rocksdb_update_cf_options = 'aaaaa';
 SELECT @@global.rocksdb_update_cf_options;
 
 SELECT * FROM ROCKSDB_CF_OPTIONS WHERE CF_NAME='default' AND OPTION_TYPE='WRITE_BUFFER_SIZE';
@@ -86,9 +95,13 @@ SELECT * FROM ROCKSDB_CF_OPTIONS WHERE CF_NAME='cf1' AND OPTION_TYPE='TARGET_FIL
 
 # Will fail to parse. No valid assignments included. Value not updated and
 # reset to NULL.
-SET @@global.rocksdb_update_cf_options = 'default={foo=bar};';
+--Error ER_WRONG_VALUE_FOR_VAR
+--eval SET @@global.rocksdb_update_cf_options = 'default={foo=bar};';
+SELECT @@global.rocksdb_update_cf_options;
+
+SET @@global.rocksdb_update_cf_options = NULL;
 SELECT @@global.rocksdb_update_cf_options;
 
 USE test;
 
-DROP TABLE t1;
+DROP TABLE t1;

storage/rocksdb/ha_rocksdb.cc

@@ -404,10 +404,14 @@ static void rocksdb_set_collation_exception_list(THD *thd,
                                                  void *var_ptr,
                                                  const void *save);
 
-void rocksdb_set_update_cf_options(THD *thd,
-                                   struct st_mysql_sys_var *var,
-                                   void *var_ptr,
-                                   const void *save);
+static int rocksdb_validate_update_cf_options(THD *thd,
+                                              struct st_mysql_sys_var *var,
+                                              void *save,
+                                              st_mysql_value *value);
+
+static void rocksdb_set_update_cf_options(THD *thd,
+                                          struct st_mysql_sys_var *var,
+                                          void *var_ptr, const void *save);
 
 static int rocksdb_check_bulk_load(THD *const thd,
                                    struct st_mysql_sys_var *var
@@ -1203,7 +1207,8 @@ static MYSQL_SYSVAR_STR(override_cf_options, rocksdb_override_cf_options,
 static MYSQL_SYSVAR_STR(update_cf_options, rocksdb_update_cf_options,
                         PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_MEMALLOC |
                             PLUGIN_VAR_ALLOCATED,
-                        "Option updates per column family for RocksDB", nullptr,
+                        "Option updates per column family for RocksDB",
+                        rocksdb_validate_update_cf_options,
                         rocksdb_set_update_cf_options, nullptr);
 
 static MYSQL_SYSVAR_UINT(flush_log_at_trx_commit,
@@ -12666,34 +12671,59 @@ static void rocksdb_set_wal_bytes_per_sync(
   RDB_MUTEX_UNLOCK_CHECK(rdb_sysvars_mutex);
 }
 
-void rocksdb_set_update_cf_options(THD *const /* unused */,
-                                   struct st_mysql_sys_var *const /* unused */,
-                                   void *const var_ptr,
-                                   const void *const save) {
+static int
+rocksdb_validate_update_cf_options(THD * /* unused */,
+                                   struct st_mysql_sys_var * /*unused*/,
+                                   void *save, struct st_mysql_value *value) {
+
+  char buff[STRING_BUFFER_USUAL_SIZE];
+  const char *str;
+  int length;
+  length = sizeof(buff);
+  str = value->val_str(value, buff, &length);
+  *(const char **)save = str;
+
+  if (str == nullptr) {
+    return HA_EXIT_SUCCESS;
+  }
+
+  Rdb_cf_options::Name_to_config_t option_map;
+
+  // Basic sanity checking and parsing the options into a map. If this fails
+  // then there's no point to proceed.
+  if (!Rdb_cf_options::parse_cf_options(str, &option_map)) {
+    my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "rocksdb_update_cf_options", str);
+    return HA_EXIT_FAILURE;
+  }
+  return HA_EXIT_SUCCESS;
+}
+
+static void
+rocksdb_set_update_cf_options(THD *const /* unused */,
+                              struct st_mysql_sys_var *const /* unused */,
+                              void *const var_ptr, const void *const save) {
   const char *const val = *static_cast<const char *const *>(save);
 
+  RDB_MUTEX_LOCK_CHECK(rdb_sysvars_mutex);
+
   if (!val) {
-    // NO_LINT_DEBUG
-    sql_print_warning("MyRocks: NULL is not a valid option for updates to "
-                      "column family settings.");
+    *reinterpret_cast<char **>(var_ptr) = nullptr;
+    RDB_MUTEX_UNLOCK_CHECK(rdb_sysvars_mutex);
     return;
   }
 
-  RDB_MUTEX_LOCK_CHECK(rdb_sysvars_mutex);
-
   DBUG_ASSERT(val != nullptr);
 
+  // Reset the pointers regardless of how much success we had with updating
+  // the CF options. This will results in consistent behavior and avoids
+  // dealing with cases when only a subset of CF-s was successfully updated.
+  *reinterpret_cast<char **>(var_ptr) = my_strdup(val, MYF(0));
+
   // Do the real work of applying the changes.
   Rdb_cf_options::Name_to_config_t option_map;
 
-  // Basic sanity checking and parsing the options into a map. If this fails
-  // then there's no point to proceed.
+  // This should never fail, because of rocksdb_validate_update_cf_options
   if (!Rdb_cf_options::parse_cf_options(val, &option_map)) {
-    *reinterpret_cast<char**>(var_ptr) = nullptr;
-
-    // NO_LINT_DEBUG
-    sql_print_warning("MyRocks: failed to parse the updated column family "
-                      "options = '%s'.", val);
     RDB_MUTEX_UNLOCK_CHECK(rdb_sysvars_mutex);
     return;
   }
@@ -12753,15 +12783,6 @@ void rocksdb_set_update_cf_options(THD *const /* unused */,
     }
   }
 
-  // Reset the pointers regardless of how much success we had with updating
-  // the CF options. This will results in consistent behavior and avoids
-  // dealing with cases when only a subset of CF-s was successfully updated.
-  if (val) {
-    *reinterpret_cast<char**>(var_ptr) = my_strdup(val,  MYF(0));
-  } else {
-    *reinterpret_cast<char**>(var_ptr) = nullptr;
-  }
-
   // Our caller (`plugin_var_memalloc_global_update`) will call `my_free` to
   // free up resources used before.