"Michael Widenius (JIRA)" <jira@mariadb.atlassian.net> writes:

Michael Widenius commented on MDEV-4937: ----------------------------------------

Thanks for review Monty! I have some comments inline, let me know if you still want me to change something. - Kristian.

revno: 4260 revision-id: knielsen at knielsen-hq.org-20140620104939-xdvn985cgmwy3odd committer: Kristian Nielsen <knielsen at knielsen-hq.org> timestamp: Fri 2014-06-20 12:49:39 +0200 message: MDEV-4937: sql_slave_skip_counter does not work with GTID

=== modified file 'sql/slave.cc' --- a/sql/slave.cc 2014-06-19 12:50:43 +0000 +++ b/sql/slave.cc 2014-06-20 10:49:39 +0000 @@ -3521,9 +3521,6 @@ static int exec_relay_log_event(THD* thd

if (opt_gtid_ignore_duplicates) { - serial_rgi->current_gtid.domain_id= gev->domain_id; - serial_rgi->current_gtid.server_id= gev->server_id; - serial_rgi->current_gtid.seq_no= gev->seq_no; int res= rpl_global_gtid_slave_state.check_duplicate_gtid (&serial_rgi->current_gtid, serial_rgi); if (res < 0)

What was the reason for removing the above assignments? Was it a bug in the old code ?

Not really a bug, however, just before this code we have: if (event_group_new_gtid(serial_rgi, gev)) and event_group_new_gtid() does the exact same assignments, so the ones I removed are completely redundant. I just happened to notice this redundancy, so I removed them...

(I have to ask as I am not 100% of what the old code did).

@@ -4594,16 +4598,27 @@ log '%s' at position %s, relay log '%s'

if (saved_skip && rli->slave_skip_counter == 0) { + String tmp; + if (mi->using_gtid != Master_info::USE_GTID_NO) + { + tmp.append(STRING_WITH_LEN(", GTID '")); + rpl_append_gtid_state(&tmp, false); + tmp.append(STRING_WITH_LEN("'; ")); + }

{noformat}

If you want to avoid checking if tmp.append() fails, you could create tmp with a static buffer.

char tmp_buff[MAX_GTID_LENGTH+10]; String tmp(tmp_buff, sizeof(tmp_buff), &my_charset_bin); tmp.length(0);

Right, hm. It does not really feel right to enforce a limit on the length; if the user uses a longer GTID position (which would be rather unusual), it might be confusing to have it silently cut. If the append() should fail, well, then it _will_ be cut, but then it's just an informational message, and that kind of out-of-memory is probably unlikely (and the error log will likely be filled up with other complaints about out-of-memory). But let me know if you still think I should change it. (If so - can you think of a place where MAX_GTID_LENGTH can be defined, that would be accessible to both client/mysqldump.c and sql/slave.cc? I could spot only my_global.h, which seems a bit too generic to contain GTID-related stuff...?)

if (saved_skip && rli->slave_skip_counter == 0) { String tmp;

saved_skip= 0; + saved_skip_gtid_pos.free(); }

If you free saved_skip_gtid_pos, it good to also free tmp!

Right ... but it should be freed automatically anyway, when tmp goes out of scope, right?

=== modified file 'sql/sys_vars.cc' --- a/sql/sys_vars.cc 2014-06-09 18:00:23 +0000 +++ b/sql/sys_vars.cc 2014-06-20 10:49:39 +0000 @@ -4287,11 +4287,6 @@ bool update_multi_source_variable(sys_va

static bool update_slave_skip_counter(sys_var *self, THD *thd, Master_info *mi) { - if (mi->using_gtid != Master_info::USE_GTID_NO) - { - my_error(ER_SLAVE_SKIP_NOT_IN_GTID, MYF(0)); - return true; - }

{noformat}

Shouldn't you also remove the error ER_SLAVE_SKIP_NOT_IN_GTID from the error file?

I think we cannot remove error codes from a GA release once introduced, right? So I left it, to not shift other error numbers. But let me know if I'm wrong, and I'll remove it. - Kristian.