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
