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