Re: bk commit into 6.0 tree (mats:1.2687) BUG#37221

[Andrei Elkin]

Mats, hello.

> Below is the list of changes that have just been committed into a local
> 6.0 repository of mats.  When mats 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-06-05 11:57:19+02:00, mats@mats-laptop.(none) +6 -0
>   Bug #37221: SET AUTOCOMMIT=1 does not commit binary log
>   

Actually there are two issues.


A  SET AUTOCOMMIT=1 does not commit a transaction started with BEGIN and
   therefore there is nothing in binlog, either engine. This contradicts
   to the current docs.

B  If a Falcon transaction is started with SET AUTOCOMMIT= 0 committing with
   SET AUTOCOMMIT=1 to engine works but nothing in binlog, as you are
   reporting.

I think both issues comprise the whole problem.


>   WHEN setting AUTOCOMMIT=1 after starting a transaction, the binary log
>   did not commit the outstanding transaction. The reason was that the binary
>   log commit function saw the values of the new settings, deciding that there
>   were nothing to commit.
>   

As we started discussing this issue on #rep on Thu, there is a list of
sql statements, incl SET AUTOCOMMIT=1, that commits implicitly:
"Statements That Cause an Implicit Commit".
Using such opportunity as having one of the list item not complying
with the specification, I'd suggest to tests the entire implicitly
committing list, not merely the immediate SET AUTOCOMMIT=1.
To motivate you in doing that, i let you know my tests already showed that
UNLOCK TABLES does not commit either.

>   Fixed the problem by moving the implicit commit to before the thread option
>   flags were changed, so that the binary log sees the old values of the flags
>   instead of the values they will take after the statement.

I doubt about this idea. Please read on.

