STATUS
------
Patch rejected.
REQUIRED
--------
1. Fix collateral test failures.
2. Why is the CREATE statement different?
REQUESTS
--------
3. Please state the problem in the patch comments.
4. Please use correct grammar and complete sentences when possible.
5. Why are we testing this?
6. Why is the indentation done this way?
COMMENTARY
----------
Thank you for fixing the issues I cited in the previous review.
Unfortunately, there are collateral test failures and the nature of
these failures is the reason I am rejecting the patch. While I do not
see anything in the patch that is a show stopper, the fact that it
changes the behavior of SHOW CREATE which is unacceptable.
DETAILS
-------
> BUG#33354 Backup: restore changes current database to null
>
> Added a variable m_db_saved_string to Si_session_context
> to save the default database if available.
>
> added test backup_use_db_restore.test and its result file
> to the backup suite.
[3] We need to include a short statement about what the problem is we're
trying to solve. It is not sufficient to simply state the solution. Just
a wee bit more text will go a long ways. ;)
[4] Second paragraph should start with Added...
...
> === added file 'mysql-test/suite/backup/r/backup_use_db_restore.result'
> === added file 'mysql-test/suite/backup/r/backup_use_db_restore.result'
> --- a/mysql-test/suite/backup/r/backup_use_db_restore.result 1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/backup/r/backup_use_db_restore.result 2009-11-04 13:38:43
> +0000
> @@ -0,0 +1,64 @@
> +#
> +# This test ensures that if there is a database in use before
> +# a "restore" command, then the same database remains the
> +# default database even after restore.
> +#
> +# for details see : Bug#33354 : Backup: restore changes current
> +# database to null
> === 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-04 13:38:43 +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
[4]
...
> +--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();
[5] I don't think that is why. I think you mean you want to ensure
databases with UTF-8 character names can be returned to default. Stating
'international' is a big vague.
> +
> +--replace_column 1 #
> +BACKUP DATABASE db1 to 'image3.bak';
> +
> +--replace_column 1 #
> +RESTORE FROM 'image3.bak' overwrite;
Just a style comment here. Most of us use all caps for commands and
lowercase for object names. For example:
RESTORE FROM 'image3.bak' OVERWRITE;
The above is a better style convention.
...
> === modified file 'sql/si_objects.cc'
> --- a/sql/si_objects.cc 2009-10-23 21:26:11 +0000
> +++ b/sql/si_objects.cc 2009-11-04 13:38:43 +0000
> @@ -75,6 +75,7 @@
> 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,16 @@
> 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);
> + else
> + /*
> + Clear up and old data in the saved database since there is no default
> + database set.
> + */
> + if (!m_db_saved_string.is_empty())
> + m_db_saved_string= 0;
> DBUG_VOID_RETURN;
[6] We would normally do this:
if (something)
{
}
/*
Some comment
*/
else if (something_else)
{
}
The indentation you have for the else if is strange. Please consider the
style above.
...
[1] There were three tests that failed as a result of this patch. These
must be corrected. I paste the test run here because I observe something
disturbing. Note: Please make sure you run all of the tests before
committing a patch. Most reviewers do not run the entire test suite. I
do (but it is a long story).
Logging: ./mysql-test-run.pl backup.backup_events backup.backup_objects
backup.backup_views --force --retry=0
MySQL Version 6.0.14
Checking supported features...
- skipping ndbcluster, mysqld not compiled with ndbcluster
- SSL connections supported
- binaries are debug compiled
Collecting tests...
vardir: /Users/cbell/source/bzr/mysql-6.0-review/mysql-test/var
Checking leftover processes...
Removing old var directory...
Creating var directory
'/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/var'...
Installing system database...
Using server port 50446
==============================================================================
TEST RESULT TIME (ms)
------------------------------------------------------------
worker[1] Using MTR_BUILD_THREAD 300, with reserved ports 13000..13009
backup.backup_events [ fail ]
Test ended at 2009-11-05 15:50:41
CURRENT_TEST: backup.backup_events
---
/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/suite/backup/r/backup_events.result
2009-11-05 23:09:58.000000000 +0300
+++
/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/suite/backup/r/backup_events.reject
2009-11-05 23:50:41.000000000 +0300
@@ -110,51 +110,6 @@
backup_id
#
SHOW EVENTS;;
-Db events
-Name e1
-Definer root@localhost
-Time zone SYSTEM
-Type ONE TIME
-Execute at #
-Interval value NULL
-Interval field NULL
-Starts #
-Ends #
-Status DISABLED
-Originator 1
-character_set_client latin1
-collation_connection latin1_swedish_ci
-Database Collation latin1_swedish_ci
-Db events
-Name ee
-Definer root@localhost
-Time zone SYSTEM
-Type RECURRING
-Execute at #
-Interval value 2
-Interval field SECOND
-Starts #
-Ends #
-Status ENABLED
-Originator 1
-character_set_client latin1
-collation_connection latin1_swedish_ci
-Database Collation latin1_swedish_ci
-Db events
-Name e_rename
-Definer root@localhost
-Time zone SYSTEM
-Type RECURRING
-Execute at #
-Interval value 1
-Interval field SECOND
-Starts #
-Ends #
-Status ENABLED
-Originator 1
-character_set_client latin1
-collation_connection latin1_swedish_ci
-Database Collation latin1_swedish_ci
INSERT INTO events.t2 VALUES('d'),('p'),('c'),('c');
# Include wait_condition and sleep so that events are executed properly
SELECT * FROM events.t2 ORDER BY a;
mysqltest: Result content mismatch
[1] This does not look right but I am not sure why. Might be ok. Please
investigate.
- saving
'/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/var/log/backup.backup_events/'
to
'/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/var/log/backup.backup_events/'
backup.backup_objects [ fail ]
Test ended at 2009-11-05 15:50:43
CURRENT_TEST: backup.backup_objects
---
/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/suite/backup/r/backup_objects.result
2009-11-05 23:08:49.000000000 +0300
+++
/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/suite/backup/r/backup_objects.reject
2009-11-05 23:50:43.000000000 +0300
@@ -229,7 +229,7 @@
collation_connection latin1_swedish_ci
SHOW CREATE VIEW db2.v21;;
View v21
-Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL
SECURITY DEFINER VIEW `v21` AS select `x`.`b` AS
`b`,`db2`.`f21`(`y`.`a`) AS `a` from (`db1`.`v11` `x` join `db2`.`v22`
`y`) where (`x`.`a` = `y`.`b`)
+Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL
SECURITY DEFINER VIEW `db2`.`v21` AS select `x`.`b` AS
`b`,`db2`.`f21`(`y`.`a`) AS `a` from (`db1`.`v11` `x` join `db2`.`v22`
`y`) where (`x`.`a` = `y`.`b`)
character_set_client latin2
collation_connection latin1_swedish_ci
SHOW CREATE PROCEDURE db1.p11;;
mysqltest: Result content mismatch
[2] This is not acceptable. We expect the CREATE statements to be of the
form CREATE ... db.objectname. The change this patch has made should not
have changed the CREATE statement output. Please investigate and change
the patch so this does not happen.
- saving
'/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/var/log/backup.backup_objects/'
to
'/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/var/log/backup.backup_objects/'
backup.backup_views [ fail ]
Test ended at 2009-11-05 15:50:46
CURRENT_TEST: backup.backup_views
---
/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/suite/backup/r/backup_views.result
2009-11-05 23:08:49.000000000 +0300
+++
/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/suite/backup/r/backup_views.reject
2009-11-05 23:50:46.000000000 +0300
@@ -317,17 +317,17 @@
Table_type VIEW
SHOW CREATE VIEW bup_db1.v1;;
View v1
-Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL
SECURITY DEFINER VIEW `bup_db1`.`v1` AS select `bup_db1`.`t1`.`id` AS
`id`,`bup_db1`.`t1`.`name` AS `name`,`bup_db1`.`t1`.`city` AS `city`
from `bup_db1`.`t1`
+Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL
SECURITY DEFINER VIEW `v1` AS select `t1`.`id` AS `id`,`t1`.`name` AS
`name`,`t1`.`city` AS `city` from `t1`
character_set_client latin1
collation_connection latin1_swedish_ci
SHOW CREATE VIEW bup_db1.vcomb;;
View vcomb
-Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL
SECURITY DEFINER VIEW `bup_db1`.`vcomb` AS select `bup_db1`.`t1`.`name`
AS `name`,`bup_db1`.`t1`.`city` AS `city`,`bup_db1`.`t3`.`ccode` AS
`ccode` from (`bup_db1`.`t1` join `bup_db1`.`t3`) where
(`bup_db1`.`t1`.`id` = `bup_db1`.`t3`.`scode`)
+Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL
SECURITY DEFINER VIEW `vcomb` AS select `t1`.`name` AS
`name`,`t1`.`city` AS `city`,`t3`.`ccode` AS `ccode` from (`t1` join
`t3`) where (`t1`.`id` = `t3`.`scode`)
character_set_client latin1
collation_connection latin1_swedish_ci
SHOW CREATE VIEW bup_db2.v3;;
View v3
-Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL
SECURITY DEFINER VIEW `v3` AS select `bup_db1`.`t1`.`name` AS
`name`,`bup_db2`.`t2`.`age` AS `age`,`bup_db2`.`t2`.`education` AS
`education` from (`bup_db1`.`t1` join `bup_db2`.`t2`) where
(`bup_db1`.`t1`.`id` = `bup_db2`.`t2`.`idno`)
+Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL
SECURITY DEFINER VIEW `bup_db2`.`v3` AS select `bup_db1`.`t1`.`name` AS
`name`,`bup_db2`.`t2`.`age` AS `age`,`bup_db2`.`t2`.`education` AS
`education` from (`bup_db1`.`t1` join `bup_db2`.`t2`) where
(`bup_db1`.`t1`.`id` = `bup_db2`.`t2`.`idno`)
character_set_client latin1
collation_connection latin1_swedish_ci
****check for view contents after Restore*****
mysqltest: Result content mismatch
[2] Again, this is wrong and should not be doing this.
- saving
'/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/var/log/backup.backup_views/'
to
'/Users/cbell/source/bzr/mysql-6.0-review/mysql-test/var/log/backup.backup_views/'
------------------------------------------------------------
The servers were restarted 2 times
Spent 0.000 of 43 seconds executing testcases
Failed 3/3 tests, 0.00% were successful.
Failing test(s): backup.backup_events backup.backup_objects
backup.backup_views
The log files in var/log may give you some hint of what went wrong.
If you want to report this error, please read first the documentation
at http://dev.mysql.com/doc/mysql/en/mysql-test-suite.html
Chuck
