Hi,
Patch good to push after addressing minor issues mentioned below. For the
future, I think you plan to change progress reporting so that the progress
tables are opened only once (at the beginning of backup/restore) and closed at
the end. Then the opening of these tables would be a good place to check for
this error and no extra function like check_ob_progress_tables() would be needed.
Rafal
Chuck Bell wrote:
> X-CSetKey: <cbell/Chuck@mysql_cab_desk.|ChangeSet|20080103223110|24819>
> X-Bug: 33352
>
> Below is the list of changes that have just been committed into a local
> 6.0 repository of cbell. When cbell does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2008-01-03 17:31:10-05:00, cbell@mysql_cab_desk. +6 -0
> BUG#33352 : Backup:crash if I use old set of mysql files.
>
> Added capability to throw an error if either of the backup
> progress tables are missing.
>
> mysql-test/r/backup_errors.result@stripped, 2008-01-03 17:31:01-05:00,
> cbell@mysql_cab_desk. +33 -0
> BUG#33352 : Backup:crash if I use old set of mysql files.
>
> New result file.
>
> mysql-test/t/backup_errors.test@stripped, 2008-01-03 17:31:02-05:00,
> cbell@mysql_cab_desk. +54 -0
> BUG#33352 : Backup:crash if I use old set of mysql files.
>
> New test for error handling.
>
> sql/backup/backup_progress.cc@stripped, 2008-01-03 17:31:02-05:00,
> cbell@mysql_cab_desk. +39 -1
> BUG#33352 : Backup:crash if I use old set of mysql files.
>
> Added a method to check to see if the backup progress tables exist.
>
> sql/backup/backup_progress.h@stripped, 2008-01-03 17:31:03-05:00,
> cbell@mysql_cab_desk. +6 -0
> BUG#33352 : Backup:crash if I use old set of mysql files.
>
> Added a method to check to see if the backup progress tables exist.
>
> sql/backup/kernel.cc@stripped, 2008-01-03 17:31:03-05:00,
> cbell@mysql_cab_desk. +9 -0
> BUG#33352 : Backup:crash if I use old set of mysql files.
>
> Added call to check if progress tables exist. If not, the code
> throws an error.
>
> sql/share/errmsg.txt@stripped, 2008-01-03 17:31:04-05:00,
> cbell@mysql_cab_desk. +3 -0
> BUG#33352 : Backup:crash if I use old set of mysql files.
>
> Added error message for backup progress tables.
>
> diff -Nrup a/mysql-test/r/backup_errors.result
> b/mysql-test/r/backup_errors.result
> --- a/mysql-test/r/backup_errors.result 2007-12-13 11:52:49 -05:00
> +++ b/mysql-test/r/backup_errors.result 2008-01-03 17:31:01 -05:00
> @@ -22,3 +22,36 @@ BACKUP DATABASE test,foo,bdb,bar TO 'tes
> ERROR 42000: Unknown database 'foo,bar'
> DROP DATABASE adb;
> DROP DATABASE bdb;
> +Making copies of progress tables.
> +CREATE TABLE IF NOT EXISTS mysql.ob_copy LIKE mysql.online_backup;
> +CREATE TABLE IF NOT EXISTS mysql.obp_copy LIKE
> mysql.online_backup_progress;
> +CREATE DATABASE test_ob_error;
> +CREATE TABLE test_ob_error.t1 (col_a int);
> +INSERT INTO test_ob_error.t1 VALUES (1), (2), (3), (4), (5);
> +Backup the database;
> +BACKUP DATABASE test_ob_error TO 'ob_err.bak';
> +backup_id
> +#
> +DROP TABLE mysql.online_backup;
> +Backup the database;
> +BACKUP DATABASE test_ob_error TO 'ob_err.bak';
> +ERROR 42S02: Table 'mysql.online_backup' doesn't exist
> +SHOW ERRORS;
> +Level Code Message
> +Error 1146 Table 'mysql.online_backup' doesn't exist
> +Error 1649 Can't open the online backup progress tables
> 'mysql.online_backup' and 'mysql.online_backup_progress'.
> +Restoring the table
> +CREATE TABLE mysql.online_backup LIKE mysql.ob_copy;
> +DROP TABLE mysql.ob_copy;
> +DROP TABLE mysql.online_backup_progress;
> +Backup the database;
> +BACKUP DATABASE test_ob_error TO 'ob_err.bak';
> +ERROR 42S02: Table 'mysql.online_backup_progress' doesn't exist
> +SHOW ERRORS;
> +Level Code Message
> +Error 1146 Table 'mysql.online_backup_progress' doesn't exist
> +Error 1649 Can't open the online backup progress tables
> 'mysql.online_backup' and 'mysql.online_backup_progress'.
> +Restoring the table
> +CREATE TABLE mysql.online_backup_progress LIKE mysql.obp_copy;
> +DROP TABLE mysql.obp_copy;
> +DROP DATABASE test_ob_error;
> diff -Nrup a/mysql-test/t/backup_errors.test
> b/mysql-test/t/backup_errors.test
> --- a/mysql-test/t/backup_errors.test 2007-12-13 11:58:31 -05:00
> +++ b/mysql-test/t/backup_errors.test 2008-01-03 17:31:02 -05:00
> @@ -57,3 +57,57 @@ DROP DATABASE bdb;
> # will fail and we will be warned that something is wrong with the test
> --error 1
> --remove_file $MYSQLTEST_VARDIR/master-data/test.bak
> +
> +#
> +# BUG#33352 - Test for presence of online backup progress tables;
> +#
> +
> +# Make backup copies of the tables first.
> +--echo Making copies of progress tables.
> +CREATE TABLE IF NOT EXISTS mysql.ob_copy LIKE mysql.online_backup;
> +CREATE TABLE IF NOT EXISTS mysql.obp_copy LIKE
> mysql.online_backup_progress;
I'm not sure if it is a good idea to create a copy inside mysql database. Would
it be possible to say:
CREATE TABLE test.ob_copy LIKE mysql.online_backup?
> +
> +# Create a database and put some data in it.
> +CREATE DATABASE test_ob_error;
> +CREATE TABLE test_ob_error.t1 (col_a int);
> +INSERT INTO test_ob_error.t1 VALUES (1), (2), (3), (4), (5);
> +
> +# Backup the database to ensure db is ok.
> +--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
> +
> +# Drop one of the tables and try a backup.
> +DROP TABLE mysql.online_backup;
> +
> +# Try to backup the database (should be error).
> +--echo Backup the database;
> +--error ER_NO_SUCH_TABLE
> +BACKUP DATABASE test_ob_error TO 'ob_err.bak';
> +--error 0,1
> +--remove_file $MYSQLTEST_VARDIR/master-data/ob_err.bak
> +SHOW ERRORS;
> +
> +# Restore the table
> +--echo Restoring the table
> +CREATE TABLE mysql.online_backup LIKE mysql.ob_copy;
> +DROP TABLE mysql.ob_copy;
> +
> +# Drop one of the tables and try a backup.
> +DROP TABLE mysql.online_backup_progress;
> +
> +# Try to backup the database (should be error).
> +--echo Backup the database;
> +--error ER_NO_SUCH_TABLE
> +BACKUP DATABASE test_ob_error TO 'ob_err.bak';
> +--error 0,1
> +--remove_file $MYSQLTEST_VARDIR/master-data/ob_err.bak
> +SHOW ERRORS;
> +
> +# Restore the table
> +--echo Restoring the table
> +CREATE TABLE mysql.online_backup_progress LIKE mysql.obp_copy;
> +DROP TABLE mysql.obp_copy;
> +
> +DROP DATABASE test_ob_error;
> diff -Nrup a/sql/backup/backup_progress.cc b/sql/backup/backup_progress.cc
> --- a/sql/backup/backup_progress.cc 2007-12-06 13:04:38 -05:00
> +++ b/sql/backup/backup_progress.cc 2008-01-03 17:31:02 -05:00
> @@ -27,7 +27,45 @@
> #include "backup_progress.h"
> #include "be_thread.h"
>
> -extern bool check_if_table_exists(THD *thd, TABLE_LIST *table, bool
> *exists);
> +/**
> + Check online backup progress tables.
> +
> + This method attempts to open the online backup progress tables. It
> returns
> + an error if either table is not present or cannot be opened.
> +
> + @param THD * The current thread.
> +
> + @returns FALSE = success
> + @returns TRUE = failed to open one of the tables
Should be formatted like this:
@retval FALSE success
@retval TRUE failed to open one of the tables
This can be preceded by general description of returned value introduced with
@returns keyword. For example
@returns Information whether backup progress tables can be used.
> + */
> +my_bool check_ob_progress_tables(THD *thd)
> +{
> + TABLE_LIST tables;
> + my_bool ret= FALSE;
> +
> + DBUG_ENTER("check_ob_progress_tables");
> +
> + /* Check mysql.online_backup */
> + tables.init_one_table("mysql", "online_backup", TL_READ);
Good to know :)
> + if (simple_open_n_lock_tables(thd, &tables))
> + {
> + ret= TRUE;
> + sql_print_error("Cannot open mysql.online_backup");
OK, you intentionally report the problem twice: here with more precise
information and below with "my_error(ER_BACKUP_PROGRESS_TABLES, MYF(0))", right?
Any reason why the above message should not be internationalized as well?
> + DBUG_RETURN(ret);
> + }
> + close_thread_tables(thd);
> +
> + /* Check mysql.online_backup_progress */
> + tables.init_one_table("mysql", "online_backup_progress", TL_READ);
> + if (simple_open_n_lock_tables(thd, &tables))
> + {
> + ret= TRUE;
> + sql_print_error("Cannot open mysql.online_backup_progress");
> + DBUG_RETURN(ret);
> + }
> + close_thread_tables(thd);
> + DBUG_RETURN(ret);
> +}
>
> /**
> Open backup progress table.
> diff -Nrup a/sql/backup/backup_progress.h b/sql/backup/backup_progress.h
> --- a/sql/backup/backup_progress.h 2007-12-03 15:28:11 -05:00
> +++ b/sql/backup/backup_progress.h 2008-01-03 17:31:03 -05:00
> @@ -93,6 +93,12 @@ enum enum_backup_op
> };
>
> /*
> + This method attempts to open the online backup progress tables. It
> returns
> + an error if either table is not present or cannot be opened.
> +*/
> +my_bool check_ob_progress_tables(THD *thd);
> +
> +/*
> This method inserts a new row in the online_backup table populating it
> with
> the initial values passed. It returns the backup_id of the new row.
> */
> diff -Nrup a/sql/backup/kernel.cc b/sql/backup/kernel.cc
> --- a/sql/backup/kernel.cc 2007-12-14 02:51:28 -05:00
> +++ b/sql/backup/kernel.cc 2008-01-03 17:31:03 -05:00
> @@ -99,6 +99,15 @@ execute_backup_command(THD *thd, LEX *le
> DBUG_RETURN(ER_SPECIFIC_ACCESS_DENIED_ERROR);
> }
>
> + /*
> + Check for progress tables.
> + */
> + if (check_ob_progress_tables(thd))
> + {
> + my_error(ER_BACKUP_PROGRESS_TABLES, MYF(0));
> + DBUG_RETURN(ER_BACKUP_PROGRESS_TABLES);
> + }
> +
> Location *loc= Location::find(lex->backup_dir);
>
> if (!loc || !loc->is_valid())
> diff -Nrup a/sql/share/errmsg.txt b/sql/share/errmsg.txt
> --- a/sql/share/errmsg.txt 2007-12-13 11:49:58 -05:00
> +++ b/sql/share/errmsg.txt 2008-01-03 17:31:04 -05:00
> @@ -6202,6 +6202,9 @@ ER_BACKUP_OPEN_TABLES
> ER_BACKUP_THREAD_INIT
> eng "Backup driver's table locking thread can not be initialized."
>
> +ER_BACKUP_PROGRESS_TABLES
> + eng "Can't open the online backup progress tables
> 'mysql.online_backup' and 'mysql.online_backup_progress'."
> +
> ER_VIEW_NO_CREATION_CTX
> eng "View `%-.64s`.`%-.64s` has no creation context"
> ER_VIEW_INVALID_CREATION_CTX
>
>
