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