RE: bk commit into 6.0 tree (cbell:1.2763) BUG#33562

[Chuck Bell]

Rafal,

I have implemented your suggested changes. Please see the new patch below.

http://lists.mysql.com/commits/41729

Chuck 

> -----Original Message-----
> From: Rafal Somla [mailto:rsomla@stripped] 
> Sent: Tuesday, February 05, 2008 5:47 AM
> To: cbell@stripped
> Cc: commits@stripped
> Subject: Re: bk commit into 6.0 tree (cbell:1.2763) BUG#33562
> 
> Chuck,
> 
> I'm concerned about efficiency of this solution. The 
> backup_error() wrapper will shuffle 
> thd->no_warnings_for_error flag each time an error or warning 
> is reported.
> 
> I think a more proper solution would be:
> 
> a) set the thd->no_warnings_for_error flag once in a connection,
> b) ensure that no code changes this flag unintentionally
> 
> Currently we know that information schema's fill() method 
> violates b) and this we could fix. I think going for this 
> solution would not only solve our problem but also promote 
> better quality of the server code (by ensuring that b) holds).
> 
> Another issue: in your patch each call to 
> Logger::report_error() will push error message on the 
> client's error stack. If an error was detected and reported, 
> at the end of backup/restore execution inside 
> execute_backup_command() function
> report_errors(info) is called (e.g. kernel.cc:337). This 
> function potentially calls util::report_mysql_error() 
> (defined in error.h) which calls
> my_printf_error() which has a side effect of pushing the 
> error on client's error stack (I think but am not sure). If 
> this is the case we end up with pushing the same error twice: 
> once in Logger::report_error() and second time in
> util::report_mysql_error() - we should not do that.
> 
> My suggestion for fixing this is to temporarily change 
> thd->no_warning_for_error value to TRUE before calling 
> my_printf_error() inside util::report_mysql_error().
> 
> Rafal
> 
> cbell@stripped wrote:
> > Below is the list of changes that have just been committed into a 
> > local 6.0 repository of cbell. When cbell does a push these changes 
> > will be propagated to the main repository and, within 24 
> hours after 
> > the push, to the public repository.
> > For information on how to access the public repository see 
> > http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> > 
> > ChangeSet@stripped, 2008-01-31 14:40:10-05:00, 
> cbell@mysql_cab_desk. +3 -0
> >   BUG#33562 : Backup: no warning about disabled storage engine
> >     
> >   Added calls to save messages so that the SHOW ERRORS and
> >   SHOW WARNINGS commands will present the errors and
> >   warnings to the client properly.
> > 
> >   sql/backup/backup_aux.h@stripped, 2008-01-31 14:40:02-05:00, 
> cbell@mysql_cab_desk. +9 -0
> >     BUG#33562 : Backup: no warning about disabled storage engine
> >     
> >     Added a method to wrapper my_error() so that is always pushes
> >     the error to the client.
> > 
> >   sql/backup/kernel.cc@stripped, 2008-01-31 14:40:03-05:00, 
> cbell@mysql_cab_desk. +9 -7
> >     BUG#33562 : Backup: no warning about disabled storage engine
> >     
> >     Added call to reset messages when backup or restore starts.
> > 
> >   sql/backup/logger.cc@stripped, 2008-01-31 14:40:04-05:00, 
> cbell@mysql_cab_desk. +4 -0
> >     BUG#33562 : Backup: no warning about disabled storage engine
> >             
> >     Added calls to save messages so that the SHOW ERRORS and
> >     SHOW WARNINGS commands will present the errors and
> >     warnings to the client properly.
> > 
> > diff -Nrup a/sql/backup/backup_aux.h b/sql/backup/backup_aux.h
> > --- a/sql/backup/backup_aux.h	2007-11-30 03:23:29 -05:00
> > +++ b/sql/backup/backup_aux.h	2008-01-31 14:40:02 -05:00
> > @@ -186,6 +186,15 @@ bool change_db(THD *thd, const Db_ref &d
> >    return 0 == ::mysql_change_db(thd,&db_name,TRUE);
> >  }
> >  
> > +inline
> > +void backup_error(int nr, myf flags, const char *str = NULL) {
> > +  bool old_value= current_thd->no_warnings_for_error;
> > +  current_thd->no_warnings_for_error= FALSE;
> > +  my_error(nr, flags, str);
> > +  current_thd->no_warnings_for_error= old_value; }
> > +
> >  /*
> >    Free the memory for the table list.
> >  */
> > diff -Nrup a/sql/backup/kernel.cc b/sql/backup/kernel.cc
> > --- a/sql/backup/kernel.cc	2008-01-31 12:26:20 -05:00
> > +++ b/sql/backup/kernel.cc	2008-01-31 14:40:03 -05:00
> > @@ -91,12 +91,14 @@ execute_backup_command(THD *thd, LEX *le
> >  
> >    using namespace backup;
> >  
> > +  mysql_reset_errors(thd, 0);
> > +
> >    /*
> >      Check access for SUPER rights. If user does not have 
> SUPER, fail with error.
> >    */
> >    if (check_global_access(thd, SUPER_ACL))
> >    {
> > -    my_error(ER_SPECIFIC_ACCESS_DENIED_ERROR, MYF(0), "SUPER");
> > +    backup_error(ER_SPECIFIC_ACCESS_DENIED_ERROR, MYF(0), "SUPER");
> >      DBUG_RETURN(ER_SPECIFIC_ACCESS_DENIED_ERROR);
> >    }
> >  
> > @@ -105,7 +107,7 @@ execute_backup_command(THD *thd, LEX *le
> >    */
> >    if (check_ob_progress_tables(thd))
> >    {
> > -    my_error(ER_BACKUP_PROGRESS_TABLES, MYF(0));
> > +    backup_error(ER_BACKUP_PROGRESS_TABLES, MYF(0));
> >      DBUG_RETURN(ER_BACKUP_PROGRESS_TABLES);
> >    }
> >  
> > @@ -113,7 +115,7 @@ execute_backup_command(THD *thd, LEX *le
> >  
> >    if (!loc || !loc->is_valid())
> >    {
> > -    my_error(ER_BACKUP_INVALID_LOC,MYF(0),lex->backup_dir.str);
> > +    backup_error(ER_BACKUP_INVALID_LOC,MYF(0),lex->backup_dir.str);
> >      DBUG_RETURN(ER_BACKUP_INVALID_LOC);
> >    }
> >  
> > @@ -125,7 +127,7 @@ execute_backup_command(THD *thd, LEX *le
> >        || (lex->sql_command == SQLCOM_BACKUP))
> >      if(start_backup_or_restore())
> >      {
> > -      my_error(ER_BACKUP_RUNNING,MYF(0));
> > +      backup_error(ER_BACKUP_RUNNING,MYF(0));
> >        DBUG_RETURN(ER_BACKUP_RUNNING);
> >      }
> >  
> > @@ -148,14 +150,14 @@ execute_backup_command(THD *thd, LEX *le
> >  
> >      if (!stream)
> >      {
> > -      my_error(ER_BACKUP_READ_LOC,MYF(0),loc->describe());
> > +      backup_error(ER_BACKUP_READ_LOC,MYF(0),loc->describe());
> >        goto restore_error;
> >      }
> >      else
> >      {
> >        if (lex->sql_command == SQLCOM_SHOW_ARCHIVE)
> >        {
> > -        my_error(ER_NOT_ALLOWED_COMMAND,MYF(0));
> > +        backup_error(ER_NOT_ALLOWED_COMMAND,MYF(0));
> >          goto restore_error;
> >        }
> >  
> > @@ -260,7 +262,7 @@ execute_backup_command(THD *thd, LEX *le
> >  
> >      if (!stream)
> >      {
> > -      my_error(ER_BACKUP_WRITE_LOC,MYF(0),loc->describe());
> > +      backup_error(ER_BACKUP_WRITE_LOC,MYF(0),loc->describe());
> >        goto backup_error;
> >      }
> >      else
> > diff -Nrup a/sql/backup/logger.cc b/sql/backup/logger.cc
> > --- a/sql/backup/logger.cc	2007-12-13 09:08:42 -05:00
> > +++ b/sql/backup/logger.cc	2008-01-31 14:40:04 -05:00
> > @@ -33,11 +33,15 @@ int Logger::write_message(log_level::val
> >         errors.push_front(new MYSQL_ERROR(::current_thd,error_code,
> >                                           
> MYSQL_ERROR::WARN_LEVEL_ERROR,msg));
> >       sql_print_error(buf);
> > +     push_warning_printf(current_thd, 
> MYSQL_ERROR::WARN_LEVEL_ERROR,
> > +                         error_code, msg);
> >       DBUG_PRINT("backup_log",("[ERROR] %s",buf));
> >       return 0;
> >  
> >     case log_level::WARNING:
> >       sql_print_warning(buf);
> > +     push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> > +                         error_code, msg);
> >       DBUG_PRINT("backup_log",("[Warning] %s",buf));
> >       return 0;
> >  
> > 
> >