List:Commits« Previous MessageNext Message »
From:Chuck Bell Date:April 1 2008 1:37am
Subject:RE: bk commit into 6.0 tree (rafal:1.2606) BUG#34721
View as plain text  
Rafal,

Ok, I understand now. Patch approved. 

Chuck 

> -----Original Message-----
> From: Rafal Somla [mailto:rsomla@stripped] 
> Sent: Monday, March 31, 2008 9:44 AM
> To: Chuck Bell
> Cc: commits@stripped; 'Ingo Strüwing'
> Subject: Re: bk commit into 6.0 tree (rafal:1.2606) BUG#34721
> 
> Chuck,
> 
> Chuck Bell wrote:
> > Rafal,
> > 
> > The patch looks good. The test is exactly what I was 
> looking for except...
> > 
> > I think we should change the use of MyISAM for this test. 
> The MyISAM 
> > native driver is a certainty (regardless of timing). 
> However, if you 
> > used the memory engine it is unlikely that we would write a 
> native engine for it.
> > Thus we would not have to change anything once this patch is pushed.
> > 
> 
> I've chosen MyISAM exactly because it will have a native 
> driver soon. The reason is that only for a storage engine 
> with native backup, there will be a difference in the choice 
> of backup driver between the normal operation and when the 
> test code is injected. That is,
> 
> 1. In normal operation (without injection) a native backup 
> driver will be used.
> 
> 2. With injected code, MyISAM will not create the native 
> backup engine and therefore the default backup driver will be used.
> 
> Note that if a storage engine has no native backup, then in 
> both cases (with and without code injection) the default (or 
> CS) backup driver will be used. This is the case now.
> 
> Thus the test case is more a clear-cut if the engine used 
> *has* native backup.
> 
> Rafal
> 
> > Chuck
> > 
> >> -----Original Message-----
> >> From: Rafal Somla [mailto:rsomla@stripped]
> >> Sent: Monday, March 31, 2008 7:32 AM
> >> To: Chuck Bell
> >> Cc: commits@stripped; Ingo Strüwing
> >> Subject: Re: bk commit into 6.0 tree (rafal:1.2606) BUG#34721
> >>
> >> Hi Chuck,
> >>
> >> I added a test case for testing the backup engine selection logic. 
> >> The new patch is <http://lists.mysql.com/commits/44675>
> >>
> >> Rafal
> >>
> >> Chuck Bell wrote:
> >>> Rafal,
> >>>
> >>> I think we need to add a test for this using 
> DBUG_EXECUTE_IF(). It 
> >>> will help us detect problems when new native drivers are
> >> added and/or
> >>> some collateral errors occur based changes to the server or
> >> backup code.
> >>> Chuck
> >>>
> >>>> -----Original Message-----
> >>>> From: rsomla@stripped [mailto:rsomla@stripped]
> >>>> Sent: Tuesday, March 25, 2008 10:22 AM
> >>>> To: commits@stripped
> >>>> Subject: bk commit into 6.0 tree (rafal:1.2606) BUG#34721
> >>>>
> >>>> 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, 2008-03-25 15:22:11+01:00,
> >> rafal@quant.(none) +2 -0
> >>>>   BUG#34721 (Backup driver can't refuse to provide driver):
> >>>>   
> >>>>   This patch modifies backup kernel so that it falls back
> >> to built in
> >>>> backup
> >>>>   engines in case a native backup engine can not be created. 
> >>>>   
> >>>>   This is for the special case when storage engine defines a
> >>>> get_backup_engine()
> >>>>   factory function (inside the handlerton), but that
> >> function fails
> >>>> to create a
> >>>>   backup engine instance. Previous code haven't dealt
> >> correctly with
> >>>> that case.
> >>>>   Now it puts a warning and tries the built-in engines.
> >>>>
> >>>>   sql/backup/backup_info.cc@stripped, 2008-03-25 15:22:04+01:00,
> >>>> rafal@quant.(none) +10 -9
> >>>>     In case a native backup engine has been selected for a given 
> >>>> table, check if
> >>>>     created Native_snapshot object is valid (in particular, has 
> >>>> successfully created
> >>>>     the engine). Only then use it for that table,
> >> otherwise try other
> >>>> backup engines
> >>>>     including the built-in ones.
> >>>>
> >>>>   sql/backup/be_native.h@stripped, 2008-03-25 15:22:05+01:00,
> >>>> rafal@quant.(none) +1 -1
> >>>>     If the get_backup_engine() factory function defined by
> >> a storage
> >>>> engine failes
> >>>>     to create a native backup engine don't treat it as a
> >>>> (fatal) error. In that case 
> >>>>     we will try to use default, built-in backup engines. 
> Thus only 
> >>>> warning is printed.
> >>>>
> >>>> diff -Nrup a/sql/backup/backup_info.cc 
> b/sql/backup/backup_info.cc
> >>>> --- a/sql/backup/backup_info.cc	2008-03-21 09:43:43 +01:00
> >>>> +++ b/sql/backup/backup_info.cc	2008-03-25 15:22:04 +01:00
> >>>> @@ -83,16 +83,17 @@ Backup_info::find_backup_engine(const ba
> >>>>      {
> >>>>        Native_snapshot *nsnap= new Native_snapshot(m_ctx, se);
> >>>>        DBUG_ASSERT(nsnap);
> >>>> -      snapshots.push_front(nsnap);
> >>>> -      native_snapshots.insert(se, nsnap);
> >>>>  
> >>>> -      /*
> >>>> -        Question: Can native snapshot for a given storage 
> >>>> engine not accept
> >>>> -        a table using that engine? If yes, then what to do 
> >>>> in that case - error 
> >>>> -        or try other (default) snapshots?
> >>>> -       */     
> >>>> -      DBUG_ASSERT(nsnap->accept(tbl, se));
> >>>> -      snap= nsnap;
> >>>> +      if (nsnap->is_valid())
> >>>> +      {
> >>>> +        snapshots.push_front(nsnap);
> >>>> +        native_snapshots.insert(se, nsnap);
> >>>> +
> >>>> +        if (nsnap->accept(tbl, se))        
> >>>> +          snap= nsnap;
> >>>> +      }
> >>>> +      else
> >>>> +        delete nsnap;
> >>>>      }
> >>>>    
> >>>>    /*
> >>>> diff -Nrup a/sql/backup/be_native.h b/sql/backup/be_native.h
> >>>> --- a/sql/backup/be_native.h	2008-03-04 17:06:22 +01:00
> >>>> +++ b/sql/backup/be_native.h	2008-03-25 15:22:05 +01:00
> >>>> @@ -97,7 +97,7 @@ int Native_snapshot::init(Logger &log, c
> >>>>      if (m_be)
> >>>>        m_be->free();
> >>>>      m_be= NULL;
> >>>> -    log.report_error(ER_BACKUP_CREATE_BE, m_name);
> >>>> +    log.report_error(log_level::WARNING,
> >>>> ER_BACKUP_CREATE_BE, m_name);
> >>>>      return 1;
> >>>>    }
> >>>>    
> >>>>
> >>>> --
> >>>> MySQL Code Commits Mailing List
> >>>> For list archives: http://lists.mysql.com/commits
> >>>> To unsubscribe:    
> >>>> http://lists.mysql.com/commits?unsub=1
> >>>>
> >>>
> > 
> 
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    
> http://lists.mysql.com/commits?unsub=1
> 

Thread
bk commit into 6.0 tree (rafal:1.2606) BUG#34721rsomla25 Mar
  • RE: bk commit into 6.0 tree (rafal:1.2606) BUG#34721Chuck Bell26 Mar
    • Re: bk commit into 6.0 tree (rafal:1.2606) BUG#34721Rafal Somla31 Mar
      • RE: bk commit into 6.0 tree (rafal:1.2606) BUG#34721Chuck Bell31 Mar
        • Re: bk commit into 6.0 tree (rafal:1.2606) BUG#34721Rafal Somla31 Mar
          • RE: bk commit into 6.0 tree (rafal:1.2606) BUG#34721Chuck Bell1 Apr