Hi Sanjay,
STATUS
------
Not approved.
REQUIRED
--------
1. Resolve issue with failing tests: backup_restore_locking and
backup_use_db_restore.
2. Fix strange diff for backup_events.test - probably you got DOS line
endings in this file.
3. Use thd->set_db() to avoid possible memory leaks.
SUGGESTIONS
-----------
4. Minor test case improvements.
5. Reuse Ed_connection object in Si_session_context.
DETAILS
-------
[1] When I run backup_restore_locking test after applying your patch, the
test timeouts and I see this:
> CURRENT_TEST: backup.backup_restore_locking
> ---
> /ext/mysql/bzr/mysql-6.0-backup/mysql-test/suite/backup/r/backup_restore_locking.result
> 2009-11-18 10:26:51.000000000 +0300
> +++
> /ext/mysql/bzr/mysql-6.0-backup/mysql-test/suite/backup/r/backup_restore_locking.reject
> 2009-11-19 17:32:15.000000000 +0300
> @@ -97,6 +97,8 @@
> # connection default, retrieve RESTORE result
> backup_id
> ###
> +Warnings:
> +### 1743 debug sync point wait timed out
> SET DEBUG_SYNC= 'RESET';
> #
> # connection con1, retrieve INSERT result.
>
> mysqltest: Result content mismatch
I see this behaviour only if your patch is applied - without your patch test
passes fine.
[1] I also got this error when running backup_use_db_restore:
> CURRENT_TEST: backup.backup_use_db_restore
> mysqltest: At line 58: query 'CREATE DATABASE
> `ï¾#ï¾#ï¾#ｺï¾#`'
> failed: 1300: Invalid utf8 character string:
> '\xEF\xBE#\xEF\xBE#\xEF\xBE#\xEF\xBD\xBA\xEF\xBE#'
Could it be that Win is not correctly storing utf8 strings?
[2] See the diff for backup_events.test in your patch - it replaces almost
every line in the test by seemingly identical line (I did not quote it
here). Most probably this is the issue of DOS line endings which got into
that file. In MySQL tree we use unix line ends.
Sanjay Manwani wrote:
> #At file:///C:/Work/mysql_checkouts/mysql-6.0-backup-02/ based on
> revid:thavamuni.alagu@stripped
>
> 2894 Sanjay Manwani 2009-11-18
> BUG#33354 Backup: restore changes current database to null
>
> If there is a database set as a default database by
> the SQL command "use <Database>"before a
> restore; this default database gets reset to blank
> after the restore.
> The si context now also saves the default database,
> and restores it .
>
> @ si_objects.cc
> Added a variable m_db_saved_string to
> Si_session_context to save the default
> database.
> Extended the restore_si_ctx and save_si_ctx to
> save and restore the default database into/from
> this variable
>
> @ backup_use_db_restore.test
> New test to ensure the 'USE' database is still default
> ensures using an keeping as default of ascii and
> utf8 character databases.
>
> @ backup_use_db_restore.result
> Added the results of above test.
>
> @ backup_objects.result
> Changed this result file since objects are now
> reported without the database. Changed the
> object in the from e.g. db1.object to simply object.
>
> @ backup_view.result
> Same change as objects. Views are changed
> to simply view1 instead of say db1.view1.
>
> @ backup_events.test
> An event was not visible since the test assumed
> that the backed up database would be the default
> database after restore. Added an explicit 'USE events'
> to be able to view the event.
>
> @ backup_events.result
> Changed to reflect the change in the test.
>
> added:
> mysql-test/suite/backup/r/backup_use_db_restore.result
> mysql-test/suite/backup/t/backup_use_db_restore.test
> modified:
> mysql-test/suite/backup/r/backup_events.result
> mysql-test/suite/backup/r/backup_objects.result
> mysql-test/suite/backup/r/backup_views.result
> mysql-test/suite/backup/t/backup_events.test
> sql/si_objects.cc
> === added file 'mysql-test/suite/backup/t/backup_use_db_restore.test'
> --- a/mysql-test/suite/backup/t/backup_use_db_restore.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/backup/t/backup_use_db_restore.test 2009-11-18 17:33:26 +0000
> @@ -0,0 +1,84 @@
> +--echo #
> +--echo # This test ensures that if there is a database in use before
> +--echo # a "restore" command, then the same database remains the
> +--echo # default database even after restore.
> +--echo #
> +--echo # for details see : Bug#33354 : Backup: restore changes current
> +--echo # database to null
> +--echo #
> +
> +
> +--disable_warnings
> +DROP DATABASE IF EXISTS db1;
> +--enable_warnings
> +
> +# Use the test database
> +USE test;
> +SELECT DATABASE();
> +
> +# Now create a database abd backup and restore it
[4] Typo: Should be "database db1"?
> +CREATE DATABASE db1;
> +CREATE TABLE db1.t (i int, j int) engine=myisam;
> +INSERT INTO db1.t values(10, 11);
> +
> +--replace_column 1 #
> +BACKUP DATABASE db1 to 'image1.bak';
> +
[4] add "SELECT database()" also here, so that we can check that BACKUP does
not mess with the current database too.
> +--replace_column 1 #
> +RESTORE FROM 'image1.bak' overwrite;
> +
> +# The database 'test' should still be the default database
> +SELECT DATABASE();
> +
> +--echo #
> +--echo # Try again now using the database which is being restored
> +--echo #
> +
> +USE db1;
> +SELECT DATABASE();
> +
> +--replace_column 1 #
> +BACKUP DATABASE db1 to 'image2.bak';
[4] Actually, no need to do BACKUP for the second time - you could restore
from the same image as before.
> +
> +--replace_column 1 #
> +RESTORE FROM 'image2.bak' overwrite;
> +
> +# The restored database should be the default database
> +SELECT DATABASE();
> +
> +# Now lets try a UTF8 character set database
> +
> +--disable_warnings
> +DROP DATABASE IF EXISTS
> `ﾆﾎ�ｺﾞ`;
> +--enable_warnings
> +
> +SET NAMES utf8;
> +SET character_set_database = utf8;
> +
> +CREATE DATABASE
> `ﾆﾎ�ｺﾞ`;
> +
> +USE
> `ﾆﾎ�ｺﾞ`;
> +
> +# Ensure that our international database is indeed the default
> +SELECT DATABASE();
> +
> +--replace_column 1 #
> +BACKUP DATABASE db1 to 'image3.bak';
[4] Not really needed - you can reuse first image.
> +
> +--replace_column 1 #
> +RESTORE FROM 'image3.bak' overwrite;
> +
> +# After restore the default database should be the international database
> +SELECT DATABASE();
> +
> +--echo #
> +--echo # Cleanup
> +--echo #
> +
> +DROP TABLE db1.t;
[4] Not needed - DROP DATABASE db1 will remove all objects in that database.
> +DROP DATABASE db1;
> +DROP DATABASE
> `ﾆﾎ�ｺﾞ`;
> +let $MYSQLD_BACKUPDIR= `select @@backupdir`;
> +remove_file $MYSQLD_BACKUPDIR/image1.bak;
> +remove_file $MYSQLD_BACKUPDIR/image2.bak;
> +remove_file $MYSQLD_BACKUPDIR/image3.bak;
>
> === modified file 'sql/si_objects.cc'
> --- a/sql/si_objects.cc 2009-11-17 20:18:50 +0000
> +++ b/sql/si_objects.cc 2009-11-18 17:33:26 +0000
> @@ -75,6 +75,7 @@ private:
> Time_zone *m_tz_saved;
> TABLE *m_tmp_tables_saved;
> bool m_engage_general_log;
> + String m_db_saved_string;
>
> private:
> Si_session_context(const Si_session_context &);
> @@ -104,6 +105,21 @@ void Si_session_context::save_si_ctx(THD
> m_tz_saved= thd->variables.time_zone;
> m_tmp_tables_saved= thd->temporary_tables;
> m_old_db_collation= thd->variables.collation_database;
> +
> + /* Saving the default database */
> + if (thd->db)
> + m_db_saved_string.copy(thd->db, thd->db_length, thd->db_charset);
> + /*
> + Clear up and old data in the saved database since there is no default
> + database set.
> + */
> + else
> + if (!m_db_saved_string.is_empty())
> + m_db_saved_string= 0;
> + /* clean up database info from thd context */
> + thd->db= 0;
> + thd->db_length= 0;
[3] I think thd->set_db(NULL, 0) should be used - otherwise we might leak
memory.
> +
> DBUG_VOID_RETURN;
> }
>
> @@ -154,8 +170,21 @@ void Si_session_context::reset_si_ctx(TH
>
> void Si_session_context::restore_si_ctx(THD *thd)
> {
> +
> + String query ("USE `",m_db_saved_string.charset());
> + Ed_connection ed_connection(thd);
[5] Pass Ed_connection object to the constructor of Si_session_context and
then it can be re-used here - no need to create a new instance. For example,
the code in run_service_interface_sql() could look as follows:
run_service_interface_sql(THD *thd, Ed_connection *ed_connection,
const LEX_STRING *query, bool get_warnings)
{
Si_session_context session_context(ed_connection);
// ...
session_context.save_si_ctx(thd);
session_context.reset_si_ctx(thd);
// ...
session_context.restore_si_ctx(thd);
DBUG_RETURN(rc);
}
The pointer to Ed_connection instance would be saved within session_context
and then used in restore_si_ctx to execute the "USE db_name" statement.
Rafal
