On Mon, 2010-08-23 at 13:06 +0100, Alfranio Correia wrote:
> On 08/06/2010 11:51 AM, Li-Bing.Song@stripped wrote:
>
> Hi Libing, great work.
>
> Patch not approved.
>
> See comments in-line.
>
> Cheers.
>
> > #At file:///home/anders/work/bzrwork/mysql-5.1-bugteam/ based on
> revid:georgi.kodinov@stripped
> >
> >
> > === modified file 'sql/log_event.cc'
> > --- a/sql/log_event.cc 2010-07-09 12:00:17 +0000
> > +++ b/sql/log_event.cc 2010-08-06 10:51:33 +0000
> > @@ -144,16 +144,21 @@ static void inline slave_rows_error_repo
> > len= my_snprintf(slider, buff_end - slider,
> > " %s, Error_code: %d;", err->msg, err->code);
> > }
> > -
> > - rli->report(level, thd->is_error()? thd->main_da.sql_errno() : 0,
> > - "Could not execute %s event on table %s.%s;"
> > - "%s handler error %s; "
> > - "the event's master log %s, end_log_pos %lu",
> > - type, table->s->db.str,
> > - table->s->table_name.str,
> > - buff,
> > - handler_error == NULL? "<unknown>" : handler_error,
> > - log_name, pos);
> > +
> > + if (ha_error != 0)
this is for handler errors.
> > + rli->report(level, thd->is_error() ? thd->main_da.sql_errno() :
> 0,
> > + "Could not execute %s event on table %s.%s;"
> > + "%s handler error %s; "
> > + "the event's master log %s, end_log_pos %lu",
> > + type, table->s->db.str, table->s->table_name.str,
> > + buff, handler_error == NULL ? "<unknown>" :
> handler_error,
> > + log_name, pos);
> > + else
This is for non-handler errors.
After this patch this function can handle both errors.
> > + rli->report(level, thd->is_error() ? thd->main_da.sql_errno() :
> 0,
> > + "Could not execute %s event on table %s.%s;"
> > + "%s the event's master log %s, end_log_pos %lu",
> > + type, table->s->db.str, table->s->table_name.str,
> > + buff, log_name, pos);
> > }
> > #endif
> >
>
> I would suggest to revert this part of the patch. See comments below.
>
> >
> > @@ -7649,7 +7654,6 @@ int Rows_log_event::do_apply_event(Relay
> > get_type_str(),
> > RPL_LOG_NAME, (ulong) log_pos);
> > thd->reset_current_stmt_binlog_row_based();
> > - const_cast<Relay_log_info*>(rli)->cleanup_context(thd, error);
> > thd->is_slave_error= 1;
> > DBUG_RETURN(error);
> > }
> >
>
> ok.
>
> > @@ -7680,14 +7684,12 @@ int Rows_log_event::do_apply_event(Relay
> > const_cast<Relay_log_info*>(rli)->last_event_start_time=
> my_time(0);
> > }
> >
> > - if (get_flags(STMT_END_F))
> > - if ((error= rows_event_stmt_cleanup(rli, thd)))
> > - rli->report(ERROR_LEVEL, error,
The low level functions in rows_event_stmt_cleanup don't return an error
number(Only return 0 or -1). So it reports an wrong error. the error
number will be -1.
That is why I want to fix it.
> > - "Error in %s event: commit of row events failed, "
> > - "table `%s`.`%s`",
> > - get_type_str(), m_table->s->db.str,
> > - m_table->s->table_name.str);
> > -
> > + if (get_flags(STMT_END_F)&& (error= rows_event_stmt_cleanup(rli,
> thd)))
> > + slave_rows_error_report(ERROR_LEVEL,
> > + thd->is_error() ? 0 : error,
> > + rli, thd, table,
> > + get_type_str(),
> > + RPL_LOG_NAME, (ulong) log_pos);
> > DBUG_RETURN(error);
> > }
> >
>
> In rows_event_stmt_cleanup, there is:
>
> error= thd->binlog_flush_pending_rows_event(true);
>
> /*
> If this event is not in a transaction, the call below will, if some
> transactional storage engines are involved, commit the statement into
> them and flush the pending event to binlog.
> If this event is in a transaction, the call will do nothing, but a
> Xid_log_event will come next which will, if some transactional
> engines
> are involved, commit the transaction and flush the pending event
> to the
> binlog.
> */
> error|= ha_autocommit_or_rollback(thd, error);
>
> I don't think you can call "slave_rows_error_report" because the error
> may not be related to the handler.
I considered that and designed for it.
That's why I always set ha_error as 0 when there is an explicit error.
+ slave_rows_error_report(ERROR_LEVEL,
+ thd->is_error() ? 0 : error,
>
> So I would suggest to revert this part of the patch.
>
>
> >
> >
> > === modified file 'sql/slave.cc'
> > --- a/sql/slave.cc 2010-07-20 18:07:36 +0000
> > +++ b/sql/slave.cc 2010-08-06 10:51:33 +0000
> > @@ -2343,7 +2343,7 @@ static int exec_relay_log_event(THD* thd
> > else
> > {
> > exec_res= 0;
> > - end_trans(thd, ROLLBACK);
> > + const_cast<Relay_log_info*>(rli)->cleanup_context(thd,
> 1);
> > /* chance for concurrent connection to get more locks */
> > safe_sleep(thd, min(rli->trans_retries,
> MAX_SLAVE_RETRY_PAUSE),
> > (CHECK_KILLED_FUNC)sql_slave_killed, (void*)rli);
> >
> ok.
> > @@ -3158,6 +3158,7 @@ the slave SQL thread with \"SLAVE START\
> > request is detected only by the present function, not by events), so we
> > must "proactively" clear playgrounds:
> > */
> > + thd->clear_error();
> > rli->cleanup_context(thd, 1);
> > /*
> > Some extra safety, which should not been needed (normally, event deletion
> >
>
> ok.
> >
> >
> >
> >
> >
>
--
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer
Email : Li-Bing.Song@stripped
Skype : libing.song
MSN : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================
