Hi, Alexey! On Nov 27, Alexey Botchkov wrote:

revision-id: 2a181ea7b16dcbc6dd30af5a2396760261d7d500 (mariadb-10.1.8-81-g2a181ea) parent(s): 9d5c9379a6fd3662d4388cda177a3b1ea7b070cc committer: Alexey Botchkov timestamp: 2015-11-27 16:33:23 +0400 message:

MDEV-8713 Add continuous binary log backup to mysqlbinlog. diff --git a/client/client_priv.h b/client/client_priv.h index 656c8fc..44f0b21 100644 --- a/client/client_priv.h +++ b/client/client_priv.h @@ -92,6 +92,7 @@ enum options_client OPT_REPORT_PROGRESS, OPT_SKIP_ANNOTATE_ROWS_EVENTS, OPT_SSL_CRL, OPT_SSL_CRLPATH, + OPT_RAW_OUTPUT, OPT_WAIT_SERVER_ID, OPT_STOP_NEVER,

1. There's no need to create named constants for OPT_RAW_OUTPUT and OPT_WAIT_SERVER_ID - they are never used anywhere. Simply put 0 in the my_options structure instead. 2. OPT_STOP_NEVER is used in the switch, so it needs a named constant. but it still doesn't have to be in client_priv.h, it's not an option common to all clients. Define it in mysqlbinlog.cc (see how mysqltest.cc and mysql_upgrade.c do it)

OPT_MAX_CLIENT_OPTION /* should be always the last */ };

diff --git a/client/mysqlbinlog.cc b/client/mysqlbinlog.cc index 5b07232..a2509b7 100644 --- a/client/mysqlbinlog.cc +++ b/client/mysqlbinlog.cc @@ -96,6 +98,8 @@ static char* database= 0; static my_bool force_opt= 0, short_form= 0, remote_opt= 0; static my_bool debug_info_flag, debug_check_flag; static my_bool force_if_open_opt= 1; +static my_bool opt_raw_mode= 0, opt_stop_never= 0; +static int64 opt_stop_never_slave_server_id= -1;

