[Fwd: Re: bzr commit into mysql-6.0-backup branch (jorgen.loland:2892) Bug#44568]

[Jørgen Løland]

Sent this to reviewer only...

-------- Original Message --------
Subject: Re: bzr commit into mysql-6.0-backup branch 
(jorgen.loland:2892) Bug#44568
Date: Thu, 26 Nov 2009 10:57:00 +0100
From: Jørgen Løland <Jorgen.Loland@stripped>
To: Rafal Somla <Rafal.Somla@stripped>
References: <20091120142738.AF5FEBCC00D@stripped> 
<4B0D656C.4000108@stripped>

Rafal Somla wrote:
> ...
>>    /*
>> -    Check whether BML is active. If yes, wait for deactivation which 
>> is -    signalled with COND_BML.
>> +    Check whether an exclusive lock is set. If yes, wait for release
>> +    which is signalled with COND_no_xlock.
>>    */
>> -  pthread_mutex_lock(&THR_LOCK_BML_active);
>> -  thd->enter_cond(&COND_BML, &THR_LOCK_BML_active,
>> -                  "BML: waiting until released");
>> -  DEBUG_SYNC(thd, "bml_enter_check");
>> -  while (BML_active && !thd->killed && !thd->BML_exception
> && (ret == 
>> 0))
>> +  pthread_mutex_lock(&LOCK_mutex);
>> +  thd->enter_cond(&COND_no_xlock, &LOCK_mutex,
>> +                  "Backup_sx_lock: waiting until exclusive lock is 
>> released");
>> +  while (xlock_set && xlock_set!=thd && !thd->killed
> && (ret == 0))
>>    {
>>      if (thd->backup_wait_timeout == 0)
>> +    {
>>        ret = -1;
>> +    }
>>      else
>> -      ret= pthread_cond_timedwait(&COND_BML, &THR_LOCK_BML_active,
>> +    {
>> +      /*
>> +        The below sync point fires if the exclusive lock is set.
>> +        +        WARNING: Be careful when using WAIT_FOR with this 
>> sync point.
>> +        We hold LOCK_mutex here.
>> +      */
>> +      DEBUG_SYNC(thd, "wait_if_backup_exclusive_locked");
>> +      ret= pthread_cond_timedwait(&COND_no_xlock, &LOCK_mutex,
>>                                    &ddl_timeout);
>> +    }
>>    }
>> +
> 
> I wonder if it would not possible/better to thd->exit_cond() here, after 
> the while loop. Then look for interruptions, errors etc.

We can't release the mutex before slock_count has been incremented.

> ...
>> +my_bool Backup_sx_lock::get_exclusive_lock(THD *thd)
>> +{
>> +  DBUG_ENTER("Backup_sx_lock::get_exclusive_lock()");
>> +
>> +  //part 1 - get the exclusive lock
>> +  pthread_mutex_lock(&LOCK_mutex);
>> +  if (unlikely(xlock_set))    {
>> -    thd->exit_cond("BML: Thread was killed");
>> +    /* +       Should never happen since only one backup/restore 
>> operation is
>> +       allowed at any time +    */
> 
> Hmm, what about the case that xlock_set == thd, that is, the same thread 
> is getting exclusive lock while already holding it. I'd allow such 
> scenario (i.e. report success in that case), but it is also OK to leave 
> it as it is.

I see your point, but this should never happen. I considered even adding
an ASSERT(!xlock_set), but figured returning false was strong enough.

> ...
>> +  //part 2 - wait for all shared locks to be released
>> +  thd->enter_cond(&COND_no_slock, &LOCK_mutex,
>> +                  "Backup_sx_lock: waiting for release of all Shared 
>> locks");
>> +  while (slock_count != 0 && !thd->killed) +  {
>> +    /*
>> +      The below sync point fires if a shared lock is set.
>> +      +      WARNING: Be careful when using WAIT_FOR with this sync 
>> point.
>> +      We hold LOCK_mutex here.
>> +    */
>> +    DEBUG_SYNC(thd, "wait_backup_exclusive_lock");
>> +    /* wait in queue that is activated when slock_count drops to 0 */
>> +    pthread_cond_wait(&COND_no_slock, &LOCK_mutex); +  }
> 
> Suggestion: First exit_cond(), then do result analysis. I do see that 
> you give different info to exit_cond() depending on the context. 
> However, in this case I'd prefer safety to verbosity - it is very easy 
> to forget to add exit_cond() in some branch and then we have potential 
> deadlock which is hard to investigate.
> 

I agree, even though the logic is really simple. exit_cond moved to
before if(thd->killed)

>
>> === modified file 'sql/handler.cc'
> ...
>> @@ -1174,6 +1180,11 @@ int ha_commit_trans(THD *thd, bool all)
>>          !thd->slave_thread)
>>      {
>>        my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
>> +      if (bcb_registered) +      {
>> +        BCB_instance->release_shared_lock();
>> +        bcb_registered=FALSE;
>> +      }
> 
> I wonder why you need this if below you "goto end" and there the lock 
> will be released.

I figured I'd release the shared lock as early as possible, but when I
think of it there shouldn't be much of a difference. This is likely a
very rear scenario, so let's not overdo it. Removed :-)


-- 
Jørgen Løland

-- 
Jørgen Løland