Hi! >>>>> "Kristian" == Kristian Nielsen <[email protected]> writes:
Kristian> Michael Widenius <[email protected]> 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:[email protected] 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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
