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().
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
