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

[Chuck Bell]

Accepted. Patch approved. 

> -----Original Message-----
> From: Rafal.Somla@stripped [mailto:Rafal.Somla@stripped] 
> Sent: Tuesday, November 25, 2008 8:41 AM
> To: Chuck Bell
> Cc: commits@stripped
> Subject: Re: bzr commit into mysql-6.0-backup branch 
> (Rafal.Somla:2734) Bug#40262
> 
> 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
> 
>