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

[Alfranio Correia]

Hi Jasonh,

Thank you for the review.
See some comments in-line.

Cheers.

On 10/18/2010 04:47 AM, He Zhenxing wrote:
> 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

ok. I will do this.

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

I think this is necessary.
This is done for non-transactional tables to guarantee a
slave will not fail due to duplicate keys after recovering.

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


I agree with you. I will do this.

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

I don't have a strong opinion.
I will consider this and let you know if it is feasible.

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

I agree.

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

This is the number of the field that is used as primary key.
We assume this is fixed and will not change. If for some reason
a super user changes this, the replication will fail with an error
message.


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

I agree with you but I prefer to do this as part of another WL.
See WL#5126.

Note also that R4 is related to R3.

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

I kept the same names from the FILE repository.
Do you have a suggestion?

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

Yes. It is possible to change it dynamically. A super user can
always execute "alter 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.

ok. I will do this.

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

Do you have any suggestion. This functions are used when the
server is starting up and there is no session/thread yet.

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

I need to remove this. I did a refactory and forgot to remove this.

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

TABLE_CATEGORY_RPL_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);

I will do this.

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

find_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]
>