List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:October 9 2007 8:48pm
Subject:Re: bk commit into 5.1 tree (anozdrin:1.2575) BUG#20023
View as plain text  
Alexander Nozdrin wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of alik. When alik 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, 2007-10-08 15:42:27+04:00, anozdrin@station. +4 -0
>   Fix for BUG#20023: mysql_change_user() resets the value
>   of SQL_BIG_SELECTS.
>   
>   The bug was that SQL_BIG_SELECTS was not properly set
>   in COM_CHANGE_USER.
>   
>   The fix is to update SQL_BIG_SELECTS properly.
> 
>   sql/mysql_priv.h@stripped, 2007-10-08 15:42:24+04:00, anozdrin@station. +0 -1
>     Cleanup: make prepare_new_connection_state() private for module.
> 
>   sql/sql_class.cc@stripped, 2007-10-08 15:42:24+04:00, anozdrin@station. +6 -0
>     Update THD::options with the respect to SQL_BIG_SELECTS
>     in COM_CHANGE_USER.
> 
>   sql/sql_connect.cc@stripped, 2007-10-08 15:42:24+04:00, anozdrin@station. +1 -1
>     Cleanup: make prepare_new_connection_state() private for module.
> 
>   tests/mysql_client_test.c@stripped, 2007-10-08 15:42:25+04:00, anozdrin@station. +160
> -0
>     Add a test case BUG#20023.
> 
> diff -Nrup a/sql/mysql_priv.h b/sql/mysql_priv.h
> --- a/sql/mysql_priv.h	2007-09-10 22:53:16 +04:00
> +++ b/sql/mysql_priv.h	2007-10-08 15:42:24 +04:00
> @@ -917,7 +917,6 @@ void decrease_user_connections(USER_CONN
>  void thd_init_client_charset(THD *thd, uint cs_number);
>  bool setup_connection_thread_globals(THD *thd);
>  bool login_connection(THD *thd);
> -void prepare_new_connection_state(THD* thd);
>  void end_connection(THD *thd);

OK.

>  bool mysql_create_db(THD *thd, char *db, HA_CREATE_INFO *create, bool silent);
> diff -Nrup a/sql/sql_class.cc b/sql/sql_class.cc
> --- a/sql/sql_class.cc	2007-08-31 22:13:22 +04:00
> +++ b/sql/sql_class.cc	2007-10-08 15:42:24 +04:00
> @@ -585,6 +585,12 @@ void THD::init(void)
>    if (variables.sql_mode & MODE_NO_BACKSLASH_ESCAPES)
>      server_status|= SERVER_STATUS_NO_BACKSLASH_ESCAPES;
>    options= thd_startup_options;
> +
> +  if (variables.max_join_size == HA_POS_ERROR)
> +    options |= OPTION_BIG_SELECTS;
> +  else
> +    options &= ~OPTION_BIG_SELECTS;
> +
>    transaction.all.modified_non_trans_table=
> transaction.stmt.modified_non_trans_table= FALSE;
>    open_options=ha_open_options;
>    update_lock_default= (variables.low_priority_updates ?

Out of curiosity, can't we remove this same check from
prepare_new_connection_state() now?

> diff -Nrup a/sql/sql_connect.cc b/sql/sql_connect.cc
> --- a/sql/sql_connect.cc	2007-08-28 00:31:26 +04:00
> +++ b/sql/sql_connect.cc	2007-10-08 15:42:24 +04:00
> @@ -995,7 +995,7 @@ void end_connection(THD *thd)
>    Initialize THD to handle queries
>  */
>  
> -void prepare_new_connection_state(THD* thd)
> +static void prepare_new_connection_state(THD* thd)
>  {
>    Security_context *sctx= thd->security_ctx;

OK.

> diff -Nrup a/tests/mysql_client_test.c b/tests/mysql_client_test.c
> --- a/tests/mysql_client_test.c	2007-09-28 23:30:52 +04:00
> +++ b/tests/mysql_client_test.c	2007-10-08 15:42:25 +04:00
> @@ -16819,6 +16819,165 @@ static void test_bug30472()
>    mysql_close(&con);
>  }
>  
> +static void test_bug20023()
> +{
> +  MYSQL con;
> +  MYSQL_RES *rs;
> +  MYSQL_ROW row;
> +
> +  int sql_big_selects_orig;
> +  int max_join_size_orig;
> +
> +  int sql_big_selects_2;
> +  int sql_big_selects_3;
> +  int sql_big_selects_4;
> +  int sql_big_selects_5;
> +
> +  char query_buffer[MAX_TEST_QUERY_LENGTH];
> +
> +  /* Create a new connection. */
> +
> +  DIE_UNLESS(mysql_init(&con));
> +
> +  DIE_UNLESS(mysql_real_connect(&con,
> +                                opt_host,
> +                                opt_user,
> +                                opt_password,
> +                                opt_db ? opt_db : "test",
> +                                opt_port,
> +                                opt_unix_socket,
> +                                CLIENT_FOUND_ROWS));
> +
> +  /***********************************************************************
> +   Remember original SQL_BIG_SELECTS, MAX_JOIN_SIZE values.
> +  ***********************************************************************/

Try to follow the file comments style:

/* Remember original SQL_BIG_SELECTS, MAX_JOIN_SIZE values. */

> +  DIE_IF(mysql_query(&con, "SELECT @@SQL_BIG_SELECTS, @@global.max_join_size,
> @@session.max_join_size"));
> +  DIE_UNLESS(rs= mysql_store_result(&con));
> +  DIE_UNLESS(row= mysql_fetch_row(rs));
> +  sql_big_selects_orig= atoi(row[0]);
> +  max_join_size_orig= atoi(row[1]);
> +  mysql_free_result(rs);
> +
> +  /***********************************************************************
> +   Test that COM_CHANGE_USER resets the SQL_BIG_SELECTS to the initial value.
> +  ***********************************************************************/
> +
> +  /* Call mysql_change_user() with the same username, password, database. */
> +
> +  DIE_IF(mysql_change_user(&con,
> +                           opt_user,
> +                           opt_password,
> +                           opt_db ? opt_db : "test"));
> +
> +  /* Query SQL_BIG_SELECTS. */
> +
> +  DIE_IF(mysql_query(&con, "SELECT @@SQL_BIG_SELECTS, @@global.max_join_size,
> @@session.max_join_size"));
> +  DIE_UNLESS(rs= mysql_store_result(&con));
> +  DIE_UNLESS(row= mysql_fetch_row(rs));
> +  sql_big_selects_2= atoi(row[0]);
> +  mysql_free_result(rs);

How about moving this code and other almost equal to their own function?
 Maybe something like this (it makes the code more readable):

static void query_bigselect_joinsize(MYSQL *con, int *big_selects, int
*max_join_size)
{
  MYSQL_RES *rs;
  MYSQL_ROW row;

  /* Query SQL_BIG_SELECTS. */
  DIE_IF(mysql_query(&con, "SELECT @@SQL_BIG_SELECTS,
@@global.max_join_size, @@session.max_join_size"));
  DIE_UNLESS(rs= mysql_store_result(&con));
  DIE_UNLESS(row= mysql_fetch_row(rs));
  if (big_selects)
    *big_selects= atoi(row[0]);
  .. others ..
  mysql_free_result(rs);
}

> +  /* Check that SQL_BIG_SELECTS is reset properly. */
> +
> +  DIE_UNLESS(sql_big_selects_orig == sql_big_selects_2);
> +
> +  /***********************************************************************
> +   Test that if MAX_JOIN_SIZE set to non-default value,
> +   SQL_BIG_SELECTS will be 0.
> +  ***********************************************************************/
> +
> +  /* Set MAX_JOIN_SIZE to some non-default value. */
> +
> +  DIE_IF(mysql_query(&con, "SET @@global.max_join_size = 10000"));
> +  DIE_IF(mysql_query(&con, "SET @@session.max_join_size = default"));
> +
> +  /* Call mysql_change_user() with the same username, password, database. */
> +
> +  DIE_IF(mysql_change_user(&con,
> +                           opt_user,
> +                           opt_password,
> +                           opt_db ? opt_db : "test"));
> +
> +  /* Query SQL_BIG_SELECTS. */
> +
> +  DIE_IF(mysql_query(&con, "SELECT @@SQL_BIG_SELECTS, @@global.max_join_size,
> @@session.max_join_size"));
> +  DIE_UNLESS(rs= mysql_store_result(&con));
> +  DIE_UNLESS(row= mysql_fetch_row(rs));
> +  sql_big_selects_3= atoi(row[0]);
> +  mysql_free_result(rs);
> +
> +  /* Check that SQL_BIG_SELECTS is 0. */
> +
> +  DIE_UNLESS(sql_big_selects_3 == 0);
> +
> +  /***********************************************************************
> +   Test that if MAX_JOIN_SIZE set to default value,
> +   SQL_BIG_SELECTS will be 1.
> +  ***********************************************************************/
> +
> +  /* Set MAX_JOIN_SIZE to the default value (-1). */
> +
> +  DIE_IF(mysql_query(&con, "SET @@global.max_join_size = -1"));
> +  DIE_IF(mysql_query(&con, "SET @@session.max_join_size = default"));
> +
> +  /* Call mysql_change_user() with the same username, password, database. */
> +
> +  DIE_IF(mysql_change_user(&con,
> +                           opt_user,
> +                           opt_password,
> +                           opt_db ? opt_db : "test"));
> +
> +  /* Query SQL_BIG_SELECTS. */
> +
> +  DIE_IF(mysql_query(&con, "SELECT @@SQL_BIG_SELECTS, @@global.max_join_size,
> @@session.max_join_size"));
> +  DIE_UNLESS(rs= mysql_store_result(&con));
> +  DIE_UNLESS(row= mysql_fetch_row(rs));
> +  sql_big_selects_4= atoi(row[0]);
> +  mysql_free_result(rs);
> +
> +  /* Check that SQL_BIG_SELECTS is 1. */
> +
> +  DIE_UNLESS(sql_big_selects_4 == 1);
> +
> +  /***********************************************************************
> +   Restore MAX_JOIN_SIZE.
> +   Check that SQL_BIG_SELECTS will be the original one.
> +  ***********************************************************************/
> +
> +  sprintf(query_buffer,
> +          "SET @@global.max_join_size = %d",
> +          (int) max_join_size_orig);
> +
> +  DIE_IF(mysql_query(&con, query_buffer));
> +  DIE_IF(mysql_query(&con, "SET @@session.max_join_size = default"));
> +
> +  /* Call mysql_change_user() with the same username, password, database. */
> +
> +  DIE_IF(mysql_change_user(&con,
> +                           opt_user,
> +                           opt_password,
> +                           opt_db ? opt_db : "test"));
> +
> +  /* Query SQL_BIG_SELECTS. */
> +
> +  DIE_IF(mysql_query(&con, "SELECT @@SQL_BIG_SELECTS, @@global.max_join_size,
> @@session.max_join_size"));
> +  DIE_UNLESS(rs= mysql_store_result(&con));
> +  DIE_UNLESS(row= mysql_fetch_row(rs));
> +  sql_big_selects_5= atoi(row[0]);
> +  mysql_free_result(rs);
> +
> +  /* Check that SQL_BIG_SELECTS is 1. */
> +
> +  DIE_UNLESS(sql_big_selects_5 == sql_big_selects_orig);
> +
> +  /***********************************************************************
> +   That's it. Cleanup.
> +  ***********************************************************************/

Useless comment?

> +  mysql_close(&con);
> +}
> +
>  
>  /*
>    Read and parse arguments and MySQL options from my.cnf
> @@ -17115,6 +17274,7 @@ static struct my_tests_st my_tests[]= {
>    { "test_bug29306", test_bug29306 },
>    { "test_change_user", test_change_user },
>    { "test_bug30472", test_bug30472 },
> +  { "test_bug20023", test_bug20023 },
>    { 0, 0 }
>  };
>  
> 

Thanks,

-- 
Davi Arnaut, Software Engineer
MySQL Inc, www.mysql.com

Are you MySQL certified?  www.mysql.com/certification
Thread
bk commit into 5.1 tree (anozdrin:1.2575) BUG#20023Alexander Nozdrin8 Oct
  • Re: bk commit into 5.1 tree (anozdrin:1.2575) BUG#20023Davi Arnaut9 Oct