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

[Mats Kindahl]

He Zhenxing wrote:
> Mats Kindahl wrote:
>> 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.
>>
> 
> apart from that, do you see anything suspect? Lars said that if this
> patch is 'quite risk-free', we can consider it for 5.1.

No, not really. The patch is straightforward and has a default that is
the same as before it was added. The changes in existing execution paths
is minimal, so I don't see any significant problems.

>>>>> @@ -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?
>>
> 
> It is possible to guard the code with another lock in the update method,
> but when running commands like SHOW VARIALBLES or SELECT, the code to
> get the value is always guarded by LOCK_global_system_variables, even if
> the SHOW_TYPE is SHOW_FUNC, the show function can only return a pointer
> to the value of the system variable, and then the code to get the value
> from the pointer is only guarded by LOCK_global_system_variables. So if
> we use our internal lock, the value returned by SHOW or SELECT command
> might be incorrect.

OK.

Just my few cents,
Mats Kindahl
-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com