MDEV-14992 BACKUP SERVER

[dr-m]

The following SQL statements will be introduced:

BACKUP SERVER TO '/path/to/directory';
BACKUP SERVER TO '/path/to/directory' 1 CONCURRENT;
BACKUP SERVER WITH 'command';
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.

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.

[CLAassistant]

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

[Thirunarayanan]

First round of review.

[Thirunarayanan]

Something like this:

1. page cleaner acquires space S and caches backup_page_end = 0 (no
   backup active yet)
2. backup: backup_start(1064) → window [1000,1064) published
3. backup: batch_wait scans 1063..1001; page P=1020 is CLEAN and skipped
4. backup: backup::copy(node->handle, dst, 100016k, 106416k) : 1 MB read in flight
5. user thread: mtr_t::commit() modifies P, inserted into
   flush_list, oldest_modification = LSN
6. page cleaner reaches P: cached backup_page_end is 0
   - page < backup_page_end is 1020 < 0 = false
   • bpage->flush(space), real write fix, 16 KB pwrite at offset 1020*16k.
     Like buf_flush_list_space(), we shouldn't rely on cached backup_page_end.

we should change in buf_do_flush_list_batch() and same for the callers of buf_flush_try_neighbors()

[dr-m]

First, let me correct the page number range in your scenario to 960‥1023, because we are dealing with integer multiples of fil_space_t::BACKUP_BATCH_SIZE (64 pages).

I’d like to observe that mtr_t::commit() must hold buf_pool.flush_list_mutex in order to be able to add a first-time-modified page to buf_pool.flush_list.

This race condition scenario will be avoided by reading the fil_space_t::backup_end after acquiring buf_pool.flush_list_mutex. I think that this is all that is needed. The store-release fil_space_t::backup_start() and the load-acquire of backup_page_end (moved to be within the critical section of buf_pool.flush_list_mutex) should guarantee the correct operation. The store should be sufficient outside any mutex.

The logic is complicated, because innodb_backup_batch_wait() must avoid dirtying a large number of cache lines in order to avoid a significant performance impact when most of the buffer pool is clean.

[dr-m]

As far as I can tell, the existing logic in buf_flush_list_space() and buf_do_flush_list_batch() should be safe with respect to the presented scenario: We acquired a U-latch on a dirty block while holding buf_pool.flush_list_mutex. The page is already dirty, and we only consume blocks via the buf_pool.flush_list. It should not matter at which exact point we read the backup_page_end; the main thing is that we were holding buf_pool.flush_list_mutex when acquiring the U-latch on the dirty page and before reading the backup_page_end.

It turns out that in buf_flush_LRU_list_batch() we are not holding buf_pool.flush_list_mutex at all. We are traversing buf_pool.LRU under the protection of buf_pool.mutex instead.

At some point I had a relaxed store of fil_space_t::backup_end inside a critical section of buf_pool.mutex. That (as well as a relaxed load of that field within buf_pool.mutex) should prevent the problem. But, it would also introduce significant additional contention on an already busy mutex.

The write/backup race scenario with buf_flush_LRU_list_batch() should be rare, because typically any pages that are about to be dirtied would first be moved to the most recently used end of buf_pool.LRU, which the scan is not expected to access.

[dr-m]

I reviewed the case of buf_flush_LRU_list_batch() once more. If the mtr_t::commit() thread had not yet released the page latch, the bpage->lock.u_lock_try(true) in buf_flush_LRU_list_batch() would have failed and we would have moved on to the next page, without initiating any write.

Also here, we load-acquire fil_space_t::backup_end while holding a page U-latch that blocks any writes to the page frame or the file system. In backup_batch_start() we first store-release that field before checking if any blocks are dirty and have to be fake-write-fixed.

Let me rephrase your scenario in more exact terms. It applies to each of the functions in buf0flu.cc:

1. buf0flu.cc holds bpage->lock.u_lock(), does load-acquire of fil_space_t::backup_end (as 0 or as a smaller value that belonged to an earlier backup batch, not matching bpage->id())
2. backup_batch_start() does a store-release of fil_space_t::backup_end that covers the bpage->id()
3. innodb_backup_batch_wait() notices bpage->oldest_modification() and does bpage->lock.u_lock() to wait for the buf_page_t::write_complete()
4. buf0flu.cc invokes buf_page_t::flush() to submit the write
5. buf_page_t::write_complete() in another thread will release the bpage->lock.u_unlock() upon write completion
6. innodb_backup_batch_wait() acquires the page latch and immediately releases it with bpage->lock.u_unlock() without any further action

I do not see a problem here. The page latch wait is doing the right thing. There is no need to move the load-acquire to another critical section. It already is protected by bpage->lock.u_lock(), which matters.

The fake write-fix takes care of a slightly different scenario where buf_page_t::flush() will be invoked in case the load of fil_space_t::backup_end occurred shortly before backup_batch_start() was invoked. In this case, buf_page_t::flush() will yield to the fake write-fix that would be set before backup_batch_start() completes.

Can you please double-check your claim and my reasoning?

