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

Jorgen Loland a écrit, Le 22.10.2010 09:04:
 > Hi Guilhem
 >
 > Due to the sheer size of this patch, I have removed comments from you
 > which I agree and would only reply "done". Places where I disagree or
 > have further information can be found inline (size is now down to 1700
 > lines!).

GB only 650 lines now!

 >  > Jorgen Loland a écrit, Le 07.10.2010 11:02:
 >  >> #At
 >
file:///export/home/jl208045/mysql/wl4800/mysql-next-mr-opt-backporting-wl4800-patchcleanup/
 > based on revid:guilhem@stripped
 >  >>
 >  >>  3219 Jorgen Loland    2010-10-07
 >  >>       WL#5594 - Add optimizer traces to the range optimizer
 >  >>
 >  >>       Add optimizer trace points to the range optimizer.
 >  >>      @ mysql-test/include/optimizer_trace.inc
 >  >>         Added tests to cover range optimizer
 >  >>      @ mysql-test/r/optimizer_trace_no_prot.result
 >  >>         Added tests to cover range optimizer. Also adds range
 > information to trace of existing queries
 >  >>      @ sql/handler.cc
 >  >>         Add tracepoints to range optimizer
 >  >>      @ sql/opt_range.cc
 >  >>         Add tracepoints to range optimizer
 >  >>      @ sql/opt_range.h
 >  >>         Add tracepoints to range optimizer
 >  >>      @ sql/sql_select.cc
 >  >>         Add tracepoints to range optimizer
 >  >
 >  >GB Ah! now I see... this patch is about adding tracepoints to the range
 >  >optimizer!
 >
 > You're not in favor of tagging each individual file?

