Hi Ingo,
Sorry this took a while. I wanted to get my beta bugs working first.
Here are my replies to your comments. Let me know if you have more comments.
> 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
...
>> --- 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".
Ok, will make these improvements.
> ...
>> --- 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
Eeek gag! *blush*
>
>> +# 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.
Ok. Yes, it shall be done.
>
>> +
>> +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?
No, it is just a choice of mine to help readability.
>> +
>> +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?
No assurance beyond the original setting of the backup_id using debug
insertion. I will add a note to warn developers of this.
>> +
>> +--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?
Yes. It is in there @ line # 225 of the original test file (without the
patch).
>
>> +#
>> # 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.
I suppose it would. I will consider doing so.
>
> ...
>> --- 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.
Ok, will do.
>
> ...
>> --- 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.
Will fix.
>
> ...
>> --- 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...
Just following direction from the replication guys. I think the reason
is because the code is split between master and slave and it is just
easier to code and debug this way. If you insist, I will ask the
replication gurus for their opinions/recommendations.
>
> ...
>> --- 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?
Yes, it is. Do you feel strongly it should be orderless? I think that
could be much more work.
>
>> {
>> 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.
This is done so that in case something goes wrong it will not trigger
the opt_overwrite option by mistake.
Chuck
