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.

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.

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.

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 reviewed Jun 4, 2026

View reviewed changes

Comment thread mysql-test/main/backup_server_restore.test Outdated

Comment thread mysql-test/main/backup_server_restore.result Outdated

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 2 times, most recently from 23ac784 to 2dfc033 Compare June 5, 2026 20:07

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 2 times, most recently from 87036f8 to 4769a43 Compare June 25, 2026 13:18

mariadb-andrzejjarzabek added 8 commits June 25, 2026 20:44

Implement table and file copy using backup_step

0c0aca4

Fix Windows build issue.

769c57a

Incorporate changes from code review.

7cd42f4

Fix Windows crash. Enable backup_server_restore for Windows.

9fdb672

Fix concurrent BACKUP_PHASE_FINISH issue.

75f3f49

Add a test for Aria concurrent backup with many tables

2d5eb1f

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

4b46a15

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

d23446e

in the same phase.

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

dr-m reviewed Jun 26, 2026

View reviewed changes

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!

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