Skip to content

MDEV-39770 log_t::persist(): Assertion is_opened() == archive failed#5131

Merged
dr-m merged 1 commit into
13.0from
MDEV-39770
May 28, 2026
Merged

MDEV-39770 log_t::persist(): Assertion is_opened() == archive failed#5131
dr-m merged 1 commit into
13.0from
MDEV-39770

Conversation

@dr-m
Copy link
Copy Markdown
Contributor

@dr-m dr-m commented May 27, 2026

log_t::persist(): Move a debug assertion after the point that we are actually about to write to the log. For crash recovery, this function may be invoked in such a way that log_sys.archive disagrees with the format of the log file.

This fixes a regression that was introduced in #4405.

@dr-m dr-m requested a review from Thirunarayanan May 27, 2026 10:22
@dr-m dr-m self-assigned this May 27, 2026
@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.

Copy link
Copy Markdown

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

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 updates the InnoDB log persistence logic to delay certain assertions until after checking if the log has already been flushed, and adds test coverage for innodb_log_archive. The reviewer suggested optimizing the early return condition in log_t::persist from old > lsn to old >= lsn to avoid redundant 0-byte write operations when the log is already flushed up to the requested LSN.

Comment on lines 1550 to 1551
if (old > lsn)
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If old == lsn, the log has already been successfully flushed to disk up to lsn. Proceeding further with start == end (a 0-byte write) is redundant and incurs unnecessary overhead. Changing the condition to old >= lsn allows an early return in this case, improving efficiency.

  if (old >= lsn)
    return;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a valid comment. However, it is a separate issue and should probably be fixed in the earliest applicable branch (10.11).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It turns out that this change was intentionally made in #4405. If I reverted that change as suggested, we would get assertion failures in innodb.log_archive,mmap shutdown like this:

mariadbd: /mariadb/main/storage/innobase/mtr/mtr0mtr.cc:1317: static void log_t::append(byte*&, const void*, size_t): Assertion `log_sys.is_mmap() ? ((d >= log_sys.buf && d + size <= log_sys.buf + log_sys.file_size) || (log_sys.archive && d >= log_sys.resize_buf && d + size <= log_sys.resize_buf + log_sys.resize_target)) : (d >= log_sys.buf && d + size <= log_sys.buf + log_sys.buf_size)' failed.

I filed MDEV-39781 to capture this.

@dr-m dr-m changed the base branch from main to 13.0 May 28, 2026 05:36
log_t::persist(): Move a debug assertion after the point that we are
actually about to write to the log. For crash recovery, this
function may be invoked in such a way that log_sys.archive disagrees
with the format of the log file.

Reviewed by: Thirunarayanan Balathandayuthapani
@dr-m dr-m merged commit 9199727 into 13.0 May 28, 2026
15 of 19 checks passed
@dr-m dr-m deleted the MDEV-39770 branch May 28, 2026 06:55
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