List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:November 19 2010 1:40pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3233)
WL#5594
View as plain text  
Hello Jorgen,

very close to a push now :-)

Jorgen Loland a écrit, Le 17.11.2010 13:27:
> #At
> file:///export/home/jl208045/mysql/wl4800/mysql-next-mr-opt-backporting-wl4800-patchcleanup/
> based on revid:jorgen.loland@stripped
> 
>  3233 Jorgen Loland	2010-11-17
>       WL#5594 - Add optimizer traces to the range optimizer
>       
>       Minor changes as requested by reviewer.

> === modified file 'mysql-test/r/optimizer_trace_no_prot.result'
> --- a/mysql-test/r/optimizer_trace_no_prot.result	2010-11-11 13:25:53 +0000
> +++ b/mysql-test/r/optimizer_trace_no_prot.result	2010-11-17 12:27:39 +0000
> @@ -5416,6 +5773,19 @@ select * from t6 where d in (select f1()
>                  }
>                ] /* attached_conditions */
>              } /* attaching_conditions_to_tables */
> +          },
> +          {
> +            "refine_plan": {
> +              "refine_plan_for_table": {
> +                "database": "test",
> +                "table": "t6"
> +              } /* refine_plan_for_table */,
> +              "refine_plan_for_table": {
> +                "database": "test",
> +                "table": "t2",
> +                "scan_type": "table"
> +              } /* refine_plan_for_table */
> +            } /* refine_plan */

GB WL4800_validate_json.py caught this one: object "refine_plan" has two
identical keys ("refine_plan_for_table"). The previous_key test didn't 
catch it, having a bug (fix is in the attached patch). I suggest instead 
this layout:
             "refine_plan": [
               {
                 "database": "test",
                 "table": "t6"
               },
               {
                 "database": "test",
                 "table": "t2",
                 "scan_type": "table"
               }
             ] /* refine_plan */

The object for t6 does not carry useful info; but this will change when
we start tracing check_join_cache_usage().

> === 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?
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.

> +    trace->add("min_aggregate", have_min).
>        add("max_aggregate", have_max).
>        add("distinct_aggregate", have_agg_distinct).
> -      add("key_parts_used_for_access", used_key_parts).      
>        add("records", records).
>        add("cost", read_cost);

> @@ -2740,7 +2750,7 @@ int SQL_SELECT::test_quick_select(THD *t
>                               " trying to construct index_merge"));
>            List_iterator_fast<SEL_IMERGE> it(tree->merges);
>            {

GB the '{' above is a new scope compared to the opt-backporting tree; I 
think it's not needed, and then the additional indentation of the block 
can be undone.

> -            Opt_trace_array trace_idx_merge(thd->opt_trace, 
> +            Opt_trace_array trace_idx_merge(trace, 
>                                              "analyzing_index_merge",
>                                              Opt_trace_context::RANGE_OPTIMIZER);
>              while ((imerge= it++))

> @@ -4134,56 +4144,60 @@ TABLE_READ_PLAN *get_best_disjunct_quick
>                                               sizeof(TRP_RANGE*)*
>                                               n_child_scans)))
>      DBUG_RETURN(NULL);
> -  {
> -    /*
> -      Collect best 'range' scan for each of disjuncts, and, while doing so,
> -      analyze possibility of ROR scans. Also calculate some values needed by
> -      other parts of the code.
> -    */
> -    Opt_trace_array ota(trace,"indices_to_merge");
> -    for (ptree= imerge->trees, cur_child= range_scans;
> -         ptree != imerge->trees_next;
> -         ptree++, cur_child++)
> +  // Note: to_merge.end() is called to close this object after this for-loop.
> +  Opt_trace_array to_merge(trace,"indices_to_merge");

GB space after ,

> +  /*
> +    Collect best 'range' scan for each of disjuncts, and, while doing so,

> @@ -4220,10 +4234,10 @@ TABLE_READ_PLAN *get_best_disjunct_quick
>        Add one ROWID comparison for each row retrieved on non-CPK scan.  (it
>        is done in QUICK_RANGE_SELECT::row_in_ranges)
>       */
> -    
> -    imerge_cost += non_cpk_scan_records / TIME_FOR_COMPARE_ROWID;
> -    trace_best_disjunct.add("cost_of_mapping_rowid_in_non_cpk_scan", 
> -                            non_cpk_scan_records / TIME_FOR_COMPARE_ROWID);
> +    double rowid_comp_cost= non_cpk_scan_records / TIME_FOR_COMPARE_ROWID;

GB "const double" if it would not make the line too long.


> +    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?).

