List:Commits« Previous MessageNext Message »
From:Charles Bell Date:December 7 2009 4:53pm
Subject:Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)
Bug#43596
View as plain text  
STATUS
------
Not approved.

REQUIRED
--------
  6. Add falcon test (test with tablespaces).

REQUESTS
--------
  1. Please use convention for bug reference.
  2. Fix spelling errors.
  4. Please rename error messages.
  5. Please use convention for avoiding multiple statements on one line.
  7. Please add TODO here.
  8. Is this assertion no longer needed?
  9. Please explain this code.

SUGGESTIONS
-----------
  3. Would it be better to have a lctn suite?
10. Please consider these code changes.

COMMENTARY
----------
I was inclined to accept the patch but I feel the lack of a special test 
for tablespaces warrants another patch for review.

I have made a number of comments on things unrelated to the patch, its 
quality, or acceptance criteria as I understand them and my role as 
reviewer. However, I would appreciate if any replies (or rebuttal) of 
these comments were addressed at some point but I present them without 
any expectations of such.

DETAILS
-------
>       Bug #43596 Restore fails on case ins. server for databases who's names differ 
>                  in case only.

[1] Please use "BUG#XXXXX"

>       
>       On a case sensitive system such as linux, it is possible to create two 
>       objects whose names differ in letter case only. This is true for databases, 
>       tables, views and triggers. When such objects are backed up and then 
>       restored on a case insensitive system, they are treated as a single object 
>       which is incorrect and leads to many problems.
>       
>       This patch modifies RESTORE so that it aborts operation if two objects with 
>       equivalent names are found in the backup image. Each time a new object is 
>       added to the catalogue (in bcat_add_item() function) a check is made for 

[2] In the US we would use catalog. I see in the code both spellings are 
used. I think we should change it to catalog both in this patch 
(repeated several times) and in the code. If you don't want to do the 
global change in this patch (I understand) we can open a new bug to 
correct the problem.