GB ;-) For a patch of this size, I am all in favour. But I find the
per-file commit comments in the new patch more informative because
some don't only repeat the global comment.

 >  >> === modified file 'mysql-test/r/optimizer_trace_no_prot.result'
 >  >> --- a/mysql-test/r/optimizer_trace_no_prot.result    2010-10-05
 > 13:19:55 +0000
 >  >> +++ b/mysql-test/r/optimizer_trace_no_prot.result    2010-10-07
 > 09:02:31 +0000
 >  >> @@ -459,9 +615,13 @@ QUERY_ID    TRACE    MISSING_BYTES_BEYOND_MAX_
 >  >>                  "best_access_path": {
 >  >>                    "considered_access_paths": [
 >  >>                      {
 >  >> -                      "access_type": "table scan",
 >  >> -                      "cost": 10,
 >  >> -                      "records": 0,
 >  >> +                      "access_type": "scan",
 >  >> +                      "using_range_access": false,
 >  >
 >  >GB I wonder how a range access can be used when we are doing a "scan"
 >  >(which means table scan)? I think range access is only when there is
 >  >an index...?
 >
 > Changed to print either
 >
 > "access_type": "scan"
 > or
 > "access_type": "range"
 >
 > but note that scan can also be index scan.

GB Looks like you are right, scan can be index scan. But in the trace,
the opposite of "access_type:scan" is "access_type:index"; maybe this
latter one is misleading: it is about an index, but not about an index
scan (it is "scan" which is about an "index scan" possibly). Then what
is "access_type:index" about? Is it "ref" access, or possibly
something else? Maybe it can only be "ref" access: I see that if we
set best_key, we then set POSITION::key, which is described in
sql_select.h as non-NULL only for "ref" methods. If that's only for
"ref" access, maybe we should say "ref" instead of "index" for
"access_type".

 >  >> @@ -4252,9 +4885,53 @@ QUERY_ID    TRACE    MISSING_BYTES_BEYOND_MAX_
 >  >>              "records_estimation": [
 >  >>                {
 >  >>                  "table": "D",
 >  >> -                "more_range_optimizer_trace": {
 >  >> -                  "TODO?": "yes!"
 >  >> -                } /* more_range_optimizer_trace */
 >  >> +                "range_analysis": {
 >  >> +                  "table_scan": {
 >  >> +                    "records": 4,
 >  >> +                    "cost": 4.9068
 >  >> +                  } /* table_scan */,
 >  >> +                  "potential_range_indices": [
 >  >> +                    {
 >  >> +                      "index": "d",
 >  >> +                      "key_parts": [
 >  >> +                        "d"
 >  >> +                      ] /* key_parts */
 >  >> +                    }
 >  >> +                  ] /* potential_range_indices */,
 >  >> +                  "best_full_index_scan": {
 >  >> +                    "index": "d",
 >  >> +                    "cost": 1.8698,
 >  >> +                    "chosen": true
 >  >> +                  } /* best_full_index_scan */,
 >  >> +                  "group_index_range": {
 >  >> +                    "chosen": false,
 >  >> +                    "cause": "not_single_table"
 >  >> +                  } /* group_index_range */,
 >  >> +                  "analyzing_range_alternatives": {
 >  >> +                    "range_scan_alternatives": [
 >  >> +                      {
 >  >> +                        "index": "d",
 >  >> +                        "ranges": [
 >  >> +                          "3 <= d <= 3"
 >  >> +                        ] /* ranges */,
 >  >> +                        "index_only": true,
 >  >> +                        "records": 1,
 >  >> +                        "cost": 2.21,
 >  >> +                        "rowid_ordered": true,
 >  >> +                        "chosen": false,
 >  >> +                        "cause": "higher_cost"
 >  >
 >
 >  >GB how can a range access with 3 <= d <= 3 (i.e. an index lookup?) be
 >  >more expensive (than index scan?)?
 >
 > A guess: there are very few records in D, and full index means that
 > the values we're looking for is in the index. Thus,
 > best_full_index_scan reads the index only. Further, range access type
 > with d=3 is not the same as ref access which is actually chosen a bit
 > further down

GB ahah... I can't see how, conceptually, a range access with d=3 is
different from a "ref" access, but this isn't related to your patch,
so you can ignore it.

 > I think "best_covering_index_scan" makes this more explicit. Agree?

GB yes

 >  >> +                }
 >  >> +              ]
 >  >> +            }
 >  >> +          }
 >  >> +        ]
 >  >> +      }
 >  >> +    }
 >  >> +  ]
 >  >> +}
 >  >> +
 >  >> +EXPLAIN SELECT key2, MIN(key2_1) FROM t2 GROUP BY key2;
 >  >> +id    select_type    table    type    possible_keys    key
 > key_len    ref    rows    Extra
 >  >> +1    SIMPLE    t2    range    NULL    i2_1    4    NULL    103
 > Using index for group-by
 >  >> +
 >  >> +SELECT TRACE FROM information_schema.OPTIMIZER_TRACE;
 >  >> +TRACE
 >  >> +{
 >  >> +  "top_query": "EXPLAIN SELECT key2, MIN(key2_1) FROM t2 GROUP BY
 > key2",
 >  >> +  "steps": [
 >  >> +    {
 >  >> +      "expanded_query": "/* select#1 */ select `key2` AS
 > `key2`,min(`key2_1`) AS `MIN(key2_1)` from `test`.`t2` group by `key2`"
 >  >> +    },
 >  >> +    {
 >  >> +      "join_preparation": {
 >  >> +        "select#": 1,
 >  >> +        "steps": [
 >  >> +        ]
 >  >> +      }
 >  >> +    },
 >  >> +    {
 >  >> +      "join_optimization": {
 >  >> +        "select#": 1,
 >  >> +        "steps": [
 >  >> +          {
 >  >> +            "constant_tables": [
 >  >> +            ],
 >  >> +            "records_estimation": [
 >  >> +              {
 >  >> +                "table": "t2",
 >  >> +                "const_keys_added": {
 >  >
 >  >GB Here again, understanding "const_key" requires knowing code.
 >
 > Only problem is that I don't fully understand which indices are put
 > into const_keys either. If you have a better naming suggestion I will
 > change it.

GB No. Let it be. Someone who will be interested in the value of
const_keys in the trace will likely know what const_keys is, or will
find out and enlighten us :-)

 >  >> +EXPLAIN SELECT * FROM t1 WHERE key2=10 OR key3=3 OR key4 <=> null;
 >  >> +id    select_type    table    type    possible_keys    key
 > key_len    ref    rows    Extra
 >  >> +1    SIMPLE    t1    index_merge    i2,i3,i4    i2,i3    4,4
 > NULL    2    Using union(i2,i3); Using where
 >  >
 >  >> +            "records_estimation": [
 >  >> +              {
 >  >> +                "table": "t2",
 >  >> +                "range_analysis": {
 >  >> +                  "table_scan": {
 >  >> +                    "records": 1024,
 >  >> +                    "cost": 215.15
 >  >> +                  },
 >  >> +                  "potential_range_indices": [
 >  >> +                    {
 >  >> +                      "index": "i1a",
 >  >> +                      "usable": false,
 >  >> +                      "cause": "not_in_keys_to_use"
 >  >> +                    },
 >  >> +                    {
 >  >> +                      "index": "i1b",
 >  >> +                      "usable": false,
 >  >> +                      "cause": "not_in_keys_to_use"
 >  >> +                    },
 >  >> +                    {
 >  >> +                      "index": "i2_1",
 >  >> +                      "key_parts": [
 >  >> +                        "key2",
 >  >> +                        "key2_1"
 >  >> +                      ]
 >  >GB here we don't have "usable":true. We need to solve the problem of
 >  >boolean attributes which are sometimes present sometimes absent: when
 >  >absent, what is their implicit value? Or should we explicitely print
 >  >"true" for them?
 >  >For what it's worth, in Python the None value evaluate to false.
 >
 > Ok, I didn't pay much attention to always display T or F for boolean
 > outputs. In this case I added usable T to potential_range_indices:
 >
 >                      "index": "i2_1",
 >                       "usable": true,
 >                       "key_parts": [
 >                         "key2",
 >                         "key2_1"
 >                       ] /* key_parts */

GB fine.

 > Are you suggesting that any Opt_trace_object that may contain
 > <something>:F should in all other cases contain <something>:T? That
 > will be a big job...

GB No, I'm not suggesting that, indeed it would be tough.

 >  >> === modified file 'sql/opt_range.cc'
 >  >> --- a/sql/opt_range.cc    2010-09-27 09:26:39 +0000
 >  >> +++ b/sql/opt_range.cc    2010-10-07 09:02:31 +0000
 >  >GB How about having a "RANGE_OPTIMIZER" flag, used in the constructor of
 >  >a few top-level objects/arrays of the range opt code, so that a user
 >  >can request hiding of the trace pieces generated by the range
 >  >optimizer?
 >
 > Of course. I have added flag a flag and use it so that when disabled
 > we still get table_scan and the range access summary parts which I
 > find interesting even if the intermediate decisions are not shown:
 >
 > Is this acceptable? Should we go as far as
 > "range_analysis": "..."
 > ?

GB No, the way you did is perfect to me.

 >  >> +        continue;
 >  >> +      }
 >  >> +      key++;                                    // Skip null byte
 >  >> +      store_length--;
 >  >> +    }
 >  >> +    field->set_key_image(key, key_part->length);
 >  >
 >
 >  >GB do you know what set_key_image() does? I imagine it leaves data in
 >  >some member of the field; so the content of that member is different
 >  >depending on whether trace is enabled or not; we need to be sure that
 >  >this difference is innocuous.
 >
 > I am pretty sure it sets the key value (less than value, greater than
 > value etc) in field's part of record[0] so that field->val_*() (which
 > is instructed to read from some offset in record[0]) can output the
 > key value as string. I am also pretty sure record[0] is overwritten
 > every time new data is requested, so whatever we put in here will not
 > be visible when reading records.

GB ok

 > Btw: note that this is SergeyP's code

GB yes. And it's us who are going to put it into a production release,
and who will fix the bug if a customer hits one ;-)

 >  >
 >  >> +    if (field->type() == MYSQL_TYPE_BIT)
 >  >> +      (void) field->val_int_as_str(&tmp, 1);
 >  >> +    else
 >  >> +      field->val_str(&tmp);
 >  >
 >  >GB do we need to have such special case for MYSQL_TYPE_BIT?
 >
 > I have no idea. SP wrote this.

GB Ok, we need to investigate/test this (if not before the push, we
should put it in WL4800_TODO.txt).

 >  >> +      out->append("'",1);
 >  >
 >  >GB out->append('\'') should do. And, what does this quote server for? Is
 >  >it that for a multi-column key we want to print
 >  >part1'part2'part3?
 >
 > Yes, that's what it is for. If we go for trace_basic_info() (comment
 > further down) we should be able to do without this.

GB but trace_basic_info() uses append_range() which uses print_key2().

 >  >If this has to be, we see that we compute key+store_length above,
 >  >compare it to key_end, then we go to the next iteration, where we
 >  >compute key+store_length again (in the incrementation in the for(...)
 >  >line) and compare it with key_end again (in the for(...)), it's a bit
 >  >inefficient.
 >  >Also, we don't append a quote after NULL (as we just "continue"), this
 >  >sounds strange. Will a multi-column key (NULL,3,4) be printed as
 >  >NULL3'4 ? That should be tested.
 >  >I suggest to test every branch of this function (the BIT one, the
 >  >quote one, etc) in some .test file.
 >
 > Ok, but can we reach a conclusion on tracing QUICK vs TRP first?

GB but even if we stick with TRP, trace_basic_info() will remain
unchanged (I think), and it uses append_range() which uses
print_key2() so we need to test every branch of print_key2()...?

 >  >> @@ -1996,8 +2052,54 @@ public:
 >  >>      }
 >  >>      DBUG_RETURN(quick);
 >  >>    }
 >  >> +  const char *get_name() const {return "range_scan";}
 >  >> +  void trace_basic_info(const PARAM *param, Opt_trace_object
 > *trace) const
 >  >> +  {
 >  >> +    trace->add("table_read_plan_type", get_name()).
 >  >> +      add("index",
 > param->table->key_info[param->real_keynr[key_idx]].name).
 >  >
 >  >GB is usage of real_keynr here inspired from existing places of code?
 >  >I need to ask, I don't know when real_keynr is good and when not.
 >  >About the latest patch: keynr_in_table could be const (maybe the
 >  >compiler can do more optimizations with this information).
 >
 > Variables storing key number can refer to the position in either table
 > (table->key_info[<this>]) or in param (param->real_keynr[<this>]).
If
 > the variable is the position in param, we use real_keynr to get the
 > position in table.
 >
 > You really have to check which one you're dealing with for each class.
 > TRP::key_idx is key number in param. In the followup patch, I tried to
 > make this more explicit. My code was wrong in some places due to
 > copy-pasting.

GB ok. I suggest to change
   uint     key_idx; /* key number in PARAM::key */
to
   uint     key_idx; /* key number in PARAM::key and PARAM::real_keynr */

 >  >> +    {
 >  >> +      String multikey;
 >  >> +      for (SEL_ARG *part= current; part; part= part->next_key_part)
 >  >
 >  >GB "part" could be const. In all logic, in those functions which only do
 >  >tracing, all pointers to the optimizer's structures should be const,
 >  >to emphasize that this only updates the trace.
 >
 > Including TRP_ROR_INTERSECT and others with double-pointers? That
 > requires a bunch of casting as well:
 >
 > from this:
 >    for (st_ror_scan_info **cur_scan= first_scan;
 > to this:
 >    for (const st_ror_scan_info **cur_scan= (const st_ror_scan_info**)
 > first_scan;
 >
 > That's a lot of noise as I see it, but I'll do it if you insist :)

GB I don't insist when C rules make it hard; as you noted,
const st_ror_scan_info **cur_scan= first_scan;
though logical, won't compile (I just re-read the explanation in some
book, where the author recognizes that this is annoying).
For simple pointers, I find const to be very valuable as
safeguard.

 >  >GB Could you also, please, add a test for this:
 >  >- a test of escaping, by storing \n in some range bound of your query,
 >  >which shows up in the trace via print_key2(), so that we can check
 >  >that displays well (escaped) and that it's valid JSON (you can use
 >  >WL4800_validate_json.py on the result file to see)
 >
 >
 > Done.
 > Problem: It breaks the line in the trace output. The problem is that
 > escape=FALSE in call to do_add(..., bool escape)

GB I will fix that later (see also reply to the new commit).

 >  >> @@ -2246,12 +2425,10 @@ int SQL_SELECT::test_quick_select(THD *t
 >  >> +    }
 >  >> +
 >  >> +free_mem:
 >  >> +    Opt_trace_object trace_range_summary(thd->opt_trace,
 >  >> +
 > "chosen_range_access_summary");
 >  >> +    if (quick)
 >  >> +    {
 >  >> +      /* Guilhem question: I've worked with two alternative ways of
 >  >> +         printing the chosen range access.
 >  >> +
 >  >> +         1) Use trace_basic_info() of the TABLE_READ_PLAN object. This
 >  >> +            can print multi-key-part ranges separately (I prefer this)
 >  >> +
 >  >> +         2) Use dbug_dump() of the QUICK_SELECT object. Maybe
 >  >> +            conceptually more correct since it is what will be
 >  >> +            executed, but I cannot make it print multi-key-part ranges
 >  >> +            in an understandable way (and I tried a lot).
 >  >
 >  >GB you mean that in a QUICK_SELECT object, we don't have all info which
 >  >trace_basic_info() uses?
 >  >Assuming we go with trace_basic_info(), do you think that what it
 >  >prints may be misleading compared to what is in QUICK_SELECT?
 >
 > The information in QUICK_SELECT is in some compressed form and I am
 > unable to find individual ranges. I don't think it misleads in any
 > way, it only shows the range in a human readable form.

GB Then let's go for the information source which is easier for you
(TRP I think).
Do I understand correctly that:
- check_quick_select() calls the handler (the storage engine), asking
it to give statistics for a set of ranges; this set is made accessible
to the engine via an "iterator" (sel_arg_range_seq_next())
- this iterator creates range intervals as it runs (so it's quite a
smart iterator), and prints them to the trace?
If you see anything which would be useful to add to the function
comment of sel_arg_range_seq_next(), please don't hesitate (have you
noticed this subtle way to hint that I don't fully understand how this
function works?).

 >  >> @@ -4683,28 +4947,42 @@ TRP_ROR_INTERSECT *get_best_ror_intersec
 >  >>    ROR_SCAN_INFO **intersect_scans_best;
 >  >>    cur_ror_scan= tree->ror_scans;
 >  >>    intersect_scans_best= intersect_scans;
 >  >> -  while (cur_ror_scan != tree->ror_scans_end &&
 > !intersect->is_covering)
 >  >>    {
 >  >> -    /* S= S + first(R);  R= R - first(R); */
 >  >> -    if (!ror_intersect_add(intersect, *cur_ror_scan, FALSE))
 >  >> +    Opt_trace_array ota(param->thd->opt_trace,
 > "intersecting_indices");
 >  >> +    while (cur_ror_scan != tree->ror_scans_end &&
 > !intersect->is_covering)
 >  >>      {
 >  >> -      cur_ror_scan++;
 >  >> -      continue;
 >  >> -    }
 >  >> +      Opt_trace_object trace_idx(param->thd->opt_trace);
 >  >> +      trace_idx.add("index",
 >  >> +
 > param->table->key_info[(*cur_ror_scan)->keynr].name);
 >  >> +      /* S= S + first(R);  R= R - first(R); */
 >  >> +      if (!ror_intersect_add(intersect, *cur_ror_scan, FALSE))
 >  >> +      {
 >  >> +        trace_idx.add("used_in_intersect", false).
 >  >> +          add("cause", "cost_not_reduced");
 >  >
 >  >GB what does cost_not_reduced mean?
 >
 > adding this index to the intersect does not reduce the cost of the
 > plan. changed to "does_not_reduce_cost_of_intersect"

GB Ok. Maybe then "used_in_intersect" could be "usable"? Trying to
standardize...

 >
 >  >> @@ -4805,6 +5105,12 @@ TRP_ROR_INTERSECT *get_best_covering_ror
 >  >>    ROR_SCAN_INFO **ror_scans_end= tree->ror_scans_end;
 >  >>    DBUG_ENTER("get_best_covering_ror_intersect");
 >  >>
 >  >> +  // None of our tests enter this function
 >  >> +  Opt_trace_object (param->thd->opt_trace).
 >  >> +    add("get_best_covering_ror_intersect", true).
 >  >> +    add("untested_code", true).
 >  >> +    add("need_tracing",true);
 >  >
 >  >GB by "our tests" do you mean the entire mtr testsuite?
 >
 > yes

GB in another place, the patch uses "done_tracing" instead of
"need_tracing".

 >  >> @@ -7839,17 +8159,18 @@ walk_up_n_right:
 >  >>          seq->param->is_ror_scan= FALSE;
 >  >>      }
 >  >>    }
 >  >> -  OPT_TRACE2(seq->param->thd->opt_trace,
 >  >> -             print_range(seq->param->thd->opt_trace,
 >  >> -
 > seq->param->table->key_info[seq->real_keyno].key_part,
 >  >> -                         range->start_key.key,
 >  >> -                         range->start_key.length,
 >  >> -                         range->end_key.key,
 >  >> -                         range->end_key.length,
 >  >> -                         cur->min_key_flag | cur->max_key_flag));
 >  >>
 >  >>    seq->param->range_count++;
 >  >>
 >
seq->param->max_key_part=max(seq->param->max_key_part,key_tree->part);
 >  >> +
 >  >> +  if (key_range_trace.length())
 >  >> +  {
 >  >> +    DBUG_ASSERT(seq->param->thd->opt_trace);
 >  >> +    DBUG_ASSERT(seq->param->thd->opt_trace->is_started());
 >  >> +    Opt_trace_array *trace_range=
 >  >> +
 > (Opt_trace_array*)seq->param->thd->opt_trace->get_current_struct();
 >  >
 >
 >  >GB Do we have a way to pass this array down from the callers, for more
 >  >safety? Maybe by adding it as a member of "SEL_ARG_RANGE_SEQ seq" (I'm
 >  >looking for a way which doesn't affect the storage engine API).
 >
 > Yes, that's possible, but I'm not sure I like that better than getting
 > it like here.
 >
 >  >If impossible, it would be better to not cast, and declare trace_range
 >  >of type Opt_trace_struct*. Indeed, with casting, if the struct happens
 >  >to be Opt_trace_object, a wrong cast may give a crash including in
 >  >release binaries. Without casting, the worse which can happen is a
 >  >JSON syntax error, which asserts in debug builds but is not dangerous
 >  >in release binaries (see Opt_trace_context::syntax_error()).
 >
 > Good point! Cast removed.
 >
 >  >I have the goal to make get_current_struct() private soon if
 >  >possible. So, nobody could access "the current struct"; it could just
 >  >add objects/arrays (which implicitely adds them as children of the
 >  >current struct), or pass down an object/array if a function wants to
 >  >add to it.
 >
 > Ok, but can I please get this patch in first?

GB yes

 >  >> @@ -11927,20 +12280,17 @@ static void print_quick(QUICK_SELECT_I *
 >  >>    DBUG_UNLOCK_FILE;
 >  >>    DBUG_VOID_RETURN;
 >  >>  }
 >  >> -#endif
 >  >> -
 >  >>
 >  >>  void QUICK_RANGE_SELECT::dbug_dump(int indent, bool verbose)
 >  >>  {
 >  >>    Opt_trace_context *out= current_thd->opt_trace;
 >  >>    /* purecov: begin inspected */
 >  >> -#ifndef NO_OPT_TRACE_FOR_RANGE_OPT
 >  >> -  Opt_trace_object oto(out, "range_select");
 >  >> -  oto.add(head->key_info[index].name, "index").
 >  >> -    add(static_cast<ulonglong>(max_used_key_length), "key_length");
 >  >> +  Opt_trace_object oto(out, "range_scan");
 >  >> +  oto.add("index", head->key_info[index].name).
 >  >> +    add("key_length", max_used_key_length).
 >  >> +    add("used_keyparts", used_key_parts);
 >  >
 >
 >  >GB This isn't covered in optimizer_trace_no_prot; do we need to print to
 >  >the trace in this dbug_dump()? My experience is that such dbug_dump()
 >  >is called from several contexts, including some where a trace would
 >  >not be desirable.
 >  >It looks like it's only print_quick() which calls dbug_dump()
 >  >functions, and print_quick() is called only once, around line 2757, in
 >  >an "else" branch, whereas the alternate branch prints to the optimizer
 >  >trace; I suggest not making print_quick() print to the opt trace, same
 >  >for dbug_dump() functions.
 >
 > I think we should revert print_quick() and dbug_dump() to how they are
 > in mysql-next-mr-opt-backporting, i.e, not use them for tracing at all.

GB sounds good; who will do it?

 >  >> @@ -6970,8 +7037,8 @@ best_access_path(JOIN      *join,
 >  >>        loose_scan_opt.next_ref_key();
 >  >>        DBUG_PRINT("info", ("Considering ref access on key %s",
 >  >>                           
keyuse->table->key_info[keyuse->key].name));
 >  >> -      Opt_trace_object oto1(thd->opt_trace);
 >  >> -      oto1.add("access_type", "index").add("index", keyinfo->name);
 >  >> +      Opt_trace_object trace_access_idx(thd->opt_trace);
 >  >> +      trace_access_idx.add("access_type", "index").add("index",
 > keyinfo->name);
 >  >>
 >  >>        /*
 >  >>           True if we find some keys from the range optimizer that
 > match more
 >  >> @@ -7027,7 +7094,7 @@ best_access_path(JOIN      *join,
 >  >>        */
 >  >>        if (!found_part && !ft_key &&
!loose_scan_opt.have_a_case())
 >  >>        {
 >  >> -        oto1.add("usable", false);
 >  >> +        trace_access_idx.add("usable", false);
 >  >>          goto done_with_index;                  // Nothing usable found
 >  >>        }
 >  >>
 >  >> @@ -7306,7 +7373,7 @@ best_access_path(JOIN      *join,
 >  >>          loose_scan_opt.check_ref_access_part2(key, start_key,
 > records, tmp);
 >  >>
 >  >>        } /* not ft_key */
 >  >> -      oto1.add("cost", tmp).add("records", records);
 >  >> +      trace_access_idx.add("records", records).add("cost", tmp);
 >  >>        /** @todo trace quick_matches_more_parts etc? */
 >  >>        if (tmp < best_time - records/(double) TIME_FOR_COMPARE ||
 >  >>            (quick_matches_more_parts &&
 >  >> @@ -7321,13 +7388,13 @@ best_access_path(JOIN      *join,
 >  >>          best_ref_depends_map= found_ref;
 >  >>        }
 >  >>    done_with_index:
 >  >> -      oto1.add("chosen", best_key == start_key);
 >  >> +      trace_access_idx.add("chosen", best_key == start_key);
 >  >>      } /* for each key */
 >  >>      records= best_records;
 >  >>    }
 >  >>
 >  >> -  Opt_trace_object oto1(thd->opt_trace);
 >  >> -  oto1.add("access_type", "table scan");
 >  >> +  Opt_trace_object trace_access_scan(thd->opt_trace);
 >  >> +  trace_access_scan.add("access_type", "scan");
 >  >>    /*
 >  >>      Don't test table scan if it can't be better.
 >  >>      Prefer key lookup if we would use the same key for scanning.
 >  >> @@ -7358,27 +7425,31 @@ best_access_path(JOIN      *join,
 >  >>    */
 >  >>    if (!(records >= s->found_records || best >
 > s->read_time))            // (1)
 >  >>    {
 >  >> -    oto1.add("cost", s->read_time).add("records",
s->found_records);
 >  >> +    trace_access_scan.add("cost", s->read_time).
 >  >> +      add("records", s->found_records).
 >  >> +      add("chosen", false);
 >  >GB There was a line, which this patch deletes, which made sure to output
 >  >'"chosen":false' at label skip_table_scan, instead of once per if()
 >  >block. Why change this?
 >
 > Because I wanted it to print "chosen:false, cause:<whatever>", and
 > <whatever> is different in each if. I could have made a "String cause"
 > instead, or I could ignore cost and just print chosen:F. What do you think?

GB you can print cause, but postpone printing "chosen" to a single call
right after skip_table_scan (and not right before), like this:
skip_table_scan:
   trace_scan_path.add("chosen", best_key == NULL);
That would replace a few add("chosen",false) calls.

Now sending a reply to the new commit.

Thread
bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3219) WL#5594Jorgen Loland7 Oct
  • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3219)WL#5594Guilhem Bichot16 Oct
    • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3219)WL#5594Jorgen Loland22 Oct
      • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3219)WL#5594Guilhem Bichot10 Nov
        • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3219)WL#5594Jorgen Loland12 Nov
          • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3219)WL#5594Guilhem Bichot15 Nov
      • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3219)WL#5594Guilhem Bichot7 Feb
        • Re: bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3219)WL#5594Jorgen Loland8 Feb