List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:January 26 2009 6:30pm
Subject:Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2749)
Bug#37780
View as plain text  
On 1/21/09 3:05 PM, Ingo Strüwing wrote:
> Hi Davi,
>
> thanks for the review. Please find comments below.
>
> Davi Arnaut, 20.01.2009 21:22:
>
> ...
>> On 1/11/09 4:05 PM, Ingo Struewing wrote:
> ...

[..]

>> The usual way to do this, as I've seen in other projects, is to use
>> non-blocking i/o and have a file descriptor for thread notification.
>> Have you considered this alternative?
>
>
>
> No. I want to keep the changes to the VIO subsystem as small as
> possible. But I agree that this would probably solve the problem ultimately.
>

Less is good, but not always.

>> Nonetheless, this approach of closing the socket needs more detailing:
>>
>> Is it portable? I mean, its it guaranteed (standard) that it will
>> interrupt a thread blocked on a read/write?
>
>
> No guarantee. It does definitely not work on Windows. But the change was
> meant to reduce the chances for missing a kill, not to make it 100%
> safe. The user can repeat the KILL statement anyway, but the tests
> should profit from a reduced chance to miss a kill.

OK, please make this clear in the changeset comment.

>> Will the client stay connected after the read direction is closed?
>
>
> Yes. Keeping the write direction open means that the client can still
> receive data. The worst thing that can happen is that the client gets an
> error on an attempt to write to the socket. If it interprets this as
> "Lost connection to server", then we've won anyway. Just an error
> message from the server might get lost.

My doubt is about a client polling his socket and bailing out in case 
poll returns a error if one of the directions is closed. This needs to 
be documented as a protocol change.

>> Why don't we just default to close the socket? Sending a error message
>> seems like a corner case that is only meaningful when the connection is
>> sitting idle.
>
>
> The client can read the error message from the socket whenever it is
> going to await a reply from the server. The socket buffers it until then.
>

OK.

>> Is there anything that prevents the thread being killed to try to read
>> or write to the "closed" socket? For example, a re-using case:
>>
>> thr1>  sleeping right before a write(fd=10)
>> thr2>  kill thr1
>> thr2>  close(fd=10)
>> thr0>  new connection, fd=accept()=10
>> thr1>  awakes, write(fd=10)
>
>
> That's an interesting scenario. Perhaps it is the reason why killing by
> close had been disabled some time ago.
>
> When closing the read direction only, the file descriptor remains in
> use. So we are safe in this case.

My worries are related to the case where the socket is closed.

> The above does also suggest not to default to a full close, if a
> half-close does not work.

OK.

>> What happens if we close one direction of a SSL connection?
>
>
> Hm. Good catch. While analyzing the code path of the server
> communication, I did not stumble over SSL. Now I looked a bit deeper.
>
> Net_serv.cc uses calls like vio_read() and vio_write(). Mysqld.cc and
> sql_class.cc use vio_close(). Functions of these names exist in
> viosocket.c. So I worked on these.
>
> After trying to find code for SSL handling, I stumbled over definitions
> in violite.h. The above mentioned "functions" are macros. In case of SSL
> they expand to vio_ssl_read(), vio_ssl_write(), and vio_ssl_close().
>
> So I will have to rework the patch to make vio_close_read() work with
> socket based connections, while returning error for other types. In case
> of SSL it will break the connection and prevent error message transport,
> but that's probably better than miss the kill.

OK.

[..]

>> I've moved the declarations to a mdl_sync.h header on December 12. You
>> might want to pull from main and drop the conflicting changes as mine
>> was done in a slightly different way.
>>
>> http://lists.mysql.com/commits/61466
>
>
> I would like to keep adding sync points as easy as possible. If
> debug_sync.h is included in mysql_priv.h, it is available in most files.
> No extra check for "#include<debug_sync.h>" needs to be done after
> adding a sync point. Since debug_sync.h has protection against double
> use, I'll leave the explicit uses in the files where you added them.

Please, don't add it back to mysql_priv.h.. easy of use is not a concern 
here.

