MDEV-37605 Extend mariadb-binlog to convert innodb based binary logs to legacy

[tarunw07]

Summary

This PR implements 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.

The current approach is to avoid fully deserializing and reserializing regular events. Since most event contents are already encoded in the .ibb file, the converter should mostly copy event bytes into the generated legacy binlog, while handling FORMAT_DESCRIPTION_EVENT, GTID_LIST_EVENT, BINLOG_CHECKPOINT_EVENT & ROTATE_EVENT.

Current status

This PR currently includes:

• New option: --convert-engine-binlog
• Initial legacy binlog writer/conversion path
• FORMAT_DESCRIPTION_EVENT handling
• Generate/write GTID_LIST_EVENT
• Use the initial FSP_BINLOG_TYPE_GTID_STATE from the first page of the .ibb file to initialize the converter’s GTID state which is used for GTID_LIST_EVENT
• Rotate the output binlog based on the size specified by --max-binlog-size.
• Handle ROTATE_EVENT
• Generate/write BINLOG_CHECKPOINT_EVENT
• Update end_log_pos correctly in generated legacy binlog events
• Test coverage for generating a .ibb file, converting it to legacy binlog format, and checking the converted output

Tests included

• Basic test: Converts a basic InnoDB binlog and checks that the converted file can be replayed correctly.
• Max-size test: Checks that the output binlog rotates when --max-binlog-size is provided.
• GTID list test: Checks that the right GTID state is added to each converted binlog.
• Replication test: Checks that a slave can replicate converted events.
• Unsupported-options test: Checks that unsupported options and legacy binlog inputs are rejected.

Design notes

The current design direction is:

• Copy regular events byte-by-byte where possible.
• Add serializer/generation logic for events(FORMAT_DESCRIPTION_EVENT, GTID_LIST_EVENT, BINLOG_CHECKPOINT_EVENT & ROTATE_EVENT) that must be synthesized for legacy binlog output.
• Use the initial FSP_BINLOG_TYPE_GTID_STATE from the first page of the .ibb file to initialize the converter’s GTID state.
• Maintain GTID state during conversion, using rpl_binlog_state_base, so that GTID_LIST_EVENT can be generated correctly at the beginning of each output legacy binlog file.
• Write a synthetic/dummy BINLOG_CHECKPOINT_EVENT near the beginning of each generated legacy binlog file.
• Update end_log_pos for all the generated legacy binlog events.
• Rotate output legacy binlogs when needed

[gemini-code-assist]

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.

[gemini-code-assist]

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);
  }

[gemini-code-assist]

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;
}

[gemini-code-assist]

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]

Having a separate rotate_output_legacy_binlog() that calls init_output_legacy_binlog() sounds better than shoving rotation inside “reïnitialization”, right?

[tarunw07]

Yes, thanks for your input. Your suggestion seems like a much better design, so I’ll go ahead and make these changes. Thanks! :)

[tarunw07]

@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.

[ParadoxV5]

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_dir
• binlog_in_engine.mariabackup_binlogs
• mariabackup_slave_provision.inc callers

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?

Suggested change

	--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.

[tarunw07]

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.

[ParadoxV5]

(answered in our fortnightly call)

[knielsen]

Some initial review comments on the draft patch in email comment.

[knielsen]

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.

[knielsen]

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

[knielsen]

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.

[knielsen]

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

[knielsen]

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

[knielsen]

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.

[ParadoxV5]

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/).

[tarunw07]

Hi Kristian,

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.

Thanks for the explanations. I'm fine with your original code in these two cases then. - Kristian.

Thanks for the clarification. That makes sense.

I will go ahead with my original code and proceed with remaining review changes.

Thanks,
Tarun

[bnestere]

Hi @tarunw07 !

This is looking very nice, it is so close to the finish line :) Thanks for all your hard work!

I've left a few notes, though overall, I'd like @knielsen to verify a few of my questions before you actually fix any of the bigger points I raise.

[bnestere]

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.

[bnestere]

After instantiating this, it should be validated (i.e.

Suggested change

	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())
	{
	...

[bnestere]

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,

[bnestere]

bracket should be on new line (and on the else at 4046)

[bnestere]

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?

[bnestere]

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).

[bnestere]

Hmm there is no mysqlbinlog_ocnvet_engine_binlog_basic-master.opt file, did you miss uploading it?

[bnestere]

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

[bnestere]

@tarunw07 ah a couple more quick comments:

• You have a lot of code comments like TODO: Tarun - these should be resolved now :) Please remove them
• Please look through your full patch series on the command line with git show - you'll see a number of places with extraneous spaces at the end of lines. Please remove these.

[knielsen]

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.