Re: bk commit into 6.0 tree (cbell:1.2784) BUG#33355

[Rafal Somla]

Chuck Bell wrote:
> Rafal,
> 
> The 'goto error' code was already in my patch. Perhaps you were looking at
> an earlier patch...I have recommitted the patch for you. See this one:
> 
> http://lists.mysql.com/commits/41711
>

Right, it is there... Then the patch is good to push.

Rafal

> Chuck 
> 
>> -----Original Message-----
>> From: Rafal Somla [mailto:rsomla@stripped] 
>> Sent: Tuesday, February 05, 2008 6:09 AM
>> To: cbell@stripped
>> Cc: commits@stripped
>> Subject: Re: bk commit into 6.0 tree (cbell:1.2784) BUG#33355
>>
>> Chuck,
>>
>> I had a quick look at your preliminary patch. All looks ok 
>> except for one small thing - see below.
>>
>> cbell@stripped wrote:
>>> 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-23 17:51:19-05:00, 
>> cbell@mysql_cab_desk. +4 -0
>>>   BUG#33355 : Backup: hang if database is mysql
>>>   
>>>   Code now warns user if mysql or information_schema is found in
>>>   the database list. Code throws error if database list is empty.
>>>
>>>   mysql-test/r/backup_errors.result@stripped, 2008-01-23 
>> 17:51:11-05:00, cbell@mysql_cab_desk. +21 -0
>>>     BUG#33355 : Backup: hang if database is mysql
>>>     
>>>     New result file.
>>>
>>>   mysql-test/t/backup_errors.test@stripped, 2008-01-23 
>> 17:51:12-05:00, cbell@mysql_cab_desk. +66 -0
>>>     BUG#33355 : Backup: hang if database is mysql
>>>     
>>>     Updated test.
>>>
>>>   sql/backup/kernel.cc@stripped, 2008-01-23 17:51:12-05:00, 
>> cbell@mysql_cab_desk. +19 -2
>>>     BUG#33355 : Backup: hang if database is mysql
>>>     
>>>     Added code to send error if no databases to backup.
>>>     Added code to capture and warn if mysql or information_schema
>>>     included in database list.
>>>
>>>   sql/share/errmsg.txt@stripped, 2008-01-23 17:51:12-05:00, 
>> cbell@mysql_cab_desk. +4 -0
>>>     BUG#33355 : Backup: hang if database is mysql
>>>     
>>>     New error/warning messages.
>>>
>>> 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-23 
>> 17:51:11 -05:00
>>> @@ -22,3 +22,24 @@ BACKUP DATABASE test,foo,bdb,bar TO 'tes  ERROR 
>>> 42000: Unknown database 'foo,bar'
>>>  DROP DATABASE adb;
>>>  DROP DATABASE bdb;
>>> +Backup of mysql, information_schema scenario 1 BACKUP 
>> DATABASE mysql 
>>> +TO 't.bak'; ERROR HY000: Nothing to backup Backup of mysql, 
>>> +information_schema scenario 2 BACKUP DATABASE 
>> information_schema TO 
>>> +'t.bak'; ERROR HY000: Nothing to backup Backup of mysql, 
>>> +information_schema scenario 3 BACKUP DATABASE mysql, 
>>> +information_schema TO 't.bak'; ERROR HY000: Nothing to 
>> backup Backup 
>>> +of mysql, information_schema scenario 4 BACKUP DATABASE 
>> mysql, test 
>>> +TO 't.bak'; backup_id # Backup of mysql, 
>> information_schema scenario 
>>> +5 BACKUP DATABASE information_schema, test TO 't.bak'; backup_id # 
>>> +Backup of mysql, information_schema scenario 6 BACKUP 
>> DATABASE mysql, 
>>> +information_schema, test TO 't.bak'; backup_id #
>>> 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-23 
>> 17:51:12 -05:00
>>> @@ -57,3 +57,69 @@ 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
>>> +
>>> +# Check error conditions for including mysql and 
>> information_schema 
>>> +in # the list of databases.
>>> +#
>>> +# There are several scenarios:
>>> +# 1) BACKUP DATABASE mysql TO 't.bak'
>>> +#    Error: Nothing to backup
>>> +# 2) BACKUP DATABASE information_schema TO 't.bak'
>>> +#    Error: Nothing to backup
>>> +# 3) BACKUP DATABASE mysql, information_schema TO 't.bak'
>>> +#    Error: Nothing to backup
>>> +# 4) BACKUP DATABASE mysql, test TO 't.bak'
>>> +#    Warning: Backup cannot include database 'mysql'
>>> +# 5) BACKUP DATABASE information_schema, test TO 't.bak'
>>> +#    Warning: Backup cannot include database 'information_schema'
>>> +# 6) BACKUP DATABASE mysql, information_schema, test TO 't.bak'
>>> +#    Warning: Backup cannot include database 'mysql'
>>> +#    Warning: Backup cannot include database 'mysql'
>>> +
>>> +# Scenario 1
>>> +--echo Backup of mysql, information_schema scenario 1 --error 
>>> +ER_BACKUP_NOTHING_TO_BACKUP BACKUP DATABASE mysql TO 't.bak';
>>> +
>>> +--error 0, 1
>>> +--remove_file $MYSQLTEST_VARDIR/master-data/t.bak
>>> +
>>> +# Scenario 2
>>> +--echo Backup of mysql, information_schema scenario 2 --error 
>>> +ER_BACKUP_NOTHING_TO_BACKUP BACKUP DATABASE information_schema TO 
>>> +'t.bak';
>>> +
>>> +--error 0, 1
>>> +--remove_file $MYSQLTEST_VARDIR/master-data/t.bak
>>> +
>>> +# Scenario 3
>>> +--echo Backup of mysql, information_schema scenario 3 --error 
>>> +ER_BACKUP_NOTHING_TO_BACKUP BACKUP DATABASE mysql, 
>> information_schema 
>>> +TO 't.bak';
>>> +
>>> +--error 0, 1
>>> +--remove_file $MYSQLTEST_VARDIR/master-data/t.bak
>>> +
>>> +# Scenario 4
>>> +--echo Backup of mysql, information_schema scenario 4 
>>> +--replace_column 1 # BACKUP DATABASE mysql, test TO 't.bak';
>>> +
>>> +--error 0, 1
>>> +--remove_file $MYSQLTEST_VARDIR/master-data/t.bak
>>> +
>>> +# Scenario 5
>>> +--echo Backup of mysql, information_schema scenario 5 
>>> +--replace_column 1 # BACKUP DATABASE information_schema, test TO 
>>> +'t.bak';
>>> +
>>> +--error 0, 1
>>> +--remove_file $MYSQLTEST_VARDIR/master-data/t.bak
>>> +
>>> +# Scenario 6
>>> +--echo Backup of mysql, information_schema scenario 6 
>>> +--replace_column 1 # BACKUP DATABASE mysql, 
>> information_schema, test 
>>> +TO 't.bak';
>>> +
>>> +--error 0, 1
>>> +--remove_file $MYSQLTEST_VARDIR/master-data/t.bak
>>> 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-23 17:51:12 -05:00
>>> @@ -295,7 +295,16 @@ execute_backup_command(THD *thd, LEX *le
>>>        else
>>>        {
>>>          info.write_message(log_level::INFO,"Backing up 
>> selected databases");
>>> -        info.add_dbs(lex->db_list); // backup databases 
>> specified by user
>>> +        res= info.add_dbs(lex->db_list); // backup 
>> databases specified by user
>>> +      }
>>> +      if ((info.db_count() == 0) && (res != ERROR))
>>> +      {
>>> +        res= ERROR;
>>> +        info.report_error(log_level::ERROR, 
>> ER_BACKUP_NOTHING_TO_BACKUP, MYF(0));
>>> +        stop= my_time(0); 
>>> +        info.save_end_time(stop);
>>> +        report_errors(thd, info, ER_BACKUP_BACKUP);
>>> +        goto backup_error;
>>>        }
>>>  
>>>        report_ob_num_objects(backup_prog_id, info.table_count); @@ 
>>> -697,7 +706,15 @@ int Backup_info::add_dbs(List< ::LEX_STR
>>>      String db_name(*s);
>>>      Db_ref db(db_name);
>>>  
>>> -    if (db_exists(db))
>>> +    if (((my_strcasecmp(system_charset_info,
>>> +      db_name.c_ptr(), "information_schema") == 0)) ||
>>> +      (my_strcasecmp(system_charset_info,
>>> +      db_name.c_ptr(), "mysql") == 0))
>>> +    {
>>> +      report_error(log_level::WARNING, ER_BACKUP_CANNOT_INCLUDE_DB,
>>> +                   db_name.c_ptr());
>>> +    }
>> I think the loop should be broken with "goto error" here.
>>
>>> +    else if (db_exists(db))
>>>      {
>>>        if (!unknown_dbs.is_empty()) // we just compose 
>> unknown_dbs list
>>>          continue;
>>> 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-23 17:51:12 -05:00
>>> @@ -6114,6 +6114,10 @@ ER_BACKUP_RESTORE_START  
>> ER_BACKUP_RESTORE_DONE
>>>          eng "Restore completed"
>>>  
>>> +ER_BACKUP_NOTHING_TO_BACKUP
>>> +        eng "Nothing to backup"
>>> +ER_BACKUP_CANNOT_INCLUDE_DB
>>> +        eng "Database '%-.64s' cannot be included in a 
>> backup and will be skipped"
>>>  ER_BACKUP_BACKUP
>>>          eng "Error during backup operation - server's 
>> error log contains more information about the error"
>>>  ER_BACKUP_RESTORE
>>>
>>>
>> Rafal
> 
>