Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the --convert-engine-binlog option to mariadb-binlog for converting InnoDB-based engine binlog files into legacy-format binary logs, along with helper functions for event serialization and new test suites. The code review identified several critical issues, including potential NULL pointer dereferences of gtid_state, fdev, gtid_set, and list, as well as a memory leak on write failures in write_event_to_legacy_binlog. Additionally, the feedback highlights return type mismatches, inconsistent boolean usage, and multiple coding standard violations regarding spacing after if keywords.
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.
| if (ev->get_type_code() == GTID_EVENT) { | ||
| rpl_gtid ev_gtid; | ||
| Gtid_log_event *gle= (Gtid_log_event*) ev; | ||
| ev_gtid= {gle->domain_id, gle->server_id, gle->seq_no}; | ||
|
|
||
| gtid_state->update_nolock(&ev_gtid); | ||
| } |
There was a problem hiding this comment.
If gtid_state is not initialized (e.g., if the conversion starts or encounters a GTID_EVENT before gtid_state is allocated), dereferencing it via gtid_state->update_nolock(&ev_gtid) will cause a crash. We should ensure gtid_state is initialized or add a defensive NULL check.
if (ev->get_type_code() == GTID_EVENT) {
rpl_gtid ev_gtid;
Gtid_log_event *gle= (Gtid_log_event*) ev;
ev_gtid= {gle->domain_id, gle->server_id, gle->seq_no};
if (gtid_state)
gtid_state->update_nolock(&ev_gtid);
}| bool | ||
| Gtid_list_log_event::to_packet(String *packet) | ||
| { | ||
| uint32 i; | ||
| uchar *p; | ||
| uint32 needed_length; | ||
|
|
||
| DBUG_ASSERT(count < 1<<28); | ||
|
|
||
| needed_length= packet->length() + get_data_size(); | ||
| if (packet->reserve(needed_length)) | ||
| return true; | ||
| p= (uchar *)packet->ptr() + packet->length();; | ||
| packet->length(needed_length); | ||
| int4store(p, (count & ((1<<28)-1)) | gl_flags); | ||
| p += 4; | ||
| /* Initialise the padding for empty Gtid_list. */ | ||
| if (count == 0) | ||
| int2store(p, 0); | ||
| for (i= 0; i < count; ++i) | ||
| { | ||
| int4store(p, list[i].domain_id); | ||
| int4store(p+4, list[i].server_id); | ||
| int8store(p+8, list[i].seq_no); | ||
| p += 16; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
If list is NULL (which can happen if my_malloc failed during construction or if the event is invalid), dereferencing list[i] in the loop will cause a crash. We should check is_valid() or list != NULL before proceeding. This also cleans up a double semicolon typo on line 2850.
bool
Gtid_list_log_event::to_packet(String *packet)
{
uint32 i;
uchar *p;
uint32 needed_length;
if (!is_valid())
return true;
DBUG_ASSERT(count < 1<<28);
needed_length= packet->length() + get_data_size();
if (packet->reserve(needed_length))
return true;
p= (uchar *)packet->ptr() + packet->length();
packet->length(needed_length);
int4store(p, (count & ((1<<28)-1)) | gl_flags);
p += 4;
/* Initialise the padding for empty Gtid_list. */
if (count == 0)
int2store(p, 0);
for (i= 0; i < count; ++i)
{
int4store(p, list[i].domain_id);
int4store(p+4, list[i].server_id);
int8store(p+8, list[i].seq_no);
p += 16;
}
return false;
}| Gtid_list_log_event::Gtid_list_log_event(rpl_binlog_state_base *gtid_set) | ||
| : count(gtid_set->count_nolock()), gl_flags(0), list(0), sub_id_list(0) | ||
| { | ||
| cache_type= EVENT_NO_CACHE; | ||
| /* Failure to allocate memory will be caught by is_valid() returning false. */ | ||
| if (count < (1<<28) && | ||
| (list = (rpl_gtid *)my_malloc(PSI_INSTRUMENT_ME, | ||
| count * sizeof(*list) + (count == 0), MYF(MY_WME)))) | ||
| { | ||
| gtid_set->get_gtid_list_nolock(list, count); | ||
| } | ||
| } |
There was a problem hiding this comment.
If gtid_set is NULL, calling gtid_set->count_nolock() in the initializer list will result in a NULL pointer dereference and crash. We should add a defensive check to handle a NULL gtid_set gracefully.
Gtid_list_log_event::Gtid_list_log_event(rpl_binlog_state_base *gtid_set)
: count(gtid_set ? gtid_set->count_nolock() : 0), gl_flags(0), list(0), sub_id_list(0)
{
cache_type= EVENT_NO_CACHE;
/* Failure to allocate memory will be caught by is_valid() returning false. */
if (gtid_set && count < (1<<28) &&
(list = (rpl_gtid *)my_malloc(PSI_INSTRUMENT_ME,
count * sizeof(*list) + (count == 0), MYF(MY_WME))))
{
gtid_set->get_gtid_list_nolock(list, count);
}
}
ParadoxV5
added
GSoC
External Contribution
528c9e1 to
da82682
Compare
| static bool init_output_legacy_binlog(FILE **out_file, char *out_name, | ||
| size_t out_name_len) | ||
| size_t out_name_len, bool rotate) | ||
| { | ||
| if (rotate) | ||
| { |
There was a problem hiding this comment.
Having a separate rotate_output_legacy_binlog() that calls init_output_legacy_binlog() sounds better than shoving rotation inside “reïnitialization”, right?
There was a problem hiding this comment.
Yes, thanks for your input. Your suggestion seems like a much better design, so I’ll go ahead and make these changes. Thanks! :)
2251204 to
937845a
Compare
| @@ -1 +1,2 @@ | |||
| --source include/not_embedded.inc | |||
There was a problem hiding this comment.
@ParadoxV5 @bnestere Can you review/confirm this change? mysqlbinlog_convert_engine_binlog_basic test was failing because it was running on embedded server. I could've added this line in the test but this felt like a better option.
There was a problem hiding this comment.
Hm, it makes sense.
Compare with have_log_bin.inc, which literally has:
source include/not_embedded.inc;
In your tests, all but mysqlbinlog_convert_engine_binlog_basic calls have_binlog_format_row.inc, which in turn calls have_log_bin.inc.
I see that most have_innodb_binlog.inc callers aren’t “affected” either because they transitively call have_log_bin.inc.
The other tests I found call not_embedded.inc directly, like your alternative solution; they are:
binlog_in_engine.mariabackup_binlog_dirbinlog_in_engine.mariabackup_binlogsmariabackup_slave_provision.inccallers
I wonder if it even makes sense to include the entire have_log_bin.inc in have_innodb_binlog.inc, not just --log-bin in have_innodb_binlog.opt?
| --source include/not_embedded.inc | |
| --source include/have_log_bin.inc |
Though a shortcoming with have_log_bin.inc is that it will run the test in each of the three content formats (Mixed (default), Statement, Row).
This is unnecessary for tests that are more about the container than the contained contents, including yours, hence the calls to have_binlog_format_X.inc.
There was a problem hiding this comment.
Could you elaborate a bit more on the shortcoming? I’m not sure I fully understood it.
My intention was for mysqlbinlog_convert_engine_binlog_basic to run with all three binlog formats: ROW, STATEMENT, and MIXED.
I did consider the change you suggested, but I was hesitant because have_log_bin.inc internally sources include/no_view_protocol.inc, and I wasn’t sure whether that could introduce any unexpected side effects. That said, logically, including have_log_bin.inc from have_innodb_binlog.inc does seem coherent.
There was a problem hiding this comment.
(answered in our fortnightly call)
6e98f55 to
1557281
Compare
6748fdc to
842ff82
Compare
knielsen
left a comment
knielsen
left a comment
There was a problem hiding this comment.
Some initial review comments on the draft patch in email comment.
|
Hi Tarun,
MDEV-37605 Extend mariadb-binlog to convert innodb based binary logs to legacy
This is an initial work-in-progress patch for MDEV-37605: Convert InnoDB
Binary Logs to Legacy Format.
The goal of this work is to extend mariadb-binlog so that it can read
InnoDB-based binary log files (.ibb) and generate legacy-format binary log
files as output. This is intended to support compatibility use cases such as
downgrades, migrations, and tooling that expects the legacy binlog format.
I read through the current draft patch for this.
Overall, I think it looks quite good, thanks for your efforts on this!
The approach looks good. And the patch itself looks quite clean.
I didn't spot any major issues in the code itself. As you also remark, more
testing will be needed, both additional mysql-test-run tests and hopefully
the Corporation replication test team can help do some testing as well.
But overall looks good.
A couple general comments:
I see you were able to use the to_packet() functions to avoid duplicating
the code for encoding the couple special events, that's good. I also see
that those to_packet() functions don't encode the full event (they miss the
common header part), I didn't realise that when I suggested it. So you had
to implement write_event_header(); but it seems reasonable the way you did
it.
In addition to the test cases you already added that use `mysqlbinlog | mysql`
to replay the converted binlogs (which look good), a good idea would be some
tests that try to install the converted binlogs in a master server and see
that a slave can replicate successfully from that. An example of how this
can be done is in eg. mysql-test/suite/rpl/t/rpl_old_master.test .
Below some detailed comments, mostly just minor stuff and a question or two,
except for the remark about checksums on rotate events:
+bool binlog_reader_innodb::get_initial_gtid_state(
+ rpl_binlog_state_base *gtid_state)
+{
+ chunk_reader_mysqlbinlog::saved_position saved_pos;
+
+ chunk_rd.save_pos(&saved_pos);
+ chunk_rd.seek(start_file_no, binlog_page_size);
+
+ int res= read_gtid_state(gtid_state);
+
+ chunk_rd.restore_pos(&saved_pos);
+ return res != 1;
+}
+bool read_initial_gtid_state(handler_binlog_reader *generic_reader,
+ rpl_binlog_state_base *gtid_state)
+{
+ binlog_reader_innodb *reader= (binlog_reader_innodb *) generic_reader;
+ return reader->get_initial_gtid_state(gtid_state);
+}
Since you're only calling get_initial_gtid_state() with an unitialised
binlog reader, you don't need to save and restore the position in that
function.
+static bool write_format_description_event_to_legacy_binlog(
+ FILE *outfile, Format_description_log_event *fdev)
+{
+ // temp_buf stores the raw bytes of the event and data_written is the length of those raw bytes
As a general rule, we keep line lengths to <= 80 characters.
+static bool
+write_rotate_log_event_to_legacy_binlog(FILE *outfile,
+ const char *binlog_file_name)
+{
+ /* Write body of ROTATE_EVENT */
+ int8store(buf + R_POS_OFFSET, BIN_LOG_HEADER_SIZE);
+
+ if (my_fwrite(outfile, (const uchar *) buf, ROTATE_HEADER_LEN, MYF(MY_NABP)))
+ {
+ error("Could not write body into converted binlog file '%s'",
+ out_file_name);
+ return true;
+ }
+
+ if (my_fwrite(outfile, (const uchar *) p, ident_len, MYF(MY_NABP)))
+ {
+ error("Could not write body into converted binlog file '%s'",
+ out_file_name);
+ return true;
+ }
Here, it would be good to introduce Rotate_log_event::to_packet() and use it
in Rotate_log_event::write(), similar to how it's done for eg.
Gtid_list_log_event. Then we don't duplicate the logic for serialising the
data for Rotate_log_event.
+ /* we are not writing the footer because we are not supporting the checksum
+ in every event */
This I don't understand? Every event in the binlog file has to have the same
status for the checksum present (except FORMAT_DESCRIPTION which always
has the checksum). I think if you write a Rotate_log_event without checksum
in a file where the other events have such checksums, it will not work?
+/*
+ Writes the event to the converted legacy binlog file
+ @PARAM ev: The event to write
+ @return: OK_CONTINUE if successful, ERROR_STOP if failed
+*/
+static Exit_status write_event_to_legacy_binlog(Log_event *ev)
+{
+ fflush(output_legacy_binlog_file);
I'm thinking you don't need these fflush() calls after writing each event
(here, and in some other places)? The flush will happen when the file is
closed.
I think the existing fflush() calls around the mysqlbinlog.cc code are there
to fix the interactive usage of `mysqlbinlog --read-from-remote-server`, so
that events will be output immediately to the console and not buffered
indefinitely while waiting for more events to be sent on the socket from the
mariadb server. That does not seem needed for writing converted binary log
files.
@@ -3504,7 +4104,6 @@ int main(int argc, char** argv)
error("The --raw mode is not allowed with --flashback mode");
die(1);
}
-
if (opt_flashback)
{
my_init_dynamic_array(PSI_NOT_INSTRUMENTED, &binlog_events,
We try to avoid unrelated whitespace/formatting changes like this (just as a
general info).
+# Rotate so binlog-000000.ibb is complete on disk. Waiting for the *next*
+# pre-allocated file (binlog-000002.ibb, fully pre-allocated and empty)
+# guarantees both 000000 and the new active 000001 exist.
It's probably more that waiting for 000002 to exist guarantees that all data
in 000000 is available to read from disk by mysqlbinlog (as opposed to being
only available in the in-memory page fifo in the server). The code in the
test case for this is fine.
+# this will trigger a ROTATE_EVENT and will be converted to/end of gtid_conv.000003
+
+--source include/restart_mysqld.inc
+
+# start of converted gtid_conv.000004
For my understanding: restarting mysqld does not cause a rotation or
ROTATE_EVENT in the InnoDB binlog. Is the point here that the restart format
description event that gets written to the InnoDB binlog at restart, will
cause the convertion to force a rotate of the generated legacy binlog file?
(If so, that makes sense).
+# end of binlog-000002.ibb which will be converted to gtid_conv.000004
+FLUSH BINARY LOGS;
+
+--let $binlog_name= binlog-000003.ibb
+--let $binlog_size= 262144
+--source include/wait_for_engine_binlog.inc
+
+
+--echo *** Convert the generated InnoDB binlogs to legacy format
+--exec $MYSQL_BINLOG --convert-engine-binlog --result-file=$MYSQL_TMP_DIR/gtid_conv $datadir/binlog-000000.ibb $datadir/binlog-000001.ibb $datadir/binlog-000002.ibb
Should we wait for 000004 here? To make sure that 000003 (active) and 000004
(empty, pre-allocated) are the two binlog files that can be in the page
fifo, and 000002 is sure to be on disk for mysqlbinlog to read? To avoid a
race where the data might still be only in the page fifo when mysqlbinlog
tries to run it.
Later the test seems to read 000003 with mysqlbinlog without waiting for the
data to be guaranteed there. It will be a very rare race to hit since the
test is doing a lot before, giving the server a lot of time to flush, so you
might not see the race.
diff --git a/sql/log_event_client.cc b/sql/log_event_client.cc
index ab884da52c5..712d359ddc5 100644
--- a/sql/log_event_client.cc
+++ b/sql/log_event_client.cc
@@ -2353,6 +2353,24 @@ bool Binlog_checkpoint_log_event::print(FILE *file,
return cache.flush_data();
}
+/*
+
+ Constructor for Gtid_list_log_event.
+ Used in mysqlbinlog to generate GTID_LIST_EVENT while converting the engine binlog to legacy binlog.
+ TODO: Tarun get this reviewed.
+*/
+Gtid_list_log_event::Gtid_list_log_event(rpl_binlog_state_base *gtid_set)
+ : count(gtid_set->count_nolock()), gl_flags(0), list(0), sub_id_list(0)
+{
+ cache_type= EVENT_NO_CACHE;
+ /* Failure to allocate memory will be caught by is_valid() returning false. */
+ if (count < (1<<28) &&
+ (list = (rpl_gtid *)my_malloc(PSI_INSTRUMENT_ME,
+ count * sizeof(*list) + (count == 0), MYF(MY_WME))))
+ {
+ gtid_set->get_gtid_list_nolock(list, count);
+ }
+}
I think you can just move the code for
Gtid_list_log_event::Gtid_list_log_event(rpl_binlog_state *, uint32)
to log_event.cc and use that, to avoid introducing this function which
almost exactly duplicates the code.
diff --git a/sql/rpl_gtid.cc b/sql/rpl_gtid.cc
index 430e4a4bfc6..e4fa974d961 100644
--- a/sql/rpl_gtid.cc
+++ b/sql/rpl_gtid.cc
+#endif // MYSQL_CLIENT
/* Helper functions for update. */
int
-rpl_binlog_state::element::update_element(const rpl_gtid *gtid)
+rpl_binlog_state_base::element::update_element(const rpl_gtid *gtid)
{
rpl_gtid *lookup_gtid;
@@ -1938,6 +1940,7 @@ rpl_binlog_state::element::update_element(const rpl_gtid *gtid)
return 0;
}
+#ifndef MYSQL_CLIENT
You can also just move that function somewhere outside existing #ifndef
MYSQL_CLIENT to avoid introducing extra #ifdef.
diff --git a/sql/sql_repl.cc b/sql/sql_repl.cc
index e178ca78502..4e80e939adc 100644
--- a/sql/sql_repl.cc
+++ b/sql/sql_repl.cc
@@ -225,6 +225,7 @@ static int fake_rotate_event(binlog_send_info *info, ulonglong position,
{
DBUG_ENTER("fake_rotate_event");
ulong ev_offset;
+ /* TODO: Tarun get this reviewed. buf should be buf[ROTATE_HEADER_LEN] */
char buf[ROTATE_HEADER_LEN+100];
my_bool do_checksum;
int err;
Agree, this change looks correct.
Thanks,
- Kristian.
|
|
Hi Kristian,
Thanks for the detailed review :)
I agree with all the suggested changes. I've replied inline, and have a two
follow-up questions on the checksum/footer handling and on
get_initial_gtid_state.
a good idea would be some
tests that try to install the converted binlogs in a master server and see
that a slave can replicate successfully from that. An example of how this
can be done is in eg. mysql-test/suite/rpl/t/rpl_old_master.test .
Noted. I'll go through this test and will add a similar test that installs the
converted binlogs on a master server and verifies that a slave can replicate
successfuly from that.
> +bool binlog_reader_innodb::get_initial_gtid_state(
> + rpl_binlog_state_base *gtid_state)
> +{
> + chunk_reader_mysqlbinlog::saved_position saved_pos;
> +
> + chunk_rd.save_pos(&saved_pos);
> + chunk_rd.seek(start_file_no, binlog_page_size);
> +
> + int res= read_gtid_state(gtid_state);
> +
> + chunk_rd.restore_pos(&saved_pos);
> + return res != 1;
> +}
> +bool read_initial_gtid_state(handler_binlog_reader *generic_reader,
> + rpl_binlog_state_base *gtid_state)
> +{
> + binlog_reader_innodb *reader= (binlog_reader_innodb *) generic_reader;
> + return reader->get_initial_gtid_state(gtid_state);
> +}
Since you're only calling get_initial_gtid_state() with an unitialised
binlog reader, you don't need to save and restore the position in that
function.
Yes, I know this and I intentionally did it this way in case someone
tries to reuse
the binlog_reader_innodb::get_initial_gtid_state method in future.
Should I still go
ahead and remove the save and restore position logic from that method?
> +static bool
> +write_rotate_log_event_to_legacy_binlog(FILE *outfile,
> + const char *binlog_file_name)
> +{
> + /* Write body of ROTATE_EVENT */
> + int8store(buf + R_POS_OFFSET, BIN_LOG_HEADER_SIZE);
> +
> + if (my_fwrite(outfile, (const uchar *) buf, ROTATE_HEADER_LEN, MYF(MY_NABP)))
> + {
> + error("Could not write body into converted binlog file '%s'",
> + out_file_name);
> + return true;
> + }
> +
> + if (my_fwrite(outfile, (const uchar *) p, ident_len, MYF(MY_NABP)))
> + {
> + error("Could not write body into converted binlog file '%s'",
> + out_file_name);
> + return true;
> + }
Here, it would be good to introduce Rotate_log_event::to_packet() and use it
in Rotate_log_event::write(), similar to how it's done for eg.
Gtid_list_log_event. Then we don't duplicate the logic for serialising the
data for Rotate_log_event.
Noted. I will introduce Rotate_log_event::to_packet() and reuse it to avoid
duplicating the serialization logic.
> + /* we are not writing the footer because we are not supporting the checksum
> + in every event */
This I don't understand? Every event in the binlog file has to have the same
status for the checksum present (except FORMAT_DESCRIPTION which always
has the checksum). I think if you write a Rotate_log_event without checksum
in a file where the other events have such checksums, it will not work?
As discussed earlier, we are not adding support for event-level
checksums during
conversion, to keep the implementation simple. Only the
FORMAT_DESCRIPTION_EVENT
contains a checksum. Since the other events do not require a checksum, we do not
need to write a footer for them, which is why I left this comment here.
I do have one follow-up question. For some of the other events, such as
BINLOG_CHECKPOINT_EVENT, I currently have code like this:
bool write_binlog_checkpoint_event_to_legacy_binlog()
{
header_and_body_logic();
if (write_event_footer(outfile, do_checksum, crc))
{
error();
return true;
}
}
Here, do_checksum is always false, so write_event_footer() effectively
does nothing.
Should I remove these no-ops blocks as well?
For write_rotate_log_event_to_legacy_binlog(), I can either remove the
existing comment
or add the same write_event_footer() block for consistency. Let me
know what you think.
> +/*
> + Writes the event to the converted legacy binlog file
> + @PARAM ev: The event to write
> + @return: OK_CONTINUE if successful, ERROR_STOP if failed
> +*/
> +static Exit_status write_event_to_legacy_binlog(Log_event *ev)
> +{
> + fflush(output_legacy_binlog_file);
I'm thinking you don't need these fflush() calls after writing each event
(here, and in some other places)? The flush will happen when the file is
closed.
I think the existing fflush() calls around the mysqlbinlog.cc code are there
to fix the interactive usage of `mysqlbinlog --read-from-remote-server`, so
that events will be output immediately to the console and not buffered
indefinitely while waiting for more events to be sent on the socket from the
mariadb server. That does not seem needed for writing converted binary log
files.
Yes, that makes sense. I remove the unnecessary fflush calls.
> @@ -3504,7 +4104,6 @@ int main(int argc, char** argv)
> error("The --raw mode is not allowed with --flashback mode");
> die(1);
> }
> -
> if (opt_flashback)
> {
> my_init_dynamic_array(PSI_NOT_INSTRUMENTED, &binlog_events,
We try to avoid unrelated whitespace/formatting changes like this (just as a
general info).
My bad. I will keep this in mind in future.
> +# Rotate so binlog-000000.ibb is complete on disk. Waiting for the *next*
> +# pre-allocated file (binlog-000002.ibb, fully pre-allocated and empty)
> +# guarantees both 000000 and the new active 000001 exist.
It's probably more that waiting for 000002 to exist guarantees that all data
in 000000 is available to read from disk by mysqlbinlog (as opposed to being
only available in the in-memory page fifo in the server). The code in the
test case for this is fine.
Understood. I will go ahead and update this comment.
> +# this will trigger a ROTATE_EVENT and will be converted to/end of gtid_conv.000003
> +
> +--source include/restart_mysqld.inc
> +
> +# start of converted gtid_conv.000004
For my understanding: restarting mysqld does not cause a rotation or
ROTATE_EVENT in the InnoDB binlog. Is the point here that the restart format
description event that gets written to the InnoDB binlog at restart, will
cause the convertion to force a rotate of the generated legacy binlog file?
(If so, that makes sense).
Yes thats correct. Restart will write a FORMAT_DESCRIPTION_EVENT which
will cause
the conversion to force a rotate. I'll rewrite this comment to make this clear.
> +# end of binlog-000002.ibb which will be converted to gtid_conv.000004
> +FLUSH BINARY LOGS;
> +
> +--let $binlog_name= binlog-000003.ibb
> +--let $binlog_size= 262144
> +--source include/wait_for_engine_binlog.inc
> +
> +
> +--echo *** Convert the generated InnoDB binlogs to legacy format
> +--exec $MYSQL_BINLOG --convert-engine-binlog --result-file=$MYSQL_TMP_DIR/gtid_conv $datadir/binlog-000000.ibb $datadir/binlog-000001.ibb $datadir/binlog-000002.ibb
Should we wait for 000004 here? To make sure that 000003 (active) and 000004
(empty, pre-allocated) are the two binlog files that can be in the page
fifo, and 000002 is sure to be on disk for mysqlbinlog to read? To avoid a
race where the data might still be only in the page fifo when mysqlbinlog
tries to run it.
Yes. My bad. I will change it to wait for 000004 here.
Later the test seems to read 000003 with mysqlbinlog without waiting for the
data to be guaranteed there. It will be a very rare race to hit since the
test is doing a lot before, giving the server a lot of time to flush, so you
might not see the race.
Thanks for this. I will go through the tests once more and ensure these issues
are not repeated anywhere else.
> diff --git a/sql/log_event_client.cc b/sql/log_event_client.cc
> index ab884da52c5..712d359ddc5 100644
> --- a/sql/log_event_client.cc
> +++ b/sql/log_event_client.cc
> @@ -2353,6 +2353,24 @@ bool Binlog_checkpoint_log_event::print(FILE *file,
> return cache.flush_data();
> }
>
> +/*
> +
> + Constructor for Gtid_list_log_event.
> + Used in mysqlbinlog to generate GTID_LIST_EVENT while converting the engine binlog to legacy binlog.
> + TODO: Tarun get this reviewed.
> +*/
> +Gtid_list_log_event::Gtid_list_log_event(rpl_binlog_state_base *gtid_set)
> + : count(gtid_set->count_nolock()), gl_flags(0), list(0), sub_id_list(0)
> +{
> + cache_type= EVENT_NO_CACHE;
> + /* Failure to allocate memory will be caught by is_valid() returning false. */
> + if (count < (1<<28) &&
> + (list = (rpl_gtid *)my_malloc(PSI_INSTRUMENT_ME,
> + count * sizeof(*list) + (count == 0), MYF(MY_WME))))
> + {
> + gtid_set->get_gtid_list_nolock(list, count);
> + }
> +}
I think you can just move the code for
Gtid_list_log_event::Gtid_list_log_event(rpl_binlog_state *, uint32)
to log_event.cc and use that, to avoid introducing this function which
almost exactly duplicates the code.
Noted. I'll go ahead and implement these changes.
Thanks again for the thorough review.
Best,
Tarun
|
|
tarun wadhwa ***@***.***> writes:
Thanks for the detailed review :)
You're welcome. It was a pleasure to read the patch. In my experience, when
the code reads so simple and effortless, it is because a significant effort
went into making it so. Nice work.
> Since you're only calling get_initial_gtid_state() with an unitialised
> binlog reader, you don't need to save and restore the position in that
> function.
Yes, I know this and I intentionally did it this way in case someone
tries to reuse
the binlog_reader_innodb::get_initial_gtid_state method in future.
Should I still go
ahead and remove the save and restore position logic from that method?
I think it is fine to do as you intended. (I was slightly wondering if it
was valid to save and restore on a newly constructed chunk reader that
hadn't had seek() called on it, but it looks like the constructor
initialises things and it should be fine.)
> This I don't understand? Every event in the binlog file has to have the same
> status for the checksum present (except FORMAT_DESCRIPTION which always
> has the checksum). I think if you write a Rotate_log_event without checksum
> in a file where the other events have such checksums, it will not work?
>
>
As discussed earlier, we are not adding support for event-level
checksums during
conversion, to keep the implementation simple. Only the
Ah, right, that makes sense.
contains a checksum. Since the other events do not require a checksum, we do not
need to write a footer for them, which is why I left this comment here.
Agree, that sounds good then.
I do have one follow-up question. For some of the other events, such as
BINLOG_CHECKPOINT_EVENT, I currently have code like this:
bool write_binlog_checkpoint_event_to_legacy_binlog()
{
header_and_body_logic();
if (write_event_footer(outfile, do_checksum, crc))
{
error();
return true;
}
}
Here, do_checksum is always false, so write_event_footer() effectively
does nothing.
Should I remove these no-ops blocks as well?
I think it's fine either way, up to you.
For write_rotate_log_event_to_legacy_binlog(), I can either remove the
existing comment
or add the same write_event_footer() block for consistency. Let me
know what you think.
I think it's fine, I just hadn't realised that we're not writing checksums
in the converted binlogs, hence I didn't understand the code. Up to you.
- Kristian.
|
|
You're welcome. It was a pleasure to read the patch. In my experience, when
the code reads so simple and effortless, it is because a significant effort
went into making it so. Nice work.
Thank you so much :) That means a lot!
I’ll keep the save-and-restore logic as intended. For consistency,
I’ll also keep
the write_event_footer() blocks for the other events and add the same block for
the rotate event.
Best,
Tarun
|
5091a7e to
b6eae6e
Compare
|
Hi Kristian,
While implementing your suggested changes for the PR
<#5276>, I ran into a few
questions that I wanted to discuss with you before proceeding.
> +static bool
> +write_rotate_log_event_to_legacy_binlog(FILE *outfile,
> + const char *binlog_file_name)
> +{
> + /* Write body of ROTATE_EVENT */
> + int8store(buf + R_POS_OFFSET, BIN_LOG_HEADER_SIZE);
> +
> + if (my_fwrite(outfile, (const uchar *) buf, ROTATE_HEADER_LEN,
MYF(MY_NABP)))
> + {
> + error("Could not write body into converted binlog file '%s'",
> + out_file_name);
> + return true;
> + }
> +
> + if (my_fwrite(outfile, (const uchar *) p, ident_len, MYF(MY_NABP)))
> + {
> + error("Could not write body into converted binlog file '%s'",
> + out_file_name);
> + return true;
> + }
Here, it would be good to introduce Rotate_log_event::to_packet() and use
it
in Rotate_log_event::write(), similar to how it's done for eg.
Gtid_list_log_event. Then we don't duplicate the logic for serialising the
data for Rotate_log_event.
Here, you recommended extracting Rotate_log_event::to_packet(). However,
instead of creating a Rotate_log_event object, I am currently generating
its
serialized representation directly and writing it to the output binlog file.
If we decide to follow your suggested approach, I would first need to
create a
Rotate_log_event object. This would require moving its constructor from
log_event_server.cc to log_event.cc.
The same applies to Binlog_checkpoint_log_event.
I am not sure which approach would be preferable, so I wanted to get your
opinion before proceeding.
> +/*
> +
> + Constructor for Gtid_list_log_event.
> + Used in mysqlbinlog to generate GTID_LIST_EVENT while converting the
engine binlog to legacy binlog.
> + TODO: Tarun get this reviewed.
> +*/
> +Gtid_list_log_event::Gtid_list_log_event(rpl_binlog_state_base
*gtid_set)
> + : count(gtid_set->count_nolock()), gl_flags(0), list(0),
sub_id_list(0)
> +{
> + cache_type= EVENT_NO_CACHE;
> + /* Failure to allocate memory will be caught by is_valid() returning
false. */
> + if (count < (1<<28) &&
> + (list = (rpl_gtid *)my_malloc(PSI_INSTRUMENT_ME,
> + count * sizeof(*list) + (count == 0),
MYF(MY_WME))))
> + {
> + gtid_set->get_gtid_list_nolock(list, count);
> + }
> +}
I think you can just move the code for
Gtid_list_log_event::Gtid_list_log_event(rpl_binlog_state *, uint32)
to log_event.cc and use that, to avoid introducing this function which
almost exactly duplicates the code.
In mysqlbinlog.cc, we are using rpl_binlog_state_base instead of
rpl_binlog_state, which uses locks internally when accessing and modifying
its state.
I am unsure whether we should introduce this locking on the client side, so
I
wanted to confirm with you before proceeding.
Thanks,
Tarun
|
|
tarun wadhwa ***@***.***> writes:
While implementing your suggested changes for the PR
<#5276>, I ran into a few
questions that I wanted to discuss with you before proceeding.
Thanks for explaining these points, Tarun.
At the root of these issues is the awkward way that code is being shared
between mysqlbinlog and the server, with a mess of #include of .cc sources
and sprinkled #ifdef. This is causing a number of difficulties when trying
to share code, including these two issues that I hadn't realised when I
first commented:
> Here, it would be good to introduce Rotate_log_event::to_packet() and use
it
> in Rotate_log_event::write(), similar to how it's done for eg.
> Gtid_list_log_event. Then we don't duplicate the logic for serialising the
> data for Rotate_log_event.
>
Here, you recommended extracting Rotate_log_event::to_packet(). However,
instead of creating a Rotate_log_event object, I am currently generating
its
serialized representation directly and writing it to the output binlog file.
Aha, right. Generally, we have the code shared for _reading_ Log_event
subclasses shared with the mysqlbinlog, but not the ability to _create_ new
Log_event objects.
I'd forgotten that the to_packet() methods work off an existing object.
Then I think your original code is better than trying to introduce Log_event
constructors into the mysqlbinlog tool. The serialisation of the
Rotate_log_event is very simple, it's not a huge issue to have it duplicated
here, no reason to introduce the complexity of trying to import the
Log_event constructors just for this.
If we wanted to share the code for the serialisation of ROTATE_EVENT (and
BINLOG_CHECKPOINT_EVENT?), the approach we could take is to introduce the
counterparts to the static methods like Gtid_log_event::peek(), and reuse
such methods both in mysqlbinlog and inside the server in
Rotate_log_event::to_packet(). But for now it's probably simpler to just
accept a bit of code duplication here as the best compromise.
> I think you can just move the code for
>
> Gtid_list_log_event::Gtid_list_log_event(rpl_binlog_state *, uint32)
>
> to log_event.cc and use that, to avoid introducing this function which
> almost exactly duplicates the code.
>
In mysqlbinlog.cc, we are using rpl_binlog_state_base instead of
rpl_binlog_state, which uses locks internally when accessing and modifying
its state.
Indeed, I'd missed that. Trying to introduce Log_event subclass constructors
into mysqlbinlog would be much more complicated than just moving the code.
For example as you point out with internal server locking.
Thanks for the explanations. I'm fine with your original code in these two
cases then.
- Kristian.
|
|
Hi Kristian,
Code sharing is not supposed to be complicated, innit?
Looks like this is another [fun](https://justforfunnoreally.dev/) project ~~for when I’m not chasing priorities~~ –
Optimistically, just a batch of moves and renames will get rid of this restrictive pile of `#ifdef`s.
Though some class members might need adjustment to avoid knowing server spaghetti such as `THD`s; when the need calls, we can tie in [an OOP redesign](https://lists.mariadb.org/hyperkitty/list/developers@lists.mariadb.org/message/IFJPYZDI5NM2MOOXS4QPTU6TTURFLM7L/).
|
|
Hi Kristian,
Thanks for the clarification. That makes sense. I will go ahead with my original code and proceed with remaining review changes. Thanks, |
b6eae6e to
d664f90
Compare
| { | ||
| if (rotate_output_legacy_binlog(&output_legacy_binlog_file, | ||
| out_file_name, sizeof(out_file_name))) | ||
| goto err; |
There was a problem hiding this comment.
This will result in a double-free of the glob_description_event (i.e. at label err, ev is deleted; and then in cleanup(), glob_description_event is freed again). You should just be able to return ERROR_STOP here.
There was a problem hiding this comment.
Noted. And is fixed now.
| } | ||
|
|
||
| /* Write the GTID_LIST_EVENT to the output legacy binlog file */ | ||
| Gtid_list_log_event gle= Gtid_list_log_event(gtid_state); |
There was a problem hiding this comment.
After instantiating this, it should be validated (i.e.
| Gtid_list_log_event gle= Gtid_list_log_event(gtid_state); | |
| Gtid_list_log_event gle= Gtid_list_log_event(gtid_state); | |
| if (!gle.is_valid()) | |
| { | |
| ... |
There was a problem hiding this comment.
Noted. I’ve added the validation now. Thanks for catching this! :)
| error("The --convert-engine-binlog option does not support GTID values " | ||
| "for --start-position or --stop-position"); | ||
| die(1); | ||
| } |
There was a problem hiding this comment.
We should be comprehensive in this list, i.e. also deny: --database, --table, --start-position (you already deny --stop-position earlier), --start-datetime, --offset, --do-domain-ids, --ignore-domain-ids, --do-server-ids, --ignore-server-ids,
| OK_CONTINUE) | ||
| goto end; | ||
|
|
||
| if (opt_convert_engine_binlog) { |
There was a problem hiding this comment.
bracket should be on new line (and on the else at 4046)
There was a problem hiding this comment.
Thanks for catching this. I’ve fixed it. I’ll run the formatter on all my changes when I squash my commits into a single commit.
| end: | ||
| if (output_legacy_binlog_file) | ||
| { | ||
| if (my_fclose(output_legacy_binlog_file, MYF(0))) |
There was a problem hiding this comment.
hmm - something doesn't seem right here. dump_local_log_entries would convert one binlog-in-innodb file to legacy. Though IIRC, we decided we didn't want a 1-to-1 relationship between innodb-binlog files to legacy binlog files. So here you close and nullify the output_legacy_binlog_flie, though IIUC, that initializes a new legacy file (with new counter) when the next innodb binlog file is read (or around then)
So what is this hunk trying to do?
| FOUND 1 /Gtid list/ in conv_listing.txt | ||
| FOUND 1 /Binlog checkpoint conv.000001/ in conv_listing.txt | ||
| FOUND 1 /Start: binlog v 4/ in conv_listing.txt | ||
| FOUND 1 /server v 13.1.0-MariaDB/ in conv_listing.txt |
There was a problem hiding this comment.
We shouldn't output any hard server versions in result files, any time we have a new release, it would cause this to fail. I'd instead change this to use assert_grep.inc and assert the version is "correct" (there should be tons of examples of this in our existing suite).
There was a problem hiding this comment.
That makes sense. I’ll replace the hardcoded server version with assert_grep.inc and validate that the version is correct. Thanks for pointing this out!
| --source include/reset_master.inc | ||
|
|
||
| # Fixed timestamp for deterministic event listings (with --timezone=GMT-3 | ||
| # from the -master.opt file, matching binlog_in_engine.mysqlbinlog). |
There was a problem hiding this comment.
Hmm there is no mysqlbinlog_ocnvet_engine_binlog_basic-master.opt file, did you miss uploading it?
There was a problem hiding this comment.
My bad. This comment was left over from the initial version of the test, and I missed removing it. I’ve removed it now.
| { | ||
|
|
||
| delete glob_description_event; | ||
| glob_description_event= (Format_description_log_event *) ev; |
There was a problem hiding this comment.
My understanding is that if the innodb binlog writes an FDE, it was due to a restart, and will have some created timestamp. This will drop temporary tables. Which is what we want for this FDE, but if we overwrite the glob_description_event with this FDE, then that means any future legacy binlogs we open (say due to reaching --max-binlog-size) will be written with a created field filled out to this value. This will cause temporary tables to drop for each legacy log rotation.
I think we don't want to delete/overwrite the glob_description_event - I think all we want to do is simply print the new found FDE (or maybe actually rotate? I'm not sure it matters either way, but a simple print is cheaper). This would also override my just-below comment about a double-free on the glob_description_event.
Though I'd like to defer to @knielsen to confirm
There was a problem hiding this comment.
I have made the changes to not overwrite the glob_description_event when we encounter a FDE in innodb binlog. And therefore, we will use the dynamically generated FDE with updated timestamp, whenever we rotate the output legacy binlog based on the max-file-size.
|
@tarunw07 ah a couple more quick comments:
|
|
Brandon Nesterenko ***@***.***> writes:
@bnestere requested changes on this pull request.
Hi @tarunw07 !
This is looking very nice, it is so close to the finish line :) Thanks for all your hard work!
end:
+ if (output_legacy_binlog_file)
+ {
+ if (my_fclose(output_legacy_binlog_file, MYF(0)))
hmm - something doesn't seem right here. `dump_local_log_entries` would convert *one* binlog-in-innodb file to legacy. Though IIRC, we decided we didn't want a 1-to-1 relationship between innodb-binlog files to legacy binlog files. So here you close and nullify the `output_legacy_binlog_flie`, though IIUC, that initializes a new legacy file (with new counter) when the next innodb binlog file is read (or around then)
So what is this hunk trying to do?
And some comments from Tarun on Zulip:
As I see it, we therefore have two possible approaches:
1. Maintain a one-to-one mapping between the supplied .ibb files and legacy
binlog files. In some cases, such as an .ibb file containing no GTID
events, this might not produce an output file.
2. Generate a combined legacy binlog and require the user to supply the
.ibb files in the correct order.
The second approach is the correct one.
The innodb binlog cannot meaningfully be considered as individual files like
the legacy one can. It is possible for a single transaction to be spread
across arbitrary number of binlog files. The split into individual files is
there to make rotation simpler, basically, but the innodb binlog needs to be
considered as a whole thing in its entirety.
I do not think we can meaningfully try to handle .ibb files in the wrong
order. Even if the user would specify eg.
mysqlbinlog ... binlog-000003.ibb binlog-000001.ibb
it may still be the case that both binlog-000001.ibb and binlog-000002.ibb
are read first, in case binlog-000003.ibb refers back to out-of-band data in
those two files.
So the innodb binlog should be read continuously from one file into the
other, a switch to the next file should not affect the output. Rotation of
the output legacy binlog should occur only based on size (or before a
restart FD event), and only at the end of an event group.
We need a test case that tests the conversion to legacy format when a large
event group crosses from one file into the other, to see that the entire
event group is included exactly once in the output, and that rotation of the
legacy binlog is delayed until the end of the event group.
+ // if event type is FORMAT_DESCRIPTION_EVENT, store the event in global
+ // variable glob_description_event
+ if (ev->get_type_code() == FORMAT_DESCRIPTION_EVENT)
+ {
+
+ delete glob_description_event;
+ glob_description_event= (Format_description_log_event *) ev;
My understanding is that if the innodb binlog writes an FDE, it was
due to a restart, and will have some `created` timestamp. This will
drop temporary tables. Which is what we want for _this_ FDE, but if we
overwrite the `glob_description_event` with this FDE, then that means
any future legacy binlogs we open (say due to reaching
`--max-binlog-size`) will be written with a `created` field filled out
to this value. This will cause temporary tables to drop for each
legacy log rotation.
I think we don't want to delete/overwrite the `glob_description_event`
- I think all we want to do is simply print the new found FDE (or
maybe actually rotate? I'm not sure it matters either way, but a
simple print is cheaper). This would also override my just-below
comment about a double-free on the `glob_description_event`.
Though I'd like to defer to @knielsen to confirm
Agree, good point Brandon. I didn't check the code in detail, but yes, the
restart FD must only be output once (per occurrence in the innodb binlog).
And a rotation of the output legacy binlog is needed before outputting the
restart FD I think. I'm not 100% sure if there is actually any code in the
slave that relies on this rotation, but the server always does such rotate
of the legacy binlog when restarting, so it cannot hurt.
(The innodb binlog does not do any rotation at restart, even when restarting
after an upgrade, it continues from the point inside the last active file
where it ended before the restart).
Hope this helps,
- Kristian.
|
|
Hi @knielsen , Thank you for your input and for the detailed explanations.
I have updated the implementation accordingly. The InnoDB binlog is now treated as one continuous stream, and crossing an .ibb file boundary no longer triggers rotation of the output legacy binlog. The output is rotated only when the size limit is reached or we encounter a FDE in the InnoDB binlog.
I have also added the test case which tests the conversion for InnoDB binlog with out of band (OOB) data.
Alright, this makes sense. I have made the changes to not overwrite the Thank you again for the detailed feedback. Please let me know if I’ve missed anything. |
2a4767f to
31a9c1f
Compare
bnestere
left a comment
bnestere
left a comment
There was a problem hiding this comment.
Thanks @tarunw07 , approved for our 13.2 preview, though we might actually want this in 12.3 instead, so those who upgrade to our LTS can have this downgrade capability. But we have some time to decide that, and having in the 13.2 preview will allow us some time to formally test this too.
Please, before September 17th, squash all of your commits into a single one, which describes:
- What problem this solves
- How this feature solves the problem
- How to use this feature
- What errors may be expected (you may generalize here, as you add a lot of errors :))
- Any caveats
As well as anything else that may be useful. Commit messages are quite helpful for a lot of folks. Testers, users, managers, etc. Please write it with a wide audience in mind (ie not just developers).
c35b119 to
baa3f18
Compare
…legacy format
MariaDB 12.3 added support for storing binary logs inside InnoDB
tablespace files (.ibb) instead of traditional flat sequential files.
There was previously no way to convert these back to the legacy
format, which is needed for downgrades, migrations, and third-party
tooling that only understands classic binlogs.
This adds a --convert-engine-binlog option to mariadb-binlog that
reads one or more InnoDB-based binlogs and writes out equivalent
legacy-format binlog files.
Usage:
mariadb-binlog --convert-engine-binlog -r output_legacy_binlog \
input1.ibb input2.ibb
This produces output_legacy_binlog.0000001, .0000002, etc. Note
there isn't a direct correlation between the number of input files
and output files. The output is split whenever the current file
hits --max-binlog-size (default 1GB, since legacy binlogs cannot
exceed 4GB), and also on every server restart recorded in the
InnoDB binlog, so a single input file can produce several output
files and a restart boundary can force a split even before the
size limit is reached.
Additionally, --max-binlog-size is also supported as a standalone
option to control this rotation threshold directly.
Only --convert-engine-binlog, -r, and --max-binlog-size are
supported alongside this mode; any other option will cause
mariadb-binlog to throw an error.
Caveats:
- Encryption: InnoDB-based binlogs don't currently support
encryption, so the converter doesn't handle it either. Output
legacy binlogs are never encrypted by this mode.
- Checksums: InnoDB-based binlogs rely on page-level checksums
instead of the legacy per-event checksums, so the initial
implementation generates legacy output with no event-level
checksum.
baa3f18 to
8907c10
Compare
|
I have squashed all my commits into a single commit and pushed the latest changes with an updated commit message. Two CI checks are still failing, but I reviewed the logs and both failures appear to be unrelated to my changes. |
4 participants
Summary
This PR implements MDEV-37605: Convert InnoDB Binary Logs to Legacy Format.
The goal of this work is to extend
mariadb-binlogso that it can read InnoDB-based binary log files (.ibb) and generate legacy-format binary log files as output. This is intended to support compatibility use cases such as downgrades, migrations, and tooling that expects the legacy binlog format.The current approach is to avoid fully deserializing and reserializing regular events. Since most event contents are already encoded in the
.ibbfile, the converter should mostly copy event bytes into the generated legacy binlog, while handlingFORMAT_DESCRIPTION_EVENT,GTID_LIST_EVENT,BINLOG_CHECKPOINT_EVENT&ROTATE_EVENT.Current status
This PR currently includes:
--convert-engine-binlogFORMAT_DESCRIPTION_EVENThandlingGTID_LIST_EVENTFSP_BINLOG_TYPE_GTID_STATEfrom the first page of the.ibbfile to initialize the converter’s GTID state which is used forGTID_LIST_EVENT--max-binlog-size.ROTATE_EVENTBINLOG_CHECKPOINT_EVENTend_log_poscorrectly in generated legacy binlog events.ibbfile, converting it to legacy binlog format, and checking the converted outputTests included
--max-binlog-sizeis provided.Design notes
The current design direction is:
FORMAT_DESCRIPTION_EVENT,GTID_LIST_EVENT,BINLOG_CHECKPOINT_EVENT&ROTATE_EVENT) that must be synthesized for legacy binlog output.FSP_BINLOG_TYPE_GTID_STATEfrom the first page of the.ibbfile to initialize the converter’s GTID state.rpl_binlog_state_base, so thatGTID_LIST_EVENTcan be generated correctly at the beginning of each output legacy binlog file.BINLOG_CHECKPOINT_EVENTnear the beginning of each generated legacy binlog file.end_log_posfor all the generated legacy binlog events.