Mats, hello.
To follow Sven's good comments ...
> 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.
If decide_logging_format() was aware of the event's format the slave thread is
executing we would add this requirement as a new rule to the
function's list, and that would be the perfect solution.
On the other hand, if the table (there can be exactly one table per
row-event) is incapable of row-logging why not to binlog the event verbatim?
If the engine won't really execute the row-event an error should appear because
of the execution not inability to binlog.
Hence, decide_logging_format() may stay event format agnostic and what
we could extend decide_logging_format() rules with smth like
R1: for the unsafe statement
if the global binlog_format = STATEMENT && thread is slave
binlog as STATEMENT
R2: for the row event
if the global binlog_format = STATEMENT && thread is slave
binlog as ROW /* regardless of the engine capability */
What's your impression for this pattern?
cheers,
Andrei
>
> - 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();
>>
>>
>
