Re: bk commit into 5.0 tree (holyfoot:1.2608) BUG#33334

[Alexey Kopytov]

Hi Alexey,

Sorry for a very delayed review, please see my comments below.

holyfoot@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.0 repository of holyfoot.  When holyfoot does a push these changes
> will be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2008-02-27 00:00:14+04:00, holyfoot@stripped +3 -0
>   Bug #33334 mysqltest_embedded crashes when disconnecting before reap.
>   
>   Before breaking the connection we have to check that there's no query
>     executing at the moment. Otherwise it can lead to crash in embedded server.
> 
>   client/mysqltest.c@stripped, 2008-02-27 00:00:09+04:00, holyfoot@stripped +35 -9
>     Bug #33334 mysqltest_embedded crashes when disconnecting before reap.
>     
>     Wait until the query thread is finished before we break the connection.
>     Waiting part moved to a separate wait_query_thread_end() function
> 
>   mysql-test/r/flush.result@stripped, 2008-02-27 00:00:09+04:00, holyfoot@stripped +1
> -0
>     Bug #33334 mysqltest_embedded crashes when disconnecting before reap.
>     
>     test result
> 
>   mysql-test/t/flush.test@stripped, 2008-02-27 00:00:09+04:00, holyfoot@stripped +9 -0
>     Bug #33334 mysqltest_embedded crashes when disconnecting before reap.
>     
>     test case
> 
> diff -Nrup a/client/mysqltest.c b/client/mysqltest.c
> --- a/client/mysqltest.c	2007-12-17 17:17:02 +04:00
> +++ b/client/mysqltest.c	2008-02-27 00:00:09 +04:00
> @@ -542,9 +542,22 @@ static int do_send_query(struct st_conne
>    return 0;
>  }
>  
> +static void wait_query_thread_end(struct st_connection *con)
> +{
> +  if (!con->query_done)
> +  {
> +    pthread_mutex_lock(&con->mutex);
> +    while (!con->query_done)
> +      pthread_cond_wait(&con->cond, &con->mutex);
> +    pthread_mutex_unlock(&con->mutex);
> +  }
> +}
> +
>  #else /*EMBEDDED_LIBRARY*/
>  
>  #define do_send_query(cn,q,q_len,flags) mysql_send_query(&cn->mysql, q,
> q_len)
> +static void wait_query_thread_end(struct st_connection *con
> __attribute__((unused)))
> +{}
>

Isn't it better to *not* define this function at all for a non-embedded 
build, and put all calls to it into the corresponding #ifdef?


>  #endif /*EMBEDDED_LIBRARY*/
>  
> @@ -3939,7 +3952,12 @@ void do_close_connection(struct st_comma
>        con->mysql.net.vio = 0;
>      }
>    }
> -#endif
> +#endif /*EMBEDDED_LIBRARY*/
> +  /*
> +   As query could be still executed in a separate theread
> +   we need to check if the query's thread was finished and probably wait
> +  */
> +  wait_query_thread_end(con);

Same question. Why not just put it to an #ifdef for the embedded build?

>    if (con->stmt)
>      mysql_stmt_close(con->stmt);
>    con->stmt= 0;
> @@ -4229,6 +4247,9 @@ void do_connect(struct st_command *comma
>            (int) (sizeof(connections)/sizeof(struct st_connection)));
>    }
>  
> +#ifdef EMBEDDED_LIBRARY
> +  con_slot->query_done= 1;
> +#endif

Why do we need to do that before even initializing the connection?

>    if (!mysql_init(&con_slot->mysql))
>      die("Failed on mysql_init()");
>    if (opt_compress || con_compress)
> @@ -5714,19 +5735,12 @@ void run_query_normal(struct st_connecti
>        goto end;
>      }
>    }
> -#ifdef EMBEDDED_LIBRARY
>    /*
>     Here we handle 'reap' command, so we need to check if the
>     query's thread was finished and probably wait
>    */
>    else if (flags & QUERY_REAP_FLAG)
> -  {
> -    pthread_mutex_lock(&cn->mutex);
> -    while (!cn->query_done)
> -      pthread_cond_wait(&cn->cond, &cn->mutex);
> -    pthread_mutex_unlock(&cn->mutex);
> -  }
> -#endif /*EMBEDDED_LIBRARY*/
> +    wait_query_thread_end(cn);

Same question. If it only makes sense for embedded, why did you take it 
out of #ifdef?

>    if (!(flags & QUERY_REAP_FLAG))
>      DBUG_VOID_RETURN;
>  
> @@ -6653,6 +6667,18 @@ void mark_progress(struct st_command* co
>  
>  }
>  
> +
> +int main0(int argc, char **argv)
> +{
> +  FILE *fl= fopen("/home/hf/fl_param", "w");
> +  while (argc--)
> +  {
> +    fprintf(fl, "%s\n", *argv);
> +    argv++;
> +  }
> +  fclose(fl);
> +  return 0;
> +}
>  

Does this really need to be in the patch?

>  int main(int argc, char **argv)
>  {
> diff -Nrup a/mysql-test/r/flush.result b/mysql-test/r/flush.result
> --- a/mysql-test/r/flush.result	2007-11-22 16:18:18 +04:00
> +++ b/mysql-test/r/flush.result	2008-02-27 00:00:09 +04:00
> @@ -72,3 +72,4 @@ flush tables with read lock;
>  unlock tables;
>  drop table t1, t2;
>  set session low_priority_updates=default;
> +select benchmark(200, (select sin(1))) > 1000;
> diff -Nrup a/mysql-test/t/flush.test b/mysql-test/t/flush.test
> --- a/mysql-test/t/flush.test	2007-11-22 16:18:18 +04:00
> +++ b/mysql-test/t/flush.test	2008-02-27 00:00:09 +04:00
> @@ -164,4 +164,13 @@ drop table t1, t2;
>  
>  set session low_priority_updates=default;
>  
> +#
> +# Bug #33334 mysqltest_embedded crashes when disconnecting before reap
> +#
> +
> +connect (con1,localhost,root,,);
> +send select benchmark(200, (select sin(1))) > 1000;
> +disconnect con1;
> +connection default;
> +
>  # End of 5.0 tests
> 

Best regards,
Alexey.


-- 
Alexey Kopytov, Software Developer
MySQL AB, www.mysql.com

Are you MySQL certified?  www.mysql.com/certification