maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

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