maria-developers team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxxxxxxx>]

Hi!

10.12.2013 17:58, Michael Widenius пишет:
[skip]
> > > > +      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?
yes. (postreview patch)
[skip]
> > > > +  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.

I hope that I have understand the question correctly.

I checked mark_columns_needed_for_insert() it always called only once.

SELECT ... CREATE has no trigger preparation because there is no way to 
get triggers on the table we are creating in the same statement.

[skip]