maria-developers team mailing list archive

[Sergei Petrunia <sergey@xxxxxxxxxxx>]

Hi Rex,

Please find below review input for the collection of patches for these MDEVs.

First, please try to have each piece of functionality in its own commit.

You can use "git rebase -i" to make your last commits be one commit,
as well as re-order them.
Then, you can do a carefully considered "git push --force" to overwrite
the contents of your branch with the new set of commits.

in prune_partitions():

+  String parts;
+  String_list parts_list;
+
+  make_used_partitions_str(thd->mem_root, prune_param.part_info,
&parts, parts_list );
+  trace_partition_pruning.add("partition_prune", parts.ptr());

This creates a list of used partitions regardless whether we need it or not
(and typically trace=off, so we don't').

Please put this code into

if (unlikely(thd->trace_started())) { ... }

Consider this testcase:

create table t2 (a int, b int) partition by hash(a) partitions 10;
create table t3 (a int, b int) partition by hash(a) partitions 10;

set optimizer_trace=1;
explain format=json select * from t2,t3 where t2.a in (2,3,4) and t3.a in (4,5);
select * from information_schema.optimizer_trace\G

This gives:
...
          {
            "partition_prune": "p2,p3,p4"
          },
          {
            "partition_prune": "p4,p5"
          },
...
which makes it impossible to see which table is pruning is done for.
Let's make the trace be:

         {
           "prune_partitions" : {
              "table": "t2",
              "used_partitions": "p2,p3,p4"
           }
         }
         {
           "prune_partitions" : {
              "table": "t3",
              "used_partitions": "p4,p5"
           }
         }
.

in item_subselect.cc:

+ {
+ OPT_TRACE_TRANSFORM(thd, trace_wrapper, trace_transform,
+                    in_subs->get_select_lex()->select_number,
+                     "EXISTS (SELECT)", "IN (SELECT)");
+ trace_transform.add( "upper_not", ( upper_not?true:false ) );
+ }

coding style: everything inside { } should be indented one level (=2 spaces)
right.

in sql_select.cc:

              "attached_conditions_summary": [
                {
                  "table": "t1",
                  "attached": "t1.c < 30",
                  "index": "t1.a < 10 and t1.b < 20"
                }

Please change "index" to "index_condition".
and "attached" to "attached_condition".

Let's not print index_condition if it is null.
(Yes, this is somewhat inconsistent with printing "attached":null)
in item.cc:

+ #include "my_json_writer.h"
+ const char *dbug_print_optrace( )

dbug_print_optrace() is fine.
I think it needs a comment like this:

/*
  Debug printout: print the trace trace we're currently writing if any,
  and return it.
*/
(Feel free to fix poor English if necessary)

Please fix the coding style in the function body as discussed on Slack.

Please move #include to the other #includes at the top of the file. Can
add a comment, "// For dbug_print_optrace"

Would dbug_print_opt_trace() be a better name?


+ const char *dbug_print_items(Item *item)
+ {
+   *big_buf= 0;
+
+   dbug_add_print_items( item );
+   return big_buf;
+ }

This function doesn't seem to be needed?

For the rest of the functions: as discussed on Slack, we can add them
in a separate commit if we decide that they're useful.

Could you provide a very short text describing:

The entry point (which function is to be called from gdb?).

The use case. Is it "to print List<Item>" ? I don't recall seeing
this data structure in the server...
Is there a lot of need to print Item_cond and then print its children?
This looks self-repetitive... I'd like to see a log of an example gdb
session...