List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:November 4 2010 8:15pm
Subject:Re: bzr commit into mysql-5.5-bugteam branch (guilhem:3115) Bug#57316
View as plain text  
Hi Guilhem,

Your patch looks correct. I have only minor comments (see further down) 
that I leave to you to decide whether you want to do something with.

The thing I do not like about the patch is that it introduces a the new 
system_variable autocommit while most of the code and the binlog is 
using the OPTION_NOT_AUTOCOMMIT bit. Feels a bit like we have this 
information stored in two places. I understand why you chose this and do 
not have any better suggestion for this.

So OK to commit!

Olav


Guilhem Bichot wrote:
> #At file:///home/mysql_src/bzrrepos_new/5.5-bugteam/ based on
> revid:georgi.kodinov@stripped
>
>  3115 Guilhem Bichot	2010-11-04
>       Fix for BUG#57316 "It is not clear how to disable autocommit"
>       add boolean command-line option --autocommit. See sys_vars.cc.
>      @ mysql-test/mysql-test-run.pl
>         do in --gdb like in --ddd: to let the developer debug the startup
>         phase (like command-line options parsing), don't "run".
>         It's the third time I do this change, it was previously lost
>         by merges, port of 6.0 to next-mr...
>      @ mysql-test/r/mysqld--help-notwin.result
>         new command-line option
>      @ mysql-test/r/mysqld--help-win.result
>         a Linux user's best guess at what the Windows result should be
>      @ mysql-test/suite/sys_vars/inc/autocommit_func2.inc
>         new test
>      @ mysql-test/suite/sys_vars/t/autocommit_func2-master.opt
>         test new option
>      @ mysql-test/suite/sys_vars/t/autocommit_func3-master.opt
>         test new option
>      @ sql/mysqld.cc
>         OPTION_AUTOCOMMIT is gone
>      @ sql/sql_class.cc
>         As we change option_bits, we need to keep "autocommit" in sync.
>      @ sql/sql_partition.cc
>         What matters to this partitioning quote is to have
>         the OPTION_QUOTE_SHOW_CREATE flag down, it's all
>         that append_identifier() uses. So we make it explicit.
>         If we kept the "=0", we would have option_bits and "autocommit"
>         not in sync, so we would need to set "autocommit" to 0,
>         then restore it (and so, first save it).
>      @ sql/sql_priv.h
>         "system_variables::option_bits & OPTION_AUTOCOMMIT" is replaced by
> system_variables::autocommit
>      @ sql/sys_vars.cc
>         Sys_var_bit doesn't support command-line options, and we want
>         one for "autocommit", so make autocommit a Sys_var_mybool.
>         autocommit used to be remembered with the
>         OPTION_NOT_AUTOCOMMIT flag in system_variables::option_bits;
>         we keep this, as it's used all around and also option_bits
>         is stored in the binlog. We keep the flag and the new
>         "autocommit" my_bool in sync all the time.
>         The OPTION_AUTOCOMMIT flag served only when updating @@autocommit,
>         we delete it as system_variables::autocommit can replace it.
>
>     added:
>       mysql-test/suite/sys_vars/inc/autocommit_func2.inc
>       mysql-test/suite/sys_vars/r/autocommit_func2.result
>       mysql-test/suite/sys_vars/r/autocommit_func3.result
>       mysql-test/suite/sys_vars/t/autocommit_func2-master.opt
>       mysql-test/suite/sys_vars/t/autocommit_func2.test
>       mysql-test/suite/sys_vars/t/autocommit_func3-master.opt
>       mysql-test/suite/sys_vars/t/autocommit_func3.test
>     modified:
>       mysql-test/mysql-test-run.pl
>       mysql-test/r/mysqld--help-notwin.result
>       mysql-test/r/mysqld--help-win.result
>       sql/mysqld.cc
>       sql/sql_class.cc
>       sql/sql_class.h
>       sql/sql_partition.cc
>       sql/sql_priv.h
>       sql/sys_vars.cc
> === modified file 'mysql-test/mysql-test-run.pl'
> --- a/mysql-test/mysql-test-run.pl	2010-10-26 06:31:22 +0000
> +++ b/mysql-test/mysql-test-run.pl	2010-11-04 14:13:43 +0000
> @@ -5317,8 +5317,7 @@ sub gdb_arguments {
>  	       "break mysql_parse\n" .
>  	       "commands 1\n" .
>  	       "disable 1\n" .
> -	       "end\n" .
> -	       "run");
> +	       "end\n");
>    }
>  
>    if ( $opt_manual_gdb )
>
> === modified file 'mysql-test/r/mysqld--help-notwin.result'
> --- a/mysql-test/r/mysqld--help-notwin.result	2010-09-30 13:52:39 +0000
> +++ b/mysql-test/r/mysqld--help-notwin.result	2010-11-04 14:13:43 +0000
> @@ -19,6 +19,8 @@ The following options may be given as th
>   --auto-increment-offset[=#] 
>   Offset added to Auto-increment columns. Used when
>   auto-increment-increment != 1
> + --autocommit        autocommit
> + (Defaults to on; use --skip-autocommit to disable.)
>   --automatic-sp-privileges 
>   Creating and dropping stored procedures alters ACLs
>   (Defaults to on; use --skip-automatic-sp-privileges to disable.)
> @@ -725,6 +727,7 @@ abort-slave-event-count 0
>  allow-suspicious-udfs FALSE
>  auto-increment-increment 1
>  auto-increment-offset 1
> +autocommit TRUE
>  automatic-sp-privileges TRUE
>  back-log 50
>  big-tables FALSE
>
> === modified file 'mysql-test/r/mysqld--help-win.result'
> --- a/mysql-test/r/mysqld--help-win.result	2010-10-05 12:26:49 +0000
> +++ b/mysql-test/r/mysqld--help-win.result	2010-11-04 14:13:43 +0000
> @@ -19,6 +19,8 @@ The following options may be given as th
>   --auto-increment-offset[=#] 
>   Offset added to Auto-increment columns. Used when
>   auto-increment-increment != 1
> + --autocommit        autocommit
> + (Defaults to on; use --skip-autocommit to disable.)
>   --automatic-sp-privileges 
>   Creating and dropping stored procedures alters ACLs
>   (Defaults to on; use --skip-automatic-sp-privileges to disable.)
> @@ -729,6 +731,7 @@ abort-slave-event-count 0
>  allow-suspicious-udfs FALSE
>  auto-increment-increment 1
>  auto-increment-offset 1
> +autocommit TRUE
>  automatic-sp-privileges TRUE
>  back-log 50
>  big-tables FALSE
>
> === added file 'mysql-test/suite/sys_vars/inc/autocommit_func2.inc'
> --- a/mysql-test/suite/sys_vars/inc/autocommit_func2.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/sys_vars/inc/autocommit_func2.inc	2010-11-04 14:13:43 +0000
> @@ -0,0 +1,33 @@
> +--source include/have_innodb.inc
> +
> +--disable_warnings
> +drop table if exists t1;
> +--enable_warnings
>   

I think you can drop this ^. Every test file should be responsible for 
cleaning up after itself.

> +
> +CREATE TABLE t1
> +(
> +id INT NOT NULL auto_increment,
> +PRIMARY KEY (id),
> +name varchar(30)
> +) ENGINE = INNODB;
> +
> +SELECT @@global.autocommit;
> +SELECT @@autocommit;
> +INSERT into t1(name) values('Record_1');
> +INSERT into t1(name) values('Record_2');
> +SELECT * from t1;
> +ROLLBACK;
> +SELECT * from t1;
> +
> +set @@global.autocommit = 1-@@global.autocommit;
> +set @@autocommit = 1-@@autocommit;
> +SELECT @@global.autocommit;
> +SELECT @@autocommit;
> +INSERT into t1(name) values('Record_1');
> +INSERT into t1(name) values('Record_2');
> +SELECT * from t1;
> +ROLLBACK;
> +SELECT * from t1;
> +
> +drop table t1;
>   

Tiny detail: consider using upper case for "DROP TABLE"?

> +set @@global.autocommit = 1-@@global.autocommit;
>
> === added file 'mysql-test/suite/sys_vars/r/autocommit_func2.result'
> --- a/mysql-test/suite/sys_vars/r/autocommit_func2.result	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/sys_vars/r/autocommit_func2.result	2010-11-04 14:13:43 +0000
> @@ -0,0 +1,47 @@
> +drop table if exists t1;
> +CREATE TABLE t1
> +(
> +id INT NOT NULL auto_increment,
> +PRIMARY KEY (id),
> +name varchar(30)
> +) ENGINE = INNODB;
> +SELECT @@global.autocommit;
> +@@global.autocommit
> +1
> +SELECT @@autocommit;
> +@@autocommit
> +1
> +INSERT into t1(name) values('Record_1');
> +INSERT into t1(name) values('Record_2');
> +SELECT * from t1;
> +id	name
> +1	Record_1
> +2	Record_2
> +ROLLBACK;
> +SELECT * from t1;
> +id	name
> +1	Record_1
> +2	Record_2
> +set @@global.autocommit = 1-@@global.autocommit;
> +set @@autocommit = 1-@@autocommit;
> +SELECT @@global.autocommit;
> +@@global.autocommit
> +0
> +SELECT @@autocommit;
> +@@autocommit
> +0
> +INSERT into t1(name) values('Record_1');
> +INSERT into t1(name) values('Record_2');
> +SELECT * from t1;
> +id	name
> +1	Record_1
> +2	Record_2
> +3	Record_1
> +4	Record_2
> +ROLLBACK;
> +SELECT * from t1;
> +id	name
> +1	Record_1
> +2	Record_2
> +drop table t1;
> +set @@global.autocommit = 1-@@global.autocommit;
>
> === added file 'mysql-test/suite/sys_vars/r/autocommit_func3.result'
> --- a/mysql-test/suite/sys_vars/r/autocommit_func3.result	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/sys_vars/r/autocommit_func3.result	2010-11-04 14:13:43 +0000
> @@ -0,0 +1,43 @@
> +drop table if exists t1;
> +CREATE TABLE t1
> +(
> +id INT NOT NULL auto_increment,
> +PRIMARY KEY (id),
> +name varchar(30)
> +) ENGINE = INNODB;
> +SELECT @@global.autocommit;
> +@@global.autocommit
> +0
> +SELECT @@autocommit;
> +@@autocommit
> +0
> +INSERT into t1(name) values('Record_1');
> +INSERT into t1(name) values('Record_2');
> +SELECT * from t1;
> +id	name
> +1	Record_1
> +2	Record_2
> +ROLLBACK;
> +SELECT * from t1;
> +id	name
> +set @@global.autocommit = 1-@@global.autocommit;
> +set @@autocommit = 1-@@autocommit;
> +SELECT @@global.autocommit;
> +@@global.autocommit
> +1
> +SELECT @@autocommit;
> +@@autocommit
> +1
> +INSERT into t1(name) values('Record_1');
> +INSERT into t1(name) values('Record_2');
> +SELECT * from t1;
> +id	name
> +3	Record_1
> +4	Record_2
> +ROLLBACK;
> +SELECT * from t1;
> +id	name
> +3	Record_1
> +4	Record_2
> +drop table t1;
> +set @@global.autocommit = 1-@@global.autocommit;
>
> === added file 'mysql-test/suite/sys_vars/t/autocommit_func2-master.opt'
> --- a/mysql-test/suite/sys_vars/t/autocommit_func2-master.opt	1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/sys_vars/t/autocommit_func2-master.opt	2010-11-04 14:13:43
> +0000
> @@ -0,0 +1 @@
> +--autocommit=1
>
> === added file 'mysql-test/suite/sys_vars/t/autocommit_func2.test'
> --- a/mysql-test/suite/sys_vars/t/autocommit_func2.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/sys_vars/t/autocommit_func2.test	2010-11-04 14:13:43 +0000
> @@ -0,0 +1 @@
> +--source suite/sys_vars/inc/autocommit_func2.inc
>
> === added file 'mysql-test/suite/sys_vars/t/autocommit_func3-master.opt'
> --- a/mysql-test/suite/sys_vars/t/autocommit_func3-master.opt	1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/sys_vars/t/autocommit_func3-master.opt	2010-11-04 14:13:43
> +0000
> @@ -0,0 +1 @@
> +--autocommit=0
>
> === added file 'mysql-test/suite/sys_vars/t/autocommit_func3.test'
> --- a/mysql-test/suite/sys_vars/t/autocommit_func3.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/sys_vars/t/autocommit_func3.test	2010-11-04 14:13:43 +0000
> @@ -0,0 +1 @@
> +--source suite/sys_vars/inc/autocommit_func2.inc
>
> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc	2010-10-08 14:52:39 +0000
> +++ b/sql/mysqld.cc	2010-11-04 14:13:43 +0000
> @@ -7264,7 +7264,8 @@ static int get_options(int *argc_ptr, ch
>    else
>      global_system_variables.option_bits&= ~OPTION_BIG_SELECTS;
>  
> -  if (global_system_variables.option_bits & OPTION_AUTOCOMMIT)
> +  // Take in account value of command-line option:
> +  if (global_system_variables.autocommit)
>      global_system_variables.option_bits&= ~OPTION_NOT_AUTOCOMMIT;
>    else
>      global_system_variables.option_bits|= OPTION_NOT_AUTOCOMMIT;
>
> === modified file 'sql/sql_class.cc'
> --- a/sql/sql_class.cc	2010-10-23 13:09:27 +0000
> +++ b/sql/sql_class.cc	2010-11-04 14:13:43 +0000
> @@ -3374,6 +3374,7 @@ void THD::restore_sub_statement_state(Su
>    count_cuted_fields= backup->count_cuted_fields;
>    transaction.savepoints= backup->savepoints;
>    variables.option_bits= backup->option_bits;
> +  variables.autocommit= !(variables.option_bits & OPTION_NOT_AUTOCOMMIT);
>    in_sub_stmt=      backup->in_sub_stmt;
>    enable_slow_log=  backup->enable_slow_log;
>    first_successful_insert_id_in_prev_stmt= 
>
> === modified file 'sql/sql_class.h'
> --- a/sql/sql_class.h	2010-10-23 13:09:27 +0000
> +++ b/sql/sql_class.h	2010-11-04 14:13:43 +0000
> @@ -449,6 +449,8 @@ typedef struct system_variables
>    my_bool old_alter_table;
>    my_bool old_passwords;
>    my_bool big_tables;
> +  /// Before using this, @sa Sys_var_autocommit and @sa fix_autocommit().
>   

Suggest you propose "///" to the coding style committee before starting 
using this one :-)
(you had good luck with your "//" proposal so this might go through too 
:-) ).

I think our coding style prefers using @see instead of @sa (tiny, tiny 
comments - feel free to ignore)

> +  my_bool autocommit;
>   
>  
>    plugin_ref table_plugin;
>  
>
> === modified file 'sql/sql_partition.cc'
> --- a/sql/sql_partition.cc	2010-10-27 07:32:26 +0000
> +++ b/sql/sql_partition.cc	2010-11-04 14:13:43 +0000
> @@ -1997,7 +1997,7 @@ static int add_part_field_list(File fptr
>      String field_string("", 0, system_charset_info);
>      THD *thd= current_thd;
>      ulonglong save_options= thd->variables.option_bits;
> -    thd->variables.option_bits= 0;
> +    thd->variables.option_bits&= ~OPTION_QUOTE_SHOW_CREATE;
>      append_identifier(thd, &field_string, field_str,
>                        strlen(field_str));
>      thd->variables.option_bits= save_options;
> @@ -2016,8 +2016,7 @@ static int add_name_string(File fptr, co
>    String name_string("", 0, system_charset_info);
>    THD *thd= current_thd;
>    ulonglong save_options= thd->variables.option_bits;
> -
> -  thd->variables.option_bits= 0;
> +  thd->variables.option_bits&= ~OPTION_QUOTE_SHOW_CREATE;
>    append_identifier(thd, &name_string, name,
>                      strlen(name));
>    thd->variables.option_bits= save_options;
>
> === modified file 'sql/sql_priv.h'
> --- a/sql/sql_priv.h	2010-10-07 08:24:44 +0000
> +++ b/sql/sql_priv.h	2010-11-04 14:13:43 +0000
> @@ -89,8 +89,7 @@
>  #define OPTION_FOUND_ROWS       (1ULL << 5)     // SELECT, user
>  #define OPTION_TO_QUERY_CACHE   (1ULL << 6)     // SELECT, user
>  #define SELECT_NO_JOIN_CACHE    (1ULL << 7)     // intern
> -/** always the opposite of OPTION_NOT_AUTOCOMMIT except when in fix_autocommit() */
> -#define OPTION_AUTOCOMMIT       (1ULL << 8)    // THD, user
> +// (1ULL << 8) is unused
>   

Maybe change this to "... is unused and can be re-used" to make it clear 
that it is really "free".

>  #define OPTION_BIG_SELECTS      (1ULL << 9)     // THD, user
>  #define OPTION_LOG_OFF          (1ULL << 10)    // THD, user
>  #define OPTION_QUOTE_SHOW_CREATE (1ULL << 11)   // THD, user, unused
>
> === modified file 'sql/sys_vars.cc'
> --- a/sql/sys_vars.cc	2010-10-08 12:22:22 +0000
> +++ b/sql/sys_vars.cc	2010-11-04 14:13:43 +0000
> @@ -2191,22 +2191,27 @@ static Sys_var_charptr Sys_time_format(
>  
>  static bool fix_autocommit(sys_var *self, THD *thd, enum_var_type type)
>  {
> +  /*
> +    We are after-update: "autocommit" contains the new value; "option_bits"
> +    contains the old value. With those two we can determine what transition is
> +    being done, and then update old with new.
> +  */
>    if (type == OPT_GLOBAL)
>    {
> -    if (global_system_variables.option_bits & OPTION_AUTOCOMMIT)
> +    if (global_system_variables.autocommit)
>        global_system_variables.option_bits&= ~OPTION_NOT_AUTOCOMMIT;
>      else
>        global_system_variables.option_bits|= OPTION_NOT_AUTOCOMMIT;
>      return false;
>    }
>  
> -  if (thd->variables.option_bits & OPTION_AUTOCOMMIT &&
> +  if (thd->variables.autocommit &&
>        thd->variables.option_bits & OPTION_NOT_AUTOCOMMIT)
>    { // activating autocommit
>  
>      if (trans_commit_stmt(thd) || trans_commit(thd))
>      {
> -      thd->variables.option_bits&= ~OPTION_AUTOCOMMIT;
> +      thd->variables.autocommit= false;
>        return true;
>      }
>      /*
> @@ -2226,7 +2231,7 @@ static bool fix_autocommit(sys_var *self
>      return false;
>    }
>  
> -  if (!(thd->variables.option_bits & OPTION_AUTOCOMMIT) &&
> +  if (!thd->variables.autocommit &&
>        !(thd->variables.option_bits & OPTION_NOT_AUTOCOMMIT))
>    { // disabling autocommit
>  
> @@ -2238,9 +2243,18 @@ static bool fix_autocommit(sys_var *self
>  
>    return false; // autocommit value wasn't changed
>  }
> -static Sys_var_bit Sys_autocommit(
> +/**
> +   As there is an autocommit flag in system_variables::option_bits,
> +   Sys_autocommit should be Sys_var_bit, but that doesn't support a
> +   command-line option; so it is a Sys_var_mybool
> +   (system_variables::autocommit).
> +   Thus we keep both system_variables members in sync: 'autocommit' is always
> +   equal to !(option_bits & OPTION_NOT_AUTOCOMMIT), except during a
> +   transition of @@autocommit's value. @sa fix_autocommit.
> +*/
>   

I think that according to our coding style there should only be two 
spaces in front of the comment text. I think also our coding style 
prefers using @see over @sa.

> +static Sys_var_mybool Sys_autocommit(
>         "autocommit", "autocommit",
> -       SESSION_VAR(option_bits), NO_CMD_LINE, OPTION_AUTOCOMMIT, DEFAULT(TRUE),
> +       SESSION_VAR(autocommit), CMD_LINE(OPT_ARG), DEFAULT(TRUE),
>         NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0), ON_UPDATE(fix_autocommit));
>  export sys_var *Sys_autocommit_ptr= &Sys_autocommit; // for sql_yacc.yy
>  
>
>   
> ------------------------------------------------------------------------
>
>

Thread
bzr commit into mysql-5.5-bugteam branch (guilhem:3115) Bug#57316Guilhem Bichot4 Nov
Re: bzr commit into mysql-5.5-bugteam branch (guilhem:3115) Bug#57316Olav Sandstaa4 Nov