Hi, Sergey! Thanks for the review! I won't push yet, need to fix what I said below. Want to see a new patch? On Jun 23, Sergey Vojtovich wrote:
Hi Sergei,
ok to push. A few minor questions inline.
On Mon, Jun 16, 2014 at 09:34:21AM +0200, serg@mariadb.org wrote:
--- a/mysql-test/r/not_embedded_server.result +++ b/mysql-test/r/not_embedded_server.result @@ -1,4 +1,4 @@ -call mtr.add_suppression("Can't open and lock privilege tables: Table 'host' was not locked with LOCK TABLES"); +call mtr.add_suppression("Can't open and lock privilege tables: Table 'roles_mapping' was not locked with LOCK TABLES"); SHOW VARIABLES like 'slave_skip_errors'; Variable_name Value slave_skip_errors OFF Why did it change (here and in other tests)? I didn't notice any table order change in the code.
Because TABLE_LIST's entries are linked in the list in the different order. And this error is issued for the first table in the list. I thought this was fine (as the new behavior is also correct) and didn't want to do something complex only to preserve old error messages. But it just occurred to me that I can trivially do that, simply by reversing the for() loop in the init_priv_tables(). I'm testing this now and restore old error messages if everything will work ok.
...skip...
diff --git a/mysql-test/r/sp-destruct.result b/mysql-test/r/sp-destruct.result index 172e40c..fe68229 100644 --- a/mysql-test/r/sp-destruct.result +++ b/mysql-test/r/sp-destruct.result @@ -128,11 +128,8 @@ CREATE FUNCTION f1() RETURNS INT RETURN 1; RENAME TABLE mysql.procs_priv TO procs_priv_backup; FLUSH TABLE mysql.procs_priv; DROP FUNCTION f1; -ERROR 42S02: Table 'mysql.procs_priv' doesn't exist SHOW WARNINGS; Level Code Message -Error 1146 Table 'mysql.procs_priv' doesn't exist -Warning 1405 Failed to revoke all privileges to dropped routine # Restore the procs_priv table RENAME TABLE procs_priv_backup TO mysql.procs_priv; FLUSH TABLE mysql.procs_priv; Hmm... why?
...skip...
For certain objects it was ok to drop them (no error issued) is some of the privilege tables were missing. For example, one could do DROP USER with no error is, say, proxies_priv or roles_mapping doesn't exist. Which is pretty logical, DROP USER means you remove all existing privileges for this user, if a privilege table doesn't exist - well, it doesn't contain any privileges to drop, does it? :) But dropping a function behaved differently. It returned an error if procs_priv did not exist. But the function was dropped regardless. Which is pretty bad on itself. And inconsistent with how DROP generally behaves. I've fixed this as a side-effect (init_priv_tables() treats all tables equally, it cannot make a table optional for one DROP and mandatory for another DROP).
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 5e8e0e7..442d512 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc + + @retval !0 a pointer to the first initialized element in the TABLE_LIST + @retval 0 replication filters matched. Abort operation, but return OK (!) +*/ +TABLE_LIST *init_priv_tables(THD *thd, TABLE_LIST *tables, + enum thr_lock_type lock_type, int tables_to_open) Nice clean-up, I assume there was a reason not rewrite open_grant_tables() instead.
Not really. Thanks, I'll do that.
+#ifdef HAVE_REPLICATION + if (lock_type >= TL_WRITE_ALLOW_WRITE && thd->slave_thread && !thd->spcont) + { + /* + GRANT and REVOKE are applied the slave in/exclusion rules as they are + some kind of updates to the mysql.% tables. + */ + Rpl_filter *rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; + if (rpl_filter->is_on() && !rpl_filter->tables_ok(0, tables)) + return NULL; + } +#endif Was it a bug that mysql_grant_role() didn't have this code?
I suppose so.
open_grant_tables() does updating= 0 after tables_ok(). Is that needed?
I don't think so (also I set updating unconditionally, old code was only doing that in #ifdef HAVE_REPLICATION). This TABLE_LIST::updating is only used in rpl filtering, I believe it doesn't matter what it's set to after the filtering is done.
...skip...
@@ -5783,8 +5796,9 @@ bool mysql_routine_grant(THD *thd, TABLE_LIST *table_list, bool is_proc, } }
- if (replace_routine_table(thd, grant_name, tables[1].table, *Str, - db_name, table_name, is_proc, rights, + if (no_such_table(tables + PROCS_PRIV_TABLE) || + replace_routine_table(thd, grant_name, tables[PROCS_PRIV_TABLE].table, + *Str, db_name, table_name, is_proc, rights, revoke_grant) != 0) { result= TRUE; Why no_such_table() here?
Yeah... See above - it's ok for a privilege table to be missing when some object is DROP'ped or a privilege is revoked. But when a privilege is granted or updated in some way, we must ensure that a privilege table exists. Because init_priv_tables() doesn't do that for all tables, it has to be done manually now. Old code was achieving that by not using TABLE_LIST::OPEN_IF_EXISTS for mandatory tables. It was possible to have a different set of mandatory tables in every function. And that also resulted in the incorrect DROP FUNCTION behavior.
...skip...
Regards, Sergei