>       possible duplicates. This check is implemented in Image_info::add_X() 
>       methods.
>       
>       For the duplicates check, a new obs::Obj::is_my_name() method is used. It 
>       checks if given name refers to the given object. The semantics of this 
>       method depends on the system on which it is run (case sensitive vs. case 
>       insensitive) and server options setting (such as --lower-case-table-names).
>       
>       To use si_objects for handling object name issues, the code had to be 
>       rearranged considerably. Basically, obs::Obj instance is created early in 
>       the restore process, in bcat_add_item() function, when image catalogue is 
>       read. Then this obs::Obj instance is used to refer to the object instead of 
>       object's name. A pointer to the instance is stored in each catalogue entry 
>       upon that entry creation (before, pointer was set later, when the 
>       serialization string of the object was available).
>       
>       Since we want to create obs::Obj instances before serialization data is 
>       available, the obs::get_X() functions are changed so that they can create 
>       instances when only object name is known. Later, these instances will be 
>       updated with the information from serialization string using new 
>       obs::Obj::init_from_image() method. After that, object can be created with 
>       obs::Obj::create() method as before.
>       
>       The patch also contains some clean-up in related code areas (see per-file 
>       comments).
>      @ mysql-test/suite/backup/include/test_for_error.inc
>         Mask possibly non-deterministic result here instead of doing it in the code.
>      @ mysql-test/suite/backup/std_data/cs_names_d.bkp
>         Image with clashing database names.
>      @ mysql-test/suite/backup/std_data/cs_names_r.bkp
>         Image with clashing trigger names.
>      @ mysql-test/suite/backup/std_data/cs_names_t.bkp
>         Image with clashing table names.
>      @ mysql-test/suite/backup/std_data/cs_names_v.bkp
>         Image with clashing view names.
>      @ mysql-test/suite/backup/t/backup_cs_names.test
>         Test script which creates backup images with clashing object names, to be
>         used by backup_cs_names_lctn? tests.
>      @ mysql-test/suite/backup/t/backup_cs_names_lctn1.test
>         Test restore of clashing object names on lctn=1 system.
>      @ mysql-test/suite/backup/t/backup_cs_names_lctn2.test
>         Test restore of clashing object names on lctn=2 system.
>      @ sql/backup/api_types.h
>         Add Db_ref::describe() method, analogous to Table_ref::describe().
>      @ sql/backup/backup_info.cc
>         - Adapt code to the fact that now add_X() methods expect obs::Obj instance
> as
>           a parameter.
>         - Has_db() is replaced by find_db().
>         - Fix possible memory leak in error inserted code - delete iterators which
> have
>           been created.
>         - Error injection point "ER_BACKUP_CATALOG_ADD_TABLE" is moved to 
>           Image_info::add_table(), so that table is *not* added to the catalogue when
> 
>           error is simulated.
>         - Move code setting coordinates of trigger's table to 
>           Backup_info::add_db_item() because this code should execute only for backup
> 
>           operation.
>         - Remove unnecessary dynamic string allocation.
>      @ sql/backup/image_info.cc
>         - When Image_info::Obj instance is created, it gets a pointer to obs::Obj and
> 
>           assumes ownership of that object. Thus we need special precautions if
> adding 
>           the instance to the catalogue fails.
>         - Implementation of find_X() methods.
>         - Code handling trigger tables is moved to Backup_info::add_db_object() 
>           because it should be executed only during backup operation.
>         - Move "ER_BACKUP_CATALOG_ADD_TABLE" error injection point to 
>           Image_info::add_table() because we need to report error before we actually
> 
>           create and add to the catalogue the Table instance.
>         - Also in Image_info::add_table() move all preparatory actions which can
> fail, 
>           before the moment Table instance is created and added to the catalogue.
>      @ sql/backup/image_info.h
>         - Add find_ts() function for finding table spaces in the catalogue.
>         - Change signatures of add_X() methods so that now they accept obs::Obj 
>           parameter.
>         - Remove materialize() method which is no longer needed - now obs::Obj 
>           instances are required when entries are created in the catalogue.
>         - Image_info::Obj() constructor accepts obs::Obj parameter, because each 
>           catalogue entry must point at obs::Obj instance.
>         - Add initialization method Image_info::Obj::store_info() which fills the 
>           basic st_bstream_info members. Object's name is read from obs::Obj 
>           instance (and thus is normalized).
>         - Make Image_info::Ts inherit from Db_ref, so that code for constructing
>           object name can be reused.
>         - Add Image_info::Db::find_table/object() mehtods.
>         - Re-use Db_ref::describe() method for Ts and Db classes - avoids code 
>           duplication.
>      @ sql/backup/kernel.cc
>         - For each object added to the catalogue, first an obs::Obj instance must be
>           created using obs::get_X() functions. If this fails, error is reported.
> Once 
>           we have obs::Obj instance, it can be added to the catalogue with add_X() 
>           method. After that the catalogue assumes ownership of that object and will
> 
>           delete it.
>         - When reading metadata, obs::Obj instances stored in catalogue entries are 
>           intialized with this data and then objects are created.

[2] initialized

>         - Avoid unnecessary dynamic string allocation.
>         - A possible non-determinism in error messages is resolved in test script,
>           not here.
>      @ sql/backup/restore_info.h
>         - Change signatures of add_X() methods so that now they accept obs::Obj
>           parameter.
>         - Add checks for duplicates in add_X() methods.
>      @ sql/share/errmsg-utf8.txt
>         New error messages.
>      @ sql/si_objects.cc
>         - Implement Obj::is_my_name() method. For databases, tables, views and
>           triggers it ignores case if lctn is set.
>         - When constructing Obj instances, transform names to lower case if lctn=1.
>         - Remove Abstract_obj::init_obj_from_image() which is no longer needed.
>         - New implementation of get_X() functions which now do not take serialization
> 
>           string parameter.
>      @ sql/si_objects.h
>         - Add Obj::is_my_name() method.
>         - Add Obj::init_from_image() method.
>         - Change signature of get_X() functions - now they dont take serialization
>           string as a parameter.

...

