List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:November 15 2010 1:42pm
Subject:Re: bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to
3229)
View as plain text  
Hello,

Jorgen Loland a écrit, Le 15.11.2010 11:10:
> Hello Guilhem,
> 
> We're almost done with this bath now :-)

yeeeeeeeeees

> On 11/12/2010 03:32 PM, Guilhem Bichot wrote:
>> Hello,
>>
>> Jorgen Loland a écrit, Le 09.11.2010 13:55:
>>> Review of changesets up to 3218:
>>>
>>> === modified file 'mysql-test/r/optimizer_trace_no_prot.result'
>>> --- mysql-test/r/optimizer_trace_no_prot.result 2010-09-22 19:50:59 
>>> +0000
>>> +++ mysql-test/r/optimizer_trace_no_prot.result 2010-10-05 13:19:55 
>>> +0000
>>> > @@ -1962,7 +1949,15 @@ QUERY_ID TRACE MISSING_BYTES_BEYOND_MAX_
>>> > }
>>> > ] /* steps */
>>> > } 0 0
>>> > -set optimizer_trace="skip_plan=off";
>>> > +set @@optimizer_trace_features="greedy_search=on,misc=off";
>>> > +explain select * from t1,t2;
>>> > +id select_type table type possible_keys key key_len ref rows Extra
>>> > +1 SIMPLE t2 ALL NULL NULL NULL NULL 2 > +1 SIMPLE t1 ALL NULL NULL
>>> NULL NULL 3 Using join buffer (BNL, regular buffers)
>>> > +select * from information_schema.OPTIMIZER_TRACE;
>>> > +QUERY_ID TRACE MISSING_BYTES_BEYOND_MAX_MEM_SIZE OS_MALLOC_ERROR
>>> > +0 0 0
>>> > +set @@optimizer_trace_features=default;
>>>
>>> jl: So turning feature "misc" off is the same as disabling tracing? I
>>> don't find that all to intuitive. If this is correct (misc=off ==
>>> enable=off), why don't we remove the misc feature name?
>>
>> What happens is that the top object (the {} around the entire trace) is
>> in category "misc" (every structure needs to have a category...), so if
>> you disable "misc", you disable this top object, and thus everything
>> below it.
>> Indeed "misc" isn't intended to be used by users, so I will hide it.
> 
> Great :)

Btw it will be in the next commit mail (I will not push, will wait for 
your review and it also gives you a chance to push range opt first).

>>> > === modified file 'sql/opt_trace.cc'
>>> > --- sql/opt_trace.cc 2010-09-22 19:50:59 +0000
>>> > +++ sql/opt_trace.cc 2010-09-27 08:00:25 +0000
>>> > @@ -86,15 +105,27 @@ void Opt_trace_ooa::do_construct(Opt_tra
>>> > */
>>> > size_t new_size= alloced + increment;
>>> > const size_t max_size= stmt->buffer.allowed_mem_size;
>>> > - if (new_size > max_size /* don't pre-allocate more than the limit
>>> */ &&
>>> > - max_size >= alloced /* don't shrink string if limit exceeded */ )
>>> > + if (new_size > max_size) // don't pre-allocate more than the limit
>>> > new_size= max_size;
>>> > - stmt->buffer.realloc(new_size - 1 /* because String::realloc has
>>> +1 */);
>>> > + const size_t overhead=
>>> > + 1 + (ALIGN_SIZE(1) - 1) // see start of String::realloc()
>>> > + /*
>>> > + Stay strictly below max_size or it would trigger truncation at
>>> > + next append(), which is potentially wrong for a "pre-emptive
>>> > + allocation" as we do here.
>>> > + */
>>> > + + 1;
>>>
>>> jl: I have problems understanding the size of the overhead. Why is 1
>>> used as parameter to ALIGN_SIZE?

> Ok, this makes a little more sense now. But the fact that you didn't 
> remember exactly why you did it like this indicates that it would be a 
> good idea to add to the comments :-)

will do

