Re: bk commit into 6.0 tree (rafal:1.2667) WL#4060

[Rafal Somla]

Chuk,

Thanks for a quick review. Due to complex merge situation it would be difficult 
for me to update my patches so that they include merge changes. Thus I propose 
the following compromise:

1. First I'll modify my patches to take into account all changes not related to 
the merge. I'll send replies to your review emails for each patch describing 
what I'm going to do.

2. Then I'll do the merge and send you a separate merge patch, which 
(unfortunately) will be incremental, i.e. to be applied over the previous patches.

Hope this is ok.

Here are replies for the first, rename patch.

Chuck Bell wrote:
> Rafal,
> 
> This patch is ok to push provided you merge to the latest clone and resolve
> the following issues (see earlier email with solutions):
> 
> 1) ensure meta_backup.cc and sql_backup.cc files get removed

I think leaving renamed files in place is a feature of bk. If I clone my 
repository, the old files are not present any more.

> 2) repair the data_backup.cc file to replace the ddl blocker and fkey code

I'll do that when I merge with current tree.

> 3) fix the Windows compilation errors/warnings in stream_v1.c 
> 

I'll do that in the patch for backup stream library (the second patch)

BTW. Could you do this experiment for me: remove any definitions/declarations of 
bzero() functions in stream_v1* files and replace them/it by

#define bzero(A,B)     memset((A),0,(B))

Then see if it will compile without a warning. Thanks!

