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.
