Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242) WL#4800

[Guilhem Bichot]

Hello,

I deleted all "ok, no comment" points.

Jorgen Loland a écrit, Le 14.01.2011 12:49:
>  >> === modified file 'sql/opt_trace.h'
>  >> --- a/sql/opt_trace.h 2010-12-03 15:26:41 +0000
>  >> +++ b/sql/opt_trace.h 2010-12-13 14:03:16 +0000
>  >> @@ -372,13 +372,18 @@ public:
>  >> @li "greedy_search" = the greedy search for a plan
>  >> @li "range_optimizer" = the cost analysis of accessing data through
>  >> ranges in indices
>  >> + @li "dynamic_range" = the range optimization performed for each
>  >> record + when access method is dynamic range (off by default)
>  >
>  > "off by default" is not needed (we didn't mention for other features
>  > what is on and what is off by default). By the way, it's on by default
>  > (see diff of opt_trace.cc). I suggest to have the same default for all
>  > "repeating" features i.e. dynamic_range and repeated_subselect. I wonder
>  > whether it should be off or on:
>  > - off: less verbose trace, no risk of spamming
>  > - on: nothing hidden by default; --optimizer_trace_max_mem_size has a
>  > low default to catch any accidental explosion due to this "on" setting.
>  >
>  > Today I favour "on", so that by default we don't miss any info when we
>  > debug.
> 
> I agree. Everything is on by default.

But in the latest patch, repeated_subselect is still off by default (see 
optimizer_trace_features_basic.result).

> If this creates a too large
> trace the person doing debugging can turn things off and retry.
> 
>  >> === modified file 'sql/sql_select.cc'
>  >> --- a/sql/sql_select.cc 2010-12-09 07:34:09 +0000
>  >> +++ b/sql/sql_select.cc 2010-12-13 14:03:16 +0000
>  >> @@ -9839,6 +9852,10 @@ static bool make_join_select(JOIN *join,
>  >> join->best_positions[i].records_read &&
>  >> !(join->select_options & OPTION_FOUND_ROWS)))
>  >> {
>  >> + Opt_trace_object trace_table(join->thd->opt_trace, +
>  >> "rechecking_index_usage");
>  >> + trace_table.add_utf8_table(tab->table);
>  >
>  > we already printed the table's name earlier in this function. This gives
>  > duplication:
>  > {
>  > "database": "test",
>  > "table": "t2",
>  > "rechecking_index_usage": {
>  > "database": "test",
>  > "table": "t2",
>  > "range_analysis": {
> 
> Left for now until we sort the issue below.

ok

>  > But wait, this is interesting. I have the following issue (in the
>  > current wl4800 tree): make_join_select() looks like this:
>  >
>  > open array in trace;
>  > for (take tables from first to last)
>  > {
>  > create object in trace for this table;
>  > write table's name to trace;
>  > determine attached condition;
>  > write attached condition to trace;
>  > }
>  >
>  > But "determine attached condition" contains pushdown_on_conditions()
>  > which may attach condition to tables located *before* the one which the
>  > iteration is positioned on. And as I have already printed the condition
>  > for this earlier table, the earlier printed condition is now incomplete.
>  > The example is "t1 LEFT JOIN (t2 LEFT JOIN t3 ON ...) ON ...": when
>  > positioned on t3, bits of the ON conditions get attached to t1 and t2.
>  > As a solution, I considered switching to this "summarizing" pseudocode:
>  >
>  > for (take tables from first to last)
>  > {
>  > determine attached condition;
>  > }
>  > open array in trace;
>  > for (take tables from first to last)
>  > {
>  > create object in trace for this table;
>  > write table's name to trace;
>  > write attached condition to trace;
>  > }
>  > But "determine attached condition" calls the range opt (right below in
>  > your patch), so this blows up (range opt tracing without the proper
>  > context => crash).
>  > So maybe we need:
>  >
>  > open array in trace, for key "attached_conditions_computation"
>  > for (take tables from first to last)
>  > {
>  > determine attached condition; as part of it, if using the range opt
>  > then create proper enclosing context with table's name as you do in the
>  > patch
>  > }
>  > close array
>  > open array in trace, for key "attached_conditions_summary"
>  > for (take tables from first to last)
>  > {
>  > create object in trace for this table;
>  > write table's name to trace;
>  > write attached condition to trace;
>  > }
>  >
>  > If you find this is ok, I can handle implementation.
> 
> If I understand correctly, you want to execute range optimization
> all over again for all tables?

No. Current code does:
open array in trace;
for (take tables from first to last)
{
create object in trace for this table;
write table's name to trace;
determine attached condition;
write attached condition to trace;
}
and I would change to
open array in trace, for key "attached_conditions_computation"
for (take tables from first to last)
{
determine attached condition; as part of it, if using the range opt
then create proper enclosing context with table's name as you do in the
patch
}
close array
open array in trace, for key "attached_conditions_summary"
for (take tables from first to last)
{
create object in trace for this table;
write table's name to trace;
write attached condition to trace;
}
so the second for() loop only does tracing, nothing else.
The first loop does no tracing except if range opt is used, in which 
case tracing is done by your:
             Opt_trace_object trace_table(join->thd->opt_trace,
                                          "rechecking_index_usage");
             trace_table.add_utf8_table(tab->table);

> Isn't there a chance this changes
> the QEP?

>  >> @@ -18372,6 +18408,21 @@ join_read_prev_same(READ_RECORD *info)
>  >> static int
>  >> join_init_quick_read_record(JOIN_TAB *tab)
>  >> {
>  >> + /* This is for QS_DYNAMIC_RANGE, i.e., "Range checked for each
>  >
>  > /* should be alone on its line
>  >
>  >> + record". The trace for the range analysis below this point will
>  >> + be printed with different ranges for every record to the left of
>  >> + this table in the join.
>  >> + */
>  >> + // Guilhem: should we do like for REPEATED_SUBQUERY feature here,
>  >> + // i.e., show the first optimization but not 2nd...Nth? That will
>  >> + // require a new variable similar to executed_before (see
>  >> + // changes in item_subselect.cc
>  >
>  > With the REPEATED_SUBQUERY flag I can choose between "only first" and
>  > "all".
>  > With the DYNAMIC_RANGE flag I can choose between "none" or "all"; I
>  > would prefer a choice between "only first" or "all", like with
>  > REPEATED_SUBQUERY.
> 
> join_init_quick_read_record is a free function. I need a location
> to put the "traced_before" variable. Suggestions?

make it a member of JOIN_TAB::select?