From 957cb7b7ba355184aebf0f5dc91b7f2aa620c0e0 Mon Sep 17 00:00:00 2001 From: Anel Husakovic Date: Tue, 12 May 2020 16:16:05 +0200 Subject: MDEV-22312: Bad error message for SET DEFAULT ROLE when user account is not granted the role - `SET DEFAULT ROLE xxx [FOR yyy]` should say: "User yyy has not been granted a role xxx" if: - The current user (not the user `yyy` in the FOR clause) can see the role xxx. It can see the role if: * role exists in `mysql.roles_mappings` (traverse the graph), * If the current user has read access on `mysql.user` table - in that case, it can see all roles, granted or not. - Otherwise it should be "Invalid role specification". In other words, it should not be possible to use `SET DEFAULT ROLE` to discover whether a specific role exist or not. --- sql/set_var.cc | 16 ++++++-- sql/set_var.h | 1 + sql/sql_acl.cc | 120 +++++++++++++++++++++++++++++++++++++++------------------ sql/sql_acl.h | 2 +- 4 files changed, 97 insertions(+), 42 deletions(-) (limited to 'sql') diff --git a/sql/set_var.cc b/sql/set_var.cc index b5f017eb85e..4b1ef63c5f3 100644 --- a/sql/set_var.cc +++ b/sql/set_var.cc @@ -923,8 +923,17 @@ int set_var_default_role::check(THD *thd) { #ifndef NO_EMBEDDED_ACCESS_CHECKS real_user= get_current_user(thd, user); - int status= acl_check_set_default_role(thd, real_user->host.str, real_user->user.str); - return status; + real_role= role.str; + if (role.str == current_role.str) + { + if (!thd->security_ctx->priv_role[0]) + real_role= "NONE"; + else + real_role= thd->security_ctx->priv_role; + } + + return acl_check_set_default_role(thd, real_user->host.str, + real_user->user.str, real_role); #else return 0; #endif @@ -935,7 +944,8 @@ int set_var_default_role::update(THD *thd) #ifndef NO_EMBEDDED_ACCESS_CHECKS Reprepare_observer *save_reprepare_observer= thd->m_reprepare_observer; thd->m_reprepare_observer= 0; - int res= acl_set_default_role(thd, real_user->host.str, real_user->user.str, role.str); + int res= acl_set_default_role(thd, real_user->host.str, real_user->user.str, + real_role); thd->m_reprepare_observer= save_reprepare_observer; return res; #else diff --git a/sql/set_var.h b/sql/set_var.h index fc79e906270..572ff9fe60a 100644 --- a/sql/set_var.h +++ b/sql/set_var.h @@ -339,6 +339,7 @@ class set_var_default_role: public set_var_base { LEX_USER *user, *real_user; LEX_STRING role; + const char *real_role; public: set_var_default_role(LEX_USER *user_arg, LEX_STRING role_arg) : user(user_arg), role(role_arg) {} diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index b6a6f806e50..af8685c458b 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -201,6 +201,8 @@ LEX_STRING current_user= { C_STRING_WITH_LEN("*current_user") }; LEX_STRING current_role= { C_STRING_WITH_LEN("*current_role") }; LEX_STRING current_user_and_current_role= { C_STRING_WITH_LEN("*current_user_and_current_role") }; +class ACL_USER; +static ACL_USER *find_user_or_anon(const char *host, const char *user, const char *ip); #ifndef NO_EMBEDDED_ACCESS_CHECKS static plugin_ref old_password_plugin; @@ -2034,8 +2036,21 @@ bool acl_getroot(Security_context *sctx, char *user, char *host, DBUG_RETURN(res); } -static int check_user_can_set_role(const char *user, const char *host, - const char *ip, const char *rolename, ulonglong *access) +static int check_role_is_granted_callback(ACL_USER_BASE *grantee, void *data) +{ + LEX_CSTRING *rolename= static_cast(data); + if (rolename->length == grantee->user.length && + !strcmp(rolename->str, grantee->user.str)) + return -1; // End search, we've found our role. + + /* Keep looking, we haven't found our role yet. */ + return 0; +} + + +static int check_user_can_set_role(THD *thd, const char *user, const char *host, + const char *ip, const char *rolename, + ulonglong *access) { ACL_ROLE *role; ACL_USER_BASE *acl_user_base; @@ -2053,8 +2068,7 @@ static int check_user_can_set_role(const char *user, const char *host, acl_user= find_user_wild(host, user, ip); if (acl_user == NULL) { - my_error(ER_INVALID_CURRENT_USER, MYF(0), rolename); - result= -1; + result= ER_INVALID_CURRENT_USER; } else if (access) *access= acl_user->access; @@ -2065,9 +2079,9 @@ static int check_user_can_set_role(const char *user, const char *host, role= find_acl_role(rolename); /* According to SQL standard, the same error message must be presented */ - if (role == NULL) { - my_error(ER_INVALID_ROLE, MYF(0), rolename); - result= -1; + if (role == NULL) + { + result= ER_INVALID_ROLE; goto end; } @@ -2088,7 +2102,6 @@ static int check_user_can_set_role(const char *user, const char *host, /* According to SQL standard, the same error message must be presented */ if (!is_granted) { - my_error(ER_INVALID_ROLE, MYF(0), rolename); result= 1; goto end; } @@ -2097,17 +2110,66 @@ static int check_user_can_set_role(const char *user, const char *host, { *access = acl_user->access | role->access; } + end: mysql_mutex_unlock(&acl_cache->lock); - return result; + /* We present different error messages depending if the user has sufficient + privileges to know if the INVALID_ROLE exists. */ + switch (result) + { + case ER_INVALID_CURRENT_USER: + my_error(ER_INVALID_CURRENT_USER, MYF(0), rolename); + break; + case ER_INVALID_ROLE: + /* Role doesn't exist at all */ + my_error(ER_INVALID_ROLE, MYF(0), rolename); + break; + case 1: + StringBuffer<1024> c_usr; + LEX_CSTRING role_lex; + /* First, check if current user can see mysql database. */ + bool read_access= !check_access(thd, SELECT_ACL, "mysql", NULL, NULL, 1, 1); + + role_lex.str= rolename; + role_lex.length= strlen(rolename); + mysql_mutex_lock(&acl_cache->lock); + ACL_USER *cur_user= find_user_or_anon(thd->security_ctx->priv_host, + thd->security_ctx->priv_user, + thd->security_ctx->ip); + + /* If the current user does not have select priv to mysql database, + see if the current user can discover the role if it was granted to him. + */ + if (cur_user && (read_access || + traverse_role_graph_down(cur_user, &role_lex, + check_role_is_granted_callback, + NULL) == -1)) + { + /* Role is not granted but current user can see the role */ + c_usr.append(user, strlen(user)); + c_usr.append('@'); + c_usr.append(host, strlen(host)); + my_printf_error(ER_INVALID_ROLE, "User %`s has not been granted role %`s", + MYF(0), c_usr.c_ptr(), rolename); + } + else + { + /* Role is not granted and current user cannot see the role */ + my_error(ER_INVALID_ROLE, MYF(0), rolename); + } + mysql_mutex_unlock(&acl_cache->lock); + break; + } + + return result; } + int acl_check_setrole(THD *thd, char *rolename, ulonglong *access) { - /* Yes! priv_user@host. Don't ask why - that's what check_access() does. */ - return check_user_can_set_role(thd->security_ctx->priv_user, - thd->security_ctx->host, thd->security_ctx->ip, rolename, access); + return check_user_can_set_role(thd, thd->security_ctx->priv_user, + thd->security_ctx->host, thd->security_ctx->ip, rolename, access); } @@ -2886,9 +2948,12 @@ WSREP_ERROR_LABEL: DBUG_RETURN(result); } -int acl_check_set_default_role(THD *thd, const char *host, const char *user) +int acl_check_set_default_role(THD *thd, const char *host, const char *user, + const char *role) { - return check_alter_user(thd, host, user); + DBUG_ENTER("acl_check_set_default_role"); + DBUG_RETURN(check_alter_user(thd, host, user) || + check_user_can_set_role(thd, user, host, NULL, role, NULL)); } int acl_set_default_role(THD *thd, const char *host, const char *user, @@ -2910,16 +2975,6 @@ int acl_set_default_role(THD *thd, const char *host, const char *user, DBUG_PRINT("enter",("host: '%s' user: '%s' rolename: '%s'", safe_str(user), safe_str(host), safe_str(rolename))); - if (rolename == current_role.str) { - if (!thd->security_ctx->priv_role[0]) - rolename= "NONE"; - else - rolename= thd->security_ctx->priv_role; - } - - if (check_user_can_set_role(user, host, host, rolename, NULL)) - DBUG_RETURN(result); - if (!strcasecmp(rolename, "NONE")) clear_role= TRUE; @@ -3370,7 +3425,7 @@ static bool test_if_create_new_users(THD *thd) if (!(db_access & INSERT_ACL)) { if (check_grant(thd, INSERT_ACL, &tl, FALSE, UINT_MAX, TRUE)) - create_new_users=0; + create_new_users=0; } } return create_new_users; @@ -8508,17 +8563,6 @@ void get_mqh(const char *user, const char *host, USER_CONN *uc) mysql_mutex_unlock(&acl_cache->lock); } -static int check_role_is_granted_callback(ACL_USER_BASE *grantee, void *data) -{ - LEX_CSTRING *rolename= static_cast(data); - if (rolename->length == grantee->user.length && - !strcmp(rolename->str, grantee->user.str)) - return -1; // End search, we've found our role. - - /* Keep looking, we haven't found our role yet. */ - return 0; -} - /* Initialize a TABLE_LIST array and open grant tables @@ -10329,7 +10373,7 @@ acl_check_proxy_grant_access(THD *thd, const char *host, const char *user, Security context in THD contains two pairs of (user,host): 1. (user,host) pair referring to inbound connection. 2. (priv_user,priv_host) pair obtained from mysql.user table after doing - authnetication of incoming connection. + authentication of incoming connection. Privileges should be checked wrt (priv_user, priv_host) tuple, because (user,host) pair obtained from inbound connection may have different values than what is actually stored in mysql.user table and while granting @@ -10746,7 +10790,7 @@ int fill_schema_user_privileges(THD *thd, TABLE_LIST *tables, COND *cond) ulong j,test_access= want_access & ~GRANT_ACL; for (priv_id=0, j = SELECT_ACL;j <= GLOBAL_ACLS; priv_id++,j <<= 1) { - if (test_access & j) + if (test_access & j) { if (update_schema_privilege(thd, table, buff, 0, 0, 0, 0, command_array[priv_id], diff --git a/sql/sql_acl.h b/sql/sql_acl.h index c191cb83de5..3bd896cab79 100644 --- a/sql/sql_acl.h +++ b/sql/sql_acl.h @@ -402,7 +402,7 @@ bool acl_check_proxy_grant_access (THD *thd, const char *host, const char *user, bool with_grant); int acl_setrole(THD *thd, char *rolename, ulonglong access); int acl_check_setrole(THD *thd, char *rolename, ulonglong *access); -int acl_check_set_default_role(THD *thd, const char *host, const char *user); +int acl_check_set_default_role(THD *thd, const char *host, const char *user, const char *role); int acl_set_default_role(THD *thd, const char *host, const char *user, const char *rolename); -- cgit v1.2.1 From a1b3bebe1f7f7221daf520e35b81e13c1478d189 Mon Sep 17 00:00:00 2001 From: Anel Husakovic Date: Thu, 28 May 2020 19:34:27 +0200 Subject: fix pre-definition for embedded server for find_user_or_anon() Pre-definitions are allowed for non-embedded. Failur catched with: ``` cmake ../../10.1 -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_COMPILER=g++-9 -DCMAKE_C_COMPILER=gcc-9 -DWITH_EMBEDDED_SERVER=ON -DCMAKE_BUILD_TYPE=Debug -DPLUGIN_{ARCHIVE,TOKUDB,MROONGA,OQGRAPH,ROCKSDB,PERFSCHEMA,SPIDER,SPHINX}=N -DMYSQL_MAINTAINER_MODE=ON -DNOT_FOR_DISTRIBUTION=ON ``` Alternative fix would be ``` --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -201,8 +201,10 @@ LEX_STRING current_user= { C_STRING_WITH_LEN("*current_user") }; LEX_STRING current_role= { C_STRING_WITH_LEN("*current_role") }; LEX_STRING current_user_and_current_role= { C_STRING_WITH_LEN("*current_user_and_current_role") }; +#ifndef EMBEDDED_LIBRARY class ACL_USER; static ACL_USER *find_user_or_anon(const char *host, const char *user, const char *ip); +#endif ``` --- sql/sql_acl.cc | 50 ++++++++++++++++++++++---------------------------- 1 file changed, 22 insertions(+), 28 deletions(-) (limited to 'sql') diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index af8685c458b..6e6135b75bb 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -201,9 +201,6 @@ LEX_STRING current_user= { C_STRING_WITH_LEN("*current_user") }; LEX_STRING current_role= { C_STRING_WITH_LEN("*current_role") }; LEX_STRING current_user_and_current_role= { C_STRING_WITH_LEN("*current_user_and_current_role") }; -class ACL_USER; -static ACL_USER *find_user_or_anon(const char *host, const char *user, const char *ip); - #ifndef NO_EMBEDDED_ACCESS_CHECKS static plugin_ref old_password_plugin; #endif @@ -2047,6 +2044,28 @@ static int check_role_is_granted_callback(ACL_USER_BASE *grantee, void *data) return 0; } +/* + unlike find_user_exact and find_user_wild, + this function finds anonymous users too, it's when a + user is not empty, but priv_user (acl_user->user) is empty. +*/ +static ACL_USER *find_user_or_anon(const char *host, const char *user, const char *ip) +{ + ACL_USER *result= NULL; + mysql_mutex_assert_owner(&acl_cache->lock); + for (uint i=0; i < acl_users.elements; i++) + { + ACL_USER *acl_user_tmp= dynamic_element(&acl_users, i, ACL_USER*); + if ((!acl_user_tmp->user.str || + !strcmp(user, acl_user_tmp->user.str)) && + compare_hostname(&acl_user_tmp->host, host, ip)) + { + result= acl_user_tmp; + break; + } + } + return result; +} static int check_user_can_set_role(THD *thd, const char *user, const char *host, const char *ip, const char *rolename, @@ -3128,31 +3147,6 @@ bool is_acl_user(const char *host, const char *user) return res; } - -/* - unlike find_user_exact and find_user_wild, - this function finds anonymous users too, it's when a - user is not empty, but priv_user (acl_user->user) is empty. -*/ -static ACL_USER *find_user_or_anon(const char *host, const char *user, const char *ip) -{ - ACL_USER *result= NULL; - mysql_mutex_assert_owner(&acl_cache->lock); - for (uint i=0; i < acl_users.elements; i++) - { - ACL_USER *acl_user_tmp= dynamic_element(&acl_users, i, ACL_USER*); - if ((!acl_user_tmp->user.str || - !strcmp(user, acl_user_tmp->user.str)) && - compare_hostname(&acl_user_tmp->host, host, ip)) - { - result= acl_user_tmp; - break; - } - } - return result; -} - - /* Find first entry that matches the specified user@host pair */ -- cgit v1.2.1 From a2932e86b5dcbc76bf2bef545cf942f202abd3e8 Mon Sep 17 00:00:00 2001 From: Alexander Barkov Date: Fri, 29 May 2020 15:31:24 +0400 Subject: MDEV-22744 *SAN: sql/item_xmlfunc.cc:791:43: runtime error: downcast of address ... which does not point to an object of type 'Item_func' note: object is of type 'Item_bool' (on optimized builds) In Item_nodeset_func_predicate::val_nodeset, args[1] is not necessarily an Item_func descendant. It can be Item_bool. Removing a wrong cast. It was not really needed anyway. --- sql/item_xmlfunc.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'sql') diff --git a/sql/item_xmlfunc.cc b/sql/item_xmlfunc.cc index d33cd30a928..97ef24b0851 100644 --- a/sql/item_xmlfunc.cc +++ b/sql/item_xmlfunc.cc @@ -797,7 +797,6 @@ String *Item_nodeset_func_attributebyname::val_nodeset(String *nodeset) String *Item_nodeset_func_predicate::val_nodeset(String *str) { Item_nodeset_func *nodeset_func= (Item_nodeset_func*) args[0]; - Item_func *comp_func= (Item_func*)args[1]; uint pos= 0, size; prepare(str); size= fltend - fltbeg; @@ -807,7 +806,7 @@ String *Item_nodeset_func_predicate::val_nodeset(String *str) ((XPathFilter*)(&nodeset_func->context_cache))->append_element(flt->num, flt->pos, size); - if (comp_func->val_int()) + if (args[1]->val_int()) ((XPathFilter*)str)->append_element(flt->num, pos++); } return str; -- cgit v1.2.1 From 1055a7f4fc0a095e6ab3a6a138d64688b764a78b Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Fri, 11 Oct 2019 17:20:28 +0400 Subject: Simplified away statistics_for_tables_is_needed() Removed redundant loops, integrated logics into the caller instead. Unified condition in read_statistics_for_tables(), less "table_share != NULL" checks, no more potential "table_share == NULL" dereferencing. Part of MDEV-19061 - table_share used for reading statistical tables is not protected --- sql/sql_statistics.cc | 112 ++++++++++++++------------------------------------ 1 file changed, 31 insertions(+), 81 deletions(-) (limited to 'sql') diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index 11e2db9ae44..0f8daa6e52f 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -3035,71 +3035,6 @@ void delete_stat_values_for_table_share(TABLE_SHARE *table_share) } -/** - @brief - Check whether any statistics is to be read for tables from a table list - - @param - thd The thread handle - @param - tables The tables list for whose tables the check is to be done - - @details - The function checks whether for any of the tables opened and locked for - a statement statistics from statistical tables is needed to be read. - - @retval - TRUE statistics for any of the tables is needed to be read - @retval - FALSE Otherwise -*/ - -static -bool statistics_for_tables_is_needed(THD *thd, TABLE_LIST *tables) -{ - if (!tables) - return FALSE; - - /* - Do not read statistics for any query that explicity involves - statistical tables, failure to to do so we may end up - in a deadlock. - */ - - for (TABLE_LIST *tl= tables; tl; tl= tl->next_global) - { - if (!tl->is_view_or_derived() && !is_temporary_table(tl) && tl->table) - { - TABLE_SHARE *table_share= tl->table->s; - if (table_share && - table_share->table_category != TABLE_CATEGORY_USER - && is_stat_table(tl->db, tl->alias)) - return FALSE; - } - } - - for (TABLE_LIST *tl= tables; tl; tl= tl->next_global) - { - if (!tl->is_view_or_derived() && !is_temporary_table(tl) && tl->table) - { - TABLE_SHARE *table_share= tl->table->s; - if (table_share && - table_share->stats_cb.stats_can_be_read && - (!table_share->stats_cb.stats_is_read || - (!table_share->stats_cb.histograms_are_read && - thd->variables.optimizer_use_condition_selectivity > 3))) - return TRUE; - if (table_share->stats_cb.stats_is_read) - tl->table->stats_is_read= TRUE; - if (table_share->stats_cb.histograms_are_read) - tl->table->histograms_are_read= TRUE; - } - } - - return FALSE; -} - - /** @brief Read histogram for a table from the persistent statistical tables @@ -3221,13 +3156,16 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) if (thd->bootstrap || thd->variables.use_stat_tables == NEVER) DBUG_RETURN(0); + bool found_stat_table= false; + bool statistics_for_tables_is_needed= false; + for (TABLE_LIST *tl= tables; tl; tl= tl->next_global) { - if (tl->table) + TABLE_SHARE *table_share; + if (!tl->is_view_or_derived() && tl->table && (table_share= tl->table->s) && + table_share->tmp_table == NO_TMP_TABLE) { - TABLE_SHARE *table_share= tl->table->s; - if (table_share && table_share->table_category == TABLE_CATEGORY_USER && - table_share->tmp_table == NO_TMP_TABLE) + if (table_share->table_category == TABLE_CATEGORY_USER) { if (table_share->stats_cb.stats_can_be_read || !alloc_statistics_for_table_share(thd, table_share)) @@ -3244,15 +3182,29 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) for ( ; *field_ptr; field_ptr++, table_field_ptr++) (*table_field_ptr)->read_stats= (*field_ptr)->read_stats; tl->table->stats_is_read= table_share->stats_cb.stats_is_read; + tl->table->histograms_are_read= + table_share->stats_cb.histograms_are_read; + + if (!tl->table->stats_is_read || + (!table_share->stats_cb.histograms_are_read && + thd->variables.optimizer_use_condition_selectivity > 3)) + statistics_for_tables_is_needed= true; } } } + else if (is_stat_table(tl->db, tl->alias)) + found_stat_table= true; } } DEBUG_SYNC(thd, "statistics_read_start"); - if (!statistics_for_tables_is_needed(thd, tables)) + /* + Do not read statistics for any query that explicity involves + statistical tables, failure to to do so we may end up + in a deadlock. + */ + if (found_stat_table || !statistics_for_tables_is_needed) DBUG_RETURN(0); if (open_stat_tables(thd, stat_tables, &open_tables_backup, FALSE)) @@ -3260,14 +3212,12 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) for (TABLE_LIST *tl= tables; tl; tl= tl->next_global) { - if (!tl->is_view_or_derived() && !is_temporary_table(tl) && tl->table) - { - TABLE_SHARE *table_share= tl->table->s; - if (table_share && !(table_share->table_category == TABLE_CATEGORY_USER)) - continue; - - if (table_share && - table_share->stats_cb.stats_can_be_read && + TABLE_SHARE *table_share; + if (!tl->is_view_or_derived() && tl->table && (table_share= tl->table->s) && + table_share->tmp_table == NO_TMP_TABLE && + table_share->table_category == TABLE_CATEGORY_USER) + { + if (table_share->stats_cb.stats_can_be_read && !table_share->stats_cb.stats_is_read) { (void) read_statistics_for_table(thd, tl->table, stat_tables); @@ -3275,8 +3225,8 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) } if (table_share->stats_cb.stats_is_read) tl->table->stats_is_read= TRUE; - if (thd->variables.optimizer_use_condition_selectivity > 3 && - table_share && table_share->stats_cb.stats_can_be_read && + if (thd->variables.optimizer_use_condition_selectivity > 3 && + table_share->stats_cb.stats_can_be_read && !table_share->stats_cb.histograms_are_read) { (void) read_histograms_for_table(thd, tl->table, stat_tables); @@ -3285,7 +3235,7 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) if (table_share->stats_cb.histograms_are_read) tl->table->histograms_are_read= TRUE; } - } + } close_system_tables(thd, &open_tables_backup); -- cgit v1.2.1 From 609a0d3db3fecc12c2d63b52bec3e3a305e6c702 Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Wed, 16 Oct 2019 18:19:59 +0400 Subject: Thread safe statistics loading Previously multiple threads were allowed to load statistics concurrently. There were no known problems caused by this. But given amount of data races in this code, it'd happen sooner or later. To avoid scalability bottleneck, statistics loading is protected by per-TABLE_SHARE atomic variable. Whenever statistics were loaded by preceding statement (hot-path), a scalable load-acquire check is performed. Whenever statistics have to be loaded anew, mutual exclusion for loaders is established by atomic variable. If statistics are being loaded concurrently, statement waits until load is completed. TABLE_STATISTICS_CB::stats_can_be_read and TABLE_STATISTICS_CB::stats_is_read are replaced with a tri state atomic variable. Part of MDEV-19061 - table_share used for reading statistical tables is not protected --- sql/sql_statistics.cc | 99 +++++++++++++++++++++++---------------------------- sql/table.cc | 2 -- sql/table.h | 75 +++++++++++++++++++++++++++++++++++--- 3 files changed, 116 insertions(+), 60 deletions(-) (limited to 'sql') diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index 0f8daa6e52f..a99dcca89e4 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -2204,27 +2204,13 @@ static int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *table_share) DBUG_ENTER("alloc_statistics_for_table_share"); - DEBUG_SYNC(thd, "statistics_mem_alloc_start1"); - DEBUG_SYNC(thd, "statistics_mem_alloc_start2"); - - mysql_mutex_lock(&table_share->LOCK_share); - - if (stats_cb->stats_can_be_read) - { - mysql_mutex_unlock(&table_share->LOCK_share); - DBUG_RETURN(0); - } - Table_statistics *table_stats= stats_cb->table_stats; if (!table_stats) { table_stats= (Table_statistics *) alloc_root(&stats_cb->mem_root, sizeof(Table_statistics)); if (!table_stats) - { - mysql_mutex_unlock(&table_share->LOCK_share); DBUG_RETURN(1); - } memset(table_stats, 0, sizeof(Table_statistics)); stats_cb->table_stats= table_stats; } @@ -2290,13 +2276,7 @@ static int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *table_share) } } } - - if (column_stats && index_stats && idx_avg_frequency) - stats_cb->stats_can_be_read= TRUE; - - mysql_mutex_unlock(&table_share->LOCK_share); - - DBUG_RETURN(0); + DBUG_RETURN(column_stats && index_stats && idx_avg_frequency ? 0 : 1); } @@ -2913,11 +2893,22 @@ int read_statistics_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables) Field **field_ptr; KEY *key_info, *key_info_end; TABLE_SHARE *table_share= table->s; - Table_statistics *read_stats= table_share->stats_cb.table_stats; DBUG_ENTER("read_statistics_for_table"); + DEBUG_SYNC(thd, "statistics_mem_alloc_start1"); + DEBUG_SYNC(thd, "statistics_mem_alloc_start2"); + + if (!table_share->stats_cb.start_stats_load()) + DBUG_RETURN(table_share->stats_cb.stats_are_ready() ? 0 : 1); + + if (alloc_statistics_for_table_share(thd, table_share)) + { + table_share->stats_cb.abort_stats_load(); + DBUG_RETURN(1); + } /* Read statistics from the statistical table table_stats */ + Table_statistics *read_stats= table_share->stats_cb.table_stats; stat_table= stat_tables[TABLE_STAT].table; Table_stat table_stat(stat_table, table); table_stat.set_key_fields(); @@ -2995,9 +2986,7 @@ int read_statistics_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables) } } } - - table->stats_is_read= TRUE; - + table_share->stats_cb.end_stats_load(); DBUG_RETURN(0); } @@ -3146,6 +3135,23 @@ int read_statistics_for_tables_if_needed(THD *thd, TABLE_LIST *tables) } +static void dump_stats_from_share_to_table(TABLE *table) +{ + TABLE_SHARE *table_share= table->s; + KEY *key_info= table_share->key_info; + KEY *key_info_end= key_info + table_share->keys; + KEY *table_key_info= table->key_info; + for ( ; key_info < key_info_end; key_info++, table_key_info++) + table_key_info->read_stats= key_info->read_stats; + + Field **field_ptr= table_share->field; + Field **table_field_ptr= table->field; + for ( ; *field_ptr; field_ptr++, table_field_ptr++) + (*table_field_ptr)->read_stats= (*field_ptr)->read_stats; + table->stats_is_read= true; +} + + int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) { TABLE_LIST stat_tables[STATISTICS_TABLES]; @@ -3167,30 +3173,17 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) { if (table_share->table_category == TABLE_CATEGORY_USER) { - if (table_share->stats_cb.stats_can_be_read || - !alloc_statistics_for_table_share(thd, table_share)) + if (table_share->stats_cb.stats_are_ready()) { - if (table_share->stats_cb.stats_can_be_read) - { - KEY *key_info= table_share->key_info; - KEY *key_info_end= key_info + table_share->keys; - KEY *table_key_info= tl->table->key_info; - for ( ; key_info < key_info_end; key_info++, table_key_info++) - table_key_info->read_stats= key_info->read_stats; - Field **field_ptr= table_share->field; - Field **table_field_ptr= tl->table->field; - for ( ; *field_ptr; field_ptr++, table_field_ptr++) - (*table_field_ptr)->read_stats= (*field_ptr)->read_stats; - tl->table->stats_is_read= table_share->stats_cb.stats_is_read; - tl->table->histograms_are_read= - table_share->stats_cb.histograms_are_read; - - if (!tl->table->stats_is_read || - (!table_share->stats_cb.histograms_are_read && - thd->variables.optimizer_use_condition_selectivity > 3)) - statistics_for_tables_is_needed= true; - } + if (!tl->table->stats_is_read) + dump_stats_from_share_to_table(tl->table); + tl->table->histograms_are_read= + table_share->stats_cb.histograms_are_read; + if (table_share->stats_cb.histograms_are_read || + thd->variables.optimizer_use_condition_selectivity <= 3) + continue; } + statistics_for_tables_is_needed= true; } else if (is_stat_table(tl->db, tl->alias)) found_stat_table= true; @@ -3217,16 +3210,14 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) table_share->tmp_table == NO_TMP_TABLE && table_share->table_category == TABLE_CATEGORY_USER) { - if (table_share->stats_cb.stats_can_be_read && - !table_share->stats_cb.stats_is_read) + if (!tl->table->stats_is_read) { - (void) read_statistics_for_table(thd, tl->table, stat_tables); - table_share->stats_cb.stats_is_read= TRUE; + if (!read_statistics_for_table(thd, tl->table, stat_tables)) + dump_stats_from_share_to_table(tl->table); + else + continue; } - if (table_share->stats_cb.stats_is_read) - tl->table->stats_is_read= TRUE; if (thd->variables.optimizer_use_condition_selectivity > 3 && - table_share->stats_cb.stats_can_be_read && !table_share->stats_cb.histograms_are_read) { (void) read_histograms_for_table(thd, tl->table, stat_tables); diff --git a/sql/table.cc b/sql/table.cc index 192faa0d00e..1d3bebcc5d5 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -416,8 +416,6 @@ void TABLE_SHARE::destroy() delete_stat_values_for_table_share(this); free_root(&stats_cb.mem_root, MYF(0)); - stats_cb.stats_can_be_read= FALSE; - stats_cb.stats_is_read= FALSE; stats_cb.histograms_can_be_read= FALSE; stats_cb.histograms_are_read= FALSE; diff --git a/sql/table.h b/sql/table.h index 29b4cfdbcf3..85ba7b29ce2 100644 --- a/sql/table.h +++ b/sql/table.h @@ -594,15 +594,82 @@ enum open_frm_error { from persistent statistical tables */ -struct TABLE_STATISTICS_CB +class TABLE_STATISTICS_CB { + class Statistics_state + { + enum state_codes + { + EMPTY, /** data is not loaded */ + LOADING, /** data is being loaded in some connection */ + READY /** data is loaded and available for use */ + }; + int32 state; + + public: + /** No state copy */ + Statistics_state &operator=(const Statistics_state &) { return *this; } + + /** Checks if data loading have been completed */ + bool is_ready() const + { + return my_atomic_load32_explicit(const_cast(&state), + MY_MEMORY_ORDER_ACQUIRE) == READY; + } + + /** + Sets mutual exclusion for data loading + + If stats are in LOADING state, waits until state change. + + @return + @retval true atomic EMPTY -> LOADING transfer completed, ok to load + @retval false stats are in READY state, no need to load + */ + bool start_load() + { + for (;;) + { + int32 expected= EMPTY; + if (my_atomic_cas32_weak_explicit(&state, &expected, LOADING, + MY_MEMORY_ORDER_RELAXED, + MY_MEMORY_ORDER_RELAXED)) + return true; + if (expected == READY) + return false; + (void) LF_BACKOFF; + } + } + + /** Marks data available for subsequent use */ + void end_load() + { + DBUG_ASSERT(my_atomic_load32_explicit(&state, MY_MEMORY_ORDER_RELAXED) == + LOADING); + my_atomic_store32_explicit(&state, READY, MY_MEMORY_ORDER_RELEASE); + } + + /** Restores empty state on error (e.g. OOM) */ + void abort_load() + { + DBUG_ASSERT(my_atomic_load32_explicit(&state, MY_MEMORY_ORDER_RELAXED) == + LOADING); + my_atomic_store32_explicit(&state, EMPTY, MY_MEMORY_ORDER_RELAXED); + } + }; + + class Statistics_state stats_state; + +public: MEM_ROOT mem_root; /* MEM_ROOT to allocate statistical data for the table */ Table_statistics *table_stats; /* Structure to access the statistical data */ - bool stats_can_be_read; /* Memory for statistical data is allocated */ - bool stats_is_read; /* Statistical data for table has been read - from statistical tables */ bool histograms_can_be_read; bool histograms_are_read; + + bool stats_are_ready() const { return stats_state.is_ready(); } + bool start_stats_load() { return stats_state.start_load(); } + void end_stats_load() { stats_state.end_load(); } + void abort_stats_load() { stats_state.abort_load(); } }; -- cgit v1.2.1 From c2798784931a59f996275e14220b74f450828b4b Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Wed, 16 Oct 2019 19:00:43 +0400 Subject: Thread safe histograms loading Previously multiple threads were allowed to load histograms concurrently. There were no known problems caused by this. But given amount of data races in this code, it'd happen sooner or later. To avoid scalability bottleneck, histograms loading is protected by per-TABLE_SHARE atomic variable. Whenever histograms were loaded by preceding statement (hot-path), a scalable load-acquire check is performed. Whenever histograms have to be loaded anew, mutual exclusion for loaders is established by atomic variable. If histograms are being loaded concurrently, statement waits until load is completed. - Table_statistics::total_hist_size moved to TABLE_STATISTICS_CB: only meaningful within TABLE_SHARE (not used for collected stats). - TABLE_STATISTICS_CB::histograms_can_be_read and TABLE_STATISTICS_CB::histograms_are_read are replaced with a tri state atomic variable. - Simplified away alloc_histograms_for_table_share(). Note: there's still likely a data race if a thread attempts accessing histograms data after it failed to load it (because of concurrent load). It was there previously and goes out of the scope of this effort. One way of fixing it could be reviving TABLE::histograms_are_read and adding appropriate checks whenever it is needed. Part of MDEV-19061 - table_share used for reading statistical tables is not protected --- sql/sql_statistics.cc | 118 +++++++++----------------------------------------- sql/sql_statistics.h | 1 - sql/table.cc | 2 - sql/table.h | 16 ++++++- 4 files changed, 34 insertions(+), 103 deletions(-) (limited to 'sql') diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index a99dcca89e4..ae02254c745 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -2280,78 +2280,6 @@ static int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *table_share) } -/** - @brief - Allocate memory for the histogram used by a table share - - @param - thd Thread handler - @param - table_share Table share for which the memory for histogram data is allocated - @param - is_safe TRUE <-> at any time only one thread can perform the function - - @note - The function allocates the memory for the histogram built for a table in the - table's share memory with the intention to read the data there from the - system persistent statistical table mysql.column_stats, - The memory is allocated in the table_share's mem_root. - If the parameter is_safe is TRUE then it is guaranteed that at any given time - only one thread is executed the code of the function. - - @retval - 0 If the memory for all statistical data has been successfully allocated - @retval - 1 Otherwise - - @note - Currently the function always is called with the parameter is_safe set - to FALSE. -*/ - -static -int alloc_histograms_for_table_share(THD* thd, TABLE_SHARE *table_share, - bool is_safe) -{ - TABLE_STATISTICS_CB *stats_cb= &table_share->stats_cb; - - DBUG_ENTER("alloc_histograms_for_table_share"); - - if (!is_safe) - mysql_mutex_lock(&table_share->LOCK_share); - - if (stats_cb->histograms_can_be_read) - { - if (!is_safe) - mysql_mutex_unlock(&table_share->LOCK_share); - DBUG_RETURN(0); - } - - Table_statistics *table_stats= stats_cb->table_stats; - ulong total_hist_size= table_stats->total_hist_size; - - if (total_hist_size && !table_stats->histograms) - { - uchar *histograms= (uchar *) alloc_root(&stats_cb->mem_root, - total_hist_size); - if (!histograms) - { - if (!is_safe) - mysql_mutex_unlock(&table_share->LOCK_share); - DBUG_RETURN(1); - } - memset(histograms, 0, total_hist_size); - table_stats->histograms= histograms; - stats_cb->histograms_can_be_read= TRUE; - } - - if (!is_safe) - mysql_mutex_unlock(&table_share->LOCK_share); - - DBUG_RETURN(0); - -} - /** @brief Initialize the aggregation fields to collect statistics on a column @@ -2925,7 +2853,7 @@ int read_statistics_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables) column_stat.get_stat_values(); total_hist_size+= table_field->read_stats->histogram.get_size(); } - read_stats->total_hist_size= total_hist_size; + table_share->stats_cb.total_hist_size= total_hist_size; /* Read statistics from the statistical table index_stats */ stat_table= stat_tables[INDEX_STAT].table; @@ -3059,26 +2987,25 @@ void delete_stat_values_for_table_share(TABLE_SHARE *table_share) static int read_histograms_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables) { - TABLE_SHARE *table_share= table->s; - + TABLE_STATISTICS_CB *stats_cb= &table->s->stats_cb; DBUG_ENTER("read_histograms_for_table"); - if (!table_share->stats_cb.histograms_can_be_read) + if (stats_cb->start_histograms_load()) { - (void) alloc_histograms_for_table_share(thd, table_share, FALSE); - } - if (table_share->stats_cb.histograms_can_be_read && - !table_share->stats_cb.histograms_are_read) - { - Field **field_ptr; - uchar *histogram= table_share->stats_cb.table_stats->histograms; - TABLE *stat_table= stat_tables[COLUMN_STAT].table; - Column_stat column_stat(stat_table, table); - for (field_ptr= table_share->field; *field_ptr; field_ptr++) + uchar *histogram= (uchar *) alloc_root(&stats_cb->mem_root, + stats_cb->total_hist_size); + if (!histogram) + { + stats_cb->abort_histograms_load(); + DBUG_RETURN(1); + } + memset(histogram, 0, stats_cb->total_hist_size); + + Column_stat column_stat(stat_tables[COLUMN_STAT].table, table); + for (Field **field_ptr= table->s->field; *field_ptr; field_ptr++) { Field *table_field= *field_ptr; - uint hist_size= table_field->read_stats->histogram.get_size(); - if (hist_size) + if (uint hist_size= table_field->read_stats->histogram.get_size()) { column_stat.set_key_fields(table_field); table_field->read_stats->histogram.set_values(histogram); @@ -3086,8 +3013,9 @@ int read_histograms_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables) histogram+= hist_size; } } + stats_cb->end_histograms_load(); } - + table->histograms_are_read= true; DBUG_RETURN(0); } @@ -3178,8 +3106,8 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) if (!tl->table->stats_is_read) dump_stats_from_share_to_table(tl->table); tl->table->histograms_are_read= - table_share->stats_cb.histograms_are_read; - if (table_share->stats_cb.histograms_are_read || + table_share->stats_cb.histograms_are_ready(); + if (table_share->stats_cb.histograms_are_ready() || thd->variables.optimizer_use_condition_selectivity <= 3) continue; } @@ -3217,14 +3145,8 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) else continue; } - if (thd->variables.optimizer_use_condition_selectivity > 3 && - !table_share->stats_cb.histograms_are_read) - { + if (thd->variables.optimizer_use_condition_selectivity > 3) (void) read_histograms_for_table(thd, tl->table, stat_tables); - table_share->stats_cb.histograms_are_read= TRUE; - } - if (table_share->stats_cb.histograms_are_read) - tl->table->histograms_are_read= TRUE; } } diff --git a/sql/sql_statistics.h b/sql/sql_statistics.h index 71d727eab07..151618ca365 100644 --- a/sql/sql_statistics.h +++ b/sql/sql_statistics.h @@ -280,7 +280,6 @@ public: Column_statistics *column_stats; /* Array of statistical data for columns */ Index_statistics *index_stats; /* Array of statistical data for indexes */ ulong *idx_avg_frequency; /* Array of records per key for index prefixes */ - ulong total_hist_size; /* Total size of all histograms */ uchar *histograms; /* Sequence of histograms */ }; diff --git a/sql/table.cc b/sql/table.cc index 1d3bebcc5d5..20fc86e35f0 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -416,8 +416,6 @@ void TABLE_SHARE::destroy() delete_stat_values_for_table_share(this); free_root(&stats_cb.mem_root, MYF(0)); - stats_cb.histograms_can_be_read= FALSE; - stats_cb.histograms_are_read= FALSE; /* The mutexes are initialized only for shares that are part of the TDC */ if (tmp_table == NO_TMP_TABLE) diff --git a/sql/table.h b/sql/table.h index 85ba7b29ce2..93795113fab 100644 --- a/sql/table.h +++ b/sql/table.h @@ -659,13 +659,25 @@ class TABLE_STATISTICS_CB }; class Statistics_state stats_state; + class Statistics_state hist_state; public: MEM_ROOT mem_root; /* MEM_ROOT to allocate statistical data for the table */ Table_statistics *table_stats; /* Structure to access the statistical data */ - bool histograms_can_be_read; - bool histograms_are_read; + ulong total_hist_size; /* Total size of all histograms */ + bool histograms_are_ready() const + { + return !total_hist_size || hist_state.is_ready(); + } + + bool start_histograms_load() + { + return total_hist_size && hist_state.start_load(); + } + + void end_histograms_load() { hist_state.end_load(); } + void abort_histograms_load() { hist_state.abort_load(); } bool stats_are_ready() const { return stats_state.is_ready(); } bool start_stats_load() { return stats_state.start_load(); } void end_stats_load() { stats_state.end_load(); } -- cgit v1.2.1