List:Commits« Previous MessageNext Message »
From:Rafal Somla Date:December 10 2009 11:47am
Subject:Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2903)
Bug#43596
View as plain text  
Hi Chuck,

Thank you for the review. I have committed an updated patch: 
http://lists.mysql.com/commits/93477.

Charles Bell wrote:
> STATUS
> ------
> Not approved.
> 
> REQUIRED
> --------
>  6. Add falcon test (test with tablespaces).

I've reported BUG#49581 about the table space code issue (that it is 
currently not used and not tested).

...
> DETAILS
> -------
>>       Bug #43596 Restore fails on case ins. server for databases who's 
>> names differ                  in case only.
> 
> [1] Please use "BUG#XXXXX"

Fixed.

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

I suggest opening a new bug to address the issue systematically. In this 
patch I try to use 'catalog'.

...
>>         - When reading metadata, obs::Obj instances stored in 
>> catalogue entries are           intialized with this data and then 
>> objects are created.
> 
> [2] initialized

Fixed.

...
>> +# The images are copied to suite/backup/std_data/ directory and commited
> 
> [2] committed
...
>> +# Note: When backup image format changes these backup images must be 
>> +# re-generated and re-commited.
> 
> [2] committed

Fixed.

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

I think the problem with this idea is that in general tests are not supposed 
to produce the same results with different lctn setting. Thus I'd stay with 
the current arrangement for the moment.

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

Since:
- it is not explicitly covered in the coding guidelines,
- Ingo has expressed different preference,
- I have similar preference as Ingo (prefer debug code to be as compact as
   possible),
I decided to leave it as it is.

...
>> +/**
>> +  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.
> 

OK, done.

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

These assertions are not really needed here, because pointer is checked 
earlier, in bcat_create_item().

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

This is because this switch() statement is within another switch() statement 
and thus we know that other values of item->type can't happen. I think this 
is pretty obvious when the whole code is seen, but looks worrying in the patch.

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

OK, done.

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

It is checked below, after possible error injection.

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

I think this particular code is tested as much as it was before. The change 
you see above has nothing to do with how much this is tested or not. It is 
just saving pointers in local variables for better readability - the new 
code does exactly the same thing as the old code.

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

I had to change "sobj->create(thd)" to "obj->m_obj_ptr->create(thd)" anyway. 
On that occasion I introduced ret value which helps in debugging and 
maintaining the code (e.g. if we want to insert an interruption check before 
checking the result or do error injection here).

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

OK, Done.

...
>> +
>> +/** +  Alternative implementation of @c is_my_name() which takes into 
>> accout the
> 
> [2] account

Fixed.

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

Fixed.

>> === 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'?

I used the same name which is used few lines above (not my idea).

Rafal

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