MDEV-26138: Fix switch case statement in trx_flush_log_if_needed_low()

[sidhujagdeep]

Description

In commit 2e814d4 on MariaDB 10.2
the switch case statement in trx_flush_log_if_needed_low() regressed.

Since 10.2 this code was refactored to have switches in descending
order, so value of 3 for innodb_flush_log_at_trx_commit is behaving
the same as value of 2, that is no FSYNC is being enforced during
COMMIT phase. The switch should however not be empty and cases 2 and 3
should not have the identical contents.

As per documentation, setting innodb_flush_log_at_trx_commit to 3
should do FSYNC to disk if innodb_flush_log_at_trx_commit is set to 3.
This fixes the regression so that the switch statement again does
what users expect the setting should do.

How can this PR be tested?

Start the server with values 2 and 3 for innodb_flush_log_at_trx_commit and expect different behaviour.

Basing the PR against the correct MariaDB version

• This is a new feature and the PR is based against the latest MariaDB development branch
• This is a bug fix and the PR is based against the earliest branch in which the bug can be reproduced
  Regression was introduced in MariaDB 10.2.2. This PR targets the 10.2 branch.

Backward compatibility

This bugfix restores previous behaviour.

Copyright

All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ dr-m
❌ sidhujagdeep
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[grooverdan]

@sidhujagdeep thank you for your contribution. BSD-new license is great so please just ignore the CLA-bot. It looks good to me but I'll let our innodb experts be the final approval.

[Thirunarayanan]

Please do the suggested changes

[Thirunarayanan]

Can you change like below:

-       case 3:
        case 2:
                /* Write the log but do not flush it to disk */
                flush = false;
                /* fall through */
        case 1:
+       case 3:
                /* Write the log and optionally flush it to disk */
                log_write_up_to(lsn, flush);

[sidhujagdeep]

Thanks for the feedback. I have update code as suggested.

[dr-m]

@sidhujagdeep Thank you for noticing this!

I see that @janlindstrom in 2e814d4 overlooked the change that had been made by @knielsen in 288eeb3 (MDEV-232).

I think that @andrelkin and @sujatha-s need to participate in deciding what should be done and in which major versions of MariaDB.

Given that innodb_flush_log_at_trx_commit=3 has been broken (an alias of innodb_flush_log_at_trx_commit=2) since the MariaDB 10.2.2 alpha release, maybe it would be acceptable to just remove the setting 3 and let a warning be issued that the value will be clamped to 2.

What is the actual benefit of supporting the setting 3, or the harm of not supporting it?

[dr-m]

After some internal discussion, it does look like that it is easiest to apply this fix. In the 10.6 branch, I tested the following:

diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc
index 66ca04bbf04..74e5be5a231 100644
--- a/storage/innobase/trx/trx0trx.cc
+++ b/storage/innobase/trx/trx0trx.cc
@@ -1135,8 +1135,8 @@ static void trx_flush_log_if_needed_low(lsn_t lsn, trx_state_t trx_state)
   if (log_sys.get_flushed_lsn() > lsn)
     return;
 
-  bool flush= srv_file_flush_method != SRV_NOSYNC &&
-              srv_flush_log_at_trx_commit == 1;
+  const bool flush= srv_file_flush_method != SRV_NOSYNC &&
+    (srv_flush_log_at_trx_commit & 1);
 
   if (trx_state == TRX_STATE_PREPARED)
   {

I hope that we will be able to streamline this logic in a separate fix. I am not convinced that MDEV-232 and MDEV-532 are working correctly; the RESET MASTER statement is hanging with some probability, even after some fixes that were applied in 10.5 (see MDEV-25611).

[sidhujagdeep]

Thank you @dr-m for looking through the change and history of this bug. I have updated the code as per feedback provided by @Thirunarayanan

[dr-m]

42b9daa is the 10.6 version of this.