Hello Jorgen,
Jorgen Loland a écrit, Le 22.10.2010 09:04:
> >>
>
> 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?
Now that the range opt patch has been pushed, it's time to think about
this get_current_struct().
I suggest that the current structure (Opt_trace_array), which is created
by handler::multi_range_read_info_const(), is passed by that creator to
seq->next(), as a third parameter named "trace_array".
So the signature of RANGE_SEQ_IF::next() would get a third parameter
(minor change to handler API, quite probably ok in Fuchsia).
If that *Opt_trace_array parameter is not NULL, next() would be expected
to add to it (and it's not deadly if the engine implementing next()
forgets to do that: the trace will just miss information). Only the
default implementation of next() (sel_arg_range_seq_next()) would add to
the array (as it is now: only sel_arg_range_seq_next() adds to the
array; quick_range_seq_next(), bka_range_seq_next(),
bka_unique_range_seq_next() don't).
Then get_current_struct() would be deleted from Opt_trace_context, which
is better for isolation.
What do you think of this?
