Re: bzr commit into mysql-next-mr-rpl-merge branch (alfranio.correia:3202) WL#2775

[He Zhenxing]

Hi Alfranio,

Thank you for the work! I see now critical issues, here are some
comments!

STATUS
------
Not Approved!

REQUIRED CHANGES
----------------
 
RC1. DBUG_ABORT(), abort() => DBUG_SUICIDE() BUG#52002

RC2. remove rli->flush_info(TRUE) when simulating a crash,
     especially with transactional repo engines, it should be
     able to recover.

RC3. I think temporarily set thd->system_thread to
     SYSTEM_THREAD_INFO when accessing info table during the
     query exection is misuse of the concept of system
     thread. And this will make the output of SHOW PROCESSLIST
     to show the thread as a system thread during that period,
     which I think can be confusing. Please consider use some
     other flag instead.

REQUESTS
--------
R1. info_field, why not using the already exist class String?
    and I also think it should be named info_value because it
    holds the value of a info field. Please also consider
    rename class Rpl_info_fields. There are also some other
    uses of the name 'field' actually means 'value', such as
    Rpl_info_table_access::store_info_fields() should be
    renamed as store_info_values().

R2. I do not quite understand why need Rpl_info_table::field_idx,
    could you please explain.

R3. I think we need to bench mark the performance of using
    table repo if we have not done that yet

R4. Wondering if it's possible to avoid open_table and
    close_table for every operation on the info repo table. For
    example, keep the table open, but introduce another mutex
    to protect the access to it.

R5. It seems what Rpl_info_*::reset_info() does is to remove
    the info from the repository, so I'd suggest rename the
    function to reflect that.

R6. Do we allow changing repo table engine dynamically? If not
    then I think Rpl_info_table::do_is_transactional() no need
    to check the table every time, I think this can be checked
    and saved the result when initializing the info table.

R7. str_schema, str_schema_size, str_table str_table_size, I'd
    suggest to use LEX_STRING to store the string and the size.

R8. Rpl_info_table_access::create/drop_bootstrap_thd(), I'd
    suggest remove the 'bootstrap' from there names because
    they are not used only when bootstrap.

R9. what's the use of NULL_TABLE_INFO? I can not find anywhere
    it was used.

R10. Please define a new TABLE_CATEGORY for repository table,
     possible candidates: TABLE_CATEGORY_SYSTEM
     TABLE_CATEGORY_REPLICATION TABLE_CATEGORY_INFO

SUGGESTIONS
-----------
S1. Suggest to change the signature of
    Rpl_info_factory::create_mi() to: Master_info*
    Rpl_info_factory::create_mi(uint mi_option);

S2. Rpl_info_table_access::find_info_id(), I'd suggest rename
    it something like 'locate_info_for_server_id'


Alfranio Correia wrote:
> #At
> file:///home/acorreia/workspace.sun/repository.mysql.new/bzrwork/wl-2775/mysql-next-mr-prepared-for-5125/
> based on revid:alfranio.correia@stripped
> 
>  3202 Alfranio Correia	2010-10-08
>       WL#2775

[snip]