List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:December 17 2010 9:47pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3237)
View as plain text  
Jorgen Loland a écrit, Le 15.12.2010 09:50:
> Guilhem,
> 
> Approved pending fix of review comments (see inline)
> 
> On 11/26/2010 05:16 PM, Guilhem Bichot wrote:
>> #At 
>> file:///home/mysql_src/bzrrepos_new/mysql-next-mr-opt-backporting-wl4800/ 
>> based on revid:guilhem@stripped
>>
>>   3237 Guilhem Bichot    2010-11-26
>>        Limit optimizer tracing to a certain set of SQL commands (safer).
>>        All other changes in this patch follow from this goal.
>>       @ mysql-test/include/optimizer_trace.inc
>>          * Marks in queries (as /**/ comments )to facilitate 
>> understanding results
>>          (exactly identical queries make it hard).
>>          * Comments explaining why results now differ between non-ps 
>> and --ps-protocol:
>>          now in ps-protocol, PREPARE and EXECUTE both generate one 
>> trace. This wasn't
>>          the case before: this is because PREPARE and EXECUTE, in 
>> mysqltest, are
>>          sent via COM_STMT_(PREPARE|EXECUTE), which are handled in 
>> dispatch_command():
>>          ** COM_STMT_PREPARE never goes through 
>> mysql_execute_command() so the
>>          preparation was not traced (now that we trace in 
>> check_prepared_statement(),
>>          it is traced)
>>          ** COM_STMT_EXECUTE executes the prepared command with 
>> mysql_execute_command()
>>          so the execution was traced and is still traced.
>>       @ mysql-test/r/optimizer_trace_no_prot.result
>>          SET is not traced anymore. PREPARE CALL neither (thus we see 
>> the trace
>>          of the statement before PREPARE CALL, which is a SELECT).
>>       @ mysql-test/r/optimizer_trace_ps_prot.result
>>          Now that PREPARE SELECT and EXECUTE SELECT
>>       @ mysql-test/t/optimizer_trace2.test
>>          Now that CALL is not traced anymore, the testcase needs to use
>>          a top-statement which is traced (SELECT STORED_FUNC() instead 
>> of CALL),
>>          in order to keep the test's intention.
>>       @ sql/opt_trace.h
>>          Updating comments: EXECUTE is not traced anymore, it's the 
>> executed statement
>>          which is (one level below, in terms of stack frames...).
>>       @ sql/opt_trace2server.cc
>>          - let opt_trace_start() limit tracing to certain SQL commands
>>          - it sounds like the similar check done in 
>> opt_trace_print_expanded_query()
>>          would become unnecessary, but alas no, because sometimes we 
>> open tables
>>          before executing the command (see comments).
>>       @ sql/sql_parse.cc
>>          The check on CF_DIAGNOSTIC_STMT is not needed, as 
>> opt_trace_start()
>>          now checks sql_command.
>>       @ sql/sql_prepare.cc
>>          To have "PREPARE<command>" be traced only if<command>  is in
> 
>> a certain set,
>>          we need to know<command>, which is possible only in 
>> check_prepared_statement();
>>          so we:
>>          - don't turn tracing on in mysql_execute_command() when 
>> command is SQLCOM_PREPARE
>>          (too early, not enough info)
>>          - check the _to-be-prepared_ command in 
>> check_prepared_statement() and decide then.
>>
>>      modified:
>>        mysql-test/include/optimizer_trace.inc
>>        mysql-test/r/optimizer_trace2.result
>>        mysql-test/r/optimizer_trace_no_prot.result
>>        mysql-test/r/optimizer_trace_ps_prot.result
>>        mysql-test/t/optimizer_trace2.test
>>        sql/opt_trace.h
>>        sql/opt_trace2server.cc
>>        sql/sql_parse.cc
>>        sql/sql_prepare.cc
> 
>  > === modified file 'mysql-test/t/optimizer_trace2.test'
>  > --- mysql-test/t/optimizer_trace2.test    2010-11-26 09:28:38 +0000
>  > +++ mysql-test/t/optimizer_trace2.test    2010-12-14 12:16:16 +0000
>  > @@ -8,18 +8,21 @@
>  > +# "distinct" is because in ps-protocol mode, we get 'select f1("c")'
>  > +# twice (one PREPARE, one EXECUTE), and we want a single result file
>  > +select distinct QUERY from information_schema.OPTIMIZER_TRACE|
>  >  delimiter ;|
> 
> jl: "single result file"?

fixed

>  > === modified file 'sql/opt_trace2server.cc'
>  > --- sql/opt_trace2server.cc    2010-11-26 09:28:38 +0000
>  > +++ sql/opt_trace2server.cc    2010-12-14 12:16:16 +0000
>  > @@ -56,4 +56,9 @@
>  > +/**
>  > +   Whether a SQL command qualifies for optimizer tracing.
>  > +   @param  sql_command  the command
>  > +*/
>  > +static bool sql_command_cannot_be_traced(enum enum_sql_command 
> sql_command)
>  > +{
> 
> jl: I prefer non-negated function names that you can negate
> programmatically, but I wount violently refuse to approve this patch.
> Consider sql_command_can_be_traced() instead.

fixed

>  > +  switch (sql_command)
>  > +  {
>  > +  case SQLCOM_SELECT: // includes EXPLAIN
>  > +  case SQLCOM_INSERT:
>  > +  case SQLCOM_INSERT_SELECT:
>  > +  case SQLCOM_REPLACE_SELECT:
>  > +  case SQLCOM_UPDATE:
>  > +  case SQLCOM_DELETE:
>  > +  case SQLCOM_UPDATE_MULTI:
>  > +  case SQLCOM_DELETE_MULTI:
>  > +    return false;
>  > +  default:
>  > +    /*
>  > +      Reasons reasons to not trace other commands:
> 
> jl: typo

fixed

>  > === modified file 'sql/sql_prepare.cc'
>  > --- sql/sql_prepare.cc    2010-08-18 10:18:27 +0000
>  > +++ sql/sql_prepare.cc    2010-12-14 12:16:16 +0000
>  > @@ -2095,10 +2110,18 @@
>  >      break;
>  >    }
>  >    if (res == 0)
>  > -    DBUG_RETURN(stmt->is_sql_prepare() ?
>  > -                FALSE : (send_prep_stmt(stmt, 0) || 
> thd->protocol->flush()));
>  > +  {
>  > +    res= stmt->is_sql_prepare() ?
>  > +      FALSE : (send_prep_stmt(stmt, 0) || thd->protocol->flush());
>  > +    goto end;
>  > +  }
>  >  error:
>  > -  DBUG_RETURN(TRUE);
>  > +  res= TRUE;
>  > +end:
>  > +  trace_command_steps.end();
>  > +  trace_command.end(); // must be closed before trace is ended below
>  > +  opt_trace_end(thd, started_optimizer_trace);
>  > +  DBUG_RETURN(res);
> 
> jl: This function would be easier to read if you replaced
> 
> goto error;
> 
> with
> 
> res= TRUE;
> goto end;
> 
> I had to read it a few times before I realized that due to
> 
> if (res == 0){}
> 
> we'll never reach the error label unless we follow a goto to it.

fixed.
And soon pushed.


-- 
Mr. Guilhem Bichot <guilhem.bichot@stripped>
Oracle / MySQL / Optimizer team, Lead Software Engineer
Bordeaux, France
www.oracle.com / www.mysql.com
Thread
Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3237)Jorgen Loland15 Dec
  • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3237)Guilhem Bichot17 Dec