Andrei Elkin wrote:
> Luis, hello.
>
> To follow our conversation, I run some tests to confirm that
> the unsafe my_error() due to the transaction isolation can be safely removed
> from ha_innobase::external_lock().
>
The call to my_error() could be removed if this what you meant to say.
However, the decision must be delegated to the engine.
I am forwarding an email that I've exchanged with Sven, Luis and Jasonh
about this.
Cheers.
> Instead of
>
> ERROR 1598 (HY000): Binary logging not possible. Message: Transaction level
> 'READ-COMMITTED' in InnoDB is not safe for binlog mode 'STATEMENT'
>
> the error message would be:
>
> ERROR 1598 (HY000): Binary logging not possible. Message: Statement-based format
> required for this statement, but not allowed by this combination of engines
>
> Certainly the former current is more readable. However, in principle
> engine could provide a message along with capabilities.
>
> Mentioning these facts and ideas, I don't mean the removal and extending
> capabilities report should be your bug's agenda. Personally I'm fine if we schedule
> that work separately, still urgent enough not to let other bug
> fixes to contribute more to ha_innodb.cc.
>
>
> Further please find some particularities.
>
>
>> #At file:///home/lsoares/Workspace/mysql-server/bugfix/42829/5.1-bt/ based on
> revid:joro@stripped
>>
>> 2950 Luis Soares 2009-06-20
>> BUG#42829: binlogging enabled for all schemas regardless of
>> binlog-db-db / binlog-ignore-db
>>
>> InnoDB will return an error if statement based replication is used
>> along with transaction isolation level READ-COMMITTED (or stricter),
>>
>
> it's `or weaker' not `or stricter'.
> E.g
>
> http://www.oracle.com/technology/sample_code/tech/java/codesnippet/j2ee/Trasactions/Transactions.html?_template=/ocom/print
> (Transaction Isolation).
>
>
>> even if the statement in question is filtered out according to the
>> binlog-do-db rules set. In this case, an error should not be printed.
>>
>> This patch addresses this issue by:
>>
>> 1. adding a innodb compatibility hook that checks if statement is
>> filtered (to be used when logging in STATEMENT mode).
>>
>
> s/hook/hack/
>
> reads much more fair to me.
>
>
>>
>> 2. extending the existing check in external_lock to verify binlog
>> filter hook before deciding to print an error.
>>
>> 3. changing decide_logging_format, so that if statement is ignored,
>> then no error is printed. This change also makes
>> decide_logging_format to calculate capabilities for both, ROW and
>> STMT formats always.
>>
>> Collecting STMT mode capabilities is done considering all engines
>> involved (this was the behavior before this patch).
>>
>>
>
>
>> Collecting ROW mode capabilities considers binlog filtering
>> rules. So, for each engine involved in the statement, its
>> capabilities are conly considered if the table does not belong to
>> a filtered database.
>>
>
> I can't find description of
> logics of
>
>
> + if ((log_as_row && !binlog_filter->db_ok(tables))
> goto end;
>
> which in human words `if any table's db is found filterable the whole
> statement is filterable' in the above paragraph.
>
>
>>
>> Decision on which capabilities to take (for checking if logging is
>> possible), depends on the logging format chosen.
>>
>> 4. extending rpl_filter with a new method that checks for a list of
>> tables if at least one of them belongs to a non filtered
>> database. This is useful to find out if a statement logged in ROW
>> mode has its all changes (possibly involving different databases)
>> filtered. Consider the following:
>>
>> UPDATE db1.t1, db2.t2 SET db1.t1.a=1, db2.t2.a=2;
>>
>> if db1 and db2 are filtered and ROW mode is used, then statement
>> is completely filtered from binlog. On the other hand, if only db1
>> was to be filtered, then changes to db2 will make it into the
>> binlog. This new method is most useful in decide_logging_format.
>>
>>
>> @ mysql-test/extra/rpl_tests/rpl_innodb_rc_do_db_cleanup.test
>> Clean up shared part of the test.
>> @ mysql-test/extra/rpl_tests/rpl_innodb_rc_do_db_mix_row.test
>> Part of the test for mixed and row mode.
>> @ mysql-test/extra/rpl_tests/rpl_innodb_rc_do_db_prepare.test
>> Shared part of the test. (Prepare section)
>> @ mysql-test/extra/rpl_tests/rpl_innodb_rc_do_db_stm.test
>> Part of the test for statement mode.
>> @ mysql-test/suite/rpl/t/rpl_innodb_rc_do_db_mix_row-master.opt
>> Option file stating which database log.
>> @ mysql-test/suite/rpl/t/rpl_innodb_rc_do_db_mix_row.test
>> Mixed and Row mode logging test.
>> @ mysql-test/suite/rpl/t/rpl_innodb_rc_do_db_stm-master.opt
>> Option file stating which database log.
>> @ mysql-test/suite/rpl/t/rpl_innodb_rc_do_db_stm.test
>> Statement mode logging test.
>> @ sql/rpl_filter.cc
>> Added Rpl_filter::db_ok(TABLE_LIST* tables) which checks if at least
>> one table in the list provided belongs to a database that is not
>> filtered out.
>> @ sql/rpl_filter.h
>> Added 'bool db_ok(TABLE_LIST *tables)' method declaration.
>> @ sql/sql_base.cc
>> Reworked decide_logging_format to take into account filtering rules.
>>
>> After this patch, it decides to skip error reporting if statement is
>> filtered out from binlog. Also, capabilities are calculated considering
>> filtering rules.
>> @ sql/sql_class.cc
>> Added thd_binlog_filter_ok to INNODB_COMPATIBILITY_HOOKS set.
>> @ storage/innobase/handler/ha_innodb.cc
>> Extended check in external_lock to take into consideration the
>> filtering when deciding to throw an error.
>> @ storage/innobase/handler/ha_innodb.h
>> Added declaration of new hook.
>>
>> added:
>> mysql-test/extra/rpl_tests/rpl_innodb_rc_do_db_cleanup.test
>> mysql-test/extra/rpl_tests/rpl_innodb_rc_do_db_mix_row.test
>> mysql-test/extra/rpl_tests/rpl_innodb_rc_do_db_prepare.test
>> mysql-test/extra/rpl_tests/rpl_innodb_rc_do_db_stm.test
>> mysql-test/suite/rpl/r/rpl_innodb_rc_do_db_mix_row.result
>> mysql-test/suite/rpl/r/rpl_innodb_rc_do_db_stm.result
>> mysql-test/suite/rpl/t/rpl_innodb_rc_do_db_mix_row-master.opt
>> mysql-test/suite/rpl/t/rpl_innodb_rc_do_db_mix_row.test
>> mysql-test/suite/rpl/t/rpl_innodb_rc_do_db_stm-master.opt
>> mysql-test/suite/rpl/t/rpl_innodb_rc_do_db_stm.test
>> modified:
>> sql/rpl_filter.cc
>> sql/rpl_filter.h
>> sql/sql_base.cc
>> sql/sql_class.cc
>> storage/innobase/handler/ha_innodb.cc
>> storage/innobase/handler/ha_innodb.h
>> === modified file 'sql/sql_base.cc'
>> --- a/sql/sql_base.cc 2009-05-30 13:32:28 +0000
>> +++ b/sql/sql_base.cc 2009-06-20 13:57:39 +0000
>> @@ -5050,7 +5050,6 @@ static void mark_real_tables_as_free_for
>> table->table->query_id= 0;
>> }
>>
>>
>
> ...
>
>
>> -
>> /**
>> Decide on logging format to use for the statement.
>>
>> @@ -5097,12 +5096,17 @@ int decide_logging_format(THD *thd, TABL
>> set with all the capabilities bits set and one with no
>> capabilities bits set.
>> */
>> + handler::Table_flags flags_some_set_row= 0;
>> handler::Table_flags flags_some_set= 0;
>> handler::Table_flags flags_all_set=
>> HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE;
>> + handler::Table_flags flags_all_set_row=
>> + HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE;
>>
>> + my_bool log_as_row= FALSE;
>> my_bool multi_engine= FALSE;
>> void* prev_ht= NULL;
>> + int error= 0;
>> for (TABLE_LIST *table= tables; table; table= table->next_global)
>> {
>> if (table->placeholder())
>> @@ -5119,8 +5123,22 @@ int decide_logging_format(THD *thd, TABL
>> if (prev_ht && prev_ht != table->table->file->ht)
>> multi_engine= TRUE;
>> prev_ht= table->table->file->ht;
>> + /*
>> + Collect capabilities for engines involved in the
>> + statement. These are the capabilities to check when logging
>> + in statement mode.
>> + */
>> flags_all_set &= flags;
>> flags_some_set |= flags;
>> + /*
>> + Collect capabilities for tables in databases that would not
>> + be filtered when logging in row mode.
>> + */
>> + if (binlog_filter->db_ok(table->db))
>> + {
>> + flags_all_set_row &= flags;
>> + flags_some_set_row |= flags;
>> + }
>> }
>> }
>>
>>
>
> Ok, falling into a binlog_filter do rule is equivalent to gaining
> the both capabilities.
> That almost says how to merge the flags_* with the new flags_*_row right away in the
> loop.
>
> Second point, the new binlog_filter->db_ok(tables) does not look necessary
> because
> the return value can be gained in the loop.
> TL_WRITE_ALLOW_WRITE narrows the scope to remove tables for SELECTing which are
> irrelevant for filtering.
>
>
>
>> @@ -5135,23 +5153,98 @@ int decide_logging_format(THD *thd, TABL
>> DBUG_PRINT("info", ("multi_engine: %s",
>> multi_engine ? "TRUE" : "FALSE"));
>>
>> - int error= 0;
>> + /**
>> + Flag stating whether this statement should be logged as row or
>> + not.
>> + */
>> + log_as_row= (thd->variables.binlog_format==BINLOG_FORMAT_ROW ||
>> + (thd->variables.binlog_format==BINLOG_FORMAT_MIXED &&
>> + (thd->lex->is_stmt_unsafe() ||
>> + (flags_all_set & HA_BINLOG_STMT_CAPABLE) ==0)));
>> +
>> + /**
>> + Check whether statement is filtered from binlog or not.
>> + */
>> + if ((log_as_row && !binlog_filter->db_ok(tables)) ||
>>
>
> I would enjoy gathering db_ok result in the above loop instead of the new method.
>
>
>> + (!log_as_row && !binlog_filter->db_ok(thd->db)))
>> + {
>> +
>> + /*
>> + We are filtering this statement from log. As such, we skip
>> + error checking.
>> +
>> + Still, we mark statement to be logged as ROW if in MIXED and
>> + switch should happen. This way, the statement is subject to
>> + the same filtering rules when the time comes to log it, that
>> + were considered here.
>> + */
>> + goto end;
>> + }
>> + else
>> + {
>> + /*
>> + Choose which capabilities to take based on the filtering
>> + analysis and the binlog format that one is to choose.
>> +
>> + So, if using RBR or MIXED+switch_to_row, the capabilities
>> + considered are the ones collected for events on *not* filtered
>> + tables (databases).
>> +
>> + For STMT or MIXED+not_switch_to_row, the capabilities to
>> + consider are the ones collected from all the engines involved.
>> +
>> + Q. What is this trying to solve?
>> + A. Some cases involving filtered and not filtered out
>> + databases, in the same statement and when using row mode
>> + logging. Consider scenario description and questions below.
>> +
>> + Scenario:
>> + 1. MIXED mode
>> + 2. filtered: db2; !filtered: db1
>> + 3. capabilities: db1 => ROW, db2 => STMT
>> + 4. UPDATE db1.t1 A, db2.t1 B SET A.x=1, B.x=2;
>> +
>> + Q. Which is the value of flags_all_set_row?
>> + A. HA_BINLOG_ROW_CAPABLE, because, db2 is filtered (MIXED
>> + should switch to row because flags_all_set would not have STMT
>> + capability).
>> +
>> + Q. Which is the value of flags_all_set?
>> + A. 0
>> +
>> + Q. This statement should log the update on db1.t1 when using
>> + ROW and MIXED mode. As such, errors should not be thrown
>> + for this statement. In this case, which capabilities flags
>> + should one use to conduct error checking?
>> + A. flags_all_set_row
>> +
>> + Basically, when one finds that is indeed logging in ROW mode,
>> + then the vector of capabilities should just hold the
>> + capabilities for the engines which are referenced by tables in
>> + databases not filtered out from binlog.
>> + */
>> + if (log_as_row)
>> + {
>> + flags_all_set= flags_all_set_row;
>> + flags_some_set= flags_some_set_row;
>> + }
>> + }
>> +
>> if (flags_all_set == 0)
>> {
>> my_error((error= ER_BINLOG_LOGGING_IMPOSSIBLE), MYF(0),
>> "Statement cannot be logged to the binary log in"
>> " row-based nor statement-based format");
>> }
>> - else if (thd->variables.binlog_format == BINLOG_FORMAT_STMT &&
>> + else if (!log_as_row &&
>> (flags_all_set & HA_BINLOG_STMT_CAPABLE) == 0)
>> {
>> my_error((error= ER_BINLOG_LOGGING_IMPOSSIBLE), MYF(0),
>> "Statement-based format required for this statement,"
>> " but not allowed by this combination of engines");
>> }
>> - else if ((thd->variables.binlog_format == BINLOG_FORMAT_ROW ||
>> - thd->lex->is_stmt_unsafe()) &&
>> - (flags_all_set & HA_BINLOG_ROW_CAPABLE) == 0)
>> + else if (log_as_row &&
>> + (flags_all_set & HA_BINLOG_ROW_CAPABLE) == 0)
>> {
>> my_error((error= ER_BINLOG_LOGGING_IMPOSSIBLE), MYF(0),
>> "Row-based format required for this statement,"
>> @@ -5179,6 +5272,7 @@ int decide_logging_format(THD *thd, TABL
>> if (error)
>> return -1;
>>
>> +end:
>> /*
>> We switch to row-based format if we are in mixed mode and one of
>> the following are true:
>> @@ -5191,8 +5285,7 @@ int decide_logging_format(THD *thd, TABL
>> this code in reset_current_stmt_binlog_row_based(), it has to be
>> here.
>> */
>> - if (thd->lex->is_stmt_unsafe() ||
>> - (flags_all_set & HA_BINLOG_STMT_CAPABLE) == 0)
>> + if (log_as_row)
>> {
>> thd->set_current_stmt_binlog_row_based_if_mixed();
>> }
>>
>> === modified file 'sql/sql_class.cc'
>> --- a/sql/sql_class.cc 2009-06-15 15:53:45 +0000
>> +++ b/sql/sql_class.cc 2009-06-20 13:57:39 +0000
>> @@ -41,6 +41,7 @@
>>
>> #include "sp_rcontext.h"
>> #include "sp_cache.h"
>> +#include "rpl_filter.h"
>>
>> /*
>> The following is used to initialise Table_ident with a internal
>> @@ -2912,6 +2913,11 @@ extern "C" void thd_mark_transaction_to_
>> {
>> mark_transaction_to_rollback(thd, all);
>> }
>> +
>> +extern "C" bool thd_binlog_filter_ok(const MYSQL_THD thd)
>> +{
>> + return binlog_filter->db_ok(thd->db);
>> +}
>> #endif // INNODB_COMPATIBILITY_HOOKS */
>>
>> /****************************************************************************
>>
>>
>
>
>> === modified file 'storage/innobase/handler/ha_innodb.cc'
>> --- a/storage/innobase/handler/ha_innodb.cc 2009-05-19 08:20:28 +0000
>> +++ b/storage/innobase/handler/ha_innodb.cc 2009-06-20 13:57:39 +0000
>> @@ -6843,8 +6843,10 @@ ha_innobase::external_lock(
>> {
>> ulong const binlog_format= thd_binlog_format(thd);
>> ulong const tx_isolation = thd_tx_isolation(current_thd);
>> + bool const is_binlog_filter_ok= thd_binlog_filter_ok(thd);
>> if (tx_isolation <= ISO_READ_COMMITTED &&
>> - binlog_format == BINLOG_FORMAT_STMT)
>> + binlog_format == BINLOG_FORMAT_STMT &&
>> + is_binlog_filter_ok)
>> {
>> char buf[256];
>> my_snprintf(buf, sizeof(buf),
>>
>>
>
> As we agreed, you are going to make this snippet as a candidate for
> refactoring. I have suggested to qualify it as urgent todo.
>
>
>> === modified file 'storage/innobase/handler/ha_innodb.h'
>> --- a/storage/innobase/handler/ha_innodb.h 2009-04-24 11:28:46 +0000
>> +++ b/storage/innobase/handler/ha_innodb.h 2009-06-20 13:57:39 +0000
>> @@ -252,4 +252,11 @@ int thd_binlog_format(const MYSQL_THD th
>> @param all TRUE <=> rollback main transaction.
>> */
>> void thd_mark_transaction_to_rollback(MYSQL_THD thd, bool all);
>> +
>> +/**
>> + Check if binary logging is filtered for thread's current db.
>> + @param thd Thread handle
>> + @retval 1 the query is not filtered, 0 otherwise.
>> +*/
>> +bool thd_binlog_filter_ok(const MYSQL_THD thd);
>> }
>>
>>
>
>
> cheers,
>
> Andrei
>
