Pass ColumnFamilyHandle around by reference; wrap Iterator in unique_ptr

facebook / mysql-5.6

Facebook's branch of the Oracle MySQL database. This includes MyRocks.

http://myrocks.io

Other

2.46k stars 711 forks source link

Pass ColumnFamilyHandle around by reference; wrap Iterator in unique_ptr #1445

Closed laurynas-biveinis closed 2 months ago

laurynas-biveinis commented 3 months ago

In preparation for the range locking patch, clean up some code related to iterators:

Pass rocksdb::ColumnFamilyHandle around and store in classes by references instead of pointers. It is never nullptr, nor is ever reseated in the classes using it.
Wrap the rocksdb::Iterator objects in std::unique_ptr: change the factory method return types and class fields.
In both cases above mark the classes containing reference fields as non-copyable and non-moveable as needed, matching their current usage.
Mark touched methods [[nodiscard]] as applicable. In rdb_index_merge.h, remove many instances of redundant MY_ATTRIBUTE((nonnull)) too.
Make touched local vars auto, but with reference or pointer pulled out, sometimes avoiding redundant returned object copies.
Remove redundant instances of const from passed-by-value arguments.

laurynas-biveinis commented 3 months ago

Unfortunately there is a small conflict with https://github.com/facebook/mysql-5.6/pull/1444, the second PR to go in will have to be rebased

laurynas-biveinis commented 2 months ago

Fixed incorrect move ctor/assignment operator signatures and rebased

facebook-github-bot commented 2 months ago

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

luqun commented 2 months ago

could you also update storage/rocksdb/rdb_vector_db.cc

storage/rocksdb/rdb_vector_db.cc:192:22: error: no matching function for call to 'rdb_tx_get_iterator' m_iterator.reset(rdb_tx_get_iterator(

facebook-github-bot commented 2 months ago

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

laurynas-biveinis commented 2 months ago

could you also update storage/rocksdb/rdb_vector_db.cc

storage/rocksdb/rdb_vector_db.cc:192:22: error: no matching function for call to 'rdb_tx_get_iterator' m_iterator.reset(rdb_tx_get_iterator(

Today I learned about -DWITH_FB_VECTORDB=ON build, and it looks like I'll have to do some PRs for it as well. I was able to make the changes, but it's possible I missed something, please let me know.

Rebased too

facebook-github-bot commented 2 months ago

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

laurynas-biveinis commented 2 months ago

In storage/rocksdb/ha_rocksdb.cc:
   m_key_merge.emplace(
std::piecewise_construct, std::make_tuple(kd_gl_id),

std::make_tuple(

get_rocksdb_tmpdir(), THDVAR(get_thd(), merge_buf_size),

THDVAR(get_thd(), merge_combine_read_size),

THDVAR(get_thd(), merge_tmp_file_removal_delay_ms), cf));

std::make_tuple(get_rocksdb_tmpdir(), THDVAR(thd, merge_buf_size),

THDVAR(thd, merge_combine_read_size),

THDVAR(thd, merge_tmp_file_removal_delay_ms),

std::ref(cf)));
why do we need std::ref? since cf is already a reference type

Without std::ref, the compiler apparently trys to instantiate std::tuple<..., ColumnFamilyHandle>, and gets an error that ColumnFamilyHandle is an abstract class

facebook-github-bot commented 2 months ago

This pull request has been merged in facebook/mysql-5.6@b09f052510c5ac360fb6e30ef3f24ab2d915a7f2.