From edc9526d29004be4f21b65a005b91452e9bbbe76 Mon Sep 17 00:00:00 2001 From: "George O. Lorch III" Date: Thu, 18 Oct 2018 11:21:01 -0700 Subject: [PATCH 1/2] PS-4937 : rocksdb_update_cf_options is useless when specified in my.cnf or on command line. - Added PLUGIN_VAR_NOCMDOPT to rocksdb_update_cf_options to prevent its use in my.cnf or command line. --- storage/rocksdb/ha_rocksdb.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc index 582f24b85da1..61f663094fc2 100644 --- a/storage/rocksdb/ha_rocksdb.cc +++ b/storage/rocksdb/ha_rocksdb.cc @@ -1214,7 +1214,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_RQCMDARG | PLUGIN_VAR_MEMALLOC | + PLUGIN_VAR_NOCMDOPT, "Option updates per column family for RocksDB", rocksdb_validate_update_cf_options, rocksdb_set_update_cf_options, nullptr); From 853dd48d3548fbbdaac71cab421a09f060172cad Mon Sep 17 00:00:00 2001 From: "George O. Lorch III" Date: Thu, 18 Oct 2018 15:16:56 -0700 Subject: [PATCH 2/2] PS-4946 : Add option to prevent implicit creation of column family in MyRocks - Added new option rocksdb_no_create_column_family to prevent a user from creating a new column family that is not already named in the rocksdb_override_cf_options. When enabled, users will get an error if they specify a non-existing column family in an index comment on CREATE or ALTER table for a MyRocks table. --- ...cksdb_no_create_column_family_basic.result | 7 +++ ...rocksdb_no_create_column_family_basic.test | 6 +++ mysql-test/suite/rocksdb/r/rocksdb.result | 1 + .../suite/rocksdb/r/rocksdb_cf_options.result | 11 ++++- .../suite/rocksdb/t/rocksdb_cf_options.test | 27 ++++++++++- storage/rocksdb/ha_rocksdb.cc | 11 ++++- storage/rocksdb/rdb_cf_manager.cc | 45 +++++++++++-------- storage/rocksdb/rdb_cf_manager.h | 4 +- storage/rocksdb/rdb_cf_options.cc | 9 ++-- storage/rocksdb/rdb_cf_options.h | 8 ++-- storage/rocksdb/rdb_datadic.cc | 3 +- 11 files changed, 102 insertions(+), 30 deletions(-) create mode 100644 mysql-test/suite/rocksdb.sys_vars/r/rocksdb_no_create_column_family_basic.result create mode 100644 mysql-test/suite/rocksdb.sys_vars/t/rocksdb_no_create_column_family_basic.test diff --git a/mysql-test/suite/rocksdb.sys_vars/r/rocksdb_no_create_column_family_basic.result b/mysql-test/suite/rocksdb.sys_vars/r/rocksdb_no_create_column_family_basic.result new file mode 100644 index 000000000000..3b8a8dc8f88b --- /dev/null +++ b/mysql-test/suite/rocksdb.sys_vars/r/rocksdb_no_create_column_family_basic.result @@ -0,0 +1,7 @@ +SET @start_global_value = @@global.ROCKSDB_NO_CREATE_COLUMN_FAMILY; +SELECT @start_global_value; +@start_global_value +0 +"Trying to set variable @@global.ROCKSDB_NO_CREATE_COLUMN_FAMILY to 444. It should fail because it is readonly." +SET @@global.ROCKSDB_NO_CREATE_COLUMN_FAMILY = 444; +ERROR HY000: Variable 'rocksdb_no_create_column_family' is a read only variable diff --git a/mysql-test/suite/rocksdb.sys_vars/t/rocksdb_no_create_column_family_basic.test b/mysql-test/suite/rocksdb.sys_vars/t/rocksdb_no_create_column_family_basic.test new file mode 100644 index 000000000000..29e455b85c06 --- /dev/null +++ b/mysql-test/suite/rocksdb.sys_vars/t/rocksdb_no_create_column_family_basic.test @@ -0,0 +1,6 @@ +--source include/have_rocksdb.inc + +--let $sys_var=ROCKSDB_NO_CREATE_COLUMN_FAMILY +--let $read_only=1 +--let $session=0 +--source ../include/rocksdb_sys_var.inc diff --git a/mysql-test/suite/rocksdb/r/rocksdb.result b/mysql-test/suite/rocksdb/r/rocksdb.result index 97bc305d2eb4..218c44ce84f1 100644 --- a/mysql-test/suite/rocksdb/r/rocksdb.result +++ b/mysql-test/suite/rocksdb/r/rocksdb.result @@ -967,6 +967,7 @@ rocksdb_merge_combine_read_size 1073741824 rocksdb_merge_tmp_file_removal_delay_ms 0 rocksdb_new_table_reader_for_compaction_inputs OFF rocksdb_no_block_cache OFF +rocksdb_no_create_column_family OFF rocksdb_override_cf_options rocksdb_paranoid_checks ON rocksdb_pause_background_work ON diff --git a/mysql-test/suite/rocksdb/r/rocksdb_cf_options.result b/mysql-test/suite/rocksdb/r/rocksdb_cf_options.result index 99a5dbf16060..a33e3ade7b43 100644 --- a/mysql-test/suite/rocksdb/r/rocksdb_cf_options.result +++ b/mysql-test/suite/rocksdb/r/rocksdb_cf_options.result @@ -59,6 +59,15 @@ z WRITE_BUFFER_SIZE 12582912 __system__ MAX_BYTES_FOR_LEVEL_MULTIPLIER 10.000000 __system__ TARGET_FILE_SIZE_BASE 1048576 __system__ WRITE_BUFFER_SIZE 12582912 +# restart:--rocksdb_no_create_column_family=1 --rocksdb_override_cf_options=cf3={write_buffer_size=8m;target_file_size_base=2m};cf4={write_buffer_size=16m;max_bytes_for_level_multiplier=8}; +include/assert.inc ["Expected cf3 and cf4 to not exist"] +create table t4 (a int, +primary key (a) comment 'cf3') engine=rocksdb; +include/assert.inc ["Expected cf3 to exist"] +create table t5 (a int, +primary key (a) comment 'nobodyknows') engine=rocksdb; +ERROR HY000: Incorrect arguments to CREATE | ALTER - can not find column family for storing index data and creation is not allowed. +include/assert.inc ["Expected cf4 to NOT exist"] # restart -drop table t1,t2,t3; +drop table t1,t2,t3,t4; diff --git a/mysql-test/suite/rocksdb/t/rocksdb_cf_options.test b/mysql-test/suite/rocksdb/t/rocksdb_cf_options.test index b9355f7b45ca..37973d3a4b7c 100644 --- a/mysql-test/suite/rocksdb/t/rocksdb_cf_options.test +++ b/mysql-test/suite/rocksdb/t/rocksdb_cf_options.test @@ -49,6 +49,30 @@ select cf_name, option_type, value 'MAX_BYTES_FOR_LEVEL_MULTIPLIER') order by cf_name, option_type; +# restart with no_create_column_family and column family options for new cf which is not yet created + +--let $restart_parameters=restart:--rocksdb_no_create_column_family=1 --rocksdb_override_cf_options=cf3={write_buffer_size=8m;target_file_size_base=2m};cf4={write_buffer_size=16m;max_bytes_for_level_multiplier=8}; +--source include/restart_mysqld.inc + +--let $assert_text="Expected cf3 and cf4 to not exist" +--let $assert_cond="[select count(distinct cf_name) from information_schema.rocksdb_cf_options where cf_name in (\'cf3\', \'cf4\')]" = 0 +--source include/assert.inc + +create table t4 (a int, + primary key (a) comment 'cf3') engine=rocksdb; + +--let $assert_text="Expected cf3 to exist" +--let $assert_cond="[select count(distinct cf_name) from information_schema.rocksdb_cf_options where cf_name in (\'cf3\')]" = 1 +--source include/assert.inc + +--error ER_WRONG_ARGUMENTS +create table t5 (a int, + primary key (a) comment 'nobodyknows') engine=rocksdb; + +--let $assert_text="Expected cf4 to NOT exist" +--let $assert_cond="[select count(distinct cf_name) from information_schema.rocksdb_cf_options where cf_name in (\'cf4\')]" = 0 +--source include/assert.inc + # syntax error in options (no equal sign) --exec echo "restart:--rocksdb_override_cf_options=cf1" > $_expect_file_name @@ -73,5 +97,4 @@ select cf_name, option_type, value --source include/restart_mysqld.inc --echo -drop table t1,t2,t3; - +drop table t1,t2,t3,t4; diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc index 61f663094fc2..5fb327aac8e0 100644 --- a/storage/rocksdb/ha_rocksdb.cc +++ b/storage/rocksdb/ha_rocksdb.cc @@ -500,6 +500,7 @@ static uint64_t rocksdb_write_policy = rocksdb::TxnDBWritePolicy::WRITE_COMMITTED; static my_bool rocksdb_error_on_suboptimal_collation = FALSE; static uint32_t rocksdb_stats_recalc_rate = 0; +static my_bool rocksdb_no_create_column_family = FALSE; std::atomic rocksdb_row_lock_deadlocks(0); std::atomic rocksdb_row_lock_wait_timeouts(0); @@ -1519,6 +1520,12 @@ static MYSQL_SYSVAR_BOOL(error_on_suboptimal_collation, "collation is used", nullptr, nullptr, false); +static MYSQL_SYSVAR_BOOL( + no_create_column_family, rocksdb_no_create_column_family, + PLUGIN_VAR_OPCMDARG | PLUGIN_VAR_READONLY, + "Do not allow creation of new Column Families through index comments.", + nullptr, nullptr, false); + static const int ROCKSDB_ASSUMED_KEY_VALUE_DISK_SIZE = 100; static struct st_mysql_sys_var *rocksdb_system_variables[] = { @@ -1666,6 +1673,7 @@ static struct st_mysql_sys_var *rocksdb_system_variables[] = { MYSQL_SYSVAR(large_prefix), MYSQL_SYSVAR(allow_to_start_after_corruption), MYSQL_SYSVAR(error_on_suboptimal_collation), + MYSQL_SYSVAR(no_create_column_family), MYSQL_SYSVAR(stats_recalc_rate), nullptr}; @@ -6241,7 +6249,8 @@ int ha_rocksdb::create_cfs( // Here's how `get_or_create_cf` will use the input parameters: // // `cf_name` - will be used as a CF name. - cf_handle = cf_manager.get_or_create_cf(rdb, cf_name); + cf_handle = cf_manager.get_or_create_cf(rdb, cf_name, + !rocksdb_no_create_column_family); if (!cf_handle) { DBUG_RETURN(HA_EXIT_FAILURE); diff --git a/storage/rocksdb/rdb_cf_manager.cc b/storage/rocksdb/rdb_cf_manager.cc index 51069dc6dd6e..a1e762ee7ca8 100644 --- a/storage/rocksdb/rdb_cf_manager.cc +++ b/storage/rocksdb/rdb_cf_manager.cc @@ -67,14 +67,14 @@ void Rdb_cf_manager::cleanup() { /* @brief - Find column family by name. If it doesn't exist, create it + Find column family by name. If it doesn't exist, maybe create it @detail See Rdb_cf_manager::get_cf */ rocksdb::ColumnFamilyHandle * Rdb_cf_manager::get_or_create_cf(rocksdb::DB *const rdb, - const std::string &cf_name_arg) { + const std::string &cf_name_arg, bool create) { DBUG_ASSERT(rdb != nullptr); rocksdb::ColumnFamilyHandle *cf_handle = nullptr; @@ -97,23 +97,32 @@ Rdb_cf_manager::get_or_create_cf(rocksdb::DB *const rdb, } else { /* Create a Column Family. */ rocksdb::ColumnFamilyOptions opts; - m_cf_options->get_cf_options(cf_name, &opts); - - // NO_LINT_DEBUG - sql_print_information("RocksDB: creating a column family %s", - cf_name.c_str()); - sql_print_information(" write_buffer_size=%ld", opts.write_buffer_size); - sql_print_information(" target_file_size_base=%" PRIu64, - opts.target_file_size_base); - - const rocksdb::Status s = - rdb->CreateColumnFamily(opts, cf_name, &cf_handle); - - if (s.ok()) { - m_cf_name_map[cf_handle->GetName()] = cf_handle; - m_cf_id_map[cf_handle->GetID()] = cf_handle; + bool cf_name_found = m_cf_options->get_cf_options(cf_name, &opts); + + if (create || cf_name_found) { + // NO_LINT_DEBUG + sql_print_information("RocksDB: creating a column family %s", + cf_name.c_str()); + sql_print_information(" write_buffer_size=%ld", + opts.write_buffer_size); + sql_print_information(" target_file_size_base=%" PRIu64, + opts.target_file_size_base); + + const rocksdb::Status s = + rdb->CreateColumnFamily(opts, cf_name, &cf_handle); + + if (s.ok()) { + m_cf_name_map[cf_handle->GetName()] = cf_handle; + m_cf_id_map[cf_handle->GetID()] = cf_handle; + } else { + cf_handle = nullptr; + } } else { - cf_handle = nullptr; + RDB_MUTEX_UNLOCK_CHECK(m_mutex); + my_error(ER_WRONG_ARGUMENTS, MYF(0), + "CREATE | ALTER - can not find column family for storing index " + "data and creation is not allowed."); + return nullptr; } } diff --git a/storage/rocksdb/rdb_cf_manager.h b/storage/rocksdb/rdb_cf_manager.h index 43e7af45481f..e241dbc38593 100644 --- a/storage/rocksdb/rdb_cf_manager.h +++ b/storage/rocksdb/rdb_cf_manager.h @@ -73,9 +73,11 @@ class Rdb_cf_manager { /* Used by CREATE TABLE. - cf_name=nullptr means use default column family + - create=true means create cf if missing, otherwise return nullptr */ rocksdb::ColumnFamilyHandle *get_or_create_cf(rocksdb::DB *const rdb, - const std::string &cf_name); + const std::string &cf_name, + bool create); /* Used by table open */ rocksdb::ColumnFamilyHandle *get_cf(const std::string &cf_name) const; diff --git a/storage/rocksdb/rdb_cf_options.cc b/storage/rocksdb/rdb_cf_options.cc index 3e973d1e7c92..b25af49e6a93 100644 --- a/storage/rocksdb/rdb_cf_options.cc +++ b/storage/rocksdb/rdb_cf_options.cc @@ -68,7 +68,7 @@ bool Rdb_cf_options::init( return true; } -void Rdb_cf_options::get(const std::string &cf_name, +bool Rdb_cf_options::get(const std::string &cf_name, rocksdb::ColumnFamilyOptions *const opts) { DBUG_ASSERT(opts != nullptr); @@ -80,7 +80,9 @@ void Rdb_cf_options::get(const std::string &cf_name, if (it != m_name_map.end()) { rocksdb::GetColumnFamilyOptionsFromString(*opts, it->second, opts); + return true; } + return false; } void Rdb_cf_options::update(const std::string &cf_name, @@ -330,16 +332,17 @@ Rdb_cf_options::get_cf_merge_operator(const std::string &cf_name) { : nullptr; } -void Rdb_cf_options::get_cf_options(const std::string &cf_name, +bool Rdb_cf_options::get_cf_options(const std::string &cf_name, rocksdb::ColumnFamilyOptions *const opts) { DBUG_ASSERT(opts != nullptr); *opts = m_default_cf_opts; - get(cf_name, opts); + bool ret = get(cf_name, opts); // Set the comparator according to 'rev:' opts->comparator = get_cf_comparator(cf_name); opts->merge_operator = get_cf_merge_operator(cf_name); + return ret; } } // namespace myrocks diff --git a/storage/rocksdb/rdb_cf_options.h b/storage/rocksdb/rdb_cf_options.h index 9aae8aded06c..2d80125d06a2 100644 --- a/storage/rocksdb/rdb_cf_options.h +++ b/storage/rocksdb/rdb_cf_options.h @@ -46,8 +46,9 @@ class Rdb_cf_options { Rdb_cf_options &operator=(const Rdb_cf_options &) = delete; Rdb_cf_options() = default; - void get(const std::string &cf_name, - rocksdb::ColumnFamilyOptions *const opts); + /* bool true return indicates cf_name was found */ + MY_NODISCARD bool get(const std::string &cf_name, + rocksdb::ColumnFamilyOptions *const opts); void update(const std::string &cf_name, const std::string &cf_options); @@ -67,7 +68,8 @@ class Rdb_cf_options { std::shared_ptr get_cf_merge_operator(const std::string &cf_name); - void get_cf_options(const std::string &cf_name, + /* bool true return indicates cf_name was found, otherwise default */ + bool get_cf_options(const std::string &cf_name, rocksdb::ColumnFamilyOptions *const opts); static bool parse_cf_options(const std::string &cf_options, diff --git a/storage/rocksdb/rdb_datadic.cc b/storage/rocksdb/rdb_datadic.cc index e4e5698ea38b..5270c5ff7a1a 100644 --- a/storage/rocksdb/rdb_datadic.cc +++ b/storage/rocksdb/rdb_datadic.cc @@ -4895,7 +4895,8 @@ bool Rdb_dict_manager::init(rocksdb::TransactionDB *const rdb_dict, m_db = rdb_dict; - m_system_cfh = cf_manager->get_or_create_cf(m_db, DEFAULT_SYSTEM_CF_NAME); + m_system_cfh = + cf_manager->get_or_create_cf(m_db, DEFAULT_SYSTEM_CF_NAME, true); rocksdb::ColumnFamilyHandle *default_cfh = cf_manager->get_cf(DEFAULT_CF_NAME);