maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Vicențiu!

That's pretty good, thanks!
Although I would like to suggest a somewhat different approach, see
below.

And there are few minor comments too.

On Mar 13, vicentiu@xxxxxxxxxxx wrote:
> revision-id: 3c941d471b79ba5d9939a4a1ac61ce5ed8bebaef
> parent(s): 197afb413fcc9f06b5e5e6ef41ce980d108b354f
> committer: Vicențiu Ciorbaru
> branch nick: server
> timestamp: 2015-03-13 17:45:07 +0200
> message:
> 
> MDEV-5214 Status variables for number of global/db/table/column/role grants
> 
> Implemented the status variables for use with the feedback plugin.
> 
> ---
>  mysql-test/suite/plugins/r/acl_statistics.result | 107 +++++++++++++++++++++++
>  mysql-test/suite/plugins/t/acl_statistics.test   |  70 +++++++++++++++
>  sql/mysqld.cc                                    |  34 +++++++
>  sql/sql_acl.cc                                   |  34 +++++++
>  sql/sql_acl.h                                    |  10 +++
>  5 files changed, 255 insertions(+)
> 
> diff --git a/mysql-test/suite/plugins/t/acl_statistics.test b/mysql-test/suite/plugins/t/acl_statistics.test
> new file mode 100644
> index 0000000..e10d907
> --- /dev/null
> +++ b/mysql-test/suite/plugins/t/acl_statistics.test
> @@ -0,0 +1,70 @@
> +# Test case for validating acl statistics for the feedback plugin.
> +--source include/not_embedded.inc
> +if (`select count(*) = 0 from information_schema.plugins where plugin_name = 'feedback' and plugin_status='active'`)
> +{
> +  --skip Feedback plugin is not active
> +}

Don't bother using feedback plugin for this test.
There is nothing in the patch that depends on or uses feedback plugin,
so there's no need to make the test depending on it.

Please, use SHOW STATUS (or SELECT ... FROM INFORMATION_SCHEMA.GLOBAL_STATUS).
And then move the test out of plugins suite.

