| List: | Commits | « Previous MessageNext Message » | |
| From: | Øystein Grøvlen | Date: | November 17 2008 2:46pm |
| Subject: | Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2731) Bug#40303 Bug#40304 | ||
| View as plain text | |||
STATUS
======
While patch is generally good and provides a good clean-up of error
reporting, some error reports are now less informative or less
specific. I request that those issues are looked into.
REQUESTS
========
1. Some error reports are now less informative than previously. (See
[2], [7]). I think that should be fixed.
2. Fix long lines: [6], [13], [19].
COMMENTARY
==========
1. A lot of the error messages used for backup have very little value
for a user and might as well have been replaced by "Internal error
during BACKUP/RESTORE". What use is messages like these to the
user:
- "Error when preparing for backup operation"
- "Can't enumerate server tables"
- "Can't read backup archive preamble"
- "Error when reading summary section of backup image"
- "Can't create %-.64s backup driver"
- "Could not create the validity point"
Error messages should give useful information to user, not reflect
the internal design/implementation of the software!
2. Changes in this patch (e.g., [5]) illustrate that making
Backup_restore_ctx a sub-class of Logger was not a good design
decision. Inheritance should only be used for is-a relationships.
I do not think it makes sense to say that a context "is a" logger.
However, a logger exist for a given context. Hence, encapsulation
should be used instead.
SUGGESTIONS
===========
1. Fix typos: [1], [4]
2. Skip masking of error numbers in SHOW WARNING [3].
3. Change Backup_info(Backup_restore_ctx&) to
Backup_info(Logger&, THD*) [5].
4. Try to give a more specific error message than
BACKUP_VP_FAILED. [8]
5. Define a constant to be used for drv and active arrays. [9]
6. execute_backup_command: Why is report_error called before
send_error in one place but not the others [10]?
7. prepare_for_backup: Add a comment for handling errors from
Logger::init as to why report_error is not called.
8. I do not understand why you have changed code to assign return
values to a local variable instead of using if-statements
directly [11], [16], [18]. What is the value of that?
If using such variables, please, be consistent wrt naming. Some
places it is called ret, other places err. [20]
9. Is it OK to use different error codes for report_error and
fatal_error for the same error? [12], [14], [15].
10. Since setting m_error is encapsulated in a method (fatal_error), I
think reading it should also be encapsulated. [17].
11. Add missing call of fatal_error? [21]
12. Logger::write_message: Add a comment when declaring out on what is
the difference between out and msg, and when each of them will be
used.
13. Private copy constructor has already been created. Should not
provide implementation since we want compile error. [22]
14. If changes wrt to calling bstream_next_chunk is not directly
related to the purpose of this patch, I suggest leaving them
out. [23]
[X] refer to inline comments.
--
Øystein
Rafal Somla wrote:
> #At file:///ext/mysql/bzr/backup/bug40303/
>
> 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.
[1] Typo: 'changed do consistently'
>
> - 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).
>
> - Improved error reporting in few places.
> modified:
> mysql-test/suite/backup/r/backup_errors.result
> mysql-test/suite/backup/r/backup_views.result
> mysql-test/suite/backup/t/backup_errors.test
> mysql-test/suite/backup/t/backup_myisam1.test
> mysql-test/suite/backup/t/backup_views.test
> sql/backup/backup_info.cc
> sql/backup/backup_info.h
> sql/backup/backup_kernel.h
> sql/backup/data_backup.cc
> sql/backup/kernel.cc
> sql/backup/logger.cc
> sql/backup/logger.h
> sql/backup/restore_info.h
> sql/backup/stream.cc
> sql/backup/stream.h
> sql/share/errmsg.txt
>
> per-file messages:
> mysql-test/suite/backup/t/backup_errors.test
> - Add SHOW WARNINGS to see what is pushed on the error stack in
case of errors.
> - Use @@backupdir and @@datadir for the corresponding paths.
> mysql-test/suite/backup/t/backup_myisam1.test
> - Update the test, taking into account that after this patch
failed BACKUP does not
> leave the unfinished backup image file around.
> - Use @@backupdir for the corresponding path.
> mysql-test/suite/backup/t/backup_views.test
> - Update the test taking into account that now RESTORE returns
the first
> error reported.
> sql/backup/backup_info.cc
> - Use m_log.report_error(...) for error reporting.
> - Use the m_thd member instead of refering to a context class
instance.
> sql/backup/backup_info.h
> Instead of storing a reference to a complete Backup_restore_ctx
instance,
> store only what is needed: a Logger instance and a THD handle.
> sql/backup/backup_kernel.h
> - Move log_error(..) method to the Logger class.
> - Make fatal_error(...) private and change its semantics. Now it
only moves context
> object to an error state, but it does not report any errors -
Logger methods should
> be used for reporting them.
> 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.
> 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.
> sql/backup/logger.cc
> - Remove code saving reported errors.
> - Use my_printf_error(..) instead of push_warning_printf(..) for
> reporting errors to the server core. This has the side effect of
> storing the first reported error so that it is sent to the client.
> sql/backup/logger.h
> - Add log_error(..) method moved here from Backup_restore_ctx.
> - Remove methods for saving reported errors - not needed any more
because the
> first reported error is remembered by the server core.
> - Add error_reported() method.
> - Add error context cleanup (on server level) to init() method,
instead of doing
> it inside Backup_restore_ctx class.
> sql/backup/restore_info.h
> Only store what is needed: reference to Logger and THD handle.
> sql/backup/stream.cc
> Remove Input_stream::next_chunk() method.
> sql/backup/stream.h
> - Move to next chunk after reading header, catalogue and metadata
sections.
> Do it inside wrapper read_*(..) functions using direct calls to
bstream
> library.
> - Remove Input_stream::next_chunk() method.
> sql/share/errmsg.txt
> Add error message for reporting problems during validity point
> creation.
> === modified file 'mysql-test/suite/backup/r/backup_errors.result'
> --- a/mysql-test/suite/backup/r/backup_errors.result 2008-11-05
09:41:15 +0000
> +++ b/mysql-test/suite/backup/r/backup_errors.result 2008-11-12
11:20:04 +0000
> @@ -7,19 +7,36 @@ CREATE DATABASE bdb;
> CREATE TABLE bdb.t1(a int) ENGINE=MEMORY;
> BACKUP DATABASE adb TO '';
> ERROR HY000: Malformed file path ''
> +SHOW WARNINGS;
> +Level Code Message
> +Error # Malformed file path ''
> BACKUP DATABASE adb TO "bdb/t1.frm";
> ERROR HY000: Can't write to backup location 'bdb/t1.frm' (file
already exists?)
> +SHOW WARNINGS;
> +Level Code Message
> +Error # Can't write to backup location 'bdb/t1.frm' (file already
exists?)
> BACKUP DATABASE adb TO "test.bak";
> backup_id
> #
> +SHOW WARNINGS;
> +Level Code Message
> BACKUP DATABASE adb TO "test.bak";
> ERROR HY000: Can't write to backup location 'test.bak' (file already
exists?)
> +SHOW WARNINGS;
> +Level Code Message
> +Error # Can't write to backup location 'test.bak' (file already exists?)
> DROP DATABASE IF EXISTS foo;
> DROP DATABASE IF EXISTS bar;
> BACKUP DATABASE foo TO 'test.bak';
> ERROR 42000: Unknown database 'foo'
> +SHOW WARNINGS;
> +Level Code Message
> +Error 1049 Unknown database 'foo'
> BACKUP DATABASE test,foo,bdb,bar TO 'test.bak';
> ERROR 42000: Unknown database 'foo,bar'
> +SHOW WARNINGS;
> +Level Code Message
> +Error # Unknown database 'foo,bar'
> BACKUP DATABASE foo,test,bar,foo TO 'test.bak';
> ERROR 42000: Not unique database: 'foo'
> use adb;
> @@ -49,21 +66,39 @@ DROP DATABASE bdb;
> Backup of mysql, information_schema scenario 1
> BACKUP DATABASE mysql TO 't.bak';
> ERROR HY000: Database 'mysql' cannot be included in a backup
> +SHOW WARNINGS;
> +Level Code Message
> +Error # Database 'mysql' cannot be included in a backup
> Backup of mysql, information_schema scenario 2
> BACKUP DATABASE information_schema TO 't.bak';
> ERROR HY000: Database 'information_schema' cannot be included in a
backup
> +SHOW WARNINGS;
> +Level Code Message
> +Error # Database 'information_schema' cannot be included in a backup
> Backup of mysql, information_schema scenario 3
> BACKUP DATABASE mysql, information_schema TO 't.bak';
> ERROR HY000: Database 'mysql' cannot be included in a backup
> +SHOW WARNINGS;
> +Level Code Message
> +Error # Database 'mysql' cannot be included in a backup
> Backup of mysql, information_schema scenario 4
> BACKUP DATABASE mysql, test TO 't.bak';
> ERROR HY000: Database 'mysql' cannot be included in a backup
> +SHOW WARNINGS;
> +Level Code Message
> +Error # Database 'mysql' cannot be included in a backup
> Backup of mysql, information_schema scenario 5
> BACKUP DATABASE information_schema, test TO 't.bak';
> ERROR HY000: Database 'information_schema' cannot be included in a
backup
> +SHOW WARNINGS;
> +Level Code Message
> +Error # Database 'information_schema' cannot be included in a backup
> Backup of mysql, information_schema scenario 6
> BACKUP DATABASE mysql, information_schema, test TO 't.bak';
> ERROR HY000: Database 'mysql' cannot be included in a backup
> +SHOW WARNINGS;
> +Level Code Message
> +Error # Database 'mysql' cannot be included in a backup
> Making copies of progress tables.
> CREATE TABLE IF NOT EXISTS test.ob_copy LIKE mysql.backup_history;
> CREATE TABLE IF NOT EXISTS test.obp_copy LIKE mysql.backup_progress;
> @@ -78,7 +113,7 @@ DROP TABLE mysql.backup_history;
> Backup the database;
> BACKUP DATABASE test_ob_error TO 'ob_err.bak';
> ERROR HY000: Can't open the backup logs as tables. Check
'mysql.backup_history' and 'mysql.backup_progress' or run mysql_upgrade
to repair.
> -SHOW ERRORS;
> +SHOW WARNINGS;
> Level Code Message
> Error # Table 'mysql.backup_history' doesn't exist
> Error # Cannot create backup/restore execution context
> @@ -89,7 +124,7 @@ DROP TABLE mysql.backup_progress;
> Backup the database;
> BACKUP DATABASE test_ob_error TO 'ob_err.bak';
> ERROR HY000: Can't open the backup logs as tables. Check
'mysql.backup_history' and 'mysql.backup_progress' or run mysql_upgrade
to repair.
> -SHOW ERRORS;
> +SHOW WARNINGS;
> Level Code Message
> Error # Table 'mysql.backup_progress' doesn't exist
> Error # Cannot create backup/restore execution context
>
> === modified file 'mysql-test/suite/backup/r/backup_views.result'
> --- a/mysql-test/suite/backup/r/backup_views.result 2008-10-07
17:15:44 +0000
> +++ b/mysql-test/suite/backup/r/backup_views.result 2008-11-12
11:20:04 +0000
> @@ -277,10 +277,10 @@ DROP DATABASE bup_db2;
> Restore database.
> restore database with view dependency to other, non-existing db
> RESTORE FROM 'bup_objectview1.bak';
> -ERROR HY000: Could not restore view `bup_db1`.`v5`. Please check the
view definition for possible missing dependencies.
> +ERROR 42S02: Table 'bup_db2.t2' doesn't exist
> DROP DATABASE bup_db1;
> RESTORE FROM 'bup_objectview2.bak';
> -ERROR HY000: Could not restore view `bup_db2`.`student_details`.
Please check the view definition for possible missing dependencies.
> +ERROR 42S02: Table 'bup_db1.t3' doesn't exist
[2] The new error message is not as informative as the old one. I do
not think this is a move in the right direction.
> DROP DATABASE bup_db2;
> RESTORE FROM 'bup_objectview.bak';
> backup_id
>
> === 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`;
> +
> --disable_warnings
> DROP DATABASE IF EXISTS adb;
> DROP DATABASE IF EXISTS bdb;
> --error 0,1
> ---remove_file $MYSQLTEST_VARDIR/master-data/test.bak
> +--remove_file $bdir/test.bak
> --enable_warnings
>
> # non-existent backup archive
> @@ -25,19 +28,27 @@ CREATE TABLE bdb.t1(a int) ENGINE=MEMORY
> # invalid location
> --error ER_BAD_PATH
> BACKUP DATABASE adb TO '';
> +--replace_column 2 #
> +SHOW WARNINGS;
[3] Do you really need mask the error code here? Should not error
codes be pretty stable? Would it not be good to get a warning if
someone does changes that changes the numbers of existing error codes?
>
> # don't overwrite existing files
> --error ER_BACKUP_WRITE_LOC
> BACKUP DATABASE adb TO "bdb/t1.frm";
> +--replace_column 2 #
> +SHOW WARNINGS;
>
> --replace_column 1 #
> BACKUP DATABASE adb TO "test.bak";
> +--replace_column 2 #
> +SHOW WARNINGS;
>
> # don't overwrite existing backup image
> --error ER_BACKUP_WRITE_LOC
> BACKUP DATABASE adb TO "test.bak";
> +--replace_column 2 #
> +SHOW WARNINGS;
>
> ---remove_file $MYSQLTEST_VARDIR/master-data/test.bak
> +--remove_file $bdir/test.bak
>
> # non-existent database
> --disable_warnings
> @@ -47,8 +58,11 @@ DROP DATABASE IF EXISTS bar;
>
> -- error ER_BAD_DB_ERROR
> BACKUP DATABASE foo TO 'test.bak';
> +SHOW WARNINGS;
> -- error ER_BAD_DB_ERROR
> BACKUP DATABASE test,foo,bdb,bar TO 'test.bak';
> +--replace_column 2 #
> +SHOW WARNINGS;
>
> # repeated database
> -- error ER_NONUNIQ_DB
> @@ -93,7 +107,7 @@ DROP DATABASE bdb;
> # Note: the file should be removed - if it is not, the following command
> # will fail and we will be warned that something is wrong with the test
> --error 1
> ---remove_file $MYSQLTEST_VARDIR/master-data/test.bak
> +--remove_file $bdir/test.bak
>
> # Check error conditions for including mysql and information_schema in
> # the list of databases.
> @@ -117,49 +131,61 @@ DROP DATABASE bdb;
> --echo Backup of mysql, information_schema scenario 1
> --error ER_BACKUP_CANNOT_INCLUDE_DB
> BACKUP DATABASE mysql TO 't.bak';
> +--replace_column 2 #
> +SHOW WARNINGS;
>
> --error 0, 1
> ---remove_file $MYSQLTEST_VARDIR/master-data/t.bak
> +--remove_file $bdir/t.bak
>
> # Scenario 2
> --echo Backup of mysql, information_schema scenario 2
> --error ER_BACKUP_CANNOT_INCLUDE_DB
> BACKUP DATABASE information_schema TO 't.bak';
> +--replace_column 2 #
> +SHOW WARNINGS;
>
> --error 0, 1
> ---remove_file $MYSQLTEST_VARDIR/master-data/t.bak
> +--remove_file $bdir/t.bak
>
> # Scenario 3
> --echo Backup of mysql, information_schema scenario 3
> --error ER_BACKUP_CANNOT_INCLUDE_DB
> BACKUP DATABASE mysql, information_schema TO 't.bak';
> +--replace_column 2 #
> +SHOW WARNINGS;
>
> --error 0, 1
> ---remove_file $MYSQLTEST_VARDIR/master-data/t.bak
> +--remove_file $bdir/t.bak
>
> # Scenario 4
> --echo Backup of mysql, information_schema scenario 4
> --error ER_BACKUP_CANNOT_INCLUDE_DB
> BACKUP DATABASE mysql, test TO 't.bak';
> +--replace_column 2 #
> +SHOW WARNINGS;
>
> --error 0, 1
> ---remove_file $MYSQLTEST_VARDIR/master-data/t.bak
> +--remove_file $bdir/t.bak
>
> # Scenario 5
> --echo Backup of mysql, information_schema scenario 5
> --error ER_BACKUP_CANNOT_INCLUDE_DB
> BACKUP DATABASE information_schema, test TO 't.bak';
> +--replace_column 2 #
> +SHOW WARNINGS;
>
> --error 0, 1
> ---remove_file $MYSQLTEST_VARDIR/master-data/t.bak
> +--remove_file $bdir/t.bak
>
> # Scenario 6
> --echo Backup of mysql, information_schema scenario 6
> --error ER_BACKUP_CANNOT_INCLUDE_DB
> BACKUP DATABASE mysql, information_schema, test TO 't.bak';
> +--replace_column 2 #
> +SHOW WARNINGS;
>
> --error 0, 1
> ---remove_file $MYSQLTEST_VARDIR/master-data/t.bak
> +--remove_file $bdir/t.bak
> #
> # BUG#33352 - Test for presence of online backup progress tables;
> #
> @@ -178,7 +204,7 @@ INSERT INTO test_ob_error.t1 VALUES (1),
> --echo Backup the database;
> --replace_column 1 #
> BACKUP DATABASE test_ob_error TO 'ob_err.bak';
> ---remove_file $MYSQLTEST_VARDIR/master-data/ob_err.bak
> +--remove_file $bdir/ob_err.bak
>
> # Drop one of the tables and try a backup.
> DROP TABLE mysql.backup_history;
> @@ -188,9 +214,9 @@ DROP TABLE mysql.backup_history;
> --error ER_BACKUP_PROGRESS_TABLES
> BACKUP DATABASE test_ob_error TO 'ob_err.bak';
> --error 0,1
> ---remove_file $MYSQLTEST_VARDIR/master-data/ob_err.bak
> +--remove_file $bdir/ob_err.bak
> --replace_column 2 #
> -SHOW ERRORS;
> +SHOW WARNINGS;
>
> # Restore the table
> --echo Restoring the table
> @@ -205,9 +231,9 @@ DROP TABLE mysql.backup_progress;
> --error ER_BACKUP_PROGRESS_TABLES
> BACKUP DATABASE test_ob_error TO 'ob_err.bak';
> --error 0,1
> ---remove_file $MYSQLTEST_VARDIR/master-data/ob_err.bak
> +--remove_file $bdir/ob_err.bak
> --replace_column 2 #
> -SHOW ERRORS;
> +SHOW WARNINGS;
>
> # Restore the table
> --echo Restoring the table
> @@ -248,8 +274,8 @@ SET DEBUG_SYNC='now WAIT_FOR running';
> --echo delete database files so that check_db_dir_exists will fail in
> --echo si_objects.cc @ DatabaseObj::do_serialize
>
> ---remove_file $MYSQLTEST_VARDIR/master-data/db1/db.opt
> ---rmdir $MYSQLTEST_VARDIR/master-data/db1
> +--remove_file $ddir/db1/db.opt
> +--rmdir $ddir/db1
>
> SET DEBUG_SYNC='now SIGNAL db_will_fail';
>
>
> === modified file 'mysql-test/suite/backup/t/backup_myisam1.test'
> --- a/mysql-test/suite/backup/t/backup_myisam1.test 2008-10-07
17:15:44 +0000
> +++ b/mysql-test/suite/backup/t/backup_myisam1.test 2008-11-12
11:20:04 +0000
> @@ -3,6 +3,8 @@
>
> --source include/not_embedded.inc
>
> +let $bdir=`SELECT @@backupdir`;
> +
> #
> # Cleanup from former test cases
> #
> @@ -10,7 +12,7 @@
> drop database if exists mysqltest;
> --enable_warnings
> --error 0,1
> ---remove_file $MYSQLTEST_VARDIR/master-data/test.ba
> +--remove_file $bdir/test.ba
>
> create database mysqltest;
> use mysqltest;
> @@ -24,5 +26,7 @@ BACKUP DATABASE mysqltest TO 'test.ba';
> # Cleanup from this test case
> #
> DROP DATABASE mysqltest;
> ---remove_file $MYSQLTEST_VARDIR/master-data/test.ba
> +# Note: The backup file should not exist as BACKUP commad failed.
[4] Typo: "commad"
> +--error 1
> +--remove_file $bdir/test.ba
>
>
> === modified file 'mysql-test/suite/backup/t/backup_views.test'
> --- a/mysql-test/suite/backup/t/backup_views.test 2008-10-07 17:15:44
+0000
> +++ b/mysql-test/suite/backup/t/backup_views.test 2008-11-12 11:20:04
+0000
> @@ -205,14 +205,14 @@ DROP DATABASE bup_db2;
>
> --echo restore database with view dependency to other, non-existing db
>
> ---error ER_BACKUP_CANT_RESTORE_VIEW
> +--error ER_NO_SUCH_TABLE
> RESTORE FROM 'bup_objectview1.bak';
>
> # An incomplete bup_db1 was created by the failing restore operation.
> # Remove it before trying restore of bup_db2.
> DROP DATABASE bup_db1;
>
> ---error ER_BACKUP_CANT_RESTORE_VIEW
> +--error ER_NO_SUCH_TABLE
> RESTORE FROM 'bup_objectview2.bak';
>
> # An incomplete bup_db2 was created by the failing restore operation.
>
> === modified file 'sql/backup/backup_info.cc'
> --- a/sql/backup/backup_info.cc 2008-11-05 09:41:15 +0000
> +++ b/sql/backup/backup_info.cc 2008-11-12 11:20:04 +0000
> @@ -87,11 +87,11 @@ Backup_info::find_backup_engine(const ba
>
> // See if table has native backup engine
>
> - storage_engine_ref se= get_storage_engine(m_ctx.m_thd, tbl);
> + storage_engine_ref se= get_storage_engine(m_thd, tbl);
>
> if (!se)
> {
> - m_ctx.fatal_error(ER_NO_STORAGE_ENGINE, tbl.describe(buf));
> + m_log.report_error(ER_NO_STORAGE_ENGINE, tbl.describe(buf));
> DBUG_RETURN(NULL);
> }
>
> @@ -126,7 +126,7 @@ Backup_info::find_backup_engine(const ba
> if (!snap)
> if (has_native_backup(se))
> {
> - Native_snapshot *nsnap= new Native_snapshot(m_ctx, se);
> + Native_snapshot *nsnap= new Native_snapshot(m_log, se);
>
> /*
> Check if the snapshot object is valid - in particular has
successfully
> @@ -175,7 +175,7 @@ Backup_info::find_backup_engine(const ba
> }
>
> if (!snap)
> - m_ctx.fatal_error(ER_BACKUP_NO_BACKUP_DRIVER,tbl.describe(buf));
> + m_log.report_error(ER_BACKUP_NO_BACKUP_DRIVER,tbl.describe(buf));
>
> DBUG_RETURN(snap);
> }
> @@ -301,7 +301,7 @@ Backup_info::Dep_node::get_key(const uch
> @c find_backup_engine().
> */
> Backup_info::Backup_info(Backup_restore_ctx &ctx)
> - :m_ctx(ctx), m_state(Backup_info::ERROR), native_snapshots(8),
> + :m_log(ctx), m_thd(ctx.thd()), m_state(Backup_info::ERROR),
native_snapshots(8),
[5] I think this illustrates that making Backup_restore_ctx a subclass
of Logger was a bad idea. Anyhow, if Backup_info is not supposed to
deal with Backup_restore_ctx, I feel it is better to pass in Logger
and THD as separate arguments to the constructor instead passing in a
Backup_restore_ctx for Backup_info to pick sub-components.
[6] The above line is too long.
> m_dep_list(NULL), m_dep_end(NULL),
> m_srout_end(NULL), m_view_end(NULL), m_trigger_end(NULL),
m_event_end(NULL)
> {
> @@ -318,7 +318,7 @@ Backup_info::Backup_info(Backup_restore_
> Dep_node::get_key, Dep_node::free, MYF(0)))
> {
> // Allocation failed. Error has been reported, but not logged to
backup logs
> - m_ctx.log_error(ER_OUT_OF_RESOURCES);
> + m_log.log_error(ER_OUT_OF_RESOURCES);
> return;
> }
>
> @@ -328,10 +328,10 @@ Backup_info::Backup_info(Backup_restore_
> element on that list, as a "catch all" entry.
> */
>
> - snap= new Nodata_snapshot(m_ctx); // logs errors
> + snap= new Nodata_snapshot(m_log); // logs errors
> if (!snap)
> {
> - m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + m_log.report_error(ER_OUT_OF_RESOURCES);
> return;
> }
>
> @@ -341,14 +341,14 @@ Backup_info::Backup_info(Backup_restore_
> if (snapshots.push_back(snap))
> {
> // Allocation failed. Error has been reported, but not logged to
backup logs
> - m_ctx.log_error(ER_OUT_OF_RESOURCES);
> + m_log.log_error(ER_OUT_OF_RESOURCES);
> return;
> }
>
> - snap= new CS_snapshot(m_ctx); // logs errors
> + snap= new CS_snapshot(m_log); // logs errors
> if (!snap)
> {
> - m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + m_log.report_error(ER_OUT_OF_RESOURCES);
> return;
> }
>
> @@ -358,14 +358,14 @@ Backup_info::Backup_info(Backup_restore_
> if (snapshots.push_back(snap))
> {
> // Allocation failed. Error has been reported, but not logged to
backup logs
> - m_ctx.log_error(ER_OUT_OF_RESOURCES);
> + m_log.log_error(ER_OUT_OF_RESOURCES);
> return; // Error has been logged
> }
>
> - snap= new Default_snapshot(m_ctx); // logs errors
> + snap= new Default_snapshot(m_log); // logs errors
> if (!snap)
> {
> - m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + m_log.report_error(ER_OUT_OF_RESOURCES);
> return;
> }
>
> @@ -375,7 +375,7 @@ Backup_info::Backup_info(Backup_restore_
> if (snapshots.push_back(snap))
> {
> // Allocation failed. Error has been reported, but not logged to
backup logs
> - m_ctx.log_error(ER_OUT_OF_RESOURCES);
> + m_log.log_error(ER_OUT_OF_RESOURCES);
> return; // Error has been logged
> }
>
> @@ -417,7 +417,7 @@ int Backup_info::close()
> // report backup drivers used in the image
>
> for (ushort n=0; n < snap_count(); ++n)
> - m_ctx.report_driver(m_snap[n]->name());
> + m_log.report_driver(m_snap[n]->name());
>
> m_state= CLOSED;
> return 0;
> @@ -465,7 +465,7 @@ backup::Image_info::Ts* Backup_info::add
>
> if (!ts)
> {
> - m_ctx.fatal_error(ER_BACKUP_CATALOG_ADD_TS, name);
> + m_log.report_error(ER_BACKUP_CATALOG_ADD_TS, name);
> return NULL;
> }
>
> @@ -479,7 +479,7 @@ backup::Image_info::Ts* Backup_info::add
>
> if (!n1)
> {
> - m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + m_log.report_error(ER_OUT_OF_RESOURCES);
> return NULL;
> }
>
> @@ -513,7 +513,7 @@ backup::Image_info::Db* Backup_info::add
>
> if (!db)
> {
> - m_ctx.fatal_error(ER_BACKUP_CATALOG_ADD_DB, name->ptr());
> + m_log.report_error(ER_BACKUP_CATALOG_ADD_DB, name->ptr());
> return NULL;
> }
>
> @@ -550,7 +550,7 @@ int Backup_info::add_dbs(List< ::LEX_STR
>
> if (is_internal_db_name(&db_name))
> {
> - m_ctx.fatal_error(ER_BACKUP_CANNOT_INCLUDE_DB, db_name.c_ptr());
> + m_log.report_error(ER_BACKUP_CANNOT_INCLUDE_DB, db_name.c_ptr());
> goto error;
> }
>
> @@ -589,7 +589,7 @@ int Backup_info::add_dbs(List< ::LEX_STR
>
> if (!unknown_dbs.is_empty())
> {
> - m_ctx.fatal_error(ER_BAD_DB_ERROR, unknown_dbs.c_ptr());
> + m_log.report_error(ER_BAD_DB_ERROR, unknown_dbs.c_ptr());
> goto error;
> }
>
> @@ -614,11 +614,11 @@ int Backup_info::add_all_dbs()
> using namespace obs;
>
> int res= 0;
> - Obj_iterator *dbit= get_databases(m_ctx.m_thd);
> + Obj_iterator *dbit= get_databases(m_thd);
>
> if (!dbit)
> {
> - m_ctx.fatal_error(ER_BACKUP_LIST_DBS);
> + m_log.report_error(ER_BACKUP_LIST_DBS);
> return ERROR;
> }
>
> @@ -701,11 +701,11 @@ int Backup_info::add_db_items(Db &db)
>
> // Add tables.
>
> - Obj_iterator *it= get_db_tables(m_ctx.m_thd, &db.name());
> + Obj_iterator *it= get_db_tables(m_thd, &db.name());
>
> if (!it)
> {
> - m_ctx.fatal_error(ER_BACKUP_LIST_DB_TABLES, db.name().ptr());
> + m_log.report_error(ER_BACKUP_LIST_DB_TABLES, db.name().ptr());
> return ERROR;
> }
>
> @@ -731,7 +731,7 @@ int Backup_info::add_db_items(Db &db)
>
> // If this table uses a tablespace, add this tablespace to the
catalogue.
>
> - obj= get_tablespace_for_table(m_ctx.m_thd, &db.name(),
&tbl->name());
> + obj= get_tablespace_for_table(m_thd, &db.name(), &tbl->name());
>
> if (obj)
> {
> @@ -748,11 +748,11 @@ int Backup_info::add_db_items(Db &db)
> // Add other objects.
>
> delete it;
> - it= get_db_stored_procedures(m_ctx.m_thd, &db.name());
> + it= get_db_stored_procedures(m_thd, &db.name());
>
> if (!it)
> {
> - m_ctx.fatal_error(ER_BACKUP_LIST_DB_SROUT, db.name().ptr());
> + m_log.report_error(ER_BACKUP_LIST_DB_SROUT, db.name().ptr());
> goto error;
> }
>
> @@ -760,11 +760,11 @@ int Backup_info::add_db_items(Db &db)
> goto error;
>
> delete it;
> - it= get_db_stored_functions(m_ctx.m_thd, &db.name());
> + it= get_db_stored_functions(m_thd, &db.name());
>
> if (!it)
> {
> - m_ctx.fatal_error(ER_BACKUP_LIST_DB_SROUT, db.name().ptr());
> + m_log.report_error(ER_BACKUP_LIST_DB_SROUT, db.name().ptr());
> goto error;
> }
>
> @@ -772,11 +772,11 @@ int Backup_info::add_db_items(Db &db)
> goto error;
>
> delete it;
> - it= get_db_views(m_ctx.m_thd, &db.name());
> + it= get_db_views(m_thd, &db.name());
>
> if (!it)
> {
> - m_ctx.fatal_error(ER_BACKUP_LIST_DB_VIEWS, db.name().ptr());
> + m_log.report_error(ER_BACKUP_LIST_DB_VIEWS, db.name().ptr());
> goto error;
> }
>
> @@ -784,11 +784,11 @@ int Backup_info::add_db_items(Db &db)
> goto error;
>
> delete it;
> - it= get_db_events(m_ctx.m_thd, &db.name());
> + it= get_db_events(m_thd, &db.name());
>
> if (!it)
> {
> - m_ctx.fatal_error(ER_BACKUP_LIST_DB_EVENTS, db.name().ptr());
> + m_log.report_error(ER_BACKUP_LIST_DB_EVENTS, db.name().ptr());
> goto error;
> }
>
> @@ -796,11 +796,11 @@ int Backup_info::add_db_items(Db &db)
> goto error;
>
> delete it;
> - it= get_db_triggers(m_ctx.m_thd, &db.name());
> + it= get_db_triggers(m_thd, &db.name());
>
> if (!it)
> {
> - m_ctx.fatal_error(ER_BACKUP_LIST_DB_TRIGGERS, db.name().ptr());
> + m_log.report_error(ER_BACKUP_LIST_DB_TRIGGERS, db.name().ptr());
> goto error;
> }
>
> @@ -808,11 +808,11 @@ int Backup_info::add_db_items(Db &db)
> goto error;
>
> delete it;
> - it= get_all_db_grants(m_ctx.m_thd, &db.name());
> + it= get_all_db_grants(m_thd, &db.name());
>
> if (!it)
> {
> - m_ctx.fatal_error(ER_BACKUP_LIST_DB_PRIV, db.name().ptr());
> + m_log.report_error(ER_BACKUP_LIST_DB_PRIV, db.name().ptr());
> goto error;
> }
>
> @@ -889,8 +889,8 @@ backup::Image_info::Table* Backup_info::
>
> if (!tbl)
> {
> - m_ctx.fatal_error(ER_BACKUP_CATALOG_ADD_TABLE,
> - dbi.name().ptr(), t.name().ptr());
> + m_log.report_error(ER_BACKUP_CATALOG_ADD_TABLE,
> + dbi.name().ptr(), t.name().ptr());
> return NULL;
> }
>
> @@ -943,7 +943,7 @@ int Backup_info::add_view_deps(obs::Obj
>
> // Get an iterator to iterate over base views of the given one.
>
> - obs::Obj_iterator *it= obs::get_view_base_views(m_ctx.m_thd,
db_name, name);
> + obs::Obj_iterator *it= obs::get_view_base_views(m_thd, db_name, name);
>
> if (!it)
> return ERROR;
> @@ -1086,7 +1086,7 @@ Backup_info::add_db_object(Db &db, const
>
> if (res == get_dep_node_res::ERROR)
> {
> - m_ctx.fatal_error(error, db.name().ptr(), name->ptr());
> + m_log.report_error(error, db.name().ptr(), name->ptr());
> return NULL;
> }
>
> @@ -1101,7 +1101,7 @@ Backup_info::add_db_object(Db &db, const
> if (type == BSTREAM_IT_VIEW)
> if (add_view_deps(*obj))
> {
> - m_ctx.fatal_error(error, db.name().ptr(), name->ptr());
> + m_log.report_error(error, db.name().ptr(), name->ptr());
> return NULL;
> }
>
> @@ -1122,7 +1122,7 @@ Backup_info::add_db_object(Db &db, const
>
> if (!o)
> {
> - m_ctx.fatal_error(error, db.name().ptr(), name->ptr());
> + m_log.report_error(error, db.name().ptr(), name->ptr());
> return NULL;
> }
>
> @@ -1342,7 +1342,7 @@ int Backup_info::Global_iterator::init()
> if (!m_it)
> {
> const Backup_info* info= static_cast<const Backup_info*>(&m_info);
> - return info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + return info->m_log.report_error(ER_OUT_OF_RESOURCES);
> }
> next(); // Never errors
>
> @@ -1388,7 +1388,7 @@ Backup_info::Global_iterator::next()
> if (!m_it)
> {
> const Backup_info* info= static_cast<const Backup_info*>(&m_info);
> - info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + info->m_log.report_error(ER_OUT_OF_RESOURCES);
> mode= DONE;
> return FALSE;
> }
> @@ -1471,7 +1471,7 @@ backup::Image_info::Iterator* Backup_inf
> Global_iterator* it = new Global_iterator(*this);
> if (it == NULL)
> {
> - m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + m_log.report_error(ER_OUT_OF_RESOURCES);
> return NULL;
> }
> if (it->init()) // Error has been logged
> @@ -1488,7 +1488,7 @@ backup::Image_info::Iterator* Backup_inf
> Perdb_iterator* it = new Perdb_iterator(*this);
> if (it == NULL)
> {
> - m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + m_log.report_error(ER_OUT_OF_RESOURCES);
> return NULL;
> }
> if (it->init()) // Error has been logged
>
> === modified file 'sql/backup/backup_info.h'
> --- a/sql/backup/backup_info.h 2008-10-27 13:06:21 +0000
> +++ b/sql/backup/backup_info.h 2008-11-12 11:20:04 +0000
> @@ -31,7 +31,7 @@ class Backup_info: public backup::Image_
> {
> public:
>
> - Backup_restore_ctx &m_ctx;
> + backup::Logger &m_log;
>
> Backup_info(Backup_restore_ctx&);
> ~Backup_info();
> @@ -48,6 +48,8 @@ class Backup_info: public backup::Image_
>
> private:
>
> + THD *m_thd;
> +
> class Global_iterator; ///< Iterates over global items (for which
meta-data is stored).
> class Perdb_iterator; ///< Iterates over all per-database objects
(except tables).
>
>
> === modified file 'sql/backup/backup_kernel.h'
> --- a/sql/backup/backup_kernel.h 2008-10-30 17:53:24 +0000
> +++ b/sql/backup/backup_kernel.h 2008-11-12 11:20:04 +0000
> @@ -75,8 +75,6 @@ class Backup_restore_ctx: public backup:
>
> int do_backup();
> int do_restore();
> - int fatal_error(int, ...);
> - int log_error(int, ...);
>
> int close();
>
> @@ -84,6 +82,8 @@ class Backup_restore_ctx: public backup:
>
> private:
>
> + int fatal_error(int);
> +
> /** @c current_op points to the @c Backup_restore_ctx for the
> ongoing backup/restore operation. If pointer is null, no
> operation is currently running. */
> @@ -145,8 +145,6 @@ class Backup_restore_ctx: public backup:
>
> int report_stream_open_failure(int open_error, const LEX_STRING
*location);
>
> - friend class Backup_info;
> - friend class Restore_info;
> friend int backup_init();
> friend void backup_shutdown();
> friend bstream_byte* bstream_alloc(unsigned long int);
> @@ -175,7 +173,7 @@ void Backup_restore_ctx::disable_fkey_co
> }
>
> /**
> - Report error and move context object into error state.
> + Move context object into error state.
>
> After this method is called the context object is in error state and
> cannot be normally used. It still can be examined for saved error
messages.
> @@ -188,19 +186,14 @@ void Backup_restore_ctx::disable_fkey_co
> a fatal error was reported before.
> */
> inline
> -int Backup_restore_ctx::fatal_error(int error_code, ...)
> +int Backup_restore_ctx::fatal_error(int error_code)
> {
> + m_remove_loc= TRUE;
> +
> if (m_error)
> return m_error;
>
> - va_list args;
> -
> m_error= error_code;
> - m_remove_loc= TRUE;
> -
> - va_start(args,error_code);
> - v_report_error(backup::log_level::ERROR, error_code, args);
> - va_end(args);
>
> return error_code;
> }
>
> === modified file 'sql/backup/data_backup.cc'
> --- a/sql/backup/data_backup.cc 2008-10-30 20:02:15 +0000
> +++ b/sql/backup/data_backup.cc 2008-11-12 11:20:04 +0000
> @@ -248,7 +248,7 @@ class Scheduler
> private:
>
> LIST *m_pumps, *m_last;
> - Logger *m_log; ///< used to report errors if not NULL
> + Logger &m_log; ///< for reporting errors
> uint m_count; ///< current number of pumps
> size_t m_total; ///< accumulated position of all drivers
> size_t m_init_left; ///< how much of init data is left (estimate)
> @@ -256,7 +256,7 @@ class Scheduler
> Output_stream &m_str; ///< stream to which we write
> bool cancelled; ///< true if backup process was cancelled
>
> - Scheduler(Output_stream &s, Logger *log)
> + Scheduler(Output_stream &s, Logger &log)
> :init_count(0), prepare_count(0), finish_count(0),
> m_pumps(NULL), m_last(NULL), m_log(log),
> m_count(0), m_total(0), m_init_left(0), m_known_count(0),
> @@ -427,7 +427,8 @@ int write_table_data(THD* thd, Backup_in
> if (info.snap_count() == 0 || info.table_count() == 0) // nothing
to backup
> DBUG_RETURN(0);
>
> - Scheduler sch(s, &info.m_ctx); // scheduler instance
> + Logger &log= info.m_log;
> + Scheduler sch(s, log); // scheduler instance
> List<Scheduler::Pump> inactive; // list of images not yet being
created
>
> // keeps maximal init size for images in inactive list
> @@ -448,9 +449,15 @@ int write_table_data(THD* thd, Backup_in
>
> Scheduler::Pump *p= new Scheduler::Pump(*i, s);
>
> - if (!p || !p->is_valid())
> + if (!p)
> {
> - info.m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + log.report_error(ER_OUT_OF_RESOURCES);
> + goto error;
> + }
> + if (!p->is_valid())
> + {
> + log.report_error(ER_BACKUP_CREATE_BACKUP_DRIVER,p->m_name);
> + delete p;
> goto error;
> }
[7] Currently, the only way that p is not valid is if allocation
fails. Hence, OUT_OF_RESOURCES is more informative to a user than
BACKUP_CREATE_BACKUP_DRIVER. A user is given no clue why a backup
driver could not be created.
>
> @@ -458,7 +465,7 @@ int write_table_data(THD* thd, Backup_in
>
> if (init_size == Driver::UNKNOWN_SIZE)
> {
> - if (sch.add(p))
> + if (sch.add(p)) // logs errors
> goto error;
> }
> else
> @@ -471,7 +478,7 @@ int write_table_data(THD* thd, Backup_in
> /* Allocation failed.
> Error has been reported, but not logged to backup logs.
> */
> - info.m_ctx.log_error(ER_OUT_OF_RESOURCES);
> + log.log_error(ER_OUT_OF_RESOURCES);
> goto error;
> }
> }
> @@ -529,7 +536,7 @@ int write_table_data(THD* thd, Backup_in
>
> // poll drivers
>
> - if (sch.step())
> + if (sch.step()) // logs errors
> goto error;
> }
>
> @@ -569,9 +576,12 @@ int write_table_data(THD* thd, Backup_in
> int error= 0;
> error= block_commits(thd, NULL);
> if (error)
> + {
> + log.report_error(ER_BACKUP_VP_FAILED);
[8] I do not feel this error gives much information about what is
actually wrong. Is it possible to say something about when
block_commits() fail and what the user could do about it?
> goto error;
> + }
>
> - if (sch.prepare())
> + if (sch.prepare()) // logs errors
> goto error;
>
> while (sch.prepare_count > 0)
> @@ -584,7 +594,7 @@ int write_table_data(THD* thd, Backup_in
>
> LOG_INFO binlog_pos;
>
> - info.m_ctx.report_state(BUP_VALIDITY_POINT);
> + log.report_state(BUP_VALIDITY_POINT);
> /*
> This breakpoint is used to assist in testing state changes for
> the backup progress. It is not to be used to indicate actual
> @@ -597,7 +607,7 @@ int write_table_data(THD* thd, Backup_in
> */
>
> DEBUG_SYNC(thd, "before_backup_data_lock");
> - if (sch.lock())
> + if (sch.lock()) // logs errors
> goto error;
>
> /*
> @@ -606,7 +616,7 @@ int write_table_data(THD* thd, Backup_in
> if (mysql_bin_log.is_open())
> if (mysql_bin_log.get_current_log(&binlog_pos))
> {
> - info.m_ctx.fatal_error(ER_BACKUP_BINLOG);
> + log.report_error(ER_BACKUP_BINLOG);
> goto error;
> }
>
> @@ -629,7 +639,7 @@ int write_table_data(THD* thd, Backup_in
> vp_time= my_time(0);
>
> DEBUG_SYNC(thd, "before_backup_data_unlock");
> - if (sch.unlock())
> + if (sch.unlock()) // logs errors
> goto error;
>
> /*
> @@ -638,17 +648,20 @@ int write_table_data(THD* thd, Backup_in
> DEBUG_SYNC(thd, "before_backup_unblock_commit");
> error= unblock_commits(thd);
> if (error)
> + {
> + log.report_error(ER_BACKUP_VP_FAILED);
> goto error;
> + }
>
> // Report and save information about VP
>
> info.save_vp_time(vp_time);
> - info.m_ctx.report_vp_time(vp_time, TRUE); // TRUE = also write
to progress log
> + log.report_vp_time(vp_time, TRUE); // TRUE = also write to
progress log
>
> if (mysql_bin_log.is_open())
> {
> info.save_binlog_pos(binlog_pos);
> - info.m_ctx.report_binlog_pos(info.binlog_pos);
> + log.report_binlog_pos(info.binlog_pos);
> }
>
> /*
> @@ -656,9 +669,10 @@ int write_table_data(THD* thd, Backup_in
> write it to the log.
> */
> if (obs::is_slave() && master_pos.pos)
> - info.m_ctx.report_master_binlog_pos(master_pos);
> + log.report_master_binlog_pos(master_pos);
> +
> + log.report_state(BUP_RUNNING);
>
> - info.m_ctx.report_state(BUP_RUNNING);
> DEBUG_SYNC(thd, "after_backup_binlog");
>
> /**** VP creation (end)
********************************************/
> @@ -819,9 +833,6 @@ int Scheduler::step()
>
> case backup_state::ERROR:
> remove_pump(p); // Note: never errors.
> - if (res)
> - cancel_backup(); // we hit an error - bail out
> - // Note: cancel_backup() never errors.
> break;
>
> default: break;
> @@ -846,11 +857,14 @@ int Scheduler::add(Pump *p)
> if (!p) // no pump to add
> return 0;
>
> - p->set_logger(m_log);
> + p->set_logger(&m_log);
> p->start_pos= avg;
>
> - if (p->begin())
> - goto error;
> + if (p->begin()) // logs errors
> + {
> + delete p;
> + return ERROR;
> + }
>
> // in case of error, above call should return non-zero code (and
report error)
> DBUG_ASSERT(p->state != backup_state::ERROR);
> @@ -894,12 +908,6 @@ int Scheduler::add(Pump *p)
> (unsigned long)m_init_left));
>
> return 0;
> -
> - error:
> -
> - delete p;
> - cancel_backup();
> - return ERROR;
> }
>
> /// Move backup pump to the end of scheduler's list.
> @@ -971,9 +979,9 @@ int Scheduler::prepare()
>
> for (Pump_iterator it(*this); it; ++it)
> {
> - if (it->prepare())
> + if (it->prepare()) // logs errors
> {
> - cancel_backup(); // Note: never errors.
> + remove_pump(it); // Note: never errors.
> return ERROR;
> }
> if (it->state == backup_state::PREPARING)
> @@ -994,9 +1002,9 @@ int Scheduler::lock()
> DBUG_PRINT("backup_data",("calling lock() for all drivers"));
>
> for (Pump_iterator it(*this); it; ++it)
> - if (it->lock())
> + if (it->lock()) // logs errors
> {
> - cancel_backup(); // Note: never errors.
> + remove_pump(it); // Note: never errors.
> return ERROR;
> }
>
> @@ -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);
> + if (m_log)
> m_log->report_error(ER_BACKUP_INIT_BACKUP_DRIVER, m_name);
> return ERROR;
> }
> @@ -1092,7 +1098,7 @@ int Backup_pump::end()
> if (ERROR == m_drv->end())
> {
> state= backup_state::ERROR;
> - DBUG_ASSERT(m_log);
> + if (m_log)
> m_log->report_error(ER_BACKUP_STOP_BACKUP_DRIVER, m_name);
> return ERROR;
> }
> @@ -1121,9 +1127,9 @@ int Backup_pump::prepare()
> case ERROR:
> default:
> state= backup_state::ERROR;
> - DBUG_ASSERT(m_log);
> + if (m_log)
> m_log->report_error(ER_BACKUP_PREPARE_DRIVER, m_name);
> - return ERROR;
> + return ERROR;
> }
>
> DBUG_PRINT("backup_data",(" preparing %s, goes to %s state",
> @@ -1138,7 +1144,7 @@ int Backup_pump::lock()
> if (ERROR == m_drv->lock())
> {
> state= backup_state::ERROR;
> - DBUG_ASSERT(m_log);
> + if (m_log)
> m_log->report_error(ER_BACKUP_CREATE_VP, m_name);
> return ERROR;
> }
> @@ -1154,7 +1160,7 @@ int Backup_pump::unlock()
> if (ERROR == m_drv->unlock())
> {
> state= backup_state::ERROR;
> - DBUG_ASSERT(m_log);
> + if (m_log)
> m_log->report_error(ER_BACKUP_UNLOCK_DRIVER, m_name);
> return ERROR;
> }
> @@ -1167,7 +1173,7 @@ int Backup_pump::cancel()
> if (ERROR == m_drv->cancel())
> {
> state= backup_state::ERROR;
> - DBUG_ASSERT(m_log);
> + if (m_log)
> m_log->report_error(ER_BACKUP_CANCEL_BACKUP, m_name);
> return ERROR;
> }
> @@ -1248,7 +1254,7 @@ int Backup_pump::pump(size_t *howmuch)
>
> case Block_writer::ERROR:
> default:
> - DBUG_ASSERT(m_log);
> + if (m_log)
> m_log->report_error(ER_BACKUP_GET_BUF);
> state= backup_state::ERROR;
> return ERROR;
> @@ -1293,7 +1299,7 @@ int Backup_pump::pump(size_t *howmuch)
>
> case ERROR:
> default:
> - DBUG_ASSERT(m_log);
> + if (m_log)
> m_log->report_error(ER_BACKUP_GET_DATA, m_name);
> state= backup_state::ERROR;
> return ERROR;
> @@ -1328,7 +1334,7 @@ int Backup_pump::pump(size_t *howmuch)
>
> case Block_writer::ERROR:
>
> - DBUG_ASSERT(m_log);
> + if (m_log)
> m_log->report_error(ER_BACKUP_WRITE_DATA, m_name,
m_buf.table_num);
> state= backup_state::ERROR;
> return ERROR;
> @@ -1371,11 +1377,25 @@ int restore_table_data(THD *thd, Restore
> if (info.snap_count() == 0 || info.table_count() == 0) // nothing
to restore
> DBUG_RETURN(0);
>
> + Logger &log= info.m_log;
> +
> + /* Drv[n] points at restore driver used to process snapshot n. */
> Restore_driver* drv[256];
> + /*
> + Active[n] is not NULL if driver drv[n] has been activated. Such
driver needs
> + an end() or cancel() call to shut it down properly.
> + */
> + Restore_driver* active[256];
[9] A named constant should be used here instead of 256.
> + /*
> + Bad_drivers string for holding comma separated list of drivers which
> + signalled errors during shutdown. If non-empty, an error will be
logged
> + at the end of the function (finish: label).
> + */
> + String bad_drivers;
>
> if (info.snap_count() > 256)
> {
> - info.m_ctx.fatal_error(ER_BACKUP_TOO_MANY_IMAGES,
info.snap_count(), 256);
> + log.report_error(ER_BACKUP_TOO_MANY_IMAGES, info.snap_count(), 256);
> DBUG_RETURN(ERROR);
> }
>
> @@ -1384,7 +1404,7 @@ int restore_table_data(THD *thd, Restore
>
> for (uint n=0; n < info.snap_count(); ++n)
> {
> - drv[n]= NULL;
> + active[n]= drv[n]= NULL;
>
> Snapshot_info *snap= info.m_snap[n];
>
> @@ -1395,7 +1415,7 @@ int restore_table_data(THD *thd, Restore
> res= snap->get_restore_driver(drv[n]);
> if (res == backup::ERROR)
> {
> - info.m_ctx.fatal_error(ER_BACKUP_CREATE_RESTORE_DRIVER,
snap->name());
> + log.report_error(ER_BACKUP_CREATE_RESTORE_DRIVER, snap->name());
> goto error;
> };
> }
> @@ -1406,9 +1426,11 @@ int restore_table_data(THD *thd, Restore
> res= drv[n]->begin(0);
> if (res == backup::ERROR)
> {
> - info.m_ctx.fatal_error(ER_BACKUP_INIT_RESTORE_DRIVER,
info.m_snap[n]->name());
> + log.report_error(ER_BACKUP_INIT_RESTORE_DRIVER,
info.m_snap[n]->name());
> goto error;
> }
> +
> + active[n]= drv[n];
> }
>
> DEBUG_SYNC(thd, "restore_in_progress");
> @@ -1449,9 +1471,8 @@ int restore_table_data(THD *thd, Restore
> break;
>
> case BSTREAM_ERROR:
> - info.m_ctx.fatal_error(ER_BACKUP_READ_DATA);
> + log.report_error(ER_BACKUP_READ_DATA);
> default:
> - state= ERROR;
> goto error;
>
> }
> @@ -1491,7 +1512,8 @@ int restore_table_data(THD *thd, Restore
> */
> DBUG_ASSERT(snap && drvr);
>
> - switch( drvr->send_data(buf) ) {
> + ret= drvr->send_data(buf);
> + switch (ret) {
>
> case backup::OK:
> info.data_size += buf.size;
> @@ -1504,8 +1526,13 @@ int restore_table_data(THD *thd, Restore
> case backup::ERROR:
> if( errors > MAX_ERRORS )
> {
> - info.m_ctx.fatal_error(ER_BACKUP_SEND_DATA,
buf.table_num, snap->name());
> - state= ERROR;
> + log.report_error(ER_BACKUP_SEND_DATA, buf.table_num,
snap->name());
> + /*
> + If driver signals error then it is not active any
longer - neither
> + ->end() nor ->cancel() should be called on it, only
->free().
> + This is why we need to remove it from active[] array.
> + */
> + active[snap_num]= NULL;
> goto error;
> }
> errors++;
> @@ -1516,8 +1543,7 @@ int restore_table_data(THD *thd, Restore
> default:
> if( repeats > MAX_REPEATS )
> {
> - info.m_ctx.fatal_error(ER_BACKUP_SEND_DATA_RETRY,
repeats, snap->name());
> - state= ERROR;
> + log.report_error(ER_BACKUP_SEND_DATA_RETRY, repeats,
snap->name());
> goto error;
> }
> repeats++;
> @@ -1526,7 +1552,7 @@ int restore_table_data(THD *thd, Restore
>
> default:
> break;
> - } // switch(state)
> + } // switch(ret)
>
> } // main reading loop
>
> @@ -1536,49 +1562,69 @@ int restore_table_data(THD *thd, Restore
> }
>
> DEBUG_SYNC(::current_thd, "restore_table_data_before_end");
> -
> - { // Shutting down drivers
>
> - String bad_drivers;
> + // Call end() for all active drivers.
>
> - for (uint n=0; n < info.snap_count(); ++n)
> - {
> - if (!drv[n])
> - continue;
> + for (uint n=0; n < info.snap_count(); ++n)
> + {
> + if (!active[n])
> + continue;
>
> - DBUG_PRINT("restore",("Shutting down restore driver %s",
> - info.m_snap[n]->name()));
> - res= drv[n]->end();
> - if (res == backup::ERROR)
> - {
> - state= ERROR;
> + DBUG_PRINT("restore",("Shutting down restore driver %s",
> + info.m_snap[n]->name()));
> + res= active[n]->end();
> + if (res == backup::ERROR)
> + {
> + state= ERROR;
>
> - if (!bad_drivers.is_empty())
> - bad_drivers.append(",");
> - bad_drivers.append(info.m_snap[n]->name());
> - }
> - drv[n]->free(); // Never errors
> + if (!bad_drivers.is_empty())
> + bad_drivers.append(",");
> + bad_drivers.append(info.m_snap[n]->name());
> }
> -
> - if (!bad_drivers.is_empty())
> - info.m_ctx.report_error(ER_BACKUP_STOP_RESTORE_DRIVERS,
bad_drivers.c_ptr());
> }
>
> - DBUG_RETURN(state == ERROR ? backup::ERROR : 0);
> + goto finish;
>
> - error:
> +error:
> +
> + state= ERROR;
>
> DBUG_PRINT("restore",("Cancelling restore process"));
>
> + // Call cancel() for all active drivers
> +
> for (uint n=0; n < info.snap_count(); ++n)
> {
> - if (!drv[n])
> + if (!active[n])
> continue;
>
> + DBUG_PRINT("restore",("Cancelling restore driver %s",
> + info.m_snap[n]->name()));
> + res= active[n]->cancel();
> +
> + if (res)
> + {
> + if (!bad_drivers.is_empty())
> + bad_drivers.append(",");
> + bad_drivers.append(info.m_snap[n]->name());
> + }
> + }
> +
> +finish:
> +
> + if (!bad_drivers.is_empty())
> + log.report_error(ER_BACKUP_STOP_RESTORE_DRIVERS,
bad_drivers.c_ptr());
> +
> + // Call free() for all existing drivers
> +
> + for (uint n=0; n < info.snap_count(); ++n)
> + {
> + if (!drv[n])
> + continue;
> drv[n]->free(); // Never errors
> }
>
> - DBUG_RETURN(backup::ERROR);
> + DBUG_RETURN(state == ERROR ? backup::ERROR : 0);
> }
>
>
>
> === 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()));
> - }
> -
> +
> 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));
[10] Why is report_error called here, but not for other errors in this
method?
> }
>
> @@ -254,23 +251,21 @@ execute_backup_command(THD *thd, LEX *le
> }
>
> /**
> - Report errors.
> + Sends error notification after failed backup/restore operation.
>
> - Current implementation reports the last error saved in the logger
if it exist.
> - Otherwise it reports error given by @c error_code.
> + @param[in] ctx The context of the backup/restore operation.
> + @param[in] error_code Error to be reported if no errors reported
yet.
>
> - @returns 0 on success, error code otherwise.
> - */
> -int send_error(Backup_restore_ctx &log, int error_code, ...)
> + If an error has been already reported then nothing is done - the
first
> + logged error will be send to the client. Otherwise, if no errors were
> + reported yet, the given error is sent to the client (but not
reported).
> +
> + @returns The error code given as argument.
> +*/
> +static
> +int send_error(Backup_restore_ctx &context, int error_code, ...)
> {
> - util::SAVED_MYSQL_ERROR *error= log.last_saved_error();
> -
> - if (error && !util::report_mysql_error(log.thd(), error, error_code))
> - {
> - if (error->code)
> - error_code= error->code;
> - }
> - else // there are no error information in the logger - report
error_code
> + if (!context.error_reported())
> {
> char buf[ERRMSGSIZE + 20];
> va_list args;
> @@ -282,8 +277,8 @@ int send_error(Backup_restore_ctx &log,
> va_end(args);
> }
>
> - if (log.backup::Logger::m_state == backup::Logger::RUNNING)
> - log.report_stop(my_time(0), FALSE); // FASLE = no success
> + if (context.backup::Logger::m_state == backup::Logger::RUNNING)
> + context.report_stop(my_time(0), FALSE); // FASLE = no success
> return error_code;
> }
>
> @@ -294,6 +289,8 @@ int send_error(Backup_restore_ctx &log,
> Currently the id of the operation is returned to the client. It can
> be used to select correct entries from the backup progress tables.
>
> + @note If an error has been reported, send_error() is invoked instead.
> +
> @returns 0 on success, error code otherwise.
> */
> int send_reply(Backup_restore_ctx &context)
> @@ -304,6 +301,9 @@ int send_reply(Backup_restore_ctx &conte
>
> DBUG_ENTER("send_reply");
>
> + if (context.error_reported())
> + return send_error(context, ER_UNKNOWN_ERROR);
> +
> /*
> Send field list.
> */
> @@ -335,9 +335,9 @@ int send_reply(Backup_restore_ctx &conte
> DBUG_RETURN(0);
>
> err:
> - DBUG_RETURN(context.fatal_error(ER_BACKUP_SEND_REPLY,
> - context.m_type ==
backup::Logger::BACKUP
> - ? "BACKUP" : "RESTORE"));
> + DBUG_RETURN(context.report_error(ER_BACKUP_SEND_REPLY,
> + context.m_type ==
backup::Logger::BACKUP
> + ? "BACKUP" : "RESTORE"));
> }
>
>
> @@ -496,22 +496,17 @@ int Backup_restore_ctx::prepare(String *
> if (m_error)
> return m_error;
>
> - // Prepare error reporting context.
> -
> - mysql_reset_errors(m_thd, 0); // Never errors
> - m_thd->no_warnings_for_error= FALSE;
> -
> - save_errors(); // Never errors
> -
> + int ret= 0;
>
> /*
> Check access for SUPER rights. If user does not have SUPER, fail
with error.
> +
> + 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)
[11] Why this change? What is better with assigning the return value
to a local variable. I fell that complicates code since I need to be
concerned with whether ret is only used in the line below or more
times below. used
> + return fatal_error(log_error(ER_SPECIFIC_ACCESS_DENIED_ERROR,
"SUPER"));
>
> /*
> Check if another BACKUP/RESTORE is running and if not, register
> @@ -528,7 +523,10 @@ int Backup_restore_ctx::prepare(String *
> pthread_mutex_unlock(&run_lock);
>
> if (m_error)
> + {
> + report_error(ER_BACKUP_RUNNING);
> return m_error;
> + }
>
> // check if location is valid (we assume it is a file path)
>
> @@ -550,10 +548,7 @@ int Backup_restore_ctx::prepare(String *
> #endif
>
> if (bad_filename)
> - {
> - fatal_error(ER_BAD_PATH, location.str);
> - return m_error;
> - }
> + return fatal_error(report_error(ER_BAD_PATH, location.str));
>
> /*
> Computer full path to backup file.
> @@ -568,18 +563,13 @@ int Backup_restore_ctx::prepare(String *
> mem_alloc= new Mem_allocator();
>
> if (!mem_alloc)
> - {
> - fatal_error(ER_OUT_OF_RESOURCES);
> - return m_error;
> - }
> + return fatal_error(report_error(ER_OUT_OF_RESOURCES));
>
> // Freeze all meta-data.
>
> - if (obs::ddl_blocker_enable(m_thd))
> - {
> - fatal_error(ER_DDL_BLOCK);
> - return m_error;
> - }
> + ret= obs::ddl_blocker_enable(m_thd);
> + if (ret)
> + return fatal_error(report_error(ER_DDL_BLOCK));
>
> return 0;
> }
> @@ -639,10 +629,13 @@ Backup_restore_ctx::prepare_for_backup(S
>
> if (!s)
> {
> - fatal_error(ER_OUT_OF_RESOURCES);
> + fatal_error(report_error(ER_OUT_OF_RESOURCES));
> return NULL;
> }
> -
> +
> + // Mark that the file should be removed unless operation completes
successfuly
> + m_remove_loc= TRUE;
> +
> int my_open_status= s->open();
> if (my_open_status != 0)
> {
> @@ -658,12 +651,16 @@ Backup_restore_ctx::prepare_for_backup(S
>
> if (!info)
> {
> - fatal_error(ER_OUT_OF_RESOURCES);
> + fatal_error(report_error(ER_OUT_OF_RESOURCES));
> return NULL;
> }
>
> if (!info->is_valid())
> - return NULL; // Error has been logged by Backup_Info constructor
> + {
> + // Error has been logged by Backup_Info constructor
> + fatal_error(ER_BACKUP_BACKUP_PREPARE);
[12] So here fatal_error will register a different error from what is
logged before. Which is reported to the user?
> + return NULL;
> + }
>
> /*
> If binlog is enabled, set BSTREAM_FLAG_BINLOG in the header to
indicate
> @@ -734,7 +731,7 @@ Backup_restore_ctx::prepare_for_restore(
>
> if (!s)
> {
> - fatal_error(ER_OUT_OF_RESOURCES);
> + fatal_error(report_error(ER_OUT_OF_RESOURCES));
> return NULL;
> }
>
> @@ -753,41 +750,40 @@ Backup_restore_ctx::prepare_for_restore(
>
> if (!info)
> {
> - fatal_error(ER_OUT_OF_RESOURCES);
> + fatal_error(report_error(ER_OUT_OF_RESOURCES));
> return NULL;
> }
>
> if (!info->is_valid())
> + {
> + fatal_error(ER_BACKUP_RESTORE_PREPARE); // Errors logged by
Restore_info ctor.
[13] Long line.
> return NULL;
> + }
>
> info->save_start_time(when);
> m_catalog= info;
>
> + int ret;
> +
> /*
> - Read catalogue from the input stream.
> + Read header and catalogue from the input stream.
> */
>
> - if (read_header(*info, *s))
> - {
> - fatal_error(ER_BACKUP_READ_HEADER);
> - return NULL;
> - }
> -
> - if (s->next_chunk() != BSTREAM_OK)
> + ret= read_header(*info, *s); // Can log errors via callback
functions.
> + if (ret)
> {
> - fatal_error(ER_BACKUP_NEXT_CHUNK);
> + if (!error_reported())
> + report_error(ER_BACKUP_READ_HEADER);
> + fatal_error(ret);
[14] Why is potentially different error messages used here for
reporr_error and fatal_error?
> return NULL;
> }
>
> - if (read_catalog(*info, *s))
> + ret= read_catalog(*info, *s); // Can log errors via callback
functions.
> + if (ret)
> {
> - fatal_error(ER_BACKUP_READ_HEADER);
> - return NULL;
> - }
> -
> - if (s->next_chunk() != BSTREAM_OK)
> - {
> - fatal_error(ER_BACKUP_NEXT_CHUNK);
> + if (!error_reported())
> + report_error(ER_BACKUP_READ_HEADER);
> + fatal_error(ret);
[15] Same as [14].
> return NULL;
> }
>
> @@ -834,6 +830,7 @@ Backup_restore_ctx::prepare_for_restore(
> int Backup_restore_ctx::lock_tables_for_restore()
> {
> TABLE_LIST *tables= NULL;
> + int ret;
>
> /*
> Iterate over all tables in all snapshots and create a linked
TABLE_LIST
> @@ -854,7 +851,7 @@ int Backup_restore_ctx::lock_tables_for_
> if (!ptr)
> {
> // Error has been reported, but not logged to backup logs
> - return log_error(ER_OUT_OF_RESOURCES);
> + return fatal_error(log_error(ER_OUT_OF_RESOURCES));
> }
>
> tables= backup::link_table_list(*ptr, tables); // Never errors
> @@ -871,16 +868,13 @@ int Backup_restore_ctx::lock_tables_for_
> Note 2: Skiping tmp tables is also important because otherwise a
tmp table
> can occlude a regular table with the same name (BUG#33574).
> */
> - if (open_and_lock_tables_derived(m_thd, tables,
> - FALSE, /* do not process derived
tables */
> - MYSQL_OPEN_SKIP_TEMPORARY
> + ret= open_and_lock_tables_derived(m_thd, tables,
> + FALSE, /* do not process derived
tables */
> + MYSQL_OPEN_SKIP_TEMPORARY
> /* do not open tmp tables */
> - )
> - )
> - {
> - fatal_error(ER_BACKUP_OPEN_TABLES,"RESTORE");
> - return m_error;
> - }
> + );
> + if (ret)
> + return fatal_error(report_error(ER_BACKUP_OPEN_TABLES,"RESTORE"));
[16] Same as [11].
>
> m_tables_locked= TRUE;
> return 0;
> @@ -905,39 +899,6 @@ void Backup_restore_ctx::unlock_tables()
>
>
> /**
> - Report error and move context object into error state without
pushing the
> - error on the server's warning stack.
> -
> - Similar to @c fatal_error, but does not push the error on the
> - server's warning stack. To be used when an error is reported from a
> - server function that has already pushed the error on the warning
stack.
> -
> - @return error code given as input or stored in the context object if
> - a fatal error was reported before.
> - */
> -inline
> -int Backup_restore_ctx::log_error(int error_code, ...)
> -{
> - if (m_error)
> - return m_error;
> -
> - bool saved = push_errors(FALSE); // Do not use warning stack
> -
> - m_error= error_code;
> - m_remove_loc= TRUE;
> -
> - va_list args;
> - va_start(args,error_code);
> - v_report_error(backup::log_level::ERROR, error_code, args);
> - va_end(args);
> -
> - push_errors(saved); // Reset
> -
> - return error_code;
> -}
> -
> -
> -/**
> Destroy a backup/restore context.
>
> This should reverse all settings made when context was created and
prepared.
> @@ -951,7 +912,6 @@ int Backup_restore_ctx::log_error(int er
> */
> int Backup_restore_ctx::close()
> {
> - int error= 0;
> if (m_state == CLOSED)
> return 0;
>
> @@ -985,7 +945,7 @@ int Backup_restore_ctx::close()
> if (m_stream && !m_stream->close())
> {
> // Note error, but complete clean-up
> - error= ER_BACKUP_CLOSE;
> + fatal_error(report_error(ER_BACKUP_CLOSE));
> }
>
> if (m_catalog)
> @@ -1005,25 +965,19 @@ int Backup_restore_ctx::close()
> Ignore ENOENT error since it is ok if the file doesn't exist.
> */
> if (res && my_errno != ENOENT)
> - {
> - error= ER_CANT_DELETE_FILE;
> - }
> + fatal_error(report_error(ER_CANT_DELETE_FILE, m_path.c_ptr(),
my_errno));
> }
>
> /* We report completion of the operation only if no errors were
detected,
> and logger has been initialized.
> */
> - if (!error)
> + if (!m_error)
[17] I suggest adding an access method for m_error.
> {
> if (backup::Logger::m_state == backup::Logger::RUNNING)
> {
> report_stop(when, TRUE);
> }
> }
> - else
> - {
> - fatal_error(error); // Log error
> - }
>
> /*
> Destroy backup stream's memory allocator (this frees memory)
> @@ -1044,7 +998,7 @@ int Backup_restore_ctx::close()
> pthread_mutex_unlock(&run_lock);
>
> m_state= CLOSED;
> - return error;
> + return m_error;
> }
>
> /**
> @@ -1067,6 +1021,7 @@ int Backup_restore_ctx::do_backup()
>
> using namespace backup;
>
> + int ret;
> Output_stream &s= *static_cast<Output_stream*>(m_stream);
> Backup_info &info= *static_cast<Backup_info*>(m_catalog);
>
> @@ -1077,27 +1032,33 @@ int Backup_restore_ctx::do_backup()
> DBUG_PRINT("backup",("Writing preamble"));
> DEBUG_SYNC(m_thd, "backup_before_write_preamble");
>
> - if (write_preamble(info, s))
> + ret= write_preamble(info, s); // Can Log errors via callback
functions.
> + if (ret)
> {
> - fatal_error(ER_BACKUP_WRITE_HEADER);
> - DBUG_RETURN(m_error);
> + if (!error_reported())
> + report_error(ER_BACKUP_WRITE_HEADER);
> + DBUG_RETURN(fatal_error(ret));
> }
>
> DBUG_PRINT("backup",("Writing table data"));
>
> DEBUG_SYNC(m_thd, "before_backup_data");
>
> - if (write_table_data(m_thd, info, s)) // logs errors
> - DBUG_RETURN(send_error(*this, ER_BACKUP_BACKUP));
> + ret= write_table_data(m_thd, info, s); // logs errors
> + if (ret)
> + DBUG_RETURN(fatal_error(ret));
[18] Same as [11]. In addition, in this method ret is used several
times for different return values which I fear may create confusion.
>
> DBUG_PRINT("backup",("Writing summary"));
>
> - if (write_summary(info, s))
> - {
> - fatal_error(ER_BACKUP_WRITE_SUMMARY);
> - DBUG_RETURN(m_error);
> - }
> + ret= write_summary(info, s);
> + if (ret)
> + DBUG_RETURN(fatal_error(report_error(ER_BACKUP_WRITE_SUMMARY)));
>
> + /*
> + 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)));
>
> // create all trigers and collect events in the events list
>
> @@ -1145,9 +1104,8 @@ int Backup_restore_ctx::restore_triggers
> Image_info::Iterator *it=
>
m_catalog->get_db_objects(*static_cast<Image_info::Db*>(obj));
> if (!it)
> - {
> - DBUG_RETURN(fatal_error(ER_OUT_OF_RESOURCES));
> - }
> + DBUG_RETURN(fatal_error(report_error(ER_OUT_OF_RESOURCES)));
> +
> while ((obj= (*it)++))
> switch (obj->type()) {
>
> @@ -1156,7 +1114,7 @@ int Backup_restore_ctx::restore_triggers
> if (events.push_back(obj))
> {
> // Error has been reported, but not logged to backup logs
> - DBUG_RETURN(log_error(ER_OUT_OF_RESOURCES));
> + DBUG_RETURN(fatal_error(log_error(ER_OUT_OF_RESOURCES)));
> }
> break;
>
> @@ -1166,8 +1124,8 @@ int Backup_restore_ctx::restore_triggers
> {
> delete it;
> delete dbit;
> -
fatal_error(ER_BACKUP_CANT_RESTORE_TRIGGER,obj->describe(buf));
> - DBUG_RETURN(m_error);
> + int err=
report_error(ER_BACKUP_CANT_RESTORE_TRIGGER,obj->describe(buf));
[19] Long lines.
> + DBUG_RETURN(fatal_error(err));
> }
> break;
>
> @@ -1187,8 +1145,8 @@ int Backup_restore_ctx::restore_triggers
> while ((ev= it++))
> if (ev->m_obj_ptr->execute(m_thd))
> {
> - fatal_error(ER_BACKUP_CANT_RESTORE_EVENT,ev->describe(buf));
> - DBUG_RETURN(m_error);
> + int err=
report_error(ER_BACKUP_CANT_RESTORE_EVENT,ev->describe(buf));
> + DBUG_RETURN(fatal_error(err));
> };
>
> DBUG_RETURN(0);
> @@ -1225,17 +1183,12 @@ int Backup_restore_ctx::do_restore()
>
> disable_fkey_constraints(); // Never errors
>
> - if (read_meta_data(info, s))
> - {
> - m_thd->main_da.reset_diagnostics_area(); // Never errors
> -
> - fatal_error(ER_BACKUP_READ_META);
> - DBUG_RETURN(m_error);
> - }
> -
> - if (s.next_chunk() == BSTREAM_ERROR)
> + err= read_meta_data(info, s); // Can log errors via callback
functions.
> + if (err)
[20] Here you are calling it return variable err, other places ret.
Please, be consistent.
> {
> - DBUG_RETURN(fatal_error(ER_BACKUP_NEXT_CHUNK));
> + if (!error_reported())
> + report_error(ER_BACKUP_READ_META);
> + DBUG_RETURN(fatal_error(err));
> }
>
> DBUG_PRINT("restore",("Restoring table data"));
> @@ -1249,16 +1202,17 @@ int Backup_restore_ctx::do_restore()
> close_thread_tables(m_thd); // Never errors
> m_thd->main_da.reset_diagnostics_area(); // Never errors
>
> - if (lock_tables_for_restore()) // logs errors
> - DBUG_RETURN(m_error);
> + err= lock_tables_for_restore(); // logs errors
> + if (err)
> + DBUG_RETURN(fatal_error(err));
>
> // Here restore drivers are created to restore table data
> - err= restore_table_data(m_thd, info, s); // reports errors
> + err= restore_table_data(m_thd, info, s); // logs errors
>
> unlock_tables(); // Never errors
>
> if (err)
> - DBUG_RETURN(ER_BACKUP_RESTORE);
> + DBUG_RETURN(fatal_error(err));
>
> /*
> Re-create all triggers and events (it was not done in @c
bcat_create_item()).
> @@ -1267,16 +1221,9 @@ int Backup_restore_ctx::do_restore()
> creation of these objects will fail.
> */
>
> - if (restore_triggers_and_events()) // reports errors
> - DBUG_RETURN(ER_BACKUP_RESTORE);
> -
> - DBUG_PRINT("restore",("Done."));
> -
> - if (read_summary(info, s))
> - {
> - fatal_error(ER_BACKUP_READ_SUMMARY);
> - DBUG_RETURN(m_error);
> - }
> + err= restore_triggers_and_events(); // logs errors
> + if (err)
> + DBUG_RETURN(fatal_error(err));
>
> /*
> FIXME: this call is here because object services doesn't clean the
> @@ -1287,6 +1234,12 @@ int Backup_restore_ctx::do_restore()
> close_thread_tables(m_thd); // Never errors
> m_thd->main_da.reset_diagnostics_area(); // Never errors
>
> + DBUG_PRINT("restore",("Done."));
> +
> + err= read_summary(info, s);
> + if (err)
> + DBUG_RETURN(report_error(ER_BACKUP_READ_SUMMARY));
> +
[21] Why is not fatal_error called here?
> /*
> Report validity point time and binlog position stored in the
backup image
> (in the summary section).
> @@ -1304,8 +1257,7 @@ int Backup_restore_ctx::do_restore()
> }
>
> /**
> - Report stream open error by calling fatal_error, effectively moving
> - context object into error state.
> + Report stream open error and move context object into error state.
>
> @return error code given as input or the one stored in the context
> object if a fatal error has already been reported.
> @@ -1316,26 +1268,26 @@ int Backup_restore_ctx::report_stream_op
> int error= 0;
> switch (my_open_status) {
> case ER_OPTION_PREVENTS_STATEMENT:
> - error= fatal_error(ER_OPTION_PREVENTS_STATEMENT,
"--secure-file-priv");
> + error= report_error(ER_OPTION_PREVENTS_STATEMENT,
"--secure-file-priv");
> break;
> case ER_BACKUP_WRITE_LOC:
> /*
> For this error, use the actual value returned instead of the
> path complimented with backupdir.
> */
> - error= fatal_error(ER_BACKUP_WRITE_LOC, location->str);
> + error= report_error(ER_BACKUP_WRITE_LOC, location->str);
> break;
> case ER_BACKUP_READ_LOC:
> /*
> For this error, use the actual value returned instead of the
> path complimented with backupdir.
> */
> - error= fatal_error(ER_BACKUP_READ_LOC, location->str);
> + error= report_error(ER_BACKUP_READ_LOC, location->str);
> break;
> default:
> DBUG_ASSERT(FALSE);
> }
> - return error;
> + return fatal_error(error);
> }
>
> namespace backup {
> @@ -1480,6 +1432,7 @@ int bcat_reset(st_bstream_image_header *
>
> DBUG_ASSERT(catalogue);
> Restore_info *info= static_cast<Restore_info*>(catalogue);
> + Logger &log= info->m_log;
>
> /*
> Iterate over the list of snapshots read from the backup image
(and stored
> @@ -1504,50 +1457,50 @@ int bcat_reset(st_bstream_image_header *
>
> if (!se || !hton)
> {
> - info->m_ctx.fatal_error(ER_BACKUP_CANT_FIND_SE, name_lex.str);
> + log.report_error(ER_BACKUP_CANT_FIND_SE, name_lex.str);
> return BSTREAM_ERROR;
> }
>
> if (!hton->get_backup_engine)
> {
> - info->m_ctx.fatal_error(ER_BACKUP_NO_NATIVE_BE, name_lex.str);
> + log.report_error(ER_BACKUP_NO_NATIVE_BE, name_lex.str);
> return BSTREAM_ERROR;
> }
>
> - info->m_snap[n]= new Native_snapshot(info->m_ctx,
snap->version, se);
> + info->m_snap[n]= new Native_snapshot(log, snap->version, se);
> //
reports errors
> break;
> }
>
> case BI_NODATA:
> - info->m_snap[n]= new Nodata_snapshot(info->m_ctx, snap->version);
> + info->m_snap[n]= new Nodata_snapshot(log, snap->version);
> //
reports errors
> break;
>
> case BI_CS:
> - info->m_snap[n]= new CS_snapshot(info->m_ctx, snap->version);
> + info->m_snap[n]= new CS_snapshot(log, snap->version);
> //
reports errors
> break;
>
> case BI_DEFAULT:
> - info->m_snap[n]= new Default_snapshot(info->m_ctx, snap->version);
> + info->m_snap[n]= new Default_snapshot(log, snap->version);
> //
reports errors
> break;
>
> default:
> // note: we use convention that snapshots are counted starting
from 1.
> - info->m_ctx.fatal_error(ER_BACKUP_UNKNOWN_BE, n + 1);
> + log.report_error(ER_BACKUP_UNKNOWN_BE, n + 1);
> return BSTREAM_ERROR;
> }
>
> if (!info->m_snap[n])
> {
> - info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + log.report_error(ER_OUT_OF_RESOURCES);
> return BSTREAM_ERROR;
> }
>
> info->m_snap[n]->m_num= n + 1;
> - info->m_ctx.report_driver(info->m_snap[n]->name());
> + log.report_driver(info->m_snap[n]->name());
> }
>
> return BSTREAM_OK;
> @@ -1577,6 +1530,7 @@ int bcat_add_item(st_bstream_image_heade
> using namespace backup;
>
> Restore_info *info= static_cast<Restore_info*>(catalogue);
> + Logger &log= info->m_log;
>
> backup::String name_str(item->name.begin, item->name.end);
>
> @@ -1617,7 +1571,7 @@ int bcat_add_item(st_bstream_image_heade
> with error earlier.
> */
> DBUG_ASSERT(it->snap_num >= info->snap_count());
> - info->m_ctx.fatal_error(ER_BACKUP_WRONG_TABLE_BE, it->snap_num
+ 1);
> + log.report_error(ER_BACKUP_WRONG_TABLE_BE, it->snap_num + 1);
> return BSTREAM_ERROR;
> }
>
> @@ -1681,6 +1635,7 @@ void* bcat_iterator_get(st_bstream_image
> DBUG_ASSERT(catalogue);
>
> Backup_info *info= static_cast<Backup_info*>(catalogue);
> + backup::Logger &log= info->m_log;
>
> switch (type) {
>
> @@ -1699,7 +1654,7 @@ void* bcat_iterator_get(st_bstream_image
> Iterator *it= info->get_tablespaces();
> if (!it)
> {
> - info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + log.report_error(ER_OUT_OF_RESOURCES);
> return NULL;
> }
>
> @@ -1711,7 +1666,7 @@ void* bcat_iterator_get(st_bstream_image
> Iterator *it= info->get_dbs();
> if (!it)
> {
> - info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + log.report_error(ER_OUT_OF_RESOURCES);
> return NULL;
> }
>
> @@ -1724,7 +1679,7 @@ void* bcat_iterator_get(st_bstream_image
>
> if (!it)
> {
> - info->m_ctx.fatal_error(ER_BACKUP_CAT_ENUM);
> + log.report_error(ER_BACKUP_CAT_ENUM);
> return NULL;
> }
>
> @@ -1815,18 +1770,19 @@ void* bcat_db_iterator_get(st_bstream_im
> DBUG_ASSERT(dbi);
>
> Backup_info *info= static_cast<Backup_info*>(catalogue);
> + backup::Logger &log= info->m_log;
> Backup_info::Db *db = info->get_db(dbi->base.pos);
>
> if (!db)
> {
> - info->m_ctx.fatal_error(ER_BACKUP_UNKNOWN_OBJECT);
> + log.report_error(ER_BACKUP_UNKNOWN_OBJECT);
> return NULL;
> }
>
> backup::Image_info::Iterator *it= info->get_db_objects(*db);
> if (!it)
> {
> - info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
> + log.report_error(ER_OUT_OF_RESOURCES);
> return NULL;
> }
>
> @@ -1879,7 +1835,8 @@ int bcat_create_item(st_bstream_image_he
> DBUG_ASSERT(item);
>
> Restore_info *info= static_cast<Restore_info*>(catalogue);
> - THD *thd= info->m_ctx.thd();
> + Logger &log= info->m_log;
> + THD *thd= info->m_thd;
> int create_err= 0;
>
> switch (item->type) {
> @@ -1901,7 +1858,7 @@ int bcat_create_item(st_bstream_image_he
> */
>
> default:
> - info->m_ctx.fatal_error(ER_BACKUP_UNKNOWN_OBJECT_TYPE);
> + log.report_error(ER_BACKUP_UNKNOWN_OBJECT_TYPE);
> return BSTREAM_ERROR;
> }
>
> @@ -1909,7 +1866,7 @@ int bcat_create_item(st_bstream_image_he
>
> if (!obj)
> {
> - info->m_ctx.fatal_error(ER_BACKUP_UNKNOWN_OBJECT);
> + log.report_error(ER_BACKUP_UNKNOWN_OBJECT);
> return BSTREAM_ERROR;
> }
>
> @@ -1928,7 +1885,7 @@ int bcat_create_item(st_bstream_image_he
>
> if (!sobj)
> {
> - info->m_ctx.fatal_error(create_err, desc);
> + log.report_error(create_err, desc);
> return BSTREAM_ERROR;
> }
>
> @@ -1970,7 +1927,7 @@ int bcat_create_item(st_bstream_image_he
> if (ts)
> {
> DBUG_PRINT("restore",(" tablespace has changed on the server -
aborting"));
> - info->m_ctx.fatal_error(ER_BACKUP_TS_CHANGE, desc);
> + log.report_error(ER_BACKUP_TS_CHANGE, desc);
> return BSTREAM_ERROR;
> }
> }
> @@ -1992,9 +1949,8 @@ int bcat_create_item(st_bstream_image_he
> */
> if (!obs::check_user_existence(thd, sobj->get_name()))
> {
> - info->m_ctx.write_message(log_level::WARNING,
> - ER(ER_BACKUP_GRANT_SKIPPED),
> - create_stmt);
> + log.report_error(log_level::WARNING, ER_BACKUP_GRANT_SKIPPED,
> + create_stmt);
> return BSTREAM_OK;
> }
> /*
> @@ -2014,14 +1970,14 @@ int bcat_create_item(st_bstream_image_he
> db_name.append(start, size);
> if (!info->has_db(db_name))
> {
> - info->m_ctx.fatal_error(ER_BACKUP_GRANT_WRONG_DB, create_stmt);
> + log.report_error(ER_BACKUP_GRANT_WRONG_DB, create_stmt);
> return BSTREAM_ERROR;
> }
> }
>
> if (sobj->execute(thd))
> {
> - info->m_ctx.fatal_error(create_err, desc);
> + log.report_error(create_err, desc);
> return BSTREAM_ERROR;
> }
>
> @@ -2092,11 +2048,12 @@ int bcat_get_item_create_query(st_bstrea
> ::String *buf= &(info->serialization_buf);
> buf->length(0);
>
> - if (obj->m_obj_ptr->serialize(info->m_ctx.thd(), buf))
> + if (obj->m_obj_ptr->serialize(info->m_thd, buf))
> {
> Image_info::Obj::describe_buf dbuf;
>
> - info->m_ctx.fatal_error(meta_err, obj->describe(dbuf));
> + info->m_log.report_error(meta_err, obj->describe(dbuf));
> +
> return BSTREAM_ERROR;
> }
>
>
> === modified file 'sql/backup/logger.cc'
> --- a/sql/backup/logger.cc 2008-10-27 13:06:21 +0000
> +++ b/sql/backup/logger.cc 2008-11-12 11:20:04 +0000
> @@ -28,7 +28,7 @@ namespace backup {
> the moment. Destinations which are not ready/initialized yet
should be
> silently ignored.
>
> - @returns 0 on success.
> + @returns Reported error code.
> */
> int Logger::write_message(log_level::value level, int error_code,
> const char *msg)
> @@ -36,7 +36,11 @@ int Logger::write_message(log_level::val
> char buf[ERRMSGSIZE + 30];
> const char *out= msg;
>
> - if (m_state == READY || m_state == RUNNING)
> + /*
> + Note: m_type is meaningful only after a call to init() i.e.,
> + if m_state != CREATED.
> + */
> + if (m_state != CREATED)
> {
> my_snprintf(buf, sizeof(buf), "%s: %s",
> m_type == BACKUP ? "Backup" : "Restore" , msg);
> @@ -45,43 +49,61 @@ int Logger::write_message(log_level::val
>
> switch (level) {
> case log_level::ERROR:
> - if (m_save_errors)
> - {
> - error.code= error_code;
> - error.level= MYSQL_ERROR::WARN_LEVEL_ERROR;
> - error.msg= sql_strdup(msg);
> - }
> + {
> + // Report to server's error log
>
> sql_print_error(out);
> - if (m_push_errors)
> - push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> - error_code, msg);
> - DBUG_PRINT("backup_log",("[ERROR] %s", out));
> -
> +
> + // Report to the client
> +
> + bool saved_value= m_thd->no_warnings_for_error;
> + m_thd->no_warnings_for_error= m_push_errors ? FALSE : TRUE;
> + my_printf_error(error_code, msg, MYF(0));
> + m_thd->no_warnings_for_error= saved_value;
> +
> + m_error_reported= TRUE;
> +
> + // Report to backup logs
> +
> if (m_state == READY || m_state == RUNNING)
> {
> time_t ts = my_time(0);
>
> backup_log->error_num(error_code);
> - backup_log->write_progress(0, ts, ts, 0, 0, error_code, out);
> + backup_log->write_progress(0, ts, ts, 0, 0, error_code, msg);
> }
>
> - return 0;
> + // Report in the debug trace
> +
> + DBUG_PRINT("backup_log",("[ERROR] %s", out));
> +
> + return error_code;
> + }
>
> case log_level::WARNING:
> + // Report to server's error log
> sql_print_warning(out);
> +
> + // Report to the client (push on warning stack)
> if (m_push_errors)
> push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> error_code, msg);
> +
> + // Report to the debug trace
> DBUG_PRINT("backup_log",("[Warning] %s", out));
> - return 0;
> +
> + return error_code;
>
> case log_level::INFO:
> + // Report to server's error log
> sql_print_information(out);
> +
> + // Report to the debug trace
> DBUG_PRINT("backup_log",("[Info] %s", out));
> - return 0;
>
> - default: return ERROR;
> + return error_code;
> +
> + default: DBUG_ASSERT(0); return ERROR;
> }
> }
>
> @@ -94,7 +116,7 @@ int Logger::write_message(log_level::val
> If the message contains placeholders, additional arguments provide
> values to be put there.
>
> - @returns 0 on success.
> + @returns Reported error code.
> */
> int Logger::v_report_error(log_level::value level, int error_code,
va_list args)
> {
>
> === modified file 'sql/backup/logger.h'
> --- a/sql/backup/logger.h 2008-10-30 17:53:24 +0000
> +++ b/sql/backup/logger.h 2008-11-12 11:20:04 +0000
> @@ -51,6 +51,7 @@ class Logger
> int report_error(log_level::value level, int error_code, ...);
> int report_error(const char *format, ...);
> int write_message(log_level::value level, const char *msg, ...);
> + int log_error(int error_code, ...);
>
> void report_start(time_t);
> void report_stop(time_t, bool);
> @@ -69,11 +70,8 @@ class Logger
> }
> void report_cleanup() { delete backup_log; }
>
> - void save_errors();
> - void stop_save_errors();
> - void clear_saved_errors();
> - util::SAVED_MYSQL_ERROR *last_saved_error();
> bool push_errors(bool);
> + bool error_reported() const;
>
> protected:
>
> @@ -85,24 +83,25 @@ class Logger
> int write_message(log_level::value level , int error_code, const
char *msg);
>
> private:
> - util::SAVED_MYSQL_ERROR error; ///< Used to store saved errors.
> - bool m_save_errors; ///< Flag telling if errors should be
saved.
> +
> bool m_push_errors; ///< Should errors be pushed on warning
stack?
> + bool m_error_reported; ///< Has any error been reported?
>
> Backup_log *backup_log; ///< Backup log interface class.
> +
> + // Disable copying of Logger instances
> + Logger(const Logger&) { DBUG_ASSERT(0); };
[22] There is not need for implementation here. We want a compile
error. (Btw. I have already checked in a patch with private copy
constructor.)
> };
>
> inline
> Logger::Logger(THD *thd)
> - :m_type(BACKUP), m_state(CREATED),
> - m_thd(thd), m_save_errors(FALSE), m_push_errors(TRUE), backup_log(0)
> + :m_type(BACKUP), m_state(CREATED), m_thd(thd), m_push_errors(TRUE),
> + m_error_reported(FALSE), backup_log(0)
> {}
>
> inline
> Logger::~Logger()
> -{
> - clear_saved_errors();
> -}
> +{}
>
> inline
> int Logger::write_message(log_level::value level, const char *msg, ...)
> @@ -155,37 +154,20 @@ int Logger::report_error(const char *for
> return res;
> }
>
> -/// Request that all reported errors are saved in the logger.
> +/// Reports error without pushing it on server's error stack.
> inline
> -void Logger::save_errors()
> +int Logger::log_error(int error_code, ...)
> {
> - if (m_save_errors)
> - return;
> - clear_saved_errors();
> - m_save_errors= TRUE;
> -}
> -
> -/// Stop saving errors.
> -inline
> -void Logger::stop_save_errors()
> -{
> - if (!m_save_errors)
> - return;
> - m_save_errors= FALSE;
> -}
> + va_list args;
> + bool saved= push_errors(FALSE);
> +
> + va_start(args, error_code);
> + int res= v_report_error(log_level::ERROR, error_code, args);
> + va_end(args);
>
> -/// Delete all saved errors to free resources.
> -inline
> -void Logger::clear_saved_errors()
> -{
> - memset(&error, 0, sizeof(error));
> -}
> + push_errors(saved);
>
> -/// Return a pointer to most recent saved error.
> -inline
> -util::SAVED_MYSQL_ERROR *Logger::last_saved_error()
> -{
> - return error.code ? &error : NULL;
> + return res;
> }
>
> /// Report start of an operation.
> @@ -330,13 +312,20 @@ int Logger::init(enum_type type, const c
> return 0;
>
> m_type= type;
> - m_state= READY;
> + mysql_reset_errors(m_thd, 0); // Never errors
> backup_log = new Backup_log(m_thd, (enum_backup_operation)type,
query);
> backup_log->state(BUP_STARTING);
> + m_state= READY;
> DEBUG_SYNC(m_thd, "after_backup_log_init");
> return 0;
> }
>
> +inline
> +bool Logger::error_reported() const
> +{
> + return m_error_reported;
> +}
> +
> } // backup namespace
>
> #endif
>
> === modified file 'sql/backup/restore_info.h'
> --- a/sql/backup/restore_info.h 2008-05-05 15:06:40 +0000
> +++ b/sql/backup/restore_info.h 2008-11-12 11:20:04 +0000
> @@ -33,7 +33,7 @@ class Restore_info: public backup::Image
> {
> public:
>
> - Backup_restore_ctx &m_ctx;
> + backup::Logger &m_log;
>
> Restore_info(Backup_restore_ctx&);
> ~Restore_info();
> @@ -47,15 +47,21 @@ class Restore_info: public backup::Image
>
> private:
>
> + THD *m_thd;
> +
> friend int backup::restore_table_data(THD*, Restore_info&,
> backup::Input_stream&);
> friend int ::bcat_add_item(st_bstream_image_header*,
> struct st_bstream_item_info*);
> + friend int ::bcat_create_item(st_bstream_image_header *catalogue,
> + struct st_bstream_item_info *item,
> + bstream_blob create_stmt,
> + bstream_blob other_meta_data);
> };
>
> inline
> Restore_info::Restore_info(Backup_restore_ctx &ctx)
> - :m_ctx(ctx)
> + :m_log(ctx), m_thd(ctx.thd())
> {}
>
> inline
> @@ -82,7 +88,7 @@ Restore_info::add_ts(const ::String &nam
> Ts *ts= Image_info::add_ts(name, pos);
>
> if (!ts)
> - m_ctx.fatal_error(ER_BACKUP_CATALOG_ADD_TS, name.ptr());
> + m_log.report_error(ER_BACKUP_CATALOG_ADD_TS, name.ptr());
>
> return ts;
> }
> @@ -95,7 +101,7 @@ Restore_info::add_db(const ::String &nam
> Db *db= Image_info::add_db(name, pos);
>
> if (!db)
> - m_ctx.fatal_error(ER_BACKUP_CATALOG_ADD_DB, name.ptr());
> + m_log.report_error(ER_BACKUP_CATALOG_ADD_DB, name.ptr());
>
> return db;
> }
> @@ -109,7 +115,7 @@ Restore_info::add_table(Image_info::Db &
> Table *t= Image_info::add_table(db, name, snap, pos);
>
> if (!t)
> - m_ctx.fatal_error(ER_BACKUP_CATALOG_ADD_TABLE, db.name().ptr(),
name.ptr());
> + m_log.report_error(ER_BACKUP_CATALOG_ADD_TABLE, db.name().ptr(),
name.ptr());
>
> return t;
> }
>
> === modified file 'sql/backup/stream.cc'
> --- a/sql/backup/stream.cc 2008-10-15 20:00:48 +0000
> +++ b/sql/backup/stream.cc 2008-11-12 11:20:04 +0000
> @@ -645,10 +645,4 @@ bool Input_stream::rewind()
> return ret ? init() : FALSE;
> }
>
> -/// Move to next chunk in the stream.
> -int Input_stream::next_chunk()
> -{
> - return bstream_next_chunk(this);
> -}
> -
> } // backup namespace
>
> === modified file 'sql/backup/stream.h'
> --- a/sql/backup/stream.h 2008-10-15 20:00:48 +0000
> +++ b/sql/backup/stream.h 2008-11-12 11:20:04 +0000
> @@ -98,7 +98,7 @@ class Stream: public fd_stream
> ::String *m_path;
> int m_flags; ///< flags used when opening the file
> size_t m_block_size;
> - Logger m_log;
> + Logger &m_log;
>
> friend int stream_write(void*, bstream_blob*, bstream_blob);
> friend int stream_read(void*, bstream_blob*, bstream_blob);
> @@ -139,8 +139,6 @@ class Input_stream:
> bool close();
> bool rewind();
>
> - int next_chunk();
> -
> private:
>
> int check_magic_and_version();
> @@ -181,6 +179,10 @@ result_t
> read_header(Image_info &info, Input_stream &s)
> {
> int ret= bstream_rd_header(&s,
static_cast<st_bstream_image_header*>(&info));
> +
> + if (ret != BSTREAM_ERROR)
> + ret= bstream_next_chunk(&s);
> +
[23] How is this change related to the purpose of this patch? If they
are not, I think it is better to do this in another patch.
> return ret == BSTREAM_ERROR ? ERROR : OK;
> }
>
> @@ -189,6 +191,10 @@ result_t
> read_catalog(Image_info &info, Input_stream &s)
> {
> int ret= bstream_rd_catalogue(&s,
static_cast<st_bstream_image_header*>(&info));
> +
> + if (ret != BSTREAM_ERROR)
> + ret= bstream_next_chunk(&s);
> +
> return ret == BSTREAM_ERROR ? ERROR : OK;
> }
>
> @@ -197,6 +203,10 @@ result_t
> read_meta_data(Image_info &info, Input_stream &s)
> {
> int ret= bstream_rd_meta_data(&s,
static_cast<st_bstream_image_header*>(&info));
> +
> + if (ret != BSTREAM_ERROR)
> + ret= bstream_next_chunk(&s);
> +
> return ret == BSTREAM_ERROR ? ERROR : OK;
> }
>
>
> === modified file 'sql/share/errmsg.txt'
> --- a/sql/share/errmsg.txt 2008-11-05 15:07:40 +0000
> +++ b/sql/share/errmsg.txt 2008-11-12 11:20:04 +0000
> @@ -6414,3 +6414,5 @@ ER_RESTORE_ON_SLAVE
> eng "A restore operation was attempted on a slave during
replication. You must stop the slave prior to running a restore."
> ER_NONUNIQ_DB 42000 S1009
> eng "Not unique database: '%-.192s'"
> +ER_BACKUP_VP_FAILED
> + eng "Could not create the validity point"
>
>
| Thread | ||
|---|---|---|
| • bzr commit into mysql-6.0-backup branch (Rafal.Somla:2731) Bug#40303Bug#40304 | Rafal Somla | 12 Nov |
| • RE: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2731)Bug#40303 Bug#40304 | Chuck Bell | 13 Nov |
| • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2731)Bug#40303 Bug#40304 | Rafal Somla | 18 Nov |
| • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2731)Bug#40303 Bug#40304 | Øystein Grøvlen | 17 Nov |
| • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2731)Bug#40303 Bug#40304 | Rafal Somla | 18 Nov |
| • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2731)Bug#40303 Bug#40304 | Øystein Grøvlen | 19 Nov |
© 1995-2008 MySQL AB, 2008-2009 Sun Microsystems, Inc.
Page generated in 1.131 sec. using MySQL 5.1.14-beta-log