Hi! Here is the review of mdev-5095 I got the diff from: bzr diff -r3942..
=== modified file 'mysql-test/r/mysqld--help.result' --- mysql-test/r/mysqld--help.result 2012-10-18 21:33:06 +0000 +++ mysql-test/r/mysqld--help.result 2013-12-03 10:53:01 +0000 @@ -747,6 +747,14 @@ --slave-net-timeout=# Number of seconds to wait for more data from a master/slave connection before aborting the read + --slave-run-triggers-for-rbr=name + Modes for how triggers in row-base replication on slave + side will be executed. Legal values are NO (default), YES + and LOGGING. NO means that trigger for RBR will not be + running on slave YES and LOGGING means that triggers will + be running on slave (if there was not triggers running on + the naster), LOGGING also mens that flag about executed + triggers will be written to binlog.
Suggested new text: --slave-run-triggers-for-rbr=name Modes for how triggers in row-base replication on slave side will be executed. Legal values are NO (default), YES and LOGGING. NO means that trigger for RBR will not be running on slave. YES and LOGGING means that triggers will be running on slave, if there was not triggers running on the master for the statement. LOGGING also means that the executed triggers will be written to binlog. <cut>
=== modified file 'sql/log_event.cc'
<cut>
-#if !defined(MYSQL_CLIENT) && defined(HAVE_REPLICATION) +#if defined(MYSQL_SERVER) && defined(HAVE_REPLICATION)
Was the above needed or mostly an optimization?
+ +static void restore_empty_query_table_list(LEX *lex) +{ + if (lex->first_not_own_table()) + (*lex->first_not_own_table()->prev_global)= NULL; + lex->query_tables= NULL; + lex->query_tables_last= &lex->query_tables; +}
Add a comment of what is the purpose of the above function.
@@ -8340,6 +8351,22 @@ int Rows_log_event::do_apply_event(Relay /* A small test to verify that objects have consistent types */ DBUG_ASSERT(sizeof(thd->variables.option_bits) == sizeof(OPTION_RELAXED_UNIQUE_CHECKS));
+ if (slave_run_triggers_for_rbr) + { + LEX *lex= thd->lex; + uint8 new_trg_event_map= get_trg_event_map(); + + DBUG_ASSERT(lex->query_tables == NULL); + if ((lex->query_tables= rli->tables_to_lock)) + rli->tables_to_lock->prev_global= &lex->query_tables;
Add a comment why we have to set prev_global (not obvious). It's also not obvious why restore_empty_tables() is not resetting rli->tables_to_lock->prev_global as it's reseting the the rest of the changed things.
@@ -8429,8 +8462,11 @@ int Rows_log_event::do_apply_event(Relay */ TABLE_LIST *ptr= rli->tables_to_lock; for (uint i=0 ; ptr && (i < rli->tables_to_lock_count); ptr= ptr->next_global, i++) + { const_cast<Relay_log_info*>(rli)->m_table_map.set_table(ptr->table_id, ptr->table); -
What does this test really do ? (I was quickly checking the code to understand what m_table_id stands for, but it was not obvious).
+ if (m_table_id == ptr->table_id) + master_had_triggers= ((RPL_TABLE_LIST*)ptr)->master_had_triggers;
<cut>
if (table) { bool transactional_table= table->file->has_transactions(); @@ -8618,6 +8656,9 @@ int Rows_log_event::do_apply_event(Relay */ thd->reset_current_stmt_binlog_format_row(); thd->is_slave_error= 1; + /* remove trigger's tables */ + if (slave_run_triggers_for_rbr) + restore_empty_query_table_list(thd->lex); DBUG_RETURN(error); }
You do the above 'restore_empty_query_table_list(thd->lex);' before every return in this function, except the last return. - Isn't it better to do a 'error:' tag at the end of the function to ensure we don't miss any cases ? - Add a comment why we don't need to do this at the very end of Rows_log_event::do_apply_event(). Something like: /* We don't have to call restore_empty_query_table_list() here as it's called in rows_event_stmt_cleanup(). */ When is lex->query_tables used? Is it only used by 'open' ? If yes, isn't it enough to always reset it after the 'open' call?
@@ -9773,6 +9831,29 @@ Write_rows_log_event::do_before_row_oper from the start. */ } + if (slave_run_triggers_for_rbr && !master_had_triggers && m_table->triggers ) + { + if (m_table->triggers->has_triggers(TRG_EVENT_DELETE, + TRG_ACTION_AFTER)) + { + /* + The table has AFTER DELETE triggers that might access to + subject table and therefore might need delete to be done + immediately. So we turn-off the batching. + */ + (void) m_table->file->extra(HA_EXTRA_DELETE_CANNOT_BATCH); + } + if (m_table->triggers->has_triggers(TRG_EVENT_UPDATE, + TRG_ACTION_AFTER)) + { + /* + The table has AFTER UPDATE triggers that might access to subject + table and therefore might need update to be done immediately. + So we turn-off the batching. + */ + (void) m_table->file->extra(HA_EXTRA_UPDATE_CANNOT_BATCH); + } + }
Good that you noticed and fixed the above. However, why not just call the function prepare_triggers_for_insert_stmt() that does exactly the same thing? <cut>
@@ -10749,6 +10894,17 @@ Delete_rows_log_event::do_before_row_ope */ return 0; } + if (slave_run_triggers_for_rbr && !master_had_triggers && m_table->triggers && + m_table->triggers->has_triggers(TRG_EVENT_DELETE, + TRG_ACTION_AFTER)) + { + /* + The table has AFTER DELETE triggers that might access to subject table + and therefore might need delete to be done immediately. So we turn-off + the batching. + */ + (void) m_table->file->extra(HA_EXTRA_DELETE_CANNOT_BATCH); + }
Would be better if we could do a function 'prepare_triggers_for_delete_stmt()' and use the same code here and in sql_delete.cc <cut>
@@ -10877,6 +11049,18 @@ Update_rows_log_event::do_before_row_ope
m_table->timestamp_field_type= TIMESTAMP_NO_AUTO_SET;
+ if (slave_run_triggers_for_rbr && !master_had_triggers && m_table->triggers && + m_table->triggers->has_triggers(TRG_EVENT_UPDATE, + TRG_ACTION_AFTER)) + { + /* + The table has AFTER UPDATE triggers that might access to subject + table and therefore might need update to be done immediately. + So we turn-off the batching. + */ + (void) m_table->file->extra(HA_EXTRA_UPDATE_CANNOT_BATCH); + } +
Would be better if we could do a function 'prepare_triggers_for_update_stmt()' and use the same code here and in sql_update.cc <cut> Regards, Monty