List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:May 5 2009 7:33pm
Subject:Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780
View as plain text  
Hi Davi,

thanks for the review. Please find replies inline.

Davi Arnaut, 05.05.2009 16:21:

...
> On 2/25/09 12:47 PM, Ingo Struewing wrote:
...
>>   2779 Ingo Struewing    2009-02-25
>>        Bug#37780 - main.kill fails randomly
>>
>>        The test case main.kill did not work reliably.
>>
>>        The following problems have been identified:
>>
>>        1. A kill signal could go lost if it came in, short before a thread went
>>        reading on the client connection.
>>
>>        2. A kill signal could go lost if it came in, short before a thread went
>>        waiting on a condition variable.
>>
>>        These problems have been solved as follows. Please see also added code
>>        comments for more details.
>>
>>        1. There is no safe way to detect, when a thread enters the blocking
>>        state of a read(2) or recv(2) system call, where it can be interrupted
>>        by a signal. Hence it is not possible to wait for the right moment to
>>        send a kill signal. To be safe, we need to close the connection.
> 
> Well, this is not exactly safe or portable either. What actually appears
> to do the cancellation tricky on some platforms is the socket shutdown
> in vio_close.

> 
> Perhaps it's too abstract.. i think we should add a explicit
> cancellation operation to Vio to better implement and document the
> quirks. What do you think?


Please be more verbose. Why is it not exactly safe? If the connection is
closed, how can the thread miss it's kill and continue to read more
statements? Why is it not portable? If "the socket shutdown in
vio_close" is not portable, we shouldn't use vio_close() at all. But how
should we then close a connection at session end?

Please note that I got rid of the idea to try shutdown of one direction
only. I'm not fond of proving that this is portable. But if a full close
of a connection isn't portable either, how could MySQL use connections
at all?

...
>> === modified file 'mysql-test/t/kill.test'
...
>> @@ -4,329 +4,594 @@
>>   # This would work if mysqltest run would be threaded and handle each
>>   # connection in a separate thread.
>>   #
> 
> BTW, is the above remark still true?


I haven't read of any change in this area. I'd expect a proper
announcement if either the embedded server or mysqltest are changed to
run sessions in multiple threads.

...
>> +--echo # Disable concurrent inserts to avoid test failures when reading the
>> +--echo # connection id which was inserted into a table by another thread.
>> +SET @old_concurrent_insert= @@global.concurrent_insert;
>> +SET @@global.concurrent_insert= 0;
> 
> Is this still the case?


I haven't read about a change in this area. The server sends ok to the
client before it unlocks the tables. That way the client can signal
another client to examine the data before the table is unlocked. For
MyISAM it means that the other client can read lock the table, where it
stores the "currently known" table size, before the server unlocks the
table from the concurrent insert, where it updates the "currently known"
table size.

> 
>> +--echo # Create two connections for use throughout the test case.
>> +--connect (con1, localhost, root,,)
>> +--connect (con2, localhost, root,,)
>> +
>> +--echo #
>> +--echo # Test kill connection when blocked in read/recv().
>> +--echo #
>>
>> +    --echo # connection con1, store own connection id
>> +    --connection con1
>> +    --let $ID= `SELECT @id := CONNECTION_ID()`
>> +
>> +        --echo # connection con2, store con1 connection id
>> +        --connection con2
>> +        --let $ignore= `SELECT @id := $ID`
> 
> Hum, i find this indentation quite confusing as it doesn't help
> readability, it actually makes it harder to read since most other test
> cases don't follow this schema.
> 
> I guess it attempted to make the code similar to the graph that is used
> to represent parallel thread execution, but in reality there is no way
> to specify simultaneous command, the interpretation and execution of the
> test file is essentially sequential.
> 
> Its a novel and lovable attempt, but using indentation to represent
> thread execution conflicts with the use of indentation to define the
> code structure. Furthermore, this kind of indentation would need to be
> submitted as a coding style proposal.
> 
> I won't reject the patch if you decide to keep this indentation, but i
> urge you to reconsider.


1. For me it is much better readable if connection switches are
emphasized by indentation. Otherwise the switches don't stand out and I
have difficulties to see which actions are done by which connection. I
need much more time to read such a test case to be confident that I
don't miss actions done by a certain connection.

2. We do already have several tests that follow this scheme.

3. Some colleagues find this scheme useful too and use it.

4. I'm nor sure what you mean with "code structure" in conjunction with
tests. I guess that you mainly mean indentation of continuation lines of
long statements. Anyway, in MySQL we indent such things by two columns,
while I (we) use to indent connections by four columns and separate it
by an empty line. That way continuation lines can easily be
distinguished from connection changes on each level.

