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

Jorgen Loland a écrit, Le 26.10.2010 13:42:
> Hi Guilhem
> 
> Consider this a review of the following changesets in the WL#4800 branch:
> 
> 3219 guilhem@stripped
> to
> 3230 guilhem@stripped
> 
> We have already discussed most of this on IRC, so there's nothing more 
> than a few bike-shed paint comments inline:

ok

>  > === modified file 'mysql-test/r/archive_gis.result'
>  > --- mysql-test/r/archive_gis.result    2010-06-09 14:16:33 +0000
>  > +++ mysql-test/r/archive_gis.result    2010-10-26 06:18:20 +0000
>  > @@ -236,7 +236,7 @@ explain extended select Dimension(g), Ge
>  >  id    select_type    table    type    possible_keys    key    
> key_len    ref    rows    filtered    Extra
>  >  1    SIMPLE    gis_geometry    ALL    NULL    NULL    NULL    
> NULL    21    100.00   
>  >  Warnings:
>  > -Note    1003    select dimension(`test`.`gis_geometry`.`g`) AS 
> `Dimension(g)`,geometrytype(`test`.`gis_geometry`.`g`) AS 
> `GeometryType(g)`,isempty(`test`.`gis_geometry`.`g`) AS 
> `IsEmpty(g)`,astext(envelope(`test`.`gis_geometry`.`g`)) AS 
> `AsText(Envelope(g))` from `test`.`gis_geometry`
>  > +Note    1003    /* select#1 */ select 
> dimension(`test`.`gis_geometry`.`g`) AS 
> `Dimension(g)`,geometrytype(`test`.`gis_geometry`.`g`) AS 
> `GeometryType(g)`,isempty(`test`.`gis_geometry`.`g`) AS 
> `IsEmpty(g)`,astext(envelope(`test`.`gis_geometry`.`g`)) AS 
> `AsText(Envelope(g))` from `test`.`gis_geometry`
> 
> Do we promise anything about the format of explain extended? Do we need 
> a release note about this change?

http://dev.mysql.com/doc/refman/5.5/en/explain.html
and
http://dev.mysql.com/doc/refman/5.5/en/explain-output.html
don't promise anything precise.
And the query printed by EXPLAIN EXTENDED is often non-parsable 
(contains strange Item names), so I doubt applications use it.
But it's a good idea to put it in the changelog; some people had trouble 
understanding what the "id" column in EXPLAIN corresponds to; with that 
output they can now see which query piece each "id" is about. I just put 
in the TODO to put it in the changelog.

> 
>  > === removed file 'mysql-test/r/bug42620.result'
>  > --- mysql-test/r/bug42620.result    2010-10-09 15:04:30 +0000
>  > +++ mysql-test/r/bug42620.result    1970-01-01 00:00:00 +0000
> 
> I take it this file was added by a mistake in the first place?

well, I used it to debug BUG#42620, but it required data files present 
only on my machine :-)

>  > === modified file 'mysql-test/r/mysqld--help-win.result'
>  > --- mysql-test/r/mysqld--help-win.result    2010-09-24 07:19:35 +0000
>  > +++ mysql-test/r/mysqld--help-win.result    2010-10-26 06:18:20 +0000
>  > @@ -409,6 +409,24 @@ The following options may be given as th
>  >   loosescan, firstmatch, mrr, mrr_cost_based,
>  >   index_condition_pushdown} and val is one of {on, off,
>  >   default}
>  > + --optimizer-trace=name
>  > + Controls tracing of the Optimizer:
>  > + optimizer_trace=option=val[,option=val...], where option
>  > + is one of {enabled, end_marker, one_line} and val is one
>  > + of {on, off, default}
>  > + --optimizer-trace-features=name
>  > + Enables/disables tracing of selected features of the
>  > + Optimizer:
>  > + optimizer_trace_features=option=val[,option=val...],
>  > + where option is one of {misc, greedy_search} and val is
>  > + one of {on, off, default}
>  > + --optimizer-trace-limit=#
>  > + Maximum number of shown optimizer traces
> 
> I think it would be clearer if you change "shown" to "stored".

