Skip to content

MDEV-39091 BACKUP SERVER of ENGINE=RocksDB to the local file system#5388

Open
Thirunarayanan wants to merge 1 commit into
MDEV-39061from
MDEV-39091
Open

MDEV-39091 BACKUP SERVER of ENGINE=RocksDB to the local file system#5388
Thirunarayanan wants to merge 1 commit into
MDEV-39061from
MDEV-39091

Conversation

@Thirunarayanan

Copy link
Copy Markdown
Member

Wire the RocksDB (MyRocks) engine into BACKUP SERVER TO and BACKUP SERVER WITH (streaming). RocksDB SST files are immutable, so a consistent point-in-time snapshot is obtained cheaply with a rocksdb::Checkpoint. The checkpoint is frozen at BACKUP_PHASE_NO_COMMIT because the checkpoint files are immutable, the actual copy is deferred to BACKUP_PHASE_FINISH, after the backup MDL has been released, and the server-side checkpoint is removed at end of FINISH. No user-level lock is needed: MDL_BACKUP_START already serializes concurrent BACKUP SERVER.

Files are copied into the target's "#rocksdb" subdirectory. The default rocksdb_datadir ("./#rocksdb") makes restore transparent: on restore the files land at /#rocksdb, so pointing a server at the extracted backup just works, with no copy-back and no RocksDB prepare step.

storage/rocksdb/rdb_backup_server.{h,cc}:
New RocksDB_backup context and the three handlerton hooks.
The checkpoint file list is drained across N CONCURRENT step threads via a lock-free atomic cursor; each file is copied with copy_entire_file() (directory target) or backup_stream_*(). Since SSTs are immutable, the sendfile(2) fast path (backup_stream_append_async) is used for the stream target.

rdb_create_checkpoint(): Factor the checkpoint creation
rdb_remove_checkpoint(): Remove the checkpoint creation logic.
rdb_get_datadir(): Get the rocksdb data directory.

Wire the RocksDB (MyRocks) engine into BACKUP SERVER TO and
BACKUP SERVER WITH (streaming). RocksDB SST files are immutable,
so a consistent point-in-time snapshot is obtained cheaply
with a rocksdb::Checkpoint. The checkpoint is frozen at
BACKUP_PHASE_NO_COMMIT because the checkpoint files are
immutable, the actual copy is deferred to BACKUP_PHASE_FINISH,
after the backup MDL has been released, and the server-side
checkpoint is removed at end of FINISH. No user-level lock
is needed: MDL_BACKUP_START already serializes concurrent BACKUP SERVER.

Files are copied into the target's "#rocksdb" subdirectory.
The default rocksdb_datadir ("./#rocksdb") makes restore
transparent: on restore the files land at <datadir>/#rocksdb, so
pointing a server at the extracted backup just works, with
no copy-back and no RocksDB prepare step.

storage/rocksdb/rdb_backup_server.{h,cc}:
 New RocksDB_backup context and the three handlerton hooks.
The checkpoint file list is drained across N CONCURRENT step threads
via a lock-free atomic cursor; each file is copied with
copy_entire_file() (directory target) or backup_stream_*().
Since SSTs are immutable, the sendfile(2) fast path
(backup_stream_append_async) is used for the stream target.

rdb_create_checkpoint(): Factor the checkpoint creation
rdb_remove_checkpoint(): Remove the checkpoint creation logic.
rdb_get_datadir(): Get the rocksdb data directory.
@Thirunarayanan Thirunarayanan requested a review from dr-m July 15, 2026 10:32
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements BACKUP SERVER support for the RocksDB (MyRocks) storage engine, introducing new backup hooks to create consistent checkpoints and copy or stream the immutable checkpoint files. The review feedback highlights several critical issues: a potential security and correctness risk where an empty datadir could write to the root directory, potential backup corruption due to an unrestored file offset after calling lseek, a resource leak on backup abort because cleanup is only executed during the finish phase, and a potential crash on Windows if target.path is null.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +47 to +48
const char *datadir= rdb_get_datadir();
std::string dir(datadir ? datadir : "");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If rdb_get_datadir() returns nullptr or an empty string, dir will resolve to / after appending the slash, causing the checkpoint directory to be created at /mariabackup-checkpoint. This is a security and correctness risk as it attempts to write to the root of the filesystem. Defaulting to the current directory . when the datadir is empty or null is much safer.

  const char *datadir= rdb_get_datadir();
  if (!datadir || !*datadir)
    datadir= ".";
  std::string dir(datadir);

Comment on lines +249 to +251
uint64_t end= uint64_t(lseek(src, 0, SEEK_END));
if (backup_stream_start(sink.stream, rel.c_str(), 0644, end, nullptr, 0) ||
backup_stream_append_async(src, sink.stream, 0, end))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using lseek(src, 0, SEEK_END) to determine the file size leaves the file offset at the end of the file. If the underlying implementation of backup_stream_append_async (or any fallback path) relies on the current file offset (e.g., using read() instead of pread()), it will read 0 bytes, leading to silent backup corruption. The file offset must be restored to the beginning of the file using lseek(src, 0, SEEK_SET) before proceeding.

    uint64_t end= uint64_t(lseek(src, 0, SEEK_END));
    if (lseek(src, 0, SEEK_SET) == (off_t)-1 ||
        backup_stream_start(sink.stream, rel.c_str(), 0644, end, nullptr, 0) ||
        backup_stream_append_async(src, sink.stream, 0, end))

Comment on lines +395 to +399
if (phase == BACKUP_PHASE_FINISH)
{
bk->remove_checkpoint();
delete bk;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the backup is aborted, rocksdb_backup_end is called with BACKUP_PHASE_ABORT. Currently, the cleanup logic (removing the checkpoint and deleting the bk context) is only executed when phase == BACKUP_PHASE_FINISH. This causes a memory leak and leaves stale checkpoint directories on disk when a backup fails or is aborted. The cleanup should be performed on both BACKUP_PHASE_FINISH and BACKUP_PHASE_ABORT.

  if (phase == BACKUP_PHASE_FINISH || phase == BACKUP_PHASE_ABORT)
  {
    bk->remove_checkpoint();
    delete bk;
  }

Comment on lines +269 to +272
if (sink.stream == sink.NO_STREAM)
{
/* Directory target */
std::string dest_path(target.path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

On Windows, if target.path is nullptr, constructing std::string dest_path(target.path) results in undefined behavior (typically a crash). Adding a null check prevents potential crashes.

  if (sink.stream == sink.NO_STREAM)
  {
    /* Directory target */
    if (!target.path)
    {
      my_error(ER_CANT_CREATE_FILE, MYF(0), rel.c_str(), EINVAL);
      return 1;
    }
    std::string dest_path(target.path);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants