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