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