> I sent you information about both of these in separate emails. Please
> remember that the block_DDL call in execute_backup_command() must be moved
> to before the instantiation of Backup_info class as shown:
> 
>     else
>     {
>       /*
>         Freeze all DDL operations by turning on DDL blocker.
>       */
>       if (!block_DDL(thd))
>         DBUG_RETURN(backup::ERROR);
> 
>       Backup_info info(thd);
> 
> Also, note the the fkey modifications need to be moved to kernel.cc. I have
> included diffs of my working compiled code for you to compare. Note: these
> are diffs from the mysql-6.0-backup clone with your original patches
> applied.
>

Ok, this is an issue to resolve at merge time. Thanks for the diffs!

> Chuck
> 
> 
> --- data_backup.cc      2007-11-28 11:48:25.328125000 -0500
> +++ ../../../data_backup.cc     2007-11-28 16:06:10.734375000 -0500
> @@ -471,25 +471,29 @@
>  {
>    DBUG_ENTER("backup::write_table_data");
> 
> -  if (info.snap_count==0 || info.table_count==0) // nothing to backup
> +  info.data_size= 0;
> +
> +  if (info.img_count==0 || info.table_count==0) // nothing to backup
>      DBUG_RETURN(0);
> 
>    Scheduler   sch(s,&info);         // scheduler instance
>    List<Scheduler::Pump>  inactive;  // list of images not yet being created
>    size_t      max_init_size=0;      // keeps maximal init size for images
> in in
> active list
> 
> +  size_t      start_bytes= s.bytes;
> +
>    DBUG_PRINT("backup/data",("initializing scheduler"));
> 
>    // add unknown "at end" drivers to scheduler, rest to inactive list
> 
> -  for (uint no=0; no < 256; ++no)
> +  for (uint no=0; no < info.img_count; ++no)
>    {
> -    Snapshot_info *i= info.m_snap[no];
> +    Image_info *i= info.images[no];
> 
>      if (!i)
>        continue;
> 
> -    Scheduler::Pump *p= new Scheduler::Pump(*i,s);
> +    Scheduler::Pump *p= new Scheduler::Pump(no,*i,s);
> 
>      if (!p || !p->is_valid())
>      {
> @@ -621,7 +625,9 @@
>      {
>        LOG_INFO li;
>        mysql_bin_log.get_current_log(&li);
> -      info.save_binlog_pos(li);
> +      info.binlog_information.position= li.pos;
> +      memcpy(info.binlog_information.binlog_file_name,
> +             li.log_file_name, strlen(li.log_file_name));
>        DBUG_PRINT("SYNC PHASE - binlog position : ", ("%d", (int) li.pos));
>        DBUG_PRINT("SYNC PHASE - binlog filename : ", ("%s",
> li.log_file_name));
>      }
> 
> 
> --- kernel.cc	2007-11-28 14:27:56.421875000 -0500
> +++ ../../../kernel.cc	2007-11-28 16:06:03.093750000 -0500
> @@ -26,7 +26,6 @@
>  #include "be_native.h"
>  #include "be_default.h"
>  #include "be_snapshot.h"
> -#include "ddl_blocker.h"
>  
>  namespace backup {
>  
> @@ -55,10 +54,6 @@
>  bool test_error_flag= FALSE;
>  #endif
>  
> -extern pthread_mutex_t THR_LOCK_DDL_blocker;
> -extern pthread_cond_t COND_backup_blocked;
> -extern pthread_cond_t COND_DDL_blocker;
> -
>  /*
>    (De)initialize memory allocator for backup stream library.
>   */
> @@ -107,7 +102,7 @@
>    case SQLCOM_SHOW_ARCHIVE:
>    case SQLCOM_RESTORE:
>    {
> -    backup::IStream *stream= open_for_read(*loc);
> +    IStream *stream= open_for_read(*loc);
>  
>      if (!stream)
>      {
> @@ -135,11 +130,7 @@
>        {
>          info.report_error(log_level::INFO,ER_BACKUP_RESTORE_START);
>  
> -        /*
> -          Freeze all DDL operations by turning on DDL blocker.
> -        */
> -//        if (!block_DDL(thd))
> -//          DBUG_RETURN(backup::ERROR);
> +        // TODO: freeze all DDL operations here
>  
>          info.save_errors();
>          info.restore_all_dbs();
> @@ -160,12 +151,7 @@
>            send_summary(thd,info);
>          }
>  
> -        /*
> -          Unfreeze all DDL operations by turning off DDL blocker.
> -        */
> -//        unblock_DDL();
> -        BACKUP_BREAKPOINT("DDL_unblocked");
> -
> +        // TODO: unfreeze DDL here
>        }
>      } // if (!stream)
>  
> @@ -173,12 +159,6 @@
>  
>     restore_error:
>  
> -    /*
> -      Unfreeze all DDL operations by turning off DDL blocker.
> -    */
> -//    unblock_DDL();
> -    BACKUP_BREAKPOINT("DDL_unblocked");
> -
>      res= res ? res : ERROR;
>  
>     finish_restore:
> @@ -202,12 +182,6 @@
>      }
>      else
>      {
> -       /*
> -        Freeze all DDL operations by turning on DDL blocker.
> -      */
> -      if (!block_DDL(thd))
> -        DBUG_RETURN(backup::ERROR);
> -
>        Backup_info info(thd);
>  
>        if (check_info(thd,info))
> @@ -215,6 +189,7 @@
>  
>        info.report_error(log_level::INFO,ER_BACKUP_BACKUP_START);
>  
> +      // TODO: freeze all DDL operations here
>  
>        /*
>          Save starting datetime of backup.
> @@ -260,23 +235,13 @@
>        */
>        save_current_time(info.end_time);
>  
> -      /*
> -        Unfreeze all DDL operations by turning off DDL blocker.
> -      */
> -      unblock_DDL();
> -
> -      BACKUP_BREAKPOINT("DDL_unblocked");
> -
> +      // TODO: unfreeze DDL here
>      } // if (!stream)
>  
>      goto finish_backup;
>  
>     backup_error:
>  
> -    /*
> -      Unfreeze all DDL operations by turning off DDL blocker.
> -    */
> -    unblock_DDL();
>      res= res ? res : ERROR;
>  
>     finish_backup:
> @@ -857,30 +822,6 @@
>    return res;
>  }
>  
> -/**
> -   Toggle foreign key constraints on and off.
> -
> -   @param THD thd          Current thread structure.
> -   @param my_bool turn_on  TRUE = turn on, FALSE = turn off.
> -
> -   @returns TRUE if foreign key contraints are turned on already
> -   @returns FALSE if foreign key contraints are turned off
> -  */
> -my_bool fkey_constr(THD *thd, my_bool turn_on)
> -{
> -  my_bool fk_status= FALSE;
> -
> -  DBUG_ENTER("mysql_restore");
> -  if (turn_on)
> -    thd->options&= ~OPTION_NO_FOREIGN_KEY_CHECKS;
> -  else
> -  {
> -    fk_status= (thd->options & OPTION_NO_FOREIGN_KEY_CHECKS)? FALSE : TRUE;
> -    thd->options|= OPTION_NO_FOREIGN_KEY_CHECKS;
> -  }
> -  DBUG_RETURN(fk_status);
> -}
> -
>  
>  /**
>    Add table to archive's list of meta-data items.
> @@ -1025,8 +966,6 @@
>  */
>  int mysql_restore(THD *thd, backup::Restore_info &info, backup::IStream &s)
>  {
> -  my_bool using_fkey_constr= FALSE;
> -
>    DBUG_ENTER("mysql_restore");
>  
>    using namespace backup;
> @@ -1035,16 +974,8 @@
>  
>    DBUG_PRINT("restore",("Restoring meta-data"));
>  
> -  /*
> -    Turn off foreign key constraints (if turned on)
> -  */
> -  using_fkey_constr= fkey_constr(thd, FALSE);
> -
>    if (read_meta_data(info, s) == ERROR)
> -  {
> -    fkey_constr(thd, using_fkey_constr);
> -    DBUG_RETURN(backup::ERROR);
> -  }
> +    DBUG_RETURN(ERROR);
>  
>    s.next_chunk();
>  
> @@ -1052,17 +983,8 @@
>  
>    // Here restore drivers are created to restore table data
>    if (restore_table_data(thd,info,s) == ERROR)
> -  {
> -    fkey_constr(thd, using_fkey_constr);
> -    DBUG_RETURN(backup::ERROR);
> -  }
> -
> -
> -  /*
> -    Turn on foreign key constraints (if previously turned on)
> -  */
> -  fkey_constr(thd, using_fkey_constr);
> - 
> +    DBUG_RETURN(ERROR);
> +
>    DBUG_PRINT("restore",("Done."));
>  
>    if (read_summary(info,s) == ERROR)
> @@ -1153,7 +1075,6 @@
>  
>  } // backup namespace
>  
> - 
>  /*************************************************
>  
>                 CATALOGUE SERVICES
> @@ -1790,15 +1711,6 @@
>    thd->net.vio= 0;
>    thd->net.no_send_error= 0;
>  
> -  /*
> -    @todo The following is a work around for online backup and the DDL
> blocker.
> -
> -          It should be removed when the generalized solution is in place.
> -          This is needed to ensure the restore (which uses DDL) is not
> blocked
> -          when the DDL blocker is engaged.
> -  */
> -  thd->DDL_exception= TRUE;
> -
>    thd->query=         query.c_ptr();
>    thd->query_length=  query.length();
>  
> @@ -1810,14 +1722,6 @@
>    const char *ptr;
>    ::mysql_parse(thd,thd->query,thd->query_length,&ptr);
>  
> -  /*
> -    @todo The following is a work around for online backup and the DDL
> blocker.
> -          It should be removed when the generalized solution is in place.
> -          This is needed to ensure the restore (which uses DDL) is not
> blocked
> -          when the DDL blocker is engaged.
> -  */
> -  thd->DDL_exception= FALSE;
> -
>    thd->net.vio= save_vio;
>  
>    if (thd->is_error())
> 
> 
> --- stream_v1.c 2007-11-28 15:06:34.046875000 -0500
> +++ ../../../stream_v1.c        2007-11-28 16:07:38.406250000 -0500
> @@ -5,7 +5,6 @@
>  #include "stream_v1.h"
>  #include "stream_v1_services.h"
> 
> -# define bzero(A,B)  memset((A),0,(B))
>  /**
>    @file
> 
> --- meta_data.cc        2007-11-28 12:21:18.578125000 -0500
> +++ ../../../meta_data.cc       2007-11-28 16:06:41.718750000 -0500
> @@ -74,7 +74,7 @@
>    if (res != OK)
>      return BSTREAM_ERROR;
> 
> -  stmt->begin= (backup::byte*)info->create_stmt_buf.ptr();
> +  stmt->begin= (byte*)info->create_stmt_buf.ptr();
>    stmt->end= stmt->begin + info->create_stmt_buf.length();
> 
>    return BSTREAM_OK;
> 
> 
>> -----Original Message-----
>> From: rsomla@stripped [mailto:rsomla@stripped] 
>> Sent: Wednesday, November 21, 2007 5:19 AM
>> To: commits@stripped
>> Subject: bk commit into 6.0 tree (rafal:1.2667) WL#4060
>>
>> Below is the list of changes that have just been committed 
>> into a local
>> 6.0 repository of rafal. When rafal does a push these changes will
>> be propagated to the main repository and, within 24 hours after the
>> push, to the public repository.
>> For information on how to access the public repository
>> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>>
>> ChangeSet@stripped, 2007-11-21 11:18:56+01:00, rafal@quant.(none) +11 -0
>>   WL#4060 (backup kernel updates for beta release)
>>   
>>   File renames in preparation for the kernel updates:
>>   
>>   archive.{h,cc}     -> catalog.{h,cc}
>>   meta_backup.{h,cc} -> meta_data.{h,cc}
>>   sql_backup.{h,cc}  -> kernel.{h,cc}
>>   
>>   The patch also introduces uniform spacing in the makefile.
>>
>