Hello Guilhem,
On 04/29/2011 12:41 AM, Guilhem Bichot wrote:
> 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).
Fixed for all kinds of new commands.
>
> 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.
Agree.
>
> 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.
Thank you for the patch.
>
> 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.
Ok.
Thank you,
Gleb.
