MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:June 23 2007 10:37am
Subject:Re: bk commit into 5.0 tree (antony:1.2492) BUG#29019
View as plain text  
Hi Antony,

antony@stripped wrote:
...
> ChangeSet@stripped, 2007-06-19 16:58:27-07:00, antony@stripped +4 -0
>   Bug#29019
>     "REPLACE/INSERT IGNORE/UPDATE IGNORE doesn't work"
>     Federated does not record neccessary HA_EXTRA flags in order to
>     support REPLACE/INSERT IGNORE/UPDATE IGNORE.
>     Implement ::extra() to capture flags neccessary for functionality.
>   New function append_ident() to better escape identifiers.
...
> +/**
> +  @brief Append identifiers to the string.
> +  @param string The target string.
> +  @param name Identifier name
> +  @param length Length of identifier name in bytes
> +  @param quote_char Quote char to use for quoting identifier.
> +  @return FALSE if successful
> +  @note This function is based upon the append_identifier() function
> +        in sql_show.cc except that quoting always occurs.
> +*/
> +
> +static bool append_ident(String *string, const char *name, uint length,
> +                         const char quote_char= '`')

Small problems with the function comment. Empty lines between sections.
@param miss [in], [out], or [in,out] qualifiers. @retval missing.

You don't use the parameter 'quote_char' anywhere. And this is good. If
we want federated to be able to connect to a non-MySQL DBMS one day, we
would have a single place where we might need to change the quote char
depending on the DBMS.

I suggest to remove the parameter and make it a local variable.

You did not change the appends of field_name in update_row() and
delete_row(). Just miss them, or is there a reason?

...
> -    query.append(FEDERATED_BTICK);
> -    escaped_table_name_length=
> -      escape_string_for_mysql(&my_charset_bin, (char*)escaped_table_name,
> -                            sizeof(escaped_table_name),
> -                            share->table_name,
> -                            share->table_name_length);
> -    query.append(escaped_table_name, escaped_table_name_length);
> -    query.append(FEDERATED_BTICK);
> +    append_ident(&query, share->table_name, share->table_name_length);

escape_string_for_mysql() does more than append_ident(). Can we be sure
that none of the precautions in escape_string_for_mysql() is required
for identifiers?

...
> @@ -1309,31 +1344,26 @@

Antony, can you please, please install diff-p-helper, so that I can see
behind the last @@ which function this diff belongs to?

I guess it's static FEDERATED_SHARE *get_share().

>      query.append(FEDERATED_SELECT);
>      for (field= table->field; *field; field++)
>      {
> -      query.append(FEDERATED_BTICK);
> -      query.append((*field)->field_name);
> -      query.append(FEDERATED_BTICK);
> +      append_ident(&query, (*field)->field_name,
> strlen((*field)->field_name));
>        query.append(FEDERATED_COMMA);
>      }
>      query.length(query.length()- strlen(FEDERATED_COMMA));
>      query.append(FEDERATED_FROM);
> -    query.append(FEDERATED_BTICK);
> +
> +    tmp_share.table_name_length= strlen(tmp_share.table_name);

Isn't this done in parse_url() already?

...
> +/**
> +  @brief Handles extra signals from MySQL server
> +  @param operation Signal
> + */
> +int ha_federated::extra(ha_extra_function operation)

Small problems with the function comment. Empty lines between sections.
@param misses [in] qualifier. @return and @retval missing. Empty line
after comment missing.

...

Regards
Ingo
-- 
Ingo Strüwing, Senior Software Developer
MySQL GmbH, Radlkoferstr. 2, D-81373 München
Geschäftsführer: Kaj Arnö - HRB München 162140


Thread
bk commit into 5.0 tree (antony:1.2492) BUG#29019antony20 Jun
  • Re: bk commit into 5.0 tree (antony:1.2492) BUG#29019Ingo Strüwing23 Jun
    • Re: bk commit into 5.0 tree (antony:1.2492) BUG#29019Antony T Curtis23 Jun