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?
> === 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?
> === modified file 'sql/opt_trace.h'
> --- sql/opt_trace.h 2010-09-22 19:50:59 +0000
> +++ sql/opt_trace.h 2010-09-27 08:00:25 +0000
> @@ -531,7 +552,7 @@ private:
> /**
> - Adds a value (of string type) to the OOA. If a key is specified, then it
> - is adds the key/value pair (the OOA must thus be an object). If no key is
> - specified, only the value is added (the OOA must thus be an array).
> - @param value the value
> - @param key the optional key
> - @return a reference to the OOA, useful for chaining like this:
> + Adds a value (of string type) to the structure. A key is specified, so it
> + is adds the key/value pair (the structure must thus be an object).
> + @param key key
> + @param value value
> + @return a reference to the structure, useful for chaining like this:
> @verbatim add(x,y).add(z,t).add(u,v) @endverbatim
jl: typo "so it is adds"
> @@ -881,40 +996,54 @@ public:
> /**
> A JSON array (ordered set of values).
> Defines only a constructor, all the rest in inherited from Opt_trace_struct.
> */
jl: s/in/is
> -class Opt_trace_array: public Opt_trace_ooa
> +class Opt_trace_array: public Opt_trace_struct
> {
> public:
> /**
> - Constructs an array. If a key is specified, the array is the value of a
> - key/value pair (serves for adding the array to an object). Otherwise the
> - array is just a value (serves for adding the array to an array).
> + Constructs an array. @sa Opt_trace_object::Opt_trace_object.
> @param ctx context for this array
> - @param key optional key
> + @param key key
> */
> - Opt_trace_array(Opt_trace_context *ctx, const char *key= NULL) :
> - Opt_trace_ooa(ctx, false, key) {}
> + Opt_trace_array(Opt_trace_context *ctx, const char *key,
> + Opt_trace_context::feature_value feature=
> + Opt_trace_context::MISC) :
> + Opt_trace_struct(ctx, false, key, feature) {}
> + Opt_trace_array(Opt_trace_context *ctx,
> + Opt_trace_context::feature_value feature=
> + Opt_trace_context::MISC) :
> + Opt_trace_struct(ctx, false, NULL, feature) {}
> };
jl: Please document both constructors and explicitly state the
difference between having and not having a key
> @@ -980,15 +1111,15 @@ private:
> /**
> - Grabs the current OOA from the context and call its methods.
> - Most of the time, there is a direct reference to this OOA available
> + Grabs the current structure from the context and call its methods.
> + Most of the time, there is a direct reference to this structure available
> (because it has been instantianted a few lines earlier), then prefer using
> it than this macro.
> */
> #define OPT_TRACE(trace, action) do { \
> if (unlikely((trace) != NULL)) \
> if (unlikely((trace)->is_started())) \
> - (trace)->get_current_ooa()->action; \
> + (trace)->get_current_struct()->action; \
> } while (0)
jl: align final '\' with the others
> @@ -1054,34 +1185,54 @@ extern ST_FIELD_INFO optimizer_trace_inf
> /* all empty */
>
> class Opt_trace_context
> -{};
> +{
> +public:
> + /* We need this one */
> + enum feature_value { MISC= 1, GREEDY_SEARCH= 2 };
> +};
jl: suggestion: "We need this enum even if tracing is disabled"
> @@ -1120,8 +1271,8 @@ public:
> #define
OPT_TRACE_TRANSFO(trace,object_level0,object_level1,select_number,from,to) \
> Opt_trace_object object_level0(trace); \
> Opt_trace_object object_level1(trace, "transformation"); \
> - object_level1.add((ulonglong)select_number, "select #"). \
> - add(from, "from").add(to, "to");
> + object_level1.add("select #", select_number).
\
> + add("from", from).add("to", to);
jl: Align '\'
> === modified file 'sql/sql_select.cc'
> --- sql/sql_select.cc 2010-09-18 16:25:43 +0000
> +++ sql/sql_select.cc 2010-09-27 08:00:25 +0000
> @@ -7379,7 +7379,7 @@ best_access_path(JOIN *join,
> }
> }
>
> - oto1.add(tmp, "cost").add(rows2double(rnd_records), "records");
> + oto1.add("cost", tmp).add("records", rows2double(rnd_records));
> /*
> We estimate the cost of evaluating WHERE clause for found records
> as record_count * rnd_records / TIME_FOR_COMPARE. This cost plus
jl: Ok, so we needed twice the number of add() methods in the trace
classes, but I find it much easier to read :)
> --- 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?
opt_trace-t.cc
--------------
1) There are many add_str calls in the test. These could cover more
code if they were called with different parameters, specifically
a) with length parameter
b) with another character set/escaped characters
2) Test get_current_struct() by add()-ing to it
3) Test set_query() and then print it with "normal" and "strange"
character set.
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
> 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?
--
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
