Re: bzr commit into mysql-5.1 branch (hezx:2632) Bug#37051

[He Zhenxing]

Sven Sandberg wrote:
> Hi Zhenxing,
> 
> Patch is good. I suggest two small refactorings below though.
> 
> /Sven
> 
> 
> He Zhenxing wrote:
> [...]
> > === modified file 'sql/log_event.cc'
> > --- a/sql/log_event.cc	2008-05-30 09:12:07 +0000
> > +++ b/sql/log_event.cc	2008-07-30 14:34:23 +0000
> > @@ -1493,21 +1493,8 @@ static void write_str_with_code_and_len(
> >  
> >  bool Query_log_event::write(IO_CACHE* file)
> >  {
> > -  /**
> > -    @todo if catalog can be of length FN_REFLEN==512, then we are not
> > -    replicating it correctly, since the length is stored in a byte
> > -    /sven
> > -  */
> > -  uchar buf[QUERY_HEADER_LEN+
> > -            1+4+           // code of flags2 and flags2
> > -            1+8+           // code of sql_mode and sql_mode
> > -            1+1+FN_REFLEN+ // code of catalog and catalog length and catalog
> > -            1+4+           // code of autoinc and the 2 autoinc variables
> > -            1+6+           // code of charset and charset
> > -            1+1+MAX_TIME_ZONE_NAME_LENGTH+ // code of tz and tz length and tz
> name
> > -            1+2+           // code of lc_time_names and lc_time_names_number
> > -            1+2            // code of charset_database and
> charset_database_number
> > -            ], *start, *start_of_status;
> > +  uchar buf[QUERY_HEADER_LEN + MAX_SIZE_LOG_EVENT_STATUS];
> > +  uchar *start, *start_of_status;
> >    ulong event_length;
> >  
> >    if (!query)
> > @@ -1575,6 +1562,11 @@ bool Query_log_event::write(IO_CACHE* fi
> >    }
> >    if (catalog_len) // i.e. this var is inited (false for 4.0 events)
> >    {
> > +    /*
> > +      only 1 byte to store the length of catalog, so it should not
> > +      surpass 255
> > +    */
> > +    DBUG_ASSERT(catalog_len <= 255);
> 
> Very good. But could it be added to write_str_with_code_and_len instead? 
> That could potentially catch more mistakes.
> 

Good

> >      write_str_with_code_and_len((char **)(&start),
> >                                  catalog, catalog_len, Q_CATALOG_NZ_CODE);
> >      /*
> > @@ -1611,6 +1603,11 @@ bool Query_log_event::write(IO_CACHE* fi
> >    }
> >    if (time_zone_len)
> >    {
> > +    /*
> > +      only 1 byte to store the length of time_zone, so it should not
> > +      surpass 255
> > +    */
> > +    DBUG_ASSERT(time_zone_len <= 255);
> 
> Same here: it can be refactored to write_str_with_code_and_len.
> 

OK

> [...]
> > === modified file 'sql/sql_class.h'
> > --- a/sql/sql_class.h	2008-05-20 07:38:17 +0000
> > +++ b/sql/sql_class.h	2008-07-30 14:34:23 +0000
> > @@ -397,6 +397,11 @@ struct system_variables
> >    DATE_TIME_FORMAT *time_format;
> >    my_bool sysdate_is_now;
> >  
> > +  /*
> > +    map for tables that will be updated for a multi-update query
> > +    statement, for other query statements, this will be zero.
> > +  */
> > +  table_map table_map_for_update;
> >  };
> 
> Hmm, I'm not sure, but it seems that system_variables is supposed to be 
> used for things that are exported to SQL as @@system_variables. So this 
> may not be the right place to declare table_map_for_update. I think it 
> is probably better to declare it directly as a field of the THD class.
> 

Yes, I thought that this should be a session variable, but now it is
not, so I think it's more appropriate to move it to THD class (although
I really hate to add new stuff to the already overstaffed THD class).

> 
> -- 
> Sven Sandberg, Software Engineer
> MySQL AB, www.mysql.com
>