List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:December 15 2010 8:50am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3237)
View as plain text  
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"?

 > === 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.

 > +  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

 > === 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.

-- 
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
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