MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Antony T Curtis Date:June 23 2007 7:27pm
Subject:Re: bk commit into 5.0 tree (antony:1.2492) BUG#29019
View as plain text  
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

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