Re: bzr commit into mysql-5.1-rpl branch (hezx:2611) Bug#35542, Bug#31665

[Mats Kindahl]

He Zhenxing wrote:
> Mats Kindahl wrote:
>> Hi Jason and Sinisa!
>>
>> Here are my comments on the patch...
>>
>> Just my few cents,
>> Mats Kindahl
>>
>> He Zhenxing wrote:
>>> #At file:///media/sda3/work/mysql/bzrwork/b31665/5.1-rpl/
>> Just a note, to avoid misunderstandings: according to bug report, this
>> patch is to be for 6.0, not 5.1.  I would also advice against
>> introducing a patch of this kind into 5.1 since it risks de-stabilizing
>> the server.
>>
> 
> Why do you think this patch is risky?

It adds a feature, i.e., a new server variable.

[snip]

>>> @@ -49,8 +49,7 @@
>>>  
>>>  LOGGER logger;
>>>  
>>> -MYSQL_BIN_LOG mysql_bin_log;
>>> -ulong sync_binlog_counter= 0;
>>> +MYSQL_BIN_LOG mysql_bin_log(&sync_binlog_period);
>> Note: there is no mutex or other form of lock protecting this variable.
>> In the event that the ulong is split into several registers, the two
>> parts might be read and written separately.
>>
> 
> yes, will be fixed

OK.

[snip]

>>> @@ -3512,9 +3516,9 @@ bool MYSQL_BIN_LOG::flush_and_sync()
>>>    safe_mutex_assert_owner(&LOCK_log);
>>>    if (flush_io_cache(&log_file))
>>>      return 1;
>>> -  if (++sync_binlog_counter >= sync_binlog_period &&
> sync_binlog_period)
>>> +  if (*sync_period_ptr && ++sync_counter >= *sync_period_ptr)
>>>    {
>>> -    sync_binlog_counter= 0;
>>> +    sync_counter= 0;
>>>      err=my_sync(fd, MYF(MY_WME));
>>>    }
>>>    return err;
>> If the variable is written at the same time as it is read, this might
>> cause a flush to be missed or an extra flush to be executed. Thanks to
>> the fact that >= is used instead of ==, it will not cease flushing the
>> relay log in the case of a race condition. However, optimization might
>> change the code in unpredictable ways, so I would suggest adding a mutex
>> to protect the variable and add that as an argument to the MYSQL_BIN_LOG
>> constructor, or use the mutex associated with the
>> sys_var_sync_relaylog_period introduced below (which is
>> LOCK_global_system_variables).
>>
> 
> right
> 
>> An alternative would be to make the variable fully internal in the
>> MYSQL_BIN_LOG class, and then change its value through getters and
>> setters, which can then appropriately protect the variable from race
>> conditions by using a fully internal mutex or rwlock, whichever seems
>> most appropriate.
>>
> 
> After dive into the code a little bit, it seems that this approach is
> not possible because when showing the value of the variable, it is
> always guarded by LOCK_global_system_variables.

Sorry, I don't follow. Is it not possible to execute a bit of code
inside the ::update function that calls a setter for the MYSQL_BIN_LOG
object in question?

[snip]

>>> @@ -285,7 +292,7 @@ public:
>>>    Format_description_log_event *description_event_for_exec,
>>>      *description_event_for_queue;
>>>  
>>> -  MYSQL_BIN_LOG();
>>> +  MYSQL_BIN_LOG(ulong *sync_period);
>>>    /*
>>>      note that there's no destructor ~MYSQL_BIN_LOG() !
>>>      The reason is that we don't want it to be automatically called
>> Uh, oh! .. not your code, but I haven't seen this before. There is
>> always a destructor, not specifying one means you get the "free" one
>> (default destructor).
>>
> 
> yes, I think maybe should define a private destructor here.

Yes, I agree.

[snip]

>>> === modified file 'sql/sql_repl.cc'
>>> --- a/sql/sql_repl.cc	2008-02-19 12:58:08 +0000
>>> +++ b/sql/sql_repl.cc	2008-07-07 08:51:09 +0000
>>> @@ -1667,6 +1667,7 @@ static sys_var_long_ptr	sys_slave_net_ti
>>>  static sys_var_long_ptr	sys_slave_trans_retries(&vars,
> "slave_transaction_retries",
>>>  						&slave_trans_retries);
>>>  static sys_var_sync_binlog_period sys_sync_binlog_period(&vars,
> "sync_binlog", &sync_binlog_period);
>>> +static sys_var_sync_binlog_period sys_sync_relaylog_period(&vars,
> "sync_relaylog", &sync_relaylog_period);
>>>  static sys_var_slave_skip_counter sys_slave_skip_counter(&vars,
> "sql_slave_skip_counter");
>>>  
>>>  static int show_slave_skip_errors(THD *thd, SHOW_VAR *var, char *buff);
>>> @@ -1759,7 +1760,7 @@ bool sys_var_slave_skip_counter::update(
>>>  
>>>  bool sys_var_sync_binlog_period::update(THD *thd, set_var *var)
>>>  {
>>> -  sync_binlog_period= (ulong) var->save_result.ulonglong_value;
>>> +  *value= (ulong) var->save_result.ulonglong_value;
>> Assume that there are two threads modifying this variable at the same
>> time (say, to 0x8000 and 0x0040) as it is being read, and also that the
>> variable is split into two parts for some reason. We can then have the
>> following executions:
>>
> 
> yes, and the code before my patch should suffer the same problem. Remove
> this and use the inherited update method will solve this. 

Correct.

> I am wondering why this method was overrided at first place.

I'm not surprised that is was overridde, but rather how. The code for
the replication variables does not acquire the mutex as the ::update
functions in set_var.cc does.

Just my few cents,
Mats Kindahl

-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com