Hi,
This patch suppresses the check for engine capabilities for row events.
That means, no error message is produces if the storage engine on slave
is incapable of row-logging. Consider the following scenario:
Master: is in ROW mode, t1 is capable of row-logging.
Slave: is in MIXED or STATEMENT mode, t1 is not capable of row-logging.
Master executes a statement that updates t1.
Then: the statement executes fine on master.
On slave, the check for capabilities is suppressed, so no error message
is generated. The server will try to log row-based despite t1 is not
capable of it.
Based on that, I reject the patch.
I think that a correct fix should:
- Execute safe statements and unsafe statements as before
- Execute row events in the sql thread and BINLOG statements as
follows. If all storage engines are capable of row-logging, go ahead and
execute. If some storage engine is incapable of row-logging, produce an
error.
- Issue warnings in some cases where it currently does not issue warnings.
Tomorrow I'll add an explanation of how I think this should be solved in
the bug report.
/Sven
Mats Kindahl wrote:
> #At file:///home/bzr/bugs/b39934-5.1-5.1.29-rc/
>
> 2771 Mats Kindahl 2008-10-13
> Bug #39934: Slave stops for engine that only support row-based logging
>
> The slave SQL thread is now executing in STATEMENT mode, meaning that
> for engines that do not support statement-based replication, it will
> not be possible to switch to row-format.
>
> If a Rows_log_event arrives to the slave with rows to apply, lock_tables()
> will be executed which in turn will call lock_tables() to open the tables
> to feed the rows into.
>
> For engines that does not support statement-based replication, the slave
> will stop with an error message since it is not possible to switch to
> row format. However, since this use is supposed to feed rows into the
> table, there should be no problems in executing this event.
>
> Bug is fixed by extending lock_tables() and friends with an extra parameter
> check_caps that will check capabilities if TRUE, and not otherwise. The
> parameter is then set to FALSE for all calls from inside Rows_log_event
> (and the old version of the same), and subclasses.
>
> Bug is reported for 6.0 but is present in 5.1 as well, so patch is submitted
> for 5.1 but will be extended when merged to 6.0.
> modified:
> sql/ha_ndbcluster_binlog.cc
> sql/log_event.cc
> sql/log_event_old.cc
> sql/mysql_priv.h
> sql/sql_base.cc
> sql/sql_table.cc
>
> === modified file 'sql/ha_ndbcluster_binlog.cc'
> --- a/sql/ha_ndbcluster_binlog.cc 2008-02-26 16:34:02 +0000
> +++ b/sql/ha_ndbcluster_binlog.cc 2008-10-13 12:28:55 +0000
> @@ -2380,7 +2380,8 @@ int ndb_add_ndb_binlog_index(THD *thd, v
> goto add_ndb_binlog_index_err;
> }
>
> - if (lock_tables(thd, &binlog_tables, 1, &need_reopen))
> + if (lock_tables(thd, &binlog_tables, 1, &need_reopen,
> + /* check_caps */ FALSE))
> {
> if (need_reopen)
> {
>
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc 2008-10-07 13:21:17 +0000
> +++ b/sql/log_event.cc 2008-10-13 12:28:55 +0000
> @@ -7067,7 +7067,10 @@ int Rows_log_event::do_apply_event(Relay
> /* A small test to verify that objects have consistent types */
> DBUG_ASSERT(sizeof(thd->options) == sizeof(OPTION_RELAXED_UNIQUE_CHECKS));
>
> - if (simple_open_n_lock_tables(thd, rli->tables_to_lock))
> + /*
> + We do not check engine capabilities when replicating row events.
> + */
> + if (simple_open_n_lock_tables(thd, rli->tables_to_lock, FALSE))
> {
> uint actual_error= thd->main_da.sql_errno();
> if (thd->is_slave_error || thd->is_fatal_error)
>
> === modified file 'sql/log_event_old.cc'
> --- a/sql/log_event_old.cc 2008-05-12 17:50:53 +0000
> +++ b/sql/log_event_old.cc 2008-10-13 12:28:55 +0000
> @@ -76,7 +76,10 @@ Old_rows_log_event::do_apply_event(Old_r
> thd->set_current_stmt_binlog_row_based();
> }
>
> - if (simple_open_n_lock_tables(thd, rli->tables_to_lock))
> + /*
> + We do not check the engine capabilities when applying rows.
> + */
> + if (simple_open_n_lock_tables(thd, rli->tables_to_lock, FALSE))
> {
> uint actual_error= thd->main_da.sql_errno();
> if (thd->is_slave_error || thd->is_fatal_error)
>
> === modified file 'sql/mysql_priv.h'
> --- a/sql/mysql_priv.h 2008-10-02 12:53:08 +0000
> +++ b/sql/mysql_priv.h 2008-10-13 12:28:55 +0000
> @@ -1520,22 +1520,26 @@ void wait_for_condition(THD *thd, pthrea
> pthread_cond_t *cond);
> int open_tables(THD *thd, TABLE_LIST **tables, uint *counter, uint flags);
> /* open_and_lock_tables with optional derived handling */
> -int open_and_lock_tables_derived(THD *thd, TABLE_LIST *tables, bool derived);
> +int open_and_lock_tables_derived(THD *thd, TABLE_LIST *tables, bool derived,
> + bool check_caps= TRUE);
> /* simple open_and_lock_tables without derived handling */
> -inline int simple_open_n_lock_tables(THD *thd, TABLE_LIST *tables)
> +inline int simple_open_n_lock_tables(THD *thd, TABLE_LIST *tables,
> + bool check_caps= TRUE)
> {
> - return open_and_lock_tables_derived(thd, tables, FALSE);
> + return open_and_lock_tables_derived(thd, tables, FALSE, check_caps);
> }
> /* open_and_lock_tables with derived handling */
> -inline int open_and_lock_tables(THD *thd, TABLE_LIST *tables)
> +inline int open_and_lock_tables(THD *thd, TABLE_LIST *tables,
> + bool check_caps= TRUE)
> {
> - return open_and_lock_tables_derived(thd, tables, TRUE);
> + return open_and_lock_tables_derived(thd, tables, TRUE, check_caps);
> }
> /* simple open_and_lock_tables without derived handling for single table */
> TABLE *open_n_lock_single_table(THD *thd, TABLE_LIST *table_l,
> - thr_lock_type lock_type);
> + thr_lock_type lock_type, bool check_caps= TRUE);
> bool open_normal_and_derived_tables(THD *thd, TABLE_LIST *tables, uint flags);
> -int lock_tables(THD *thd, TABLE_LIST *tables, uint counter, bool *need_reopen);
> +int lock_tables(THD *thd, TABLE_LIST *tables, uint counter, bool *need_reopen,
> + bool check_caps= TRUE);
> int decide_logging_format(THD *thd, TABLE_LIST *tables);
> TABLE *open_temporary_table(THD *thd, const char *path, const char *db,
> const char *table_name, bool link_in_list);
>
> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc 2008-10-08 08:46:25 +0000
> +++ b/sql/sql_base.cc 2008-10-13 12:28:55 +0000
> @@ -4818,7 +4818,8 @@ static bool check_lock_and_start_stmt(TH
> */
>
> TABLE *open_n_lock_single_table(THD *thd, TABLE_LIST *table_l,
> - thr_lock_type lock_type)
> + thr_lock_type lock_type,
> + bool check_caps)
> {
> TABLE_LIST *save_next_global;
> DBUG_ENTER("open_n_lock_single_table");
> @@ -4834,7 +4835,7 @@ TABLE *open_n_lock_single_table(THD *thd
> table_l->required_type= FRMTYPE_TABLE;
>
> /* Open the table. */
> - if (simple_open_n_lock_tables(thd, table_l))
> + if (simple_open_n_lock_tables(thd, table_l, check_caps))
> table_l->table= NULL; /* Just to be sure. */
>
> /* Restore list. */
> @@ -4928,6 +4929,7 @@ TABLE *open_ltable(THD *thd, TABLE_LIST
> thd - thread handler
> tables - list of tables for open&locking
> derived - if to handle derived tables
> + check_caps - Check engine capabilities
>
> RETURN
> FALSE - ok
> @@ -4944,7 +4946,8 @@ TABLE *open_ltable(THD *thd, TABLE_LIST
> the third argument set appropriately.
> */
>
> -int open_and_lock_tables_derived(THD *thd, TABLE_LIST *tables, bool derived)
> +int open_and_lock_tables_derived(THD *thd, TABLE_LIST *tables, bool derived,
> + bool check_caps)
> {
> uint counter;
> bool need_reopen;
> @@ -4962,7 +4965,7 @@ int open_and_lock_tables_derived(THD *th
> my_sleep(6000000);
> thd->proc_info= old_proc_info;});
>
> - if (!lock_tables(thd, tables, counter, &need_reopen))
> + if (!lock_tables(thd, tables, counter, &need_reopen, check_caps))
> break;
> if (!need_reopen)
> DBUG_RETURN(-1);
> @@ -5186,6 +5189,7 @@ int decide_logging_format(THD *thd, TABL
> and therefore invoker should reopen tables and
> try to lock them once again (in this case
> lock_tables() will also return error).
> + check_caps Check engine capabilities
>
> NOTES
> You can't call lock_tables twice, as this would break the dead-lock-free
> @@ -5201,7 +5205,8 @@ int decide_logging_format(THD *thd, TABL
> -1 Error
> */
>
> -int lock_tables(THD *thd, TABLE_LIST *tables, uint count, bool *need_reopen)
> +int lock_tables(THD *thd, TABLE_LIST *tables, uint count, bool *need_reopen,
> + bool check_caps)
> {
> TABLE_LIST *table;
>
> @@ -5214,7 +5219,7 @@ int lock_tables(THD *thd, TABLE_LIST *ta
> *need_reopen= FALSE;
>
> if (!tables && !thd->lex->requires_prelocking())
> - DBUG_RETURN(decide_logging_format(thd, tables));
> + DBUG_RETURN(check_caps ? decide_logging_format(thd, tables) : 0);
>
> /*
> We need this extra check for thd->prelocked_mode because we want to avoid
> @@ -5353,7 +5358,7 @@ int lock_tables(THD *thd, TABLE_LIST *ta
> }
> }
>
> - DBUG_RETURN(decide_logging_format(thd, tables));
> + DBUG_RETURN(check_caps ? decide_logging_format(thd, tables) : 0);
> }
>
>
>
> === modified file 'sql/sql_table.cc'
> --- a/sql/sql_table.cc 2008-10-08 09:15:00 +0000
> +++ b/sql/sql_table.cc 2008-10-13 12:28:55 +0000
> @@ -7417,7 +7417,12 @@ bool mysql_checksum_table(THD *thd, TABL
>
> strxmov(table_name, table->db ,".", table->table_name, NullS);
>
> - t= table->table= open_n_lock_single_table(thd, table, TL_READ);
> + /*
> + Since the tables are opened in TL_READ_NO_INSERT, nothing will
> + be logged. Therefore, we can improve speed slightly by not
> + checking engine capabilities.
> + */
> + t= table->table= open_n_lock_single_table(thd, table, TL_READ, FALSE);
> thd->clear_error(); // these errors shouldn't get client
>
> protocol->prepare_for_resend();
>
>
--
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com
