List:Commits« Previous MessageNext Message »
From:Gleb Shchepa Date:May 25 2011 2:20pm
Subject:Re: bzr commit into mysql-trunk branch (gleb.shchepa:3358) WL#4897
View as plain text  
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.

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