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

The following SQL statements will be introduced: BACKUP SERVER TO '/path/to/directory' [ 1 CONCURRENT ]; BACKUP SERVER WITH [ 1 CONCURRENT ] 'command'; In place of the 1, any positive number of threads may be specified. For the first variant, '/path/to' must exist and '/path/to/directory' must not exist; that is where the backup will be written to. For the second variant, 'command' must be the name of a script or command that will be executed in a child process. The standard input of that command will be in a format that is compatible with GNU tar --format=oldgnu (and also BSD tar variants that are also part of Microsoft Windows and Apple macOS). The command is expected to optionally compress and encrypt the stream and redirect it to a file on a local or a remote server. The BACKUP SERVER WITH will append an additional argument, a positive base-ten number in ASCII, starting with 1, to identify the current thread. In this way, each concurrent stream can write a separate file. The backup or the first stream will contain a file backup.cnf, which includes parameters needed for restoring the backup. Currently, these are innodb_log_recovery_start and innodb_log_recovery_target. If innodb_log_recovery_target>0, InnoDB will be in read-only mode, not allowing any writes to persistent files other than via the log application. To restore a streaming backup made with BACKUP SERVER WITH, an empty directory needs to be created and all streams be extracted there using the standard tar utility of the operating system, optionally after undoing any encryption or compression that had been added by the backup command. Then, the backup is prepared or MariaDB server started up on the extracted directory, similar to as if the BACKUP SERVER TO statement had been used. Note: The parameter innodb_log_recovery_start in backup.cnf is STRICTLY NECESSARY TO AVOID CORRUPTION! By default, InnoDB crash recovery starts from the latest available log checkpoint. However, for restoring a backup, recovery must start from the checkpoint that was the latest when the backup was started. Starting recovery from a possible later checkpoint will result in a corrupted database! The following will be implemented separately: MDEV-39061 mariadb-backup compatible wrapper script for BACKUP SERVER MDEV-40163 Partial backup and restore MDEV-39091 Back up ENGINE=RocksDB MDEV-39092 Less blocking backup of ENGINE=Aria The implementation introduces a basic driver Sql_cmd_backup, storage engine interfaces, and basic copying of the storage engines InnoDB, Aria, MyISAM, MERGE (MyISAM), Archive, CSV. 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 partial or sparse file-copying service for all systems. backup_stream_append(): Equivalent to copy_file(), but appending to a stream. On Linux, this uses sendfile(2), which assumes that the source data will not be changed before the data has been consumed from the pipe. 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. InnoDB_backup::queue: Collection of tablespace IDs and payload sizes at the start of the backup. If any file is created or extended while the backup is executing, we must have the corresponding write-ahead-log entries that we are copying since the latest checkpoint that was completed when the backup started. If any tablespaces are deleted during the backup, we may or may not copy them, and the application of a FILE_DELETE record will remove them. Similarly, FILE_RENAME or FILE_CREATE records will take care of renaming or creating files during recovery (applying the backed-up log). 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(). fil_system.have_all_spaces: Whether all tablespace metadata is guaranteed to be known. To speed up startup, InnoDB does not normally open all tablespace files.

sanja-byelkin

Please answer and/or fix comments

sanja-byelkin · 2026-07-29T12:46:46Z

-    {
-      return scan_datadir() || copy_databases(target, sink) ||
-        copy_control_file(target, sink) ||
-        translog_flush(translog_get_horizon()) ||


The flush is removed is it really OK?

It's ok, we only want to see committed transactions in the backup, and committing ensures the transaction is present in the log file. The translog_flush call was only added as an experiment and mistakenly left in.

sanja-byelkin · 2026-07-29T12:53:01Z

+  for (uint32 i= 0; i < tc_instances; i++)
+  {
+    mysql_mutex_lock(&tc[i].LOCK_table_cache);
+    auto free_list= element->free_tables[i].list;


how I hate that auto, never known what type is behind (in this case appeared to be ( I_P_List <TABLE, TABLE_share>)

about the problem: we copy head the the list, change it and do not assign the copy back or empty the original list which is unsafe from maintaince point of view

sanja-byelkin · 2026-07-29T13:13:23Z

+
+    /* Copy an entity that is not safe to copy if there are concurrent
+    writes to it. One entity is copied, of the first category that has
+    any remaning entities to be copied. Returns the total number of


Is the claim about returning total number really true?

sanja-byelkin · 2026-07-29T13:16:03Z

+  }
+  else
+  {
+    uint64_t end= uint64_t(lseek(src_fd, 0, SEEK_END));


lseek error is not checked

sanja-byelkin · 2026-07-29T13:18:32Z

+
+/* Files not copied by plugin backup implementations: files managed by
+SQL layer and miscellaneous engine files to be copied bunde DDL lock */
+static constexpr const char* misc_exts[] {".frm", ".par", ".MYD", ".MYI", ".MRG",


Are triggers (.trg/trn) works correctly?

sanja-byelkin · 2026-07-29T13:24:01Z

+DROP TABLE t_archive;
+DROP TABLE t_archive_part;
+
+--rmdir $target_directory


version marker should be at the end of the test (Having N/A instead version in the mdev I can not say which but both have to have target version)

--echo End of XX.Y tests

sanja-byelkin · 2026-07-29T13:24:25Z

+--source include/restart_mysqld.inc
+
+--rmdir $target_directory
+--rmdir $log_directory


version marker should be at the end of the test (Having N/A instead version in the mdev I can not say which but both have to have target version)

--echo End of XX.Y tests

sanja-byelkin · 2026-07-29T13:24:37Z

+
+--enable_query_log
+
+--rmdir $target_directory


version marker should be at the end of the test (Having N/A instead version in the mdev I can not say which but both have to have target version)

--echo End of XX.Y tests

sanja-byelkin · 2026-07-29T13:25:12Z

+DROP TABLE t_mrg;
+DROP DATABASE d;
+
+--rmdir $target_directory


version marker should be at the end of the test (Having N/A instead version in the mdev I can not say which but both have to have target version)

--echo End of XX.Y tests

Oops I missed #, it should be

--echo # End of XX.Y tests

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

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

Comment thread storage/maria/ma_backup.cc Outdated

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 2 times, most recently from fe1f0a2 to 9e27d73 Compare June 30, 2026 12:13

dr-m force-pushed the MDEV-14992 branch 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

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch 2 times, most recently from 807e8de to bb6359c Compare July 28, 2026 17:57

sanja-byelkin requested changes Jul 29, 2026

View reviewed changes

mariadb-andrzejjarzabek added 2 commits July 30, 2026 19:15

mariadb-andrzejjarzabek force-pushed the MDEV-39092 branch from bb6359c to 1146eea Compare July 30, 2026 18:27

dr-m force-pushed the MDEV-14992 branch from 5235e7a to bfed32b Compare August 4, 2026 13:26


		--enable_query_log

		--rmdir $target_directory

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!

Choose a reason for hiding this comment

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!

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!

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!

sanja-byelkin left a comment

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!

Choose a reason for hiding this comment

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