← Back to team overview

maria-developers team mailing list archive

Re: Rev 4004: RBR triggers compiled-out with ifdefs in 10.0

 

Hi, Sanja!

On Feb 12, sanja@xxxxxxxxxxxx wrote:
> At file:///home/bell/maria/bzr/work-maria-10.0-MDEV-5095/
> 
> ------------------------------------------------------------
> revno: 4004
> revision-id: sanja@xxxxxxxxxxxx-20140212204600-n89hns6zvrut1nl7
> parent: sanja@xxxxxxxxxxxx-20140212190858-38n5izb9m3pwz7wc
> committer: sanja@xxxxxxxxxxxx
> branch nick: work-maria-10.0-MDEV-5095
> timestamp: Wed 2014-02-12 22:46:00 +0200
> message:
>   RBR triggers compiled-out with ifdefs in 10.0

Here're my comments, see below:
Note, that I didn't review the implementation of RBR triggers, I
looked at ifdefs only.

> === modified file 'mysql-test/suite/rpl/disabled.def'
> --- mysql-test/suite/rpl/disabled.def	2013-09-27 12:58:49 +0000
> +++ mysql-test/suite/rpl/disabled.def	2014-02-28 10:56:24 +0000
> @@ -14,3 +14,5 @@ rpl_row_create_table      : Bug#11759274
>  rpl_spec_variables        : BUG#11755836 2009-10-27 jasonh rpl_spec_variables fails on PB2 hpux
>  rpl_get_master_version_and_clock : Bug#11766137 Jan 05 2011 joro Valgrind warnings rpl_get_master_version_and_clock
>  rpl_partition_archive     : MDEV-5077 2013-09-27 svoj Cannot exchange partition with archive table
> +rpl_row_triggers          : IFDEF-ed in 10.0 (RBR_TRIGGERS)
> +rpl_row_triggers_sbr      : IFDEF-ed in 10.0 (RBR_TRIGGERS)

better than disabled.def would be to have a file, say

--source include/have_rbr_triggers.inc

that would do, like

if (`select count(*) = 0 from information_schema.session_variables where variable_name = 'slave_run_triggers_for_rbr'`)
{
  skip RBR triggers are not available;
}

> === modified file 'sql/log_event.cc'
> --- sql/log_event.cc	2013-11-13 22:03:48 +0000
> +++ sql/log_event.cc	2014-02-28 10:56:24 +0000
> @@ -9282,10 +9284,31 @@ int Rows_log_event::do_add_row_data(ucha
>  }
>  #endif
>  
> -#if !defined(MYSQL_CLIENT) && defined(HAVE_REPLICATION)
> +#if defined(MYSQL_SERVER) && defined(HAVE_REPLICATION)

why is this change?

