STATUS
------
Changes requested
REQUESTS
--------
1. Add comments stated in patch comments concerning fatal_error() to the
doxygen comments for the method.
2. Please restore the { } around the debug return macro. This could cause
compilation issues on some platforms.
3. I see an example of where including refactoring in this patch has left an
inconsistency. The changes to the tests to use @backupdir and @datadir are
warranted (but outside of the scope of the bug report) but do not change all
tests in the backup and backup_engines suite. Please open a bug report
requesting all tests be rewritten using this technique.
4. I don't understand the need to change/reformat the code in the following
manner:
if(something())
{
return(error);
}
to
ret= something()
if(ret)
return(error);
If this is a style issue or developer preference please refrain from making
these sort of changes. While mundane, changes like these can be the source
of errors. This is done several places in the code. See details for
examples.
5. Why is the assertion for m_log removed? Don't we need that?
6. Why is the report_error() not used in the patch fragment in kernel.cc?
COMMENTARY
----------
It's a good patch. I have very little objections to the code in the patch
that is within the scope of the bug report, but I feel there are too many
extra coding bits added thereby increasing the risk of this patch
unnecessarily.
SUGGESTIONS
-----------
7. Don't include refactoring or other reorganization of the code where the
changes are not within the scope of the bug report. There are several places
where this takes place in this patch and I do not see the justification for
it. I will not reject the patch on this point this time, but in the future
please do not include changes that are out of the scope of the bug report --
they introduce risk and complicate an already large patch. I suggest making
either separate patches and/or new bug reports to handle these sort of
changes.
DETAILS
-------
> 2731 Rafal Somla 2008-11-12
> 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 do 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).
[1] This should be added to code doxygen comments.
...
> sql/backup/kernel.cc
> - Use explicit calls to report/log_error(..) for error
> logging - now fatal_error(..)
> does not log anything.
> - Simplified send_error(..) implementation - now it does
> not have to look for the
> last error reported. The first reported error is
> automatically stored by the
> server and send to the client.
> - Move error reporting context preparations to Logger::init(..).
> - To clarify the code, calls to s->next_chunk() have been
> moved to wrapper
> functions read_header(..), read_catalog(..) and
> read_meta_data(..) where they
> really belong.
[7] I think this is out of scope of the patch. Nothing in the bug report or
subsequent comments mentions the need for this change. I see the need for
refactoring, but this doesn't improve the patch per the bug report. Indeed,
it adds risk and therefore the potential of introducing more bugs. Please
refrain from doing this sort of thing in the same patch as a bug report.
...
> === modified file 'mysql-test/suite/backup/t/backup_errors.test'
> --- a/mysql-test/suite/backup/t/backup_errors.test
> 2008-11-05 09:41:15 +0000
> +++ b/mysql-test/suite/backup/t/backup_errors.test
> 2008-11-12 11:20:04 +0000
> @@ -7,11 +7,14 @@
> # TODO: When we know how to do that, check that the backup
> progress table
> # contains appropriate rows when errors have been detected.
>
> +let $bdir= `SELECT @@backupdir`;
> +let $ddir= `SELECT @@datadir`;
[3] This sort of refactoring has left the tests in an inconsistent state --
some use this while others still default to assuming backupdir == datadir.
Please open a bug report to have all tests changed to use this technique.
...
> @@ -1013,9 +1021,9 @@ int Scheduler::unlock()
>
> for(Pump_iterator it(*this); it; ++it)
> {
> - if (it->unlock())
> + if (it->unlock()) // logs errors
> {
> - cancel_backup(); // Note: never errors.
> + remove_pump(it); // Note: never errors.
> return ERROR;
> }
> if (it->state == backup_state::FINISHING)
> @@ -1072,9 +1080,7 @@ int Backup_pump::begin()
> if (ERROR == m_drv->begin(m_bw.buf_size))
> {
> state= backup_state::ERROR;
> - // We check if logger is always setup. Later the assertion can
> - // be replaced with "if (m_log)"
> - DBUG_ASSERT(m_log);
[5] Why was this removed (it happens other places too)? Don't we need it?
> + if (m_log)
> m_log->report_error(ER_BACKUP_INIT_BACKUP_DRIVER, m_name);
> return ERROR;
> }
...
> === modified file 'sql/backup/kernel.cc'
> --- a/sql/backup/kernel.cc 2008-10-30 20:02:15 +0000
> +++ b/sql/backup/kernel.cc 2008-11-12 11:20:04 +0000
> @@ -154,11 +154,8 @@ execute_backup_command(THD *thd, LEX *le
> folders in the path could have been moved, deleted, etc.
> */
> if (backupdir->length() && my_access(backupdir->c_ptr(),
> (F_OK|W_OK)))
> - {
> - context.fatal_error(ER_BACKUP_BACKUPDIR, backupdir->c_ptr());
> DBUG_RETURN(send_error(context, ER_BACKUP_BACKUPDIR,
> backupdir->c_ptr()));
> - }
> -
[6] Why isn't report_error() called here like in the other places (see
below)?
> +
> switch (lex->sql_command) {
>
> case SQLCOM_BACKUP:
> @@ -195,7 +192,7 @@ execute_backup_command(THD *thd, LEX *le
>
> if (info->db_count() == 0)
> {
> - context.fatal_error(ER_BACKUP_NOTHING_TO_BACKUP);
> + context.report_error(ER_BACKUP_NOTHING_TO_BACKUP);
> DBUG_RETURN(send_error(context, ER_BACKUP_NOTHING_TO_BACKUP));
> }
...
> +
> + In case of error, we write only to backup logs, because
> check_global_access()
> + pushes the same error on the error stack.
> */
> - if (check_global_access(m_thd, SUPER_ACL))
> - {
> - fatal_error(ER_SPECIFIC_ACCESS_DENIED_ERROR, "SUPER");
> - return m_error;
> - }
> + ret= check_global_access(m_thd, SUPER_ACL);
> + if (ret)
> + return
[4] This is just one of the many places where this reformatting takes place.
Since there is nothing wrong with it aside from the fact that extra effort
was used where it wasn't needed, I will not object this time. If this is
warranted, please make a note in the patch comments otherwise in the future
we may wish to request these sort of changes be removed from patches.
...
> + /*
> + Now backup image has been written. Set m_remove_loc to
> FALSE, so that the
> + backup file is not removed in Backup_restore_ctx::close().
> + */
> + m_remove_loc= FALSE;
> report_stats_post(info); // Never errors
>
> DBUG_PRINT("backup",("Backup done."));
> @@ -1134,9 +1095,7 @@ int Backup_restore_ctx::restore_triggers
>
> Image_info::Iterator *dbit= m_catalog->get_dbs();
> if (!dbit)
> - {
> - DBUG_RETURN(fatal_error(ER_OUT_OF_RESOURCES));
> - }
> + DBUG_RETURN(fatal_error(report_error(ER_OUT_OF_RESOURCES)));
[2] Removing the { } from around the macro can (and has) caused compilation
errors (or was it warnings?) on some platforms. Please restore these.
...
Chuck
