Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2734) Bug#40262

[Rafal Somla]

Hi Chuck,

See my replies below and tell me if you still think this patch does not qualify 
for pushing.

Chuck Bell wrote:
...
>>>  > +  Information such as validity point time is logged 
>> using backup 
>>> logger which,  > +  in particular, writes it to backup history and 
>>> progress logs.
>>>  > +  +  @note Logging the information may involve time 
>> consuming I/O. 
>>> Therefore this
>>>  > +  function should not be called in the time critical 
>>> synchronization phase, but  > +  after the synchronisation has been 
>>> done.
>>>
>>> [1] Spelling error in comments: synchronisation
>>>
>> I don't think it is a spelling error - see here 
>> <http://www.merriam-webster.com/dictionary/synchronization>. 
>> Perhaps it is British vs. American spelling issue. I'll 
>> change it as you suggest.
> 
> I am aware of that, yes. However, in this instance it was a case of pick one
> -- you spelled it both ways in the same sentence. :P
>

Oh dear! Now I see it :/

> ...
> 
>>> [2] I am not certain of this. I think it does matter what 
>> time is used 
>>> and it should be accurate. Consider a backup done when tables are 
>>> empty, some data is added, then the system crashes. User 
>> wants to do 
>>> PTR but if the VP is "any good time" then it won't be 
>> accurate. Please 
>>> convince me that we are still setting the VP correctly in this 
>>> scenario and if so, remove the comment that seems to 
>> indicate that we 
>>> are arbitrarily choosing a VP time.
>>>
>> Please note the difference between these two situations:
>>
>> 1. We backup some tables but these tables are empty.
>> 2. We do not backup any tables.
>>
>> In the first case you are right, VP must be constructed 
>> carefully using the synchronization protocol and this is what 
>> will happen, because in that case
>> info.table_count() != 0 and the branch below will not be entered.
>>
>> However, in the second case VP time can be chosen arbitrarily 
>> because there are no tables whose data could be not correctly 
>> aligned with the VP. Note that a backup image which contains 
>> no tables can not be used for PTR. Such image basically 
>> stores only metadata which will be used to create global 
>> objects but not to restore any table data and thus not to 
>> roll forward from the restored data.
>>
>> Thus in case 2 VP can be chosen arbitrarily and this is what 
>> we are doing. I think a comment is in place because readers 
>> of the code may ask themselves the same question you asked.
> 
> I don't think this works either because if we have a backup that is backing
> up 2 databases, one with tables and one without, the VP must be accurate.
> Are you suggesting that this case '2' will occur only if we backup a single
> database with no tables? I don't think we should do anything arbitrarily no
> matter what the circumstances.
> 

The logic of the new code is like this:

   if (info.snap_count() == 0 || info.table_count() == 0) // nothing to backup
   {
     int res= save_vp_info(info);    // logs errors
     if (!res)
       report_vp_info(info);
     DBUG_RETURN(res);
   }

Regardless of the number of databases, if there is even a single table in one of 
them, then info.table_count() > 0 and we will not enter this branch (i.e., the 
normal synchronization and VP creation protocol will be followed). But if 
info.table_count() == 0, then there are no tables at all, no backup drivers are 
involved and we are in case '2' above. Then my argument applies and any time 
(while DDLs are blocked) is a correct validity point. Do you disagree?

> ...
> 
>>>  > === modified file 'sql/share/errmsg.txt'
>>>  > --- a/sql/share/errmsg.txt    2008-11-05 15:07:40 +0000
>>>  > +++ b/sql/share/errmsg.txt    2008-11-17 14:43:10 +0000
>>>  > @@ -6414,3 +6414,5 @@ ER_RESTORE_ON_SLAVE
>>>  >    eng "A restore operation was attempted on a slave during 
>>> replication. You must stop the slave prior to running a restore."
>>>  >  ER_NONUNIQ_DB 42000 S1009
>>>  >          eng "Not unique database: '%-.192s'"
>>>  > +ER_BACKUP_UNEXPECTED_DATA
>>>  > +  eng "Backup image contains no tables, but table data 
>> was found in it"
>>> This error is not covered by the test. One of the issues I've 
>>> uncovered in the WL#4578 work is that about 30-40% of all our error 
>>> conditions are not covered in the tests. Please add this if 
>> it doesn't 
>>> require super human efforts or create a new bug report to 
>> analyze how 
>>> to add it to the test.
>>>
>> It would not be easy to test it. This would require editing 
>> backup image to make it inconsistent and we don't have an 
>> infrastructure for doing this yet.
>>
>> As you say, about 30-40% of error branches in our code are 
>> not tested. Instead of doing uncoordinated and ad-hoc actions 
>> such as reporting bugs about isolated cases I suggest to 
>> create a general testing plan, part of which should be 
>> testing code coverage and this, in particular, should ensure 
>> that all error situations are covered.
> 
> I anticipated that would be the case. Can you please open a bug report for
> this test? That will satify my concerns.
> 

I've reported BUG#41012.

Rafal