[Thirunarayanan]

I do have only these comments as of now

[Thirunarayanan]

what if the imported tablespace started before init() and ends up before NO_DDL? will it be copied or reconstructable ?

[dr-m]

Because ALTER TABLE…IMPORT TABLESPACE is currently bypassing the write-ahead-logging protocol that both BACKUP SERVER and mariadb-backup will depend on, the effects of any such statements executed during the backup could be lost in the backup. For mariadb-backup --backup there already is an existing report MDEV-29208 about this. A possible fix might be to make ALTER TABLE…IMPORT TABLESPACE mutually exclusive with backup locks.

[Thirunarayanan]

1. FLUSH TABLES FOR EXPORT and encryption_rotate_key_age key rotation concurrent with backup server
2. Run 2 concurrent backup server statement
3. log rotation during backup
4. system tablespace larger than dblwr
   These test case exist already?

[dr-m]

@Thirunarayanan, thank you for your review comments. I am linking to additional comments to individual source files so that each open topic can be followed up in separate threads and marked as resolved individually.

1. FLUSH TABLES FOR EXPORT and encryption_rotate_key_age key rotation concurrent with backup server

This will be followed up here and here.

2. Run 2 concurrent backup server statement

Please refer to mysql-test/main/backup_server_locking.result:

BACKUP SERVER TO '$MYSQLTEST_VARDIR/some_directory';
ERROR HY000: Can't execute the command as you have a BACKUP STAGE active

That scenario confirms the mutual exclusion of BACKUP STAGE and BACKUP SERVER. Writing a similar test between two BACKUP SERVER would require some debug injection to block the first BACKUP SERVER execution until an attempt to execute the second one is made.

3. log rotation during backup

"Log rotation" is a term that I associate with ENGINE=Aria and the binlog. The former is prevented by translog_disable_purge() during aria_backup_logs(). As noted, we are currently missing an option that allows binlog_storage_engine=innodb to be copied.

If you are referring to innodb_log_archive=ON file wrap-around, it is exercised by backup.backup_innodb,debug since the very beginning of that test. If you run a test that is patched to fail right before the backup is restored:

diff --git a/mysql-test/suite/backup/backup_innodb.test b/mysql-test/suite/backup/backup_innodb.test
index 87b56fb7614..bdc75c8bacd 100644
--- a/mysql-test/suite/backup/backup_innodb.test
+++ b/mysql-test/suite/backup/backup_innodb.test
@@ -84,7 +84,7 @@ if (!$MARIADB_UPGRADE_EXE) {
   --exec cat $MYSQLTEST_VARDIR/my.cnf >> $target_directory/backup.cnf
 }
 --let $restart_parameters=--defaults-file=$target_directory/backup.cnf --datadir=$target_directory
---source include/restart_mysqld.inc
+--source include/shutdown_mysqld.inc
 
 SELECT * FROM t;
 # A nonzero innodb_log_recovery_target makes InnoDB read-only.

you will find two log files ib_0000000000003000.log and ib_0000000000a00000.log in each $target_directory.

4. system tablespace larger than dblwr

buf_dblwr_t::create() guarantees the assumption of InnoDB_backup::backup() and InnoDB_backup::stream() that the first file of the system tablespace always comprises the entire doublewrite buffer. This constraint is tested by mysql-test/suite/innodb/t/doublewrite_debug.test.

[dr-m]

@vuvova suggested that the BACKUP SERVER WITH command be prefixed with a directory path that is defaults to opt_basedir. When I asked for a clarification how to handle environments where any customization is kept separately (for example, /usr in Debian is supposed to be read-only), he suggested that there be a parameter that specifies a number of alternatives. That would allow DBAs to place their custom backup scripts anywhere.

[dr-m]

