List:Commits« Previous MessageNext Message »
From:Libing Song Date:August 23 2010 1:37pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3473)
Bug#55375
View as plain text  
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
==================================

Thread
bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3473) Bug#55375Li-Bing.Song6 Aug
  • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3473)Bug#55375Alfranio Correia23 Aug
    • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3473)Bug#55375Libing Song23 Aug
      • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3473)Bug#55375Alfranio Correia23 Aug
Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3473)Bug#55375Libing Song6 Sep
Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3473)Bug#55375Libing Song7 Sep
Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3473)Bug#55375Libing Song7 Sep
Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3473)Bug#55375Libing Song16 Sep
Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3473)Bug#55375Libing Song17 Sep