Here we need to think about this.
"shown" is the most exact word. Consider this case:
optimizer_trace_offset=-5
optimizer_trace_limit=1
I start a session, I execute statement#1, #2, #3, #4, #5.
I read OPTIMIZER_TRACE I find only one trace, the trace of #1.
If we "store" only optimizer_trace_limit (1 here) traces, #1 is all we 
have in memory.
Then I issue a new statement #6, which creates a new trace; 
OPTIMIZER_TRACE should now show me the trace of #2, but it cannot, 
because it doesn't have it in memory (we stored only one trace: #1).
In fact, we had to store #1,#2,#3,#4,#5 (always the last 5 ones i.e. the 
value of "optimizer_trace_offset"), and show only 1 (the value of 
"optimizer_trace_limit").
That's why I used "shown" instead of "stored".

If optimizer_trace_offset>=0, we do an optimization of the logic above, 
storing only optimizer_trace_limit traces indeed.

Ideas?

>  > + --optimizer-trace-max-mem-size=#
>  > + maximum allowed cumulated size of remembered optimizer
>  > + traces
> 
> *M*aximum

fixed

> remembered -> stored (suggestion)

fixed

> 
>  > === modified file 'sql/opt_trace.cc'
>  > --- sql/opt_trace.cc    2010-10-09 15:04:30 +0000
>  > +++ sql/opt_trace.cc    2010-10-26 06:18:20 +0000
>  > @@ -826,8 +829,8 @@ void Opt_trace_stmt::next_line(void)
>  >    char spaces[]= "                               ";
>  >    const int nb= depth * 2, spaces_len= sizeof(spaces) - 1;
>  >    for (int i= 0; i < nb/spaces_len; i++)
>  > -    buffer.append(spaces);
>  > -  buffer.append(spaces + spaces_len - (nb % spaces_len));
>  > +    buffer.append(spaces, spaces_len);
>  > +  buffer.append(spaces, (nb % spaces_len));
>  >  }
> 
> Nothing wrong, but this function was unintuitive and deserves some 
> comments.

added comments to code; it also had a doxygen comment in opt_trace.h.

>  > === modified file 'sql/opt_trace.h'
>  > --- sql/opt_trace.h    2010-10-09 15:04:30 +0000
>  > +++ sql/opt_trace.h    2010-10-26 06:18:20 +0000
>  > @@ -1299,9 +1311,11 @@ public:
>  >    Opt_trace_object(Opt_trace_context *ctx,
>  >                     Opt_trace_context::feature_value feature=
>  >                     Opt_trace_context::MISC) {}
>  > -  Opt_trace_object& add(const char *key, const char *value, size_t 
> val_length)
>  > +  Opt_trace_object& add_str(const char *key,
>  > +                            const char *value, size_t val_length)
>  > +  { return *this; }
>  > +  Opt_trace_object& add_str(const char *key, const char *value)
>  >    { return *this; }
>  > -  Opt_trace_object& add(const char *key, const char *value) { return 
> *this; }
>  >    Opt_trace_object& add(const char *key, Item *item) { return *this; }
>  >    Opt_trace_object& add(const char *key, bool value) { return *this; }
>  >    Opt_trace_object& add(const char *key, int value) { return *this; }
> 
> Now that I haven't looked at opt_trace.h for a few weeks, I got really 
> confused by having two definitions of the Opt_trace_* classes. The cause 
> of confusion was that I looked at what I thought was the only 
> implementation, while I was really looking at the 
> optimizer-trace-not-compiled-in definitions.This can easily be remedied 
> by documenting the classes, something along the lines of "Empty class 
> implementation used when optimizer trace is disabled"

fixed.
I just pushed all fixes above.

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