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:
> === 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?
> === 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?
> === 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".
> + --optimizer-trace-max-mem-size=#
> + maximum allowed cumulated size of remembered optimizer
> + traces
*M*aximum
remembered -> stored (suggestion)
> === 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.
> === 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"
--
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
