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
