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
