Rdb clone fixes

[laurynas-biveinis]

Clone: fix race condition that may skip some SST files A donor thread in the main copy loop took a file to send, released the donor state mutex, and then opened the file. If an ENOENT was received at this point, it was assumed that this was a stale SST file from an older checkpoint that was rolled since.

Because the donor state mutex was released between taking of the file and opening it, the following race was possible: 1) Thread 1 takes the file 2) Thread 2 decides to roll the checkpoint, the old checkpoint is deleted 3) Thread 1 tries to open the file, gets ENOENT 4) Thread 2 creates the new checkpoint, the file re-appears, but it's too late.

Rolling the checkpoint in a donor state mutex critical section is a possible fix, but such section would do a lot of I/O, serializing the parallel threads. Instead, fix by taking the file and opening it in the same critical section.

Do not leave an empty WAL temp dir behind after clone if rocksdb_wal_dir != rocksdb_datadir

Add asserts and __builtin_unreachable after exhaustive switch statements

To help the compiler with the case where it cannot tell that the switch is actually exhaustive. This fixes, with GCC:

/home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/clone/common.cc: In function ‘bool myrocks::clone::should_use_direct_io(const string&, myrocks::clone::mode_for_direct_io)’: /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/clone/common.cc:295:1: error: control reaches end of non-void function [-Werror=return-type] 295 | } | ^ /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/clone/client.cc: In function ‘int myrocks::rocksdb_clone_apply(handlerton, THD, const uchar, uint, uint, int, Ha_clone_cbk)’: /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/clone/client.cc:821:1: error: control reaches end of non-void function [-Werror=return-type] 821 | } | ^

Remove redundant constexpr

This fixes, under GCC:

In file included from /usr/include/c++/11/cassert:44, from /home/laurynas/vilniusdb/rdb-clone-fixes/rocksdb/include/rocksdb/file_checksum.h:11, from /home/laurynas/vilniusdb/rdb-clone-fixes/rocksdb/include/rocksdb/options.h:26, from /home/laurynas/vilniusdb/rdb-clone-fixes/rocksdb/include/rocksdb/file_system.h:34, from /home/laurynas/vilniusdb/rdb-clone-fixes/rocksdb/include/rocksdb/sst_file_manager.h:13, from /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/clone/../ha_rocksdb.h:45, from /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/clone/client.cc:30: /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/clone/client.cc: In member function ‘constexpr void {anonymous}::client::assert_inactive() const’: /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/clone/client.cc:288:37: error: call to non-‘constexpr’ function ‘bool std::unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::empty() const [with _Key = long unsigned int; _Tp = {anonymous}::file_in_progress; _Hash = std::hash; _Pred = std::equal_to; _Alloc = std::allocator<std::pair<const long unsigned int, {anonymous}::file_in_progress> >]’ 288 | assert(m_files_in_progress.empty()); | ~~~~~^~ In file included from /usr/include/c++/11/unordered_map:47, from /usr/include/c++/11/functional:61, from /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/clone/common.h:25, from /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/clone/client.cc:17: /usr/include/c++/11/bits/unordered_map.h:305:7: note: ‘bool std::unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::empty() const [with _Key = long unsigned int; _Tp = {anonymous}::file_in_progress; _Hash = std::hash; _Pred = std::equal_to; _Alloc = std::allocator<std::pair<const long unsigned int, {anonymous}::file_in_progress> >]’ declared here 305 | empty() const noexcept | ^~~~~

Factor out clone metadata deserialization buffer length checking

Add an overflow check and a cast, extract resulting code to a new helper function.

This fixes GCC warnings:

Fix a cast

"const" was redundant because it indicated that the pointer itself, not its contents, were const. At the same time replace the C style cast with a C++ one, and drop a redundant trailing semicolon.

This fixes a GCC warning:

In file included from /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/rdb_cf_options.cc:36: /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/./rdb_sst_partitioner_factory.h: In function ‘std::string myrocks::get_index_key(myrocks::Index_id)’: /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/./rdb_sst_partitioner_factory.h:38:26: error: type qualifiers ignored on cast result type [-Werror=ignored-qualifiers] 38 | rdb_netbuf_store_index((uchar *const)buf.data(), index_id); | ^~~~~~~~

Use GCC-compatible attribute((nonnull)) syntax

Clang allows adding this attribute to individual function parameters, while GCC only allows annotating the whole declaration with relevant parameter indexes. Use the GCC-compatible syntax.

This fixes

In file included from /home/laurynas/vilniusdb/rdb-clone-fixes/include/my_checksum.h:39, from /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/./ha_rocksdb.h:32, from /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/ha_rocksdb.cc:24: /home/laurynas/vilniusdb/rdb-clone-fixes/include/my_compiler.h:100:40: error: ‘nonnull’ attribute only applies to function types [-Werror=attributes] 100 | #define MY_ATTRIBUTE(A) attribute(A) | ^ /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/./ha_rocksdb.h:495:43: note: in expansion of macro ‘MY_ATTRIBUTE’ 495 | int rename_table(const char const from MY_ATTRIBUTE((nonnull)), | ^~~~ /home/laurynas/vilniusdb/rdb-clone-fixes/include/my_compiler.h:100:40: error: ‘nonnull’ attribute only applies to function types [-Werror=attributes] 100 | #define MY_ATTRIBUTE(A) attribute(A) | ^ /home/laurynas/vilniusdb/rdb-clone-fixes/storage/rocksdb/./ha_rocksdb.h:496:41: note: in expansion of macro ‘MY_ATTRIBUTE’ 496 | const char const to MY_ATTRIBUTE((nonnull)), | ^~~~

Handle my_tell return value correctly. Fixes #1270

The incorrect my_tell value handling was pointed out by GCC:

[ 22%] Building CXX object plugin/clone/CMakeFiles/clone.dir/src/clone_os.cc.o /home/laurynas/vilniusdb/fb-mysql/plugin/clone/src/clone_os.cc: In function ‘int clone_os_copy_buf_to_file(uchar, Ha_clone_file, uint, const char)’: /home/laurynas/vilniusdb/fb-mysql/plugin/clone/src/clone_os.cc:312:21: error: comparison of unsigned expression in ‘< 0’ is always false [-Werror=type-limits] 312 | if (file_size < 0) { | ~~^~~ cc1plus: all warnings being treated as errors

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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

[laurynas-biveinis]

One more fix commit added: "Do not leave an empty WAL temp dir behind after clone if rocksdb_wal_dir != rocksdb_datadir", https://github.com/facebook/mysql-5.6/pull/1271/commits/c3fcd4c3cd9f154537af642e11b4640b73714f03

[facebook-github-bot]

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

[facebook-github-bot]

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

[laurynas-biveinis]

Marking as draft because 4b86ba2 testing is in progress, the older commits are fine as before.

[laurynas-biveinis]

4b86ba2 testing passed

[laurynas-biveinis]

@hermanlee, moved the race-fixing commit to its own PR so that this one with simple fixes can be merged faster. Now this PR is at the same state as it was at your last import