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);
}
