List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:March 15 2011 1:45pm
Subject:Re: bzr commit into mysql-trunk branch (jorgen.loland:3279)
View as plain text  
Hello,

It's ok to push, with minor details below.

Jorgen Loland a écrit, Le 15.03.2011 12:28:
> #At file:///export/home/jl208045/mysql/wl4800/mysql-next-mr-opt-backporting-wl4800/
> based on revid:guilhem.bichot@stripped
> 
>  3279 Jorgen Loland	2011-03-15
>       Review comment by Guilhem:
>       
>       Pass Opt_trace_array pointer to sel_arg_range_seq_next(), which 
>       no longer needs to call get_current_struct() to get the optimizer
>       trace array. With this, get_current_struct() is no longer used 
>       outside the trace classes and can be made private or removed.
> 
>     modified:
>       sql/handler.cc
>       sql/handler.h
>       sql/opt_range.cc
>       sql/opt_range.h
>       sql/sql_join_cache.cc

> === modified file 'sql/opt_range.cc'
> --- a/sql/opt_range.cc	2011-02-16 15:06:06 +0000
> +++ b/sql/opt_range.cc	2011-03-15 11:28:17 +0000
> @@ -7969,13 +7969,16 @@ range_seq_t sel_arg_range_seq_init(void 
>    return init_param;
>  }
>  
> -
> -static void step_down_to(String *s, SEL_ARG_RANGE_SEQ *arg, SEL_ARG *key_tree)
> +/**
> + @param trace_string  Append range information for this part of the range
> +                      condition to this String if not NULL. Used by optimizer
> +                      trace
> +*/
> +static void step_down_to(String *trace_string, 
> +                         SEL_ARG_RANGE_SEQ *arg, SEL_ARG *key_tree)
>  {
>  
> -#ifdef OPTIMIZER_TRACE
> -  if (unlikely(arg->param->thd->opt_trace != NULL) &&
> -      arg->param->thd->opt_trace->is_started())
> +  if (trace_string != NULL)
>    {
>      /*
>         Stepping down will append the range for the current keypart (in
> @@ -7985,11 +7988,10 @@ static void step_down_to(String *s, SEL_
>      const KEY_PART_INFO *key_part=
>        arg->param->table->key_info[arg->real_keyno].key_part +
> key_tree->part;
>  
> -    append_range(s, key_part,
> +    append_range(trace_string, key_part,
>                   key_tree->min_value, key_tree->max_value,
>                   key_tree->min_flag | key_tree->max_flag);
>    }
> -#endif
>  
>    RANGE_SEQ_ENTRY *cur= &arg->stack[arg->i+1];
>    RANGE_SEQ_ENTRY *prev= &arg->stack[arg->i];
> @@ -8027,6 +8029,9 @@ static void step_down_to(String *s, SEL_
>      sel_arg_range_seq_next()
>        rseq        Value returned from sel_arg_range_seq_init
>        range  OUT  Store information about the range here
> +      trace_array Optimizer trace information about range boundaries are
> +                  appended to this if tracing is enabled. If NULL, no 
> +                  optimizer tracing is done
>  
>    DESCRIPTION
>      This is "get_next" function for Range sequence interface implementation
> @@ -8044,12 +8049,23 @@ static void step_down_to(String *s, SEL_
>  */
>  
>  //psergey-merge-todo: support check_quick_keys:max_keypart
> -uint sel_arg_range_seq_next(range_seq_t rseq, KEY_MULTI_RANGE *range)
> +uint sel_arg_range_seq_next(range_seq_t rseq, KEY_MULTI_RANGE *range, 
> +                            Opt_trace_array *trace_array)
>  {
>    SEL_ARG *key_tree;
>    SEL_ARG_RANGE_SEQ *seq= (SEL_ARG_RANGE_SEQ*)rseq;
>    String key_range_trace;
> -  key_range_trace.set_charset(system_charset_info);
> +
> +  bool is_tracing= false;
> +#ifdef OPTIMIZER_TRACE
> +  if (trace_array && 
> +      unlikely(seq->param->thd->opt_trace != NULL) &&
> +      seq->param->thd->opt_trace->is_started())
> +  {
> +    is_tracing= true;
> +    key_range_trace.set_charset(system_charset_info);
> +  }
> +#endif
>    if (seq->at_start)
>    {
> @@ -8066,7 +8082,7 @@ uint sel_arg_range_seq_next(range_seq_t 
>    {
>      //step down; (update the tuple, we'll step right and stay there)
>      seq->i--;
> -    step_down_to(&key_range_trace, seq, key_tree->next);
> +    step_down_to(is_tracing ? &key_range_trace : NULL, seq, key_tree->next);

this "?:" is repeated several times, could be cached in a pointer variable.

>      key_tree= key_tree->next;
>      seq->param->is_ror_scan= FALSE;
>      goto walk_right_n_up;
> @@ -8087,7 +8103,7 @@ uint sel_arg_range_seq_next(range_seq_t 
>      {
>        // Step down; update the tuple
>        seq->i--;
> -      step_down_to(&key_range_trace, seq, key_tree->next);
> +      step_down_to(is_tracing ? &key_range_trace : NULL, seq,
> key_tree->next);

same

>        key_tree= key_tree->next;
>        seq->param->is_ror_scan= FALSE;
>        break;
> @@ -8127,9 +8143,7 @@ walk_right_n_up:
>                                            &cur->max_key_flag,
>                                            MAX_KEY);
>  
> -#ifdef OPTIMIZER_TRACE
> -        if (unlikely(seq->param->thd->opt_trace != NULL) &&
> -            seq->param->thd->opt_trace->is_started())
> +        if (is_tracing)

it's unlikely()

>          {
>            // Trace the range we just stored
>            KEY_PART_INFO *kpi=
> @@ -8140,7 +8154,6 @@ walk_right_n_up:
>                         store_key_part->min_value, store_key_part->max_value,
>                         store_key_part->min_flag | store_key_part->max_flag);
>          }
> -#endif
>          break;
>        }
>      }
> @@ -8157,7 +8170,7 @@ walk_up_n_right:
>        /* Step up */
>        key_tree= key_tree->prev;
>      }
> -    step_down_to(&key_range_trace, seq, key_tree);
> +    step_down_to(is_tracing ? &key_range_trace : NULL, seq, key_tree);

could cache pointer

>    }
>  
>    /* Ok got a tuple */
> @@ -8219,22 +8232,12 @@ walk_up_n_right:
>    seq->param->range_count++;
>   
> seq->param->max_key_part=max(seq->param->max_key_part,key_tree->part);
>  
> -#ifdef OPTIMIZER_TRACE
> -  if (key_range_trace.length())
> +  if (is_tracing && key_range_trace.length())

it's unlikely()

>    {
>      DBUG_ASSERT(seq->param->thd->opt_trace);
>      DBUG_ASSERT(seq->param->thd->opt_trace->is_started());

Given how "is_tracing" is computed, those two asserts can be removed.

I suggest doing a build with -DOPTIMIZER_TRACE=0, I think there are 
small issues with it.
Thread
bzr commit into mysql-trunk branch (jorgen.loland:3279) Jorgen Loland15 Mar
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3279)Guilhem Bichot15 Mar