From 16ec069bdbf0a744e0b470b879d343866596a9be Mon Sep 17 00:00:00 2001 From: Vegetable26 Date: Mon, 8 Jan 2018 12:35:27 -0800 Subject: [PATCH] Added error message when client sets an invalid rocksdb_update_cf_options 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: d39ba651a72 --- .../r/rocksdb_update_cf_options.result | 38 +++++++++ .../r/rocksdb_update_cf_options_basic.result | 18 ++++- .../t/rocksdb_update_cf_options.test | 22 +++++ .../t/rocksdb_update_cf_options_basic.test | 19 ++++- storage/rocksdb/ha_rocksdb.cc | 81 ++++++++++++------- 5 files changed, 143 insertions(+), 35 deletions(-) create mode 100644 mysql-test/suite/rocksdb_sys_vars/r/rocksdb_update_cf_options.result create mode 100644 mysql-test/suite/rocksdb_sys_vars/t/rocksdb_update_cf_options.test diff --git a/mysql-test/suite/rocksdb_sys_vars/r/rocksdb_update_cf_options.result b/mysql-test/suite/rocksdb_sys_vars/r/rocksdb_update_cf_options.result new file mode 100644 index 000000000000..126b4cffe8bc --- /dev/null +++ b/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 diff --git a/mysql-test/suite/rocksdb_sys_vars/r/rocksdb_update_cf_options_basic.result b/mysql-test/suite/rocksdb_sys_vars/r/rocksdb_update_cf_options_basic.result index 5ad5394db29f..ba24fafd0ec2 100644 --- a/mysql-test/suite/rocksdb_sys_vars/r/rocksdb_update_cf_options_basic.result +++ b/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 diff --git a/mysql-test/suite/rocksdb_sys_vars/t/rocksdb_update_cf_options.test b/mysql-test/suite/rocksdb_sys_vars/t/rocksdb_update_cf_options.test new file mode 100644 index 000000000000..03626260cab8 --- /dev/null +++ b/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'; diff --git a/mysql-test/suite/rocksdb_sys_vars/t/rocksdb_update_cf_options_basic.test b/mysql-test/suite/rocksdb_sys_vars/t/rocksdb_update_cf_options_basic.test index 15d5d870ae64..382338a2d401 100644 --- a/mysql-test/suite/rocksdb_sys_vars/t/rocksdb_update_cf_options_basic.test +++ b/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; \ No newline at end of file +DROP TABLE t1; diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc index 247a1dc46a56..7e9cf05dbbf5 100644 --- a/storage/rocksdb/ha_rocksdb.cc +++ b/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(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(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(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(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(var_ptr) = my_strdup(val, MYF(0)); - } else { - *reinterpret_cast(var_ptr) = nullptr; - } - // Our caller (`plugin_var_memalloc_global_update`) will call `my_free` to // free up resources used before.