215c4e7 simply prepends the user-specified command with mariabd-backup- and quotes it. Path separators and quotes ('/ or "\/ depending on platform) are disallowed. It is up to the user to ensure that such a script exists in the $PATH of the mariadbd process (or in the case of Microsoft Windows, in datadir).

For some reason, invoking mysql-test/mtr --suite=backup will not add client to the $PATH. This will cause the test backup_stream to fail when invoked in such a way. If no --suite parameter is passed, the mariadb-backup-cat will be found.

For reference, I checked what the wsp::process proc in sst_donor_thread() does. The constructor initializes a popen(3) like argument list for posix_spawnp(3) that starts with sh -c. The safety of this appears to be guaranteed by sst_donate_other() in sql/wsrep_sst.cc, which guarantees that the name of the script starts with wsrep_sst_. The name is passed in via wsrep_sst_donate(), which validates it via wsrep_check_request_str() and wsrep_filename_char(). That is, each character of the name of the script must be one of the characters -, _, or ., or satisfy isalnum(3) in the current locale. There is no check for IFS or PATH and no quoting around the name of the script.

In /bin/sh, commands must be either built-in commands or searched via PATH. So, a script like wsrep_sst_mariabackup can be trivially overloaded by writing a modified copy of the script somewhere and specifying the directory at the start of the PATH in the environment of the mariadbd process.

The macro my_popen expands to either the standard popen or to my_win_popen(). These should prepend the specified command line with /bin/sh -c and cmd.exe /c, respectively. So far, I only ran some interactive tests from the shell on Microsoft Windows [Version 10.0.26100.33296]; we’ll see soon if my interpretation of https://learn.microsoft.com/en-us/cpp/c-language/parsing-c-command-line-arguments and my invocation of my_popen() works as desired.

[dr-m]

@vuvova, this change is making the test backup.backup_stream fail on all autobake targets, because an executable mariadb-backup-cat is missing. How can we install a script in the $PATH of the mariadbd process
for test purposes, without distributing it? To add some extra challenge, mysql-test/mtr --mem may be using a file system mounted with noexec. That is why the test used to invoke the script as follows:

--remove_files_wildcard $MYSQL_TMP_DIR/. stream.bat
--write_file $MYSQL_TMP_DIR/stream.bat
#!/bin/sh
exec cat > $MYSQL_TMP_DIR/$1.tar
EOF

--let $script=$MYSQL_TMP_DIR/stream.bat
if (!$MARIADB_UPGRADE_EXE) {
# Because we may run on Linux /dev/shm which may be mounted as noexec,
# we cannot rely on chmod +x, but must explicitly invoke a shell on the script.
--let $script=/bin/sh $script
}

[vuvova]

if you're taking the approach from wsrep scripts, it must be all solved there, I suppose. Let's just do the same, it's a proven working solution

[dr-m]

In the rebase b387a4a I made mysql-test/suite/backup/suite.pm adjust the $PATH and check for the availability of mariadb-backup-cat. So far, it has gotten past one builder where the script is missing: https://buildbot.mariadb.org/#/builders/889/builds/12954/steps/14/logs/stdio

backup.backup_stream                     [ skipped ]  needs cat,tar,mariadb-backup-cat

The test is running (and passing) on both CMAKE_BUILD_TYPE=Debug and CMAKE_BUILD_TYPE=RelWithDebInfo targets, where the script client/mariadb-backup-cat will be available. On Microsoft Windows, a mariadb-backup-cat.bat will be created in the server @@datadir by the test backup.backup_stream.

[dr-m]

Currently, for the streaming BACKUP SERVER WITH deferring the copying to this point seems to be an acceptable and competitive choice. (It would also be hard to change that, because our stream format of choice (GNU tar --format=oldgnu) requires the size to be declared upfront.)

However, for the BACKUP SERVER TO … CONCURRENT variant we can and should start copying the log right from the beginning. Otherwise mariadb-backup --backup may finish faster and produce less log, thanks to its dedicated log_copying_thread() that starts eagerly.

[dr-m]

a1c90c4 implements more eager log copying in the HAVE_INNODB_PMEM code path. I must extend it for the pwrite based log writing path before marking this as resolved.

[dr-m]

I filed MDEV-41040 for implementing the BACKUP SERVER TO … CONCURRENT performance improvement separately and reverted the current prototype (a1c90c4 and df3d431).

[dr-m]

We actually must remember the file name as it was at this point of time. @mleich1 shared an rr replay trace where InnoDB_backup::backup() invokes the following:

  fail:
    my_error(ER_CANT_CREATE_FILE, MYF(0), node->name, errno);
    return -1;

The reason is that the backup included two files, space ID 28 and 52, the latter one with a space.create_lsn that is slightly before start, by ALTER TABLE t8 FORCE.

The original space ID 28 was copied as test/t8.ibd. Subsequently, fil_space_t::drop() was invoked on it. After that, tablespace ID 52 was copied with the its current name test/t8.ibd.

I believe that the easiest way to fix this is to add a field fil_node_t::backup_name that will store the current fil_node_t::name at the start of backup. fil_space_t::rename() must preserve the old name (so that backup_name will remain intact) and InnoDB_backup::init() as well as InnoDB_backup::step() must collect the garbage.

[dr-m]

@montywi does not want to see any _WIN32 in storage/maria (other than the 38 that exist there before these changes).

I think that his request can be accommodated by moving some definitions and code to sql/sql_backup_interface.h and sql/sql_backup.cc. What can’t be easily accommodated is to avoid any new _WIN32 in the sql subdirectory. There already are well over a hundred references to _WIN32 in sql without any of these changes.

[dr-m]

69dad73 refactors the way how non-ACID files are backed up. The directory scan is now being driven by backup_context in sql/sql_backup.cc. maria_backup_file() gets to decide which files should be backed up. Aria log files are being copied sequentially in aria_backup_step(). This is work in progress.

[dr-m]

@vuvova wrote the following in a private email:

Secure_file_priv is definitely not appropriate. You don't want any user with FILE privilege to be able to read your whole backup.

Therefore, I will remove the is_secure_file_path check and assume that the file system permissions for the service user that runs the mariadbd server process have been configured appropriately. We will only check that the requested BACKUP SERVER TO target is outside the @@datadir, by invoking error_if_data_home_dir().

[dr-m]

b387a4a removed the secure_file_priv related changes.