5. Test case coding style
(http://forge.mysql.com/wiki/How_to_Create_Good_Tests) does not comment
about indentation at all. So this style is neither demanded nor
forbidden. I don't think, we need to change that.

6. There is a second reviewer, who may have another vote on this.

> 
>> +    --echo # connection con1, request a signal from thread end
>> +    --connection con1
>> +    SET DEBUG_SYNC= 'thread_end SIGNAL con1_end';
>> +    #
>> +    --echo # connection con1, disable reconnect
>> +    --disable_reconnect
>> +
>> +        --echo # connection con2, wait for con1 to reach read/recv()
>> +        --echo # Note that this may not be reliable. There is no way to have
>> +        --echo # read/recv() emit a signal which another thread can wait for.
>> +        --echo # We could place a sync point short before vio_read(), but
>> +        --echo # this would still not be safe. So sorry guys, here we must
>> +        --echo # use a sleep! :-( This is still not safe, but best we can do.
> 
> "but it's the best we can do".
> 
> Nonetheless, I think we would be better off with a sync point that would
> work sometimes, compared to a fixed 2 second sleep.


How would you make it into a stable test, in cases where the sync point
doesn't work?

What we could do, however, is to use both. Send a signal at the very
last reasonable moment before going into read(2)/recv(2) and then sleep
two seconds after receiving the signal.

...
>> +    --echo # connection con1, request a signal from ACOS() function
>> +    SET DEBUG_SYNC= 'before_acos_function SIGNAL in_sync';
>> +    #
>> +    --echo # connection con1, run a slow query, using ACOS()
>> +    --echo # This is a very long running query. If this test start failing,
>> +    --echo # it may be necessary to change to an even longer query.
> 
> Is this remark still valid? We use the sync point now...


Yes, it is still valid. The sync point doesn't wait. If it would wait,
the kill would abort the sync point and we won't test anymore, if SELECT
receives a KILL. The difference may be small. If setting the "killed"
flag at some place during SELECT is all that counts, then I can change it.

> 
>> +    send SELECT id FROM t1 WHERE id IN
>> +           (SELECT DISTINCT a.id FROM t2 a, t2 b, t2 c, t2 d
>> +              GROUP BY ACOS(1/a.id), b.id, c.id, d.id
>> +              HAVING a.id BETWEEN 10 AND 20);
...
>> === modified file 'sql/lock.cc'
...
>> @@ -1128,6 +1128,14 @@ bool lock_global_read_lock(THD *thd)
>>       const char *new_message= "Waiting to get readlock";
>>       (void) pthread_mutex_lock(&LOCK_global_read_lock);
>>
>> +   
> old_message=thd->enter_cond(&COND_global_read_lock,&LOCK_global_read_lock,
>> +                                new_message);
> 
> I would take the opportunity and fix indentation :-)


Do you mean "spacing" (space after = and ,)? I don't see a problem with
indentation at this place in sql/lock.cc. I don't have an explanation
for the above misplacement in the diff output.

...
>> === modified file 'sql/mdl.cc'
...
>> +    DEBUG_SYNC(context->thd, "before_wait_excl_mdl_lock");
> 
> Unused sync point, please drop.
...
>> +    DEBUG_SYNC(context->thd, "before_wait_upgrade_mdl_lock");
> 
> Unused sync point, please drop.
...
>> +#if defined(ENABLED_DEBUG_SYNC)
>> +  /*
>> +    The below sync point fires if we have to wait for a table lock.
>> +
>> +    WARNING: Beware to use WAIT_FOR with this sync point. We hold
>> +    LOCK_mdl here.
>> +
>> +    Call the sync point after enter_cond() so that the proc_info is
>> +    available immediately after the sync point sends a SIGNAL. This
>> +    makes tests more reliable.
>> +
>> +    Call the sync point before the loop so that it is executed once only.
>> +  */
>> +  if (!mysys_var->abort&&  global_lock.active_intention_exclusive)
>> +  {
>> +    DEBUG_SYNC(context->thd, "wait_mdl_global_shared");
>> +  }
>> +#endif /* defined(ENABLED_DEBUG_SYNC) */
>> +
> 
> Unused sync point, please drop.
...
>> +    DEBUG_SYNC(context->thd, "before_wait_mdl_locks");
> 
> Unused sync point, please drop.
> 
> Sneaking unused debug sync points in this patch is beyond the scope of
> this bug report and makes reviewing harder. Unused stuff tends to get
> bit rotten overtime...


Ok. I'll remove the unused sync points. And I'll also remove the unused
sync point "mdl_exit_cond", added by Kostja.

Regarding "wait_mdl_global_shared" I can only hope that the "if" will be
added again (ideally together with the comment), if someone needs that
sync point.