>
>   mysql-test/extra/binlog_tests/autocommit.test@stripped, 2008-06-05 11:57:16+02:00,
> mats@mats-laptop.(none) +27 -0
>     New BitKeeper file ``mysql-test/extra/binlog_tests/autocommit.test''
>
>   mysql-test/extra/binlog_tests/autocommit.test@stripped, 2008-06-05 11:57:16+02:00,
> mats@mats-laptop.(none) +0 -0
>
>   mysql-test/suite/binlog/r/binlog_autocommit.result@stripped, 2008-06-05 11:57:16+02:00,
> mats@mats-laptop.(none) +72 -0
>     New BitKeeper file ``mysql-test/suite/binlog/r/binlog_autocommit.result''
>
>   mysql-test/suite/binlog/r/binlog_autocommit.result@stripped, 2008-06-05 11:57:16+02:00,
> mats@mats-laptop.(none) +0 -0
>
>   mysql-test/suite/binlog/t/binlog_autocommit.test@stripped, 2008-06-05 11:57:16+02:00,
> mats@mats-laptop.(none) +18 -0
>     New BitKeeper file ``mysql-test/suite/binlog/t/binlog_autocommit.test''
>
>   mysql-test/suite/binlog/t/binlog_autocommit.test@stripped, 2008-06-05 11:57:16+02:00,
> mats@mats-laptop.(none) +0 -0
>
>   mysql-test/suite/falcon/r/rpl_falcon_bug_36468.result@stripped, 2008-06-05
> 11:57:17+02:00, mats@mats-laptop.(none) +36 -0
>     New BitKeeper file ``mysql-test/suite/falcon/r/rpl_falcon_bug_36468.result''
>
>   mysql-test/suite/falcon/r/rpl_falcon_bug_36468.result@stripped, 2008-06-05
> 11:57:17+02:00, mats@mats-laptop.(none) +0 -0
>
>   mysql-test/suite/falcon/t/rpl_falcon_bug_36468.test@stripped, 2008-06-05 11:57:17+02:00,
> mats@mats-laptop.(none) +42 -0
>     New BitKeeper file ``mysql-test/suite/falcon/t/rpl_falcon_bug_36468.test''
>
>   mysql-test/suite/falcon/t/rpl_falcon_bug_36468.test@stripped, 2008-06-05 11:57:17+02:00,
> mats@mats-laptop.(none) +0 -0
>
>   sql/set_var.cc@stripped, 2008-06-05 11:57:16+02:00, mats@mats-laptop.(none) +12 -2
>     Moving implicit commit to be first in set_option_autocommit() so that all
>     involved committers see the old values of option flags.
>
> diff -Nrup a/mysql-test/extra/binlog_tests/autocommit.test
> b/mysql-test/extra/binlog_tests/autocommit.test
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/extra/binlog_tests/autocommit.test	2008-06-05 11:57:16 +02:00
> @@ -0,0 +1,27 @@
> +# First half of test: setting autocommit outside a transaction
> +RESET MASTER;
> +SET AUTOCOMMIT=0;
> +
> +INSERT INTO t1 VALUES (1);
> +--echo # Shall not commit the INSERT
> +SET AUTOCOMMIT=0;
> +source include/show_binlog_events.inc;
> +--echo # Shall commit the INSERT
> +SET AUTOCOMMIT=1;
> +source include/show_binlog_events.inc;
> +
> +# Second half of test: setting autocommit inside a transaction
> +RESET MASTER;
> +SET AUTOCOMMIT=0;
> +
> +BEGIN;
> +INSERT INTO t1 VALUES (2);
> +--echo # Shall not commit the INSERT
> +SET AUTOCOMMIT=0;
> +source include/show_binlog_events.inc;
> +--echo # Shall not commit the INSERT either since we are inside a transaction
> +SET AUTOCOMMIT=1;
> +source include/show_binlog_events.inc;
> +COMMIT;
> +--echo # Now the INSERT shall be in the binary log
> +source include/show_binlog_events.inc;
> diff -Nrup a/mysql-test/suite/binlog/r/binlog_autocommit.result
> b/mysql-test/suite/binlog/r/binlog_autocommit.result
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/binlog/r/binlog_autocommit.result	2008-06-05 11:57:16 +02:00
> @@ -0,0 +1,72 @@
> +CREATE TABLE t1 (id INT) ENGINE = InnoDB;
> +SET BINLOG_FORMAT = 'STATEMENT';
> +RESET MASTER;
> +SET AUTOCOMMIT=0;
> +INSERT INTO t1 VALUES (1);
> +# Shall not commit the INSERT
> +SET AUTOCOMMIT=0;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +# Shall commit the INSERT
> +SET AUTOCOMMIT=1;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Query	#	#	use `test`; BEGIN
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (1)
> +master-bin.000001	#	Xid	#	#	COMMIT /* XID */
> +RESET MASTER;
> +SET AUTOCOMMIT=0;
> +BEGIN;
> +INSERT INTO t1 VALUES (2);
> +# Shall not commit the INSERT
> +SET AUTOCOMMIT=0;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +# Shall not commit the INSERT either since we are inside a transaction
> +SET AUTOCOMMIT=1;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +COMMIT;
> +# Now the INSERT shall be in the binary log
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Query	#	#	use `test`; BEGIN
> +master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t1 VALUES (2)
> +master-bin.000001	#	Xid	#	#	COMMIT /* XID */
> +SET BINLOG_FORMAT = 'ROW';
> +RESET MASTER;
> +SET AUTOCOMMIT=0;
> +INSERT INTO t1 VALUES (1);
> +# Shall not commit the INSERT
> +SET AUTOCOMMIT=0;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +# Shall commit the INSERT
> +SET AUTOCOMMIT=1;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Query	#	#	use `test`; BEGIN
> +master-bin.000001	#	Table_map	#	#	table_id: # (test.t1)
> +master-bin.000001	#	Write_rows	#	#	table_id: # flags: STMT_END_F
> +master-bin.000001	#	Xid	#	#	COMMIT /* XID */
> +RESET MASTER;
> +SET AUTOCOMMIT=0;
> +BEGIN;
> +INSERT INTO t1 VALUES (2);
> +# Shall not commit the INSERT
> +SET AUTOCOMMIT=0;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +# Shall not commit the INSERT either since we are inside a transaction
> +SET AUTOCOMMIT=1;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +COMMIT;
> +# Now the INSERT shall be in the binary log
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Query	#	#	use `test`; BEGIN
> +master-bin.000001	#	Table_map	#	#	table_id: # (test.t1)
> +master-bin.000001	#	Write_rows	#	#	table_id: # flags: STMT_END_F
> +master-bin.000001	#	Xid	#	#	COMMIT /* XID */
> +DROP TABLE t1;
> diff -Nrup a/mysql-test/suite/binlog/t/binlog_autocommit.test
> b/mysql-test/suite/binlog/t/binlog_autocommit.test
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/binlog/t/binlog_autocommit.test	2008-06-05 11:57:16 +02:00
> @@ -0,0 +1,18 @@
> +# The purpose of this test is to test that setting autocommit does a
> +# commit of outstanding transactions and nothing is left pending in
> +# the transaction cache.
> +
> +source include/have_log_bin.inc;
> +source include/have_falcon.inc;
> +
> +# We need a transactional engine, so let's use Falcon
> +CREATE TABLE t1 (id INT) ENGINE = Falcon;
> +
> +SET BINLOG_FORMAT = 'STATEMENT';
> +source extra/binlog_tests/autocommit.test;
> +
> +SET BINLOG_FORMAT = 'ROW';
> +source extra/binlog_tests/autocommit.test;
> +
> +# Cleaning up
> +DROP TABLE t1;
> diff -Nrup a/mysql-test/suite/falcon/r/rpl_falcon_bug_36468.result
> b/mysql-test/suite/falcon/r/rpl_falcon_bug_36468.result
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/falcon/r/rpl_falcon_bug_36468.result	2008-06-05 11:57:17
> +02:00
> @@ -0,0 +1,36 @@
> +stop slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +reset master;
> +reset slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +start slave;
> +[master]
> +SET GLOBAL BINLOG_FORMAT = 'ROW';
> +CREATE TABLE IF NOT EXISTS t1 (id INT) ENGINE = Falcon;
> +SET AUTOCOMMIT=0;
> +INSERT INTO t1 VALUES (1);
> +SET AUTOCOMMIT=1;
> +SELECT * FROM t1;
> +id
> +1
> +[slave]
> +SELECT * FROM t1;
> +id
> +1
> +[master]
> +BEGIN;
> +INSERT INTO t1 VALUES (2);
> +SET AUTOCOMMIT=1;
> +[slave]
> +SELECT * FROM t1 /* Should not contain 2 */;
> +id
> +1
> +[master]
> +INSERT INTO t1 VALUES (3);
> +COMMIT;
> +[slave]
> +SELECT * FROM t1 /* Should contain 2 and 3 */;
> +id
> +1
> +2
> +3
> diff -Nrup a/mysql-test/suite/falcon/t/rpl_falcon_bug_36468.test
> b/mysql-test/suite/falcon/t/rpl_falcon_bug_36468.test
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/falcon/t/rpl_falcon_bug_36468.test	2008-06-05 11:57:17 +02:00
> @@ -0,0 +1,42 @@
> +source include/master-slave.inc;
> +
> +--echo [master]
> +connection master;
> +SET GLOBAL BINLOG_FORMAT = 'ROW';
> +
> +CREATE TABLE IF NOT EXISTS t1 (id INT) ENGINE = Falcon;
> +
> +# When switching to AUTOCOMMIT=1, there is an implicit commit if we
> +# are outside a real transaction.
> +
> +SET AUTOCOMMIT=0;
> +INSERT INTO t1 VALUES (1);
> +SET AUTOCOMMIT=1;
> +SELECT * FROM t1;
> +--echo [slave]
> +sync_slave_with_master;
> +SELECT * FROM t1;
> +
> +# When inside a transaction, AUTOCOMMIT=1 should not commit the
> +# transaction. 

