MDEV-39092 Copy Aria data and logs as part of backup by mariadb-andrzejjarzabek · Pull Request #4971 · MariaDB/server

mariadb-andrzejjarzabek · 2026-04-22T08:28:29Z

An interim solution with some room for optimization:

All DDL is blcoked while Aria is being backed up
All table caches purged when Aria backup starts (including for non-Aria tables)
Writes to non-transactional, but not to transactional tables are blocked when table files are being backed up
All commits blocked when Aria log files are being backed up

CLAassistant · 2026-04-22T08:28:38Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

dr-m · 2026-06-09T07:01:04Z

    backup_target target;
 #ifndef _WIN32
    const int datadir_fd;
 #endif


Why is there a declaration of datadir_fd in the first place? It turns out that it is never being assigned, only passed to openat(2). This seems to rely on zero-initialization and some undefined behaviour. For example, in Linux, AT_FDCWD is defined as -100.

I noticed later that this data member is being initialized from a call to open(…, O_DIRECTORY) in the constructor. I think that this is bad practice; we should allow the singleton object to be initialized statically.

It is unclear when if ever the directory handle would be closed. I suspect that we would hold the handle open until the server is shut down.

datadir_fd is initialized in the Aria_backup constructor.

Is it a reasonable trade-off to have a descriptor permanently opened for backups for the purpose of just one plugin? Should each plugin that performs the backup keep an open descriptor to the data directory? When adding another bit of functionality (not backup) to any part of the server or a plugin, with its own class/module/translation unit, which needs to open files in the data directory, should it also maintain a file descriptor to the data directory? There may be a case for defining an API for the server to maintain a single descriptor and make it available for other code, but given that there isn't one, I would argue that opening a descriptor for the duration of the backup and closing it on backup end. A backup isn't a fine-grained operation which we expect to be performed multiple times per second (or even minute), but we expect that many hours may pass between successive backups. And every backup will typically open hundreds or thousands of files, so saving the time and I/O of one open/close is not significant and it's not clear that maintaining an additional open descriptor the whole time the server is running is is balanced out by that.

In any case I propose putting that discussion off for a possible future improvement, where we could discuss defining an API to make a data directory descriptor available to the whole server.

dr-m · 2026-06-17T08:24:52Z

+  case BACKUP_PHASE_NO_DML_NON_TRANS:
+    /* FIXME: Would be better to selectively purge only the tables we need. */
+    tc_purge();
+    tdc_purge(true);
+    return 0;


The comment is missing a reference to a ticket where this performance problem will be fixed. As far as I understand, we only need such cleanup for ENGINE=Aria and ENGINE=MyISAM. We don’t want unnecessary disruption of ENGINE=InnoDB. The BACKUP_PHASE_NO_COMMIT that will be executed a couple of steps after this one will be disruptive enough.

This introduces a basic driver Sql_cmd_backup, storage engine interfaces, and basic copying of InnoDB, Aria, and MyISAM files. For streaming, we aim to generate streams that are compatible with GNU tar --format=oldgnu, which is also supported by the built-in BSD tar on FreeBSD, macOS as well as Microsoft Windows. That is, to extract files from a backup stream, you can use the standard tar utility of the operating system, instead of anything nonstandard like xbstream or mbstream. TODO: Support partial backup and restore. This should be done by implementing some configuration parameters as well as a predicate that checks if a file name pattern should be included. TODO: Back up ENGINE=RocksDB. TODO: Refactor the crude first implementation of Aria_backup that is based on the work of Andrzej Jarzabek, to copy as many files as concurrently as possible while holding the minimum amount of locks. --- backup_target: A structured data type to represent a target directory. On Microsoft Windows, we must use directory paths because there is no variant of CopyFileEx() that would work on file handles. backup_sink: Wraps a per-thread output stream as well as storage engine specific context. handlerton::backup_start(), handlerton::backup_end(): Invoked at the start or end of a backup phase, in the thread that executes a BACKUP SERVER statement. handlerton::backup_step(): A backup step that can be invoked from multiple threads concurrently, between the execution of the corresponding handlerton::backup_start() and handlerton::backup_end() of the same phase. copy_entire_file(): A file copying service for POSIX systems. copy_file(): A sparse file-copying service for all systems. backup_stream_append_async(): A variant of backup_stream_append() where the source file region is guaranteed to be immutable after the call returns. We must not use Linux sendfile(2) for copying data files that may be modified in place, because it could introduce a race condition between a page write that runs concurrently with a child process that is reading the data from the pipe. InnoDB_backup::context: Backup context, attached to backup_sink so that context can continue to exist between the time a BACKUP SERVER releases all locks and another BACKUP SERVER starts executing, with innodb_backup pointing to the new backup, while the old backup is still being finished. fil_space_t::write_or_backup: Keep track of in-flight page writes and pending backup operation. We must not allow them concurrently, because that could lead into torn pages in the backup. fil_space_t::backup_end: The first page number that is not being backed up (by default 0, to indicate that no backup is in progress). fil_space_t::BACKUP_BATCH_SIZE: The number of preceding pages that will be covered by fil_space_t::backup_end. This is the unit of "page range locking" during InnoDB backup. log_sys.backup: Whether BACKUP SERVER is in progress. The purpose of this is to make BACKUP SERVER prevent the concurrent execution of SET GLOBAL innodb_log_archive=OFF or SET GLOBAL innodb_log_file_size when innodb_log_archive=OFF. log_sys.archived_checkpoint: Keep track of the earliest available checkpoint, corresponding to log_sys.archived_lsn. This reflects SET GLOBAL innodb_log_recovery_start (which is settable now), for incremental backup. buf_flush_list_space(): Check for concurrent backup before writing each page. This is inefficient, but this function may be invoked from multiple threads concurrently, and it cannot be changed easily, especially for fil_crypt_thread().

