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
> @@ -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;
[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.
> +{
> + 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.
> + {
> + 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"
[snip]