According to the current docs it should commit.

>  # This is hard to test for sure (since delays might cause
> +# the transaction to not propagate fast enough), but it will not cause
> +# any false negatives (just false positives).
> +
> +--echo [master]
> +connection master;
> +BEGIN;
> +INSERT INTO t1 VALUES (2);
> +SET AUTOCOMMIT=1;
> +
> +--echo [slave]
> +connection slave;
> +SELECT * FROM t1 /* Should not contain 2 */;
> +
> +--echo [master]
> +connection master;
> +INSERT INTO t1 VALUES (3);
> +COMMIT;
> +
> +--echo [slave]
> +sync_slave_with_master;
> +SELECT * FROM t1 /* Should contain 2 and 3 */;
> diff -Nrup a/sql/set_var.cc b/sql/set_var.cc
> --- a/sql/set_var.cc	2008-05-21 12:04:21 +02:00
> +++ b/sql/set_var.cc	2008-06-05 11:57:16 +02:00
> @@ -2970,6 +2970,18 @@ static bool set_option_autocommit(THD *t
>  
>    ulonglong org_options= thd->options;
>  
> +  /*
> +    If we are not in an explicit transaction (started with a BEGIN)
> +    and setting AUTOCOMMIT=1, then we need to commit any outstanding
> +    transactions. If not, the binary log commit function will not
> +    empty the transaction cache since it does not see the end of a
> +    transaction.
> +   */

In my opinion the A part of the problem needs addressing.
In your view such transaction that started with BEGIN goes on
"ignoring" SET AUTOCOMMIT=1.

I suggest for us to check with Peter Gulutzan how to proceed.

Obviously, with the change below a transaction

   begin; 
   insert into t set a=1;
   set autocommit=1;

won't commit with the last statement.
But it should as the docs say.

> +  if (!(thd->options & OPTION_BEGIN) &&
> +      var->save_result.ulong_value != 0 &&
> +      ha_commit(thd))
> +    return 1;
> +
>    if (var->save_result.ulong_value != 0)
>      thd->options&= ~((sys_var_thd_bit*) var->var)->bit_flag;
>    else
> @@ -2983,8 +2995,6 @@ static bool set_option_autocommit(THD *t
>        thd->options&= ~(ulonglong) (OPTION_BEGIN | OPTION_KEEP_LOG);
>        thd->transaction.all.modified_non_trans_table= FALSE;
>        thd->server_status|= SERVER_STATUS_AUTOCOMMIT;
> -      if (ha_commit(thd))
> -	return 1;

Let's clear air regarding to A.

cheers,

Andrei