maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx 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