List:Commits« Previous MessageNext Message »
From:anders Date:February 1 2011 11:40am
Subject:Re: bzr commit into mysql-trunk branch (alfranio.correia:3527)
Bug#57873
View as plain text  
Hi Alfranio,

Great work!
Please find my review comments below.

STATUS
------

  Not Approved.


[snip]
> -static char *quote_name(const char *name, char *buff, my_bool force)
> +static char *quote_name(const char *name, char *buffer, my_bool force)
>  {
> -  char *to= buff;
>    char qtype= (opt_compatible_mode & MASK_ANSI_QUOTES) ? '\"' : '`';
Do we really need this?
backtick is always the identifier quote character. So we can always use
backtick.
>  
>    if (!force && !opt_quoted && !test_if_special_chars(name))
>      return (char*) name;
> -  *to++= qtype;
> -  while (*name)
> -  {
> -    if (*name == qtype)
> -      *to++= qtype;
> -    *to++= *name++;
> -  }
> -  to[0]= qtype;
> -  to[1]= 0;
> -  return buff;
> +
> +  return (quote_str(buffer, name, qtype));
>  } /* quote_name */
>  
> 
> 
[snip]

> @@ -2399,15 +2409,27 @@ void Query_log_event::pack_info(Protocol
>  {
>    // TODO: show the catalog ??
>    char *buf, *pos;
> -  if (!(buf= (char*) my_malloc(9 + db_len + q_len, MYF(MY_WME))))
> +  if (!(buf= (char*) my_malloc(6 + q_len + 1 + (db_len * 2) + 2, MYF(MY_WME))))
>      return;
>    pos= buf;
>    if (!(flags & LOG_EVENT_SUPPRESS_USE_F)
>        && db && db_len)
>    {
> -    pos= strmov(buf, "use `");
> -    memcpy(pos, db, db_len);
> -    pos= strmov(pos+db_len, "`; ");
> +    /*
> +      Statically allocates room to store '\0' and an identifier
> +      that may have NAME_LEN * 2 due to quoting and there are
> +      two quoting characters that wrap them.
> +    */
> +    char quoted_db[1 + NAME_LEN * 2 + 2];
> +    size_t size= 0;
> +
> +    pos= strmov(buf, "use ");
> +
> +    Log_event::quote_name(quoted_db, db);
> +    size= strlen(quoted_db);     
> +    memcpy(pos, quoted_db, size);
It is better that quote_name() supports coping to pos, and
returns the length or position.
eg.
+    size= Log_event::quote_name(pos, db);
or 
+    pos= Log_event::quote_name(pos, db);
I prefer the second one.
> +
> +    pos= strmov(pos + size, "; ");
>    }

[snip]
>  
>  #if defined(HAVE_REPLICATION) && !defined(MYSQL_CLIENT)
>  uint Load_log_event::get_query_buffer_length()
> -{
> +{ 
>    return
> -    5 + db_len + 3 +                        // "use DB; "
> -    18 + fname_len + 2 +                    // "LOAD DATA INFILE 'file''"
> -    11 +                                    // "CONCURRENT "
> -    7 +					    // LOCAL
> -    9 +                                     // " REPLACE or IGNORE "
> -    13 + table_name_len*2 +                 // "INTO TABLE `table`"
> -    21 + sql_ex.field_term_len*4 + 2 +      // " FIELDS TERMINATED BY 'str'"
> -    23 + sql_ex.enclosed_len*4 + 2 +        // " OPTIONALLY ENCLOSED BY 'str'"
> -    12 + sql_ex.escaped_len*4 + 2 +         // " ESCAPED BY 'str'"
> -    21 + sql_ex.line_term_len*4 + 2 +       // " LINES TERMINATED BY 'str'"
> -    19 + sql_ex.line_start_len*4 + 2 +      // " LINES STARTING BY 'str'"
> -    15 + 22 +                               // " IGNORE xxx  LINES"
> -    3 + (num_fields-1)*2 + field_block_len; // " (field1, field2, ...)"
> +    5 + (NAME_LEN * 2)  + 3 +                   // "use DB; "
> +    18 + fname_len + 2 +                        // "LOAD DATA INFILE 'file''"
> +    11 +                                        // "CONCURRENT "
> +    7 +					        // LOCAL
> +    9 +                                         // " REPLACE or IGNORE "
> +    13 + table_name_len +                       // "INTO TABLE `table`"
It should be table_name_len * 2.

> +    21 + sql_ex.field_term_len*4 + 2 +          // " FIELDS TERMINATED BY 'str'"
> +    23 + sql_ex.enclosed_len*4 + 2 +            // " OPTIONALLY ENCLOSED BY 'str'"
> +    12 + sql_ex.escaped_len*4 + 2 +             // " ESCAPED BY 'str'"
> +    21 + sql_ex.line_term_len*4 + 2 +           // " LINES TERMINATED BY 'str'"
> +    19 + sql_ex.line_start_len*4 + 2 +          // " LINES STARTING BY 'str'"
> +    15 + 22 +                                   // " IGNORE xxx  LINES"
> +    3 + (num_fields - 1) * 2 + field_block_len; // " (field1, field2, ...)"
It should be 3 + (num_fields) * 2 * 2 + (field_block_len * 2)

>  }
>  
> 
>  void Load_log_event::print_query(bool need_db, const char *cs, char *buf,
>                                   char **end, char **fn_start, char **fn_end)
>  {
> +  /*
> +    Statically allocates room to store '\0' and an identifier
> +    that may have NAME_LEN * 2 due to quoting and there are
> +    two quoting characters that wrap them.
> +  */
> +  char quoted_id[1 + NAME_LEN * 2 + 2];
> +  size_t quoted_id_len= 0;
>    char *pos= buf;
>  
>    if (need_db && db && db_len)
>    {
> -    pos= strmov(pos, "use `");
> -    memcpy(pos, db, db_len);
> -    pos= strmov(pos+db_len, "`; ");
> +    String buffer;
> +
> +    buffer.set_charset(&my_charset_bin);
> +    buffer.append("use ");
> +    buffer.append(Log_event::quote_name(quoted_id, db));
> +    buffer.append("; ");
> +
I think we should operate the original buf directly, it will reduce
one times copy.

> +    pos= strmov(pos, buffer.c_ptr_safe());
>    }
>  
>    pos= strmov(pos, "LOAD DATA ");
> @@ -4710,17 +4753,15 @@ void Load_log_event::print_query(bool ne
>    if (fn_end)
>      *fn_end= pos;
>  
> -  pos= strmov(pos ," TABLE `");
> +  pos= strmov(pos ," TABLE ");
>    memcpy(pos, table_name, table_name_len);
table_name should be quoted.

>    pos+= table_name_len;
>  
>    if (cs != NULL)
>    {
> -    pos= strmov(pos ,"` CHARACTER SET ");
> +    pos= strmov(pos ," CHARACTER SET ");
>      pos= strmov(pos ,  cs);
>    }
> -  else
> -    pos= strmov(pos, "`");
>  
>    /* We have to create all optional fields as the default is not empty */
>    pos= strmov(pos, " FIELDS TERMINATED BY ");
> @@ -4760,8 +4801,12 @@ void Load_log_event::print_query(bool ne
>          *pos++= ' ';
>          *pos++= ',';
>        }
> -      memcpy(pos, field, field_lens[i]);
> -      pos+=   field_lens[i];
> +
> +      Log_event::quote_name(quoted_id, field);
> +      quoted_id_len= strlen(quoted_id);
> +      memcpy(pos, quoted_id, quoted_id_len);
> +      pos+= quoted_id_len;
> +
It is better to operate on the 'pos' directly.

>        field+= field_lens[i]  + 1;
>      }
>      *pos++= ')';
> @@ -5006,10 +5051,15 @@ void Load_log_event::print(FILE* file, P
>    print(file, print_event_info, 0);
>  }
>  
> -
>  
[snip]
> @@ -6062,7 +6122,15 @@ Xid_log_event::do_shall_skip(Relay_log_i
>  void User_var_log_event::pack_info(Protocol* protocol)
>  {
>    char *buf= 0;
> -  uint val_offset= 4 + name_len;
> +  /*
> +    Statically allocates room to store '\0' and an identifier
> +    that may have NAME_LEN * 2 due to quoting and there are
> +    two quoting characters that wrap them.
> +  */
> +  char quoted_id[1 + NAME_LEN * 2 + 2];
> +  Log_event::quote_name(quoted_id, name);
It is better to operate on 'buf' directly.

> +  size_t size= strlen(quoted_id);
> +  uint val_offset= 2 + size;
>    uint event_len= val_offset;
>  
>    if (is_null)
> @@ -6131,16 +6199,13 @@ void User_var_log_event::pack_info(Proto
>      }
>    }
>    buf[0]= '@';
> -  buf[1]= '`';
> -  memcpy(buf+2, name, name_len);
> -  buf[2+name_len]= '`';
> -  buf[3+name_len]= '=';
> +  memcpy(buf + 1, quoted_id, size);
> +  buf[1 + size]= '=';
see above.
>    protocol->store(buf, event_len, &my_charset_bin);
>    my_free(buf);
>  }
>  #endif /* !MYSQL_CLIENT */
>  

[snip]
> @@ -7426,14 +7497,24 @@ void Execute_load_query_log_event::print
>  void Execute_load_query_log_event::pack_info(Protocol *protocol)
>  {
>    char *buf, *pos;
> -  if (!(buf= (char*) my_malloc(9 + db_len + q_len + 10 + 21, MYF(MY_WME))))
> +  if (!(buf= (char*) my_malloc(9 + (db_len * 2) + 2 + q_len + 10 + 21,
> MYF(MY_WME))))
>      return;
>    pos= buf;
>    if (db && db_len)
>    {
> -    pos= strmov(buf, "use `");
> -    memcpy(pos, db, db_len);
> -    pos= strmov(pos+db_len, "`; ");
> +    /*
> +      Statically allocates room to store '\0' and an identifier
> +      that may have NAME_LEN * 2 due to quoting and there are
> +      two quoting characters that wrap them.
> +    */
> +    char quoted_db[1 + NAME_LEN * 2 + 2];
> +    size_t size= 0;
> +    Log_event::quote_name(quoted_db, db);
> +    size= strlen(quoted_db);
> +
> +    pos= strmov(buf, "use ");
> +    memcpy(pos, quoted_db, size);
> +    pos= strmov(pos + size, "; ");
It is better to operate on 'pos' directly.
>    }
>    if (query && q_len)
>    {
> @@ -7446,7 +7527,6 @@ void Execute_load_query_log_event::pack_
>    my_free(buf);
>  }
>  

[snip]

> +  static char *quote_name(char *buffer, const char *name)
> +  {
> +    return (quote_str(buffer, name, '`'));
> +  }
Please add a comment to describe the function.


> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc	2011-01-11 11:45:02 +0000
> +++ b/sql/sql_base.cc	2011-01-19 15:48:29 +0000
> @@ -3758,22 +3758,45 @@ static bool open_table_entry_fini(THD *t
>      entry->file->implicit_emptied= 0;
>      if (mysql_bin_log.is_open())
>      {
> -      char *query, *end;
> -      uint query_buf_size= 20 + share->db.length + share->table_name.length
> +1;
> -      if ((query= (char*) my_malloc(query_buf_size,MYF(MY_WME))))
> +      /*
> +        Dynamically allocates room to store delete from, '\0', dot
> +        and some identifiers: db and table_name.
> +
> +        Each identifier may have NAME_LEN * 2 due to quoting and
> +        there are two quoting characters that wrap them.
> +      */
> +      char *buffer= NULL;
> +      uint buffer_size= 13 + 1 + 1 + (share->db.length * 2) + 2 +
> +                        (share->table_name.length * 2) + 2;
> +      if ((buffer= (char*) my_malloc(buffer_size, MYF(MY_WME))))
>        {
> +        /*
> +          Defines a buffer in the stack to avoid having dynamic allocation
> +          issues within the String object.
> +        */
> +        String log_query(buffer, buffer_size, system_charset_info);
How about to define a String with default constructor?
+          String log_query;
then we don't need to pay attention to free the memory.
> +        log_query.length(0);

[snip]
> 
> === modified file 'sql/sql_db.cc'
> --- a/sql/sql_db.cc	2010-12-10 12:52:55 +0000
> +++ b/sql/sql_db.cc	2011-01-19 15:48:29 +0000
> @@ -30,6 +30,7 @@
>  #include "log_event.h"                   // Query_log_event
>  #include "sql_base.h"                    // lock_table_names, tdc_remove_table
>  #include "sql_handler.h"                 // mysql_ha_rm_tables
> +#include "sql_show.h"                    // append_identifier
>  #include <mysys_err.h>
>  #include "sp.h"
>  #include "events.h"
> @@ -541,7 +542,6 @@ int mysql_create_db(THD *thd, char *db,
>                       bool silent)
>  {
>    char	 path[FN_REFLEN+16];
> -  char	 tmp_query[FN_REFLEN+16];
>    long result= 1;
>    int error= 0;
>    MY_STAT stat_info;
> @@ -618,12 +618,25 @@ not_silent:
>    {
>      char *query;
>      uint query_length;
> +    /*
> +      Statically allocates room to store create database, '\0' and
> +      an identifier that may have NAME_LEN * 2 due to quoting and
> +      there are two quoting characters that wrap them.
> +    */
> +    char buffer[16 + 1 + NAME_LEN * 2 + 2];
> +    String log_query(buffer, sizeof(buffer), system_charset_info);
> +    log_query.length(0);
>  
>      if (!thd->query())                          // Only in replication
>      {
> -      query= 	     tmp_query;
> -      query_length= (uint) (strxmov(tmp_query,"create database `",
Is it is better to use uppercase(CREATE DATABASE)?
Most of the query generated by server is in uppercase.

> -                                    db, "`", NullS) - tmp_query);
> +      if (log_query.append("create database ") ||
> +          append_identifier(thd, &log_query, db, strlen(db)))
> +      {
> +        error= -1;
> +        goto exit;
> +      }
> +      query= log_query.c_ptr_safe();
> +      query_length= log_query.length();
>      }
>      else
>      {
> @@ -885,12 +898,26 @@ update_binlog:
>    {
>      const char *query;
>      ulong query_length;
> +    /*
> +      Statically allocates room to store drop database, '\0' and
> +      an identifier that may have NAME_LEN * 2 due to quoting and
> +      there are two quoting characters that wrap them.
> +    */
> +    char buffer[16 + 1 + NAME_LEN * 2 + 2];
> +    String log_query(buffer, sizeof(buffer), system_charset_info);
> +    log_query.length(0);
> +
>      if (!thd->query())
>      {
>        /* The client used the old obsolete mysql_drop_db() call */
> -      query= path;
> -      query_length= (uint) (strxmov(path, "drop database `", db, "`",
> -                                     NullS) - path);
> +      if (log_query.append("drop database ") ||
Is it is better to use uppercase(CREATE DATABASE)?
Most of the query generated by server is in uppercase.

> +          append_identifier(thd, &log_query, db, strlen(db)))
> +      {
> +        error= true;
> +        goto exit;
> +      }
> +      query= log_query.c_ptr_safe();
> +      query_length= log_query.length();
>      }
>      else
>      {
> @@ -938,11 +965,22 @@ update_binlog:
>  
[snip]
>      for (tbl= tables; tbl; tbl= tbl->next_local)
>      {
> -      uint tbl_name_len;
> -
> -      /* 3 for the quotes and the comma*/
> -      tbl_name_len= strlen(tbl->table_name) + 3;
> -      if (query_pos + tbl_name_len + 1 >= query_end)
> +      size_t size= 0;
> +      /*
> +        Statically allocates room to store an identifier that may
> +        have NAME_LEN * 2 due to quoting and there are two quoting
> +        characters that wrap them.
> +      */
> +      char buffer[1 + NAME_LEN * 2 + 2];
> +      String log_query(buffer, sizeof(buffer), system_charset_info);
> +      log_query.length(0);
> +
> +      append_identifier(thd, &log_query, tbl->table_name,
> +                        strlen(tbl->table_name));
> + 
> +      /* we add 1 to the size for the comma*/
> +      size= log_query.length() + 1;
> +      if (query_pos + size + 1 >= query_end)
>        {
>          /*
>            These DDL methods and logging are protected with the exclusive
I think this part should be refactored.
We can use a String to store the query. As this part happens only when
some error happens, performance isn't a matter.
eg.
- char *query, *query_pos, *query_end, *query_data_start; 
+ String query;
+ query.append("DROP TABLE IF EXISTS ");
for (tbl= tables; tbl; tbl= tbl->next_local) 
{
....
+ append_identifier()
....
} 

[snip]
> 
> === modified file 'sql/sql_load.cc'
> --- a/sql/sql_load.cc	2010-12-10 16:55:50 +0000
> +++ b/sql/sql_load.cc	2011-01-19 15:48:29 +0000
> @@ -29,6 +29,7 @@
>                          // prepare_triggers_for_insert_stmt,
>                          // write_record
>  #include "sql_acl.h"    // INSERT_ACL, UPDATE_ACL
> +#include "sql_show.h"   // append_identifier
>  #include "log_event.h"  // Delete_file_log_event,
>                          // Execute_load_query_log_event,
>                          // LOG_EVENT_UPDATE_TABLE_MAP_VERSION_F
> @@ -672,10 +673,23 @@ static bool write_execute_load_query_log
>    Item                *item, *val;
>    String               pfield, pfields;
>    int                  n;
> -  const char          *tbl= table_name_arg;
> +  const char          *tbl= NULL; 
>    const char          *tdb= (thd->db != NULL ? thd->db : db_arg);
>    String              string_buf;
>  
> +  /*
> +    The SQL statement is not written to the event and is automaticaly
typo: automaticaly.
> +    constructed based on information, such as table and fields, that
> +    is stored in the event.
[snip]
> +
> +/**
> +  Wraps up name with qtype, doubles any qtype in name and stores the
> +  result string in buffer, i.e., this function quotes name.
> +
> +  @param[in,out] buffer Where the quoted name is stored.
> +  @param[in]     name   String that will be quoted.
> +  @param[in]     qtype  Token used to quote name.
> +
> +  @return
> +    @c buffer where the result string is placed.
> +*/
> +char *quote_str(char *buffer, const char *name, char qtype)
> +{
> +  char *to= buffer;
> +
> +  *to++= qtype;
> +  while (*name)
> +  {
> +    if (*name == qtype)
> +      *to++= qtype;
> +    *to++= *name++;
> +  }
> +  to[0]= qtype;
> +  to[1]= 0;
> +  return buffer;
> +}
> 
After reviewing the patch, I think it is not need to implement
quote_str(). It can be done by refactoring append_identifier().
And there is an advantage in append_identifier, append_identifier can
handle different character sets. That is probably necessary by binlog
and other replication functions.

-- 
Your Sincerely, 
Libing Song
==================================
MySQL Replication Team
Software Engineer


Email : Anders.Song@stripped
Skype : libing.song
MSN   : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================


Thread
bzr commit into mysql-trunk branch (alfranio.correia:3527) Bug#57873Alfranio Correia19 Jan
  • Re: bzr commit into mysql-trunk branch (alfranio.correia:3527)Bug#57873anders31 Jan
    • Re: bzr commit into mysql-trunk branch (alfranio.correia:3527) Bug#57873Alfranio Correia31 Jan
      • Re: bzr commit into mysql-trunk branch (alfranio.correia:3527)Bug#57873anders31 Jan
  • Re: bzr commit into mysql-trunk branch (alfranio.correia:3527)Bug#57873anders1 Feb
  • Re: bzr commit into mysql-trunk branch (alfranio.correia:3527) Bug#57873Luís Soares1 Mar