Re: bzr commit into mysql-5.1 branch (aelkin:2719) Bug#38798

[Sven Sandberg]

Hi Andrei,

The source patch is good. Just some suggestions on the documentation in
the test case, and the thing we discussed on IRC.

/Sven


Andrei Elkin wrote:
> #At file:///home/andrei/MySQL/BZR/FIXES/5.1-bug38798-assert_binlog_open/
> 
>  2719 Andrei Elkin	2008-08-26
>       Bug #38798 Assertion mysql_bin_log.is_open() failed in
> binlog_trans_log_savepos()
>       
>       The assert is about binlogging must have been activated, but it was not
> actually according
>       to the reported how-to-repeat instuctions.
>       Analysis revealed that binlog_start_trans_and_stmt() was called without prior
> testing 
>       if binlogging is ON.
>       
>       Fixed with avoing entering binlog_start_trans_and_stmt() if binlog is not
> activated.
> added:
>   mysql-test/r/binlog_off.result
>   mysql-test/t/binlog_off-master.opt
>   mysql-test/t/binlog_off.test
> modified:
>   sql/sql_insert.cc
> 
> per-file messages:
>   mysql-test/r/binlog_off.result
>     new results file
>   mysql-test/t/binlog_off-master.opt
>     the option to deactivate binlogging
>   mysql-test/t/binlog_off.test
>     regression test for the bug
>   sql/sql_insert.cc
>     avoing entering binlog_start_trans_and_stmt() if binlog is not activated.
> === added file 'mysql-test/r/binlog_off.result'
> --- a/mysql-test/r/binlog_off.result	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/r/binlog_off.result	2008-08-26 17:01:49 +0000
> @@ -0,0 +1,6 @@
> +DROP TABLE IF EXISTS t1, t2;
> +set @@session.binlog_format=row;
> +create table t1 (a int);
> +insert into t1 values (1);
> +create table t2 select * from t1;
> +drop table t1, t2;
> 
> === added file 'mysql-test/t/binlog_off-master.opt'
> --- a/mysql-test/t/binlog_off-master.opt	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/t/binlog_off-master.opt	2008-08-26 17:01:49 +0000
> @@ -0,0 +1 @@
> +--loose-skip-log-bin
> 
> === added file 'mysql-test/t/binlog_off.test'
> --- a/mysql-test/t/binlog_off.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/t/binlog_off.test	2008-08-26 17:01:49 +0000
> @@ -0,0 +1,22 @@
> +#
> +# binlog_off.test purpose is check that algorithms dealing with binlogging
> +# are robbust to sustain --skip-log-bin
> +#

It is not completely clear what it means that "algorithms dealing with
binlogging are robust to sustain --skip-log-bin". I think a more direct
way to say this would be something like "Verify that the --skip-log-bin
flag works correctly and turns off binlogging".

If this is supposed to be a generic test case verifying that
--skip-log-bin works, I guess it would be good to add a check that no
binlog exists at the end of the test. Not needed for this bug though.

> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1, t2;
> +--enable_warnings
> +
> +#
> +# Bug #38798 Assertion mysql_bin_log.is_open() failed in 
> +#            binlog_trans_log_savepos()
> +# testing that there is no crash

It is not clear from reading the test case why there would be a crash
here. I would suggest to add a comment saying something like "Before
BUG#38798, the code for CREATE...SELECT called an internal function to
binlog the statement, even with --skip-log-bin. This caused an assertion
to be thrown since the binlog was not open."

> +
> +set @@session.binlog_format=row;
> +
> +create table t1 (a int);
> +insert into t1 values (1);
> +create table t2 select * from t1;
> +
> +# clean-up
> +drop table t1, t2;
> 
> === modified file 'sql/sql_insert.cc'
> --- a/sql/sql_insert.cc	2008-07-11 18:51:10 +0000
> +++ b/sql/sql_insert.cc	2008-08-26 17:01:49 +0000
> @@ -3523,7 +3523,8 @@ select_create::prepare(List<Item> &value
>      temporary table, we need to start a statement transaction.
>    */
>    if ((thd->lex->create_info.options & HA_LEX_CREATE_TMP_TABLE) == 0
> &&
> -      thd->current_stmt_binlog_row_based)
> +      thd->current_stmt_binlog_row_based &&
> +      (thd->options & OPTION_BIN_LOG) && mysql_bin_log.is_open())
>    {
>      thd->binlog_start_trans_and_stmt();
>    }
> @@ -3619,10 +3620,11 @@ select_create::binlog_show_create_table(
>    result= store_create_info(thd, &tmp_table_list, &query, create_info);
>    DBUG_ASSERT(result == 0); /* store_create_info() always return 0 */
>  
> -  thd->binlog_query(THD::STMT_QUERY_TYPE,
> -                    query.ptr(), query.length(),
> -                    /* is_trans */ TRUE,
> -                    /* suppress_use */ FALSE);
> +  if ((thd->options & OPTION_BIN_LOG) && mysql_bin_log.is_open())

I think the above is correct, but since thd->binlog_query is not guarded
by (thd->options & OPTION_BIN_LOG) anywhere else, I suggest removing
that check here.

> +    thd->binlog_query(THD::STMT_QUERY_TYPE,
> +                      query.ptr(), query.length(),
> +                      /* is_trans */ TRUE,
> +                      /* suppress_use */ FALSE);
>  }
>  
>  void select_create::store_values(List<Item> &values)
> 
> 

-- 
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com