maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Oleksandr" == Oleksandr Byelkin <sanja@xxxxxxxxxxxxxxxx> 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).

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.

>>> +      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?

>> 
>> 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