Cleanup non-null pointer args, increase const correctness #1311

laurynas-biveinis · 2023-05-18T10:11:34Z

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.

facebook-github-bot · 2023-05-22T17:08:06Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-05-30T11:21:26Z

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

storage/rocksdb/rdb_datadic.h

storage/rocksdb/rdb_datadic.cc

facebook-github-bot · 2023-05-31T06:15:13Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

luqun · 2023-05-31T16:46:47Z

storage/rocksdb/ha_rocksdb.cc

+            row_info.tx->get_indexed_write_batch(m_tbl_def->get_table_type())
+                ->SingleDelete(kd.get_cf(), old_key_slice);
+        if (!s.ok()) {
+          return rdb_error_to_mysql(s);


nit: in ha_rocksdb::delete_row(), if SD or delete fail, it will call tx->set_status_error() instead of rdb_error_to_mysql(). Although these two function are similar, maybe we should have same behavior?

storage/rocksdb/ha_rocksdb.cc

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, GCC-style attribute replaced with a standard C++ one. - 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.

facebook-github-bot · 2023-06-01T09:03:45Z

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

laurynas-biveinis · 2023-06-01T09:04:11Z

Updated, ready for review

facebook-github-bot · 2023-06-01T09:06:37Z

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-06-01T16:38:45Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-06-01T21:13:30Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-06-02T05:51:32Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

luqun · 2023-06-02T05:58:12Z

still waiting for test result and will update when it is done.

facebook-github-bot · 2023-06-07T17:24:03Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-06-07T17:49:38Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-06-07T19:41:19Z

@bigfootjon has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

luqun · 2023-06-09T15:14:47Z

storage/rocksdb/ha_rocksdb.cc

+          row_info.tx->get_indexed_write_batch(m_tbl_def->get_table_type())
+              ->SingleDelete(kd.get_cf(), old_key_slice);
+      if (!s.ok()) {
+        return rdb_error_to_mysql(s);


nit: maybe also change this from rdb_error_to_mysql to row_info.tx->set_status_error().

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: #1311 Differential Revision: D46075453 fbshipit-source-id: 5965df2

…ness 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: facebook#1311 GitHub Author: Laurynas Biveinis <[email protected]> Test Plan: Imported from GitHub, without a `Test Plan:` line. Reviewers: herman, chni, #mysql_eng Reviewed By: herman Subscribers: herman, [email protected] Differential Revision: https://phabricator.intern.facebook.com/D46075453 Tags: accept2ship

) 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: facebook#1311 Differential Revision: D46075453 fbshipit-source-id: 5965df2

) 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: facebook#1311 Differential Revision: D46075453

Upstream commit ID: facebook/mysql-5.6@63b8aae 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: facebook/mysql-5.6#1311 Differential Revision: D46075453 fbshipit-source-id: 5965df275a5a7371a015ebc0d5ac1ab59dc47a64

) 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: facebook#1311 Differential Revision: D46075453

Upstream commit ID: facebook/mysql-5.6@63b8aae 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: facebook/mysql-5.6#1311 Differential Revision: D46075453 fbshipit-source-id: 5965df275a5a7371a015ebc0d5ac1ab59dc47a64

) 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: facebook#1311 Differential Revision: D46075453

Upstream commit ID: facebook/mysql-5.6@63b8aae 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: facebook/mysql-5.6#1311 Differential Revision: D46075453 fbshipit-source-id: 5965df275a5a7371a015ebc0d5ac1ab59dc47a64

) 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: facebook#1311 Differential Revision: D46075453

Upstream commit ID: facebook/mysql-5.6@63b8aae 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: facebook/mysql-5.6#1311 Differential Revision: D46075453 fbshipit-source-id: 5965df275a5a7371a015ebc0d5ac1ab59dc47a64

facebook-github-bot added the CLA Signed label May 18, 2023

luqun reviewed May 31, 2023

View reviewed changes

storage/rocksdb/rdb_datadic.h Show resolved Hide resolved

storage/rocksdb/rdb_datadic.cc Show resolved Hide resolved

luqun reviewed May 31, 2023

View reviewed changes

luqun reviewed Jun 1, 2023

View reviewed changes

storage/rocksdb/ha_rocksdb.cc Outdated Show resolved Hide resolved

laurynas-biveinis force-pushed the myr-create_key_defs-fix branch from a00447b to d275f0f Compare June 1, 2023 09:03

luqun reviewed Jun 9, 2023

View reviewed changes

laurynas-biveinis closed this Jun 12, 2023

laurynas-biveinis deleted the myr-create_key_defs-fix branch June 12, 2023 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup non-null pointer args, increase const correctness #1311

Cleanup non-null pointer args, increase const correctness #1311

laurynas-biveinis commented May 18, 2023 •

edited

Loading

facebook-github-bot commented May 22, 2023

facebook-github-bot commented May 30, 2023

facebook-github-bot commented May 31, 2023

luqun May 31, 2023

facebook-github-bot commented Jun 1, 2023

laurynas-biveinis commented Jun 1, 2023

facebook-github-bot commented Jun 1, 2023

facebook-github-bot commented Jun 1, 2023

facebook-github-bot commented Jun 1, 2023

facebook-github-bot commented Jun 2, 2023

luqun commented Jun 2, 2023

facebook-github-bot commented Jun 7, 2023

facebook-github-bot commented Jun 7, 2023

facebook-github-bot commented Jun 7, 2023

luqun Jun 9, 2023

Cleanup non-null pointer args, increase const correctness #1311

Cleanup non-null pointer args, increase const correctness #1311

Conversation

laurynas-biveinis commented May 18, 2023 • edited Loading

facebook-github-bot commented May 22, 2023

facebook-github-bot commented May 30, 2023

facebook-github-bot commented May 31, 2023

luqun May 31, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Jun 1, 2023

laurynas-biveinis commented Jun 1, 2023

facebook-github-bot commented Jun 1, 2023

facebook-github-bot commented Jun 1, 2023

facebook-github-bot commented Jun 1, 2023

facebook-github-bot commented Jun 2, 2023

luqun commented Jun 2, 2023

facebook-github-bot commented Jun 7, 2023

facebook-github-bot commented Jun 7, 2023

facebook-github-bot commented Jun 7, 2023

luqun Jun 9, 2023

Choose a reason for hiding this comment

laurynas-biveinis commented May 18, 2023 •

edited

Loading