From: Jorgen Loland
Date: October 26 2010 11:42am
Subject: Re: bzr push into mysql-next-mr-bugfixing branch (guilhem:3228 to
 3229)
List-Archive: http://lists.mysql.com/commits/121919
Message-Id: <4CC6BEA1.4060606@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

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