STATUS
------
Changes requested.
REQUESTS
--------
1. Spelling error in patch comments.
2. The new error ER_BACKUP_VP_FAILED is not tested. Please add a test
case for this condition (appears in two locations in the code).
COMMENTARY
----------
3. Patch does not apply correctly to current tree. Got this rejection:
***************
*** 1232,1244 ****
Image_info::Db_iterator *dbit= info.get_dbs();
if (!dbit) {
- DBUG_RETURN(fatal_error(ER_OUT_OF_RESOURCES));
}
Image_info::Db *mydb;
- while (mydb= static_cast<Image_info::Db*>((*dbit)++)) {
if (!obs::check_db_existence(&mydb->name())) {
- DBUG_RETURN(fatal_error(ER_RESTORE_DB_EXISTS,
mydb->name().ptr()));
}
}
delete dbit;
--- 1204,1217 ----
Image_info::Db_iterator *dbit= info.get_dbs();
if (!dbit) {
+ DBUG_RETURN(fatal_error(report_error(ER_OUT_OF_RESOURCES)));
}
Image_info::Db *mydb;
+ while ((mydb= static_cast<Image_info::Db*>((*dbit)++))) {
if (!obs::check_db_existence(&mydb->name())) {
+ err= report_error(ER_RESTORE_DB_EXISTS, mydb->name().ptr());
+ DBUG_RETURN(fatal_error(err));
}
}
delete dbit;
But the code at this location is:
if (!dbit) {
DBUG_RETURN(fatal_error(ER_OUT_OF_RESOURCES));
}
Image_info::Db *mydb;
while ((mydb= static_cast<Image_info::Db*>((*dbit)++))) {
if (!obs::check_db_existence(&mydb->name())) {
delete dbit;
DBUG_RETURN(fatal_error(ER_RESTORE_DB_EXISTS,
mydb->name().ptr()));
}
}
delete dbit;
}
Note the extra delete dbit call inside the loop...should that be there?
I left it in and there were no side effects.
SUGGESTIONS
-----------
We need to start ensuring all error conditions introduced are captured
in at least one test case.
DETAILS
-------
> BUG#40303 (Backup: Wrong error send to client if several errors reported)
> BUG#40304 (Backup: Errors reported during BACKUP show as warnings on the
> error stack)
>
> Before: Errors reported using backup::Logger class could be saved and in
> case several errors are reported, the last one was sent to client as an
> error reply from BACKUP/RESTORE command.
>
> After: Errors reported with backup::Logger are reported to the server
> using my_printf_error(..) function. This function pushes error on the
> error stack (unles told otherwise) and stores the first reported error
> so that it is send to the client.
>
> Additionally.
>
> - Function log_error(..) is moved to the Logger class.
>
> - Code is changed to consistently use Logger::report/log_error() for error
> reporting.
>
> - Semantics of Backup_restore_ctx::fatal_error() has been changed to
> discourage its use as an error reporting facility. Now it is an internal helper
>
> method which moves the context object into error state. It is used only by the
>
> class methods and it is made private. The idea is that context object should
> move into error state only as a result of executing one of its methods (not
> externally).
...
> sql/backup/data_backup.cc
> - Use Logger instance for reporting errors.
> - Report few errors not logged previously (e.g. when (un)block_commits(..)
> fails).
> - Remove cancel_backup() calls as this is done in Backup_restore_ctx::close()
> called from destructor.
> - Inside Backup_pump class: log errors only if logger is set.
> - Fix the shutdown logic in restore_table_data(..). Drivers need to be correctly
>
> de-initialized by a call to ->end() during normal operation or ->cacnel()
> in case
> of interruption.
[1] Spelling error. '->cancel()'
...
> === modified file 'sql/share/errmsg.txt'
> --- a/sql/share/errmsg.txt 2008-11-19 22:08:05 +0000
> +++ b/sql/share/errmsg.txt 2008-11-20 12:41:07 +0000
> @@ -6436,3 +6436,5 @@ ER_RESTORE_DB_EXISTS
> eng "Database \'%-.64s\' already exists. Use OVERWRITE flag to overwrite."
> ER_QUERY_CACHE_DISABLED
> eng "Query cache is disabled; restart the server with query_cache_type=1 to enable
> it"
> +ER_BACKUP_VP_FAILED
> + eng "Could not create the validity point"
[2] This error is not tested anywhere. I think it needs to be added as a
test case to the backup, backup_vp_*, or backup_online_testing tests.
This may require debug insertion to achieve, but I think it warrants
testing. We need to see what happens if the commit blocker fails in general.
Chuck
