From 62bd4ff544b46e404fbb1da333a5b6a8354a43de Mon Sep 17 00:00:00 2001 From: Laurynas Biveinis Date: Thu, 1 Jun 2023 23:00:09 -0700 Subject: [PATCH] Cleanup non-null pointer args, increase const correctness (#1311) Upstream commit ID: https://github.com/facebook/mysql-5.6/commit/63b8aae2051b5d286661c8215716cff61eeec80b PS-8951: Merge percona-202305 (https://jira.percona.com/browse/PS-8951) Summary: Various minor cleanups All done or noticed in the native DD prototype work, are independent from it, and will help to reduce its diff. - `ha_rocksdb::create_key_defs` had non-null attribute for the 2nd and 3rd argument. It seems that the attribute non-null arg indexes got desynced, because the 3rd argument is a reference, and by usage the 1st and the 2nd args are pointers that cannot be `nullptr`. Instead of fixing the attribute indexes, convert pointers to references, which cannot be null by design. - Do the same for `ha_rocksdb::create_cfs`. At the same time make `tbl_def_arg` `const`, which previously was not, and make the function return `bool`, as it only returns two values for success and error. Convert it from `DBUG_ENTER_FUNC`/`DBUG_RETURN` to a newer (and lighter) `DBUG_TRACE`. - `ha_rocksdb::create_table` made `private`, pointer args converted to reference args. - Added missing error checking for RocksDB `WriteBatch::SingleDelete` calls - `ha_rocksdb::has_hidden_pk` made `static`, args changed from pointers to references, removed redundant `nonnull` attribute. - `ha_rocksdb::pk_index` args changed from pointers to references, removed redundant `nonnull` attribute. - `ha_rocksdb::set_last_rowkey` argument was marked unused even though it was used - `ha_rocksdb::check_and_lock_sk`, `ha_rocksdb::check_uniqueness_and_lock`, `ha_rocksdb::update_write_row`, `ha_rocksdb::reset`, `ha_rocksdb::calc_updated_indexes`: added asserts for relationships between arguments and `table->record[0]`/`table->record[1]` - `ha_rocksdb::adjust_handler_stats_sst_and_memtable`: reduce scope of two local vars - `Rdb_key_def`: introduce invalid index ID constant based on `dd::INVALID_OBJECT_ID`. Check it on construction and in the index ID getter. Replace direct field accesses with the getter call. - `Rdb_key_def::table_has_hidden_pk` arg changed from pointer to reference. - Replace `warn_unused_result` attribute with the standard `[[nodiscard]]` for all the touched signatures. Pull Request resolved: https://github.com/facebook/mysql-5.6/pull/1311 Differential Revision: D46075453 fbshipit-source-id: 5965df275a5a7371a015ebc0d5ac1ab59dc47a64 --- storage/rocksdb/ha_rocksdb.cc | 241 ++++++++++++++++--------------- storage/rocksdb/ha_rocksdb.h | 52 ++++--- storage/rocksdb/rdb_converter.cc | 4 +- storage/rocksdb/rdb_datadic.cc | 29 ++-- storage/rocksdb/rdb_datadic.h | 19 ++- 5 files changed, 183 insertions(+), 162 deletions(-) diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc index 09f9bbe831ef..ffecc917461b 100644 --- a/storage/rocksdb/ha_rocksdb.cc +++ b/storage/rocksdb/ha_rocksdb.cc @@ -58,6 +58,7 @@ #include "sql/sql_partition.h" #include "sql/sql_table.h" #include "sql/sql_thd_internal_api.h" +#include "sql/strfunc.h" #include "sql/table.h" /* RocksDB includes */ @@ -4736,8 +4737,7 @@ class Rdb_writebatch_impl : public Rdb_transaction { rocksdb::ColumnFamilyHandle *const column_family, const rocksdb::Slice &key, const bool assume_tracked) override { ++m_write_count; - m_batch->SingleDelete(column_family, key); - return rocksdb::Status::OK(); + return m_batch->SingleDelete(column_family, key); } bool has_modifications() const override { @@ -7345,7 +7345,7 @@ int ha_rocksdb::load_hidden_pk_value() { /* Get PK value from m_tbl_def->m_hidden_pk_info. */ longlong ha_rocksdb::update_hidden_pk_val() { - assert(has_hidden_pk(table)); + assert(has_hidden_pk(*table)); const longlong new_val = m_tbl_def->m_hidden_pk_val++; return new_val; } @@ -7354,7 +7354,7 @@ longlong ha_rocksdb::update_hidden_pk_val() { int ha_rocksdb::read_hidden_pk_id_from_rowkey(longlong *const hidden_pk_id) { assert(hidden_pk_id != nullptr); assert(table != nullptr); - assert(has_hidden_pk(table)); + assert(has_hidden_pk(*table)); rocksdb::Slice rowkey_slice(m_last_rowkey.ptr(), m_last_rowkey.length()); @@ -7663,7 +7663,7 @@ int ha_rocksdb::alloc_key_buffers(const TABLE *const table_arg, uint max_packed_sk_len = 0; uint pack_key_len = 0; - m_pk_descr = kd_arr[pk_index(table_arg, tbl_def_arg)]; + m_pk_descr = kd_arr[pk_index(*table_arg, *tbl_def_arg)]; // move this into get_table_handler() ?? m_pk_descr->setup(table_arg, tbl_def_arg); @@ -7856,7 +7856,7 @@ int ha_rocksdb::open(const char *const name, int mode, uint test_if_locked, } /* Load hidden pk only once on first use. */ - if (has_hidden_pk(table) && m_tbl_def->m_hidden_pk_val == 0 && + if (has_hidden_pk(*table) && m_tbl_def->m_hidden_pk_val == 0 && (err = load_hidden_pk_value()) != HA_EXIT_SUCCESS) { rdb_open_tables.release_table_handler(m_table_handler); free_key_buffers(); @@ -8054,15 +8054,13 @@ int ha_rocksdb::rdb_error_to_mysql(const rocksdb::Status &s, other - error, either given table ddl is not supported by rocksdb or OOM. */ int ha_rocksdb::create_key_defs( - const TABLE *const table_arg, Rdb_tbl_def *const tbl_def_arg, + const TABLE &table_arg, Rdb_tbl_def &tbl_def_arg, const std::string &actual_user_table_name, bool is_dd_tbl, const TABLE *const old_table_arg /* = nullptr */, - const Rdb_tbl_def *const old_tbl_def_arg - /* = nullptr */) const { + const Rdb_tbl_def *const old_tbl_def_arg /* = nullptr */) const { DBUG_ENTER_FUNC(); - assert(table_arg != nullptr); - assert(table_arg->s != nullptr); + assert(table_arg.s != nullptr); DBUG_EXECUTE_IF("rocksdb_truncate_failure", { my_error(ER_INTERNAL_ERROR, MYF(0), "Simulated truncation failure."); @@ -8083,7 +8081,7 @@ int ha_rocksdb::create_key_defs( allocated to each key definition. See below for more details. http://github.com/MySQLOnRocksDB/mysql-5.6/issues/86#issuecomment-138515501 */ - if (create_cfs(table_arg, tbl_def_arg, actual_user_table_name, &cfs, + if (create_cfs(table_arg, tbl_def_arg, actual_user_table_name, cfs, is_dd_tbl)) { DBUG_RETURN(HA_EXIT_FAILURE); } @@ -8093,12 +8091,12 @@ int ha_rocksdb::create_key_defs( uint ttl_field_offset; uint err; - if ((err = Rdb_key_def::extract_ttl_duration(table_arg, tbl_def_arg, + if ((err = Rdb_key_def::extract_ttl_duration(&table_arg, &tbl_def_arg, &ttl_duration))) { DBUG_RETURN(err); } - if ((err = Rdb_key_def::extract_ttl_col(table_arg, tbl_def_arg, &ttl_column, + if ((err = Rdb_key_def::extract_ttl_col(&table_arg, &tbl_def_arg, &ttl_column, &ttl_field_offset))) { DBUG_RETURN(err); } @@ -8126,9 +8124,9 @@ int ha_rocksdb::create_key_defs( Get the index numbers (this will update the next_index_number) and create Rdb_key_def structures. */ - for (uint i = 0; i < tbl_def_arg->m_key_count; i++) { - if (create_key_def(table_arg, i, tbl_def_arg, &m_key_descr_arr[i], cfs[i], - ttl_duration, ttl_column, is_dd_tbl)) { + for (uint i = 0; i < tbl_def_arg.m_key_count; i++) { + if (create_key_def(&table_arg, i, &tbl_def_arg, &m_key_descr_arr[i], + cfs[i], ttl_duration, ttl_column, is_dd_tbl)) { DBUG_RETURN(HA_EXIT_FAILURE); } } @@ -8138,8 +8136,8 @@ int ha_rocksdb::create_key_defs( in-place alter table. Copy over existing keys from the old_tbl_def and generate the necessary new key definitions if any. */ - if (create_inplace_key_defs(table_arg, tbl_def_arg, old_table_arg, - old_tbl_def_arg, cfs, ttl_duration, + if (create_inplace_key_defs(table_arg, tbl_def_arg, *old_table_arg, + *old_tbl_def_arg, cfs, ttl_duration, ttl_column)) { DBUG_RETURN(HA_EXIT_FAILURE); } @@ -8165,17 +8163,17 @@ int ha_rocksdb::create_key_defs( 0 - Ok other - error */ -int ha_rocksdb::create_cfs( - const TABLE *const table_arg, Rdb_tbl_def *const tbl_def_arg, +bool ha_rocksdb::create_cfs( + const TABLE &table_arg, const Rdb_tbl_def &tbl_def_arg, const std::string &actual_user_table_name, - std::array *const cfs, + std::array &cfs, bool is_dd_tbl) const { - DBUG_ENTER_FUNC(); + DBUG_TRACE; - assert(table_arg->s != nullptr); + assert(table_arg.s != nullptr); char tablename_sys[NAME_LEN + 1]; - my_core::filename_to_tablename(tbl_def_arg->base_tablename().c_str(), + my_core::filename_to_tablename(tbl_def_arg.base_tablename().c_str(), tablename_sys, sizeof(tablename_sys)); uint primary_key_index = pk_index(table_arg, tbl_def_arg); @@ -8185,31 +8183,31 @@ int ha_rocksdb::create_cfs( The first loop checks the index parameters and creates column families if necessary. */ - for (uint i = 0; i < tbl_def_arg->m_key_count; i++) { + for (uint i = 0; i < tbl_def_arg.m_key_count; i++) { std::shared_ptr cf_handle; // Internal consistency check to make sure that data in TABLE and // Rdb_tbl_def structures matches. Either both are missing or both are // specified. Yes, this is critical enough to make it into SHIP_ASSERT. - SHIP_ASSERT(!table_arg->part_info == tbl_def_arg->base_partition().empty()); + SHIP_ASSERT(!table_arg.part_info == tbl_def_arg.base_partition().empty()); // Generate the name for the column family to use. bool per_part_match_found = false; std::string cf_name = - generate_cf_name(i, table_arg, tbl_def_arg, &per_part_match_found); + generate_cf_name(i, &table_arg, &tbl_def_arg, &per_part_match_found); // Prevent create from using the system column family. if (cf_name == DEFAULT_SYSTEM_CF_NAME || cf_name == DEFAULT_TMP_SYSTEM_CF_NAME) { my_error(ER_WRONG_ARGUMENTS, MYF(0), "column family not valid for storing index data."); - DBUG_RETURN(HA_EXIT_FAILURE); + return true; } if (cf_name == DEFAULT_TMP_CF_NAME) { my_error(ER_WRONG_ARGUMENTS, MYF(0), "reserved column family for storing temporary table data."); - DBUG_RETURN(HA_EXIT_FAILURE); + return true; } // Populate cf_name for data dictionary table @@ -8218,16 +8216,16 @@ int ha_rocksdb::create_cfs( my_error( ER_WRONG_ARGUMENTS, MYF(0), "custom column family for data dictionary table is not allowed."); - DBUG_RETURN(HA_EXIT_FAILURE); + return true; } cf_name = DEFAULT_SYSTEM_CF_NAME; } // Populate cf_name for tmp tables. - else if (is_tmp_table(tbl_def_arg->full_tablename())) { + else if (is_tmp_table(tbl_def_arg.full_tablename())) { if (!cf_name.empty()) { my_error(ER_WRONG_ARGUMENTS, MYF(0), "custom column family for temporary table is not allowed."); - DBUG_RETURN(HA_EXIT_FAILURE); + return true; } cf_name = DEFAULT_TMP_CF_NAME; } @@ -8270,7 +8268,7 @@ int ha_rocksdb::create_cfs( cf_handle = cf_manager.get_or_create_cf(rdb, cf_name, !rocksdb_no_create_column_family); if (!cf_handle) { - DBUG_RETURN(HA_EXIT_FAILURE); + return true; } uint32 cf_id = cf_handle->GetID(); @@ -8280,27 +8278,27 @@ int ha_rocksdb::create_cfs( // check again when committing metadata changes. if (local_dict_manager->get_dropped_cf(cf_id)) { my_error(ER_CF_DROPPED, MYF(0), cf_name.c_str()); - DBUG_RETURN(HA_EXIT_FAILURE); + return true; } if (cf_manager.create_cf_flags_if_needed(local_dict_manager, cf_handle->GetID(), cf_name, per_part_match_found)) { - DBUG_RETURN(HA_EXIT_FAILURE); + return true; } } // The CF can be dropped from cf_manager at this point. This is part of // create table or alter table. If the drop happens before metadata are // written, create table or alter table will fail. - auto &cf = (*cfs)[i]; + auto &cf = cfs[i]; cf.cf_handle = cf_handle; cf.is_reverse_cf = Rdb_cf_manager::is_cf_name_reverse(cf_name.c_str()); cf.is_per_partition_cf = per_part_match_found; } - DBUG_RETURN(HA_EXIT_SUCCESS); + return false; } /* @@ -8314,31 +8312,28 @@ int ha_rocksdb::create_cfs( cfs Struct array which contains column family information @return - 0 - Ok - other - error, either given table ddl is not supported by rocksdb or OOM. + false - Ok + true - error, either given table ddl is not supported by rocksdb or OOM. */ -int ha_rocksdb::create_inplace_key_defs( - const TABLE *const table_arg, Rdb_tbl_def *const tbl_def_arg, - const TABLE *const old_table_arg, const Rdb_tbl_def *const old_tbl_def_arg, +bool ha_rocksdb::create_inplace_key_defs( + const TABLE &table_arg, Rdb_tbl_def &tbl_def_arg, + const TABLE &old_table_arg, const Rdb_tbl_def &old_tbl_def_arg, const std::array &cfs, uint64 ttl_duration, const std::string &ttl_column) const { - DBUG_ENTER_FUNC(); - - assert(table_arg != nullptr); - assert(tbl_def_arg != nullptr); - assert(old_tbl_def_arg != nullptr); + DBUG_TRACE; std::shared_ptr *const old_key_descr = - old_tbl_def_arg->m_key_descr_arr; + old_tbl_def_arg.m_key_descr_arr; std::shared_ptr *const new_key_descr = - tbl_def_arg->m_key_descr_arr; + tbl_def_arg.m_key_descr_arr; const std::unordered_map old_key_pos = - get_old_key_positions(table_arg, tbl_def_arg, old_table_arg, - old_tbl_def_arg); + get_old_key_positions(&table_arg, &tbl_def_arg, &old_table_arg, + &old_tbl_def_arg); uint i; - for (i = 0; i < tbl_def_arg->m_key_count; i++) { - const auto &it = old_key_pos.find(get_key_name(i, table_arg, tbl_def_arg)); + for (i = 0; i < tbl_def_arg.m_key_count; i++) { + const auto &it = + old_key_pos.find(get_key_name(i, &table_arg, &tbl_def_arg)); if (it != old_key_pos.end()) { /* @@ -8355,8 +8350,8 @@ int ha_rocksdb::create_inplace_key_defs( "Could not get index information for Index Number " "(%u,%u), table %s", gl_index_id.cf_id, gl_index_id.index_id, - old_tbl_def_arg->full_tablename().c_str()); - DBUG_RETURN(HA_EXIT_FAILURE); + old_tbl_def_arg.full_tablename().c_str()); + return true; } uint32 ttl_rec_offset = @@ -8379,18 +8374,18 @@ int ha_rocksdb::create_inplace_key_defs( dict_manager.get_dict_manager_selector_const(gl_index_id.cf_id) ->get_stats(gl_index_id), index_info.m_index_flags, ttl_rec_offset, ttl_duration); - } else if (create_key_def(table_arg, i, tbl_def_arg, &new_key_descr[i], + } else if (create_key_def(&table_arg, i, &tbl_def_arg, &new_key_descr[i], cfs[i], ttl_duration, ttl_column)) { - DBUG_RETURN(HA_EXIT_FAILURE); + return true; } assert(new_key_descr[i] != nullptr); - new_key_descr[i]->setup(table_arg, tbl_def_arg); + new_key_descr[i]->setup(&table_arg, &tbl_def_arg); } - tbl_def_arg->m_tbl_stats.set(new_key_descr[0]->m_stats.m_rows, 0, 0); + tbl_def_arg.m_tbl_stats.set(new_key_descr[0]->m_stats.m_rows, 0, 0); - DBUG_RETURN(HA_EXIT_SUCCESS); + return false; } std::unordered_map ha_rocksdb::get_old_key_positions( @@ -8823,14 +8818,14 @@ static void rdb_gen_normalized_tablename(const std::string *db, */ int ha_rocksdb::create_table(const std::string &table_name, const std::string &actual_user_table_name, - const TABLE *table_arg, + const TABLE &table_arg, ulonglong auto_increment_value, - dd::Table *table_def MY_ATTRIBUTE((__unused__))) { + const dd::Table *table_def [[maybe_unused]]) { DBUG_ENTER_FUNC(); int err; bool is_dd_tbl = dd::get_dictionary()->is_dd_table_name( - table_arg->s->db.str, table_arg->s->table_name.str); + table_arg.s->db.str, table_arg.s->table_name.str); DBUG_EXECUTE_IF("simulate_dd_table", { is_dd_tbl = true; }); auto local_dict_manager = dict_manager.get_dict_manager_selector_non_const( is_tmp_table(table_name)); @@ -8840,7 +8835,7 @@ int ha_rocksdb::create_table(const std::string &table_name, /* Create table/key descriptions and put them into the data dictionary */ m_tbl_def = new Rdb_tbl_def(table_name); - uint n_keys = table_arg->s->keys; + uint n_keys = table_arg.s->keys; /* If no primary key found, create a hidden PK and place it inside table @@ -8855,16 +8850,16 @@ int ha_rocksdb::create_table(const std::string &table_name, m_key_descr_arr = new std::shared_ptr[n_keys]; m_tbl_def->m_key_count = n_keys; - m_tbl_def->m_pk_index = table_arg->s->primary_key; + m_tbl_def->m_pk_index = table_arg.s->primary_key; m_tbl_def->m_key_descr_arr = m_key_descr_arr; err = - create_key_defs(table_arg, m_tbl_def, actual_user_table_name, is_dd_tbl); + create_key_defs(table_arg, *m_tbl_def, actual_user_table_name, is_dd_tbl); if (err != HA_EXIT_SUCCESS) { goto error; } - m_pk_descr = m_key_descr_arr[pk_index(table_arg, m_tbl_def)]; + m_pk_descr = m_key_descr_arr[pk_index(table_arg, *m_tbl_def)]; if (auto_increment_value) { bool autoinc_upgrade_test = false; @@ -9004,7 +8999,7 @@ int ha_rocksdb::create(const char *const name, TABLE *const table_arg, DBUG_RETURN(handle_rocksdb_corrupt_data_error()); } } - DBUG_RETURN(create_table(str, create_info->actual_user_table_name, table_arg, + DBUG_RETURN(create_table(str, create_info->actual_user_table_name, *table_arg, create_info->auto_increment_value, table_def)); } @@ -9067,7 +9062,7 @@ int ha_rocksdb::truncate_table(Rdb_tbl_def *tbl_def_arg, Attempt to create the table. If this succeeds, then drop the old table. Otherwise, try to restore it. */ - err = create_table(orig_tablename, actual_user_table_name, table_arg, + err = create_table(orig_tablename, actual_user_table_name, *table_arg, auto_increment_value, table_def); bool should_remove_old_table = true; @@ -9246,7 +9241,7 @@ int ha_rocksdb::secondary_index_read(const int keyno, uchar *const buf, */ longlong hidden_pk_id = 0; - if (has_hidden_pk(table) && + if (has_hidden_pk(*table) && (rc = read_hidden_pk_id_from_rowkey(&hidden_pk_id))) { goto done; } @@ -9626,7 +9621,7 @@ int ha_rocksdb::check(THD *const thd, HA_CHECK_OPT *const check_opt) { } longlong hidden_pk_id = 0; - if (has_hidden_pk(table) && + if (has_hidden_pk(*table) && read_hidden_pk_id_from_rowkey(&hidden_pk_id)) { goto error; } @@ -9767,7 +9762,7 @@ bool ha_rocksdb::is_blind_delete_enabled() { return (THDVAR(thd, blind_delete_primary_key) && thd->lex->sql_command == SQLCOM_DELETE && thd->lex->table_count == 1 && table->s->keys == 1 && - !has_hidden_pk(table) && !thd->rli_slave); + !has_hidden_pk(*table) && !thd->rli_slave); } /* @@ -10252,8 +10247,8 @@ void ha_rocksdb::unlock_row() { are covered by the PRIMARY KEY, SingleDelete can be used. */ bool ha_rocksdb::can_use_single_delete(const uint index) const { - return (index != pk_index(table, m_tbl_def) || - (!has_hidden_pk(table) && + return (index != pk_index(*table, *m_tbl_def) || + (!has_hidden_pk(*table) && table->key_info[index].actual_key_parts == table->s->fields)); } @@ -10302,9 +10297,8 @@ bool ha_rocksdb::do_bulk_commit(Rdb_transaction *const tx) { does not contain a primary key. (In which case we generate a hidden 'auto-incremented' pk.) */ -bool ha_rocksdb::has_hidden_pk(const TABLE *const table) const { - assert(table != nullptr); - return Rdb_key_def::table_has_hidden_pk(table); +bool ha_rocksdb::has_hidden_pk(const TABLE &t) { + return Rdb_key_def::table_has_hidden_pk(t); } /* @@ -10322,14 +10316,12 @@ bool ha_rocksdb::is_hidden_pk(const uint index, const TABLE *const table_arg, } /* Returns index of primary key */ -uint ha_rocksdb::pk_index(const TABLE *const table_arg, - const Rdb_tbl_def *const tbl_def_arg) { - assert(table_arg != nullptr); - assert(table_arg->s != nullptr); - assert(tbl_def_arg != nullptr); +uint ha_rocksdb::pk_index(const TABLE &table_arg, + const Rdb_tbl_def &tbl_def_arg) { + assert(table_arg.s != nullptr); - return table_arg->s->primary_key == MAX_INDEXES ? tbl_def_arg->m_key_count - 1 - : table_arg->s->primary_key; + return table_arg.s->primary_key == MAX_INDEXES ? tbl_def_arg.m_key_count - 1 + : table_arg.s->primary_key; } /* Returns the index into m_key_descr_arr array based on active_index */ @@ -10546,7 +10538,7 @@ int ha_rocksdb::get_pk_for_update(struct update_row_info *const row_info) { Get new row key for any insert, and any update where the pk is not hidden. Row key for updates with hidden pk is handled below. */ - if (!has_hidden_pk(table)) { + if (!has_hidden_pk(*table)) { row_info->hidden_pk_id = 0; row_info->new_pk_unpack_info = &m_pk_unpack_info; @@ -10681,6 +10673,10 @@ int ha_rocksdb::check_and_lock_sk( const uint key_id, const struct update_row_info &row_info, bool *const found, const bool skip_unique_check MY_ATTRIBUTE((__unused__))) { + assert( + (row_info.old_data == table->record[1] && + row_info.new_data == table->record[0]) || + (row_info.old_data == nullptr && row_info.new_data == table->record[0])); assert(found != nullptr); *found = false; @@ -10848,6 +10844,11 @@ int ha_rocksdb::check_and_lock_sk( int ha_rocksdb::check_uniqueness_and_lock( const struct update_row_info &row_info, bool pk_changed, bool skip_unique_check) { + assert( + (row_info.old_data == table->record[1] && + row_info.new_data == table->record[0]) || + (row_info.old_data == nullptr && row_info.new_data == table->record[0])); + Rdb_transaction *const tx = get_or_create_tx(ha_thd()); tx->acquire_snapshot(false); @@ -11199,15 +11200,21 @@ int ha_rocksdb::update_write_sk(const TABLE *const table_arg, rc = check_partial_index_prefix(table_arg, kd, row_info.tx, row_info.old_data); if (!rc) { - row_info.tx->get_indexed_write_batch()->SingleDelete(kd.get_cf(), - old_key_slice); + const auto s = row_info.tx->get_indexed_write_batch()->SingleDelete( + kd.get_cf(), old_key_slice); + if (!s.ok()) { + return row_info.tx->set_status_error(table->in_use, s, kd, m_tbl_def); + } } else if (rc != HA_ERR_KEY_NOT_FOUND) { return rc; } } else { // Unconditionally issue SD if rocksdb_partial_index_blind_delete. - row_info.tx->get_indexed_write_batch()->SingleDelete(kd.get_cf(), - old_key_slice); + const auto s = row_info.tx->get_indexed_write_batch()->SingleDelete( + kd.get_cf(), old_key_slice); + if (!s.ok()) { + return row_info.tx->set_status_error(table->in_use, s, kd, m_tbl_def); + } } } @@ -11293,6 +11300,9 @@ int ha_rocksdb::update_write_row(const uchar *const old_data, const bool skip_unique_check) { DBUG_ENTER_FUNC(); + assert((old_data == table->record[1] && new_data == table->record[0]) || + (old_data == nullptr && new_data == table->record[0])); + THD *thd = ha_thd(); if (thd && thd->killed) { DBUG_RETURN(HA_ERR_QUERY_INTERRUPTED); @@ -11426,7 +11436,7 @@ int ha_rocksdb::rnd_init(bool scan MY_ATTRIBUTE((__unused__))) { m_need_build_decoder = true; m_rnd_scan_started = false; DBUG_RETURN( - index_init(has_hidden_pk(table) ? MAX_KEY : pk_index(table, m_tbl_def), + index_init(has_hidden_pk(*table) ? MAX_KEY : pk_index(*table, *m_tbl_def), false /* sorted */)); } @@ -11591,7 +11601,7 @@ int ha_rocksdb::reset() { int ha_rocksdb::delete_row(const uchar *const buf) { DBUG_ENTER_FUNC(); - assert(buf != nullptr); + assert(buf == table->record[0] || buf == table->record[1]); ha_statistic_increment(&System_status_var::ha_delete_count); set_last_rowkey(buf); @@ -11599,7 +11609,7 @@ int ha_rocksdb::delete_row(const uchar *const buf) { rocksdb::Slice key_slice(m_last_rowkey.ptr(), m_last_rowkey.length()); Rdb_transaction *const tx = get_or_create_tx(table->in_use); - const uint index = pk_index(table, m_tbl_def); + const uint index = pk_index(*table, *m_tbl_def); rocksdb::Status s = delete_or_singledelete(index, tx, m_pk_descr->get_cf(), key_slice); if (!s.ok()) { @@ -11607,7 +11617,7 @@ int ha_rocksdb::delete_row(const uchar *const buf) { } longlong hidden_pk_id = 0; - if (m_tbl_def->m_key_count > 1 && has_hidden_pk(table)) { + if (m_tbl_def->m_key_count > 1 && has_hidden_pk(*table)) { int err = read_hidden_pk_id_from_rowkey(&hidden_pk_id); if (err) { DBUG_RETURN(err); @@ -11653,8 +11663,11 @@ int ha_rocksdb::delete_row(const uchar *const buf) { nullptr, false, hidden_pk_id); rocksdb::Slice secondary_key_slice( reinterpret_cast(m_sk_packed_tuple), packed_size); - tx->get_indexed_write_batch()->SingleDelete(kd.get_cf(), - secondary_key_slice); + s = tx->get_indexed_write_batch()->SingleDelete(kd.get_cf(), + secondary_key_slice); + if (!s.ok()) { + DBUG_RETURN(rdb_error_to_mysql(s)); + } } } @@ -11887,7 +11900,7 @@ void ha_rocksdb::position(const uchar *const record) { DBUG_ENTER_FUNC(); longlong hidden_pk_id = 0; - if (has_hidden_pk(table) && read_hidden_pk_id_from_rowkey(&hidden_pk_id)) { + if (has_hidden_pk(*table) && read_hidden_pk_id_from_rowkey(&hidden_pk_id)) { assert(false); // should never reach here } @@ -11966,7 +11979,7 @@ void ha_rocksdb::calc_updated_indexes() { /* Walk over all key parts, including the "extended key" suffix */ const uint key_parts = kd.get_key_parts(); for (uint kp = 0; kp < key_parts; kp++) { - if (has_hidden_pk(table) && kp + 1 == key_parts) break; + if (has_hidden_pk(*table) && kp + 1 == key_parts) break; Field *const field = kd.get_table_field_for_part_no(table, kp); if (bitmap_is_set(table->write_set, field->field_index())) { @@ -11988,8 +12001,8 @@ void ha_rocksdb::calc_updated_indexes() { int ha_rocksdb::update_row(const uchar *const old_data, uchar *const new_data) { DBUG_ENTER_FUNC(); - assert(old_data != nullptr); - assert(new_data != nullptr); + assert(old_data == table->record[1]); + assert(new_data == table->record[0]); assert(m_lock_rows == RDB_LOCK_WRITE); /* old_data points to record we're updating. It is the same as the record @@ -13365,13 +13378,12 @@ int ha_rocksdb::adjust_handler_stats_sst_and_memtable(ha_statistics *ha_stats, uchar buf[Rdb_key_def::INDEX_NUMBER_SIZE * 2]; std::shared_ptr pk_def = tbl_def->get_pk_def(); auto r = ha_rocksdb::get_range(*pk_def, buf); - uint64_t sz = 0; - - rocksdb::DB::SizeApproximationFlags include_flags = - rocksdb::DB::SizeApproximationFlags::INCLUDE_FILES; // recompute SST files stats only if records count is 0 if (ha_stats->records == 0) { + uint64_t sz = 0; + rocksdb::DB::SizeApproximationFlags include_flags = + rocksdb::DB::SizeApproximationFlags::INCLUDE_FILES; rdb->GetApproximateSizes(pk_def->get_cf(), &r, 1, &sz, include_flags); ha_stats->records += sz / ROCKSDB_ASSUMED_KEY_VALUE_DISK_SIZE; ha_stats->data_file_length += sz; @@ -13768,7 +13780,7 @@ my_core::enum_alter_inplace_result ha_rocksdb::check_if_supported_inplace_alter( /* We don't support unique keys on table w/ no primary keys */ if ((ha_alter_info->handler_flags & my_core::Alter_inplace_info::ADD_UNIQUE_INDEX) && - has_hidden_pk(altered_table)) { + has_hidden_pk(*altered_table)) { DBUG_RETURN(my_core::HA_ALTER_INPLACE_NOT_SUPPORTED); } @@ -13794,7 +13806,7 @@ my_core::enum_alter_inplace_result ha_rocksdb::check_if_supported_inplace_alter( DBUG_RETURN(my_core::HA_ALTER_INPLACE_NOT_SUPPORTED); // check ttl column - uint pk = pk_index(altered_table, m_tbl_def); + uint pk = pk_index(*altered_table, *m_tbl_def); std::string ttl_col = m_tbl_def->m_key_descr_arr[pk]->m_ttl_column; std::string altered_ttl_col; uint altered_ttl_field_offset; @@ -13882,7 +13894,7 @@ bool ha_rocksdb::prepare_inplace_alter_table( my_core::Alter_inplace_info::ADD_INDEX | my_core::Alter_inplace_info::ADD_UNIQUE_INDEX)) || update_comment) { - if (has_hidden_pk(altered_table)) { + if (has_hidden_pk(*altered_table)) { new_n_keys += 1; } @@ -13899,8 +13911,9 @@ bool ha_rocksdb::prepare_inplace_alter_table( new_tdef->m_hidden_pk_val = m_tbl_def->m_hidden_pk_val.load(std::memory_order_relaxed); - if (create_key_defs(altered_table, new_tdef, "" /*actual_user_table_name*/, - false /*is_dd_tbl*/, table, m_tbl_def)) { + if (create_key_defs(*altered_table, *new_tdef, + "" /*actual_user_table_name*/, false /*is_dd_tbl*/, + table, m_tbl_def)) { /* Delete the new key descriptors */ delete[] new_key_descr; @@ -14188,7 +14201,7 @@ int ha_rocksdb::inplace_populate_sk( ddl_manager.add_uncommitted_keydefs(indexes); } - const bool hidden_pk_exists = has_hidden_pk(table); + const bool hidden_pk_exists = has_hidden_pk(*table); Rdb_transaction *tx = get_or_create_tx(table->in_use); @@ -14470,7 +14483,7 @@ bool ha_rocksdb::commit_inplace_alter_table( m_tbl_def = ctx0->m_new_tdef; m_key_descr_arr = m_tbl_def->m_key_descr_arr; - m_pk_descr = m_key_descr_arr[pk_index(altered_table, m_tbl_def)]; + m_pk_descr = m_key_descr_arr[pk_index(*altered_table, *m_tbl_def)]; DBUG_EXECUTE_IF("rocksdb_commit_alter_table", { static constexpr char act[] = @@ -14583,7 +14596,7 @@ bool ha_rocksdb::commit_inplace_alter_table( m_tbl_def = ctx0->m_new_tdef; m_key_descr_arr = m_tbl_def->m_key_descr_arr; - m_pk_descr = m_key_descr_arr[pk_index(altered_table, m_tbl_def)]; + m_pk_descr = m_key_descr_arr[pk_index(*altered_table, *m_tbl_def)]; { auto local_dict_manager = @@ -16658,9 +16671,9 @@ bool ha_rocksdb::use_read_free_rpl() const { case read_free_rpl_type::OFF: DBUG_RETURN(false); case read_free_rpl_type::PK_ONLY: - DBUG_RETURN(!has_hidden_pk(table) && table->s->keys == 1); + DBUG_RETURN(!has_hidden_pk(*table) && table->s->keys == 1); case read_free_rpl_type::PK_SK: - DBUG_RETURN(!has_hidden_pk(table)); + DBUG_RETURN(!has_hidden_pk(*table)); } assert(false); diff --git a/storage/rocksdb/ha_rocksdb.h b/storage/rocksdb/ha_rocksdb.h index 93a4f5dc7452..56244380752a 100644 --- a/storage/rocksdb/ha_rocksdb.h +++ b/storage/rocksdb/ha_rocksdb.h @@ -323,12 +323,12 @@ class ha_rocksdb : public my_core::handler, public blob_buffer { */ uint m_dupp_errkey; - int create_key_defs(const TABLE *const table_arg, - Rdb_tbl_def *const tbl_def_arg, - const std::string &actual_user_table_name, bool is_dd_tbl, - const TABLE *const old_table_arg = nullptr, - const Rdb_tbl_def *const old_tbl_def_arg = nullptr) const - MY_ATTRIBUTE((__warn_unused_result__)); + [[nodiscard]] int create_key_defs( + const TABLE &table_arg, Rdb_tbl_def &tbl_def_arg, + const std::string &actual_user_table_name, bool is_dd_tbl, + const TABLE *const old_table_arg = nullptr, + const Rdb_tbl_def *const old_tbl_def_arg = nullptr) const; + int secondary_index_read(const int keyno, uchar *const buf, const rocksdb::Slice *key, const rocksdb::Slice *value, bool *skip_row) @@ -373,8 +373,7 @@ class ha_rocksdb : public my_core::handler, public blob_buffer { bool skip_unique_check() const MY_ATTRIBUTE((__warn_unused_result__)); bool do_bulk_commit(Rdb_transaction *const tx) MY_ATTRIBUTE((__warn_unused_result__)); - bool has_hidden_pk(const TABLE *const table) const - MY_ATTRIBUTE((__warn_unused_result__)); + [[nodiscard]] static bool has_hidden_pk(const TABLE &t); void update_row_stats(const operation_type &type, ulonglong count = 1); @@ -532,9 +531,8 @@ class ha_rocksdb : public my_core::handler, public blob_buffer { const Rdb_tbl_def *const tbl_def_arg) MY_ATTRIBUTE((__warn_unused_result__)); - static uint pk_index(const TABLE *const table_arg, - const Rdb_tbl_def *const tbl_def_arg) - MY_ATTRIBUTE((__warn_unused_result__)); + [[nodiscard]] static uint pk_index(const TABLE &table_arg, + const Rdb_tbl_def &tbl_def_arg); uint active_index_pos() MY_ATTRIBUTE((__warn_unused_result__)); @@ -694,11 +692,17 @@ class ha_rocksdb : public my_core::handler, public blob_buffer { INSTANT_ADD_COLUMN }; - int create_cfs(const TABLE *const table_arg, Rdb_tbl_def *const tbl_def_arg, - const std::string &actual_user_table_name, - std::array *const cfs, - bool is_dd_tbl) const - MY_ATTRIBUTE((__nonnull__, __warn_unused_result__)); + [[nodiscard]] int create_table(const std::string &table_name, + const std::string &actual_user_table_name, + const TABLE &table_arg, + ulonglong auto_increment_value, + const dd::Table *table_def); + + [[nodiscard]] bool create_cfs( + const TABLE &table_arg, const Rdb_tbl_def &tbl_def_arg, + const std::string &actual_user_table_name, + std::array &cfs, + bool is_dd_tbl) const; int create_key_def(const TABLE *const table_arg, const uint i, const Rdb_tbl_def *const tbl_def_arg, @@ -708,13 +712,11 @@ class ha_rocksdb : public my_core::handler, public blob_buffer { bool is_dd_tbl = false) const MY_ATTRIBUTE((__warn_unused_result__)); - int create_inplace_key_defs( - const TABLE *const table_arg, Rdb_tbl_def *vtbl_def_arg, - const TABLE *const old_table_arg, - const Rdb_tbl_def *const old_tbl_def_arg, - const std::array &cf, - uint64 ttl_duration, const std::string &ttl_column) const - MY_ATTRIBUTE((__warn_unused_result__)); + [[nodiscard]] bool create_inplace_key_defs( + const TABLE &table_arg, Rdb_tbl_def &tbl_def_arg, + const TABLE &old_table_arg, const Rdb_tbl_def &old_tbl_def_arg, + const std::array &cfs, + uint64 ttl_duration, const std::string &ttl_column) const; std::unordered_map get_old_key_positions( const TABLE *table_arg, const Rdb_tbl_def *tbl_def_arg, @@ -863,10 +865,6 @@ class ha_rocksdb : public my_core::handler, public blob_buffer { int create(const char *const name, TABLE *const form, HA_CREATE_INFO *const create_info, dd::Table *table_def) override MY_ATTRIBUTE((__warn_unused_result__)); - int create_table(const std::string &table_name, - const std::string &actual_user_table_name, - const TABLE *table_arg, ulonglong auto_increment_value, - dd::Table *table_def); int truncate_table(Rdb_tbl_def *tbl_def, const std::string &actual_user_table_name, TABLE *table_arg, ulonglong auto_increment_value, diff --git a/storage/rocksdb/rdb_converter.cc b/storage/rocksdb/rdb_converter.cc index ee2cc259552f..9f6ad679417b 100644 --- a/storage/rocksdb/rdb_converter.cc +++ b/storage/rocksdb/rdb_converter.cc @@ -387,7 +387,7 @@ Rdb_converter::~Rdb_converter() { void Rdb_converter::get_storage_type(Rdb_field_encoder *const encoder, const uint kp) { auto pk_descr = - m_tbl_def->m_key_descr_arr[ha_rocksdb::pk_index(m_table, m_tbl_def)]; + m_tbl_def->m_key_descr_arr[ha_rocksdb::pk_index(*m_table, *m_tbl_def)]; // STORE_SOME uses unpack_info. if (pk_descr->has_unpack_info(kp)) { @@ -534,7 +534,7 @@ void Rdb_converter::setup_field_encoders(const dd::Table *dd_table) { If hidden pk exists, we skip this check since the field will never be part of the hidden pk. */ - if (!Rdb_key_def::table_has_hidden_pk(m_table)) { + if (!Rdb_key_def::table_has_hidden_pk(*m_table)) { KEY *const pk_info = &m_table->key_info[m_table->s->primary_key]; for (uint kp = 0; kp < pk_info->user_defined_key_parts; kp++) { // key_part->fieldnr is counted from 1 diff --git a/storage/rocksdb/rdb_datadic.cc b/storage/rocksdb/rdb_datadic.cc index 6c8992ab21fc..a12bba4aea96 100644 --- a/storage/rocksdb/rdb_datadic.cc +++ b/storage/rocksdb/rdb_datadic.cc @@ -219,7 +219,7 @@ Rdb_key_field_iterator::Rdb_key_field_iterator( m_buf = buf; m_secondary_key = (key_def->m_index_type == Rdb_key_def::INDEX_TYPE_SECONDARY); - m_hidden_pk_exists = Rdb_key_def::table_has_hidden_pk(table); + m_hidden_pk_exists = Rdb_key_def::table_has_hidden_pk(*table); m_is_hidden_pk = (key_def->m_index_type == Rdb_key_def::INDEX_TYPE_HIDDEN_PRIMARY); m_curr_bitmap_pos = 0; @@ -311,8 +311,9 @@ Rdb_key_def::Rdb_key_def( m_prefix_extractor(nullptr), m_maxlength(0) // means 'not intialized' { + assert(m_index_number != INVALID_INDEX_NUMBER); mysql_mutex_init(0, &m_mutex, MY_MUTEX_INIT_FAST); - rdb_netbuf_store_index(m_index_number_storage_form, m_index_number); + rdb_netbuf_store_index(m_index_number_storage_form, get_index_number()); m_total_index_flags_length = calculate_index_flag_offset(m_index_flags_bitmap, MAX_FLAG); assert_IMP(m_index_type == INDEX_TYPE_SECONDARY && @@ -325,7 +326,7 @@ Rdb_key_def::Rdb_key_def( } Rdb_key_def::Rdb_key_def(const Rdb_key_def &k) - : m_index_number(k.m_index_number), + : m_index_number(k.get_index_number()), m_cf_handle(k.m_cf_handle), m_is_reverse_cf(k.m_is_reverse_cf), m_is_per_partition_cf(k.m_is_per_partition_cf), @@ -346,7 +347,7 @@ Rdb_key_def::Rdb_key_def(const Rdb_key_def &k) m_prefix_extractor(k.m_prefix_extractor), m_maxlength(k.m_maxlength) { mysql_mutex_init(0, &m_mutex, MY_MUTEX_INIT_FAST); - rdb_netbuf_store_index(m_index_number_storage_form, m_index_number); + rdb_netbuf_store_index(m_index_number_storage_form, get_index_number()); m_total_index_flags_length = calculate_index_flag_offset(m_index_flags_bitmap, MAX_FLAG); assert_IMP(m_index_type == INDEX_TYPE_SECONDARY && @@ -401,7 +402,7 @@ void Rdb_key_def::setup(const TABLE *const tbl, multiple threads, so there is a mutex to protect this code. */ const bool is_hidden_pk = (m_index_type == INDEX_TYPE_HIDDEN_PRIMARY); - const bool hidden_pk_exists = table_has_hidden_pk(tbl); + const bool hidden_pk_exists = table_has_hidden_pk(*tbl); const bool secondary_key = (m_index_type == INDEX_TYPE_SECONDARY); if (!m_maxlength) { RDB_MUTEX_LOCK_CHECK(m_mutex); @@ -763,7 +764,7 @@ uint Rdb_key_def::extract_partial_index_info( return HA_EXIT_FAILURE; } - if (table_has_hidden_pk(table_arg)) { + if (table_has_hidden_pk(*table_arg)) { my_printf_error(ER_NOT_SUPPORTED_YET, "Table with no primary key cannot have a partial index.", MYF(0)); @@ -1003,7 +1004,7 @@ uint Rdb_key_def::get_primary_key_tuple(const Rdb_key_def &pk_descr, assert(m_pk_key_parts); /* Put the PK number */ - rdb_netbuf_store_index(buf, pk_descr.m_index_number); + rdb_netbuf_store_index(buf, pk_descr.get_index_number()); buf += INDEX_NUMBER_SIZE; size += INDEX_NUMBER_SIZE; @@ -1057,7 +1058,7 @@ uint Rdb_key_def::get_memcmp_sk_parts(const TABLE *table, assert(sk_buffer != nullptr); assert(n_null_fields != nullptr); assert(m_keyno != table->s->primary_key); - assert(!table_has_hidden_pk(table)); + assert(!table_has_hidden_pk(*table)); uchar *buf = sk_buffer; @@ -1180,7 +1181,7 @@ void Rdb_key_def::get_lookup_bitmap(const TABLE *table, MY_BITMAP *map) const { bitmap_init(&maybe_covered_bitmap, nullptr, table->read_set->n_bits); for (uint i = 0; i < m_key_parts; i++) { - if (table_has_hidden_pk(table) && i + 1 == m_key_parts) { + if (table_has_hidden_pk(*table) && i + 1 == m_key_parts) { continue; } @@ -1358,9 +1359,9 @@ uint Rdb_key_def::pack_record(const TABLE *const tbl, uchar *const pack_buffer, size_t unpack_start_pos = size_t(-1); size_t unpack_len_pos = size_t(-1); size_t covered_bitmap_pos = size_t(-1); - const bool hidden_pk_exists = table_has_hidden_pk(tbl); + const bool hidden_pk_exists = table_has_hidden_pk(*tbl); - rdb_netbuf_store_index(tuple, m_index_number); + rdb_netbuf_store_index(tuple, get_index_number()); tuple += INDEX_NUMBER_SIZE; // If n_key_parts is 0, it means all columns. @@ -1524,7 +1525,7 @@ uint Rdb_key_def::pack_hidden_pk(const longlong hidden_pk_id, assert(packed_tuple != nullptr); uchar *tuple = packed_tuple; - rdb_netbuf_store_index(tuple, m_index_number); + rdb_netbuf_store_index(tuple, get_index_number()); tuple += INDEX_NUMBER_SIZE; assert(m_key_parts == 1); assert(is_storage_available(tuple - packed_tuple, @@ -2268,8 +2269,8 @@ int Rdb_key_def::unpack_record(TABLE *const table, uchar *const buf, return HA_EXIT_SUCCESS; } -bool Rdb_key_def::table_has_hidden_pk(const TABLE *const table) { - return table->s->primary_key == MAX_INDEXES; +bool Rdb_key_def::table_has_hidden_pk(const TABLE &table) { + return table.s->primary_key == MAX_INDEXES; } void Rdb_key_def::report_checksum_mismatch(const bool is_key, diff --git a/storage/rocksdb/rdb_datadic.h b/storage/rocksdb/rdb_datadic.h index 0d3049cff660..79e3d5cb052f 100644 --- a/storage/rocksdb/rdb_datadic.h +++ b/storage/rocksdb/rdb_datadic.h @@ -39,6 +39,9 @@ #include "./rdb_mutex_wrapper.h" #include "./rdb_utils.h" +/* Server header files */ +#include "sql/dd/object_id.h" + namespace myrocks { class Rdb_dict_manager; @@ -290,13 +293,13 @@ class Rdb_key_def { /* Get the key that is the "infimum" for this index */ inline void get_infimum_key(uchar *const key, uint *const size) const { - rdb_netbuf_store_index(key, m_index_number); + rdb_netbuf_store_index(key, get_index_number()); *size = INDEX_NUMBER_SIZE; } /* Get the key that is a "supremum" for this index */ inline void get_supremum_key(uchar *const key, uint *const size) const { - rdb_netbuf_store_index(key, m_index_number + 1); + rdb_netbuf_store_index(key, get_index_number() + 1); *size = INDEX_NUMBER_SIZE; } @@ -367,10 +370,13 @@ class Rdb_key_def { uint32 get_keyno() const { return m_keyno; } - uint32 get_index_number() const { return m_index_number; } + uint32 get_index_number() const { + assert(m_index_number != INVALID_INDEX_NUMBER); + return m_index_number; + } GL_INDEX_ID get_gl_index_id() const { - const GL_INDEX_ID gl_index_id = {m_cf_handle->GetID(), m_index_number}; + const GL_INDEX_ID gl_index_id = {m_cf_handle->GetID(), get_index_number()}; return gl_index_id; } @@ -621,7 +627,7 @@ class Rdb_key_def { inline bool has_unpack_info(const uint kp) const; /* Check if given table has a primary key */ - static bool table_has_hidden_pk(const TABLE *const table); + [[nodiscard]] static bool table_has_hidden_pk(const TABLE &table); void report_checksum_mismatch(const bool is_key, const char *const data, const size_t data_size) const; @@ -866,6 +872,9 @@ class Rdb_key_def { } #endif // NDEBUG + static constexpr auto INVALID_INDEX_NUMBER = + static_cast(dd::INVALID_OBJECT_ID); + /* Global number of this index (used as prefix in StorageFormat) */ const uint32 m_index_number;