List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:May 5 2009 11:03pm
Subject:Re: bzr commit into mysql-6.0 branch (ingo.struewing:2779) Bug#37780
View as plain text  
On 5/5/09 4:33 PM, Ingo Strüwing wrote:
> 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

You should know this answer. Testing on Linux, for example, what does 
the trick is a combination of shutdown and close. Simply closing won't 
interrupt the thread. On the other hand, just a close causes a interrupt 
on Mac. So, this is definitely not portable, doesn't appear to be widely 
documented... so it all boils down to a best effort.

> vio_close" is not portable, we shouldn't use vio_close() at all. But how
> should we then close a connection at session end?

I said earlier that the ultimate and right solution is using 
notification (through a event fd), but i think we deemed it too complex 
for the scope of this bug fix.

> 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?

I made a simple using a thread that connected to a server and slept 
reading from the socket. Once the thread was sleeping, the main thread 
would call close on the socket. Only with the close, nothing would 
happen and the thread would still wait for i/o. A shutdown followed by a 
close caused the thread to be interrupted.

It would be nice if you could confirm my results...

Anyway, this whole thing for me always sounded like a shot at 
interrupting a thread sleeping on a socket operation. I never found a 
clear and portable indication (docs) that this should work 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.

--send on a embedded library linked mysqltest uses threads. It has been 
this way for quite some time.

> ...
>>> +--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.

I mean, the test case still inserts the connection id on a table as a 
mean to pass the id between sessions?

>>> +--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.

OTOH, i find more difficult to follow indentation mixed with switches.

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

Really? From a quick glance, I encountered only one. Do other people 
follow the same schema?

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

A large majority of MySQL developers?

> 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,

"code structure" is the demarcation used in programming languages.

> 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.

BTW, where is this indentation schema documented? Now i got curious how 
things are indented if there are ifs, loops, etc.

> 5. Test case coding style
> (http://forge.mysql.com/wiki/How_to_Create_Good_Tests) does not comment
> about indentation at all.

No comment at all? I think you missed the heading "1.1.1 Coding style".

 > 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?

I was under the impression that the sync point would always work. What 
we would sometimes miss testing is the "interrupt thread sleeping" part.

> 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.

I still don't like the two seconds sleep part.

> ...
>>> +    --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.

OK.

>>> +    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.

Yes, spacing.. but i guess you didn't modify the line then.. it was only 
diff moving the code around.

[..]

> ...
>>> === 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:

I don't remember memory barriers being mentioned in the pthread standard 
as a sure fact. IIRC, the only guarantee is that variables modified 
within a lock/unlock pair are visible to other threads that lock the 
same mutex. There is no mention that unrelated variables being visible...

>    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?

Who cares? It's a implementation detail. Besides, i doubt that your 
example is guaranteed to work... have you checked the standard?

Again, IMHO, memory barriers are a implementation detail of mutexes. We 
shouldn't rely on internal proprieties, we should rely on what is 
documented to work.

> ...
>>> +  @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?

Where the killed variable is defined.

> ...
>>> === 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".

I was looking at the tense, past seemed more appropriate.

Regards,

-- Davi Arnaut
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