> +
> +# First get a baseline of the initial statistics.
...
> diff --git a/mysql-test/suite/plugins/r/acl_statistics.result b/mysql-test/suite/plugins/r/acl_statistics.result
> new file mode 100644
> index 0000000..dad37bf
> --- /dev/null
> +++ b/mysql-test/suite/plugins/r/acl_statistics.result
> @@ -0,0 +1,107 @@
> +SELECT * FROM information_schema.feedback
> +WHERE variable_name like '%Acl%';
> +VARIABLE_NAME	VARIABLE_VALUE
> +ACL_COUNT_COLUMN_GRANTS	0
> +ACL_COUNT_DATABASE_GRANTS	2
> +ACL_COUNT_FUNCTION_GRANTS	0
> +ACL_COUNT_PROCEDURE_GRANTS	0
> +ACL_COUNT_PROXY_USERS	2
> +ACL_COUNT_ROLE_MAPPINGS	0
> +ACL_COUNT_ROLES	0
> +ACL_COUNT_TABLE_GRANTS	0
> +ACL_COUNT_USERS	4
> +SELECT count(*) COLUMN_GRANTS_COUNT from mysql.columns_priv;
> +COLUMN_GRANTS_COUNT
> +0
> +SELECT count(*) DATABASE_GRANTS_COUNT from mysql.db;
> +DATABASE_GRANTS_COUNT
> +2
> +SELECT count(*) FUNCTION_GRANTS_COUNT from mysql.procs_priv where routine_type='FUNCTION';
> +FUNCTION_GRANTS_COUNT
> +0
> +SELECT count(*) PROCEDURE_GRANTS_COUNT from mysql.procs_priv where routine_type='PROCEDURE';
> +PROCEDURE_GRANTS_COUNT
> +0
> +SELECT count(*) PROXY_USERS from mysql.proxies_priv;
> +PROXY_USERS
> +2
> +SELECT count(*) ROLE_GRANTS_COUNT from mysql.user where is_role='Y';
> +ROLE_GRANTS_COUNT
> +0
> +SELECT count(*) ROLE_MAPPINGS_COUNT from mysql.roles_mapping;
> +ROLE_MAPPINGS_COUNT
> +0
> +SELECT count(*) TABLE_GRANTS_COUNT from mysql.tables_priv;
> +TABLE_GRANTS_COUNT
> +0
> +SELECT count(*) USERS_COUNT from mysql.user where is_role='N';
> +USERS_COUNT
> +4
> +CREATE USER u1;
> +CREATE ROLE r1;
> +CREATE ROLE r2;
> +GRANT PROXY ON root TO u1;
> +GRANT SELECT ON *.* to u1;
> +GRANT SELECT ON *.* to r1;
> +GRANT DELETE ON mysql.* to u1;
> +GRANT DELETE ON mysql.* to r1;
> +GRANT INSERT ON mysql.user to u1;
> +GRANT INSERT ON mysql.user to r1;
> +GRANT UPDATE (host) ON mysql.user to u1;
> +GRANT UPDATE (host) ON mysql.user to r1;
> +GRANT r1 to u1;
> +GRANT r2 to r1;
> +create procedure mysql.test_proc (OUT param1 INT)
> +begin
> +select COUNT(*) into param1 from mysql.roles_mapping;
> +end|
> +GRANT EXECUTE ON PROCEDURE mysql.test_proc TO r1;
> +GRANT EXECUTE ON PROCEDURE mysql.test_proc TO u1;
> +CREATE FUNCTION mysql.test_func (param INT) RETURNS INT
> +RETURN (SELECT COUNT(*) FROM mysql.user);
> +GRANT EXECUTE ON FUNCTION mysql.test_func TO r1;
> +GRANT EXECUTE ON FUNCTION mysql.test_func TO u1;
> +SELECT * FROM information_schema.feedback
> +WHERE variable_name like '%Acl%';
> +VARIABLE_NAME	VARIABLE_VALUE
> +ACL_COUNT_COLUMN_GRANTS	2
> +ACL_COUNT_DATABASE_GRANTS	4
> +ACL_COUNT_FUNCTION_GRANTS	2
> +ACL_COUNT_PROCEDURE_GRANTS	2
> +ACL_COUNT_PROXY_USERS	3
> +ACL_COUNT_ROLE_MAPPINGS	4
> +ACL_COUNT_ROLES	2
> +ACL_COUNT_TABLE_GRANTS	2
> +ACL_COUNT_USERS	5
> +SELECT count(*) COLUMN_GRANTS_COUNT from mysql.columns_priv;
> +COLUMN_GRANTS_COUNT
> +2
> +SELECT count(*) DATABASE_GRANTS_COUNT from mysql.db;
> +DATABASE_GRANTS_COUNT
> +4
> +SELECT count(*) FUNCTION_GRANTS_COUNT from mysql.procs_priv where routine_type='FUNCTION';
> +FUNCTION_GRANTS_COUNT
> +2
> +SELECT count(*) PROCEDURE_GRANTS_COUNT from mysql.procs_priv where routine_type='PROCEDURE';
> +PROCEDURE_GRANTS_COUNT
> +2
> +SELECT count(*) PROXY_USERS from mysql.proxies_priv;
> +PROXY_USERS
> +3
> +SELECT count(*) ROLE_GRANTS_COUNT from mysql.user where is_role='Y';
> +ROLE_GRANTS_COUNT
> +2
> +SELECT count(*) ROLE_MAPPINGS_COUNT from mysql.roles_mapping;
> +ROLE_MAPPINGS_COUNT
> +4

