List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:January 14 2011 11:49am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242)
WL#4800
View as plain text  
Guilhem,

Thank you for the review. New commit coming soon...

On 01/04/2011 03:03 PM, Guilhem Bichot wrote:
 > Hello,
 >
 > First, I'm grateful that you took the time to fix all issues; I see
 > there were quite a few in my own code :-/
 >
 > Jorgen Loland a écrit, Le 13.12.2010 15:03:
 >> === modified file 'mysql-test/r/optimizer_trace_ps_prot.result'
 >> @@ -870,16 +1022,16 @@ select (@query:=QUERY)+NULL, (@trace:=TR
 >> NULL NULL
 >> select length(@trace);
 >> length(@trace)
 >> -8631
 >> +10628
 >> set optimizer_trace_max_mem_size=9000;
 >> select length(@query)+length(@trace) > @@optimizer_trace_max_mem_size;
 >> length(@query)+length(@trace) > @@optimizer_trace_max_mem_size
 >> -0
 >> +1
 >> SELECT * FROM t5 WHERE 5 IN (SELECT 1 FROM t6 WHERE d = ifnull(c,null)
 >> UNION SELECT 2 FROM t6 WHERE d = ifnull(c,null));
 >> c
 >> select (@missing_bytes:=missing_bytes_beyond_max_mem_size) from
 >> information_schema.OPTIMIZER_TRACE;
 >> (@missing_bytes:=missing_bytes_beyond_max_mem_size)
 >> -0
 >> +1756
 >
 > Because the trace grew (due to more tracepoints), this test portion does
 > not test anymore what it intends to. You can check the comments in
 > optimizer_trace.inc, they explain what we want. 0 should still be the
 > result.
 >
 > I'll re-check this result file more thoroughly after the first issues
 > are addressed.
 > What I also do is comparing the result file of no_prot and ps_prot to
 > see whether the differences all make sense.

please do.

 >> === modified file 'mysql-test/t/optimizer_trace_range.test'
 >> --- a/mysql-test/t/optimizer_trace_range.test 2010-11-22 09:59:17 +0000
 >> +++ b/mysql-test/t/optimizer_trace_range.test 2010-12-13 14:03:16 +0000
 >> @@ -168,4 +168,46 @@ SELECT * FROM information_schema.OPTIMIZ
 >>
 >> DROP TABLE t1;
 >>
 >> -set optimizer_trace=default;
 >> +--echo
 >> +CREATE TABLE t1 (a int, b int, PRIMARY KEY (a), KEY bkey (b));
 >> +INSERT INTO t1 VALUES (1,2),(3,2),(2,2),(4,2),(5,2),(6,2),(7,2),(8,2);
 >> +INSERT INTO t1 SELECT a + 8, 2 FROM t1;
 >> +INSERT INTO t1 SELECT a + 16, 1 FROM t1;
 >> +
 >> +--echo
 >> +# Make quick select for ordering inside test_if_skip_sort_order
 >> +EXPLAIN SELECT * FROM t1 WHERE b BETWEEN 1 AND 2 ORDER BY b,a;
 >
 > In the trace for EXPLAIN I don't see the quick select of
 > test_if_skip_sort_order() (nothing which says
 > "records_estimation_for_index_ordering")?

Strange... Not sure now that happened. But there are numerous
other queries that do this, so I removed this test.

 >> +
 >> thd->opt_trace->feature_enabled(Opt_trace_context::REPEATED_SUBSELECT) :
 >> + false;
 >
 > This should be in #ifdef OPTIMIZER_TRACE or it won't compile in some
 > builds.

ifdef'ed the whole block related to tracing.

 >> + Opt_trace_disable_I_S disable_trace_wrapper(thd->opt_trace,
 >> disable_trace);
 >> + executed_before= true;
 >
 > Maybe executed_before should be reset in Item_subselect::cleanup() (see
 > how Item_in_subselect::cleanup() resets "first_execution")?

good point

 > I wondered whether "executed_before" is needed or the existing
 > "first_execution" can be reused instead (they do have similar namings,
 > so why not). But in fact, first_execution seems dedicated to subquery
 > materialization...
 > Still they have similar namings and it's not good to add confusion.
 > Maybe "executed_before" should be named "traced_before" or
 > "already_traced"?

traced_before it is.


 >> === 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. 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
 >> @@ -2663,9 +2663,6 @@ JOIN::optimize()
 >> setup_subq_exit:
 >>
 >> DBUG_ASSERT(zero_result_cause != NULL);
 >> - trace_steps.end(); // because all steps are done
 >> - Opt_trace_object trace_empty_result(trace, "empty_result");
 >> - trace_empty_result.add_alnum("cause", zero_result_cause);
 >> /*
 >> Even with zero matching rows, subqueries in the HAVING clause may
 >> need to be evaluated if there are aggregate functions in the
 >> @@ -2674,6 +2671,11 @@ setup_subq_exit:
 >> */
 >> if (setup_subquery_materialization())
 >> DBUG_RETURN(1);
 >> +
 >> + trace_steps.end(); // because all steps are done
 >> + Opt_trace_object trace_empty_result(trace, "empty_result");
 >> + trace_empty_result.add_alnum("cause", zero_result_cause);
 >
 > I guess this moved down because setup_subquery_materialization() could
 > generate traces?

correct

 >> @@ -4737,7 +4745,6 @@ make_join_statistics(JOIN *join, TABLE_L
 >> join->const_table_map= 0;
 >>
 >> {
 >> - Opt_trace_object trace_wrapper(trace);
 >> for (POSITION *p_pos= join->positions, *p_end= p_pos + const_count;
 >> p_pos < p_end ;
 >> p_pos++)
 >> @@ -4921,6 +4928,7 @@ make_join_statistics(JOIN *join, TABLE_L
 >> }
 >> }
 >>
 >> + Opt_trace_object trace_wrapper(trace);
 >
 > No idea why this moved down, but probably the earlier code, made of
 > join_read_const_table(), cannot generate tracing nowadays.

To be honest, I don't remember

 >> @@ -7611,17 +7619,22 @@ choose_plan(JOIN *join, table_map join_t
 >> jtab_sort_func, (void*)join->emb_sjm_nest);
 >> join->cur_sj_inner_tables= 0;
 >>
 >> - if (straight_join)
 >> {
 >> - optimize_straight_join(join, join_tables);
 >> - }
 >> - else
 >> - {
 >> - if (search_depth == 0)
 >> - /* Automatically determine a reasonable value for 'search_depth' */
 >> - search_depth= determine_search_depth(join);
 >> - if (greedy_search(join, join_tables, search_depth, prune_level))
 >> - DBUG_RETURN(TRUE);
 >> + Opt_trace_object wrapper(join->thd->opt_trace);
 >> + Opt_trace_array trace_plan(join->thd->opt_trace,
 >> "considered_execution_plans",
 >> + Opt_trace_context::GREEDY_SEARCH);
 >
 > good idea, so straight join tracing can be disabled now!

Technically yes, but I don't think it should. See below

 >> + trace_table.add_utf8_table(s->table);
 >> +
 >> /* Find the best access method from 's' to the current partial plan */
 >> best_access_path(join, s, join_tables, idx, FALSE, record_count,
 >> join->positions + idx, &loose_scan_pos);
 >> @@ -7833,6 +7849,8 @@ optimize_straight_join(JOIN *join, table
 >> advance_sj_state(join, join_tables, s, idx, &record_count, &read_time,
 >> &loose_scan_pos);
 >>
 >> + trace_table.add("cost_for_plan", read_time).
 >> + add("records_for_plan", record_count);
 >
 > with STRAIGHT JOIN, nobody cares for cost as the plan is fixed by the
 > user (there is normally no cost-based choice). But maybe printing
 > cost/records here is for uniformity with greedy_search()?
 > Or it could serve for answering this question: I want to know the cost
 > of plan t1-t2-...-t20 without having the greedy search algo take minutes
 > to explore the full space, so I use STRAIGHT JOIN and read the cost.

I think we should provide this info. It's not a lot...

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

 > 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? Isn't there a chance this changes
the QEP?

 >> + Opt_trace_disable_I_S disable_trace_wrapper(trace, +
 >> !conds->with_subselect);
 >> + Opt_trace_array trace_subselect(trace,
 >> "evaluate_subselect_cond_steps");
 >> + conds= build_equal_items(join->thd, conds, NULL, join_list,
 >> + &join->cond_equal);
 >> + }
 >> trace_cond.add("after_equality_propagation", conds);
 >> }
 >> - /* change field = field to field = const for each found field =
 >> const */
 >> - propagate_cond_constants(thd, (I_List<COND_CMP> *) 0, conds, conds);
 >> +
 >> + {
 >> + Opt_trace_disable_I_S disable_trace_wrapper(trace, +
 >> !conds->with_subselect);
 >
 > I imagine this silencer is so that if there is no subselect we prefer to
 > not print "evaluate_subselect_cond_steps" ? Or is it that if there is no
 > subselect, something else would be printed but we want to silence it?

The former. If there are no subselects, no tracing is done by
these method calls. If there are subselects, it is possible that
they will be executed and thereby create tracing. So this trace
point is sort of a wrapper for that subselect execution.

 >> @@ -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?

 >> + Opt_trace_object wrapper(tab->join->thd->opt_trace);
 >> + Opt_trace_object trace_table(tab->join->thd->opt_trace,
 >> + "records_estimation_per_record",
 >
 > In tests there is no display of this information (only with "..."), this
 > is not really covered.

See optimizer_trace_range.result, trace starting like this:

SET optimizer_trace_features="greedy_search=off,dynamic_range=on";
EXPLAIN SELECT 1 FROM
(SELECT 1 FROM t2,t1 WHERE b < c GROUP BY 1 LIMIT 1) AS d2;

 >
 >> + Opt_trace_context::DYNAMIC_RANGE);
 >> + trace_table.add_utf8_table(tab->table);
 >> +
 >> if (test_if_quick_select(tab) == -1)
 >> return -1; /* No possible records */
 >> return join_init_read_record(tab);
 >> @@ -20013,6 +20064,12 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
 >> key_map new_ref_key_map; // Force the creation of quick select
 >> new_ref_key_map.set_bit(new_ref_key); // only for new_ref_key.
 >>
 >> + Opt_trace_context * const trace= tab->join->thd->opt_trace;
 >> + Opt_trace_object trace_wrapper(trace);
 >> + Opt_trace_object trace_recest(trace, +
 >> "records_estimation_for_index_ordering");
 >
 > In tests there is no coverage of this information.
 > I imagine it is covered when one runs the full testsuite with
 > --opt-trace-protocol...
 > It would be good to have, in one optimizer_trace* test, coverage for this.
 > Because I suspect people will rarely run with --opt-trace-protocol (I
 > wonder whether we should have a special run in pb2, in our team tree,
 > which runs the full suite with --opt-trace-protocol and verifies that
 > there is no crash).

See optimizer_trace_range.result, trace starting like this:
EXPLAIN SELECT * FROM t1 WHERE i1 > '2' ORDER BY i1, i2;

 > Note, for packets of similar tracepoints like we have in the range opt:
 > if (x)
 > put "blah" to trace
 > if (y)
 > put "foo" to trace
 > we don't need to cover all. I care more for tracing of "unique"
 > tracepoints. I also care for not causing an effort bigger than a few
 > hours on your side, so let me know if I'm asking too much.

No, these trace points should be tested and, AFAICT, already are.

-- 
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
Thread
bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242) WL#4800Jorgen Loland21 Dec
  • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242)WL#4800Guilhem Bichot30 Dec
    • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242)WL#4800Guilhem Bichot30 Dec
    • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242)WL#4800Jorgen Loland3 Jan
Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242)WL#4800Jorgen Loland5 Jan
  • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242)WL#4800Guilhem Bichot11 Jan
Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242)WL#4800Jorgen Loland14 Jan
  • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242)WL#4800Guilhem Bichot14 Jan
    • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242)WL#4800Jorgen Loland14 Jan
      • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3242)WL#4800Guilhem Bichot17 Jan