Re: #37656 [Pnd]: lower_case_table_names=1 doesn't convert database names in replicated statements

[Luís Soares]

Hi Zhenxing,

On Wed, 2010-03-03 at 15:44 +0800, He Zhenxing wrote:
> Hi Luis,
> 
> I think the change is good, but I think set_thd_db() is a little bit
> confusing with THD::set_db(), better be named set_rewrite_thd_db() or
> alike.

Yeah, I agree. Will change the name to set_rewrite_thd_db (which 
is pretty much what originally Bar had suggested as a method name).

Regards,
Luís

> 
> Luís Soares wrote:
> > Hi Bar,
> >   After some discussion inside the replication team, I have 
> > recommitted a patch, which addresses this one concernof yours!
> > 
> > On Tue, 2009-07-21 at 14:37 +0500, Alexander Barkov wrote:
> > > Hi Luis,
> > > 
> > 
> > [snip]
> > 
> > > >>>>
> > > >>>>
> > > >> I made a closer look into the patch
> > > >> and found some duplicate code.
> > > >>
> > > >> Perhaps a better idea is even to add a new method:
> > > >>
> > > >>
> > > >> Log_event::set_rewrite_db()
> > > >> {
> > > >>    char lcase_db_buf[NAME_LEN +1];
> > > >>    LEX_STRING new_db;
> > > >>    new_db.length= db_len;
> > > >>    if (lower_case_table_names == 1)
> > > >>    {
> > > >>      strmov(lcase_db_buf, db);
> > > >>      my_casedn_str(system_charset_info, lcase_db_buf);
> > > >>      newdb.str= lcase_db_buf;
> > > >>    }
> > > >>    else
> > > >>    {
> > > >>      newdb.str= db;
> > > >>    }
> > > >>    new_db.str= (char*) rpl_filter->get_rewrite_db(newdb.str,
> > > >>                                                  
> &new_db.length);
> > > >>    thd->set_db(new_db.str, new_db.length);
> > > >> }
> > > > 
> > > > Both Load_log_event and Query_log_event derive from Log_event. 
> > > > However, Log_event has no reference to db_len and db, these are
> > > > properties defined in Load_log_event and Query_log_event
> (separately).
> > > > So one cannot just define a top level method to aggregate
> > > > functionality. 
> > > > 
> > > > OTOH, we could, declare a static function in log_event.cc that would
> > > > remove the duplicate portion of code and would pretty much serve the
> > > > same purpose of the Log_event::set_rewrite_db. Although I am not very
> > > > keen on this approach, it would look something like (I renamed
> > > > set_rewrite_db to set_thd_db - I think this name is more
> appropriate):
> > > > 
> > > > static void set_thd_db(THD *thd, const char *db, uint32 db_len)
> > > > {
> > > >   char lcase_db_buf[NAME_LEN +1]; 
> > > >   LEX_STRING new_db;
> > > >   new_db.length= db_len;
> > > >   if (lower_case_table_names == 1)
> > > >   {
> > > >     strmov(lcase_db_buf, db); 
> > > >     my_casedn_str(system_charset_info, lcase_db_buf);
> > > >     new_db.str= lcase_db_buf;
> > > >   }
> > > >   else 
> > > >     new_db.str= (char*) db;
> > > >   new_db.str= (char*) rpl_filter->get_rewrite_db(new_db.str,
> > > > &new_db.length);
> > > >   thd->set_db(new_db.str, new_db.length);
> > > > }
> > > 
> > > 
> > > I agree that using a static function is not a good approach.
> > > 
> > > It seems that  Load_log_event and Query_log_event are missing
> > > an intermediary class on top of them, which can have at least
> > > the following common properties and methods:
> > > 
> > > 
> > > class Log_event_with_db: public Log_event
> > > {
> > > public:
> > >    const char *db;
> > >    uint32 db_len;
> > >    const char *get_db() { return db; };
> > >    ulong thread_id;
> > >    ulong slave_proxy_id;
> > >    virtual bool is_valid();
> > >    void set_thd_db(THD *thd, const char *db, uint32 db_len);
> > >    /*
> > >       Perhaps we even don't need the last two parameters
> > >       in the above method.
> > >    */
> > > }
> > > 
> > > 
> > > Then both Load_log_event and Query_log_event can derive from
> > > Log_event_with_db.
> > > 
> > 
> > Please check the new patch and the comments in the bug report,
> > which address your concern.
> > 
> > Basically, we chose to go with the static function, instead of
> > deepening the class hierarchy. 
> > 
> > In the future we may do this bu most likely within the context of 
> > a refactoring effort (which in itself would/will probably
> > address more than just "db" and "db_len" fields).
> > 
> > > > 
> > > > Then in the {Query,Load}_log_event we would have the following:
> > > 
> > > Agree, using Log_evet_with_db::set_thd_db() instead of
> > > a static function set_thd_db().
> > > 
> > > > 
> > > > @@ -2939,9 +2958,6 @@
> > > > int Query_log_event::do_apply_event(Relay_log_info const *rli,
> > > >                                        const char *query_arg, uint32
> > > > q_len_arg)
> > > > {
> > > > -  char db_buf[NAME_LEN + 1];
> > > > -  strmov(db_buf, db);
> > > > -  LEX_STRING new_db;
> > > >    int expected_error,actual_error= 0;
> > > >    /*
> > > >      Colleagues: please never free(thd->catalog) in MySQL. This
> would
> > > > @@ -2951,11 +2967,7 @@
> > > >      you.
> > > >    */
> > > >    thd->catalog= catalog_len ? (char *) catalog : (char *)"";
> > > > -  new_db.length= db_len;
> > > > -  if (lower_case_table_names == 1)
> > > > -    my_casedn_str(system_charset_info, db_buf);
> > > > -  new_db.str= (char *) rpl_filter->get_rewrite_db(db_buf,
> > > > &new_db.length);
> > > > -  thd->set_db(new_db.str, new_db.length);       /* allocates a
> copy of
> > > > 'db' */
> > > > +  set_thd_db(thd, db, db_len);
> > > >    thd->variables.auto_increment_increment=
> auto_increment_increment;
> > > >    thd->variables.auto_increment_offset=    auto_increment_offset;
> > > > 
> > > > @@ -4449,14 +4461,7 @@
> > > > int Load_log_event::do_apply_event(NET* net, Relay_log_info const
> *rli,
> > > >                                     bool use_rli_only_for_errors)
> > > > {
> > > > -  char db_buf[NAME_LEN + 1];
> > > > -  strmov(db_buf, db);
> > > > -  LEX_STRING new_db;
> > > > -  new_db.length= db_len;
> > > > -  if (lower_case_table_names == 1)
> > > > -    my_casedn_str(system_charset_info, db_buf);
> > > > -  new_db.str= (char *) rpl_filter->get_rewrite_db(db_buf,
> > > > &new_db.length);
> > > > -  thd->set_db(new_db.str, new_db.length);
> > > > +  set_thd_db(thd, db, db_len);
> > > > 
> > > >> This will help to avoid unnecessary strmov() when
> > > >> lower_case_table_names is 0, and also help to get
> > > >> rid of duplicate code in
> > > >> Query_log_event::do_apply_event() and
> Load_log_event::do_apply_event().
> > > >>
> > > >>
> > > >> So instead of
> > > >>
> > > >>    char db_buf[NAME_LEN + 1];
> > > >>    strmov(db_buf, db);
> > > >>    LEX_STRING new_db;
> > > >>    int expected_error,actual_error= 0;
> > > >>    thd->catalog= catalog_len ? (char *) catalog : (char *)"";
> > > >>    new_db.length= db_len;
> > > >>    if (lower_case_table_names == 1)
> > > >>      my_casedn_str(system_charset_info, db_buf);
> > > >>    new_db.str= (char *) rpl_filter->get_rewrite_db(db_buf,
> > > >>                                                   
> &new_db.length);
> > > >>    thd->set_db(new_db.str, new_db.length); /* allocates a copy
> of 'db' */
> > > >>
> > > >>
> > > >> you could just do:
> > > >>
> > > >>    thd->catalog= catalog_len ? (char *) catalog : (char *)"";
> > > >>    set_rewrite_db();
> > > >>
> > > >>
> > > >> in both Query_log_event::do_apply_event() and 
> > > >> Load_log_event::do_apply_event().
> > > >>
> > > >>
> > > >> Please also find one comment below:
> > > >>
> > > >>
> > > >>>> And in many other places below.
> > > >>>>
> > > >>>>
> > > >>>> Otherwise the patch looks fine for me.
> > > >>>>
> > > >>>> I have ticked the checkbox.
> > > >>> Thanks for the input I will check this. Also, I was mostly
> concerned
> > > >>> about the charset issue which you provided very good feedback,
> again
> > > >>> thanks.
> > > >>>
> > 
> > [snip]
> > 
> > Regards,
> > Luís
> > 
> > 
> 
>