> From: Libing Song <anders.slb(a)alibaba-inc.com>
>
> Description
> ===========
> When a transaction commits, it copies the binlog events from
> binlog cache to binlog file. Very large transactions
> (eg. gigabytes) can stall other transactions for a long time
> because the data is copied while holding LOCK_log, which blocks
> other commits from binlogging.
Hi Libing Song,
Here is my full review of the patch for MDEV-32014 Rename binlog cache
temporary file to binlog file for large transaction.
First let me say that thanks, reviewing this patch was a pleasant surprise.
The binlog code that you are integrating this new feature in is complex to
work with, but overall I think you have managed to do it a pretty clean way.
That is great work, and I think a lot of effort must have gone into getting
it this way. I have a number of comments and requests for changes below, so
I wanted to give this overall good impression up-front.
Also thanks to Brandon for doing first review. My apologies if I am
repeating something already discussed, but Github pull request reviews of
non-trivial patches are almost impossible to read, with reviews being split
in fragments, patches disappearing after force-pushes, etc.
The root cause for the problem being addressed by the patch is a fundamental
limitation in the current binlog format. It requires all transactions to be
binlogged as a single event group, using consecutive bytes in a single
binlog file. Thus, while writing the large transaction to the binlog during
commit, all other transaction commits will have to wait before they can
commit.
Eventually, we should move towards an enhanced binlog format that doesn't
have this limitation, so that transactions can be partially binlogged during
execution, avoiding this bottlenect. There are a number of other problems
caused by this limitation that are not addressed by this patch. Changing the
binlog format is a complex task not easily done, so this patch should be
seen as a temporary solution to one specific bottleneck, until if/when this
underlying limitation can hopefully be removed.
First some overall comments:
As Brandon also remarked, the name "binlog_free_flush" does not describe the
feature very clearly. I would suggest to use the term "rotate" in the name,
as the concept of binlog rotation is well known in MariaDB replication, and
the patch effectively performs large commits by doing it as a binlog
rotation instead of a simple write to the existing binlog file.
So I suggest to name the option --binlog-commit-by-rotate-threshold. And I
suggest in the code (names and comments) to use the phrase
"rotate_by_commit" instead of "free_flush".
The patch enables the feature by default, with a threshold of 128MB.
We need to consider if that is appropriate, or if the feature should be
disabled by default.
Reasons for disabling by default:
- The feature causes some binlog files to be cut short of the configured
--max-binlog-size, which will be a surprise to users.
- The feature adds an overhead of 4096 pre-allocated bytes at the start of
every trx cache (though the overhead is minimal).
- A default of 128MB is somewhat arbitrary.
- Releasing this feature disabled by default is somewhat less risky wrt.
regressions for users that upgrade.
Reasons to enable by default:
- Most users will otherwise not know the feature exists, and will thus not
benefit from this work.
- It will get much less testing "in the wild".
So the decision will be a compromise between these two. If I had to make the
choice, I would default to this feature being disabled. But I'm open to hear
opinions the other way.
The patch calls the trx_group_commit_leader() function. That is ackward, as
commit-by-rotate does not actually participate in group commit. But the
problem is with the existing function trx_group_commit_leader(), which does
too much in a single function.
So please do a small refactoring of trx_group_commit_leader() and keep only
the first part of that function that fetches the group commit queue. Move
the second part to a new function trx_group_commit_with_engines() that is
called by trx_group_commit_leader() passing the "queue" and "last_in_queue"
values. And assert in trx_group_commit_with_engines() that LOCK_log is held
on entry and comment that this function will release the lock.
Then you can from your Binlog_free_flush::commit() function call into
trx_group_commit_with_engines() directly, avoiding the need for adding a
true/false flag, and also avoiding that the trx_group_commit_leader()
function is sometimes called with LOCK_log held and sometimes not.
Same situation with MYSQL_BIN_LOG::write_transaction_to_binlog_events(),
this function is also doing too much and makes integrating your code
ackward.
Please refactor the MYSQL_BIN_LOG::write_transaction_to_binlog_events()
function something like this:
bool
MYSQL_BIN_LOG::write_transaction_to_binlog_events(group_commit_entry *entry)
{
if (binlog_commit_by_rotate.should_commit_by_rotate(entry) &&
binlog_commit_by_rotate.commit(entry))
{
/* Commit done by renaming the trx/stmt cache. */
}
else
{
if (write_transaction_with_group_commit(entry))
return true; /* Error */
}
if (unlikely(entry->error))
{
write_transaction_handle_error(entry);
return 1;
}
return 0;
}
Introducing two new functions:
bool write_transaction_with_group_commit(group_commit_entry *entry)
This function contains the code from write_transaction_to_binlog_events() up
to and including this:
if (likely(!entry->error))
return entry->thd->wait_for_prior_commit();
You don't need the wait_for_prior_commit() in the commit-by-rotate case, as
you already did this in Binlog_free_flush::commit()).
You also don't need to handle the (!opt_optimize_thread_scheduling) case for
commit-by-rotate, this is old code that is being deprecated. Just add a
condition in should_free_flush() to skip if opt_optimize_thread_scheduling
is set.
Other new function:
void write_transaction_handle_error(group_commit_entry *entry)
This function contains the remainder of the
write_transaction_to_binlog_events() function starting from
"switch (entry->error) ...".
This way, the commit-by-rotate case is cleanly separated from the normal
group commit case and the code of write_transaction_to_binlog_events()
becomes much cleaner.
Now follows more detailed comments on specific parts of the patch:
> diff --git a/sql/log.cc b/sql/log.cc
> index 34f9ad745fc..3dc57b21c05 100644
> --- a/sql/log.cc
> +++ b/sql/log.cc
> @@ -163,6 +163,111 @@ static SHOW_VAR binlog_status_vars_detail[]=
> + void set_reserved_bytes(uint32 header_len)
> + {
> + // Gtid event length
> + header_len+= LOG_EVENT_HEADER_LEN + Gtid_log_event::max_data_length +
> + BINLOG_CHECKSUM_LEN;
> + header_len= header_len - (header_len % IO_SIZE) + IO_SIZE;
This has an off-by-one error, it will round up IO_SIZE to 2*IO_SIZE.
Use eg. header_len = (header_len + (IO_SIZE - 1)) & ~(IO_SIZE - 1)
You can use static_assert((IO_SIZE & (IO_SIZE - 1)) == 0, "power of two")
if you want to catch if anyone would ever change IO_SIZE to be not a power
of two.
+private:
+ Binlog_free_flush &operator=(const Binlog_free_flush &);
+ Binlog_free_flush(const Binlog_free_flush &);
Add before this a comment "singleton object, prevent copying by mistake".
> + char m_cache_dir[FN_REFLEN];
This member appears to be unused?
> + /** The cache_data which will be renamed to binlog. */
> + binlog_cache_data *m_cache_data{nullptr};
Also appears unused.
Maybe add a comment that m_entry and m_replaced are protected by LOCK_log?
> +static Binlog_free_flush binlog_free_flush;
> +ulonglong opt_binlog_free_flush_threshold= 10 * 1024 * 1024;
This initializes the value to 10 MB, but the default value in sys_vars.cc is
128MB, so that seems inconsistent.
> @@ -4126,8 +4238,7 @@ bool MYSQL_BIN_LOG::open(const char *log_name,
> /* Notify the io thread that binlog is rotated to a new file */
> if (is_relay_log)
> signal_relay_log_update();
> - else
> - update_binlog_end_pos();
> +
Why do you remove update_binlog_end_pos() here? Doesn't the binlog dump
threads need this update?
> @@ -8703,7 +8821,16 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry)
> bool
> MYSQL_BIN_LOG::write_transaction_to_binlog_events(group_commit_entry *entry)
> {
> - int is_leader= queue_for_group_commit(entry);
> + int is_leader;
> +
> + if (binlog_free_flush.should_free_flush(entry) &&
> + binlog_free_flush.commit(entry))
> + {
> + is_leader= 1;
> + goto commit;
> + }
> +
> + is_leader= queue_for_group_commit(entry);
As described above, re-factor the write_transaction_to_binlog_events()
function to avoid this assignment of is_leader and the goto.
> @@ -8863,6 +8992,16 @@ MYSQL_BIN_LOG::trx_group_commit_leader(group_commit_entry *leader)
> uint64 commit_id;
> DBUG_ENTER("MYSQL_BIN_LOG::trx_group_commit_leader");
>
> + /*
> + When move a binlog cache to a binlog file, the transaction itself is
> + a group.
> + */
> + if (unlikely(is_free_flush))
> + {
> + last_in_queue= leader;
> + queue= leader;
> + }
> + else
As described above, re-factor trx_group_commit_leader() to avoid this check
in trx_group_commit_leader(), instead calling directly into the relevant
code now in trx_group_commit_with_engines().
> +inline bool Binlog_free_flush::should_free_flush(
> + const MYSQL_BIN_LOG::group_commit_entry *entry) const
> +{
> + /* Do not free flush if total_bytes smaller than limit size. */
> + if (likely(cache_data->get_byte_position() <=
> + opt_binlog_free_flush_threshold))
> + return false;
Put this check as the first check in the function. So the common case of a
small transaction does not need to run any of the other checks.
And as suggested above, put a check:
if (unlikely(!opt_optimize_thread_scheduling))
return false;
(This is not critical, but just to make things a bit easier; there is no
reason for you/us to worry about making commit-by-rotate work with disabled
optimize_thread_scheduling, this option isn't used any more. Otherwise you'd
need to test that your patch works correctly with optimize_thread_scheduling
disabled, which would just be a waste of time.)
> +bool Binlog_free_flush::commit(MYSQL_BIN_LOG::group_commit_entry *entry)
> +{
> + // It will be released by trx_group_commit_leader
> + mysql_mutex_lock(&mysql_bin_log.LOCK_log);
After suggested changes, comment should read "It will be released by
trx_group_commit_with_engines()."
> + mysql_bin_log.trx_group_commit_leader(entry, true /* is_free_flush */);
As discussed above, change this to call a new function
mysql_bin_log.trx_group_commit_with_engines(), which contains the second
part of trx_group_commit_leader() that you need to run. Then there is no
longer a need to pass a `true` flag here to skip the first part.
Also, before the call, this is the place to set entry->next to nullptr, to
convert the stand-alone entry into a (singleton) list (instead of
initializing it in MYSQL_BIN_LOG::write_transaction_to_binlog().)
> @@ -8301,6 +8418,7 @@ MYSQL_BIN_LOG::write_transaction_to_binlog(THD *thd,
> DBUG_RETURN(0);
> }
>
> + entry.next= nullptr;
> entry.thd= thd;
> entry.cache_mngr= cache_mngr;
> entry.error= 0;
Do you need to initialize entry->next here, and if so why?
At this point, the entry is not a list, it is a stand-alone struct that is
not yet linked into any list, the next pointer should be set appropriately
when/if the struct is later put into a list.
More important than the minor cost of initializing, by redundantly
initializing here we are hiding bugs (eg. Valgrind/msan/asan) if some code
would incorrectly read the next pointer.
> +bool Binlog_free_flush::replace_binlog_file()
> +{
> + size_t binlog_size= my_b_tell(&mysql_bin_log.log_file);
> + size_t required_size= binlog_size;
> + // space for Gtid_log_event
> + required_size+= LOG_EVENT_HEADER_LEN + Gtid_log_event::max_data_length +
> + BINLOG_CHECKSUM_LEN;
Just a remark, here you actually know the real required size of the GTID
event, so you don't need to conservatively estimate
Gtid_log_event::max_data_length. But I think doing it this way is fine, it
is simpler and in most cases we are rounding up to IO_SIZE anyway, so I'm
fine with keeping it this way.
> + sql_print_information("Could not rename binlog cache to binlog, "
> + "require %llu bytes but only %llu bytes reserved.",
> + required_size, m_cache_data->file_reserved_bytes());
This log message will perhaps appear a bit out-of-context in the error log.
Suggested clarification:
"Could not rename binlog cache to binlog (as requested by
--binlog-commit-by-rotate-threshold). Required ..."
> + // Set the cache file as binlog file.
> + mysql_file_close(mysql_bin_log.log_file.file, MYF(MY_WME));
Do not to pass MY_WME here (instead pass MYF(0)). Since you are ignoring the
return of the function, so we don't want mysql_file_close to call my_error()
leaving an error code set even though the operation succeeds.
> +size_t Binlog_free_flush::get_gtid_event_pad_size()
> +{
> + size_t begin_pos= my_b_tell(&mysql_bin_log.log_file);
> + size_t pad_to_size=
> + m_cache_data->file_reserved_bytes() - begin_pos - LOG_EVENT_HEADER_LEN;
> +
> + if (binlog_checksum_options != BINLOG_CHECKSUM_ALG_OFF)
> + pad_to_size-= BINLOG_CHECKSUM_LEN;
Adjusting with BINLOG_CHECKSUM_LEN is to account for the 4 bytes of checksum
at the end of the GTID_EVENT, right?
But why do you subtract LOG_EVENT_HEADER_LEN when calculating pad_to_size?
(I mean, it is probably correct, otherwise nothing would work I think. I
just did not understand it, maybe you can explain in a short comment. Or
maybe Gtid_log_event::write() could be modified to do the adjusting for
LOG_EVENT_HEADER_LEN and BINLOG_CHECKSUM_LEN).
> + // offset must be saved before replace_binlog_file(), it will update the pos
> + my_off_t offset= my_b_tell(&log_file);
Suggest to clarify comment to "..., it will update the file position".
> @@ -756,18 +759,20 @@ class MYSQL_BIN_LOG: public TC_LOG, private Event_log
> new_file() is locking. new_file_without_locking() does not acquire
> LOCK_log.
> */
> - int new_file_impl();
> + int new_file_impl(bool is_free_flush= false);
> void do_checkpoint_request(ulong binlog_id);
> - int write_transaction_or_stmt(group_commit_entry *entry, uint64 commit_id);
> + int write_transaction_or_stmt(group_commit_entry *entry, uint64 commit_id,
> + bool is_free_flush= false);
> int queue_for_group_commit(group_commit_entry *entry);
> bool write_transaction_to_binlog_events(group_commit_entry *entry);
> - void trx_group_commit_leader(group_commit_entry *leader);
> + void trx_group_commit_leader(group_commit_entry *leader,
> + bool is_free_flush= false);
> bool is_xidlist_idle_nolock();
> void update_gtid_index(uint32 offset, rpl_gtid gtid);
>
> public:
> void purge(bool all);
> - int new_file_without_locking();
> + int new_file_without_locking(bool is_free_flush= false);
> /*
> A list of struct xid_count_per_binlog is used to keep track of how many
> XIDs are in prepared, but not committed, state in each binlog. And how
> @@ -997,7 +1002,8 @@ class MYSQL_BIN_LOG: public TC_LOG, private Event_log
> enum cache_type io_cache_type_arg,
> ulong max_size,
> bool null_created,
> - bool need_mutex);
> + bool need_mutex,
> + bool is_free_flush = false);
> bool open_index_file(const char *index_file_name_arg,
> const char *log_name, bool need_mutex);
> /* Use this to start writing a new log file */
> @@ -1037,7 +1043,7 @@ class MYSQL_BIN_LOG: public TC_LOG, private Event_log
> bool is_active(const char* log_file_name);
> bool can_purge_log(const char *log_file_name, bool interactive);
> int update_log_index(LOG_INFO* linfo, bool need_update_threads);
> - int rotate(bool force_rotate, bool* check_purge);
> + int rotate(bool force_rotate, bool* check_purge, bool is_free_flush= false);
> void checkpoint_and_purge(ulong binlog_id);
> int rotate_and_purge(bool force_rotate, DYNAMIC_ARRAY* drop_gtid_domain= NULL);
> /**
Style preference: please add the these new boolean parameters without
default value, and instead change existing callers to pass false explicitly.
(And the parameter will be "commit_by_rotate" after name change.)
> +
> +extern ulonglong opt_binlog_free_flush_threshold;
> +static Sys_var_ulonglong Sys_binlog_free_flush_threshold(
> + "binlog_free_flush_threshold",
> + "Try to rename the binlog cache temporary file of the commiting "
> + "transaction to a binlog file when its binlog cache size "
> + "is bigger than the value of this variable",
> + GLOBAL_VAR(opt_binlog_free_flush_threshold),
> + CMD_LINE(REQUIRED_ARG), VALID_RANGE(10 * 1024 * 1024, ULLONG_MAX),
> + DEFAULT(128 * 1024 * 1024), BLOCK_SIZE(1));
As described above:
- Suggested name: "binlog_commit_by_rotate_threshold".
- Suggested text: "For transactions that exceed this size in the binlog,
commit by a more efficient method that forces a binlog rotation and
re-uses the pre-existing temporary file as the next binlog file."
- Let me hear opinions on whether this should default to 128 MB or should
default to eg. ULONGLONG_MAX to disable the feature by default.
> diff --git a/mysql-test/main/tmp_space_usage.test b/mysql-test/main/tmp_space_usage.test
> index af7b295f343..1685dbbc450 100644
> --- a/mysql-test/main/tmp_space_usage.test
> +++ b/mysql-test/main/tmp_space_usage.test
> @@ -215,7 +215,8 @@ select count(distinct concat(seq,repeat('x',1000))) from seq_1_to_1000;
>
> set @save_max_tmp_total_space_usage=@@global.max_tmp_total_space_usage;
> set @@global.max_tmp_total_space_usage=64*1024*1024;
> -set @@max_tmp_session_space_usage=1179648;
> +# Binlog cache reserve 4096 bytes at the begin of the temporary file.
> +set @@max_tmp_session_space_usage=1179648+65536;
> select @@max_tmp_session_space_usage;
> set @save_aria_repair_threads=@@aria_repair_threads;
> set @@aria_repair_threads=2;
> @@ -224,6 +225,7 @@ set @@max_heap_table_size=16777216;
>
> CREATE TABLE t1 (a CHAR(255),b INT,INDEX (b));
> INSERT INTO t1 SELECT SEQ,SEQ FROM seq_1_to_100000;
> +set @@max_tmp_session_space_usage=1179648;
> --error 200
> SELECT * FROM t1 UNION SELECT * FROM t1;
> DROP TABLE t1;
> @@ -266,11 +268,15 @@ SELECT MIN(VARIABLE_VALUE) OVER (), NTILE(1) OVER (), MAX(VARIABLE_NAME) OVER ()
>
> connect(c1, localhost, root,,);
> set @@binlog_format=row;
> -CREATE OR REPLACE TABLE t1 (a DATETIME) ENGINE=MyISAM;
> +CREATE OR REPLACE TABLE t1 (a DATETIME) ENGINE=InnoDB;
> +# Use the transaction to keep binlog cache temporary file large enough
> +BEGIN;
> INSERT INTO t1 SELECT NOW() FROM seq_1_to_6000;
> +
> SET max_tmp_session_space_usage = 64*1024;
> --error 200
> SELECT * FROM information_schema.ALL_PLUGINS LIMIT 2;
> +ROLLBACK;
> drop table t1;
Can you explain why you need these changes to the test case? I was not able
to understand that from the patch.
With these changes, I approve of this patch / pull-request to merge into the
next MariaDB release.
- Kristian.
