MDEV-39092 Copy Aria data and logs as part of backup#4971
MDEV-39092 Copy Aria data and logs as part of backup#4971mariadb-andrzejjarzabek wants to merge 2 commits intoMariaDB:MDEV-14992from
Conversation
This is an initial simple implementation which copies all the Aria files in the "end" phase of the backup. Nothing protects the copy from concurrent DDL or DML. Copying only works on MacOS (intended for refactoring to use common file copy method across engines and SQL layer).
|
|
mariadb-andrzejjarzabek
force-pushed
the
MDEV-39092
branch
5 times, most recently
from
dbc91ba to
4ef94be
Compare
Enable backup for non-Apple systems. Copy non-Aria-specific files *.frm and db.opt as part of Aria backup.
mariadb-andrzejjarzabek
force-pushed
the
MDEV-39092
branch
from
4ef94be to
c143ff2
Compare
| namespace backup | ||
| { | ||
|
|
||
| using target_dir_t= IF_WIN(const char*,int); |
There was a problem hiding this comment.
This violates MDEV-25861. We should not use the reserved POSIX _t suffix in any new type declarations.
|
|
||
| #pragma once | ||
|
|
||
| #include <cstdint> |
There was a problem hiding this comment.
Why would we include this header here? For uintptr_t perhaps? But we also use IF_WIN() and off_t without any declaration.
| using target_dir_t= IF_WIN(const char*,int); | ||
|
|
||
| inline void* to_void_ptr(target_dir_t tgt) noexcept | ||
| { | ||
| return IF_WIN(const_cast<char*>, reinterpret_cast<void*>)(tgt); | ||
| } | ||
|
|
||
| inline target_dir_t to_target_dir(void* ptr) noexcept | ||
| { | ||
| return IF_WIN(static_cast<const char*>(ptr), | ||
| int(reinterpret_cast<uintptr_t>(ptr))); | ||
| } | ||
|
|
||
| #ifndef _WIN32 |
There was a problem hiding this comment.
As far as I understand, this header is not at all usable on Windows, because Microsoft Windows does not provide primitives for copying data between file descriptors, other than TransmitFile(), which we would put into use in MDEV-38362.
I don’t see the value of including this header on Microsoft Windows (until we make it cover file streaming) or defining such conversion functions. On Windows, we will need a different way of copying files, based on file names. I don’t think we should try to shoehorn it into a common interface.
| file_logger.c my_dlerror.c crc32/crc32c.cc | ||
| my_timezone.cc my_compr_int.cc my_thread_name.cc | ||
| my_virtual_mem.c) | ||
| my_virtual_mem.c my_backup.cc) |
There was a problem hiding this comment.
The functions are about copying files, not backup. The file name is misleading.
| # endif | ||
| DBUG_ASSERT(ret <= 0); | ||
| return int(ret); | ||
| #endif | ||
| } | ||
| #endif | ||
| } No newline at end of file |
There was a problem hiding this comment.
The preprocessor directives are indented inconsistently, and the last line is missing a terminator.
| int (*backup_start)(THD *thd, IF_WIN(const char*,int) target); | ||
| int (*backup_start)(THD *thd, backup::target_dir_t target); |
There was a problem hiding this comment.
The name change is misleading. In MDEV-38362, this interface is subject to revision. Then it’s thinkable that the int argument would be the handle of an output a stream rather than of a target directory.
| #include "my_backup.h" | ||
|
|
||
| using namespace backup; |
There was a problem hiding this comment.
Can we define the file-copying API in C? Most of mysys is in C.
| /* TODO: .frm failes are nto Aria-specific, they are copied here as a stop-gap */ | ||
| const std::vector<std::string> Aria_backup::data_exts {".MAD"s, ".MAI", ".frm"s}; |
There was a problem hiding this comment.
Why is this not an array of const char*?
| int copy_file(const std::string &path) const noexcept | ||
| { | ||
| std::string src_path= std::string(maria_data_root) + "/" + path; | ||
| #ifndef _WIN32 | ||
| int ret_val = 0; | ||
| int src_fd = open(src_path.c_str(), O_RDONLY); |
There was a problem hiding this comment.
Outside Windows, could we please retain a handle to open(maria_data_root, O_DIRECTORY) and invoke openat(src_dir, path, O_RDONLY) to open the file? That would reduce the amount of memory heap operations.
I’d avoid std::string whenever possible. We already have too many issues around heap memory fragmentation. Even on Windows we could use NtCreateFile() to simulate openat(2). I will give it a try, because it would allow us to pass target directory handles rather than names across all platforms.
| int perform_backup() noexcept | ||
| { | ||
| if (scan_datadir()) | ||
| return 1; | ||
| if (copy_databases()) | ||
| return 1; | ||
| if (copy_control_file()) | ||
| return 1; | ||
| if (copy_logs()) | ||
| return 1; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
This is fundamentally incompatible with the handlerton::backup_step API. Even if you are for now copying everything in handlerton::backup_end, the internal API should be kept as compatible with handlerton::backup_step as possible: copying one file at a time. At least the copy_databases() step must be refactored so that the higher-level API is invoking something that copies one file at a time. Possibly, the copying of log files should be interleaved with that, like innodb_backup_step is doing.
4 participants
This is an initial simple implementation which copies all the Aria files in the "end" phase of the backup. Nothing protects the copy from concurrent DDL or DML. Copying only works on MacOS (intended for refactoring to use common file copy method across engines and SQL layer).