> +    trace_best_disjunct.add("cost_of_mapping_rowid_in_non_clustered_pk_scan",
> +                            rowid_comp_cost);
>    }
>  
>    /* Calculate cost(rowid_to_row_scan) */

> @@ -10139,47 +10152,61 @@ get_best_group_min_max(PARAM *param, SEL
>    Item_field *item_field;
>    bool is_agg_distinct;
>    List<Item_field> agg_distinct_flds;
> +  KEY *cur_index_info= table->key_info;
> +  KEY *cur_index_info_end= cur_index_info + table->s->keys;
> +  /* Cost-related variables for the best index so far. */
> +  double best_read_cost= DBL_MAX;
> +  ha_rows best_records= 0;
> +  SEL_ARG *best_index_tree= NULL;
> +  ha_rows best_quick_prefix_records= 0;
> +  uint best_param_idx= 0;
> +  List_iterator<Item> select_items_it;
> +
> +  const uint pk= param->table->s->primary_key;
> +  SEL_ARG *cur_index_tree= NULL;
> +  ha_rows cur_quick_prefix_records= 0;
> +  uint cur_param_idx=MAX_KEY;

GB I understand why all this had to move up (because of "jump to label 
crosses initialization" errors because of the new goto-s which I 
suggested). Sad that it forces us to do all those initializations, which 
are a waste if we leave the function in one of the early tests (like "no 
join"). In particular, the dummy initialization of select_items_it is a 
waste (its pointers are set to 0 in the no-args constructor and then set 
properly further down before the list is actually used).
But I see no better solution (without massive re-indentation).
However, there are a few declarations which don't have to move so much 
up, I'm attaching a patch. In the end, only select_items_it and the 
best_* variables are moved up.

> === modified file 'sql/sql_delete.cc'
> --- a/sql/sql_delete.cc	2010-08-12 00:26:10 +0000
> +++ b/sql/sql_delete.cc	2010-11-17 12:27:39 +0000
> @@ -190,6 +190,13 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>    select=make_select(table, 0, 0, conds, 0, &error);
>    if (error)
>      DBUG_RETURN(TRUE);
> +
> +  { // Enter scope for optimizer trace wrapper

GB It's only twenty lines before we close this scope, so you can indent
those twenty by two characters (I also checked whether it was possible
to use end() but it doesn't work because of goto). Same for sql_update.cc.

> +  Opt_trace_object wrapper(thd->opt_trace);
> +  wrapper.add_str("database", table_list->db,
> +                  strlen(table_list->db)).
> +    add_str("table", table_list->alias, strlen(table_list->alias));
> +

> === 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).

>        break;
>      case JT_FT:
>        break;
> 

GB I looked at the diff of opt_range.cc compared to 
opt-backporting-wl4800, to see whether some massive re-indentation, due 
to new scopes, could be avoided with Opt_trace_struct::end(). I see two 
blocks which could be candidates for this:
- the one of "analyzing_roworder_scans"
- the one of "intersecting_indices"
It is up to you to decide, do as you like best.

GB in opt_range.cc, after comment
"Ok, found the best ROR-intersection of non-CPK key scans" (line 5071), 
we create the same object in all 3 branches of the if()/else/else. We 
could create it before the first if() and close it with end(). Or not 
use end() but add a scope outside and declare the object at the start of 
this new scope.

GB when testing thd->opt_trace (in opt_range.cc and sql_select.cc) I 
suggest using unlikely(), like

if (unlikely(thd->opt_trace != NULL) && thd->opt_trace->is_started())

(as is done in opt_trace.h). Because thd->opt_trace will be NULL largely 
more than 99% of time on production systems (production system => no 
debug build, thousands or millions of queries by applications, optimizer 
trace used only by humans for troubleshooting).

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?

GB in opt_range.cc, comment starting with
"/* Byte 0 of key is the null-byte"
should have /* alone on its line.

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

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

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

GB I think we decided for "roworder" over "ror": there is one left 
("ror_is_covering") in opt_range.cc

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.

Thread
bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3233) WL#5594Jorgen Loland17 Nov
  • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3233)WL#5594Guilhem Bichot19 Nov