> === added file 'mysql-test/suite/backup/t/backup_cs_names.test'
> --- a/mysql-test/suite/backup/t/backup_cs_names.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/backup/t/backup_cs_names.test	2009-12-04 15:14:43 +0000
> @@ -0,0 +1,120 @@
> +#
> +# Check how RESTORE handles objects whose names differ in case only
> +# on a case insensitive system.
> +#
> +# This test script creates backup images which contain objects of different
> +# types whose names might clash on case insensitive system:
> +#
> +# - cs_names_d.bkp: clash in database names
> +# - cs_names_t.bkp: clash in table names
> +# - cs_names_v.bkp: clash in view names
> +# - cs_names_r.bkp: clash in trigger name
> +#
> +# Note that for other types of objects supported by backup (stored routines, 
> +# events) it is not possible to create two objects whose names differ in
> +# case only, even on case sensitive system with lctn=0.
> +#
> +# The images are copied to suite/backup/std_data/ directory and commited

[2] committed

> +# to the repository. They are used by backup_cs_names_lctn? tests to check
> +# how RESTORE handles them on systems with different lctn settings.
> +#
> +# Note: When backup image format changes these backup images must be 
> +# re-generated and re-commited.

[2] committed

...

> === added file 'mysql-test/suite/backup/t/backup_cs_names_lctn1-master.opt'
> --- a/mysql-test/suite/backup/t/backup_cs_names_lctn1-master.opt	1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/backup/t/backup_cs_names_lctn1-master.opt	2009-12-04 15:14:43
> +0000
> @@ -0,0 +1 @@
> +--lower-case-table-names=1

[3] With so many tests now using lctn values, wouldn't it make more 
sense to have a backup_lctn suite that iterates over the values rather 
than a series of ...lcntN tests?

> === added file 'mysql-test/suite/backup/t/backup_cs_names_lctn1.test'
> --- a/mysql-test/suite/backup/t/backup_cs_names_lctn1.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/backup/t/backup_cs_names_lctn1.test	2009-12-04 15:14:43 +0000
> @@ -0,0 +1,74 @@
> +#
> +# Check how RESTORE handles objects whose names differ in case only
> +# on a case insensitive system.
> +#
> +# This test script uses backup images created by backup_cs_names script:
> +#
> +# - cs_names_d.bkp: clash in database names
> +# - cs_names_t.bkp: clash in table names
> +# - cs_names_v.bkp: clash in view names
> +# - cs_names_r.bkp: clash in trigger name
> +#
> +# It restores from each image and checks that correct error is reported in 
> +# each case.
> +#
> +# The images are stored in suite/backup/std_data/ directory. For simplicity,
> +# we copy them first to the standard backup location
> +#
> +# Note: When backup image format changes these backup images must be 
> +# re-generated and re-commited.

[2] committed

...


> === added file 'mysql-test/suite/backup/t/backup_cs_names_lctn2-master.opt'
> --- a/mysql-test/suite/backup/t/backup_cs_names_lctn2-master.opt	1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/backup/t/backup_cs_names_lctn2-master.opt	2009-12-04 15:14:43
> +0000
> @@ -0,0 +1 @@
> +--lower-case-table-names=2

...a backup_lctn suite would have made this much cleaner.

...


> -  DBUG_EXECUTE_IF("ER_BACKUP_LIST_DBS", dbit= 0;);
> +  DBUG_EXECUTE_IF("ER_BACKUP_LIST_DBS", { delete dbit; dbit= NULL; };);

[5] I think the convention should be to do this:

    DBUG_EXECUTE_IF("something",
    {
      <stmt>
      <stmt>
    };);

