maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

"Michael Widenius (JIRA)" <jira@xxxxxxxxxxxxxxxxxxxxx> 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.