Hi, Sanja! On Feb 12, sanja@askmonty.org wrote:

At file:///home/bell/maria/bzr/work-maria-10.0-MDEV-5095/

------------------------------------------------------------ revno: 4004 revision-id: sanja@askmonty.org-20140212204600-n89hns6zvrut1nl7 parent: sanja@askmonty.org-20140212190858-38n5izb9m3pwz7wc committer: sanja@askmonty.org 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