From: Jorgen Loland Date: December 15 2010 8:50am Subject: Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3237) List-Archive: http://lists.mysql.com/commits/126892 Message-Id: <4D088156.9060109@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit 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" be traced only if is in a certain set, > we need to know, 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