From: Roy Lyseng
Date: June 20 2011 7:33am
Subject: Re: WL4897: Review comments
List-Archive: http://lists.mysql.com/commits/139488
Message-Id: <4DFEF7C6.5010109@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Gleb,

thank you for the update. Just some concluding remarks from me, no additional 
requests.

On 17.06.11 21.57, Gleb Shchepa wrote:
> Hello Roy,
>
> Thank you for the review.
> The patch is available in the tree:
> gleb.shchepa@stripped
>
> Thank you,
> Gleb.
>
> On 06/17/2011 03:41 PM, Roy Lyseng wrote:
>> Hi Gleb,
>>
>> I approve the coding for this worklog.
>>
>> Please consider the (rather few) suggestions that I have provided inline. They
>> should all be tagged with RL: at the front of the line.
>>
>> === modified file 'client/mysqltest.cc'
>> --- client/mysqltest.cc 2011-03-22 15:40:32 +0000
>> +++ client/mysqltest.cc 2011-06-16 12:58:35 +0000
>> @@ -201,6 +201,7 @@ static char *opt_plugin_dir= 0;
>> static my_regex_t ps_re; /* the query can be run using PS protocol */
>> static my_regex_t sp_re; /* the query can be run as a SP */
>> static my_regex_t view_re; /* the query can be run as a view*/
>> +static my_regex_t explain_re;/* the query can be converted to EXPLAIN */
>>
>> static void init_re(void);
>> static int match_re(my_regex_t *, char *);
>> @@ -6389,7 +6390,7 @@ static struct my_option my_long_options[
>> {"view-protocol", OPT_VIEW_PROTOCOL, "Use views for select.",
>> &view_protocol, &view_protocol, 0,
>> GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
>> - {"explain-protocol", OPT_EXPLAIN_PROTOCOL, "Explains all select.",
>> + {"explain-protocol", OPT_EXPLAIN_PROTOCOL, "Explains all select/update/etc.",
>>
>> RL: Suggestion: "Explain all DML statements"
>
> Unfortunately not all:
> http://dev.mysql.com/doc/refman/5.6/en/sql-syntax-data-manipulation.html
>
> : 12.2. Data Manipulation Statements
> :
> : 12.2.1. CALL Syntax
> : 12.2.2. DELETE Syntax
> : 12.2.3. DO Syntax
> : 12.2.4. HANDLER Syntax
> : 12.2.5. INSERT Syntax
> : 12.2.6. LOAD DATA INFILE Syntax
> : 12.2.7. LOAD XML Syntax
> : 12.2.8. REPLACE Syntax
> : 12.2.9. SELECT Syntax
> : 12.2.10. Subquery Syntax
> : 12.2.11. UPDATE Syntax
>
> I remember that different RDBMS manufacturers use different definition of DML
> (for example PgSQL's definition of DML doesn't include SELECT, they call it
> "query", not DML :-)
> So, I'll change that string but with "Explain all
> SELECT/INSERT/REPLACE/UPDATE/DELETE statements".

According to the SQL standard, CALL, DO and HANDLER are not DML statements. But 
that's OK.

>>
>> + const char *str;
>> + size_t length;
>> +
>> + mem_root_str(THD *thd_arg) : thd(thd_arg) { cleanup(); }
>> + void cleanup()
>> + {
>> + str= NULL;
>> + length= 0;
>> + }
>>
>> RL: Consider naming this function empty() instead of cleanup(). This name is
>> IMHO more describing.
>
> It is easy to confuse the "empty()" name with a predicate. Moreover, in all STL
> containers "empty" method is a predicate (common practice and a part of the
> standard).

Well, the predicate will have a bool return value, but this one will not.

> I believe that 50% of our colleagues think that "empty" is a predicate and of
> course other 50% think that this is an imperative ;-)
> OTOH I believe such a confuse is impossible with the "cleanup" name :-)
> The practice to use the "cleanup" name is common for MySQL and I'd like to save it.

I agree, but this holds for composite entities mostly. When you have a simple 
concrete class like this, I prefer concrete function names, which I think 
empty() is. But I will not insist on a change, so I accept your explanation.

Thanks,
Roy