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

REQUESTS
--------
1. Correct spelling error.
2. Validity point for when backup contains no data may not be correct.

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);
> +  }
>  

...

Chuck