List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:June 8 2011 7:59pm
Subject:Re: review of new commit for WL#4897
View as plain text  
Hello,

Gleb Shchepa a écrit, Le 02.06.2011 15:52:
> Hello Guilhem,
> 
> On 05/27/2011 05:59 PM, Guilhem Bichot wrote:
>> Hello Gleb,
>>
>>> === modified file 'mysql-test/r/explain_non_select.result'
>>> --- mysql-test/r/explain_non_select.result    2011-03-30 16:48:25 +0000
>>> +++ mysql-test/r/explain_non_select.result    2011-05-27 07:17:57 +0000

>>> +USE grant_db;
>>> +EXPLAIN INSERT INTO v1 SELECT * FROM v1;
>>> +ERROR 42000: SELECT command denied to user 'granttest'@'localhost' 
>>> for table 'v1'
>>> +EXPLAIN REPLACE INTO v1 SELECT * FROM v1;
>>> +ERROR 42000: SELECT command denied to user 'granttest'@'localhost' 
>>> for table 'v1'
>>> +EXPLAIN UPDATE t1 SET t1.a = 10 WHERE t1.a IN (SELECT a FROM v1);
>>> +ERROR 42000: SELECT command denied to user 'granttest'@'localhost' 
>>> for table 'v1'
>>> +EXPLAIN UPDATE t1 t11, (SELECT * FROM v1) t12 SET t11.a = 10 WHERE 
>>> t11.a = 1;
>>> +ERROR 42000: SELECT command denied to user 'granttest'@'localhost' 
>>> for table 'v1'
>>> +EXPLAIN DELETE FROM t1 WHERE t1.a IN (SELECT a FROM v1);
>>> +ERROR 42000: SELECT command denied to user 'granttest'@'localhost' 
>>> for table 'v1'
>>> +EXPLAIN DELETE FROM t1 USING t1 WHERE t1.a IN (SELECT a FROM v1);
>>> +ERROR 42000: SELECT command denied to user 'granttest'@'localhost' 
>>> for table 't1'
>>> +DROP USER 'granttest'@localhost;
>>
>> GB104 I think all those tests fail simply because the user doesn't 
>> have privilege to execute the subquery "(SELECT a FROM v1)". They 
>> don't exercise your fix of sql_view.cc (I checked).
> 
> I've changed my mind about this issue: we don't need SHOW VIEW privilege 
> for EXPLAIN INSERT/UPDATE... since EXPLAIN SELECT doesn't require it.
> So I'll remove these test cases. Also I'll remove the text about "SHOW 
> VIEW" privilege from the WL page.
> Other permissions are satisfied by the implementation.

GB the trunk code in sql_view.cc is:
     if (!table->prelocking_placeholder &&
         (old_lex->sql_command == SQLCOM_SELECT && old_lex->describe))
     {
       if (check_table_access(thd, SELECT_ACL, view_tables, FALSE,
                              UINT_MAX, TRUE) &&
           check_table_access(thd, SHOW_VIEW_ACL, table, FALSE, 
UINT_MAX, TRUE))
       {
         my_message(ER_VIEW_NO_EXPLAIN, ER(ER_VIEW_NO_EXPLAIN), MYF(0));
         goto err;
       }
     }
this checks that if the statement is EXPLAIN SELECT, the user has either 
SELECT privilege on all view's underlying tables or SHOW VIEW privilege 
on the view.

Consider the example below.
t and v are created by root with
  create table t(a int);
  create view v as select * from t;
User "gb" has privileges granted by root:
+------------------------------------------------------+
| Grants for gb@localhost                              |
+------------------------------------------------------+
| GRANT USAGE ON *.* TO 'gb'@'localhost'               |
| GRANT SELECT, UPDATE ON `db`.`v` TO 'gb'@'localhost' |
+------------------------------------------------------+

And now "gb" does
mysql> explain update v set a=a+1;
+----+-------------+-------+------+---------------+------+---------+------+------+-------+
| id | select_type | table | type | possible_keys | key  | key_len | ref 
  | rows | Extra |
