Re: bk commit into 6.0 tree (rafal:1.2606) BUG#34721

[Ingo Strüwing]

Hi Rafal,

rsomla@stripped, 31.03.2008 13:28:
...
> ChangeSet@stripped, 2008-03-31 13:28:32+02:00, rafal@quant.(none) +6 -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.

Formally a changeset comment should say 1. what was the problem, and 2.
how was it fixed. The latter is well explained. So I'd appreciate if you
prepend a sentence saying what the problem was. The bug synopsis is too
short IMHO.

...
>   sql/backup/backup_info.cc@stripped, 2008-03-31 13:28:29+02:00, rafal@quant.(none) +44
> -9
>     Debug code injected into has_native_backup() function which sets MyISAM backup 
>     factory function to a dummy one. This is used to test the backup engine
> selection
>     logic.
>     
>     In find_backup_engine: 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.

Great comment. However I would prefer to have the explanation, how it
works, in a code comment. IMHO changeset file comments do just need to
say _what_ was changed, which helps when resolving merge conflicts.

> 
>   sql/backup/be_native.h@stripped, 2008-03-31 13:28:29+02:00, rafal@quant.(none) +1 -1
>     If the get_backup_engine() factory function defined by a storage engine failes 

Typo: failes -> fails

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

Again, here I would like to see something like "Changed
ER_BACKUP_CREATE_BE to report as a warning". And the above comment moved
into the code.

...
> +++ b/mysql-test/t/backup_no_be-master.opt	2008-03-31 13:28:29 +02:00
> @@ -0,0 +1 @@
> +--debug=d,backup_test_dummy_be_factory

To try to get rid of a server restart and thus this option file, can't
we remember the old value of hton->get_backup_engine and restore it
after doing the related checks?

...
> +++ b/sql/backup/backup_info.cc	2008-03-31 13:28:29 +02:00
...
>  /// Determine if a given storage engine has native backup support.
>  static
>  bool has_native_backup(storage_engine_ref se)

Please fix the format of the function comment.

>  {
>    handlerton *hton= se_hton(se);
>  
> +  /*
> +    The code below is used to test the backup engine selection logic. 
...
> +    dummy backup engine factory which never creates any engines.
> +   */ 

Please remove the trailing space from the */ line and indent it
correctly (one space less).

> +  DBUG_EXECUTE_IF("backup_test_dummy_be_factory", 
> +    if (hton == myisam_hton) hton->get_backup_engine=
> dummy_backup_engine_factory;);

If you find (or create) some temporary storage in 'se', you could save
the current value of hton->get_backup_engine so that you can restore it
later. Then we could avoid a server restart.

> +
>    return hton && hton->get_backup_engine;
>  }
>  
> @@ -83,16 +117,17 @@ Backup_info::find_backup_engine(const ba
>      {
>        Native_snapshot *nsnap= new Native_snapshot(m_ctx, se);
>        DBUG_ASSERT(nsnap);

Not part of your patch, but as it pops up in the changeset: This is ok
in a prototype. But before push to main the assert must be changed to
error handling.

...
> +      if (nsnap->is_valid())
> +      {
> +        snapshots.push_front(nsnap);
> +        native_snapshots.insert(se, nsnap);
> +
> +        if (nsnap->accept(tbl, se))        
> +          snap= nsnap;
> +      }

Shouldn't you delete nsnap if accept() returns FALSE? You do it only if
is_valid() returns FALSE.

> +      else
> +        delete nsnap;
>      }

Here you could restore the myisam get_backup_engine pointer if you saved
it above.

...

Otherwise approved if you agree with my above change requests and
considered my suggestions.

Regards
Ingo
-- 
Ingo Strüwing, Senior Software Developer
MySQL GmbH, Dachauer Str. 37, D-80335 München
Geschäftsführer: Kaj Arnö - HRB München 162140