> BTW. The quoted patch does not include debug_sync.h itself.

Yes, I fixed it later.

> You did also move the old macro DBUG_SYNC_POINT to debug_sync.h. This is
> an independent facility. But I verified that it is not in real use any
> more. So I will remove it together with its only use in sql_repl.cc,
> which is not triggered from tests or otherwise anyway.

OK.

>>> added:
>>>     sql/debug_sync.h
>>> modified:
>>>     client/mysql.cc
>>>     include/my_net.h
>>>     include/violite.h
>>>     mysql-test/r/kill.result
>>>     mysql-test/suite/backup/t/backup_triggers_and_events.test
>>>     mysql-test/t/debug_sync.test
>>>     mysql-test/t/kill.test
>>>     sql/CMakeLists.txt
>>>     sql/Makefile.am
>>>     sql/debug_sync.cc
>>>     sql/event_queue.cc
>>>     sql/lock.cc
>>>     sql/mdl.cc
>>>     sql/mysql_priv.h
>>>     sql/mysqld.cc
>>>     sql/slave.cc
>>>     sql/sql_base.cc
>>>     sql/sql_class.cc
>>>     sql/sql_class.h
>>>     sql/sql_parse.cc
>>>     sql/sql_select.cc
>>>     vio/viosocket.c
>>>
>>> per-file messages:
>>>     client/mysql.cc
>>>       Bug#37780 - main.kill fails randomly
>> Out of curiosity, why do you add the bug name to each per-file comment?
>
>
> Once upon a time, when we had Bitkeeper, I often used revtool on single
> files to find certain changes. Bug/WL numbers helped me to avoid to take
> the detour over the changeset comment to understand in which context it
> was added. This may work with bzr too, or will be added later.

gannotate has a "General" tab which shows the main changeset comment and 
a "Per-file" tab that shows the per-file comments for each line.

