Re: bzr commit into mysql-5.1 branch (mats:2771) Bug#39934

[Mats Kindahl]

Andrei Elkin wrote:
> Mats, hello.
> 
> I have some suggestions.
> Please find my further comments inlined and a summary at the end.
> 
>> #At file:///home/bzr/bugs/b39934-5.1-5.1.29-rc/

[snip]

>>   mysql-test/extra/rpl_tests/rpl_truncate.test
>>     Test now only test one binlog format instead of all three
>>     (the combinations file take care of testing all combinations).
> 
> Minor note:
> I think this particular test does not need the run time re-assignment
> to the binlog_format. Mentioning "combinations" sounds a bit
> misleading to me: one might start to think about a combination of
> the row-based master and the mixed slave.

I think it is quite clear what the combinations file is if you are working with
the test system.

>>   mysql-test/extra/rpl_tests/rpl_truncate_helper.test
>>     Removing explicit set of binlog format. Removing redundant binlog
>>     listing causing difference in result file between binlog
>>     formats.
> 
> I agree with the difference argument, but it's not obvious my
> redundant. Although I have guessed it out, it'd be a grace from you to
> explain why in the comments.

I'll clarify the comment.

>>   mysql-test/include/rpl_udf.inc
>>     Disabling warnings over code that generates warnings for some
>>     binlog formats but not for some.
>>   mysql-test/lib/mtr_report.pl
>>     rpl_udf and rpl_user_variables can generate harmless warnings.
>>   mysql-test/suite/rpl/t/rpl_bug31076.test
>>     Test only valid for mixed and row mode.
>>   mysql-test/suite/rpl/t/rpl_events.test
>>     Removing explicit set of binlog format and removing duplicate test
>>     (combinations file take care of testing all combinations).
>>   mysql-test/suite/rpl/t/rpl_extraColmaster_innodb.test
>>     Removing explicit set of binlog format and removing duplicate test
>>     (combinations file take care of testing all combinations).
>>   mysql-test/suite/rpl/t/rpl_extraColmaster_myisam.test
>>     Removing explicit set of binlog format and removing duplicate test
>>     (combinations file take care of testing all combinations).
>>   mysql-test/suite/rpl/t/rpl_found_rows.test
>>     Factoring out part that depends on mixed mode into separate test case.
>>   mysql-test/suite/rpl/t/rpl_found_rows_mixed.test
>>     New test case for parts of rpl_found_rows that are only relevant for mixed
> mode.
>>   mysql-test/suite/rpl/t/rpl_idempotency.test
> 
>>     Idempotency test is only relevant for row mode.
> 
> Sorry, it's not.
> 
> * `Slave_exec_mode'
> 
>      *Version Introduced* 6.0.5
>      *Variable Name*      `slave_exec_mode'
>      *Variable Scope*     Global
>      *Dynamic Variable*   Yes
>      *Value Set *         
>      Typeenumeration      
>      DefaultSTRICT Valid  
>      ValuesIDEMPOTENT,    
>      STRICT               
> 
>      Controls whether `IDEMPOTENT' or `STRICT' mode is used in
>      replication conflict resolution and error checking.  `IDEMPOTENT'
>      mode causes suppression of some errors, including duplicate-key
>      and no-key-found errors. Beginning with MySQL 6.0.5, this mode
>      should be employed in multi-master replication, circular
>      replication, and some other special replication scenarios.
>      `STRICT' mode is the default, and is suitable for most other cases.
> 
>>     Removing explicit test of row format.
> 
> I think it's too bold declaration.
> 
> A section in the test file
> 
> # BUG#19958: RBR idempotency issue for UPDATE and DELETE
> 
> # Statement-based and row-based replication have different behavior
> # when updating a row with an explicit WHERE-clause that matches
> # exactly one row (or no row at all). For statement-based replication,
> # the statement is idempotent since the first time it is executed, it
> # will update exactly one row, and the second time it will not update
> # any row at all.
> 
> describes circumstances when the binlog format is statement and
> idempotency holds.
> I guess there is no reason per your bug to split this test apart with the
> binlog_format as a wedge.

Test reverted.

>>   mysql-test/suite/rpl/t/rpl_mix_insert_delayed.test
>>     Version of rpl_stm_insert_delayed to test mixed mode.
> 
>>   mysql-test/suite/rpl/t/rpl_rbr_to_sbr.test
>>     Removing explicit set of mixed mode and turning test into a mixed-mode only
> test.
> 
> It'd be nice if you say why it's okay to not execute the test with the
> statement format. As I see that is because of your fixes to
> the immediate problem. Changes to this test are okay to go to the
> bug#39934 fixes.
> 
>>   mysql-test/suite/rpl/t/rpl_row_max_relay_size.test
>>     Test only relevant for row mode, but requirement was missing.
> 
>>   mysql-test/suite/rpl/t/rpl_slave_skip.test
>>     Test only relevant for mixed mode.
> 
> It's not obvious, you need to prove that.