Please, swap these two selects (ROLE_GRANTS_COUNT and
ROLE_MAPPINGS_COUNT) to have values go in the same order as in the I_S
table (it's easier to compare numbers that way). Same for the previous
set of selects.

> +SELECT count(*) TABLE_GRANTS_COUNT from mysql.tables_priv;
> +TABLE_GRANTS_COUNT
> +2
> +SELECT count(*) USERS_COUNT from mysql.user where is_role='N';
> +USERS_COUNT
> +5
> +DROP PROCEDURE mysql.test_proc;
> +DROP FUNCTION mysql.test_func;
> +DROP ROLE r2;
> +DROP ROLE r1;
> +DROP USER u1;
> diff --git a/sql/mysqld.cc b/sql/mysqld.cc
> index aa85a02..a7be18f 100644
> --- a/sql/mysqld.cc
> +++ b/sql/mysqld.cc
> @@ -8149,6 +8149,39 @@ int show_threadpool_idle_threads(THD *thd, SHOW_VAR *var, char *buff,
>  }
>  #endif
>  
> +int show_acl_statistics(THD *thd, SHOW_VAR *var, char *buff,
> +                       enum enum_var_type scope)
> +{
> +
> +  var->type= SHOW_ARRAY;
> +  var->value= buff;
> +
> +  SHOW_VAR *v= (SHOW_VAR *)buff;
> +  static ACL_statistics acl_stats;
> +  acl_stats= get_acl_statistics(thd);
> +  static SHOW_VAR statistics_array[]= {
> +    {"column_grants",    (char *)&acl_stats.column_grants,    SHOW_ULONG},
> +    {"database_grants",  (char *)&acl_stats.database_grants,  SHOW_ULONG},
> +    {"function_grants",  (char *)&acl_stats.function_grants,  SHOW_ULONG},
> +    {"procedure_grants", (char *)&acl_stats.procedure_grants, SHOW_ULONG},
> +    {"proxy_users",      (char *)&acl_stats.proxy_users,      SHOW_ULONG},
> +    {"role_mappings",    (char *)&acl_stats.role_mappings,    SHOW_ULONG},

rename to "role_grants" please.

> +    {"roles",            (char *)&acl_stats.roles,            SHOW_ULONG},
> +    {"table_grants",     (char *)&acl_stats.table_grants,     SHOW_ULONG},
> +    {"users",            (char *)&acl_stats.users,            SHOW_ULONG},
> +    {NullS, NullS, SHOW_LONG}
> +  };
> +  v->name= "count";

the name doesn't matter, no need to set it.

> +  v->value = statistics_array;
> +  v->type= SHOW_ARRAY;
> +  v++;
> +  v->name= NullS;
> +  v->value= NullS;
> +  v->type= SHOW_LONG;
> +
> +  return 0;
> +}
> +
>  /*
>    Variables shown by SHOW STATUS in alphabetical order
>  */
> @@ -8157,6 +8190,7 @@ SHOW_VAR status_vars[]= {
>    {"Aborted_clients",          (char*) &aborted_threads,        SHOW_LONG},
>    {"Aborted_connects",         (char*) &aborted_connects,       SHOW_LONG},
>    {"Access_denied_errors",     (char*) offsetof(STATUS_VAR, access_denied_errors), SHOW_LONG_STATUS},
> +  {"Acl",                      (char*) &show_acl_statistics,    SHOW_FUNC},
>    {"Binlog_bytes_written",     (char*) offsetof(STATUS_VAR, binlog_bytes_written), SHOW_LONGLONG_STATUS},
>    {"Binlog_cache_disk_use",    (char*) &binlog_cache_disk_use,  SHOW_LONG},
>    {"Binlog_cache_use",         (char*) &binlog_cache_use,       SHOW_LONG},
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index aaa1276..e97d094 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -10340,6 +10340,14 @@ applicable_roles_insert(ACL_USER_BASE *grantee, ACL_ROLE *role, void *ptr)
>    return 0;
>  }
>  
> +static my_bool count_column_privileges(void *grant_table,
> +                                       void *current_count)
> +{
> +  HASH hash_columns = ((GRANT_TABLE *)grant_table)->hash_columns;
> +  *(ulong *) current_count+= hash_columns.records;
> +  return 0;
> +}
> +
>  #else
>  bool check_grant(THD *, ulong, TABLE_LIST *, bool, uint, bool)
>  {
> @@ -10347,6 +10355,32 @@ bool check_grant(THD *, ulong, TABLE_LIST *, bool, uint, bool)
>  }
>  #endif /*NO_EMBEDDED_ACCESS_CHECKS */
>  
> +ACL_statistics get_acl_statistics(THD* thd)
> +{
> +  ACL_statistics result;
> +  bzero(&result, sizeof(result));
> +#ifndef NO_EMBEDDED_ACCESS_CHECKS
> +  mysql_rwlock_rdlock(&LOCK_grant);
> +  mysql_mutex_lock(&acl_cache->lock);
> +
> +  result.users= acl_users.elements;
> +  result.roles= acl_roles.records;
> +  result.database_grants= acl_dbs.elements;
> +  result.function_grants= proc_priv_hash.records;
> +  result.procedure_grants= proc_priv_hash.records;
> +  result.proxy_users= acl_proxy_users.elements;
> +  result.role_mappings= acl_roles_mappings.records;
> +  result.table_grants= column_priv_hash.records;
> +  my_hash_iterate(&column_priv_hash, count_column_privileges,
> +                  &result.column_grants);
> +
> +  mysql_mutex_unlock(&acl_cache->lock);
> +  mysql_rwlock_unlock(&LOCK_grant);
> +
> +#endif
> +  return result;
> +}
> +
>  int fill_schema_enabled_roles(THD *thd, TABLE_LIST *tables, COND *cond)
>  {
>    TABLE *table= tables->table;
> diff --git a/sql/sql_acl.h b/sql/sql_acl.h
> index 55b00a9..3abf5d7 100644
> --- a/sql/sql_acl.h
> +++ b/sql/sql_acl.h
> @@ -406,4 +406,14 @@ int acl_set_default_role(THD *thd, const char *host, const char *user,
>  extern ulong role_global_merges, role_db_merges, role_table_merges,
>               role_column_merges, role_routine_merges;
>  #endif
> +/**
> +  Specify which statistical information (counts) to retrieve from
> +  the ACL data structures.
> +*/
> +struct ACL_statistics
> +{
> +  ulong column_grants, database_grants, function_grants, procedure_grants,
> +        proxy_users, role_mappings, roles, table_grants, users;
> +};
> +ACL_statistics get_acl_statistics(THD *thd);
>  #endif /* SQL_ACL_INCLUDED */

I would do it slightly differently. No ACL_statistics structure and no
get_acl_statistics function, but instead I'd use SHOW_ARRAY in
mysqld.cc:

     {"Acl", (char*) &show_acl_statistics,    SHOW_ARRAY},

and in sql_acl.cc I'd have an array

  static SHOW_VAR statistics_array[]= {
    {"column_grants",    (char *)&get_column_grants,          SHOW_SIMPLE_FUNC},
    {"database_grants",  (char *)&acl_dbs.elements,           SHOW_ULONG},
    {"function_grants",  (char *)&proc_priv_hash.records,     SHOW_ULONG},
    {"procedure_grants", (char *)&proc_priv_hash.records,     SHOW_ULONG},
    {"proxy_users",      (char *)&acl_proxy_users.elements,   SHOW_ULONG},
    {"role_grants",      (char *)&acl_roles_mappings.records, SHOW_ULONG},
    {"roles",            (char *)&acl_roles.records,          SHOW_ULONG},
    {"table_grants",     (char *)&column_priv_hash.records,   SHOW_ULONG},
    {"users",            (char *)&acl_users.elements,         SHOW_ULONG},
    {NullS, NullS, SHOW_LONG}
  };

That is, only column_grants need a function call (with mutex locks),
and it's SHOW_SIMPLE_FUNC, not SHOW_FUNC.

The different is that a typical (for example)

   SHOW STATUS LIKE '%sort%'

will have to invoke all SHOW_FUNC functions, because otherwise it cannot
know names of the variables and cannot match them. But SHOW_SIMPLE_FUNC
never returns a SHOW_ARRAY, so it can be matched without a function
call (so, no mutex lock unless the user specifically asked for
Acl_column_grants value).

Also, status variables are allowed to be "slightly imprecise", so
they're often incremented without a mutex. That is, you aren't required
to protect them here with a mutex either.

Regards,
Sergei