Indeed, we see in stream.cc this is how we've done it in the past.

   {
     DBUG_EXECUTE_IF("backup_read_simulate_pipe",{
       /*
         Simulate reading from a pipe, when less bytes is read...
         requested.
       */
       if (howmuch > 1024) howmuch= 1024;
     });

Please use this format for the debug execute if statements that have 
more than one statement. This repeats several times in this patch.

...

> +    If this is a trigger, get the table for the trigger. If we fail here,
> +    we abort operation without inserting the object into the catalog.
> +  */
> +  if (type == BSTREAM_IT_TRIGGER)
> +  {
> +    THD *thd= ::current_thd;
> +    obs::Obj *tbl_obj= obs::find_table_for_trigger(thd, &db.name(), 
> +                                                        obj->get_name());
> +    DBUG_EXECUTE_IF("ER_BACKUP_CATALOG_ADD_TRIGGER_1", 
> +                    { delete tbl_obj; tbl_obj= NULL; };);


[5] Another good example of the inconsistency and how the above request 
would make this formatting problem much easier to read.

...

> === modified file 'sql/backup/image_info.cc'
> --- a/sql/backup/image_info.cc	2009-11-04 14:18:21 +0000
> +++ b/sql/backup/image_info.cc	2009-12-04 15:14:43 +0000
> @@ -215,15 +237,41 @@ int Image_info::add_snapshot(Snapshot_in
>  }
>  
>  
> -///  Check if catalogue contains given database.
> +/**
> +  Find in the catalog a database with the given name.

[2] See, we use 'catalog' here but 'catalogue' other places. I see where 
the patch changed it from catalogue to catalog. Let's be consistent and 
use 'catalog' please.

> +
> +  @return Pointer to the catalog entry for the database if found, 
> +  NULL otherwise.
>  
> -bool Image_info::has_db(const String &db_name) const
> +  @todo More efficient implementation, e.g. using hash.
> +*/
> +
> +Image_info::Db* Image_info::find_db(const String &db_name) const
>  {
>    for (uint n=0; n < m_dbs.count() ; ++n)
> -    if (m_dbs[n] && m_dbs[n]->name() == db_name)
> -      return TRUE;
> +    if (m_dbs[n] && m_dbs[n]->m_obj_ptr->is_my_name(&db_name))
> +      return m_dbs[n];
> +
> +  return NULL;
> +}
> +
> +
> +/**
> +  Find in the catalog a tablespace with the given name.
> +
> +  @return Pointer to the catalog entry for the tablespace if found, 
> +  NULL otherwise.
> +
> +  @todo More efficient implementation, e.g. using hash.
> +*/
>  
> -  return FALSE;
> +Image_info::Ts* Image_info::find_ts(const String &ts_name) const
> +{
> +  for (uint n=0; n < m_ts_map.count() ; ++n)
> +    if (m_ts_map[n] &&
> m_ts_map[n]->m_obj_ptr->is_my_name(&ts_name))
> +      return m_ts_map[n];
> +
> +  return NULL;

[6] I am concerned that this new tablespace code is not being tested. Is 
it possible for you to do a Falcon test and include it in the patch so 
that if (when?) we revisit Falcon we can have a test that alerts us to 
problems? I know it will not run in the tree now, but maybe somewhere it 
will run and thus help protect us.

...

> -  /*
> -    If this is a trigger, get the table for the trigger.
> -    Do this only for backup.
> -  */
> -  THD *thd= ::current_thd;
> -  if ((type == BSTREAM_IT_TRIGGER) && 
> -      (thd->backup_in_progress == SQLCOM_BACKUP))
> -  {
> -    obs::Obj *tbl_obj= obs::find_table_for_trigger(thd, &db.name(), &name);
> -    DBUG_EXECUTE_IF("ER_BACKUP_CATALOG_ADD_TRIGGER_1", return NULL;);
> -    if (!tbl_obj)
> -      return NULL;
> -    
> -    Table *tbl_found;
> -    Db *db_found= find_db(db.name());
> -
> -    DBUG_EXECUTE_IF("ER_BACKUP_CATALOG_ADD_TRIGGER_2", return NULL;);
> -    if (!db_found)
> -      return NULL;
> -    
> -    tbl_found= db_found->find_table(*tbl_obj->get_name());
> -    
> -    DBUG_EXECUTE_IF("ER_BACKUP_CATALOG_ADD_TRIGGER_3", return NULL;);
> -    if (!tbl_found)
> -      return NULL;
> -    
> -    /* 
> -      Save table's snap and position values. 
> -    */
> -    obj->snap_num= tbl_found->snap_num;
> -    obj->pos= tbl_found->base.base.pos;
> -  }
> -  

Commentary: I don't like the reorganizing of this code. Now it is 
scattered throughout the rest of the code and therefore much harder to 
maintain. I would really appreciate it if we resist the urge to 
reorganize code to our own personal tastes or conceptions of 'how it 
should be written'.

...

> === modified file 'sql/backup/image_info.h'
> --- a/sql/backup/image_info.h	2009-11-04 14:18:21 +0000
> +++ b/sql/backup/image_info.h	2009-12-04 15:14:43 +0000

...

>  
> +/**
> +  Search for an object of a given type by its name.
> + 
> +  @param[in] obj_type    Type of the object.
> +  @param[in] table_name  The name of the object.
> + 
> +  @returns Pointer to @c Image_info::Dbobj instance storing information
> +  about the object or NULL if the object is not found.
> +*/
>  inline
> -const char* Image_info::Table::describe(Obj::describe_buf &buf) const
> +Image_info::Dbobj* 
> +Image_info::Db::find_object(const obj_type type, const String &name) const
>  {
> -  return Table_ref::describe(buf);
> +  for (uint pos=0; pos < obj_count(); ++pos)
> +  {
> +    Dbobj *obj= get_obj(pos);
> +    DBUG_ASSERT(!obj || obj->m_obj_ptr);
> +    if (obj && obj->type() == type &&
> obj->m_obj_ptr->is_my_name(&name))
> +      return obj;
> +  }
> +  return NULL;
>  }
>  

[7] Please add TODO here for making this more efficient. Suggestions 
include Ingo's hash idea and keeping the list ordered so that we 
terminate early when the ordering is greater than the item you're 
looking for. I am sure there are other techniques we can consider.

...

> === modified file 'sql/backup/kernel.cc'
> --- a/sql/backup/kernel.cc	2009-12-02 08:01:43 +0000
> +++ b/sql/backup/kernel.cc	2009-12-04 15:14:43 +0000
> @@ -1482,7 +1482,6 @@ int Backup_restore_ctx::restore_triggers
>        switch (obj->type()) {
>  
>        case BSTREAM_IT_EVENT:
> -        DBUG_ASSERT(obj->m_obj_ptr);

[8] Why was this assertion removed? Was it moved or no longer needed?

>          if (events.push_back(obj))
>          {
>            /* Error has been reported, but not logged to backup logs. */
> @@ -1493,7 +1492,6 @@ int Backup_restore_ctx::restore_triggers
>  
>        case BSTREAM_IT_TRIGGER:
>        {
> -        DBUG_ASSERT(obj->m_obj_ptr);

[8] again here

>          int ret= obj->m_obj_ptr->create(m_thd);
>          /* Mimic error in restore of trigger. */
>          DBUG_EXECUTE_IF("restore_trigger", ret= TRUE;);
> @@ -2056,17 +2054,30 @@ int bcat_add_item(st_bstream_image_heade
>  
>    case BSTREAM_IT_TABLESPACE:
>    {
> -    Image_info::Ts *ts= info->add_ts(name_str, item->pos); // Reports errors.
> +    obs::Obj *ts_obj= obs::get_tablespace(&name_str);
> +
> +    if (!ts_obj)
> +    {
> +      log.report_error(ER_BACKUP_CATALOG_ADD_TS, name_str.ptr());
> +      return BSTREAM_ERROR;
> +    }
> +
> +    Image_info::Ts *ts= info->add_ts(ts_obj, item->pos); // Reports errors.

[6] Really concerned now about not having a test for tablespaces.

...

...

> +    switch (item->type)
> +    {
> +      case BSTREAM_IT_VIEW:
> +         error= ER_BACKUP_CATALOG_ADD_VIEW;
> +         obj= obs::get_view(&db->name(), &name_str); 
> +         break;
> +      case BSTREAM_IT_SPROC:
> +         error= ER_BACKUP_CATALOG_ADD_SROUT;
> +         obj= obs::get_stored_procedure(&db->name(), &name_str);
> +         break;
> +      case BSTREAM_IT_SFUNC:
> +         error= ER_BACKUP_CATALOG_ADD_SROUT;
> +        obj= obs::get_stored_function(&db->name(), &name_str);
> +        break;
> +      case BSTREAM_IT_EVENT:
> +        error= ER_BACKUP_CATALOG_ADD_EVENT;
> +        obj= obs::get_event(&db->name(), &name_str);
> +        break;
> +      case BSTREAM_IT_TRIGGER:
> +        error= ER_BACKUP_CATALOG_ADD_TRIGGER;
> +        obj= obs::get_trigger(&db->name(), &name_str);
> +        break;
> +      case BSTREAM_IT_PRIVILEGE:
> +        error= ER_BACKUP_CATALOG_ADD_PRIV;
> +        obj= obs::get_db_grant(&db->name(), &name_str);
> +        break;
> +      default: DBUG_ASSERT(FALSE);    

[9] Why is the default done like this. I don't understand why it would 
be written this way. Shouldn't it simply return or thrown an error? It 
seems this would be a far more severe issue than a debugging solution.

...

> @@ -2374,6 +2432,7 @@ int bcat_create_item(st_bstream_image_he
>    Logger       &log=  info->m_log;
>    THD          *thd=  info->m_thd;
>    int          create_err= 0;
> +  int          ret;


[10] Please initialize variables so that some compilers won't complain.

...

>  
>    /*
>      Check for interruption before creating (and thus first destroying)
> @@ -2417,19 +2476,16 @@ int bcat_create_item(st_bstream_image_he
>      return BSTREAM_ERROR;
>    }
>  
> -  backup::String sdata(create_stmt.begin, create_stmt.end);
> -
> -  DBUG_PRINT("restore",("Creating item of type %d pos %ld: %s",
> -                         item->type, item->pos, sdata.ptr()));
> -  /*
> -    Note: The instance created by Image_info::Obj::materialize() is deleted
> -    when *info is destroyed.
> -  */
> -  obs::Obj *sobj= obj->materialize(0, sdata);
> +  DBUG_ASSERT(obj->m_obj_ptr);
>  
> +  const ::String *name= obj->m_obj_ptr->get_name(); 
>    Image_info::Obj::describe_buf buf;
>    const char *desc= obj->describe(buf);
>  
> +  backup::String sdata(create_stmt.begin, create_stmt.end);
> +
> +  ret= obj->m_obj_ptr->init_from_image(0, &sdata);
> +

[9] Shouldn't the return code be checked here? Otherwise, why are we 
storing it?

...

> -  /*
> -     Grants are a special case because of the way the message is generated --
> -     it produces a non-deterministic error string. Note that a real error
> -     would be deterministic.
> -  */
> -  DBUG_EXECUTE_IF("ER_BACKUP_CANT_RESTORE_PRIV",
> -    log.report_error(ER_BACKUP_CANT_RESTORE_PRIV,
> -      "Debug insertion test");
> -    return BSTREAM_ERROR;);
> -

Commentary: Ok, so again a change that wasn't needed for this patch. I 
would really like to see less of this.

>  #endif
>  
> -  if (!sobj)
> +  if (ret)

Aha, here it is...


> @@ -2496,11 +2540,14 @@ int bcat_create_item(st_bstream_image_he
>  
>    }
>  
> +  DBUG_PRINT("restore",("Creating item %s of type %d pos %ld: %s",
> +                         desc, item->type, item->pos, sdata.ptr()));
> +
>    /* If we are to create a tablespace, first check if it already exists. */
>  
>    if (item->type == BSTREAM_IT_TABLESPACE)
>    {
> -    if (obs::find_tablespace(thd, sobj->get_name()))
> +    if (obs::find_tablespace(thd, name))
>      {
>        // A tablespace with the same name exists. Nothing more to do.
>        DBUG_PRINT("restore",(" skipping tablespace which exists"));
> @@ -2523,19 +2570,22 @@ int bcat_create_item(st_bstream_image_he
>              error handling work in WL#4384 with possible implementation
>              via a related bug report.
>      */
> -    if (!obs::check_user_existence(thd, sobj))
> +    if (!obs::check_user_existence(thd, obj->m_obj_ptr))
>      {
> -      log.report_error(log_level::WARNING,
> -                       ER_BACKUP_GRANT_SKIPPED,
> -                       obs::grant_get_grant_info(sobj)->ptr(),
> -                       obs::grant_get_user_name(sobj)->ptr());
> +      const ::String *info= obs::grant_get_grant_info(obj->m_obj_ptr);
> +      const ::String *user= obs::grant_get_user_name(obj->m_obj_ptr);
> +
> +      log.report_error(log_level::WARNING, ER_BACKUP_GRANT_SKIPPED, 
> +                       info->ptr(), user->ptr());
> +

[6] Ok, now I make it a requirement. Too much untested code for my comfort.

>        return BSTREAM_OK;
>      }
>    }
>  
>    /* Mark that data is being changed. */
>    info->m_data_changed= TRUE;
> -  if (sobj->create(thd))
> +  ret= obj->m_obj_ptr->create(thd);
> +  if (ret)

Commentary. Another non-related change? This seems like another 
unnecessary change in a patch that is huge to begin with. Please refrain 
from making these types of changes. It isn't refactoring either as it 
neither improves nor changes the behavior. If ret were used elsewhere 
then perhaps it would be ok, but I don't see that.

...

> === modified file 'sql/backup/restore_info.h'
> --- a/sql/backup/restore_info.h	2009-12-01 01:49:23 +0000
> +++ b/sql/backup/restore_info.h	2009-12-04 15:14:43 +0000
> @@ -41,10 +41,12 @@ public:
>    /// Determine of information class is valid.
>    bool is_valid();
>  
> -  Image_info::Ts* add_ts(const ::String&, uint);
> -  Image_info::Db* add_db(const ::String&, uint);
> -  Image_info::Table* add_table(Image_info::Db&, const ::String&,
> +  Image_info::Ts*    add_ts(obs::Obj*, uint);
> +  Image_info::Db*    add_db(obs::Obj*, uint);
> +  Image_info::Table* add_table(Image_info::Db&, obs::Obj*,
>                                 backup::Snapshot_info&, ulong);
> +  Image_info::Dbobj* add_db_object(Image_info::Db &db, const obj_type type, 
> +                                   obs::Obj*, ulong pos);
>                                 
>    bool check_restore_privileges(struct st_bstream_item_info *item);
>    
> @@ -115,12 +117,20 @@ bool Restore_info::is_valid()
>  
>  inline
>  backup::Image_info::Ts*
> -Restore_info::add_ts(const ::String &name, uint pos)
> +Restore_info::add_ts(obs::Obj *ts_obj, uint pos)
>  {
> -  Ts *ts= Image_info::add_ts(name, pos);
> +  const String *name= ts_obj->get_name();
> +
> +  if (Ts *other= find_ts(*name))
> +  {
> +    m_log.report_error(ER_RESTORE_NONUNIQ_OBJECT, other->name().ptr());
> +    return NULL;
> +  }
> +
> +  Ts *ts= Image_info::add_ts(ts_obj, pos);

[6] Yep, got to be tested.

...

> === modified file 'sql/share/errmsg-utf8.txt'
> --- a/sql/share/errmsg-utf8.txt	2009-12-01 01:49:23 +0000
> +++ b/sql/share/errmsg-utf8.txt	2009-12-04 15:14:43 +0000
> @@ -6572,4 +6572,7 @@ ER_RESTORE_ACCESS_DEFINER
>    eng "Insufficient privileges. You must have the SUPER privilege to restore the
> object '%s'.'%s'."
>  ER_RESTORE_DB_ERROR
>    eng "The database for object '%s' was not found in the catalog."
> -
> +ER_RESTORE_NONUNIQ_OBJECT
> +  eng "Backup image contains two objects named '%.64s'. The problem might be that
> objects' names differ in letter case only and you are restoring on a case insensitive
> system or --lower-case-table is set to 1."
> +ER_RESTORE_NONUNIQ_DBOBJECT
> +  eng "Backup image contains two objects named '%.64s' in database '%.64s'. The
> problem might be that objects' names differ in letter case only and you are restoring on a
> case insensitive system or --lower-case-table is set to 1."
> 

[4] I do not like the name of these errors. Please consider renaming 
them ER_RESTORE_NONUNIQUE_* so that they read better.

> === modified file 'sql/si_objects.cc'
> --- a/sql/si_objects.cc	2009-12-01 01:49:23 +0000
> +++ b/sql/si_objects.cc	2009-12-04 15:14:43 +0000
> @@ -577,13 +577,10 @@ namespace obs {
>  class Abstract_obj : public Obj
>  {
>  public:
> -  static Obj *init_obj_from_image(Abstract_obj *obj,
> -                                  uint image_version,
> -                                  const String *image);
> -
> -public:
>    virtual inline const String *get_name() const    { return &m_id; }
>    virtual inline const String *get_db_name() const { return NULL; }
> +  virtual inline bool is_my_name(const String *name) const 
> +  { return *name == m_id; }
>  
>  public:
>    /**
> @@ -653,7 +650,8 @@ protected:
>    String m_id; //< identify object
>  
>  protected:
> -  inline Abstract_obj(LEX_STRING id);
> +  inline Abstract_obj(LEX_STRING id, bool lc_name= FALSE);
> +  inline bool is_my_name_lctn(const String *name) const; 
>  
>    virtual inline ~Abstract_obj();
>  
> @@ -664,30 +662,22 @@ private:
>  
>  ///////////////////////////////////////////////////////////////////////////
>  
> -Obj *Abstract_obj::init_obj_from_image(Abstract_obj *obj,
> -                                       uint image_version,
> -                                       const String *image)
> -{
> -  if (!obj)
> -    return NULL;
> -
> -  if (obj->init_from_image(image_version, image))
> -  {
> -    delete obj;
> -    return NULL;
> -  }
> -
> -  return obj;
> -}
> -
> -///////////////////////////////////////////////////////////////////////////
> +/**
> +  Create instance of an object with given name.
>  
> -inline Abstract_obj::Abstract_obj(LEX_STRING id)
> +  @param[in]  id       Name of the object.
> +  @param[in]  lc_name  Should id be converted to lower case.
> +*/ 
> +inline Abstract_obj::Abstract_obj(LEX_STRING id, bool lc_name)
>  {
>    init_sql_alloc(&m_mem_root, ALLOC_ROOT_MIN_BLOCK_SIZE, 0);
>  
>    if (id.str && id.length)
> +  {
>      m_id.copy(id.str, id.length, system_charset_info);
> +    if (lc_name)
> +      my_casedn_str(system_charset_info, m_id.c_ptr());      
> +  }
>    else
>      m_id.length(0);
>    m_errno= 0;
> @@ -873,6 +863,21 @@ bool Abstract_obj::do_init_from_image(In
>  }
>  
>  ///////////////////////////////////////////////////////////////////////////
> +
> +/** 
> +  Alternative implementation of @c is_my_name() which takes into accout the

[2] account

> +  --lower-case-table-names setting. If option is set, it considers all names
> +  which differ from id in case only as refering to this object.

[2] referring

...

> === modified file 'sql/si_objects.h'
> --- a/sql/si_objects.h	2009-11-26 09:52:48 +0000
> +++ b/sql/si_objects.h	2009-12-04 15:14:43 +0000
> @@ -63,6 +63,9 @@ public:
>    */
>    virtual const String *get_db_name() const = 0;
>  
> +  /// Determine if given name refers to this object.
> +  virtual bool is_my_name(const String *name) const = 0;
> +
>  public:
>    /**
>      Serialize an object to an image. The serialization image is opaque
> @@ -76,6 +79,17 @@ public:
>    */
>    virtual bool serialize(THD *thd, String *image) = 0;
>  
> +  /** 
> +    Init this object instance with serialization image so that it is ready
> +    for creation.

[2] Image? Do you mean 'serialization data'?

...

Chuck
Thread
bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903) Bug#43596Rafal Somla4 Dec
  • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Charles Bell7 Dec
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Charles Bell8 Dec
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Ingo Strüwing9 Dec
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Rafal Somla10 Dec
  • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Ingo Strüwing9 Dec
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)Bug#43596Rafal Somla10 Dec