He Zhenxing wrote:
> Hi Alfranio,
>
> Patch looks good, I have some minor comments in line.
>
> Alfranio Correia wrote:
>
>> #At
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-39393/mysql-5.1-bugteam/
> based on revid:sergey.glukhov@stripped
>>
>> 2802 Alfranio Correia 2009-03-28
>> BUG#39393 slave-skip-errors does not work when using ROW based replication
>>
>> RBR was not considering the option --slave-skip-errors.
>>
>> To fix the problem, we are reporting the ignored ERROR(s) as warnings thus
> avoiding
>> stopping the SQL Thread. Besides, it fixes the output of "SHOW VARIABLES
> LIKE
>> 'slave_skip_errors'" which was showing nothing when the value "all" was
> assigned
>> to --slave-skip-errors.
>>
>> @sql/log_event.cc
>> skipped rbr errors when the option skip-slave-errors is set.
>> @sql/slave.cc
>> fixed the output of for SHOW VARIABLES LIKE 'slave_skip_errors'"
>> @test-cases
>> fixed the output of rpl.rpl_idempotency
>> create a test case rpl_skiperrors_rbr
>> added:
>> mysql-test/suite/rpl/r/rpl_skiperrors_rbr.result
>> mysql-test/suite/rpl/t/rpl_skiperrors_rbr-slave.opt
>> mysql-test/suite/rpl/t/rpl_skiperrors_rbr.test
>> modified:
>> mysql-test/suite/rpl/r/rpl_idempotency.result
>> mysql-test/suite/rpl/t/rpl_idempotency.test
>> sql/log_event.cc
>> sql/slave.cc
>>
>
> [snip]
>
>
>> === added file 'mysql-test/suite/rpl/t/rpl_skiperrors_rbr-slave.opt'
>> --- a/mysql-test/suite/rpl/t/rpl_skiperrors_rbr-slave.opt 1970-01-01 00:00:00
> +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_skiperrors_rbr-slave.opt 2009-03-28 11:32:15
> +0000
>> @@ -0,0 +1 @@
>> +--slave_skip_errors=all
>>
>> === added file 'mysql-test/suite/rpl/t/rpl_skiperrors_rbr.test'
>> --- a/mysql-test/suite/rpl/t/rpl_skiperrors_rbr.test 1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_skiperrors_rbr.test 2009-03-28 11:32:15 +0000
>>
>
> I'd suggest to name it rpl_row_skip_error.test
>
ok.
>
>> @@ -0,0 +1,132 @@
>> +#################################################################
>> +# This test cases checks if slave-skip-errors works when
>> +# using ROW based by generating forcing duplicate keys
>> +# in the SLAVE.
>> +#
>> +# The following scenarios are checked:
>> +#
>> +# 1 - InnoDB with transactions, both commit and rollback.
>> +# 2 - InnoDB with an UPDATE on a SET of rows.
>> +# 3 - MyIsam with an UPDATE on a SET of rows.
>> +#
>> +#################################################################
>> +
>> +--source include/have_innodb.inc
>> +--source include/master-slave.inc
>> +
>>
>
> source include/have_binlog_format_row.inc;
>
>
I would keep as it is just to make sure that the result in row mode is
equal to the statement mode.
> [snip]
>
>
>> === modified file 'sql/log_event.cc'
>> --- a/sql/log_event.cc 2009-02-04 11:08:27 +0000
>> +++ b/sql/log_event.cc 2009-03-28 11:32:15 +0000
>> @@ -277,6 +277,47 @@ static void clear_all_errors(THD *thd, R
>> rli->clear_error();
>> }
>>
>> +inline int idempotent_error_code(int err_code)
>> +{
>> + int ret= 0;
>> +
>> + switch (err_code)
>> + {
>> + case 0:
>> + ret= 1;
>> + break;
>> + /*
>> + The following list of "idempotent" errors
>> + means that an error from the list might happen
>> + because of idempotent (more than once)
>> + applying of a binlog file.
>> + Notice, that binlog has a ddl operation its
>> + second applying may cause
>> +
>> + case HA_ERR_TABLE_DEF_CHANGED:
>> + case HA_ERR_CANNOT_ADD_FOREIGN:
>> +
>> + which are not included into to the list.
>> +
>> + Note that HA_ERR_RECORD_DELETED is not in the list since
>> + do_exec_row() should not return that error code.
>> + */
>> + case HA_ERR_RECORD_CHANGED:
>> + case HA_ERR_KEY_NOT_FOUND:
>> + case HA_ERR_END_OF_FILE:
>> + case HA_ERR_FOUND_DUPP_KEY:
>> + case HA_ERR_FOUND_DUPP_UNIQUE:
>> + case HA_ERR_FOREIGN_DUPLICATE_KEY:
>> + case HA_ERR_NO_REFERENCED_ROW:
>> + case HA_ERR_ROW_IS_REFERENCED:
>> + ret= 1;
>> + break;
>> + default:
>> + ret= 0;
>> + break;
>> + }
>> + return (ret);
>> +}
>>
>> /**
>> Ignore error code specified on command line.
>> @@ -301,14 +342,37 @@ inline int ignored_error_code(int err_co
>> return ((err_code == ER_SLAVE_IGNORED_TABLE) ||
>> (use_slave_mask && bitmap_is_set(&slave_error_mask,
> err_code)));
>> }
>> -#endif
>>
>> +/*
>> + This function converts an engine's error to a server error.
>> +
>> + If the thread does not have an error already reported, it tries to
>> + define it by calling the engine's method print_error. However, if a
>> + mapping is not found, it uses the ER_UNKNOWN_ERROR and prints out a
>> + warning message.
>> +*/
>> +int convert_row_error(int error, THD* thd, TABLE *table)
>>
>
> Although this function is now only used for RBR, I think what this
> function does is not specific to ROW format, so I think a name like
> 'convert_handler_error' or 'print_handler_error' is more appropriate.
>
ok !!! convert_handler_error(...).
>
>> +{
>> + uint actual_error= (thd->is_error() ? thd->main_da.sql_errno() :
>> + 0);
>> +
>> + if (actual_error == 0)
>>
>
> I would suggest to remove all the calls to table->file->print_error in
> Rows_log_event::write/delete/update_row, so that you can avoid the check
> here in this function.
>
I would keep as it is just to make sure that in the future someone
does change the code and call the print_error or simply forget
to print it.
>
>> + {
>> + table->file->print_error(error, MYF(0));
>> + actual_error= (thd->is_error() ? thd->main_da.sql_errno() :
>> + ER_UNKNOWN_ERROR);
>> + if (actual_error == ER_UNKNOWN_ERROR)
>> + if (global_system_variables.log_warnings)
>> + sql_print_warning("Unmapped error detected %d in RBR", error);
>>
>
> I think the warning message should be: "Unknown error detected '%d' in
> handler"
>
ok. I will change it.
> [snip]
>
>
>
Cheers.
