List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:April 28 2011 8:41pm
Subject:Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897
View as plain text  
Hello Gleb,

I'll be sending multiple mails over time, here are just a few first points.
I have prefixed each point with "GB" and a sequence number. The number
will make it easy to reference a point in another email. "GB" is used to
spot comments in the middle of hundreds of diff lines.

Gleb Shchepa a écrit, Le 01.04.2011 15:54:
> #At file:///mnt/sda7/work/mysql-next-mr-opt-backporting-wl4897/ based on
> revid:tor.didriksen@stripped
> 
>  3358 Gleb Shchepa	2011-04-01
>       WL#4897: Add EXPLAIN INSERT/UPDATE/DELETE

GB1 in sql_view.cc there is:
      /*
        Check rights to run commands (EXPLAIN SELECT & SHOW CREATE) which
show
        underlying tables.
        Skip this step if we are opening view for prelocking only.
      */
      if (!table->prelocking_placeholder &&
          (old_lex->sql_command == SQLCOM_SELECT && old_lex->describe))
            { require SHOW VIEW privilege }

For
"EXPLAIN UPDATE some_view WHERE ...",
this check will be bypassed (wrongly) because old_lex->sql_command will
not be SQLCOM_SELECT but SQLCOM_UPDATE (I guess, haven't checked;
testing this in some MTR test would be good).

GB2 The WL says
"The user must have:
    1) SELECT privilege for any referenced tables,
    2) EXECUTE privilege for any referenced functions, in 'statement',
    3) for every modifying table:
       a) for EXPLAIN INSERT: INSERT privilege,
       b) for EXPLAIN REPLACE: INSERT & DELETE privilege,
       c) for EXPLAIN UPDATE: UPDATE privilege,
       d) for EXPLAIN DELETE: DELETE privilege."
Actually, even though the list above is a very good "executive summary",
what is checked for INSERT/etc is complicated. If the query contains
LOAD_FILE(), FILE privilege is checked. Sometimes a privilege on the
column-to-be-updated is enough, we don't require the privilege on all
columns. So instead of describing again what INSERT/UPDATE/etc require,
I suggest deleting this list above, leaving
"the user must have the same privileges as would be required
to actually execute INSERT/REPLACE/UPDATE/DELETE respectively"
and adding
"additionally SHOW VIEW privilege is needed on any used view".
And also a mention (in the LLD?) that all this is achieved by having
EXPLAIN simply going through the same code paths as INSERT/REPLACE/etc.

GB3 mysqltest has option --explain-protocol, which, for each SELECT of
the test, runs EXPLAIN EXTENDED. Attached is a quick patch to make this
option apply to more statements (ignore the fprintf() in it, it was for 
debugging only). Btw, using this option in 5.1 led to find BUG#12409205 
today.

GB4 I propose that for any future change, you just commit/push to your
feature tree. It's better because it will produce incremental revisions
easy to review. And if you push, it allows me to easily have a tree 
representing the state of your work; which is not the case with several 
unpushed committed incremental revisions.
Re-committing the entire patch with just a few lines changed here or 
there would be impractical.

=== modified file 'client/mysqltest.cc'
--- client/mysqltest.cc	2011-03-22 15:40:32 +0000
+++ client/mysqltest.cc	2011-04-28 12:53:30 +0000
@@ -201,6 +201,7 @@
 static my_regex_t ps_re;     /* the query can be run using PS protocol */
 static my_regex_t sp_re;     /* the query can be run as a SP */
 static my_regex_t view_re;   /* the query can be run as a view*/
+static my_regex_t explain_re;/* the query can be converted to EXPLAIN */
 
 static void init_re(void);
 static int match_re(my_regex_t *, char *);
@@ -6389,7 +6390,7 @@
   {"view-protocol", OPT_VIEW_PROTOCOL, "Use views for select.",
    &view_protocol, &view_protocol, 0,
    GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
-  {"explain-protocol", OPT_EXPLAIN_PROTOCOL, "Explains all select.",
+  {"explain-protocol", OPT_EXPLAIN_PROTOCOL, "Explains all select/update/etc.",
    &explain_protocol, &explain_protocol, 0,
    GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
   {"connect_timeout", OPT_CONNECT_TIMEOUT,
@@ -7880,7 +7881,7 @@
 {
   if (explain_protocol_enabled &&
       !command->expected_errors.count &&
-      match_re(&view_re, command->query))
+      match_re(&explain_re, command->query))
   {
     st_command save_command= *command;
     DYNAMIC_STRING query_str;
@@ -7890,6 +7891,8 @@
     init_dynamic_string(&query_str, "EXPLAIN EXTENDED ", 256, 256);
     dynstr_append_mem(&query_str, command->query,
                       command->end - command->query);
+    
+    fprintf(stderr, "autogen %.*s\n", (int)query_str.length, query_str.str);
     command->query= query_str.str;
     command->query_len= query_str.length;
     command->end= strend(command->query);
@@ -7963,9 +7966,16 @@
     "^("
     "[[:space:]]*SELECT[[:space:]])";
 
+
+  /* Filter for queries that can be converted to EXPLAIN */
+  const char *explain_re_str =
+    "^("
+    "[[:space:]]*(SELECT|DELETE|UPDATE|INSERT|REPLACE)[[:space:]])";
+
   init_re_comp(&ps_re, ps_re_str);
   init_re_comp(&sp_re, sp_re_str);
   init_re_comp(&view_re, view_re_str);
+  init_re_comp(&explain_re, explain_re_str);
 }
 
 



Thread
bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa1 Apr
  • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Guilhem Bichot28 Apr
    • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa25 May
      • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Guilhem Bichot27 May
  • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Guilhem Bichot9 May
    • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa25 May
      • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Guilhem Bichot27 May
        • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa2 Jun
          • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Guilhem Bichot9 Jun
            • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa9 Jun
    • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Roy Lyseng27 May
      • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa2 Jun
  • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Roy Lyseng27 May
    • Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897Gleb Shchepa2 Jun