longlong, not int64. Because 1) you've used GET_LL for it, and 2) we generally don't use int64 (it's only used in special cases to mean exactly "signed 64-bit integer").

static my_bool opt_verify_binlog_checksum= 1; static ulonglong offset = 0; static char* host = 0; @@ -1368,8 +1373,14 @@ static struct my_option my_options[] = {"read-from-remote-server", 'R', "Read binary logs from a MySQL server.", &remote_opt, &remote_opt, 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0}, - {"result-file", 'r', "Direct output to a given file.", 0, 0, 0, GET_STR, - REQUIRED_ARG, 0, 0, 0, 0, 0, 0}, + {"raw", OPT_RAW_OUTPUT, "Requires -R. Output raw binlog data instead of SQL " + "statements and write it to files corresponding to server logs.",

What does that mean? "write it to files corresponding to server logs"? Perhaps you could clarify the help text?

+ &opt_raw_mode, &opt_raw_mode, 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, + 0, 0}, + {"result-file", 'r', "Direct output to a given file. With --raw this is a " + "prefix for the file names.", + &result_file_name, &result_file_name, 0, GET_STR, REQUIRED_ARG, + 0, 0, 0, 0, 0, 0}, {"server-id", 0, "Extract only binlog entries created by the server having the given id.", &server_id, &server_id, 0, GET_ULONG, @@ -1418,6 +1429,16 @@ static struct my_option my_options[] = "(you should probably use quotes for your shell to set it properly).", &stop_datetime_str, &stop_datetime_str, 0, GET_STR_ALLOC, REQUIRED_ARG, 0, 0, 0, 0, 0, 0}, + {"stop-never", OPT_STOP_NEVER, "Wait for more data from the server " + "instead of stopping at the end of the last log. Implicitly sets " + "--to-last-log but instead of stopping at the end of the last log " + "it continues to wait till the server disconnects.",

I don't know why you needed to say the same thing twice. What about: Wait for more data from the server instead of stopping at the end of the last log. Implies --to-last-log.

+ &opt_stop_never, &opt_stop_never, 0, + GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0}, + {"stop-never-slave-server-id", OPT_WAIT_SERVER_ID, + "The slave server_id used for --read-from-remote-server --stop-never.", + &opt_stop_never_slave_server_id, &opt_stop_never_slave_server_id, 0, + GET_LL, REQUIRED_ARG, -1, -1, 0xFFFFFFFFLL, 0, 0, 0},

GET_LL, really? So, negative server_id's are allowed? Either do it like the existing --server-id option (GET_ULONG, 0 means "not set"), or, if you want to allow 0 as a valid server-id value, you can do, like case OPT_WAIT_SERVER_ID: if (argument == disabled_my_option) opt_stop_never_slave_server_id= -1; break; but really I wouldn't bother and just do like --server-id does.

{"stop-position", OPT_STOP_POSITION, "Stop reading the binlog at position N. Applies to the last binlog " "passed on the command line.", @@ -1701,6 +1718,9 @@ get_one_option(int optid, const struct my_option *opt __attribute__((unused)), print_version(); opt_version= 1; break; + case OPT_STOP_NEVER: + to_last_remote_log= 1; + break;

This is wrong, it allows someone to do mysqlbinlog --stop-never --to-last-remote-log=0 and you get incorrect configuration. Alternatively one can do mysqlbinlog --stop-never=1 --stop-never=0 and to_last_remote_log will be magically enabled. Heck, one can even do mysqlbinlog --stop-never=0 and that'll enable to_last_remote_log. The correct approach is to do if (opt_stop_never) to_last_remote_log= TRUE; and only *after* all options are read. Note that in this case you won't need OPT_STOP_NEVER constant either (see my comment earlier in this review)

case '?': usage(); opt_version= 1; @@ -1922,6 +1943,240 @@ static Exit_status check_master_version() }

+ + +static char out_file_name[FN_REFLEN + 1]; + +static Exit_status handle_event_raw_mode(PRINT_EVENT_INFO *print_event_info, + ulong *len, + const char* logname, uint logname_len) +{

I'm not reviewing the logic of handle_event_raw_mode, as far as I can see you've copied it from mysqlbinlog in MySQL. Did you use the latest version of mysqlbinlog or the one from the original patch? Use the latest one, it might have bugfixes.

+ const char *error_msg; + const unsigned char *read_pos= mysql->net.read_pos + 1; + Log_event_type type; + DBUG_ENTER("handle_event_raw_mode"); + DBUG_ASSERT(opt_raw_mode && remote_opt); + + type= (Log_event_type) read_pos[EVENT_TYPE_OFFSET]; + + if (type == HEARTBEAT_LOG_EVENT) + DBUG_RETURN(OK_CONTINUE); + + if (type == ROTATE_EVENT || type == FORMAT_DESCRIPTION_EVENT) + { + Log_event *ev; + if (!(ev= Log_event::read_log_event((const char*) read_pos , + *len - 1, &error_msg, + glob_description_event, + opt_verify_binlog_checksum))) + { + error("Could not construct annotate event object: %s", error_msg);

may be: "%s", type == ROTATE_EVENT ? "rotate":"annotate"

+ DBUG_RETURN(ERROR_STOP); + } + /* + If reading from a remote host, ensure the temp_buf for the + Log_event class is pointing to the incoming stream. + */ + ev->register_temp_buf((char *) read_pos, FALSE); + @@ -2000,113 +2264,23 @@ static Exit_status dump_remote_log_entries(PRINT_EVENT_INFO *print_event_info, break; // end of data DBUG_PRINT("info",( "len: %lu net->read_pos[5]: %d\n", len, net->read_pos[5])); - if (net->read_pos[5] == ANNOTATE_ROWS_EVENT) + if (opt_raw_mode) { - if (!(ev= read_remote_annotate_event(net->read_pos + 1, len - 1, - &error_msg))) - { - error("Could not construct annotate event object: %s", error_msg); - DBUG_RETURN(ERROR_STOP); - } + retval= handle_event_raw_mode(print_event_info, &len, + logname, logname_len); } else { - if (!(ev= Log_event::read_log_event((const char*) net->read_pos + 1 , - len - 1, &error_msg, - glob_description_event, - opt_verify_binlog_checksum))) - { - error("Could not construct log event object: %s", error_msg); - DBUG_RETURN(ERROR_STOP); - } - /* - If reading from a remote host, ensure the temp_buf for the - Log_event class is pointing to the incoming stream. - */ - ev->register_temp_buf((char *) net->read_pos + 1, FALSE); + retval= handle_event_text_mode(print_event_info, &len, + logname, logname_len, old_off); } - - Log_event_type type= ev->get_type_code(); - if (glob_description_event->binlog_version >= 3 || - (type != LOAD_EVENT && type != CREATE_FILE_EVENT)) + if (retval != OK_CONTINUE) { - /* - If this is a Rotate event, maybe it's the end of the requested binlog; - in this case we are done (stop transfer). - This is suitable for binlogs, not relay logs (but for now we don't read - relay logs remotely because the server is not able to do that). If one - day we read relay logs remotely, then we will have a problem with the - detection below: relay logs contain Rotate events which are about the - binlogs, so which would trigger the end-detection below. - */ - if (type == ROTATE_EVENT) - { - Rotate_log_event *rev= (Rotate_log_event *)ev; - /* - If this is a fake Rotate event, and not about our log, we can stop - transfer. If this a real Rotate event (so it's not about our log, - it's in our log describing the next log), we print it (because it's - part of our log) and then we will stop when we receive the fake one - soon. - */ - if (rev->when == 0) - { - if (!to_last_remote_log) - { - if ((rev->ident_len != logname_len) || - memcmp(rev->new_log_ident, logname, logname_len)) - { - DBUG_RETURN(OK_CONTINUE); - } - /* - Otherwise, this is a fake Rotate for our log, at the very - beginning for sure. Skip it, because it was not in the original - log. If we are running with to_last_remote_log, we print it, - because it serves as a useful marker between binlogs then. - */ - continue; - } - len= 1; // fake Rotate, so don't increment old_off - } - } - else if (type == FORMAT_DESCRIPTION_EVENT) - { - /* - This could be an fake Format_description_log_event that server - (5.0+) automatically sends to a slave on connect, before sending - a first event at the requested position. If this is the case, - don't increment old_off. Real Format_description_log_event always - starts from BIN_LOG_HEADER_SIZE position. - */ - if (old_off != BIN_LOG_HEADER_SIZE) - len= 1; // fake event, don't increment old_off - } - Exit_status retval= process_event(print_event_info, ev, old_off, logname); - if (retval != OK_CONTINUE) - DBUG_RETURN(retval); + if (retval == OK_EOF) + break; + DBUG_RETURN(retval); } - else - { - Load_log_event *le= (Load_log_event*)ev; - const char *old_fname= le->fname; - uint old_len= le->fname_len; - File file; - Exit_status retval;

- if ((file= load_processor.prepare_new_file_for_old_format(le,fname)) < 0) - DBUG_RETURN(ERROR_STOP); - - retval= process_event(print_event_info, ev, old_off, logname); - if (retval != OK_CONTINUE) - { - my_close(file,MYF(MY_WME)); - DBUG_RETURN(retval); - } - retval= load_processor.load_old_format_file(net,old_fname,old_len,file); - my_close(file,MYF(MY_WME)); - if (retval != OK_CONTINUE) - DBUG_RETURN(retval); - }

Hmm, when you moved all that code above into handle_event_text_mode(), you seem to have changed the behavior for to_last_remote_log. Was (comments removed for brevity): if (!to_last_remote_log) { if ((rev->ident_len != logname_len) || memcmp(rev->new_log_ident, logname, logname_len)) { DBUG_RETURN(OK_CONTINUE); } continue; } Now instead of 'continue' you return OK_CONTINUE from handle_event_text_mode() and it does not skip the code below (after the comment Let's adjust), like the old 'continue' did.

/* Let's adjust offset for remote log as for local log to produce similar text and to have --stop-position to work identically.

Regards, Sergei Chief Architect MariaDB and security@mariadb.org