List:Commits« Previous MessageNext Message »
From:Mats Kindahl Date:April 19 2010 3:19pm
Subject:Re: bzr push into mysql-5.1 branch (sven.sandberg:3367 to 3368)
Bug#28760 WL#344
View as plain text  
Sven Sandberg wrote:
> Hi Andrei,
> 
> Thank you for a deep and detailed review! Please see comments inline.
> 
> /Sven
> 
> 
> Andrei Elkin wrote:
[snip]

>>   +  field_list.push_back(new
>> Item_empty_string("Slave_SQL_Running_State", 20));
>>
>> is clear to qualify more specifically the existing Slave_SQL_Running.
>> If we really have to do it for the SQL thread should not it be
>> followed up
>> with the IO thread as well?
> 
> There is already a field for the IO thread: Slave_IO_State.
> Slave_SQL_Running_State is the corresponding field for the SQL thread.

Is there a reason for not calling it Slave_SQL_State in that case (to
match the Slave_IO_State)?

[snip]

>> I have almost agreed. Deploying a mutex lock/unlock fixes an
>> issue. What's good that the mutex work is not on a critical exec path.
>> Still making
>>     `mi->clock_diff_with_master' volatile achieves the same w/o
>> considering
>> a deadlock threat now or in future. While there is only one writer to
>> the variable volatilizing looks looks somehow better to me.
> 
> Do you mean we should do this:
> 
>  struct Master_info {
> [...]
> -  long clock_diff_with_master;
> +  volatile long clock_diff_with_master;
> 
> and not take any lock? But volatile is not meant for threading. In
> particular, access to a volatile variable is not guaranteed to be
> atomic. The standard allows, e.g., that on some platform a 32 bit word
> is written in two instructions: first the low 16 bits and then the high
> 16 bits. So if clock_diff_with_master is updated in the SQL thread, and
> there is a thread switch in the middle of the update, then the other
> thread may read an inconsistent state (i.e., it may read something that
> is neither the value before update nor the value after update).
> 
> So I think we should keep the lock.

... or replace the update with an atomic update. We have those in the
code base now.

/Matz
-- 
Mats Kindahl
Senior Software Engineer
Database Technology Group
Sun Microsystems
Thread
Re: bzr push into mysql-5.1 branch (sven.sandberg:3367 to 3368)Bug#28760 WL#344Sven Sandberg19 Apr
  • Re: bzr push into mysql-5.1 branch (sven.sandberg:3367 to 3368)Bug#28760 WL#344Mats Kindahl19 Apr
    • Re: bzr push into mysql-5.1 branch (sven.sandberg:3367 to 3368)Bug#28760 WL#344Sven Sandberg19 Apr
      • Re: bzr push into mysql-5.1 branch (sven.sandberg:3367 to 3368)Bug#28760 WL#344Mats Kindahl19 Apr
Re: bzr push into mysql-5.1 branch (sven.sandberg:3367 to 3368)Bug#28760 WL#344Sven Sandberg20 Apr