Re: [Maria-developers] Some more input on optimizer_trace

2019-02-10 Thread Sergey Petrunia
On Sun, Feb 10, 2019 at 02:14:51PM +0200, Sergey Petrunia wrote:
> Hi Varun,
> 
> I've did some adjustments MDEV-18489 and pushed the patch into
> 10.4-optimizer-trace, please check it out.
> 
> Also I have filed MDEV-18527 and MDEV-18528.
> 
> Some input on the code:
> 
> > --- 10.4-optimizer-trace-orig/sql/sql_select.cc
> > +++ 10.4-optimizer-trace-cl/sql/sql_select.cc
> 
One more thing - Json_value_context is not really a context anymore.

(the original intent was that "one gets a value context object if the
Json_writer's current state is such that it currently expects a value (and not
a name)"  but apparently it is not used this way. Which is fine, but then I
guess the object should be renamed. (to _helper? or something like that?)

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp


[Maria-developers] Some more input on optimizer_trace

2019-02-10 Thread Sergey Petrunia
Hi Varun,

I've did some adjustments MDEV-18489 and pushed the patch into
10.4-optimizer-trace, please check it out.

Also I have filed MDEV-18527 and MDEV-18528.

Some input on the code:

> --- 10.4-optimizer-trace-orig/sql/sql_select.cc
> +++ 10.4-optimizer-trace-cl/sql/sql_select.cc

> @@ -15983,12 +16250,26 @@ optimize_cond(JOIN *join, COND *conds,
>that occurs in a function set a pointer to the multiple equality
>predicate. Substitute a constant instead of this field if the
>multiple equality contains a constant.
> -*/ 
> -DBUG_EXECUTE("where", print_where(conds, "original", QT_ORDINARY););
> +*/
> +
> +Opt_trace_context *const trace = >opt_trace;
> +Json_writer *writer= trace->get_current_json();
> +Json_writer_object trace_wrapper(writer);
> +Json_writer_object trace_cond(writer, "condition_processing");
> +trace_cond.add("condition", join->conds == conds ? "WHERE" : "HAVING")
> +  .add("original_condition", conds);
> +
> +Json_writer_array trace_steps(writer, "steps");
> +  DBUG_EXECUTE("where", print_where(conds, "original", QT_ORDINARY););

Small question: Why was DBUG_EXECUTE shifted right? 

A bigger question: the code seems unnecesarily verbose:

1. > +Opt_trace_context *const trace = >opt_trace;
2. > +Json_writer *writer= trace->get_current_json();
3. > +Json_writer_object trace_wrapper(writer);
4. > +Json_writer_object trace_cond(writer, "condition_processing");

Can we save the space by just calling the constructors:

  Json_writer_object trace_wrapper(thd);
  Json_writer_object trace_cond(thd, "condition_processing");

?
This applies here and in many other places.

Alternative, we could use:

  Json_writer_object trace_wrapper(thd);
  Json_writer_object trace_cond(trace_wrapper, "condition_processing");

.. which makes the nesting clearer (and we could also add debug safety check: it
is invalid to operate on trace_wrapper until trace_cond hasn't been end()'ed)


> --- 10.4-optimizer-trace-orig/sql/opt_range.cc
> +++ 10.4-optimizer-trace-cl/sql/opt_range.cc  
> 
> +void TRP_ROR_UNION::trace_basic_info(const PARAM *param,
> + Json_writer_object *trace_object) const
> +{
> +  Opt_trace_context *const trace = >thd->opt_trace;
> +  Json_writer* writer= trace->get_current_json();
> +  trace_object->add("type", "index_roworder_union");
> +  Json_writer_array ota(writer, "union_of");

The name 'ota' makes sense in MySQL codebase (where it is a contraction of
Optimizer_trace_array), but is really confusing in MariaDB codebase. Please
change everywhere to "smth_trace" or something like that (jwa in the worst
case).

> @@ -2654,12 +2833,18 @@ int SQL_SELECT::test_quick_select(THD *t
> 
>  if (cond)
>  {
> -  if ((tree= cond->get_mm_tree(, )))
> +  {
> +Json_writer_array trace_range_summary(writer,
> +   "setup_range_conditions");
> +tree= cond->get_mm_tree(, );
> +  }

Does this ever produce anything meaningful, other than empty:

"setup_range_conditions": [],

In MySQL, the possible contents of this array are:

  "impossible_condition": {
"cause": "comparison_with_null_always_false"
  } /* impossible_condition */

   "impossible_condition": {
 "cause": "null_field_in_non_null_column"
   } /* impossible_condition */

But in MariaDB we don't have this (should we?)

Also,  why_use_underscores_in_value_of_cause?  It is a quoted string where
spaces are allowed. MySQL seems to have figured this out at some point and have
a few cause strings using spaces.

> --- 10.4-optimizer-trace-orig/sql/opt_table_elimination.cc
> +++ 10.4-optimizer-trace-cl/sql/opt_table_elimination.cc  
> @@ -522,7 +524,8 @@ eliminate_tables_for_list(JOIN *join,
>List *join_list,
>table_map tables_in_list,
>Item *on_expr,
> -  table_map tables_used_elsewhere);
> +  table_map tables_used_elsewhere,
> +  Json_writer_array* eliminate_tables);

Please change the name to something indicating it's just a trace.


BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp