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.
