maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

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 = &thd->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 = &thd->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 = &param->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(&param, &cond)))
> +      {
> +        Json_writer_array trace_range_summary(writer,
> +                                           "setup_range_conditions");
> +        tree= cond->get_mm_tree(&param, &cond);
> +      }

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<TABLE_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