Hi!
"Oleksandr" == Oleksandr Byelkin <sanja@montyprogram.com> writes:
<cut>
@@ -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 */
Oleksandr> Following is passing flag about triggers on the server. The problem was Oleksandr> to pass it between table map event and row event. I do it via extended Oleksandr> TABLE_LIST (RPL_TABLE_LIST). but row event uses only TABLE so I need to Oleksandr> find somehow the corresponding TABLE_LIST. Please add a comment about this to the code. 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. Oleksandr> it is done before very last return but not so close to it.
- 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(). */
Did you do the above?
When is lex->query_tables used?
Oleksandr> the problem was that trigger procedures works only with lex-> query_tables (and it was one of the cause of skipping triggers in Oleksandr> row log events.
Is it only used by 'open' ? Oleksandr> It is where triggers processed (openng and locking tables). If yes, isn't it enough to always reset it after the 'open' call? Oleksandr> We have a lot of ASSERTS that check the lists when we close tables. they Oleksandr> have the same problem with MERGE MYISAM tables and went this way I only Oleksandr> trying to do more or less the same.
Ok. Please add the above as a comment in the place where set query_tables.
@@ -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?
Oleksandr> it also has table->mark_columns_needed_for_insert(); Raw log event mark Oleksandr> all columns in any case also checks some correspondence between real Oleksandr> columns on master & slave so I prefer do not interfere here. Actually, having mark_columns_needed_for_insert() in prepare_triggers_for_insert_stmt() is wrong. This has nothing to do with triggers. Please move this function out of prepare_triggers_for_insert_stmt(() and then you can reuse this function for your code.
<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
Oleksandr> maybe, but as I it will be temptation to put something more to the Oleksandr> function without noticing that it is called for events and statements... Oleksandr> I'd prefer method of TABLE:: and called something like Oleksandr> prepare_triggers_for_delete_operation() Oleksandr> or Oleksandr> prepare_triggers_for_delete_event_or_stmt() That is ok. As long as we use the same method for the insert case.
<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 Oleksandr> see above.
Agree with having this as a table function. One last question: Do you happen to know why we in select_create::prepare() call table->mark_columns_needed_for_insert(); but not prepare_triggers_for_insert_stmt() ? Does this mean: 1) This is a bug, we should instead call prepare_triggers_for_insert_stmt(). 2) prepare_triggers_for_insert_stmt() is called later (and thus table->mark_columns_needed_for_insert() is called twice). 3) prepare_triggers_for_insert_stmt() is not needed in this case ??? My guess would be that we are calling mark_columns_needed_for_insert() twice and we can delete one of the calls, probably in select_create::prepare(), but I didn't have time to verify this. Regards, Monty