+----+-------------+-------+------+---------------+------+---------+------+------+-------+
|  1 | SIMPLE      | t     | ALL  | NULL          | NULL | NULL    | 
NULL |    1 |       |
+----+-------------+-------+------+---------------+------+---------+------+------+-------+
1 row in set (0.00 sec)

mysql> explain select * from v;
ERROR 1345 (HY000): EXPLAIN/SHOW can not be issued; lacking privileges 
for underlying table

I think the fact that EXPLAIN UPDATE works but EXPLAIN SELECT fails, is 
inconsistent. EXPLAIN UPDATE shows info which is very similar to EXPLAIN 
SELECT, so I would make it require the same privileges as EXPLAIN SELECT.

I wouldn't require UPDATE privilege, because EXPLAIN UPDATE doesn't 
update. But if the implementation requires UPDATE privilege just "due to 
using the same code path as UPDATE" (and it seems to be the case), I 
don't complain, just make sure to have it documented.

Note: the requirements of EXPLAIN SELECT are being worked on currently: 
see BUG#11765687 and the discussion on the internal developers' list 
with subject "Privileges required for EXPLAIN on SELECT using view".

>>> === modified file 'sql/sql_view.cc'
>>> --- sql/sql_view.cc    2011-03-17 17:39:31 +0000
>>> +++ sql/sql_view.cc    2011-05-27 07:17:57 +0000
>>> @@ -1277,8 +1277,14 @@
>>>        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))
>>> +    if (!table->prelocking_placeholder && old_lex->describe
> &&
>>> +        (old_lex->sql_command == SQLCOM_SELECT ||
>>> +         old_lex->sql_command == SQLCOM_INSERT ||
>>> +         old_lex->sql_command == SQLCOM_INSERT_SELECT ||
>>> +         old_lex->sql_command == SQLCOM_UPDATE ||
>>> +         old_lex->sql_command == SQLCOM_UPDATE_MULTI ||
>>> +         old_lex->sql_command == SQLCOM_DELETE ||
>>> +         old_lex->sql_command == SQLCOM_DELETE_MULTI))
>>
>> GB133 If one day we add EXPLAIN support for another SQL command, this 
>> if() will be out-of-sync. I suggest to add a function
>> bool command_can_be_explained(sql_command)
>> {
>>   return sql_command == SQLCOM_SELECT || ...
>>             || sql_command == SQLCOM_DELETE_MULTI;
>> }
>> and put at the start of mysql_execute_command():
>>  DBUG_ASSERT(!lex->describe || command_can_be_explained())
>> and reuse this function in sql_view.cc above.
>> The assertion ensures that if we add to sql_yacc.yy the possibility to 
>> explain a new type of statement, we have to update the function (or it 
>> will assert). Using the function in sql_view.cc ensures that the 
>> security check is done for the new command.
> 
> Removed: see a commentary to GB104.

Right, it depends on outcome of GB104.

>>
>>>      {
>>>        if (check_table_access(thd, SELECT_ACL, view_tables, FALSE,
>>>                               UINT_MAX, TRUE) &&
>>>
>>
>> GB134 about Evgeny's suggestion; IIUC he means that for "EXPLAIN 
>> non-SELECT" we should start with one additional row which would say 
>> the type of the data modifying statement and the table's name.
>> So "EXPLAIN INSERT one table SELECT one table" would show two rows 
>> (one INSERT row for the inserted table, one usual row for the selected 
>> table).
>> I think it's a good idea. Note that I didn't follow all your discussion.
>> But what about multi-table UPDATE which updates two tables, like:
>>  update t1, t2 set t1.a=3,t2.a=3 where t1.b=t2.b;
>> will you be able to show _two_ "UPDATE" rows ?
>>
> 
> Not ready to discuss/commit yet (probably will be a next tiny WL).

Ok: so there is this to think about, and also EXPLAIN EXTENDED 
UPDATE/DELETE showing the transformed query.
Thread
review of new commit for WL#4897Guilhem Bichot27 May
  • Re: review of new commit for WL#4897Davi Arnaut31 May
  • Re: review of new commit for WL#4897Gleb Shchepa2 Jun
    • Re: review of new commit for WL#4897Guilhem Bichot9 Jun
      • Re: review of new commit for WL#4897Gleb Shchepa9 Jun