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

[Luís Soares]

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