From: Luís Soares Date: March 3 2010 5:51pm Subject: Re: bzr commit into mysql-next-mr-bugfixing branch (alfranio.correia:2974) Bug#51291 Bug#51564 List-Archive: http://lists.mysql.com/commits/102229 Message-Id: <1267638713.2907.689.camel@mobilos.lan> MIME-Version: 1.0 Content-Type: text/plain; CHARSET=US-ASCII Content-Transfer-Encoding: 7BIT Hi Alfranio, Nice Work. The patch seems a good step in the right direction, but still needs some work. Please find my review comments below. STATUS ------ Not Approved. REQUIRED CHANGES ---------------- RC1. Context: In hunk #2, sql/sql_class.cc. Problem: I don't think that all_trans_engines is used anymore. We never read from it, just write to it (two assignments). RC2. Context: In hunk #2, sql/log.cc. Problem: Please improve the statement! As I see it should say something like (pretty close to what you have already, but with some emphasis on the committed: "Set to true if there is at least one persistent, ie, committed, statement/row in the cache." RC3. Context: In hunk #3, sql/log.cc. Problem: remove "empty" change. RC4. Context: In hunk #6, sql/log.cc. Problem: In the comments I find: "- there is no previously committed statements in the cache and;" I think that here means that there is no previously committed transactional statement in the cache. When the binlog_direct_non_... is OFF, the only case that the cache is not empty and the current statement is updating a non-transactional table is when previously some transactional statement was executed. Also I think that the next comment should start with: "- the current statement..." RC5. Context: In hunk #6, sql/log.cc. Problem: In comments, I think there may be missing some observation stating that the cache is not fully truncated, but rather truncated to the last saved position. REQUESTS -------- R1. Context: In hunk #7, sql/log.cc. Problem: Wow... That if just seems like a bad weed growing... :( I tried to use 'qmc' simplifier to make it smaller (perhaps sacrificing readability), but I got no luck (maybe I used it wrong...). Please, check if you can improve this condition, and/or expand the comments preceding it with referrals to how both caches are used when the binlog direct option is ON and OFF. SUGGESTIONS ----------- S1. Context: In hunk #5, sql/log.cc. Problem: In the vicinity of this change there is a definition of: bool const is_transactional= FALSE; Which is later only used in the call to binlog_pending_... . I suggest that we don't declare this is_transactional variable and use FALSE directly on the call, probably with a comment: binlog_flush_pending_rows_event(TRUE, FALSE /* we're flushing non-transactional changes => so must be FALSE */); I think it makes it easier to read the code and to review some patches in that function. Take your hunk as an example... It got suspicious to have a variable is_transactional in binlog_flush_stmt_cache, when . S2. While looking at this patch sometimes I found myself confused. For instance: !stmt_has_updated_trans_table(thd) && thd->transaction.stmt.modified_non_trans_table)) So here, we have two conditions that seem to check the same thing. But as we have discussed, this is because there is a design flaw in the transaction.all and transaction.stmt flags. They only contain information about whether a statement has modified a non-transactional table or not. I suggest that we encapsulate these flags and use the same function to check whether the statement has modified a trans, non-trans or both table(s). For example: bool stmt_has_updated_table(THD *thd, uint flags) bool all_has_updated_table(THD *thd, uint flags) Where flags can be: #define TRANS 0x1 #define NON_TRANS 0x2 So if we would like to check if the statement/transaction has changed both types of tables (for instance in a mixed statement), we would use: /* check if statement has updated both types o tables */ if (stmt_has_update_table(thd, TRANS | NON_TRANS)) { ... } DETAILS ------- n/a On Fri, 2010-02-26 at 20:47 +0000, Alfranio Correia wrote: > #At file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-51291/mysql-next-mr-bugfixing/ based on revid:alik@stripped > > 2974 Alfranio Correia 2010-02-26 > BUG#51291, BUG#51564 > > --- > BUG#51291 Unfortunate effect around variable binlog_direct_non_transactional_updates > > The new option binlog_direct_non_transactional_updates does not mimic the behavior > in 5.1 when set to OFF. This happens due to the following reasons: > > . non-transactional changes executed before all transactional changes are stored > in the non-trx-cache in order to avoid tracking the the trx-cache and figuring out > when it might be flushed. However, this could lead to the following scenario: > > EXECUTION: > > BEGIN; MIXED-STATEMENT; COMMIT; > > BINLOG ENTRIES: > > STMT-CACHE: > ----------- > > BEGIN; > > TableMap on N; > > Write to N; > > COMMIT; > > TRX-CACHE: > ---------- > > BEGIN; > > TableMap on T; > > Write to T; > > Write to N; > > COMMIT; > > This happens because changes to non-transactional engines go to the stmt-cache > when there is no changes to transactional engines. However as soon as the first > change to the transactional engine gets into the trx-cache, additional changes > to transactional engines go to the trx-cache. Either the non-transactional changes > go to the stmt-cache or to the trx-cache but using both caches may generate > inconsistencies as the TableMap on N is not in the trx-cache. So as an unfortunate > consequence, the slave simply ignores changes whose TableMap do no exist. > > Recall that TableMaps have a life-cycle in the context of a transaction and as > such do not go beyond transaction's boundaries. > > . in mixed and row modes, the "binlog rollback" was not considering the fact that > there might be non-transactional changes in the trx-cache and was truncating the > cache. > > . in mixed and row modes, the "binlog rollback" was not considering the fact that > there might be non-transactional changes in a failed statement and was truncating > the cache. > > . there is no need to generate warning messages for changes to non-transactional > engines in mixed statements or that happen after changes to transactional engines > within a transaction's context. > > --- > BUG#51564 Error ER_BINLOG_UNSAFE_NONTRANS_AFTER_TRANS is too eager after BUG#46364 > > > Error ER_BINLOG_UNSAFE_NONTRANS_AFTER_TRANS is too eager after BUG#46364 because > warning messages are being printed out for statements that only update > non-transactional engines with the SBR mode. > > To fix the problem, we only print warning messages when mixed statements are > executed with the SBR mode and create a new error message > BINLOG_STMT_UNSAFE_MIXED_STATEMENT for this case. > > modified: > sql/log.cc > sql/share/errmsg-utf8.txt > sql/sql_class.cc > sql/sql_lex.cc > sql/sql_lex.h > === modified file 'sql/log.cc' > --- a/sql/log.cc 2010-02-15 11:16:49 +0000 > +++ b/sql/log.cc 2010-02-26 20:47:36 +0000 > @@ -199,8 +199,8 @@ private: > class binlog_cache_data > { > public: > - binlog_cache_data(): m_pending(0), before_stmt_pos(MY_OFF_T_UNDEF), > - incident(FALSE) > + binlog_cache_data(): at_least_one_stmt_committed(0), m_pending(0), > + before_stmt_pos(MY_OFF_T_UNDEF), incident(FALSE) > { > cache_log.end_of_file= max_binlog_cache_size; > } > @@ -273,6 +273,11 @@ public: > } > > /* > + This is true if there is at least one statement/row in the cache. > + */ > + bool at_least_one_stmt_committed; > + > + /* > Cache to store data before copying it to the binary log. > */ > IO_CACHE cache_log; > @@ -288,7 +293,7 @@ private: > Binlog position before the start of the current statement. > */ > my_off_t before_stmt_pos; > - > + > /* > This indicates that some events did not get into the cache and most likely > it is corrupted. > @@ -309,8 +314,9 @@ private: > } > reinit_io_cache(&cache_log, WRITE_CACHE, pos, 0, 0); > cache_log.end_of_file= max_binlog_cache_size; > + at_least_one_stmt_committed= (pos > 0); > } > - > + > binlog_cache_data& operator=(const binlog_cache_data& info); > binlog_cache_data(const binlog_cache_data& info); > }; > @@ -1631,7 +1637,10 @@ binlog_flush_stmt_cache(THD *thd, binlog > */ > bool const is_transactional= FALSE; > IO_CACHE *cache_log= &cache_mngr->stmt_cache.cache_log; > - thd->binlog_flush_pending_rows_event(TRUE, is_transactional); > + > + if (thd->binlog_flush_pending_rows_event(TRUE, is_transactional)) > + DBUG_RETURN(1); > + > Query_log_event qev(thd, STRING_WITH_LEN("COMMIT"), TRUE, FALSE, TRUE, 0); > if ((error= mysql_bin_log.write(thd, cache_log, &qev, > cache_mngr->stmt_cache.has_incident()))) > @@ -1694,17 +1703,32 @@ static int binlog_commit(handlerton *hto > } > > /* > - We commit the transaction if: > + When the variables.binlog_direct_non_trans_update is ON, > + we commit the transaction if: > - We are not in a transaction and committing a statement, or > - We are in a transaction and a full transaction is committed. > + > + When variables.binlog_direct_non_trans_update is OFF, we > + need to flush the cache's contents if: > + - committing a statement and; > + - there is no previously committed statements in the cache and; > + - the statement has updated only non-transactional tables. > + > Otherwise, we accumulate the changes. > */ > - if (!in_transaction || all) > + if (!in_transaction || all || > + (!thd->variables.binlog_direct_non_trans_update && > + !all && !cache_mngr->trx_cache.at_least_one_stmt_committed && > + !stmt_has_updated_trans_table(thd) && > + thd->transaction.stmt.modified_non_trans_table)) > { > Query_log_event qev(thd, STRING_WITH_LEN("COMMIT"), TRUE, FALSE, TRUE, 0); > error= binlog_flush_trx_cache(thd, cache_mngr, &qev); > } > > + cache_mngr->trx_cache.at_least_one_stmt_committed= > + !cache_mngr->trx_cache.empty(); > + > /* > This is part of the stmt rollback. > */ > @@ -1774,32 +1798,47 @@ static int binlog_rollback(handlerton *h > error= binlog_truncate_trx_cache(thd, cache_mngr, all); > } > else > - { > + { > /* > - We flush the cache wrapped in a beging/rollback if: > - . aborting a transcation that modified a non-transactional table or; > - . aborting a statement that modified both transactional and > - non-transctional tables but which is not in the boundaries of any > - transaction; > - . the OPTION_KEEP_LOG is activate. > + When variables.binlog_direct_non_trans_update is OFF or the > + binlog format is SBR, we abort the transaction if: > + - aborting a transcation that modified a non-transactional table or; > + - aborting a statement that modified a non-transctional table but is > + not in the boundaries of any transaction; > + - the OPTION_KEEP_LOG is activate. > + > + When variables.binlog_direct_non_trans_update is OFF, we > + need to flush the cache's contents if: > + - aborting a statement and; > + - there is no previously committed statements in the cache and; > + - the statement has updated a non-transactional table. > */ > - if (thd->variables.binlog_format == BINLOG_FORMAT_STMT && > - ((all && thd->transaction.all.modified_non_trans_table) || > - (!all && thd->transaction.stmt.modified_non_trans_table && > - !thd->in_multi_stmt_transaction()) || > - (thd->variables.option_bits & OPTION_KEEP_LOG))) > + if (((!thd->variables.binlog_direct_non_trans_update || > + thd->variables.binlog_format == BINLOG_FORMAT_STMT) && > + ((all && thd->transaction.all.modified_non_trans_table) || > + (!all && thd->transaction.stmt.modified_non_trans_table && > + !thd->in_multi_stmt_transaction()) || > + (thd->variables.option_bits & OPTION_KEEP_LOG))) || > + (!thd->variables.binlog_direct_non_trans_update && > + !all && thd->transaction.stmt.modified_non_trans_table && > + !cache_mngr->trx_cache.at_least_one_stmt_committed)) > { > Query_log_event qev(thd, STRING_WITH_LEN("ROLLBACK"), TRUE, FALSE, TRUE, 0); > error= binlog_flush_trx_cache(thd, cache_mngr, &qev); > } > /* > - Otherwise, we simply truncate the cache as there is no change on > - non-transactional tables as follows. > + We truncate the cache if: > + - the variables.binlog_direct_non_trans_update is ON and > + binlog format is RBR or MIXED or; > + - aborting a transaction that did not modify a non-transactional table or; > + - aborting a statement that did not modify a non-transactional table; > + > + Otherwise, we accumulate the changes. > */ > - else if (all || (!all && > - (!thd->transaction.stmt.modified_non_trans_table || > - !thd->in_multi_stmt_transaction() || > - thd->variables.binlog_format != BINLOG_FORMAT_STMT))) > + else if ((thd->variables.binlog_direct_non_trans_update && > + thd->variables.binlog_format != BINLOG_FORMAT_STMT) || > + (all && !thd->transaction.all.modified_non_trans_table) || > + (!all && !thd->transaction.stmt.modified_non_trans_table)) > error= binlog_truncate_trx_cache(thd, cache_mngr, all); > } > > @@ -4180,8 +4219,9 @@ bool MYSQL_BIN_LOG::is_query_in_union(TH > } > > /** > - This function checks if a transactional talbe was updated by the > - current transaction. > + This function checks if a transactional table was updated by the > + current transaction. If variables.binlog_direct_non_trans_update > + is ON, this may return false positives though. > > @param thd The client thread that executed the current statement. > @return > @@ -4193,11 +4233,11 @@ trans_has_updated_trans_table(const THD* > binlog_cache_mngr *const cache_mngr= > (binlog_cache_mngr*) thd_get_ha_data(thd, binlog_hton); > > - return (cache_mngr ? my_b_tell (&cache_mngr->trx_cache.cache_log) : 0); > + return (cache_mngr ? !cache_mngr->trx_cache.empty() : 0); > } > > /** > - This function checks if a transactional talbe was updated by the > + This function checks if a transactional table was updated by the > current statement. > > @param thd The client thread that executed the current statement. > @@ -4209,7 +4249,8 @@ stmt_has_updated_trans_table(const THD * > { > Ha_trx_info *ha_info; > > - for (ha_info= thd->transaction.stmt.ha_list; ha_info; ha_info= ha_info->next()) > + for (ha_info= thd->transaction.stmt.ha_list; ha_info; > + ha_info= ha_info->next()) > { > if (ha_info->is_trx_read_write() && ha_info->ht() != binlog_hton) > return (TRUE); > @@ -4222,22 +4263,17 @@ stmt_has_updated_trans_table(const THD * > be used. If @c bin_log_direct_non_trans_update is active, the cache > to be used depends on the flag @c is_transactional. > > - Otherswise, we use the trx-cache if either the @c is_transactional > - is true or the trx-cache is not empty. > + Otherswise, we use the trx-cache. > > @param thd The client thread. > - @param is_transactional The changes are related to a trx-table. > + @param is_transactional If the changes are related to a trx-table. > @return > @c true if a trx-cache should be used, @c false otherwise. > */ > -bool use_trans_cache(const THD* thd, bool is_transactional) > +inline bool use_trans_cache(const THD* thd, bool is_transactional) > { > - binlog_cache_mngr *const cache_mngr= > - (binlog_cache_mngr*) thd_get_ha_data(thd, binlog_hton); > - > return > - (thd->variables.binlog_direct_non_trans_update ? is_transactional : > - (cache_mngr->trx_cache.empty() && !is_transactional ? FALSE : TRUE)); > + (thd->variables.binlog_direct_non_trans_update ? is_transactional : TRUE); > } > > /* > > === modified file 'sql/share/errmsg-utf8.txt' > --- a/sql/share/errmsg-utf8.txt 2010-02-21 19:36:05 +0000 > +++ b/sql/share/errmsg-utf8.txt 2010-02-26 20:47:36 +0000 > @@ -6321,3 +6321,6 @@ ER_SPATIAL_MUST_HAVE_GEOM_COL 42000 > > ER_TOO_LONG_INDEX_COMMENT > eng "Comment for index '%-.64s' is too long (max = %lu)" > + > +ER_BINLOG_UNSAFE_MIXED_STATEMENT > + eng "Statements that read from both transactional and non-transactional tables and write to any of them are unsafe." > > === modified file 'sql/sql_class.cc' > --- a/sql/sql_class.cc 2010-02-15 11:16:49 +0000 > +++ b/sql/sql_class.cc 2010-02-26 20:47:36 +0000 > @@ -3640,13 +3640,11 @@ int THD::decide_logging_format(TABLE_LIS > } > > /* > - Set the statement as unsafe if: > + When variables.binlog_direct_non_trans_update is ON, > + set the statement as unsafe if: > > . it is a mixed statement, i.e. access transactional and non-transactional > tables, and updates at least one; > - or > - . an early statement updated a transactional table; > - . and, the current statement updates a non-transactional table. > > Any mixed statement is classified as unsafe to ensure that mixed mode is > completely safe. Consider the following example to understand why we > @@ -3708,9 +3706,8 @@ int THD::decide_logging_format(TABLE_LIS > isolation level but if we have pure repeatable read or serializable the > lock history on the slave will be different from the master. > */ > - if (mixed_engine || > - (trans_has_updated_trans_table(this) && !all_trans_engines)) > - lex->set_stmt_unsafe(LEX::BINLOG_STMT_UNSAFE_NONTRANS_AFTER_TRANS); > + if (variables.binlog_direct_non_trans_update && mixed_engine) > + lex->set_stmt_unsafe(LEX::BINLOG_STMT_UNSAFE_MIXED_STATEMENT); > > DBUG_PRINT("info", ("flags_all_set: 0x%llx", flags_all_set)); > DBUG_PRINT("info", ("flags_some_set: 0x%llx", flags_some_set)); > > === modified file 'sql/sql_lex.cc' > --- a/sql/sql_lex.cc 2010-01-29 13:55:35 +0000 > +++ b/sql/sql_lex.cc 2010-02-26 20:47:36 +0000 > @@ -52,7 +52,7 @@ Query_tables_list::binlog_stmt_unsafe_er > ER_BINLOG_UNSAFE_UDF, > ER_BINLOG_UNSAFE_SYSTEM_VARIABLE, > ER_BINLOG_UNSAFE_SYSTEM_FUNCTION, > - ER_BINLOG_UNSAFE_NONTRANS_AFTER_TRANS > + ER_BINLOG_UNSAFE_MIXED_STATEMENT > }; > > > > === modified file 'sql/sql_lex.h' > --- a/sql/sql_lex.h 2010-02-15 11:16:49 +0000 > +++ b/sql/sql_lex.h 2010-02-26 20:47:36 +0000 > @@ -1135,11 +1135,10 @@ public: > BINLOG_STMT_UNSAFE_SYSTEM_FUNCTION, > > /** > - Mixing transactional and non-transactional statements are unsafe if > - non-transactional reads or writes are occur after transactional > - reads or writes inside a transaction. > + Statements that read from both transactional and non-transactional > + tables and write to any of them are unsafe. > */ > - BINLOG_STMT_UNSAFE_NONTRANS_AFTER_TRANS, > + BINLOG_STMT_UNSAFE_MIXED_STATEMENT, > > /* The last element of this enumeration type. */ > BINLOG_STMT_UNSAFE_COUNT >