4 Dec
2013
4 Dec
'13
8:32 a.m.
Hi!
03.12.2013 14:09, Michael Widenius пишет:
> Hi!
>
> Here is the review of mdev-5095
>
> I got the diff from:
> bzr diff -r3942..
>
[skip]
>> === 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?
Actually above made to make headers and bodies of methods be controlled
by the same condition, because it is strange that header has one
condition and body different (now it means the same bt who know what it
will be in the future).
[skip]
> @@ -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).
Following is passing flag about triggers on the server. The problem was
to pass it between table map event and row event. I do it via extended
TABLE_LIST (RPL_TABLE_LIST). but row event uses only TABLE so I need to
find somehow the corresponding TABLE_LIST.
>
>> + 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.
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().
> */
>
> When is lex->query_tables used?
the problem was that trigger procedures works only with
lex->query_tables (and it was one of the cause of skipping triggers in
row log events.
> Is it only used by 'open' ?
It is where triggers processed (openng and locking tables).
> If yes, isn't it enough to always reset it after the 'open' call?
We have a lot of ASSERTS that check the lists when we close tables. they
have the same problem with MERGE MYISAM tables and went this way I only
trying to do more or less the same.
>
>> @@ -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?
it also has table->mark_columns_needed_for_insert(); Raw log event mark
all columns in any case also checks some correspondence between real
columns on master & slave so I prefer do not interfere here.
>
>
> <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
maybe, but as I it will be temptation to put something more to the
function without noticing that it is called for events and statements...
I'd prefer method of TABLE:: and called something like
prepare_triggers_for_delete_operation()
or
prepare_triggers_for_delete_event_or_stmt()
>
> <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
see above.
>
> <cut>
>
> Regards,
> Monty