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

[Sven Sandberg]

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.

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

[...]
> === 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.


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