Because there are explicit sets of the binlog format to STATEMENT and ROW in the
file, meaning that the slave has to run in MIXED mode to be able to execute the
tests.

> 
> 
>>   mysql-test/suite/rpl/t/rpl_stm_insert_delayed.test
>>     Removing part of test that is only relevant for mixed mode and
>>     putting into separate file.
>>   mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test
>>     Test only relevant for mixed or row format.
> 
>>   mysql-test/suite/rpl/t/rpl_temporary_errors.test
>>     Removing explicit set of row format and turning test into
>>     row-mode only test.
> 
> And where are split-out mixed and statement parts of it?
> I assume that is 
>   set @@global.slave_exec_mode= 'IDEMPOTENT';
> which made you think on the row-basic specifics. Still, I don't think
> that's correct as said above.

No, it was because the format was explicitly set to ROW at the beginning of the
test.

>>   mysql-test/suite/rpl/t/rpl_truncate_2myisam.test
>>     Test only relevant for row mode.
>>   mysql-test/suite/rpl/t/rpl_truncate_3innodb.test
>>     Test only relevant for row mode.
> 
> Could you please explain why?

There is only a difference between the two statements for row format. However, I
replaced them with one version for each binlog mode instead.

[snip]

>>   sql/sql_class.h
>>     Adding THD::m_flags_all_set field with getter and setter.
> 
> I think you need to say what for. My version: in order to pass
> decide_logging_format() handlers capabilities parsing info down to
> execution path.

I'll extend the comment.

[snip]

