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.

In the latest commit there are 2 directory descriptors involved, one for the data directory (which is shared by SQL layer and plugins) and one for Aria log directory. The data directory descriptor is opened once and never closed, and the Aria log directory descriptor is opened and closed once per backup. By default they point to the same directory, so for the default case the second descriptor could be optimized away altogether. This would be a small increase in code complexity, but the real cost is that we would have an additional path through code that would only be exercised by the non-default case. I have opted not to do this optimization and keep the code simple on the assumption that the cost of opening and closing one descriptor per backup is negligible.

If we decide otherwise, I can include this optimization and then perhaps also keep the log directory descriptor opened in the non-default case.

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.

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?

backup_sink is not defined in this header (the thing defined is backup_chunk).

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?

nullptr seems to be valid input to my_dirend: there's no documentation for that function, but the definition checks for null and does nothing if it's passed. It's also a common convention: both the standard library free and the built-in delete operator are defined as no-op for nullptr.

Explicit constructors prevent the object from being implicitly constructed from a braced init list. In this case either case could be argued, I prefer to use explicit constructors everywhere unless conversion semantics are specifically wanted.

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.

Given that this path needs to be constructed anyway for the target, and it would only be avoidable for non-streaming backup, I'm not sure how to write this code "well" to cover all cases (Windows, non-Windows streaming, non-Windows to directory) so as to strike the right between duplication and complexity. I propose that this PR be merged into MDEV-14992 without this particular optimization and then to figure out how to best optimize away string allocation.

To my previous comment: because I fixed the generally incorrect assumption that the data directory is the current directory, there do need to be additional paths constructed to list subdirectories of the data directory. There is room for optimization to reduce the number of allocations, but since there are a number of ways to do this with trade-offs that are not always obvious, I propose to consider doing it as part of in MDEV-39987.

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.

Changed this to use mysql_real_data_home everywhere. Leaving open the questions for future improvements:

Is it better to use mysql_data_home instead? This is what's used in the logic to locate table files,

Is it worth to optimize for the special case where data directory is current directory, if we could e.g. reduce the number of heap allocations? (because we have fewer paths to format)

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?

Locally benchmarking
a. the strrchr/strcmp method in 4769a43,
b. the method in 5235e7a Aria_backup::is_db_file() of taking the last 4 bytes as an int and checking using switch, and
c. the method committed here of checking the last 4 bytes by doing a linear scan against known extensions using memcmp,
method c. generates the smallest object code (only slightly smaller than a, but significantly smaller than b) and runs the fastest (only slightly faster than b, but significantly faster than a). I also feel the gains are not significant enough (in my experiment, less than 100 bytes difference in object code size, duration difference of about 1ms per backup on a data directory with 200 million files, which is in line with what I would expect it to be) to warrant an extended investigation with multiple toolchains and architectures. I don't feel that comparing the last 4 bytes is any more complicated than the other 2 methods.

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.

On top of the provisional Aria backup solution provisionally incorporated into MDEV-14992, the following improvements have been made: Aria data and index files are copied under DDL-locked lock level instead of commit-locked, making the backup operation less disruptive. Only log files are copied in the commit-locked phase. Writes to non-transactional Aria tables are blocked in the DDL-locked phase, while writes to transactional tables are written to the log file, allowing consistent point-in-time backup at the time of acquiring the commit lock. Data, index and log files are now copied as a "step" action rather than "end phase" action, allowing them to be copied in parallel using the CONCURRENT option. Non-Aria files, including common SQL-layer metadata and files from other storage engines are copied by the SQL layer rather than the Aria plugin. Note these files are at this time not copied concurrently when the concurrent option is used.

The BACKUP SERVER functionality supports a number of non-ACID engines by simply copying their datafiles under a lock level which blocks other threads from accessing these tables. For this to work correctly the files must be consistent and up-to-date when copied. Thic is achieved by purging their table caches. This is the change to selectively purge caches only for tables, for which this is necessary. This covers the set of storage engines supported directly from SQL layer (MyISAM, CSV, ARCHIVE, Merge), and additionally the Aria engine, for which only non-transactional tables are purged. Purging of Table Definition Cache is only done for ARCHIVE angine tables, as it is needed to close the archive. Non-standard engine plugins will not have their table caches purged. If necessary, they need to implement the flushing/closing/purging logic in the plugin itself.

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 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

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 3 times, most recently from 9e27d73 to 5235e7a Compare June 30, 2026 12:43

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 4 times, most recently from d28041f to 8f37343 Compare July 7, 2026 09:58

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch from 8f37343 to b3645b2 Compare July 10, 2026 06:07

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!

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!

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!

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

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!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

mariadb-andrzejjarzabek commented Apr 22, 2026 •

edited

Loading

CLAassistant commented Apr 22, 2026 •

edited

Loading

mariadb-andrzejjarzabek Jul 8, 2026 •

edited

Loading

mariadb-andrzejjarzabek Jul 13, 2026 •

edited

Loading

mariadb-andrzejjarzabek Jul 13, 2026 •

edited

Loading