Skip to content

MDEV-36886 log_t::get_lsn_approx() isn't lower bound #4074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 28, 2025
Merged

MDEV-36886 log_t::get_lsn_approx() isn't lower bound #4074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions storage/innobase/buf/buf0flu.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2294,6 +2294,12 @@ redo log capacity filled threshold.
@return true if adaptive flushing is recommended. */
static bool af_needed_for_redo(lsn_t oldest_lsn) noexcept
{
/* We may have oldest_lsn > log_sys.get_lsn_approx() if
log_t::write_buf() or log_t::persist() are executing concurrently
with this. In that case, age > af_lwm should hold, and
buf_flush_page_cleaner() would execute one more timed wait. (Not a
big problem.) */

lsn_t age= log_sys.get_lsn_approx() - oldest_lsn;
lsn_t af_lwm= static_cast<lsn_t>(srv_adaptive_flushing_lwm *
static_cast<double>(log_sys.log_capacity) / 100);
Comment on lines +2297 to 2305
Copy link
Contributor

@mariadb-DebarunBanerjee mariadb-DebarunBanerjee May 28, 2025

Choose a reason for hiding this comment

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

The explanation is correct but we don't want to subtract a bigger number and then convert the -ve number to get a huge(incorrect) age and falsely recommend adaptive flushing. Can you please add similar condition as in the other caller of get_lsn_approx (page_cleaner_flush_pages_recommendation) ?

Copy link
Contributor Author

@dr-m dr-m May 28, 2025

Choose a reason for hiding this comment

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

lsn_t is an unsigned integer (uint64_t aliased via ib_uint64_t), and any wrap-around for unsigned integers is well defined behaviour. For signed integers, wrap-around from positive to negative or vice versa is undefined behaviour. This fact allows compilers to optimize some loop conditions when the loop variable typically is a signed int (although not in InnoDB).

The comment tries to explain that the impact of the odd return true instead of a more correct return false is limited to the following in the only caller buf_flush_page_cleaner():

    possibly_unemployed:
      if (!soft_lsn_limit && !af_needed_for_redo(oldest_lsn))
        goto set_idle;

    buf_pool.page_cleaner_set_idle(false);
    buf_pool.n_flush_inc();
    mysql_mutex_unlock(&buf_pool.flush_list_mutex);

    if (UNIV_UNLIKELY(soft_lsn_limit != 0))
    {
      n= srv_max_io_capacity;
      goto background_flush;
    }

    if (!srv_adaptive_flushing)

That is, an accidental return true may lead to attempting to run an extra flushing batch instead of entering an unbounded wait for buf_pool_t::page_cleaner_wakeup(). This is similar to the glitch in page_cleaner_flush_pages_recommendation(), which could return an unnecessarily large number of recommended pages to flush.

Expand Down Expand Up @@ -2355,8 +2361,10 @@ static ulint page_cleaner_flush_pages_recommendation(ulint last_pages_in,
ulint n_pages = 0;

const lsn_t cur_lsn = log_sys.get_lsn_approx();
ut_ad(oldest_lsn <= cur_lsn);
ulint pct_for_lsn = af_get_pct_for_lsn(cur_lsn - oldest_lsn);
/* We may have cur_lsn < oldest_lsn if
log_t::write_buf() or log_t::persist() were executing concurrently. */
ulint pct_for_lsn = cur_lsn < oldest_lsn
? 0 : af_get_pct_for_lsn(cur_lsn - oldest_lsn);
time_t curr_time = time(nullptr);
const double max_pct = srv_max_buf_pool_modified_pct;

Expand Down
5 changes: 3 additions & 2 deletions storage/innobase/include/log0log.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,9 +406,10 @@ struct log_t
@param encrypted whether the log is encrypted */
static void header_write(byte *buf, lsn_t lsn, bool encrypted) noexcept;

/** @return a lower bound estimate of get_lsn(),
/** @return an estimate of get_lsn(),
using acquire-release ordering with write_buf() or persist();
this is exact unless append_prepare_wait() is pending */
an upper bound if said functions have updated only one of the fields,
a lower bound if append_prepare_wait() is pending, otherwise exact */
lsn_t get_lsn_approx() const noexcept
{
/* acquire-release ordering with write_buf() and persist() */
Expand Down