>> === modified file 'mysql-test/extra/rpl_tests/rpl_insert_delayed.test'
>> --- a/mysql-test/extra/rpl_tests/rpl_insert_delayed.test	2007-07-27 14:29:48
> +0000
>> +++ b/mysql-test/extra/rpl_tests/rpl_insert_delayed.test	2008-11-17 10:34:29
> +0000
> 
> ok
> 
>> @@ -108,6 +108,7 @@ if  (`SELECT @@global.binlog_format != '
>>  {
>>    #must show two INSERT DELAYED
>>    --replace_column 1 x 2 x 3 x 4 x 5 x
>> +  --replace_regex /table_id: [0-9]+/table_id: #/
>>    show binlog events in 'master-bin.000002' LIMIT 2,2;
>>  }
>>  select * from t1;
> 
> I think a better option is to hide filtering rules and  extend 
> 
> show_binlog_events with an argument passing the binlog file name, by
> default 'master-bin.000001':
> 
> if (!$binlog_file_name)
> {
>   let $binlog_file_name="master-bin.000001";
> }
> ...
> show binlog events in $binlog_file_name ...
> 
> Why do not we change it here and in many other places as well?

Because patches should be minimal and this will cause every SHOW BINLOG EVENTS
statement in the a bunch of result files to be different. In addition, fixing
this problem this is not part of the bug fix, and I am reluctant to have
"feature creep" in bug fixes.

[snip]

>> === modified file 'mysql-test/lib/mtr_report.pl'
>> --- a/mysql-test/lib/mtr_report.pl	2008-10-02 07:46:14 +0000
>> +++ b/mysql-test/lib/mtr_report.pl	2008-11-17 10:34:29 +0000
> 
> 
> I am afraid it's too dangerous to disable warning for CREATE TABLE t1
> ENGINE=$engine_type.
> I am sure so many times your fingers got burned as well as mine when
> a table "mischievously" because of MyISAM type when it must have
> been INNODB.
> 
> Can't we move CREATE out of the disabled block?

OK

>> @@ -382,6 +382,13 @@ sub mtr_report_stats ($) {
>>                    /Slave: Cannot add or update a child row: a foreign key
> constraint fails .* Error_code: 1452/
>>  		 )) or
>>  
>> +                # These tests generate warnings since they write
>> +                # unsafe statements to the binary log
>> +                (($testname eq 'rpl.rpl_udf' or
>> +                  $testname eq 'rpl.rpl_user_variables') and
>> +                  /Unsafe statement binlogged as statement/
>> +                ) or
>> +
>>  		# These tests does "kill" on queries, causing sporadic errors when writing to
> logs
>>  		(($testname eq 'rpl.rpl_skip_error' or
>>  		  $testname eq 'rpl.rpl_err_ignoredtable' or
>>
>> === modified file 'mysql-test/suite/binlog/r/binlog_stm_ps.result'
>> --- a/mysql-test/suite/binlog/r/binlog_stm_ps.result	2008-03-25 13:28:12 +0000
>> +++ b/mysql-test/suite/binlog/r/binlog_stm_ps.result	2008-11-17 10:34:29 +0000
> 
> Where are you going to push the patch?
> For the rpl trees I suggest to inline suppressions in the mentioned
> tests instead of the global suppression (which is the only possible
> with the old mtr, afaik)

It is going to be pushed to bugteam, so that means old MTR. However, the tests
are not suppressed globally, only for those files mentioned in the clause.

>> @@ -11,7 +11,7 @@ prepare s from "insert into t1 select 10
>>  set @a=100;
>>  execute s using @a;
>>  Warnings:
>> -Warning	1592	Statement is not safe to log in statement format.
>> +Warning	1592	Unsafe statement binlogged as statement since binlog_format =
> STATEMENT.
>>  show binlog events from <binlog_start>;
>>  Log_name	Pos	Event_type	Server_id	End_log_pos	Info
>>  master-bin.000001	#	Query	#	#	use `test`; create table t1 (a int)
>>
>> === modified file 'mysql-test/suite/binlog/r/binlog_unsafe.result'
>> --- a/mysql-test/suite/binlog/r/binlog_unsafe.result	2008-03-26 09:56:03 +0000
>> +++ b/mysql-test/suite/binlog/r/binlog_unsafe.result	2008-11-17 10:34:29 +0000
> 
>> @@ -10,25 +10,25 @@ INSERT DELAYED INTO t1 VALUES (5);
>>  ---- Insert directly ----
>>  INSERT INTO t1 VALUES (@@global.sync_binlog);
>>  Warnings:
>> -Warning	1592	Statement is not safe to log in statement format.
>> +Warning	1592	Unsafe statement binlogged as statement since binlog_format =
> STATEMENT.
> 
> A matter of taste, still the text 
> 
> `Unsafe statement is binlogged as statement according to the binlog format'

I think it is a little unclear, I used this: "Unsafe statement is binlogged as
statement because binlog_format is STATEMENT." I think that is succinct and clear.

> 
> pleases my eyes more.

[snip]

>> === modified file 'mysql-test/suite/rpl/t/rpl_extraColmaster_innodb.test'
>> --- a/mysql-test/suite/rpl/t/rpl_extraColmaster_innodb.test	2007-10-10 14:43:20
> +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_extraColmaster_innodb.test	2008-11-17 10:34:29
> +0000
> 
>> @@ -3,14 +3,7 @@
>>  #############################################################
>>  -- source include/master-slave.inc
>>  -- source include/have_innodb.inc
> 
> You have said 
> 
>    >   mysql-test/suite/rpl/t/rpl_extraColmaster_innodb.test
>    >     Removing explicit set of binlog format and removing duplicate test
>    >     (combinations file take care of testing all combinations).
> 
> and now are removing the possibility to run this test with other than row
> format.
> 
>> +source include/have_binlog_format_row.inc;
> 
> "Combinations" won't help us to do it.
> 
> Please help me to understand.

Only row format supports extra columns in tables on slave, statement based
replication cannot handle it. Granted, the first comment should be different.

master> create table t1 (a int, b int);
Query OK, 0 rows affected (0.00 sec)

slave> alter table t1 drop b;
Query OK, 0 rows affected (0.01 sec)
Records: 0  Duplicates: 0  Warnings: 0

master> insert into t1 values (1,2);
Query OK, 1 row affected (0.00 sec)

master> select * from t1;
+------+------+
| a    | b    |
+------+------+
|    1 |    2 |
+------+------+
1 row in set (0.00 sec)

slave> select * from t1;
Empty set (0.00 sec)

slave> show slave status\G
*************************** 1. row ***************************
               Slave_IO_State: Waiting for master to send event
                       ...
             Slave_IO_Running: Yes
            Slave_SQL_Running: No
                       ...
               Last_SQL_Errno: 1136
               Last_SQL_Error: Error 'Column count doesn't match value count at
row 1' on query. Default database: 'test'. Query: 'insert into t1 values (1,2)'
1 row in set (0.00 sec)


[snip]

>> === modified file 'mysql-test/suite/rpl/t/rpl_extraColmaster_myisam.test'
>> --- a/mysql-test/suite/rpl/t/rpl_extraColmaster_myisam.test	2007-10-10 14:43:20
> +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_extraColmaster_myisam.test	2008-11-17 10:34:29
> +0000
> 
> 
>> @@ -2,14 +2,7 @@
>>  # Purpose: To test having extra columns on the master WL#3915
>>  #############################################################
>>  -- source include/master-slave.inc
>> +source include/have_binlog_format_row.inc;
> 
> similar to the innodb test problem.
> 
>>  
>>  let $engine_type = 'MyISAM';
>> -
>> -set binlog_format=row;
>> --- source extra/rpl_tests/rpl_extraMaster_Col.test
>> -
>> -set binlog_format=statement;
>> --- source extra/rpl_tests/rpl_extraMaster_Col.test
>> -
>> -set binlog_format=mixed;
>> --- source extra/rpl_tests/rpl_extraMaster_Col.test
>> +--source extra/rpl_tests/rpl_extraMaster_Col.test
>>
> 
> 
> I doubt the being split test stopped to pass with your fixed to the immediate
> problem of the bug.
> If it really did, should not `rpl_found_rows.test' be renamed
> 'rpl_found_rows_stm_and_row.test' to designate in the suffix part the
> same thing as the new name `rpl_found_rows_mixed.test' does? 

See Sven's comments...

[snip]

>> === modified file 'mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test'
>> --- a/mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test	2008-07-03 08:27:25
> +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_switch_stm_row_mixed.test	2008-11-17 10:34:29
> +0000
>> @@ -7,10 +7,14 @@
>>  #     e.g a query invokes UUID(), thereafter to serve as the definition
>>  #     of MIXED binlog format
>>  #   - correctness of execution
> 
>> +#
>> +# Since this test generates row-based events in the binary log, the
>> +# slave SQL thread cannot be in STATEMENT mode to execute this test.
> 
> That's understood.
> 
> A little confusion can arise though because the header mentions
> `STATEMENT' without specifying the format is exclusively for the
> master side:
> 
>    #
>    # rpl_switch_stm_row_mixed tests covers
>    #
>    #   - switching explicitly between STATEMENT, ROW, and MIXED binlog format
> 
> 
> Another point,

OK. Clarifying comments.

>>  -- source include/not_ndb_default.inc
>>  -- source include/master-slave.inc
> 
>> +-- source include/have_binlog_format_mixed_or_row.inc
> 
> should not it be
>  
>   +-- source include/have_binlog_format_mixed_or_row_on_slave.inc

Well.. that is written

connection slave;
-- source include/have_binlog_format_mixed_or_row.inc

but I can change to that.

> instead? I.e  a new macro that implements precisely your comments
> above about the slave. The master which can be in any mood that
> the test confirms with set @@binlog_format=...
> 
> The same seems to be applicable to other test with the comments "Since
> the master generates row-based events, the slave may not be in
> STATEMENT mode ..."

[snip]

>> === modified file 'mysql-test/suite/rpl/t/rpl_truncate_2myisam.test'
>> --- a/mysql-test/suite/rpl/t/rpl_truncate_2myisam.test	2007-06-27 12:28:02 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_truncate_2myisam.test	2008-11-17 10:34:29 +0000
>> @@ -1,4 +1,5 @@
>>  --source include/not_ndb_default.inc
>> +--source include/have_binlog_format_row.inc
> 
> I did not get why the test is row-based specific. You need to explain
> that.

See above.

[snip]

>> +  handler::Table_flags flags_all_set() const {
> 
> to me `get_flags_all_set()' looks nicer.
> I think the name would not far from perfect unless it contains
> "handlers". 
> 
> so 
> 
> I suggest {set_,get_}ha_binlog_flags.

IMHO, the "handler" is implicit since it is a member function in that class.

The "ha_" prefix used elsewhere is just a hack to allow the virtual function
with the corresponding name to be hidden.

I have no strong opinion on the "get_", so I can add it (to me, it seems
redundant and I feel that arbitrary annotations does not help when programming,
but that is just me.)

[snip]

> 
> A summary.
> 
> First of all, considering a number of concerns and questions 
> I would prefer if splitting apart tests, those that were not affected
> with the immediate problem fixes would be reviewed separately.
> 
> As to the immediate problem fixes,
> I like the idea of your decision diagram.
> Sven already requested to denote boldly what is the input and the
> output of the diagram.
> I would not call as row-injection what as I see is really an attribute
> of the Statement, namely the format attribute.
> Therefore, in my opinion, the diagram would look more readable if 
> on the place of Statement there were as many lines as many necessary
> to consider attributes the Statement has. As current there would be two
> two lines for two properties:
> safe/unsafe (determinism) and row/statement (format).

I think that for enumerable values, there is little point in having a separate
row for each enumeration value: it will just produce a diagonal with "Y".

> The new version of decide_logging_format() is much nicer! It also
> allows checking with gcov if all the branches are passable by our
> tests. I suggest to execute your patch with gcov to be ensured this
> critical part has the full test coverage.
> 
> Joining to Sven's request to point out explicilty what is a test case
> for the immediate problem of the bug.

Will do.

Just my few cents,
Mats
-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com