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

[Rafal Somla]

Hi Chuck,

Thanks for review. See my explanations below.

Chuck Bell wrote:
> STATUS
> ------
> Patch ok, but approval pending resolution of requests (#2 specifically).
> 
> Note: disregard last email. Hit send too early. ;)
> 
> 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.
> 
> COMMENTARY
> ----------
> Good job.
> 
> SUGGESTIONS
> -----------
> None.
> 
> DETAILS
> -------
> 
>  >  2734 Rafal Somla    2008-11-17
>  >       Bug #40262  - Binary log information incorrect in backup of no 
> data with binlog                     turned on
>  >             This is the second and last patch which fixes the main 
> problem of reporting VP       info also in the case there are no tables 
> to be backed-up.             It also fixes an issue with incorrect 
> parsing of backup image upon restore, if       there are no table data 
> stored in it.
> 
> ...
> 
>  > +/**
>  > +  Log validity point information.
>  > +
>  > +  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.

> ...
> 
>  > +*/ +static
>  > +void report_vp_info(Backup_info &info)
>  > +{
>  > +  info.m_ctx.report_vp_time(info.get_vp_time(), TRUE); // TRUE = 
> also write to progress log
>  > +  if (info.flags & BSTREAM_FLAG_BINLOG)
>  > +    info.m_ctx.report_binlog_pos(info.binlog_pos);
>  > +  if (info.master_pos.pos)
>  > +    info.m_ctx.report_master_binlog_pos(info.master_pos);
>  > +}
>  > +
>  > +
>  > +/**
>  >    Save data from tables being backed up.
>  >
>  >    Function initializes and controls backup drivers which create the 
> image
>  > @@ -424,8 +486,21 @@ int write_table_data(THD* thd, Backup_in
>  >  {
>  >    DBUG_ENTER("backup::write_table_data");
>  >
>  > +  /*
>  > +    If there no tables to backup, there is nothing to do in this 
> function
>  > +    except for storing and reporting the validity point info.
>  > +    +    Note that since backup image contains no table data, any 
> time is a good
>  > +    validity time -- there is no issue of synchronizing the data 
> stored in +    the image with the data in the rest of the server.
>  > +  */
> 
> [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.

>  >    if (info.snap_count() == 0 || info.table_count() == 0) // nothing 
> to backup
>  > -    DBUG_RETURN(0);
>  > +  {
>  > +    int res= save_vp_info(info);    // logs errors
>  > +    if (!res)
>  > +      report_vp_info(info);
>  > +    DBUG_RETURN(res);
>  > +  }
>  >
> 
> ...
> 
>  > === 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.

Rafal