Hi Jasonh,
Thanks for the comments.
I merged the two test cases. There is a new patch available.
Cheers.
He Zhenxing wrote:
> Alfranio Correia wrote:
>
>> 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.
>>
>
> OK, then change the name of the test and the comment of the test.
>
> I noticed that there is already a test named rpl_skip_error.test, which
> only runs in STATEMENT format, and with the option
> --slave-skip-error=1062, maybe you can combine them into one test that
> runs for all format.
>
Sure. Thank you for pointing this out.
I missed it.
>
>>> [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.
>>
>
> OK
>
>
>>>
>>>
>>>> + {
>>>> + 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.
>>
>>
>
>
>
