Hi Chuck,
State: undefined, need answers for a decision on approval
Requests: minor (typo, alignment), please see below
Suggestions: several, please see below
Questions: some, please see below
Chuck Bell, 06.01.2009 21:28:
...
> 2746 Chuck Bell 2009-01-06
> BUG#39780 Allow restore to skip gap event during restore on master
>
> This patch adds a new option to the restore command: skip_gap_event.
> It is used to skip writing the incident event when a restore is
> run on a master in an active replication topology (gap event).
...
> --- a/mysql-test/suite/rpl/r/rpl_backup.result 2008-12-16 20:54:07 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_backup.result 2009-01-06 20:28:28 +0000
...
> +Now back to the master to make a backup of the new database.
> +Backup_id = 505.
> +BACKUP DATABASE not_replicated TO 'rpl_bup_m4.bak';
> +backup_id
> +505
> +Now let's run the restore and see if the slave stops.
> +Backup_id = 506.
> +RESTORE FROM 'rpl_bup_m4.bak' OVERWRITE SKIP_GAP_EVENT;
> +backup_id
> +506
> +Let's ensure replication is still running.
> +INSERT INTO rpl_backup.t1 VALUES (901), (902), (903);
> +SELECT * FROM rpl_backup.t1 WHERE a > 900;
> +a
> +901
> +902
> +903
> +Now stop the slave.
> +SLAVE STOP;
Due to the "echo"es, the result is good readable. If you like, you could
improve readability even more.
The comments would be identifiable better, if they would start with a #.
For example, when reading "Backup_id = 505", I suspected a cool feature
to preset the backup id. Only the test source showed that it is a mere
comment.
I would have understood the test a bit faster, if you had marked
connection changes, like we do in many other tests: "# connection slave".
...
> --- a/mysql-test/suite/rpl/t/rpl_backup.test 2008-12-16 20:54:07 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_backup.test 2009-01-06 20:28:28 +0000
> @@ -432,6 +432,65 @@ SLAVE STOP;
> --source include/wait_for_slave_to_stop.inc
>
> #
> +# Test case for BUG#39780 - Skip gap event on restore.
> +#
> +# The new SKIP_GAP_EVENT option for the RESTORE command
> +# allows users to pruposefully skip writing the gap event
Typo pruposefully
> +# when the restore is run on a master and the databases
> +# being restored do not exist on the slave nor are they
> +# intended to be on the slave.
> +#
> +
> +--echo First, reset replication.
> +connection master;
> +RESET MASTER;
> +
> +connection slave;
> +RESET SLAVE;
> +SET DEBUG_SYNC = 'reset';
The test doesn't seem to use DEBUG_SYNC. So I suggest not to reset it
either. This makes the test independent from DEBUG_SYNC altogether.
> +
> +connection slave;
> +START SLAVE;
> +--source include/wait_for_slave_to_start.inc
> +
> +--echo Create a new database on the master.
> +connection master;
> +
> +CREATE DATABASE not_replicated;
> +CREATE TABLE not_replicated.t1 (a int);
> +INSERT INTO not_replicated.t1 VALUES (200), (300), (400);
Just curious: Is the test suite replication set up so that names like
"not_replicated" are not replicated?
> +
> +SHOW DATABASES;
> +
> +--echo Now let's see if it is replicated on the slave (shouldn't be).
> +sync_slave_with_master;
> +connection slave;
> +
> +SHOW DATABASES;
> +
> +--echo Now back to the master to make a backup of the new database.
> +connection master;
> +
> +--echo Backup_id = 505.
> +BACKUP DATABASE not_replicated TO 'rpl_bup_m4.bak';
Just curious: How do you assure that the backup id will be 505 here?
Will this still be the case if someone adds or removes tests before this
one?
> +
> +--echo Now let's run the restore and see if the slave stops.
> +--echo Backup_id = 506.
> +RESTORE FROM 'rpl_bup_m4.bak' OVERWRITE SKIP_GAP_EVENT;
> +
> +--echo Let's ensure replication is still running.
> +INSERT INTO rpl_backup.t1 VALUES (901), (902), (903);
> +
> +sync_slave_with_master;
> +connection slave;
> +
> +SELECT * FROM rpl_backup.t1 WHERE a > 900;
> +
> +--echo Now stop the slave.
> +SLAVE STOP;
> +--source include/wait_for_slave_to_stop.inc
> +
Do we have a test somewhere, which proves that replication is stopped by
a restore without the SKIP_GAP_EVENT option?
> +#
> # Cleanup
> #
> connection master;
> @@ -439,10 +498,12 @@ connection master;
> FLUSH BACKUP LOGS;
> PURGE BACKUP LOGS;
> DROP DATABASE rpl_backup;
> +DROP DATABASE not_replicated;
>
> --remove_file $MYSQLTEST_VARDIR/master-data/rpl_bup_m1.bak
> --remove_file $MYSQLTEST_VARDIR/master-data/rpl_bup_m2.bak
> --remove_file $MYSQLTEST_VARDIR/master-data/rpl_bup_m3.bak
> +--remove_file $MYSQLTEST_VARDIR/master-data/rpl_bup_m4.bak
Just curious: Is it useful to pile up all these backup files? Wouldn't
it be better to remove the backup file(s) after each test?
I suggest to try to remove the file(s) in the preparatory cleanup
section at test case begin. Just in case they are left over from a
crashed test case.
...
> --- a/sql/backup/backup_kernel.h 2008-12-18 21:46:36 +0000
> +++ b/sql/backup/backup_kernel.h 2009-01-06 20:28:28 +0000
> @@ -30,7 +30,7 @@ void backup_shutdown();
> Called from the big switch in mysql_execute_command() to execute
> backup related statement
> */
> -int execute_backup_command(THD*, LEX*, String*, bool);
> +int execute_backup_command(THD*, LEX*, String*, bool, bool);
I would prefer to have meaningful parameter names here.
Our coding guidelines say that functions should be documented at their
definition, not their declaration. I'm not happy with it, but ok.
However, I think it is ok to have at least the minimal documentation of
using meaningful parameter names. In many cases it saves a lookup of the
documentation.
When changing the signature of a function, it should also be ok to add
the parameter names.
>
> // forward declarations
>
> @@ -73,7 +73,7 @@ public:
> const char*, bool);
> Restore_info* prepare_for_restore(String *location,
> LEX_STRING orig_loc,
> - const char*);
> + const char*, bool);
Please add parameter names.
Though trailing spaces are not forbidden, I would prefer not to have them.
...
> --- a/sql/backup/kernel.cc 2008-12-18 21:46:36 +0000
> +++ b/sql/backup/kernel.cc 2009-01-06 20:28:28 +0000
...
> @@ -209,7 +216,7 @@ execute_backup_command(THD *thd, LEX *le
> {
>
> Restore_info *info= context.prepare_for_restore(backupdir, lex->backup_dir,
> - thd->query);
> + thd->query, skip_gap_event);
Alignment seems broken.
...
> --- a/sql/sql_parse.cc 2008-12-04 23:14:30 +0000
> +++ b/sql/sql_parse.cc 2009-01-06 20:28:28 +0000
...
> @@ -2316,26 +2316,43 @@ mysql_execute_command(THD *thd)
...
> + int8 val= (int8)it->val_int();
> + /*
> + Check options.
> + */
> + switch (val) {
> + /* OVERWRITE option */
> + case 1:
> + overwrite_restore= true;
> + break;
> + /* SKIP GAP EVENT option */
> + case 2:
> + skip_gap_event= true;
> + break;
> + }
Wouldn't it be better to have symbolic names instead of numeric literals
for the options?
I don't see a good reason for reducing the value from longlong to int8.
Using the natural machine size of 'int' seems more appropriate.
Hopefully we won't have more than 255 options ever, but...
...
> --- a/sql/sql_yacc.yy 2008-12-04 23:14:30 +0000
> +++ b/sql/sql_yacc.yy 2009-01-06 20:28:28 +0000
...
> @@ -6435,7 +6436,7 @@ restore:
> }
> FROM
> TEXT_STRING_sys
> - opt_overwrite
> + opt_overwrite opt_skip_gap_event
This means that the options have to be given is a certain order. Is this
intentional?
> {
> Lex->backup_dir = $4;
> }
> @@ -6459,6 +6460,23 @@ opt_overwrite:
> lex->value_list.push_front(it);
> }
> ;
> +
> +opt_skip_gap_event:
> + /* empty */
> + {
> + LEX *lex= Lex;
> + Item *it= new Item_int((int8) 0);
> +
> + lex->value_list.push_back(it);
> + }
What is the advantage to add a zero option in case the SKIP_GAP_EVENT
option is not present? In mysql_execute_command() you ignore it anyway.
> + | SKIP_GAP_EVENT_SYM
> + {
> + LEX *lex= Lex;
> + Item *it= new Item_int((int8) 2);
> +
> + lex->value_list.push_back(it);
> + }
> + ;
...
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
