List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:November 22 2010 10:03am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3233)
WL#5594
View as plain text  
On 11/20/2010 02:57 PM, Guilhem Bichot wrote:
> [resending without forgetting attachments, this time]
>
> Hello Jorgen,
>
> very close to a push now :-)

:-)

>> === modified file 'sql/opt_range.cc'
>> --- a/sql/opt_range.cc 2010-10-22 13:36:50 +0000
>> +++ b/sql/opt_range.cc 2010-11-17 12:27:39 +0000
>
>> @@ -2277,31 +2278,41 @@ public:
>> ha_rows quick_prefix_records;
>> public:
>>
>> -#ifdef OPTIMIZER_TRACE
>> void trace_basic_info(const PARAM *param, Opt_trace_object *trace) const
>> {
>> +#ifdef OPTIMIZER_TRACE
>> trace->add_str("type", "index_group").
>> - add_str("index", index_info->name, strlen(index_info->name)).
>> - add_str("group_attribute", min_max_arg_part->field->field_name,
>> - strlen(min_max_arg_part->field->field_name)).
>> - add("min_aggregate", have_min).
>> + add_str("index", index_info->name, strlen(index_info->name));
>> + if (min_max_arg_part)
>> + trace->add_str("group_attribute",
> min_max_arg_part->field->field_name,
>> + strlen(min_max_arg_part->field->field_name));
>> + else
>> + trace->add_str("group_attribute", "<none>");
>
> GB This "else" branch is not covered in tests. Do you know when it may
> be executed?

I found this when running --opt-trace-protocol on subquery_none.test, this query:

SELECT DISTINCT topic FROM t2 WHERE NOT EXISTS(SELECT * FROM t3 
WHERE\nnumeropost=topic)

I added a test case to optimizer_trace_range.test

> Instead of
> trace->add_str("group_attribute", "<none>");
> I propose those two alternatives:
> 1) trace->add_null("group_attribute");
> (JSON's "null" converts to Python's None which "is frequently used to
> represent the absence of a value", says the Python doc; it also converts
> to Perl's "undef")
> 2) or no code at all (I mean, no "else" branch): if group_attribute is
> not put in the object, it means the object has no group_attribute.

In almost all cases, I would agree that 2) was the better choice. However, 
personally I found it strange that a GROUP quick select did not have a group 
attribute. I therefore wanted to explicitly output this information. I then 
tried to use add_null(), but there's no such function. If you intend to add one: 
could you update this tracepoint as well?

>> + imerge_cost += rowid_comp_cost;
>
> GB no space before += (I know it's annoying; maybe one day we'll
> eliminate this rule from the coding style?).

Well, I'm getting used to greping for "<my_var>=" to find assignments (but 
setter methods would be much more effective).

>> === modified file 'sql/sql_select.cc'
>> --- a/sql/sql_select.cc 2010-11-11 13:25:53 +0000
>> +++ b/sql/sql_select.cc 2010-11-17 12:27:39 +0000
>
>> @@ -11308,6 +11311,8 @@ make_join_readinfo(JOIN *join, ulonglong
>> tab->select->quick->index != MAX_KEY && !
> tab->table->key_read)
>> push_index_cond(tab, tab->select->quick->index, icp_other_tables_ok);
>> }
>> + trace_refine_table.add_str("scan_type",
>> + tab->type==JT_NEXT ? "index" : "table");
>
> GB There is another place where we set tab->type to JT_NEXT, it's
> test_if_skip_sort_order(); in theory we would also trace that; but that
> is an ORDER-BY-related function, and tracing decisions around ORDER BY
> will hopefully be a WL by itself (that's what I concluded when fixing a
> bug around ORDER BY vs index scan: it's not trivial to understand the
> myriad of decisions which handle ORDER BY, and to trace them).

I read the doc before deciding to not trace this. My impression from the doc is 
that this code is for reverting to a decision already made. Comment a few lines up:

         /*
            If ref_key used index tree reading only ('Using index' in EXPLAIN),
            and best_key doesn't, then revert the decision.
         */

Anyway, as you say, we'll have to trace ordering at some point and then we can 
investigate in more detail.

> GB optimizer_trace_ps_prot.result needs to be updated too; I suggest
> diff-ing the result file with optimizer_trace_no_prot.result to verify
> that new differences are reasonable.
>
> GB if you apply a temporary workaround for the "uninitialized memory in
> print_keyuse" problem/crash, do you get any crash with
> ./mtr --opt-trace-protocol
> in your tree?

Lots and lots of JSON syntax error asserts for subqueries.

> GB There remains one NO_OPT_TRACE_IN_RANGE_OPT in opt_range.cc, have you
> decided how to eliminate it?

I never saw the problem that required us to compile this in. This code has never 
been active in my trees, so to me it can be removed. Since you don't want it 
either, it's gone...

> GB I have scanned our previous emails for remaining points (I'm starting
> to loose track :-), here is the exhaustive list below.
>
> GB http://lists.mysql.com/commits/123954
>
> GB http://lists.mysql.com/commits/124324 (from yesterday)
>
> GB as requested the "trace" parameter of "virtual void
> trace_basic_info()" was renamed to trace_object, but this wasn't done in
> the implementations of this function in derived classes

Oops... done

> GB there was a suggestion that
> "is_something": true/false
> should be replaced with
> "something": true/false
> (like "chosen" or "usable"); how about applying it to
> "is_distinct_query" and "is_index_scan" ?

Done. Also fixed "is_rollup"

> GB there was a "one trace per row scanned" email thread: we didn't reach
> a conclusion, we need to fix this (there are problems with "range
> checked for each record" and for subqueries). Fixing this can happen
> after your push, I have put it in WL4800_TODO.txt. Could you please
> share problematic queries (those which produce too much tracing at
> execution) which you have identified? By email or by committing them
> into optimizer_trace_bugs.test.

I have an example below, but I'd prefer to add problematic queries to our tests 
when working on subquery tracing.

Here's an example from subselect_none.test:

------------ Apply this ------------
=== modified file 'sql/sql_select.cc'
--- sql/sql_select.cc	2010-11-17 12:27:39 +0000
+++ sql/sql_select.cc	2010-11-22 09:31:33 +0000
@@ -2797,6 +2797,8 @@ JOIN::exec()
    int      tmp_error;
    DBUG_ENTER("JOIN::exec");

+  Opt_trace_object trace_wrapper(thd->opt_trace);
+  Opt_trace_object trace_exec(thd->opt_trace, "join_exec");
    thd_proc_info(thd, "executing");
-------------------------------------

./mtr --mem subquery_none --opt-trace-protocol

look for tracing of this query and go to the join_exec part of it. You'll see it 
executes the subselect for each record in the outer select:

SELECT `pk` FROM t1 AS OUTR WHERE `int_key` = ALL (
SELECT `int_key` FROM t1 AS INNR WHERE INNR . `pk` >= 9
);

-- 
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
Thread
bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3233)WL#5594Guilhem Bichot20 Nov
  • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3233)WL#5594Jorgen Loland22 Nov
    • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3233)WL#5594Guilhem Bichot22 Nov