Re: bk commit into 6.0 tree (aelkin:1.2634) BUG#25998

[Andrei Elkin]

Mats, hello.

> Hi Andrei!
>
> Patch is a lot better, but I still have comments on it. There are no
> fundamental issues this time, but rather details about the implementation.

I understand. This fix is a WL size of work and some efforts had to be
spend on HLD.

I am accepting your comment on using the standard function.
Please find inlined my answers to your other notes as well.



>
> Just my few cents,
> Mats Kindahl
>
> Andrei Elkin wrote:
>> Below is the list of changes that have just been committed into a local
>> 6.0 repository of aelkin.  When aelkin does a push these changes
>> will be propagated to the main repository and, within 24 hours after the
>> push, to the public repository.
>> For information on how to access the public repository
>> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>> 
>> ChangeSet@stripped, 2008-06-03 14:36:57+03:00, aelkin@stripped +12 -0
>>   Bug #25998 problems about circle replication
>>   Bug #27808 Infinite looping in circular replication
>>   
>>   In case of withdrawing one of the servers from the circular multi-master
> replication group
>>   events generated by the removed server could become unstoppable (bug#25998).
>>   That's because the originator had been the terminator of the own event flow.
>>   
>>   Other possibility of the unstoppable event is the cluster replication
> (bug#27808).
>>   In that case an event generated by a member of a cluster was
>>   replicated to another member, got accepted and executed.
>>   By that same time effects of the event had been already propagated
>>   across the cluster via the cluster communications.
>>   In order to avoid double-applying, a replication event generated 
>>   by a co-member of the cluster should not be accepted.
>>   
>>   Both variations of the unstoppable replication event are fixable with 
>>   introducing a new option for CHANGE MASTER: 
>>   
>>   IGNORE_SERVER_IDS= (sid_1, sid_2, ... )
>>   
>>   The option can be set to the empty list that resets.
>>   
>>   Fixed with implementing the feature.
>>   
>>   Properties of the feature:
>>   
>>    a. reporting an error if the id of an ignored server is the slave itself and
>>       its configuration on startup was with --replicate-same-server-id;
>>    b. overriding the existing IGNORE_SERVER_IDS list by the following 
>>       CHANGE MASTER ... IGNORE_SERVER_IDS= (list), the empty list arg nullifies
>>       the current ignored list;
>>    c. preserving the existing list by CHANGE MASTER w/o IGNORE_SERVER_ID;
>
> Typo: IGNORE_SERVER_ID --> IGNORE_SERVER_IDS

ok

>
>>    d. preserving the ignored server ids after RESET SLAVE;
>>    e. extending SHOW SLAVE STATUS with a new line listing ignored servers;
>
> Do you mention that the list is saved to master.info as well?
>

good point, be added.


>>    f. Differently from --replicate-same-server-id handling, the sql thread is
> not
>>       concerned with the ignored server ids, because it's supposed that
>>       the relay log constists only of events that can not be unstoppable.
>
> Typo: constists --> consists

ok. I can not spell check in bk citool ... Hope for bright future with
baazar :-).

>
>>       In order to guarantee that, e.g in case of the circular replication with a
> failing
>>       server DBA needs to change master necessarily using the new option.
>>    g. Rotate and FD events originated by the current master listed
>>       in the ignored list are still relay-logged which does not create
>>       any termination issue.
>>    h. The possible list of ignored servers is sorted for the fastest processing
> of filtering
>>       algorithm.
>>   
>>   Two new lines to show slave status output are added: the list of ignored
> servers and
>>   the current master server id (yet another feature for the user!).
>
> Normally, we should not add features in bug-fixes, but this is a minor
> one, so I'll accept it.

I hoped on that understanding, i has been mutual so far. thanks!

>
>>   
>>   Use cases for this feature can be found on the bug report page.
>
> Make sure that all information about the new features are available for
> Jon so that it can be documented correctly.
>

Sure.

>> 
>>   mysql-test/include/master-slave.inc@stripped, 2008-06-03 14:36:55+03:00,
> aelkin@stripped +2 -0
>>     forcing this basic macro to synchronize the slave right after it has
> connected
>>     to the master. Tests get the guarantee of synchronization at the beginning.
>> 
>>   mysql-test/suite/rpl/r/rpl_server_id_ignore.result@stripped, 2008-06-03
> 14:36:55+03:00, aelkin@stripped +46 -0
>>     new results
>> 
>>   mysql-test/suite/rpl/r/rpl_server_id_ignore.result@stripped, 2008-06-03
> 14:36:55+03:00, aelkin@stripped +0 -0
>> 
>>   mysql-test/suite/rpl/t/rpl_server_id_ignore-slave.opt@stripped, 2008-06-03
> 14:36:55+03:00, aelkin@stripped +1 -0
>>     --replicate-same-server-id is set in order to show reaction on its possible
> clashing with
>>     the new gnored_server_id:s option of change master.
>> 
>>   mysql-test/suite/rpl/t/rpl_server_id_ignore-slave.opt@stripped, 2008-06-03
> 14:36:55+03:00, aelkin@stripped +0 -0
>> 
>>   mysql-test/suite/rpl/t/rpl_server_id_ignore.test@stripped, 2008-06-03
> 14:36:55+03:00, aelkin@stripped +114 -0
>>     The basic tests for the new feature.
>> 
>>   mysql-test/suite/rpl/t/rpl_server_id_ignore.test@stripped, 2008-06-03
> 14:36:55+03:00, aelkin@stripped +0 -0
>> 
>>   sql/lex.h@stripped, 2008-06-03 14:36:55+03:00, aelkin@stripped +1 -0
>>     IGNORE_SERVER_ID new option for change master
>> 
>>   sql/rpl_mi.cc@stripped, 2008-06-03 14:36:55+03:00, aelkin@stripped
> +105 -4
>>     allocation, storing into master info, reading from the file, deallocation
>>     of the list of ignored server id:s;
>>     a new dynamic_array_ulong_bin_search() search function definition in ulong
> item dyn array,
>>     a new Master_info::repl_server_id_ignore_p() method definition, calling it to
> find out
>>     if a server_id should be ignored;
>>     a formal initialization of a new member Master_info::master_id;
>>     extending the number of lines in master.info.
>> 
>>   sql/rpl_mi.h@stripped, 2008-06-03 14:36:55+03:00, aelkin@stripped +4
> -0
>>     a new method Master_info::repl_server_id_ignore_p answering filtering
> question;
>>     a new member Master_info::ignore_server_ids to hold the list of ignored
> servers;
>>     a new member Master_info::master_id to hold the current master server id.
>> 
>>   sql/share/errmsg.txt@stripped, 2008-06-03 14:36:55+03:00,
> aelkin@stripped +2 -0
>>     a new error message about possible clashing of options.
>> 
>>   sql/slave.cc@stripped, 2008-06-03 14:36:55+03:00, aelkin@stripped
> +151 -11
>>     a new init_dynarray_intvar_from_file() helper function to read an array of
> variable size 
>>     of intergers.
>>     Refining io-thread's filtering condition to consult with the list of ignored
> servers.
>>     Note, that FD and Rotate events from an ignored server being the current
> master 
>>     are still accepted.
>>     Filtering basing on --replicate-same-server-id remains as before if the
> ignored servers
>>     list is empty.
>>     Adding two new lines in show slave status.
>>     Initialization of the new member Master_info::master_id in
> get_master_version_and_clock().
>>     Displaying the ignored servers and the current master id with show slave
> status.
>> 
>>   sql/sql_lex.h@stripped, 2008-06-03 14:36:55+03:00, aelkin@stripped +2
> -1
>>     parser time storage for variable size array of server ids;
>>     a new repl_ignore_server_ids_opt member for LEX_MASTER_INFO;
>>      
>>      
>> 
>>   sql/sql_repl.cc@stripped, 2008-06-03 14:36:55+03:00, aelkin@stripped
> +46 -13
>>     aux function for sorting the ignored server_id:s;
>>     sorting of the ignored servers implementation;
>>     failing change master do not forget to free lex's dynamical storage with
> parsed
>>     ignored server_ids;
>>     resetting the old list of ignored server if the parser detected the new
> list;
>>     shifting some pieces of code in order to comply with the above logics;
>> 
>>   sql/sql_yacc.yy@stripped, 2008-06-03 14:36:55+03:00, aelkin@stripped
> +25 -0
>>     allocation a dyn array for possible list of ignored server ids;
>>     filling the array;
>>     resetting (missed part of wl#342) heartbeat_opt at the beginning of
>>     CHANGE MASTER processing;
>> 
>> diff -Nrup a/mysql-test/include/master-slave.inc
> b/mysql-test/include/master-slave.inc
>> --- a/mysql-test/include/master-slave.inc	2007-06-07 21:25:46 +03:00
>> +++ b/mysql-test/include/master-slave.inc	2008-06-03 14:36:55 +03:00
>> @@ -8,5 +8,7 @@ connect (slave1,127.0.0.1,root,,test,$SL
>>  
>>  -- source include/master-slave-reset.inc
>>  
>> +connection master;
>> +sync_slave_with_master;
>
> Thank you!

not at all.

>
>>  # Set the default connection to 'master'
>>  connection master;
>> diff -Nrup a/mysql-test/suite/rpl/r/rpl_server_id_ignore.result
> b/mysql-test/suite/rpl/r/rpl_server_id_ignore.result
>> --- /dev/null	Wed Dec 31 16:00:00 196900
>> +++ b/mysql-test/suite/rpl/r/rpl_server_id_ignore.result	2008-06-03 14:36:55
> +03:00
>> @@ -0,0 +1,46 @@
>> +stop slave;
>> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
>> +reset master;
>> +reset slave;
>> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
>> +start slave;
>> +master_id: 1
>> +stop slave;
>> +*** --replicate-same-server-id and change master option can clash ***
>> +change master to IGNORE_SERVER_IDS= (2, 1);
>> +ERROR HY000: The requested server id 2 clashes with the slave startup option
> --replicate-same-server-id
>> +*** must be empty due to the error ***
>> +ignore server id list: 
>
> OK
>
>> +change master to IGNORE_SERVER_IDS= (10, 100);
>> +*** must be 10, 100 ***
>> +ignore server id list: 10, 100
>
> OK
>
>> +reset slave;
>> +*** must be empty due to reset slave ***
>> +ignore server id list: 10, 100
>> +change master to IGNORE_SERVER_IDS= (10, 100);
>
> Comment here is not correct: the list is not empty
>

Sigh, you must have reviewed not the latest commit...

   http://lists.mysql.com/commits/47357 has

   +change master to IGNORE_SERVER_IDS= (10, 100);
   +*** the list must remain (10, 100) after reset slave ***
   +change master to IGNORE_SERVER_IDS= ();
   +*** must be empty due to IGNORE_SERVER_IDS empty list ***


>> +*** CHANGE MASTER with IGNORE_SERVER_IDS option overrides (does not increment)
> the previous setup ***
>> +change master to IGNORE_SERVER_IDS= (1, 3, 4);
>> +*** must be 1, 3, 4 due to overriding policy ***
>> +ignore server id list: 1, 3, 4
>
> OK
>
>> +*** ignore master (server 1) queries for a while ***
>> +start slave;
>> +create table t1 (n int);
>> +*** must be empty as the event is to be filtered out ***
>> +show tables;
>> +Tables_in_test
>
> OK
>
>> +*** allowing events from master ***
>> +stop slave;
>> +reset slave;
>> +change master to IGNORE_SERVER_IDS= (10, 100);
>> +*** the list must remain (10, 100) after reset slave ***
>
> Where is the check that list is the same?
>

It's in the latest commit.

>> +change master to IGNORE_SERVER_IDS= ();
>
> Normally, when one tests a function with lists, usually lengths 0, 1, 2,
> and 3 are tested. That means that the most problematic cases are tested,
> i.e.:
>
> - there is an empty list
>
> - there is a list where an element is both first and last
>
> - there is a list where an element is first and one element is last,
>   but they are not the same element
>
> - there is a case where an element is neither first nor last
>
>> +*** must be empty due to IGNORE_SERVER_IDS empty list ***
>> +ignore server id list: 

It's easy to add what you are asking. Still, i think such testing
would be for the parser's basic funcionality. The patch does not
introduce any new list operation explicitly.

>
> OK
>
>> +change master to master_host='127.0.0.1', master_port=MASTER_PORT,
> master_user='root';
>> +start slave;
>> +*** must have caught create table ***
>> +show tables;
>> +Tables_in_test
>> +t1
>
> OK.
>
> I don't see a test for a list with duplicate server id:s, for example,
> (1, 1, 1, 3, 5). Could you please add such a case and show what is the
> result of that setting.
>
> Also, what about giving server id's in random order, e.g.: (4, 2, 1, 5)?

sure

>
>> +drop table t1;
>> +end of the tests
>> diff -Nrup a/mysql-test/suite/rpl/t/rpl_server_id_ignore-slave.opt
> b/mysql-test/suite/rpl/t/rpl_server_id_ignore-slave.opt
>> --- /dev/null	Wed Dec 31 16:00:00 196900
>> +++ b/mysql-test/suite/rpl/t/rpl_server_id_ignore-slave.opt	2008-06-03 14:36:55
> +03:00
>> @@ -0,0 +1 @@
>> +--disable-log-slave-updates --replicate-same-server-id
>> diff -Nrup a/mysql-test/suite/rpl/t/rpl_server_id_ignore.test
> b/mysql-test/suite/rpl/t/rpl_server_id_ignore.test
>> --- /dev/null	Wed Dec 31 16:00:00 196900
>> +++ b/mysql-test/suite/rpl/t/rpl_server_id_ignore.test	2008-06-03 14:36:55
> +03:00
>> @@ -0,0 +1,114 @@
>> +# This test checks that the slave rejects events originating
>> +# by a server from the list of ignored originators (bug#27808 etc)
>> +#
>> +# phases of tests:
>> +#
>> +# 0. master_id new line in show slave status
>> +# 1. syntax related: 
>> +#    a. error reporting if options clash;
>> +#    b. overriding the old IGNORE_SERVER_IDS setup by the following 
>> +#       CHANGE MASTER ... IGNORE_SERVER_IDS= (list);
>> +#    c. the old setup preserving by CHANGE MASTER w/o IGNORE_SERVER_IDS
>> +#    d. resetting the ignored server ids with the empty list arg to 
>> +#       IGNORE_SERVER_IDS=()
>> +#    e. RESET SLAVE preserves the list
>> +# 2. run time related: 
>> +#    a. observing no processing events from a master listed in
> IGNORE_SERVER_IDS
>> +#    b. nullifying the list and resuming of taking binlog from the very
> beginning with
>> +#       executing events this time
>> +
>> +source include/master-slave.inc;
>> +
>> +connection slave;
>> +
>> +# a new line for master_id
>> +let $master_id= query_get_value(SHOW SLAVE STATUS, Master_Server_Id, 1);
>> +echo master_id: $master_id;
>> +
>> +stop slave;
>> +--echo *** --replicate-same-server-id and change master option can clash ***
>> +--error ER_SLAVE_IGNORE_SERVER_IDS
>> +change master to IGNORE_SERVER_IDS= (2, 1);
>> +--echo *** must be empty due to the error ***
>> +let $ignore_list= query_get_value(SHOW SLAVE STATUS,
> Replicate_Ignore_Server_Ids, 1);
>> +echo ignore server id list: $ignore_list;
>> +
>> +change master to IGNORE_SERVER_IDS= (10, 100);
>> +--echo *** must be 10, 100 ***
>> +let $ignore_list= query_get_value(SHOW SLAVE STATUS,
> Replicate_Ignore_Server_Ids, 1);
>> +echo ignore server id list: $ignore_list;
>> +reset slave;
>> +--echo *** must be empty due to reset slave ***
>> +let $ignore_list= query_get_value(SHOW SLAVE STATUS,
> Replicate_Ignore_Server_Ids, 1);
>> +echo ignore server id list: $ignore_list;
>> +change master to IGNORE_SERVER_IDS= (10, 100);
>> +--echo *** CHANGE MASTER with IGNORE_SERVER_IDS option overrides (does not
> increment) the previous setup ***
>> +change master to IGNORE_SERVER_IDS= (1, 3, 4);
>> +--echo *** must be 1, 3, 4 due to overriding policy ***
>> +let $ignore_list= query_get_value(SHOW SLAVE STATUS,
> Replicate_Ignore_Server_Ids, 1);
>> +echo ignore server id list: $ignore_list;
>> +--echo *** ignore master (server 1) queries for a while ***
>> +start slave;
>> +
>> +connection master;
>> +
>> +#connection slave;
>> +sync_slave_with_master;
>> +let $slave_relay_pos0= query_get_value(SHOW SLAVE STATUS, Relay_Log_Pos, 1);
>> +
>> +connection master;
>> +create table t1 (n int);
>> +let $master_binlog_end= query_get_value(SHOW MASTER STATUS, Position, 1);
>> +
>> +connection slave;
>> +let $slave_param= Exec_Master_Log_Pos;
>> +let $slave_param_value= $master_binlog_end;
>> +source include/wait_for_slave_param.inc;
>> +--echo *** must be empty as the event is to be filtered out ***
>> +show tables;
>> +--echo *** allowing events from master ***
>> +let $slave_relay_pos1= query_get_value(SHOW SLAVE STATUS, Relay_Log_Pos, 1);
>> +#
>> +# checking stability of relay log pos
>> +#
>> +if (`select $slave_relay_pos1 - $slave_relay_pos0`)
>> +{
>> +    --echo Error: relay log position changed:  $slave_relay_pos0,
> $slave_relay_pos1
>> +    query_vertical show slave status;
>> +}
>> +
>> +stop slave;
>> +source include/wait_for_slave_to_stop.inc;
>> +reset slave;
>> +change master to IGNORE_SERVER_IDS= (10, 100);
>> +--echo *** the list must remain (10, 100) after reset slave ***
>> +let $ignore_list= query_get_value(SHOW SLAVE STATUS,
> Replicate_Ignore_Server_Ids, 1);
>> +
>> +change master to IGNORE_SERVER_IDS= ();
>> +--echo *** must be empty due to IGNORE_SERVER_IDS empty list ***
>> +let $ignore_list= query_get_value(SHOW SLAVE STATUS,
> Replicate_Ignore_Server_Ids, 1);
>> +echo ignore server id list: $ignore_list;
>> +--replace_result $MASTER_MYPORT MASTER_PORT
>> +eval change master to master_host='127.0.0.1', master_port=$MASTER_MYPORT,
> master_user='root';
>> +start slave;
>> +
>> +connection master;
>> +
>> +#connection slave;
>> +sync_slave_with_master;
>> +--echo *** must have caught create table ***
>> +show tables;
>> +
>> +# cleanup
>> +connection master;
>> +drop table t1;
>> +#connection slave
>> +sync_slave_with_master;
>> +
>> +--echo end of the tests
>> +
>> +
>> +
>> +
>> +
>> +
>> diff -Nrup a/sql/lex.h b/sql/lex.h
>> --- a/sql/lex.h	2008-04-01 13:56:38 +03:00
>> +++ b/sql/lex.h	2008-06-03 14:36:55 +03:00
>> @@ -243,6 +243,7 @@ static SYMBOL symbols[] = {
>>    { "IDENTIFIED",	SYM(IDENTIFIED_SYM)},
>>    { "IF",		SYM(IF)},
>>    { "IGNORE",		SYM(IGNORE_SYM)},
>> +  { "IGNORE_SERVER_IDS", SYM(IGNORE_SERVER_IDS_SYM)},
>>    { "IMPORT",		SYM(IMPORT)},
>>    { "IN",		SYM(IN_SYM)},
>>    { "INDEX",		SYM(INDEX_SYM)},
>> diff -Nrup a/sql/rpl_mi.cc b/sql/rpl_mi.cc
>> --- a/sql/rpl_mi.cc	2008-03-15 00:21:24 +02:00
>> +++ b/sql/rpl_mi.cc	2008-06-03 14:36:55 +03:00
>> @@ -28,17 +28,19 @@ int init_intvar_from_file(int* var, IO_C
>>  int init_strvar_from_file(char *var, int max_size, IO_CACHE *f,
>>  			  const char *default_val);
>>  int init_floatvar_from_file(float* var, IO_CACHE* f, float default_val);
>> +int init_dynarray_intvar_from_file(DYNAMIC_ARRAY* arr, IO_CACHE* f);
>>  
>>  Master_info::Master_info()
>>    :Slave_reporting_capability("I/O"),
>>     ssl(0), ssl_verify_server_cert(0), fd(-1),  io_thd(0), port(MYSQL_PORT),
>>     connect_retry(DEFAULT_CONNECT_RETRY), heartbeat_period(0),
>> -   received_heartbeats(0), inited(0),
>> +   received_heartbeats(0), inited(0), master_id(0),
>>     abort_slave(0), slave_running(0), slave_run_id(0)
>>  {
>>    host[0] = 0; user[0] = 0; password[0] = 0;
>>    ssl_ca[0]= 0; ssl_capath[0]= 0; ssl_cert[0]= 0;
>>    ssl_cipher[0]= 0; ssl_key[0]= 0;
>> +  my_init_dynamic_array(&ignore_server_ids, sizeof(::server_id), 16, 16);
>>  
>>    bzero((char*) &file, sizeof(file));
>>    pthread_mutex_init(&run_lock, MY_MUTEX_INIT_FAST);
>> @@ -50,6 +52,7 @@ Master_info::Master_info()
>>  
>>  Master_info::~Master_info()
>>  {
>> +  delete_dynamic(&ignore_server_ids);
>>    pthread_mutex_destroy(&run_lock);
>>    pthread_mutex_destroy(&data_lock);
>>    pthread_cond_destroy(&data_cond);
>> @@ -58,6 +61,65 @@ Master_info::~Master_info()
>>  }
>>  
>>  
>> +/**
>> +   Binary search in a specific type of the array having ulong items.
>> +   The items are ascendant-sorted.
>> +
>> +   @param A             pointer to an array
>> +   @param search        pointer to a search pattern
>> +   @param index[in,out] pointer to a store for the index 
>> +                        of a found element or NULL - not store
>> +   @return        
>> +      @retval FALSE  if the pattern is found,
>> +      @retval TRUE    otherwise
>> + */
>> +
>> +my_bool dynamic_array_ulong_bin_search(DYNAMIC_ARRAY *array, const ulong
> *search,
>> +                                       uint * index)
>> +{
>> +     DBUG_ENTER("dynamic_array_bin_search");
>> +     long l= 0, i, r= array->elements - 1;
>> +     while (l <= r)
>> +     {
>> +          ulong found;
>> +          DBUG_ASSERT((r < 0 || (ulong) r < array->elements) &&
> l >=0);
>> +          i= (r + l)/2;
>> +          if ((found= (* (ulong*) dynamic_array_ptr(array, i))) == *search)
>> +          {
>> +               if (index) 
>> +                    *index= i;
>> +               DBUG_RETURN(FALSE);
>> +          }
>> +          if (found < *search)
>> +               l= i + 1;
>> +          else
>> +               r= i - 1;
>> +     }
>> +     DBUG_RETURN(TRUE);
>> +}
>
> Why not use the standard bsearch() function available in all known
> compiler implementations? The dynamic array is designed to support that
> usage.
>

I really missed that's the standard C lib ...
Sure, we are better to switch to the standard.

>> +
>> +/**
>> +   Reports if the s_id server has been configured to ignore events 
>> +   it generates with
>> +
>> +      CHANGE MASTER IGNORE_SERVER_IDS= ( list of server ids )
>> +
>> +   Method is called from the io thread event receiver filtering.
>> +
>> +   @param      s_id    the master server identifier
>> +   @return 
>> +     @retval   TRUE    if s_id is in the list of ignored master  servers,
>> +     @retval   FALSE   otherwise.
>> + */
>> +bool Master_info::repl_server_id_ignore_p(ulong s_id)
>
> Please pick another name: there is no other ...p function in the server.

Although there are some defines

>  Although I prefer the p suffix myself (coming from a Lisp background),
> I still think that we should try to keep with the conventions in the server.
>
> Suggestions: shall_ignore_server_id()
>

there is no problem with taking this name instead.

>> +{
>> +  if (likely(ignore_server_ids.elements == 1))
>> +    return (* (ulong*) dynamic_array_ptr(&ignore_server_ids, 0)) == s_id;
>> +  else      
>> +    return !dynamic_array_ulong_bin_search(&ignore_server_ids,
>> +                                           (const ulong *) &s_id, NULL);
>> +}
>> +
>>  void init_master_log_pos(Master_info* mi)
>>  {
>>    DBUG_ENTER("init_master_log_pos");
>> @@ -87,8 +149,11 @@ enum {
>>    /* 6.0 added value of master_heartbeat_period */
>>    LINE_FOR_MASTER_HEARTBEAT_PERIOD= 16,
>>  
>> +  /* 6.0 added value of master_ignore_server_id */
>> +  LINE_FOR_REPLICATE_IGNORE_SERVER_IDS= 17,
>> +
>>    /* Number of lines currently used when saving master info file */
>> -  LINES_IN_MASTER_INFO= LINE_FOR_MASTER_HEARTBEAT_PERIOD
>> +  LINES_IN_MASTER_INFO= LINE_FOR_REPLICATE_IGNORE_SERVER_IDS
>>  };
>>  
>>  
>> @@ -280,6 +345,17 @@ file '%s')", fname);
>>        if (lines >= LINE_FOR_MASTER_HEARTBEAT_PERIOD &&
>>            init_floatvar_from_file(&master_heartbeat_period,
> &mi->file, 0.0))
>>          goto errwithmsg;
>> +      /*
>> +        Starting from 6.0 list of server_id of ignorable servers might be
>> +        in the file
>> +      */
>> +      if (lines >= LINE_FOR_REPLICATE_IGNORE_SERVER_IDS &&
>> +          init_dynarray_intvar_from_file(&mi->ignore_server_ids,
> &mi->file))
>> +      {
>> +        sql_print_error("Failed to initialize master info ignore_server_ids");
>> +        goto errwithmsg;
>> +      }
>> +
>>      }
>>  
>>  #ifndef HAVE_OPENSSL
>> @@ -379,16 +455,41 @@ int flush_master_info(Master_info* mi, b
>>    */
>>    char heartbeat_buf[sizeof(mi->heartbeat_period) * 4]; // buffer to suffice
> always
>>    my_sprintf(heartbeat_buf, (heartbeat_buf, "%.3f", mi->heartbeat_period));
>> +  /*
>> +    produce a line listing the total number and all the ignored server_id:s
>> +  */
>> +  char* ignore_server_ids_buf;
>> +  {
>> +    ulong cur_len;
>> +    ignore_server_ids_buf=
>> +      (char *) my_malloc((sizeof(::server_id) * 3 + 1) *
>> +                         (1 + mi->ignore_server_ids.elements), MYF(MY_WME));
>> +    if (!ignore_server_ids_buf)
>> +      DBUG_RETURN(1);
>> +    my_sprintf(ignore_server_ids_buf,
>> +               (ignore_server_ids_buf, "%u",
> mi->ignore_server_ids.elements));
>> +    for (ulong i= 0, cur_len= strlen(ignore_server_ids_buf);
>
> Suggestion, set cur_len from the result of the my_sprintf() call instead
> of calling strlen() and re-compute the length as follows:
>
> cur_len= my_sprintf(ignore_server_ids_buf,
>                     (ignore_server_ids_buf, "%u",
>                      mi->ignore_server_ids.elements));
>

sure.


> for (ulong i= 0;
>
>> +         i < mi->ignore_server_ids.elements; i++)
>> +    {
>> +      ulong s_id;
>> +      get_dynamic(&mi->ignore_server_ids, (uchar*) &s_id, i);
>> +      cur_len +=my_sprintf(ignore_server_ids_buf + cur_len,
>> +                           (ignore_server_ids_buf + cur_len,
>> +                            " %lu", s_id));
>> +    }
>> +  }
>>    my_b_seek(file, 0L);
>>    my_b_printf(file,
>> -             
> "%u\n%s\n%s\n%s\n%s\n%s\n%d\n%d\n%d\n%s\n%s\n%s\n%s\n%s\n%d\n%s\n",
>> +             
> "%u\n%s\n%s\n%s\n%s\n%s\n%d\n%d\n%d\n%s\n%s\n%s\n%s\n%s\n%d\n%s\n%s\n",
>>                LINES_IN_MASTER_INFO,
>>                mi->master_log_name, llstr(mi->master_log_pos, lbuf),
>>                mi->host, mi->user,
>>                mi->password, mi->port, mi->connect_retry,
>>                (int)(mi->ssl), mi->ssl_ca, mi->ssl_capath,
> mi->ssl_cert,
>>                mi->ssl_cipher, mi->ssl_key, mi->ssl_verify_server_cert,
>> -              heartbeat_buf);
>> +              heartbeat_buf,
>> +              ignore_server_ids_buf);
>> +  my_free(ignore_server_ids_buf, MYF(0));
>>    DBUG_RETURN(-flush_io_cache(file));
>>  }
>>  
>> diff -Nrup a/sql/rpl_mi.h b/sql/rpl_mi.h
>> --- a/sql/rpl_mi.h	2008-02-03 11:00:47 +02:00
>> +++ b/sql/rpl_mi.h	2008-06-03 14:36:55 +03:00
>> @@ -20,6 +20,7 @@
>>  
>>  #include "rpl_rli.h"
>>  #include "rpl_reporting.h"
>> +#include "my_sys.h"
>>  
>>  
>>  /*****************************************************************************
>> @@ -60,6 +61,7 @@ class Master_info : public Slave_reporti
>>   public:
>>    Master_info();
>>    ~Master_info();
>> +  bool repl_server_id_ignore_p(ulong s_id);
>>  
>>    /* the variables below are needed because we can change masters on the fly */
>>    char master_log_name[FN_REFLEN];
>> @@ -85,10 +87,12 @@ class Master_info : public Slave_reporti
>>    uint connect_retry;
>>    float heartbeat_period;         // interface with CHANGE MASTER or
> master.info
>>    ulonglong received_heartbeats;  // counter of received heartbeat events
>> +  DYNAMIC_ARRAY ignore_server_ids;
>>  #ifndef DBUG_OFF
>>    int events_till_disconnect;
>>  #endif
>>    bool inited;
>> +  ulong master_id;
>>    volatile bool abort_slave;
>>    volatile uint slave_running;
>>    volatile ulong slave_run_id;
>> diff -Nrup a/sql/share/errmsg.txt b/sql/share/errmsg.txt
>> --- a/sql/share/errmsg.txt	2008-04-14 19:58:40 +03:00
>> +++ b/sql/share/errmsg.txt	2008-06-03 14:36:55 +03:00
>> @@ -6252,3 +6252,5 @@ ER_SLAVE_HEARTBEAT_FAILURE
>>    eng "Unexpected master's heartbeat data: %s"
>>  ER_SLAVE_HEARTBEAT_VALUE_OUT_OF_RANGE
>>    eng "The requested value for the heartbeat period %s %s"
>> +ER_SLAVE_IGNORE_SERVER_IDS
>> +  eng "The requested server id %d clashes with the slave startup option
> --replicate-same-server-id"
>> diff -Nrup a/sql/slave.cc b/sql/slave.cc
>> --- a/sql/slave.cc	2008-04-01 13:56:42 +03:00
>> +++ b/sql/slave.cc	2008-06-03 14:36:55 +03:00
>> @@ -759,6 +759,95 @@ int init_floatvar_from_file(float* var, 
>>    DBUG_RETURN(1);
>>  }
>>  
>> +/**
>> +   A master info read method
>> +
>> +   This function is called from @c init_master_info() @c along with
>
> Drop the second @c. The @c make the next word in "code font", and is not
> a delimiter. The delimiters in Doxygen comments is <code> and </code>
>
>> +   relatives to restore some of @c active_mi @c members.
>
> See above
>
>> +   Particularly, this function is responsible for restoring
>> +   IGNORE_SERVER_IDS list of servers whose events the slave is
>> +   going to ignore (to not log them in the relay log).
>> +   Items being read are supposed to be decimal output of values of a
>> +   type shorter or equal of @c long @c and separated by the single space.
>> +
>> +   @param arr         @c DYNAMIC_ARRAY @c pointer to storage for servers id
>
> See above.
>
>> +   @param f           @c IO_CACHE @c pointer to the source file
>
> See above.
>
>> +
>> +   @return
>
> Not necessary, just use the @retval tags. The @return tag is for more
> descriptive return values, e.g., "@return A pointer to the structure
> member".
>


ok. Thanks for explaning the rules!

>> +     @retval 0         All OK
>> +     @retval non-zero  An error
>> +*/
>> +
>> +int init_dynarray_intvar_from_file(DYNAMIC_ARRAY* arr, IO_CACHE* f)
>> +{
>> +  int ret= 0;
>> +  char buf[16 * (sizeof(long)*4 + 1)]; // static buffer to use most of times
>> +  char *buf_act= buf; // actual buffer can be dynamic if static is short
>> +  char *token, *last;
>> +  uint num_items;     // number of items of `arr'
>> +  size_t read_size;
>> +  DBUG_ENTER("init_dynarray_intvar_from_file");
>> +
>> +  if ((read_size= my_b_gets(f, buf_act, sizeof(buf))) == 0)
>> +  {
>> +    return 0; // no line in master.info
>> +  }e
>> +  if (read_size + 1 == sizeof(buf) && buf[sizeof(buf) - 2] != '\n')
>> +  {
>> +    /*
>> +      short read happpend; allocate sufficient memory and make the 2nd read
>
> Typo: three "p"
>

ok

>> +    */
>> +    char buf_work[(sizeof(long)*3 + 1)*16];
>> +    memcpy(buf_work, buf, sizeof(buf_work));
>> +    num_items= atoi(strtok_r(buf_work, " ", &last));
>> +    size_t snd_size;
>> +    /*
>> +      max size lower bound approximate estimation bases on the formula:
>> +      (the items number + items themselves) * 
>> +          (decimal size + space) - 1 + `\n' + '\0'
>> +    */
>> +    size_t max_size= (1 + num_items) * (sizeof(long)*3 + 1) + 1;
>> +    buf_act= (char*) my_malloc(max_size, MYF(MY_WME));
>> +    memcpy(buf_act, buf, read_size);
>> +    snd_size= my_b_gets(f, buf_act + read_size, max_size - read_size);
>> +    if (snd_size == 0 ||
>> +        (snd_size + 1 == max_size - read_size) &&  buf[max_size - 2] !=
> '\n')
>> +    {
>> +      /*
>> +        failure to make the 2nd read or short read again
>> +      */
>> +      ret= 1;
>> +      goto err;
>> +    }
>> +  }
>> +  token= strtok_r(buf_act, " ", &last);
>> +  if (token == NULL)
>> +  {
>> +    ret= 1;
>> +    goto err;
>> +  }
>> +  num_items= atoi(token);
>> +  for (uint i=0; i < num_items; i++)
>> +  {
>> +    token= strtok_r(NULL, " ", &last);
>> +    if (token == NULL)
>> +    {
>> +      ret= 1;
>> +      goto err;
>> +    }
>> +    else
>> +    {
>> +      ulong val= atol(token);
>> +      insert_dynamic(arr, (uchar *) &val);
>> +    }
>> +  }
>> +err:
>> +  if (buf_act != buf)
>> +    my_free(buf_act, MYF(0));
>> +  DBUG_RETURN(ret);
>> +}
>> +
>
> Is the list in the file guaranteed to be in sorted order? The reading
> might produce a list in non-sorted order otherwise.
>

it is sorted, because the mi->ignore_server_ids is sorted.

>
>> +
>>  static bool check_io_slave_killed(THD *thd, Master_info *mi, const char *info)
>>  {
>>    if (io_slave_killed(thd, mi))
>> @@ -907,7 +996,7 @@ static int get_master_version_and_clock(
>>        (master_res= mysql_store_result(mysql)))
>>    {
>>      if ((master_row= mysql_fetch_row(master_res)) &&
>> -        (::server_id == strtoul(master_row[1], 0, 10)) &&
>> +        (::server_id == (mi->master_id= strtoul(master_row[1], 0, 10)))
> &&
>
> Any specific reason to restrict it to base 10 instead of just relying on
> the 0 base?
>

Why should we rely on anything implicit if there is a clear simple way to
demand what we need explicitly?

>>          !mi->rli.replicate_same_server_id)
>>      {
>>        errmsg=
>> @@ -923,7 +1012,14 @@ static int get_master_version_and_clock(
>>      if (errmsg)
>>        goto err;
>>    }
>> -
>> +  if (mi->master_id == 0 && mi->ignore_server_ids.elements >
> 0)
>> +  {
>> +    errmsg="Slave configured with server id filtering could not detect the
> master server id.";
>> +    err_code= ER_SLAVE_FATAL_ERROR;
>> +    sprintf(err_buff, ER(err_code), errmsg);
>> +    err_msg.append(err_buff);
>> +    goto err;
>> +  }
>>    /*
>>      Check that the master's global character_set_server and ours are the same.
>>      Not fatal if query fails (old master?).
>> @@ -1238,6 +1334,10 @@ bool show_master_info(THD* thd, Master_i
>>    field_list.push_back(new Item_empty_string("Last_IO_Error", 20));
>>    field_list.push_back(new Item_return_int("Last_SQL_Errno", 4,
> MYSQL_TYPE_LONG));
>>    field_list.push_back(new Item_empty_string("Last_SQL_Error", 20));
>> +  field_list.push_back(new Item_empty_string("Replicate_Ignore_Server_Ids",
>> +                                             FN_REFLEN));
>> +  field_list.push_back(new Item_return_int("Master_Server_Id", sizeof(ulong),
>> +                                           MYSQL_TYPE_LONG));
>>  
>>    if (protocol->send_fields(&field_list,
>>                              Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF))
>> @@ -1357,6 +1457,32 @@ bool show_master_info(THD* thd, Master_i
>>      protocol->store(mi->rli.last_error().number);
>>      // Last_SQL_Error
>>      protocol->store(mi->rli.last_error().message, &my_charset_bin);
>> +    // Replicate_Ignore_Server_Ids
>> +    {
>> +      char buff[FN_REFLEN];
>> +      ulong i, cur_len;
>> +      for (i= 0, buff[0]= 0, cur_len= 0;
>> +           i < mi->ignore_server_ids.elements; i++)
>> +      {
>> +        ulong s_id, slen;
>> +        char sbuff[FN_REFLEN];
>> +        get_dynamic(&mi->ignore_server_ids, (uchar*) &s_id, i);
>> +        slen= my_sprintf(sbuff, (sbuff, (i==0? "%lu" : ", %lu"), s_id));
>> +        if (cur_len + slen + 4 > FN_REFLEN)
>> +        {
>> +          /*
>> +            break the loop whenever remained space could not fit
>> +            ellipses on the next cycle
>> +          */
>> +          my_sprintf(buff + cur_len, (buff + cur_len, "..."));
>
> Show master info is not only for providing human-readable output, it is
> also used by applications to get information. You need to ensure that
> *all* information is printed *and* that it is printed in a format that
> an application can use (it seems like you're using a space-separated
> list here).
>

Please consider two things.

1. it will never happen in the real world that the length of the
ignored sever list exceed FN_REFLEN/11 > 46 elements.

2. The point why I made ellipses is because conceptually we would need to
report something instead of nothing if an accident happened (not in
the real world).

An application needs to take care of ellipsis, and I don't see any
problem in that. I am not adding ellipesis to an existing parameter's
format, it's a new parameter. 
I hope you share my opinion that we need merely document that (without
refering to the real word time time :-).



>> +          break;
>> +        }
>> +        cur_len += my_sprintf(buff + cur_len, (buff + cur_len, "%s", sbuff));
>> +      }
>> +      protocol->store(buff, &my_charset_bin);
>> +    }
>> +    // Master_Server_id
>> +    protocol->store((uint32) mi->master_id);
>>  
>>      pthread_mutex_unlock(&mi->rli.data_lock);
>>      pthread_mutex_unlock(&mi->data_lock);
>> @@ -3069,6 +3195,7 @@ static int queue_event(Master_info* mi,c
>>    ulong inc_pos;
>>    Relay_log_info *rli= &mi->rli;
>>    pthread_mutex_t *log_lock= rli->relay_log.get_log_lock();
>> +  ulong s_id;
>>    DBUG_ENTER("queue_event");
>>  
>>    LINT_INIT(inc_pos);
>> @@ -3215,9 +3342,18 @@ static int queue_event(Master_info* mi,c
>>    */
>>  
>>    pthread_mutex_lock(log_lock);
>> -
>> -  if ((uint4korr(buf + SERVER_ID_OFFSET) == ::server_id) &&
>> -      !mi->rli.replicate_same_server_id)
>> +  s_id= uint4korr(buf + SERVER_ID_OFFSET);
>> +  if ((s_id == ::server_id && !mi->rli.replicate_same_server_id) ||
>> +      /*
>> +        the following conjunction deals with IGNORE_SERVER_IDS, if set
>> +      */
>> +      (mi->ignore_server_ids.elements > 0 &&
>> +       mi->repl_server_id_ignore_p(s_id) &&
>> +       /* everything is filtered out from non-master */
>> +       (s_id != mi->master_id ||
>
> OK, but could you write out the comments. It was not immediate that the
> code was correct. 

What do you mean "It was not immediate the code is not correct"? 
How comments can prevent that?
And there are comments.

>(I.e., that if the master is on the ignore list, all
> events are ignored except those relating to the relay log management.)
>

Sorry, I seem to misunderstand your concern...

In the immediate ignored master case the slave needs meta info.
Do you really think I need to prove that?

>
>> +        /* for the master meta information is necessary */
>> +        buf[EVENT_TYPE_OFFSET] != FORMAT_DESCRIPTION_EVENT &&
>> +        buf[EVENT_TYPE_OFFSET] != ROTATE_EVENT)))
>>    {
>>      /*
>>        Do not write it to the relay log.
>> @@ -3232,10 +3368,14 @@ static int queue_event(Master_info* mi,c
>>        But events which were generated by this slave and which do not exist in
>>        the master's binlog (i.e. Format_desc, Rotate & Stop) should not
> increment
>>        mi->master_log_pos.
>> -    */
>> -    if (buf[EVENT_TYPE_OFFSET]!=FORMAT_DESCRIPTION_EVENT &&
>> -        buf[EVENT_TYPE_OFFSET]!=ROTATE_EVENT &&
>> -        buf[EVENT_TYPE_OFFSET]!=STOP_EVENT)
>> +      If the event is originated remotely and is being filtered out by
>> +      IGNORE_SERVER_IDS it increments mi->master_log_pos
>> +      as well as rli->group_relay_log_pos.
>> +    */
>> +    if (!(s_id == ::server_id && !mi->rli.replicate_same_server_id)
> ||
>> +        buf[EVENT_TYPE_OFFSET] != FORMAT_DESCRIPTION_EVENT &&
>> +        buf[EVENT_TYPE_OFFSET] != ROTATE_EVENT &&
>> +        buf[EVENT_TYPE_OFFSET] != STOP_EVENT)
>>      {
>>        mi->master_log_pos+= inc_pos;
>>        memcpy(rli->ign_master_log_name_end, mi->master_log_name,
> FN_REFLEN);
>> @@ -3243,8 +3383,8 @@ static int queue_event(Master_info* mi,c
>>        rli->ign_master_log_pos_end= mi->master_log_pos;
>>      }
>>      rli->relay_log.signal_update(); // the slave SQL thread needs to
> re-check
>> -    DBUG_PRINT("info", ("master_log_pos: %lu, event originating from the same
> server, ignored",
>> -                        (ulong) mi->master_log_pos));
>> +    DBUG_PRINT("info", ("master_log_pos: %lu, event originating from %u server,
> ignored",
>> +                        (ulong) mi->master_log_pos, uint4korr(buf +
> SERVER_ID_OFFSET)));
>>    }
>>    else
>>    {
>> diff -Nrup a/sql/sql_lex.h b/sql/sql_lex.h
>> --- a/sql/sql_lex.h	2008-04-01 13:56:44 +03:00
>> +++ b/sql/sql_lex.h	2008-06-03 14:36:55 +03:00
>> @@ -207,10 +207,11 @@ typedef struct st_lex_master_info
>>      changed variable or if it should be left at old value
>>     */
>>    enum {LEX_MI_UNCHANGED, LEX_MI_DISABLE, LEX_MI_ENABLE}
>> -    ssl, ssl_verify_server_cert, heartbeat_opt;
>> +    ssl, ssl_verify_server_cert, heartbeat_opt, repl_ignore_server_ids_opt;
>>    char *ssl_key, *ssl_cert, *ssl_ca, *ssl_capath, *ssl_cipher;
>>    char *relay_log_name;
>>    ulong relay_log_pos;
>> +  DYNAMIC_ARRAY repl_ignore_server_ids; 
>>  } LEX_MASTER_INFO;
>>  
>>  
>> diff -Nrup a/sql/sql_repl.cc b/sql/sql_repl.cc
>> --- a/sql/sql_repl.cc	2008-03-08 12:14:45 +02:00
>> +++ b/sql/sql_repl.cc	2008-06-03 14:36:55 +03:00
>> @@ -1189,32 +1189,41 @@ void kill_zombie_dump_threads(uint32 sla
>>    }
>>  }
>>  
>> +/**
>> +   A comparison function to be supplied as argument to @c sort_dynamic() @c
>> +   provide ascendant sort of a ulong-items dynamic array
>> +*/
>> +static int change_master_server_id_cmp(ulong *id1, ulong *id2)
>> +{
>> +  return *id1 > *id2;
>> +}
>>  
>>  bool change_master(THD* thd, Master_info* mi)
>>  {
>>    int thread_mask;
>>    const char* errmsg= 0;
>>    bool need_relay_log_purge= 1;
>> +  bool ret= FALSE;
>>    DBUG_ENTER("change_master");
>>  
>>    lock_slave_threads(mi);
>>    init_thread_mask(&thread_mask,mi,0 /*not inverse*/);
>> +  LEX_MASTER_INFO* lex_mi= &thd->lex->mi;
>>    if (thread_mask) // We refuse if any slave thread is running
>>    {
>>      my_message(ER_SLAVE_MUST_STOP, ER(ER_SLAVE_MUST_STOP), MYF(0));
>> -    unlock_slave_threads(mi);
>> -    DBUG_RETURN(TRUE);
>> +    ret= TRUE;
>> +    goto err;
>>    }
>>  
>>    thd_proc_info(thd, "Changing master");
>> -  LEX_MASTER_INFO* lex_mi= &thd->lex->mi;
>>    // TODO: see if needs re-write
>>    if (init_master_info(mi, master_info_file, relay_log_info_file, 0,
>>  		       thread_mask))
>>    {
>>      my_message(ER_MASTER_INFO, ER(ER_MASTER_INFO), MYF(0));
>> -    unlock_slave_threads(mi);
>> -    DBUG_RETURN(TRUE);
>> +    ret= TRUE;
>> +    goto err;
>>    }
>>  
>>    /*
>> @@ -1259,6 +1268,27 @@ bool change_master(THD* thd, Master_info
>>      mi->heartbeat_period= (float) min(SLAVE_MAX_HEARTBEAT_PERIOD,
>>                                        (slave_net_timeout/2.0));
>>    mi->received_heartbeats= LL(0); // counter lives until master is CHANGEd
>> +  /*
>> +    reset the last time server_id list if the current CHANGE MASTER 
>> +    is mentioning IGNORE_SERVER_IDS= (...)
>> +  */
>> +  if (lex_mi->repl_ignore_server_ids_opt == LEX_MASTER_INFO::LEX_MI_ENABLE)
>> +    reset_dynamic(&mi->ignore_server_ids);
>> +  for (uint i= 0; i < lex_mi->repl_ignore_server_ids.elements; i++)
>> +  {
>> +    ulong s_id;
>> +    get_dynamic(&lex_mi->repl_ignore_server_ids, (uchar*) &s_id, i);
>> +    if (s_id == ::server_id && replicate_same_server_id)
>> +    {
>> +      my_error(ER_SLAVE_IGNORE_SERVER_IDS, MYF(0), s_id);
>> +      ret= TRUE;
>> +      goto err;
>> +    }
>> +    else
>> +      insert_dynamic(&mi->ignore_server_ids, (uchar*) &s_id);
>> +  }
>> +  sort_dynamic(&mi->ignore_server_ids, (qsort_cmp)
> change_master_server_id_cmp);
>> +
>>    if (lex_mi->ssl != LEX_MASTER_INFO::LEX_MI_UNCHANGED)
>>      mi->ssl= (lex_mi->ssl == LEX_MASTER_INFO::LEX_MI_ENABLE);
>>  
>> @@ -1336,8 +1366,8 @@ bool change_master(THD* thd, Master_info
>>    if (flush_master_info(mi, 0))
>>    {
>>      my_error(ER_RELAY_LOG_INIT, MYF(0), "Failed to flush master info file");
>> -    unlock_slave_threads(mi);
>> -    DBUG_RETURN(TRUE);
>> +    ret= TRUE;
>> +    goto err;
>>    }
>>    if (need_relay_log_purge)
>>    {
>> @@ -1348,8 +1378,8 @@ bool change_master(THD* thd, Master_info
>>  			 &errmsg))
>>      {
>>        my_error(ER_RELAY_LOG_FAIL, MYF(0), errmsg);
>> -      unlock_slave_threads(mi);
>> -      DBUG_RETURN(TRUE);
>> +      ret= TRUE;
>> +      goto err;
>>      }
>>    }
>>    else
>> @@ -1364,8 +1394,8 @@ bool change_master(THD* thd, Master_info
>>  			   &msg, 0))
>>      {
>>        my_error(ER_RELAY_LOG_INIT, MYF(0), msg);
>> -      unlock_slave_threads(mi);
>> -      DBUG_RETURN(TRUE);
>> +      ret= TRUE;
>> +      goto err;
>>      }
>>    }
>>    /*
>> @@ -1402,10 +1432,13 @@ bool change_master(THD* thd, Master_info
>>    pthread_cond_broadcast(&mi->data_cond);
>>    pthread_mutex_unlock(&mi->rli.data_lock);
>>  
>> +err:
>>    unlock_slave_threads(mi);
>>    thd_proc_info(thd, 0);
>> -  my_ok(thd);
>> -  DBUG_RETURN(FALSE);
>> +  if (ret == FALSE)
>> +    my_ok(thd);
>> +  delete_dynamic(&lex_mi->repl_ignore_server_ids); //freeing of
> parser-time alloc
>> +  DBUG_RETURN(ret);
>>  }
>>  
>>  int reset_master(THD* thd)
>> diff -Nrup a/sql/sql_yacc.yy b/sql/sql_yacc.yy
>> --- a/sql/sql_yacc.yy	2008-04-01 16:19:00 +03:00
>> +++ b/sql/sql_yacc.yy	2008-06-03 14:36:55 +03:00
>> @@ -818,6 +818,7 @@ bool my_yyoverflow(short **a, YYSTYPE **
>>  %token  IDENT_QUOTED
>>  %token  IF
>>  %token  IGNORE_SYM
>> +%token  IGNORE_SERVER_IDS_SYM
>>  %token  IMPORT
>>  %token  INDEXES
>>  %token  INDEX_SYM
>> @@ -1636,6 +1637,13 @@ change:
>>              LEX *lex = Lex;
>>              lex->sql_command = SQLCOM_CHANGE_MASTER;
>>              bzero((char*) &lex->mi, sizeof(lex->mi));
>> +            /*
>> +              resetting flags that can left from the previous CHANGE MASTER
>> +            */
>> +            lex->mi.heartbeat_opt= LEX_MASTER_INFO::LEX_MI_UNCHANGED;
>> +            lex->mi.repl_ignore_server_ids_opt=
> LEX_MASTER_INFO::LEX_MI_UNCHANGED;
>> +            my_init_dynamic_array(&Lex->mi.repl_ignore_server_ids,
>> +                                  sizeof(::server_id), 16, 16);
>>            }
>>            master_defs
>>            {}
>> @@ -1736,8 +1744,25 @@ master_def:
>>              }
>>              Lex->mi.heartbeat_opt=  LEX_MASTER_INFO::LEX_MI_ENABLE;
>>            }
>> +        | IGNORE_SERVER_IDS_SYM EQ '(' ignore_server_id_list ')'
>> +          {
>> +            Lex->mi.repl_ignore_server_ids_opt=
> LEX_MASTER_INFO::LEX_MI_ENABLE;
>> +          }
>>          |
>>          master_file_def
>> +        ;
>> +
>> +ignore_server_id_list:
>> +          /* Empty */
>> +          | ignore_server_id
>> +          | ignore_server_id_list ',' ignore_server_id
>> +        ;
>> +
>> +ignore_server_id:
>> +          ulong_num
>> +          {
>> +            insert_dynamic(&Lex->mi.repl_ignore_server_ids, (uchar*)
> &($1));
>> +          }
>>          ;
>>  
>>  master_file_def:
>> 


cheers,

Andrei