Re: bzr commit into mysql-6.0-backup branch (oystein.grovlen:2699) Bug#39089 WL#4384

[Øystein Grøvlen]

Rffal,

Thanks for the review.  Responses to requests and suggestiongs below.  I 
will address the commentary part in a separate email.


Rafal Somla wrote:
> STATUS
> ------
> Changes requested.
> 
> REQUESTS
> --------
> 
> 1. In Logger::write_message() (logger.cc), please fix the error which 
> was present there. In case of reporting errors using 
> push_warning_printf() the constant should be 
> MYSQL_ERROR::WARN_LEVEL_ERROR, not MYSQL_ERROR::WARN_LEVEL_WARN.

I am not quite sure this is the right patch to fix this.  It is kind of 
unrelated to what I try to achieve, and it seems it also involves fixing 
at least one result file.  I think it is better to make this a separate 
patch since it would then be easier to spot the specific change.

> 
> 2. In the same function, use m_push_errors flags also for controlling 
> whether *warnings* are pushed on the stack.

OK.  Will do.

> 
> 3. Inside Backup_pump constructor, interrupt construction if errors are 
> detected (e.g., if bitmap_init() fails).

Right. Will do.

> 
> 4. Implementation of Backup_info::Global_iterator class. Either keep the 
> current design of "construction is initialization", in which case there 
> is no init() method, or switch to using *::init() method consistently 
> for all iterators. The second alternative would mean adding init() to 
> the public interface of Image_info::Iterator and implementing it in 
> derived classes.

"Construction is initialization" is not achievable without using 
exceptions since that is the only way to abort a construction upon errors.

On the other hand, adding init() methods that will lead me into the same 
trouble as we discussed before.  The iterators are created by functions 
that return pointers to the created objects.  Since many of them does 
not have the context to log errors, there is no way to report the errors 
that init() may generate.  (Global_iterator is different since it is a 
inner class to Backup_info).  Hence, information about the error will be 
lost.  From a pragmatic point of view, that is probably not an issue 
since these init methods will currently not generate any errors anyway. 
      So I will go ahead and add init() methods.

> 
> 5. Use macros TRUE and FALSE instead of C++ constants true and false. 
> This is for portability.

OK.

> 
> 6. Remove incorrect "never errors" comments:
>  - at info.get_db_object() call inside find_obj() (image_info.cc) - this 
> method
>    can error and signal this by returning NULL.
>  - when Backup_info instance is created inside
>    Backup_restore_ctx::prepare_for_backup(). The constructor detects and 
> logs
>    errors.
> 
> SUGGESTIONS
> -----------
> 
> 1. Implement Logger::push_errors() as inlined method in logger.h.

This is not according to the code standard:

"Do not make a function inline if you don't have a very good reason for 
it. In many cases, the extra code that is generated is more likely to 
slow down the resulting code than give a speed increase because the 
bigger code will cause more data fetches and instruction misses in the 
processor cache."

I do not think there is any very good reasons for making error handling 
code inline.  This is code that that is not part of the normal execution 
part.

> 
> 2. In this fragment:
> 
>  > @@ -1646,11 +1658,9 @@ void* bcat_db_iterator_get(st_bstream_im
>  >    }
>  >
>  >    backup::Image_info::Iterator *it= info->get_db_objects(*db);
>  > -
>  >    if (!it)
>  >    {
>  > -    info->m_ctx.fatal_error(ER_BACKUP_LIST_DB_TABLES);
>  > -    return NULL;
>  > +    info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES);
>  >    }
> 
> retain early return as in the original code. This is safer since the 
> function will correctly return in case of error, even if someone adds 
> some code after if( ...) statement.

Good point.  I will do  this.

--
Øystein