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

[He Zhenxing]

Alfranio Correia wrote:
> 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.
> 

I think it is needed for non-transactional repo engines, but for
transactional repo engines, it could be removed and the server should be
able to recovery.

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

OK

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

It seems currently it is always 0 for all info tables, I think we can
always assume the first column to be the primary key, and so we can
remove this member, but I'm OK if you decide to keep this.

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

OK

> 
> Note also that R4 is related to R3.
> 

yes, and that's one of the main concerns for possible performance
impact.

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

I'd suggest to rename all of then to remove_info()

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

I tent to think this is not a good idea to allow repo table engine to be
changed at runtime, it might cause unexpected behavior,  so I'd strongly
suggest to disable this if you do not have any good use cases for this
feature.

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

Sorry, I think there is a misunderstanding, what I mean is to rename the
function to create_thd() or drop_thd(), because this function is not
only used at bootstrap time, but also used after server is up.

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

Fine for me.

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

OK

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