maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Kristian" == Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx> writes:

Kristian> Michael Widenius <monty@xxxxxxxxxxxx> writes:
>> I have now got the first version of multi source patch ported to
>> MariadB.

Kristian> As requested, I did a quick read through of the patch. I took the patch from
Kristian> your 10.0-mdev253 tree, this revision:

Kristian>     revid:monty@xxxxxxxxxxxx-20120930233044-e0m6u6t9q3jeqx03

Kristian> Below are my comments. Overall, it was nice to see the relatively modest
Kristian> changes needed to get this working.

Kristian>  - Kristian.

Kristian> -----------------------------------------------------------------------

>> === modified file 'sql/sys_vars.cc'
>> --- sql/sys_vars.cc	2012-09-22 12:30:24 +0000
>> +++ sql/sys_vars.cc	2012-10-01 09:55:23 +0000

>> +static Sys_var_multi_source_ulong
>> +Sys_slave_skip_counter("sql_slave_skip_counter",
>> +                       "Skip the next N events from the master log",
>> +                       SESSION_VAR(slave_skip_counter),
>> +                       NO_CMD_LINE,
>> +                       offsetof(Master_info, rli.slave_skip_counter),

Kristian> This use of offsetof() on a C++ class (non-POD) is incorrect. Even if it works
Kristian> for you with your current version of GCC and current compiler options, it is
Kristian> likely to break at some point on some future compiler. And it will be hell to
Kristian> track down.

Kristian> Look, I understand that you do not like some of the ways that the C++ language
Kristian> is defined, like here that offsetof() is not allowed on non-POD data
Kristian> structures. But we have to live with it. Surely, it is better to have a
Kristian> standard language that compiler writers and compiler users agree on how works
Kristian> - even if that language is less convenient than what we would like.

Kristian> So let us please change this to be correct C++. I can change it for you if you
Kristian> want.

Yes, please suggest/push a fix. I tried but couldn't come up with
anything efficient, short to write or that didn't require a function
for every offset.

I think this is safe as long as Sys_vars doesn't have any virtual
functions. According to gcc manual it looks like future gcc version
will support offsetof() on base C++ classes.

>> === modified file 'sql/slave.cc'
>> --- sql/slave.cc	2012-09-13 12:31:29 +0000
>> +++ sql/slave.cc	2012-10-01 09:55:23 +0000

>> +bool show_all_master_info(THD* thd)
>> +{

<cut>

>> +  for (i= 0; i < elements; i++)
>> +  {
>> +    tmp[i]= (Master_info *) my_hash_element(&master_info_index->
>> +                                            master_info_hash, i);
>> +  }

Kristian> Isn't there a race condition here? What happens if the number of elements
Kristian> changes between looking up number of elements and the loop?

It can't do that because it's protected by LOCK_active_mi.
(It's only under this lock the hash can change).

Kristian> If there is a lock protecting this, please add a safe_mutex_assert_owner()
Kristian> and/or a comment.

I have added safe_mutex and comment

Kristian> The call of show_all_master_info() is like this:

Kristian>    mysql_mutex_lock(&LOCK_active_mi);
Kristian>    if (lex->verbose)
Kristian>      res= show_all_master_info(thd);

Kristian> Does LOCK_active_mi protect all changes to multi-master state? If so, I think
Kristian> it should be renamed - "active_mi" is rather confusing in this case.

Yes, but it has to be done in a separate patch as some point.


>> === modified file 'sql/sql_class.h'
>> --- sql/sql_class.h	2012-09-22 14:11:40 +0000
>> +++ sql/sql_class.h	2012-10-01 09:55:23 +0000

>> +  /**
>> +     Place holders to store Multi-source variables in sys_var.cc during
>> +     update and show of variables.
>> +  */
>> +  ulong slave_skip_counter;
>> +  ulong max_relay_log_size;

>> +  /* Names. These will be allocated in buffers in thd */
>> +  LEX_STRING default_master_connection;
>> +

>> +  LEX_STRING connection_name;                   /* If slave */
>> +  char       default_master_connection_buff[MAX_CONNECTION_NAME+1];

Kristian> Is there not some other way to do this?

Kristian> THD seems to have become some kind of garbage dump for all kinds of
Kristian> special-purpose data. This is not good, it pollutes the CPU caches for all
Kristian> threads, and generally makes everything a mess :-/

There was no easier way I could to fix the issue with the variables.
(because of how set_var.cc is constructed)

default_master_connection is needed in THD, as this is specific for
this THD.  connection_name is just a pointer to make a lot of code
easier and safer.

Kristian> I would like to see us move towards having less things in THD that are not
Kristian> relevant to all threads, not more.

Kristian> Could we add a single void * somewhere in THD for special-purpose threads?
Kristian> And then replication threads could use that for their special-purpose stuff.
Kristian> Or maybe sub-class THD.

Kristian> Then later we could start moving other stuff out of THD that is not relevant
Kristian> to all threads...

I am happy to look at patch from you for the above ;)


>> === modified file 'sql/rpl_rli.h'
>> --- sql/rpl_rli.h	2012-08-27 16:13:17 +0000
>> +++ sql/rpl_rli.h	2012-10-01 09:55:23 +0000

>> +  volatile ulong slave_skip_counter;    /* Must be ulong */

Kristian> Such comments are extremely unhelpful. Instead explain why it must be ulong.

Fixed.

>> === modified file 'sql/item_create.cc'
>> --- sql/item_create.cc	2012-01-13 14:50:02 +0000
>> +++ sql/item_create.cc	2012-10-01 09:55:23 +0000
>> @@ -4393,27 +4393,36 @@ Create_func_master_pos_wait::create_nati

>> +  thd->lex->safe_to_cache_query= 0;

>> case 3:

thd-> lex->safe_to_cache_query= 0;

>> +  case 4:

>> +    thd->lex->safe_to_cache_query= 0;

Kristian> Isn't it redundant to set safe_to_cache_query twice here?

That come from me reorganziation the code. I forgot to remove the
above that was needed in the old code.

>> === modified file 'sql/rpl_mi.cc'
>> --- sql/rpl_mi.cc	2012-03-27 23:04:46 +0000
>> +++ sql/rpl_mi.cc	2012-10-01 09:55:23 +0000

>> +  /* Store connection name and lower case connection name */
>> +  connection_name.length= cmp_connection_name.length=
>> +    connection_name_arg->length;
>> +  if ((connection_name.str= (char*) my_malloc(connection_name_arg->length*2+2,
>> +                                              MYF(MY_WME))))
>> +  {
>> +    cmp_connection_name.str= (connection_name.str +
>> +                              connection_name_arg->length+1);
>> +    strmake(connection_name.str, connection_name_arg->str,
>> +            connection_name.length);
>> +    memcpy(cmp_connection_name.str, connection_name_arg->str,
>> +           connection_name.length+1);
>> +    my_casedn_str(system_charset_info, cmp_connection_name.str);
>> +  }

Kristian> Is it really correct to ignore out-of-memory error here, and just do nothing?

Kristian> Ah, I think this is handled by checking Master_info::error() in other places?
Kristian> If so, then please add a comment here explaining this.

Yes, it's handled. Comment added.


>> +/**
>> +   Create a log file with a signed suffix.

Kristian> I do not know what is meant with the term "signed suffix". This is not
Kristian> something that people will understand.

'signed' was a typo left from the old code.

However the comment for the function should explain this fully.

Kristian> Does it mean "Create a log file name by appending a dash "-" followed by a
Kristian> suffix" ?

Kristian> Also rename the file create_signed_file_name() - again, the "signed" is
Kristian> confusing. For example "create_augmented_file_name", or
Kristian> "create_name_with_dash_and_suffix" or something.

augmented is even more confusing for me...

I am going with 'create_logfile_name_with_suffix'


>> +   It's valid if it's a valid system name, is less than
>> +   MAX_CONNECTION_NAME.

Kristian> "It is valid if it is a valid system name of length less than
Kristian> MAX_CONNECTION_NAME".

fixed.


>> === modified file 'client/mysqltest.cc'
>> --- client/mysqltest.cc	2012-08-31 21:54:54 +0000
>> +++ client/mysqltest.cc	2012-10-01 09:55:23 +0000

>> +  if (buff)
>> +    my_free(start);

Kristian> Better use my_free(buff) here.

Doesn't work. (different variables)

>> === modified file 'sql/rpl_mi.h'
>> --- sql/rpl_mi.h	2012-03-27 23:04:46 +0000
>> +++ sql/rpl_mi.h	2012-10-01 09:55:23 +0000

>> +  char master_log_name[FN_REFLEN+6]; /* Place for multi-*/

Kristian> "place for" -> "room for".

Fixed

>> === modified file 'sql/mysqld.cc'
>> --- sql/mysqld.cc	2012-09-22 14:11:40 +0000
>> +++ sql/mysqld.cc	2012-10-01 09:55:23 +0000

>> +  /*
>> +    We must have LOCK_open before LOCK_global_system_variables because
>> +    LOCK_open is hold while sql_plugin.c::intern_sys_var_ptr() is called.

Kristian> "is hold" -> "is held"

Fixed

>> === modified file 'sql/item_create.cc'
>> --- sql/item_create.cc	2012-01-13 14:50:02 +0000
>> +++ sql/item_create.cc	2012-10-01 09:55:23 +0000
>> @@ -4393,27 +4393,36 @@ Create_func_master_pos_wait::create_nati
>> if (item_list != NULL)
>> arg_count= item_list->elements;
>> 
>> +  if (arg_count < 2 || arg_count >4)

Kristian> Missing space before "4"

Fixed.

Thanks!

Regards,
Monty