> +
> +
> +#ifdef RBR_TRIGGERS
> +/**
> +  Restores empty table list as it was before trigger processing.
> +
> +  @note We have a lot of ASSERTS that check the lists when we close tables.
> +  There was the same problem with MERGE MYISAM tables and so here we try to
> +  go the same way.
> +*/
> +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;
> +}
> +#endif //RBR_TRIGGERS
> +
> +
>  int Rows_log_event::do_apply_event(rpl_group_info *rgi)
>  {
>    Relay_log_info const *rli= rgi->rli;
> +  TABLE* table;
>    DBUG_ENTER("Rows_log_event::do_apply_event(Relay_log_info*)");
>    int error= 0;
>    /*
> @@ -9632,9 +9691,15 @@ int Rows_log_event::do_apply_event(rpl_g
>      */
>      thd->reset_current_stmt_binlog_format_row();
>      thd->is_slave_error= 1;
> -    DBUG_RETURN(error);
> +    /* remove trigger's tables */
> +    goto err;

hmm, here you've added rgi->slave_close_thread_tables(thd);

>    }
>  
> +#ifdef RBR_TRIGGERS
> +  /* remove trigger's tables */
> +  if (slave_run_triggers_for_rbr)
> +    restore_empty_query_table_list(thd->lex);
> +#endif //RBR_TRIGGERS
>    if (get_flags(STMT_END_F) && (error= rows_event_stmt_cleanup(rgi, thd)))
>      slave_rows_error_report(ERROR_LEVEL,
>                              thd->is_error() ? 0 : error,
> @@ -10132,6 +10206,9 @@ Table_map_log_event::Table_map_log_event
>    DBUG_ASSERT(static_cast<size_t>(cbuf_end - cbuf) <= sizeof(cbuf));
>    m_data_size+= (cbuf_end - cbuf) + m_colcnt;	// COLCNT and column types
>  
> +  if (tbl->triggers)
> +    m_flags|= TM_BIT_HAS_TRIGGERS_F;

why didn't you ifdef this?

> +
>    /* If malloc fails, caught in is_valid() */
>    if ((m_memory= (uchar*) my_malloc(m_colcnt, MYF(MY_WME))))
>    {
> @@ -10538,7 +10616,11 @@ int Table_map_log_event::do_apply_event(
>    table_list->table_id= DBUG_EVALUATE_IF("inject_tblmap_same_id_maps_diff_table", 0, m_table_id);
>    table_list->updating= 1;
>    table_list->required_type= FRMTYPE_TABLE;
> -  DBUG_PRINT("debug", ("table: %s is mapped to %u", table_list->table_name, table_list->table_id));
> +  table_list->master_had_triggers= ((m_flags & TM_BIT_HAS_TRIGGERS_F) ? 1 : 0);

why didn't you ifdef this?

> +  DBUG_PRINT("debug", ("table: %s is mapped to %u%s",
> +                       table_list->table_name, table_list->table_id,
> +                       (table_list->master_had_triggers ?
> +                        " (master had triggers)" : "")));
>    enum_tbl_map_status tblmap_status= check_table_map(rgi, table_list);
>    if (tblmap_status == OK_TO_PROCESS)
>    {
> @@ -10801,6 +10887,10 @@ Write_rows_log_event::do_before_row_oper
>        from the start.
>      */
>    }
> +#ifdef RBR_TRIGGERS
> +  if (slave_run_triggers_for_rbr && !master_had_triggers && m_table->triggers)
> +    m_table->prepare_triggers_for_insert_stmt_or_event();
> +#endif //RBR_TRIGGERS

But you don't need to ifdef this and many blocks in 
Rows_log_event::do_apply_event. If you only do
 #define slave_run_triggers_for_rbr 0
the compiler will remove the whole if() block anyway.

>  
>    /* Honor next number column if present */
>    m_table->next_number_field= m_table->found_next_number_field;
> @@ -10928,6 +11039,13 @@ Rows_log_event::write_row(rpl_group_info
>    TABLE *table= m_table;  // pointer to event's table
>    int error;
>    int UNINIT_VAR(keynum);
> +
> +#ifdef RBR_TRIGGERS
> +  bool invoke_triggers=
> +    slave_run_triggers_for_rbr && !master_had_triggers && table->triggers;
> +#else
> +#define invoke_triggers 0
> +#endif

if you have slave_run_triggers_for_rbr defined to 0
then you don't need this idef either.
although I'd still declare invoke_triggers as const,
to make it easier for the compiler.

>    auto_afree_ptr<char> key(NULL);
>  
>    prepare_record(table, m_width,
> @@ -10953,6 +11075,14 @@ Rows_log_event::write_row(rpl_group_info
>    DBUG_PRINT_BITSET("debug", "read_set = %s", table->read_set);
>  #endif
>  
> +#ifdef RBR_TRIGGERS
> +  if (invoke_triggers &&
> +      process_triggers(TRG_EVENT_INSERT, TRG_ACTION_BEFORE, TRUE))
> +  {
> +      DBUG_RETURN(HA_ERR_GENERIC); // in case if error is not set yet
> +  }
> +#endif //RBR_TRIGGERS

why ifdef if invoke_triggers is 0 anyway?

> +
>    /* 
>      Try to write record. If a corresponding record already exists in the table,
>      we try to change it using ha_update_row() if possible. Otherwise we delete
> @@ -11079,38 +11209,71 @@ Rows_log_event::write_row(rpl_group_info
>          !table->file->referenced_by_foreign_key())
>      {
>        DBUG_PRINT("info",("Updating row using ha_update_row()"));
> -      error=table->file->ha_update_row(table->record[1],
> -                                       table->record[0]);
> -      switch (error) {
> -                
> -      case HA_ERR_RECORD_IS_THE_SAME:
> -        DBUG_PRINT("info",("ignoring HA_ERR_RECORD_IS_THE_SAME error from"
> -                           " ha_update_row()"));
> -        error= 0;
> -      
> -      case 0:
> -        break;
> -        
> -      default:    
> -        DBUG_PRINT("info",("ha_update_row() returns error %d",error));
> -        table->file->print_error(error, MYF(0));
> +#ifdef RBR_TRIGGERS

why ifdef if invoke_triggers is 0 anyway?

> +      if (invoke_triggers &&
> +          process_triggers(TRG_EVENT_UPDATE, TRG_ACTION_BEFORE, FALSE))
> +        error= HA_ERR_GENERIC; // in case if error is not set yet
> +      else
> +#endif
> +      {
> +        error= table->file->ha_update_row(table->record[1],
> +                                         table->record[0]);
> +        switch (error) {
> +
> +        case HA_ERR_RECORD_IS_THE_SAME:
> +          DBUG_PRINT("info",("ignoring HA_ERR_RECORD_IS_THE_SAME error from"
> +                             " ha_update_row()"));
> +          error= 0;
> +
> +        case 0:
> +          break;
> +
> +        default:
> +          DBUG_PRINT("info",("ha_update_row() returns error %d",error));
> +          table->file->print_error(error, MYF(0));
> +        }
> +#ifdef RBR_TRIGGERS
> +        if (invoke_triggers && !error &&
> +            (process_triggers(TRG_EVENT_UPDATE, TRG_ACTION_AFTER, TRUE) ||
> +             process_triggers(TRG_EVENT_INSERT, TRG_ACTION_AFTER, TRUE)))
> +          error= HA_ERR_GENERIC; // in case if error is not set yet
> +#endif //RBR_TRIGGERS
>        }
> -      
> +
>        DBUG_RETURN(error);
>      }
>      else
>      {
>        DBUG_PRINT("info",("Deleting offending row and trying to write new one again"));
> -      if ((error= table->file->ha_delete_row(table->record[1])))
> +#ifdef RBR_TRIGGERS
> +      if (invoke_triggers &&
> +          process_triggers(TRG_EVENT_DELETE, TRG_ACTION_BEFORE, TRUE))
> +        error= HA_ERR_GENERIC; // in case if error is not set yet
> +      else
> +#endif //RBR_TRIGGERS
>        {
> -        DBUG_PRINT("info",("ha_delete_row() returns error %d",error));
> -        table->file->print_error(error, MYF(0));
> -        DBUG_RETURN(error);
> +        if ((error= table->file->ha_delete_row(table->record[1])))
> +        {
> +          DBUG_PRINT("info",("ha_delete_row() returns error %d",error));
> +          table->file->print_error(error, MYF(0));
> +          DBUG_RETURN(error);
> +        }
> +#ifdef RBR_TRIGGERS
> +        if (invoke_triggers &&
> +            process_triggers(TRG_EVENT_DELETE, TRG_ACTION_AFTER, TRUE))
> +          DBUG_RETURN(HA_ERR_GENERIC); // in case if error is not set yet
> +#endif //RBR_TRIGGERS
>        }
>        /* Will retry ha_write_row() with the offending row removed. */
>      }
>    }
>  
> +#ifdef RBR_TRIGGERS
> +  if (invoke_triggers &&
> +      process_triggers(TRG_EVENT_INSERT, TRG_ACTION_AFTER, TRUE))
> +    error= HA_ERR_GENERIC; // in case if error is not set yet
> +#endif //RBR_TRIGGERS
> +
>    DBUG_RETURN(error);
>  }
>  
> === modified file 'sql/sys_vars.cc'
> --- sql/sys_vars.cc	2013-11-25 14:49:40 +0000
> +++ sql/sys_vars.cc	2014-02-28 10:56:24 +0000
> @@ -2601,6 +2601,22 @@ static Sys_var_enum Slave_exec_mode(
>         GLOBAL_VAR(slave_exec_mode_options), CMD_LINE(REQUIRED_ARG),
>         slave_exec_mode_names, DEFAULT(SLAVE_EXEC_MODE_STRICT));
>  
> +#ifdef RBR_TRIGGERS
> +static const char *slave_run_triggers_for_rbr_names[]=
> +  {"NO", "YES", "LOGGING", 0};
> +static Sys_var_enum Slave_run_triggers_for_rbr(
> +       "slave_run_triggers_for_rbr",
> +       "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 "
> +       "results of that the executed triggers work will be written to "
> +       "the binlog.",
> +       GLOBAL_VAR(slave_run_triggers_for_rbr), CMD_LINE(REQUIRED_ARG),
> +       slave_run_triggers_for_rbr_names,
> +       DEFAULT(SLAVE_RUN_TRIGGERS_FOR_RBR_NO));
> +#endif //RBR_TRIGGERS

empty line between variables would be nice

>  static const char *slave_type_conversions_name[]= {"ALL_LOSSY", "ALL_NON_LOSSY", 0};
>  static Sys_var_set Slave_type_conversions(
>         "slave_type_conversions",
> 
> === modified file 'sql/table.cc'
> --- sql/table.cc	2013-11-11 18:46:14 +0000
> +++ sql/table.cc	2014-02-27 19:10:57 +0000
> @@ -6660,6 +6660,81 @@ int TABLE::update_default_fields()
>  
>  
>  /*
> +  Prepare triggers  for INSERT-like statement.
> +
> +  SYNOPSIS
> +    prepare_triggers_for_insert_stmt_or_event()
> +
> +  NOTE
> +    Prepare triggers for INSERT-like statement by marking fields
> +    used by triggers and inform handlers that batching of UPDATE/DELETE 
> +    cannot be done if there are BEFORE UPDATE/DELETE triggers.
> +*/
> +
> +void TABLE::prepare_triggers_for_insert_stmt_or_event()
> +{
> +  if (triggers)
> +  {
> +    if (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) file->extra(HA_EXTRA_DELETE_CANNOT_BATCH);
> +    }
> +    if (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) file->extra(HA_EXTRA_UPDATE_CANNOT_BATCH);
> +    }
> +  }
> +}
> +
> +
> +bool TABLE::prepare_triggers_for_delete_stmt_or_event()
> +{
> +  if (triggers &&
> +      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) file->extra(HA_EXTRA_DELETE_CANNOT_BATCH);
> +    return TRUE;
> +  }
> +  return FALSE;
> +}

why didn't you put this in the .h file?
it's something that should rather be inlined

> +
> +
> +bool TABLE::prepare_triggers_for_update_stmt_or_event()
> +{
> +  if (triggers &&
> +      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) file->extra(HA_EXTRA_UPDATE_CANNOT_BATCH);
> +    return TRUE;
> +  }
> +  return FALSE;
> +}
> +
> +/*
>    @brief Reset const_table flag
>  
>    @detail
> 

Regards,
Sergei