...
>> === modified file 'sql/sql_base.cc'
...
>> +  @note 'killed': It is not sufficient to test thd->killed at the top of
>> +  the loop. If the thread is killed after it tested thd->killed and
>> +  before it registered the mutex (here: LOCK_open) in its mysys_var (in
>> +  enter_cond()), the killing thread won't do anything but set
>> +  thd->killed. If we don't check it again after registering the mutex,
>> +  we would start waiting, though thd->killed is set. The solution is to
>> +  check thd->killed again, after registering the mutex and before start
>> +  waiting. Since we cannot 'break' between enter_cond() and exit_cond(),
>> +  we just omit the wait. An additional advantage is the memory barrier,
>> +  included in the mutex handling. This prevents out-of-order execution
>> +  in the processor. Thus we cannot read an old value.
>>   */
> 
> Hum, imho, we should drop the memory barrier bits.. we don't want anyway
> relying on it.


I disagree. The memory barrier is an important property of some hardware
architectures. I failed a couple of times by ignoring it. Since the
"killed" flag wasn't handled properly at some places, I guess that I'm
not the only one, forgetting about it sometimes. E.g. the following will
fail sometimes on some platforms:

  global_variable= new_value;
  pthread_cond_broadcast(...);

Threads that are in pthread_cond_wait() at that time will receive the
signal reliably, but may not see the new value in the global variable.
The following fixes it:

  global_variable= new_value;
  pthread_mutex_lock(...);
  pthread_cond_broadcast(...);
  pthread_mutex_unlock(...);

What else but a hardware memory barrier can be the cause?

...
>> +  @note 'killed': It is not sufficient to test thd->killed at the top of
>> +  the loop. If the thread is killed after it tested thd->killed and
>> +  before it registered the mutex (here: LOCK_open) in its mysys_var (in
>> +  enter_cond()), the killing thread won't do anything but set
>> +  thd->killed. If we don't check it again after registering the mutex,
>> +  we would start waiting, though thd->killed is set. The solution is to
>> +  check thd->killed again, after registering the mutex and before start
>> +  waiting. Since we cannot 'break' between enter_cond() and exit_cond(),
>> +  we just omit the wait. An additional advantage is the memory barrier,
>> +  included in the mutex handling. This prevents out-of-order execution
>> +  in the processor. Thus we cannot read an old value.
>>   */
> 
> I seriously doubt the usefulness of repeating this over and over...


This had been done wrong over and over. Why so, if everybody knows this
by heart anyway? Or, if you mean that writing it once is sufficient,
where to put it then? If not mention it at every relevant place, what
place would everyone look for it when copying one of the sections?

...
>> === modified file 'sql/sql_class.cc'
...
>> +/**
>> +  Awake a thread.
>> +
>> +  @param[in]    state_to_set            value for THD::killed
>> +
>> +  This is normally called from another thread's THD object.
>> +
>> +  @note Do not call this within LOCK_thread_count.
> 
> within sounds strange.


This note is obsolete anyway. I'll remove it. I don't use that mutex any
more.

> 
>> +  @note Do always call this within LOCK_delete.
>> +*/
>> +
>>   void THD::awake(THD::killed_state state_to_set)
>>   {
>>     DBUG_ENTER("THD::awake");
>>     DBUG_PRINT("enter", ("this: %p", this));
>>     THD_CHECK_SENTRY(this);
>> -  safe_mutex_assert_owner(&LOCK_delete);
>> +  safe_mutex_assert_owner(&LOCK_delete);
> 
> How about a safe_mutex_assert_not_owner for LOCK_thread_count?


I did so in my former patch. But since we have the mutex checker, I
cannot (and don't) use that mutex here any more anyway.

...
>> +        In addition to a signal, let's close the socket of the thread
>> +        that is being killed ("this", which is not the current thread).
>> +        This is to make sure it does not block if the signal is lost.
>> +        Another reason is that the target thread may not have been
> 
> may -> might


Ok, will change. I thought that "may" is a little stronger than "might".
According to Murphy these cases will happen at the worst possible time
and in the worst possible way.

...
>> === modified file 'sql/sql_class.h'
...
>> +  The only reliable workaround is to close the connection. This will
>> +  prevent read/recv() to block, and even awake them if they are blocked
>> +  already.
>> +*/
>> +#ifndef SIGNAL_WITH_VIO_CLOSE
>> +#define SIGNAL_WITH_VIO_CLOSE 1
>> +#endif
> 
> I prefer we get rid of SIGNAL_WITH_VIO_CLOSE.. it seems pointless now.


Ok. Will remove.

...

Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder,   Wolfgang Engels,   Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028
Thread
bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Struewing25 Feb
  • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Davi Arnaut5 May
    • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing5 May
      • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Konstantin Osipov5 May
        • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing6 May
          • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Konstantin Osipov6 May
            • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing6 May
              • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Konstantin Osipov6 May
                • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing7 May
                  • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing12 May
              • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Davi Arnaut6 May
      • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Davi Arnaut6 May
        • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing6 May
          • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Konstantin Osipov6 May
            • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Ingo Strüwing7 May
          • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780Davi Arnaut7 May