On Sat, 2007-06-23 at 12:37 +0200, Ingo Strüwing wrote:
> 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.
Future plan involved making it a method of String and cleaning up the
similar use in sql_show.cc
> You did not change the appends of field_name in update_row() and
> delete_row(). Just miss them, or is there a reason?
Yes. After these patches are out of the way, I fully intend to file new
bug reports and fix the other cases,
I am trying to not do the rampant "fix everything in sight" patches I
have tended to do in the past.
> ...
> > - 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?
Identifiers quoting with backticks are a bit simpler. Don't need such a
heavyweight function.
>
> ...
> > @@ -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?
Hmm, yes it is - I was simply moving existing code around and I didn't
check if the code was already redundant.
> ...
> > +/**
> > + @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.
Ok, I will address this,
Regards,
Antony
> ...
>
> 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
>
>
>
--
Antony T Curtis, Senior Software Developer
MySQL Inc, www.mysql.com
SIP: 4468@stripped
