Hi, Sanja!
On Feb 12, sanja(a)askmonty.org wrote:
> At file:///home/bell/maria/bzr/work-maria-10.0-MDEV-5095/
>
> ------------------------------------------------------------
> revno: 4004
> revision-id: sanja(a)askmonty.org-20140212204600-n89hns6zvrut1nl7
> parent: sanja(a)askmonty.org-20140212190858-38n5izb9m3pwz7wc
> committer: sanja(a)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
