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

[Chuck Bell]

Rafal, 

Replies to your replies.... ;)

> > REQUESTS
> > --------
> > 1. Correct spelling error.
> > 2. Validity point for when backup contains no data may not 
> be correct.
> > 3. The new error introduced is not covered in the test.

...

> >  > +  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

...

> > [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.

...

> >  > === 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.

Chuck