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

[Chuck Bell]

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

...

 > +*/ +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.

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

...

Chuck