> ...
>>> --- a/client/mysql.cc    2008-12-05 01:05:05 +0000
>>> +++ b/client/mysql.cc    2009-01-11 18:05:39 +0000
>>> @@ -1133,10 +1133,13 @@ int main(int argc,char *argv[])
>>>      if (mysql_server_init(embedded_server_arg_count,
>>> embedded_server_args,
>>>                            (char**) embedded_server_groups))
>>>      {
>>> +    /* purecov: begin inspected */
>>>        put_error(NULL);
>>>        free_defaults(defaults_argv);
>>> +    batch_readline_end(status.line_buff);
>>>        my_end(0);
>>>        exit(1);
>>> +    /* purecov: end */
>> Unrelated, please commit separated so it can be pushed early.
>
>
> Reporting a bug for this, and waiting for it to become tagged by triage,
> may finally take longer than to have it included here.

OK to push separately. I didn't suggest a separate bug report.

> ...
>>> --- a/mysql-test/t/debug_sync.test    2008-04-29 09:22:04 +0000
>>> +++ b/mysql-test/t/debug_sync.test    2009-01-11 18:05:39 +0000
>>> @@ -206,7 +206,7 @@ SET GLOBAL DEBUG_SYNC= 'p0 CLEAR';
>>>    #       immediately after setting of the DEBUG_SYNC variable.
>>>    #       So it is executed before the SET statement ends.
>>>    #
>>> -# NOTE: There is only on global signal (say "signal post" or "flag
>>> mast").
>>> +# NOTE: There is only one global signal (say "signal post" or "flag
>>> mast").
>>>    #       A SIGNAL action writes its signal into it ("sets a flag").
>>>    #       The signal persists until explicitly overwritten.
>>>    #       To avoid confusion for later tests, it is recommended to clear
>> Feel free to push this kind of stuff anytime without approval.
>
>
> This is not what I have learned in my 5 years with MySQL. Never push
> anything without approval. The only exception are changes made during merge.
>
> While I would like to have a rule that allows to push trivial changes, I
> fear that it can go wrong to leave it to the developer to decide, what a
> "trivial" change is.

Right, developers can't be trusted! :)

>>> === modified file 'mysql-test/t/kill.test'
>>> --- a/mysql-test/t/kill.test    2008-05-25 07:19:02 +0000
>>> +++ b/mysql-test/t/kill.test    2009-01-11 18:05:39 +0000
>>> @@ -4,329 +4,594 @@
>>>    # This would work if mysqltest run would be threaded and handle each
>>>    # connection in a separate thread.
>>>    #
>>> --- source include/not_embedded.inc
>>> +--source include/not_embedded.inc
>>> +--source include/have_debug_sync.inc
>> Hum, we now restrict kill.test to debug builds. BTW, all tests really
>> needed a port to sync points?
>
>
> Yes. All tests had a sleep or multiple wait_conditions (short sleeps) in
> it. My goal was to replace them all. This made the test case much
> faster. OTOH each sleep can someday become too short. So syncing seems
> to be the right thing to do.
>
>> [..]
>>
>>> +
>>> +--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`
>>> +
>>> +    --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
>> Whats up with all those spaces? never seem this on test case and it
>> makes things harder to read.
>
>
> Oh. What a surprise. Everyone else, I have talked about this, agreed
> that indentation of the threads makes the tests better readable. Or are
> the empty lines disturbing you?
>

If its just me.. OK then.

>
>>> +  Note that it is important to repeat the test for thd->killed after
>>> +  enter_cond(). Otherwise the killing thread may kill this thread after
>>> +  it tested thd->killed in the loop condition and bfore it registered
>> bfore>  before
>>
>>> +  the condition variable and mutex in enter_cond(). In this case, the
>>> +  killing thread does not know that this thread is going to wait on a
>>> +  condition variable. It would just set THD::killed. But if we would not
>>> +  test it again, we would go asleep though we are killed. If the killing
>>> +  thread would kill us when we are after the second test, but still
>>> +  before sleeping, we hold the mutex, which is registered in mysys_var.
>>> +  The killing thread would try to acquire the mutex before signalling
>> signalling ->  signaling
>>
>>> +  the condition variable. Since the mutex is only released implicitly in
>>> +  pthread_cond_wait(), the signalling happens at the right place. We
>>> +  have a safe synchronization.
>> I think you should just drop the above paragraph and point out that
>> people should know how to properly use wait conditions.
>
>
> Not everybody is as familiar with "wait conditions" as you. IMHO we have
> much too few comments in the code. Many things are not properly
> documented. This may be a result from specialists thinking everybody
> knows, how things work.
>
> If people would "know how to properly use wait conditions", the mistakes
> of not checking thd->killed inside the "condition" would not have been done.
>
> Please allow me to leave the paragraph in.

OK, but if they didn't pay attention to the read and understand 
pthread_cond_wait documentation, I think they won't bother to read your 
long comments too.

>
>>> --- a/sql/event_queue.cc    2008-05-14 13:49:41 +0000
>>> +++ b/sql/event_queue.cc    2009-01-11 18:05:39 +0000
>>> @@ -723,6 +723,18 @@ Event_queue::unlock_data(const char *fun
>>>          abstime If not null then call pthread_cond_timedwait()
>>>          func    Which function is requesting cond_wait
>>>          line    On which line cond_wait is requested
>>> +
>>> +  @note 'killed': It is not sufficient to test thd->killed at the top of
>>> +  the loop (from which this function is called). If the thread is killed
>>> +  after it tested thd->killed and before it registered the mutex (here:
>>> +  LOCK_event_queue) 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. 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.
>>>    */
>> Let's shorten or drop this comment, its already explained in various
>> other places.
>
>
> I noticed the same when I read through my patch. But then I thought that
> a reader of the code won't read all these places in one go normally. So
> I think, better to repeat this explanation at different places than to
> risk someone to copy a section without the explanation and removing the
> extra check of thd->killed to "optimize" the code.
>
> ...
>>> --- a/sql/lock.cc    2008-10-23 16:29:52 +0000
>>> +++ b/sql/lock.cc    2009-01-11 18:05:39 +0000
>>> @@ -1127,6 +1127,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);
>>> +    DBUG_PRINT("info",
>>> +               ("waiting_for: %d  protect_against: %d",
>>> +                waiting_for_read_lock,
>>> protect_against_global_read_lock));
>>> +
>>> +    waiting_for_read_lock++;
>> Care to explain why this is being moved?
>
>
> I thought, I explained it enough in the code comment below:

OK.

>
> ...
>>>    #if defined(ENABLED_DEBUG_SYNC)
> ...
>>> +      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.
>
>
> If this is insufficient explanation, can you please try to say, what the
> open questions are?
>
> ...
>
>>> --- a/sql/mysqld.cc    2008-12-16 20:54:07 +0000
>>> +++ b/sql/mysqld.cc    2009-01-11 18:05:39 +0000
>>> @@ -5804,7 +5804,6 @@ enum options_mysqld
>>>      OPT_BACKUP_PROGRESS_LOG,
>>>      OPT_SLOW_LOG,
>>>      OPT_THREAD_HANDLING,
>>> -  OPT_INNODB_ROLLBACK_ON_TIMEOUT,
>>>      OPT_SECURE_FILE_PRIV,
>>>      OPT_MIN_EXAMINED_ROW_LIMIT,
>>>      OPT_LOG_SLOW_SLAVE_STATEMENTS,
>> Push separatedly. You don't need approval for trivial stuff.
>
>
> As mentioned before, I doubt it. I would at least once need agreement
> about this from my devlead. This has never happened yet. And I'm
> definitely not the only one, who does not push without approval of even
> trivial stuff.

OK from me to push separately.

>
>>> === modified file 'sql/slave.cc'
>>> --- a/sql/slave.cc    2008-12-08 11:29:15 +0000
>>> +++ b/sql/slave.cc    2009-01-11 18:05:39 +0000
>>> @@ -557,9 +557,17 @@ int start_slave_thread(pthread_handler h
>>>        while (start_id == *slave_run_id)
>>>        {
>>>          DBUG_PRINT("sleep",("Waiting for slave thread to start"));
>>> -      const char* old_msg = thd->enter_cond(start_cond,cond_lock,
>>> -                                            "Waiting for slave thread
>>> to start");
>>> -      pthread_cond_wait(start_cond,cond_lock);
>>> +      const char* old_msg= thd->enter_cond(start_cond,cond_lock,
>>> +                                           "Waiting for slave thread
>>> to start");
>>> +      /*
>>> +        It is not sufficient to test this at loop bottom. We must test
>>> +        it after registering the mutex in enter_cond(). If the kill
>>> +        happens after testing of thd->killed and before the mutex is
>>> +        registered, we would otherwise go waiting though thd->killed is
>>> +        set.
>>> +      */
>>> +      if (!thd->killed)
>>> +        pthread_cond_wait(start_cond,cond_lock);
>>>          thd->exit_cond(old_msg);
>>>          pthread_mutex_lock(cond_lock); // re-acquire it as exit_cond()
>>> released
>>>          if (thd->killed)
>> BTW, don't we need to release start_lock on this error path?
>
>
> Agree. Seems like the slave has never before been killed in this situation.
>
> ...
>>> +  @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.
>> Theorically, this only applies for the same mutex. Better stick to
>> pthread rules.
>
>
> Agree. But do you suggest a change here?

I need to think about it.

Regards,

-- Davi Arnaut
Thread
bzr commit into mysql-6.0-backup branch (ingo.struewing:2749) Bug#37780Ingo Struewing11 Jan
  • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2749)Bug#37780Davi Arnaut20 Jan
    • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2749)Bug#37780Ingo Strüwing21 Jan
      • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2749)Bug#37780Davi Arnaut26 Jan
        • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2749)Bug#37780Ingo Strüwing27 Jan
          • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2749) Bug#37780Paul DuBois27 Jan
          • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2749)Bug#37780Davi Arnaut28 Jan
    • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2749)Bug#37780Ingo Strüwing25 Feb