>>> > --- sql/sql_view.cc 2010-08-30 06:38:09 +0000
>>> > +++ sql/sql_view.cc 2010-10-05 10:29:43 +0000
>>> > @@ -1138,6 +1139,12 @@ bool mysql_make_view(THD *thd, File_pars
>>> > table->definer.user.str= table->definer.host.str= 0;
>>> > table->definer.user.length= table->definer.host.length= 0;
>>> >
>>> > + Opt_trace_object oto0(thd->opt_trace);
>>> > + Opt_trace_object oto1(thd->opt_trace, "view");
>>> > + oto1.add("db", table->db, table->db_length).
>>> > + add("name", table->table_name, table->table_name_length).
>>> > + add("in_select#", old_lex->select_lex.select_number);
>>> > +
>>>
>>> jl: This has been changed to add_str in a later cs, but don't we need
>>> to use add_str_escaped when we don't know the character set?
>>
>> Normally in MySQL (I just checked with Bar the specialist), TABLE's and
>> TABLE_LIST's db and table_name are in utf8. So we can pass them to
>> add_str() without charset conversion; but you are right that they can
>> contain characters like \n which must be escaped. I have put it in my
>> TODO to reconsider all add_str* calls in code, to see which ones need
>> escaping, and to add tests (see also same discussion in a review of your
>> range opt patch).
> 
> I think we should add a few escaped table/view/column names to the tests 
> to verify that everything is ship shape.

will do

> Btw, at time of implementation of the range tracing, I had the 
> impression that add_str did escaping, so I can imagine that there are 
> some comments on that in your review :)

there are :-)

>>> opt_trace2server.cc
>>> -------------------
>>> > if (thd->opt_trace->start(need_it_for_I_S,
>>> > (var & Opt_trace_context::FLAG_END_MARKER),
>>> > (var & Opt_trace_context::FLAG_ONE_LINE),
>>> > thd->variables.optimizer_trace_offset,
>>> > thd->variables.optimizer_trace_limit,
>>> > thd->variables.optimizer_trace_max_mem_size,
>>> >
>>>
> Opt_trace_context::feature_value(thd->variables.optimizer_trace_features))) 
>>>
>>>
>>>
>>> jl: long line
>>
>> Yes... even just
>> Opt_trace_context::feature_value(thd->variables.optimizer_trace_features
>> is 72 chars, so I don't see how to break the line properly. What would
>> you suggest?
> 
> Touchè. I see no good way to break it, but I would probably start a new 
> line before the argument (between "(" and "thd"). I'm not sure if that 
> is any better. I'll leave it for you to decide.

I tried the breaking as you suggest, but for Emacs it changes nothing. 
Then I tried this:
const ulonglong features= thd->variables.optimizer_trace_features;
if (thd->opt_trace->start(...,
                           Opt_trace_context::feature_values(features))
but then it does not compile because "goto end" jumps over the 
initialization. And I find it ugly to introduce a variable just to 
shorten a line.
So I gave up.

>>> > char buff[1024];
>>> > String str(buff,(uint32) sizeof(buff), system_charset_info);
>>> > str.length(0);
>>> > /**
>>> > If this statement is not SELECT, what is shown here can be
>>> > inexact. INSERT SELECT is shown as SELECT. DELETE WHERE is shown
>>> > as SELECT WHERE.
>>> > This is acceptable given the audience (developers) and the goal (the
>>> > inexact parts are irrelevant for the optimizer).
>>> > */
>>> > thd->lex->unit.print(&str,
> enum_query_type(QT_EXPAND_VIEW_DEFINITION |
>>> > QT_SHOW_SELECT_NUMBER));
>>>
>>> jl: what happens if the expanded query turns out to be longer than
>>> 1024 characters?
>>
>> Most print() uses String::append(), which calls String::realloc(), which
>> has this logic:
>> if (this string's buffer was allocated) then realloc() it
>> else malloc() a new buffer and use it for the String.
>> Normally, when we exceed 1024 bytes for the first time, we fall into the
>> "else" case.
>> The 1024-byte buffer is only, I think, an optimization to use the stack
>> instead of the heap, most of the times. But is not a limitation.
>> It's by reading code that I say this, you may want to double-check, or
>> ask me to add a test with a very long query and run with Valgrind.
>>
>> I will make a commit when ready.
> 
> Yes, if you're not 100% certain I would like you to double check that it 
> does not break. Apart from that I'm happy with the explanation.

I just added a test with a query of 1070 characters.
Thread
bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to 3229) Guilhem Bichot22 Oct
Re: bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to3229)Jorgen Loland26 Oct
  • Re: bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to3229)Guilhem Bichot5 Nov
    • Re: bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to3229)Jorgen Loland10 Nov
  • Re: bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to3229)Jorgen Loland9 Nov
    • Re: bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to3229)Guilhem Bichot12 Nov
      • Re: bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to3229)Jorgen Loland15 Nov
        • Re: bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to3229)Guilhem Bichot15 Nov
    • Re: bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to3229)Guilhem Bichot15 Nov