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;
>
>
>