Eliminate the buggy Source_dir that broke error propagation. Also, do not misidentify Linux block devices or sockets as directories.

InnoDB_backup::context::de_hardlink(): Avoid copying garbage at the start of the first file.

fil_system.have_all_spaces: A flag that indicates whether all tablespace metadata is present in fil_system.

dr-m · 2026-06-26T13:22:53Z

+static my_bool copy_misc_files(const backup_target *target,
+                               const backup_sink *sink)
+{
+  Dir_scan datadir(".", MYF(MY_WANT_STAT));
+  if (datadir.is_error())
+    return true;
+  std::unordered_set<std::string> ensured_dirs;
+  int error= datadir.for_each([target, sink, &ensured_dirs](const fileinfo &fi)


As I pointed out in this comment in #4817, executing potentially failing operations in a constructor is not a good design. This must be refactored in the style of fffd4b6. Inline iteration (range for using st_::span or std::span) should be more efficient than lambda functions, which cannot always be inlined by compilers. In this function we even have two nested lambda function iterations, on two different Dir_scan.

dr-m · 2026-06-26T13:23:53Z

+struct backup_sink;

 /** A payload chunk in a sparse file that is being streamed */
 struct backup_chunk


What is the point of adding a forward declaration right before an actual declaration?

dr-m · 2026-06-26T13:26:11Z

+ /* RAII wrapper for my_dir() */
+class Dir_scan
+{
+public:
+  explicit Dir_scan(const char* path, myf flags) noexcept
+  {
+    dir_info= my_dir(path, flags);
+    if (!dir_info)
+    {
+      my_error(ER_CANT_READ_DIR, MYF(0), path, my_errno);
+    }
+  }
+  ~Dir_scan() noexcept
+  {
+    my_dirend(dir_info);
+  }
+  bool is_error() const noexcept
+  {
+    return !dir_info;
+  }


Please, do not execute potentially failing operations in a constructor.

Why is the destructor potentially invoking my_dirend(nullptr)?

What purpose does explicit have on a constructor that takes more than 1 parameter?

dr-m · 2026-06-26T13:27:33Z

+  MY_DIR *dir_info {nullptr};
+};
+
+std::string build_path(const char *base_path, const char *filename) noexcept;


It would be better to refactor my_dir() so that it can return a directory handle, on which openat may be invoked. We generally try to avoid operations on std::string because there already are enough issues with heap fragmentation.

dr-m · 2026-06-26T13:29:56Z

+    if (phase == BACKUP_PHASE_NO_DDL)
+      fail= copy_misc_files(&target_phase->target, &target_phase->sink);
+    if (fail)
+      break;


This is unnecessarily testing fail when the first if does not hold.

dr-m · 2026-06-26T13:30:48Z

+                               const backup_sink *sink)
+{
+  Dir_scan datadir(".", MYF(MY_WANT_STAT));
+  if (datadir.is_error())


In the embedded server library libmariadbd, the data directory is not necessarily the current directory.

dr-m · 2026-06-26T13:34:08Z

+static bool is_misc_file(const char* filename)
+{
+  size_t filename_len= strlen(filename);
+  if (filename_len < ext_len)
+    return false;
+  const char *file_ext = filename + filename_len - ext_len;
+  return match_misc_ext(file_ext) || is_db_opt(filename, filename_len);
+}


In 4769a43, Aria_backup::is_db_file() is using strrchr and strcmp for these, and it is omitting a . prefix from the suffix string that are being compared to. This feels unnecessarily complicated. Can you demonstrate that this approach results in smaller or more efficient code on at least one ISA?

dr-m · 2026-06-26T13:40:03Z

+#if 1 // FIXME: invoke these only for Aria, MyISAM, CSV but not others
+  case BACKUP_PHASE_NO_DML_NON_TRANS:
+    /* FIXME: Would be better to selectively purge only the tables we need.
+    To be fixed in MDEV-39987. */
+    tc_purge();
+    tdc_purge(true);
+    break;
+#endif


This is only partly addressing my previous comment about this. MDEV-39987 does not make it clear that this performance issue mainly affects other storage engines, such as ENGINE=InnoDB or ENGINE=RocksDB, which do not need any table handles to be closed.

Move copying of common files and MyISAM files out of Aria plugim Fix incorrect handling of non-default aria_log_dir_path.

in the same phase.

mariadb-andrzejjarzabek requested a review from dr-m April 22, 2026 08:28

gkodinov added the MariaDB Corporation label Apr 23, 2026

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 6 times, most recently from 4ef94be to c143ff2 Compare April 25, 2026 19:02

dr-m reviewed Apr 28, 2026

View reviewed changes

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch from efbc62b to c77278b Compare May 4, 2026 14:00

dr-m force-pushed the MDEV-14992 branch from bcbda03 to e0d850e Compare May 5, 2026 12:06

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 2 times, most recently from e89625e to 06fd556 Compare May 17, 2026 19:19

dr-m force-pushed the MDEV-14992 branch from e0d850e to 0c52540 Compare May 18, 2026 09:40

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 2 times, most recently from 4d6a19c to d86a6a1 Compare May 20, 2026 05:50

dr-m force-pushed the MDEV-14992 branch from bdec600 to 45e7902 Compare May 20, 2026 06:52

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 2 times, most recently from c6f5119 to 9ebb23e Compare May 20, 2026 20:24

dr-m reviewed May 21, 2026

View reviewed changes

Comment thread sql/sql_backup.cc Outdated

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 2 times, most recently from 8565956 to 04e3bc2 Compare May 21, 2026 10:53

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 2 times, most recently from 14ca552 to c9429eb Compare May 29, 2026 09:47

dr-m force-pushed the MDEV-14992 branch from d595ce0 to c0e48fc Compare May 29, 2026 14:17

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 2 times, most recently from d35cd47 to 824afeb Compare June 1, 2026 15:15

mariadb-andrzejjarzabek marked this pull request as ready for review June 1, 2026 15:16

mariadb-andrzejjarzabek marked this pull request as draft June 1, 2026 15:17

dr-m force-pushed the MDEV-14992 branch from b98be03 to 10539aa Compare June 8, 2026 09:35

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch from 2dfc033 to 01a069c Compare June 9, 2026 06:47

dr-m reviewed Jun 9, 2026

View reviewed changes

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 4 times, most recently from 0de6368 to 8b1c81b Compare June 12, 2026 06:11

dr-m reviewed Jun 17, 2026

View reviewed changes

mariadb-andrzejjarzabek marked this pull request as ready for review June 17, 2026 08:44

dr-m force-pushed the MDEV-14992 branch from e81af7f to 6c8a37f Compare June 18, 2026 09:59

dr-m force-pushed the MDEV-14992 branch from 53cf648 to 87036f8 Compare June 25, 2026 13:10

dr-m force-pushed the MDEV-14992 branch from 87036f8 to 4769a43 Compare June 25, 2026 13:18

fixup! 4769a43

9ac620a

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch from 8b1c81b to d23446e Compare June 26, 2026 00:54

dr-m added 4 commits June 26, 2026 09:22

Remove an unnecessary suppression

301f196

fixup! 4769a43

fffd4b6

Eliminate the buggy Source_dir that broke error propagation. Also, do not misidentify Linux block devices or sockets as directories.

fixup! 4769a43

18490c8

InnoDB_backup::context::de_hardlink(): Avoid copying garbage at the start of the first file.

squash! 4769a43

a7570c4

fil_system.have_all_spaces: A flag that indicates whether all tablespace metadata is present in fil_system.

dr-m reviewed Jun 26, 2026

View reviewed changes

mariadb-andrzejjarzabek added 8 commits June 26, 2026 16:50

Implement table and file copy using backup_step

554a791

Fix Windows build issue.

863dede

Incorporate changes from code review.

e8b376c

Fix Windows crash. Enable backup_server_restore for Windows.

0e18555

Fix concurrent BACKUP_PHASE_FINISH issue.

2c50745

Add a test for Aria concurrent backup with many tables

b38451a

MDEV-39988 Copy non-ACID engine files in BACKUP SERVER

6a7286e

Move copying of common files and MyISAM files out of Aria plugim Fix incorrect handling of non-default aria_log_dir_path.

Refactor and simplify code given all Aria tables files are now copied

d4b2048

in the same phase.

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch from d23446e to d4b2048 Compare June 27, 2026 16:44

dr-m force-pushed the MDEV-14992 branch from a7570c4 to 81b3ae7 Compare June 29, 2026 08:23

Uh oh!

Uh oh!

Conversation

mariadb-andrzejjarzabek commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

mariadb-andrzejjarzabek commented Apr 22, 2026 •

edited

Loading

CLAassistant commented Apr 22, 2026 •

edited

Loading