Hi Chuck,
I want to propose a different solution to the same problem.
My rationale is that I want to separate error reporting in the BACKUP/RESTORE
command from logging errors in the error log and progress table via Logger class.
Generally there are several destinations where error or warning message can end up:
a) error response from BACKUP/RESTORE command,
b) SQL connection's error/warning stack which can be examined with SHOW
ERRORS/WARNINGS,
c) error log of the server,
d) backup progress table (or equivalent),
e) debug trace of the server (if enabled).
Now, if we execute backup/restore operation asynchronously in a separate thread,
then destinations a) and b) can not be used. The BACKUP/RESTORE command will
usually return immediately without any errors and then, if error happens later,
it should be reported to destinations c-e).
My intention behind the Logger class was to make it a general interface to all
online backup logging facilities. If all backup code consistently uses Logger
methods to report errors, warnings and other messages, then they all should be
reported in the same way and it is easy to change the way they are reported.
Since Logger is supposed to be a general class for reporting during the whole
backup/restore process, it can not report to destinations a) and b), as they are
not (will be not) available during the process. This is why
execute_backup_command() uses function report_errors() whose task is to look at
errors reported during backup/restore operation and stored inside the logger and
send them to the client.
Report_errors() doesn't do its job very well and this is why we have this bug.
To fix it, I propose to not change the Logger class, but the report_errors()
function to do what it is supposed to do, including pushing errors and warnings
if there are many of them. Actually few more things need to be fixed to make
everything work as I imagined it. I prepared a patch introducing necessary
changes - it should hopefully explain more what solution I have in mind. Please
have a look and tell me if you agree with it.
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-23 13:57:48-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.
>
> mysql-test/r/backup_no_data.result@stripped, 2008-01-23 13:57:37-05:00,
> cbell@mysql_cab_desk. +2 -0
> BUG#33562 : Backup: no warning about disabled storage engine
>
> New result file
>
> mysql-test/r/backup_no_engine.result@stripped, 2008-01-23 13:57:37-05:00,
> cbell@mysql_cab_desk. +2 -0
> BUG#33562 : Backup: no warning about disabled storage engine
>
> New result file
>
> sql/backup/kernel.cc@stripped, 2008-01-23 13:57:38-05:00, cbell@mysql_cab_desk. +2 -0
> 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-23 13:57:39-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.
>
===== sql/backup/backup_kernel.h 1.5 vs edited =====
--- 1.5/sql/backup/backup_kernel.h 2008-01-24 11:53:22 +01:00
+++ edited/sql/backup/backup_kernel.h 2008-01-24 11:36:44 +01:00
@@ -146,7 +146,7 @@ class Backup_info: public Image_info, pu
int add_dbs(List< ::LEX_STRING >&);
int add_all_dbs();
- bool close();
+ int close();
private:
===== sql/backup/kernel.cc 1.15 vs edited =====
--- 1.15/sql/backup/kernel.cc 2008-01-24 11:53:22 +01:00
+++ edited/sql/backup/kernel.cc 2008-01-24 11:50:31 +01:00
@@ -47,12 +47,6 @@ static void finish_backup_or_restore();
*/
static int report_errors(THD*, Logger&, int, ...);
-/*
- Check if info object is valid. If not, report error to client.
- */
-static int check_info(THD*,Backup_info&);
-static int check_info(THD*,Restore_info&);
-
static bool send_summary(THD*,const Backup_info&);
static bool send_summary(THD*,const Restore_info&);
@@ -90,6 +84,8 @@ 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.
*/
@@ -160,9 +156,10 @@ execute_backup_command(THD *thd, LEX *le
info.backup_prog_id= backup_prog_id;
- if (check_info(thd,info))
+ if (!info.is_valid())
{
stop= my_time(0);
+ my_error(ER_BACKUP_RESTORE_PREPARE,MYF(0));
goto restore_error;
}
@@ -179,29 +176,29 @@ execute_backup_command(THD *thd, LEX *le
{
stop= my_time(0);
info.save_end_time(stop);
+ my_error(ER_BACKUP_RESTORE_PREPARE,MYF(0)); // TODO: better error message
goto restore_error;
}
info.save_errors();
- info.restore_all_dbs();
+ res= info.restore_all_dbs();
- if (check_info(thd,info))
+ if (res)
{
stop= my_time(0);
info.save_end_time(stop);
- goto restore_error;
+ res= ER_BACKUP_RESTORE_PREPARE; // TODO: better error message (?)
+ goto send_restore_reply;
}
- info.clear_saved_errors();
-
res= mysql_restore(thd,info,*stream);
stop= my_time(0);
info.save_end_time(stop);
if (res)
{
- report_errors(thd,info,ER_BACKUP_RESTORE);
- goto restore_error;
+ res= ER_BACKUP_RESTORE;
+ goto send_restore_reply;
}
report_ob_num_objects(backup_prog_id, info.table_count);
@@ -211,7 +208,20 @@ execute_backup_command(THD *thd, LEX *le
BACKUP_BREAKPOINT("bp_complete_state");
info.report_error(log_level::INFO,ER_BACKUP_RESTORE_DONE);
- send_summary(thd,info);
+
+ /*
+ Send reply to client. If res != 0 then error reply will be sent,
+ otherwise the normal reply (currently backup_id). In either case errors
+ and warnings stored inside info structured are pushed to client's
+ error/warning list.
+ */
+ send_restore_reply:
+
+ if ((res= report_errors(thd, info, res)))
+ goto restore_error;
+ else
+ send_summary(thd,info);
+
} // if (!stream)
goto finish_restore;
@@ -266,7 +276,10 @@ execute_backup_command(THD *thd, LEX *le
skip new or dropped tables.
*/
if (!DDL_blocker->block_DDL(thd))
+ {
+ my_error(ER_BACKUP_BACKUP_PREPARE,MYF(0)); // TODO: better error message
goto backup_error;
+ }
Backup_info info(thd);
@@ -277,8 +290,11 @@ execute_backup_command(THD *thd, LEX *le
info.backup_prog_id= backup_prog_id;
- if (check_info(thd,info))
+ if (!info.is_valid())
+ {
+ my_error(ER_BACKUP_BACKUP_PREPARE,MYF(0)); // TODO: better error message
goto backup_error;
+ }
info.report_error(log_level::INFO,ER_BACKUP_BACKUP_START);
info.save_start_time(start);
@@ -290,42 +306,42 @@ execute_backup_command(THD *thd, LEX *le
if (lex->db_list.is_empty())
{
info.write_message(log_level::INFO,"Backing up all databases");
- info.add_all_dbs(); // backup all databases
+ res= info.add_all_dbs(); // backup all databases
}
else
{
info.write_message(log_level::INFO,"Backing up selected databases");
- info.add_dbs(lex->db_list); // backup databases specified by user
+ res= info.add_dbs(lex->db_list); // backup databases specified by user
}
- report_ob_num_objects(backup_prog_id, info.table_count);
-
- if (check_info(thd,info))
+ if (res)
{
stop= my_time(0);
info.save_end_time(stop);
- goto backup_error;
+ res= ER_BACKUP_BACKUP_PREPARE; // TODO: better error message (?)
+ goto send_backup_reply;
}
- info.close();
+ report_ob_num_objects(backup_prog_id, info.table_count);
+
+ res= info.close();
- if (check_info(thd,info))
+ if (res)
{
stop= my_time(0);
info.save_end_time(stop);
- goto backup_error;
+ res= ER_BACKUP_BACKUP_PREPARE; // TODO: better error message (?)
+ goto send_backup_reply;
}
- info.clear_saved_errors();
-
res= mysql_backup(thd,info,*stream);
stop= my_time(0);
info.save_end_time(stop);
if (res)
{
- report_errors(thd,info,ER_BACKUP_BACKUP);
- goto backup_error;
+ res= ER_BACKUP_BACKUP;
+ goto send_backup_reply;
}
report_ob_size(info.backup_prog_id, info.data_size);
@@ -334,8 +350,20 @@ execute_backup_command(THD *thd, LEX *le
BACKUP_BREAKPOINT("bp_complete_state");
info.report_error(log_level::INFO,ER_BACKUP_BACKUP_DONE);
- send_summary(thd,info);
+ /*
+ Send reply to client. If res != 0 then error reply will be sent,
+ otherwise the normal reply (currently backup_id). In either case errors
+ and warnings stored inside info structured are pushed to client's
+ error/warning list.
+ */
+ send_backup_reply:
+
+ if ((res= report_errors(thd, info, res)))
+ goto backup_error;
+ else
+ send_summary(thd,info);
+
} // if (!stream)
goto finish_backup;
@@ -641,7 +669,7 @@ void save_snapshot_info(const Snapshot_i
After this call the @c Backup_info object is ready for use as a catalogue
for backup stream functions such as @c bstream_wr_preamble().
*/
-bool Backup_info::close()
+int Backup_info::close()
{
bool ok= is_valid();
@@ -674,7 +702,7 @@ bool Backup_info::close()
if (m_state == INIT)
m_state= READY;
- return ok;
+ return ok? OK : ERROR;
}
@@ -1727,45 +1755,59 @@ TABLE *create_schema_table(THD *thd, TAB
namespace backup {
/**
- Report errors.
+ Report errors stored in a logger object to a client.
- Current implementation reports the last error saved in the logger if it exist.
- Otherwise it reports error given by @c error_code.
+ This function pushes all errors and warnings stored in the logger to the
+ client side. If @c error_code is non-zero, then also an error is reported to
+ the client. It is either the last error (not warning) stored in the logger,
+ if there is one, or the error given by @c error_code. In the latter case
+ additional parameters used to compose error message can be passed.
+
+ If @c error_code is zero, then no reply is sent to the client but still all
+ errors and warnings from the logger are pushed to the client side.
+
+ @returns 0 if no error was reported, otherwise error code of the error
+ reported to the client.
*/
int report_errors(THD *thd, Logger &log, int error_code, ...)
{
- MYSQL_ERROR *error= log.last_saved_error();
+ MYSQL_ERROR *error= log.last_error();
+ List_iterator<MYSQL_ERROR> it(log.errors);
+ MYSQL_ERROR *p;
+
+ // push all saved warnings and errors
- if (error && !util::report_mysql_error(thd,error,error_code))
+ while ((p=it++))
{
- if (error->code)
- error_code= error->code;
+ // Don't push the last error if it is going to be reported below
+ if (p == error && error_code)
+ continue;
+ push_warning_printf(thd, p->level, p->code, p->msg);
}
- else // there are no error information in the logger - report error_code
- {
- char buf[ERRMSGSIZE + 20];
- va_list args;
- va_start(args,error_code);
- my_vsnprintf(buf,sizeof(buf),ER_SAFE(error_code),args);
- my_printf_error(error_code,buf,MYF(0));
+ // report error if error_code is non-zero
- va_end(args);
+ if (error_code)
+ {
+ if (error && !util::report_mysql_error(thd,error,error_code))
+ {
+ if (error->code)
+ error_code= error->code;
+ }
+ else // there are no error information in the logger - report error_code
+ {
+ char buf[ERRMSGSIZE + 20];
+ va_list args;
+ va_start(args,error_code);
+
+ my_vsnprintf(buf,sizeof(buf),ER_SAFE(error_code),args);
+ my_printf_error(error_code,buf,MYF(0));
+
+ va_end(args);
+ }
}
return error_code;
-}
-
-inline
-int check_info(THD *thd, Backup_info &info)
-{
- return info.is_valid() ? OK : report_errors(thd,info,ER_BACKUP_BACKUP_PREPARE);
-}
-
-inline
-int check_info(THD *thd, Restore_info &info)
-{
- return info.is_valid() ? OK : report_errors(thd,info,ER_BACKUP_RESTORE_PREPARE);
}
/**
===== sql/backup/logger.cc 1.3 vs edited =====
--- 1.3/sql/backup/logger.cc 2008-01-24 11:53:22 +01:00
+++ edited/sql/backup/logger.cc 2008-01-24 11:40:05 +01:00
@@ -24,14 +24,42 @@ int Logger::write_message(log_level::val
{
const char *prefix= m_type == BACKUP ? "Backup" : "Restore";
char buf[ERRMSGSIZE + 30];
+ MYSQL_ERROR *error= NULL;
my_snprintf(buf,sizeof(buf),"%s: %s",prefix,msg);
+ // create MYSQL_ERROR object if this is error or warning
+
+ if ((level == log_level::ERROR || level == log_level::WARNING))
+ {
+ MYSQL_ERROR::enum_warning_level sql_lvl =
+ level == log_level::ERROR ? MYSQL_ERROR::WARN_LEVEL_ERROR
+ : MYSQL_ERROR::WARN_LEVEL_WARN;
+ error= new MYSQL_ERROR(::current_thd, error_code, sql_lvl, msg);
+
+ // if we are storing errors and warnings, add it to the list
+
+ if (m_save_errors)
+ errors.push_back(error);
+ }
+
+ // if this is error, save it as m_last_error
+
+ if (level == log_level::ERROR)
+ {
+ if (!m_save_errors)
+ delete m_last_error;
+
+ m_last_error= error;
+ }
+ else if (!m_save_errors)
+ delete error;
+
+ // now send message to the logs
+
switch (level) {
+
case log_level::ERROR:
- if (m_save_errors)
- errors.push_front(new MYSQL_ERROR(::current_thd,error_code,
- MYSQL_ERROR::WARN_LEVEL_ERROR,msg));
sql_print_error(buf);
DBUG_PRINT("backup_log",("[ERROR] %s",buf));
return 0;
===== sql/backup/logger.h 1.2 vs edited =====
--- 1.2/sql/backup/logger.h 2008-01-24 11:53:22 +01:00
+++ edited/sql/backup/logger.h 2008-01-24 11:25:30 +01:00
@@ -33,7 +33,7 @@ class Logger
enum enum_type { BACKUP, RESTORE } m_type;
- Logger(enum_type type): m_type(type),m_save_errors(FALSE)
+ Logger(enum_type type): m_type(type), m_last_error(NULL), m_save_errors(FALSE)
{}
int write_message(log_level::value level , int error_code, const char *msg);
@@ -104,15 +104,20 @@ class Logger
/// Delete all saved errors to free resources.
void clear_saved_errors()
- { errors.delete_elements(); }
+ {
+ errors.delete_elements();
+ m_last_error= NULL;
+ }
/// Return a pointer to most recent saved error.
- MYSQL_ERROR *last_saved_error()
- { return errors.is_empty() ? NULL : errors.head(); }
+ MYSQL_ERROR *last_error()
+ { return m_last_error; }
+
+ List<MYSQL_ERROR> errors; ///< List of saved errors
private:
- List<MYSQL_ERROR> errors; ///< Used to store saved errors
+ MYSQL_ERROR *m_last_error; ///< Pointer to the last saved error
bool m_save_errors; ///< Flag telling if errors should be saved
int v_report_error(log_level::value,int,va_list);
