[Maria-developers] review of mdev-5095 (executing triggers on slave)
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
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
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
Hi!
10.12.2013 17:58, Michael Widenius пишет:
[skip]
>>>> + 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.
> 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?
>
>
yes. (postreview patch)
[skip]
>>>> + 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.
I hope that I have understand the question correctly.
I checked mark_columns_needed_for_insert() it always called only once.
SELECT ... CREATE has no trigger preparation because there is no way to
get triggers on the table we are creating in the same statement.
[skip]
Hi!
"Oleksandr" == Oleksandr Byelkin <sanja@montyprogram.com> writes:
One last question:
Do you happen to know why we in select_create::prepare() call
<cut> 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.
Oleksandr> I hope that I have understand the question correctly. Oleksandr> I checked mark_columns_needed_for_insert() it always called only once. Oleksandr> SELECT ... CREATE has no trigger preparation because there is no way to Oleksandr> get triggers on the table we are creating in the same statement. Ok. I had forgot that MySQL changed how CREATE ... SELECT works. Originally we did execute the SELECT even if the table existed but now we ignore the whole statement if the table exists, so there can't be any triggers. Regards, Monty
participants (2)
-
Michael Widenius -
Oleksandr Byelkin