From 0e91e0c377f38c31cd5ad766b9a6faceacb7ce18 Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Wed, 13 Mar 2019 14:41:52 +0400 Subject: A proper State_tracker::m_changed enacpsulation Note: preserved original behaviour, where remaining trackers are not reset on error from store(). This effectively means subsequent statements will start tracking from unclean state. Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 91 +++++++------------------------------------------- 1 file changed, 12 insertions(+), 79 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index b59475645fa..c049e1bab33 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -209,7 +209,6 @@ public: return result; } - void reset(); bool enable(THD *thd); bool check_str(THD *thd, LEX_STRING *val); bool update(THD *thd, set_var *var); @@ -241,7 +240,6 @@ class Current_schema_tracker : public State_tracker { private: bool schema_track_inited; - void reset(); public: @@ -272,17 +270,11 @@ public: class Session_state_change_tracker : public State_tracker { -private: - - void reset(); - public: - Session_state_change_tracker(); bool enable(THD *thd) { return update(thd, NULL); }; bool update(THD *thd, set_var *var); bool store(THD *thd, String *buf); - bool is_state_changed(THD*); }; @@ -829,7 +821,7 @@ bool Session_sysvars_tracker::store(THD *thd, String *buf) if (orig_list->store(thd, buf)) return true; - reset(); + orig_list->reset(); return false; } @@ -892,17 +884,6 @@ void Session_sysvars_tracker::vars_list::reset() my_hash_iterate(&m_registered_sysvars, &reset_variable, NULL); } -/** - Prepare/reset the m_registered_sysvars hash for next statement. -*/ - -void Session_sysvars_tracker::reset() -{ - - orig_list->reset(); - m_changed= false; -} - static Session_sysvars_tracker* sysvar_tracker(THD *thd) { return (Session_sysvars_tracker*) @@ -991,24 +972,10 @@ bool Current_schema_tracker::store(THD *thd, String *buf) /* Length and current schema name */ buf->q_net_store_data((const uchar *)thd->db.str, thd->db.length); - reset(); - return false; } -/** - Reset the m_changed flag for next statement. - - @return void -*/ - -void Current_schema_tracker::reset() -{ - m_changed= false; -} - - /////////////////////////////////////////////////////////////////////////////// @@ -1331,24 +1298,13 @@ bool Transaction_state_tracker::store(THD *thd, String *buf) } } - reset(); + tx_reported_state= tx_curr_state; + tx_changed= TX_CHG_NONE; return false; } -/** - Reset the m_changed flag for next statement. -*/ - -void Transaction_state_tracker::reset() -{ - m_changed= false; - tx_reported_state= tx_curr_state; - tx_changed= TX_CHG_NONE; -} - - /** Helper function: turn table info into table access flag. Accepts table lock type and engine type flag (transactional/ @@ -1518,11 +1474,6 @@ void Transaction_state_tracker::set_isol_level(THD *thd, /////////////////////////////////////////////////////////////////////////////// -Session_state_change_tracker::Session_state_change_tracker() -{ - m_changed= false; -} - /** @Enable/disable the tracker based on @@session_track_state_change value. @@ -1560,34 +1511,12 @@ bool Session_state_change_tracker::store(THD *thd, String *buf) /* Length of the overall entity (1 byte) */ buf->q_append('\1'); - DBUG_ASSERT(is_state_changed(thd)); + DBUG_ASSERT(is_changed()); buf->q_append('1'); - reset(); - return false; } - -/** - Reset the m_changed flag for next statement. -*/ - -void Session_state_change_tracker::reset() -{ - m_changed= false; -} - - -/** - Find if there is a session state change. -*/ - -bool Session_state_change_tracker::is_state_changed(THD *) -{ - return m_changed; -} - /////////////////////////////////////////////////////////////////////////////// /** @@ -1682,11 +1611,15 @@ void Session_tracker::store(THD *thd, String *buf) /* Get total length. */ for (int i= 0; i < SESSION_TRACKER_END; i++) { - if (m_trackers[i]->is_changed() && - m_trackers[i]->store(thd, buf)) + if (m_trackers[i]->is_changed()) { - buf->length(start); // it is safer to have 0-length block in case of error - return; + if (m_trackers[i]->store(thd, buf)) + { + // it is safer to have 0-length block in case of error + buf->length(start); + return; + } + m_trackers[i]->reset_changed(); } } -- cgit v1.2.1 From 8f594b3384f5c68f530730a037a7e74fa215b67d Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Mon, 18 Mar 2019 14:43:14 +0400 Subject: Session_sysvars_tracker::vars_list cleanups - return proper type - removed useless node argument - removed useless mem_flag - classes are "private" by default - simplified iterators Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 142 +++++++++++++++++-------------------------------- 1 file changed, 50 insertions(+), 92 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index c049e1bab33..04a8b783837 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -70,7 +70,6 @@ private: class vars_list { - private: /** Registered system variables. (@@session_track_system_variables) A hash to store the name of all the system variables specified by the @@ -101,12 +100,20 @@ private: } } - uchar* search(const sys_var *svar) + sysvar_node_st *search(const sys_var *svar) { - return (my_hash_search(&m_registered_sysvars, (const uchar *)&svar, - sizeof(sys_var *))); + return reinterpret_cast( + my_hash_search(&m_registered_sysvars, + reinterpret_cast(&svar), + sizeof(sys_var*))); } + sysvar_node_st *at(ulong i) + { + DBUG_ASSERT(i < m_registered_sysvars.records); + return reinterpret_cast( + my_hash_element(&m_registered_sysvars, i)); + } public: vars_list() : buffer_length(0) @@ -129,22 +136,21 @@ private: } } - uchar* insert_or_search(sysvar_node_st *node, const sys_var *svar) + sysvar_node_st *insert_or_search(const sys_var *svar) { - uchar *res; - res= search(svar); + sysvar_node_st *res= search(svar); if (!res) { if (track_all) { - insert(node, svar, m_mem_flag); + insert(svar); return search(svar); } } return res; } - bool insert(sysvar_node_st *node, const sys_var *svar, myf mem_flag); + bool insert(const sys_var *svar); void reinit(); void reset(); inline bool is_enabled() @@ -218,11 +224,6 @@ public: static uchar *sysvars_get_key(const char *entry, size_t *length, my_bool not_used __attribute__((unused))); - // hash iterators - static my_bool name_array_filler(void *ptr, void *data_ptr); - static my_bool store_variable(void *ptr, void *data_ptr); - static my_bool reset_variable(void *ptr, void *data_ptr); - static bool check_var_list(THD *thd, LEX_STRING var_list, bool throw_error, CHARSET_INFO *char_set, bool take_mutex); }; @@ -313,25 +314,20 @@ void Session_sysvars_tracker::vars_list::copy(vars_list* from, THD *thd) /** Inserts the variable to be tracked into m_registered_sysvars hash. - @param node Node to be inserted. @param svar address of the system variable @retval false success @retval true error */ -bool Session_sysvars_tracker::vars_list::insert(sysvar_node_st *node, - const sys_var *svar, - myf mem_flag) +bool Session_sysvars_tracker::vars_list::insert(const sys_var *svar) { - if (!node) + sysvar_node_st *node; + if (!(node= (sysvar_node_st *) my_malloc(sizeof(sysvar_node_st), + MYF(MY_WME | m_mem_flag)))) { - if (!(node= (sysvar_node_st *) my_malloc(sizeof(sysvar_node_st), - MYF(MY_WME | mem_flag)))) - { - reinit(); - return true; - } + reinit(); + return true; } node->m_svar= (sys_var *)svar; @@ -433,7 +429,7 @@ bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, else if ((svar= find_sys_var_ex(thd, var.str, var.length, throw_error, true))) { - if (insert(NULL, svar, m_mem_flag) == TRUE) + if (insert(svar) == TRUE) goto error; } else if (throw_error && thd) @@ -536,24 +532,6 @@ bool Session_sysvars_tracker::check_var_list(THD *thd, return false; } -struct name_array_filler_data -{ - LEX_CSTRING **names; - uint idx; - -}; - -/** Collects variable references into array */ -my_bool Session_sysvars_tracker::name_array_filler(void *ptr, - void *data_ptr) -{ - Session_sysvars_tracker::sysvar_node_st *node= - (Session_sysvars_tracker::sysvar_node_st *)ptr; - name_array_filler_data *data= (struct name_array_filler_data *)data_ptr; - if (*node->test_load) - data->names[data->idx++]= &node->m_svar->name; - return FALSE; -} /* Sorts variable references array */ static int name_array_sorter(const void *a, const void *b) @@ -573,7 +551,8 @@ static int name_array_sorter(const void *a, const void *b) bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, size_t buf_len) { - struct name_array_filler_data data; + LEX_CSTRING **names; + uint idx; size_t left= buf_len; size_t names_size= m_registered_sysvars.records * sizeof(LEX_CSTRING *); const char separator= ','; @@ -596,16 +575,19 @@ bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, return false; } - data.names= (LEX_CSTRING**)my_safe_alloca(names_size); - - if (unlikely(!data.names)) + if (unlikely(!(names= (LEX_CSTRING**) my_safe_alloca(names_size)))) return true; - data.idx= 0; + idx= 0; mysql_mutex_lock(&LOCK_plugin); - my_hash_iterate(&m_registered_sysvars, &name_array_filler, &data); - DBUG_ASSERT(data.idx <= m_registered_sysvars.records); + for (ulong i= 0; i < m_registered_sysvars.records; i++) + { + sysvar_node_st *node= at(i); + if (*node->test_load) + names[idx++]= &node->m_svar->name; + } + DBUG_ASSERT(idx <= m_registered_sysvars.records); /* We check number of records again here because number of variables @@ -618,17 +600,16 @@ bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, return false; } - my_qsort(data.names, data.idx, sizeof(LEX_CSTRING *), - &name_array_sorter); + my_qsort(names, idx, sizeof(LEX_CSTRING*), &name_array_sorter); - for(uint i= 0; i < data.idx; i++) + for(uint i= 0; i < idx; i++) { - LEX_CSTRING *nm= data.names[i]; + LEX_CSTRING *nm= names[i]; size_t ln= nm->length + 1; if (ln > left) { mysql_mutex_unlock(&LOCK_plugin); - my_safe_afree(data.names, names_size); + my_safe_afree(names, names_size); return true; } memcpy(buf, nm->str, nm->length); @@ -639,7 +620,7 @@ bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, mysql_mutex_unlock(&LOCK_plugin); buf--; buf[0]= '\0'; - my_safe_afree(data.names, names_size); + my_safe_afree(names, names_size); return false; } @@ -724,24 +705,15 @@ bool Session_sysvars_tracker::update(THD *thd, set_var *var) } -/* - Function and structure to support storing variables from hash to the buffer. -*/ - -struct st_store_variable_param -{ - THD *thd; - String *buf; -}; - -my_bool Session_sysvars_tracker::store_variable(void *ptr, void *data_ptr) +bool Session_sysvars_tracker::vars_list::store(THD *thd, String *buf) { - Session_sysvars_tracker::sysvar_node_st *node= - (Session_sysvars_tracker::sysvar_node_st *)ptr; - if (node->m_changed) + for (ulong i= 0; i < m_registered_sysvars.records; i++) { - THD *thd= ((st_store_variable_param *)data_ptr)->thd; - String *buf= ((st_store_variable_param *)data_ptr)->buf; + sysvar_node_st *node= at(i); + + if (!node->m_changed) + continue; + char val_buf[SHOW_VAR_FUNC_BUFF_SIZE]; SHOW_VAR show; CHARSET_INFO *charset; @@ -750,7 +722,7 @@ my_bool Session_sysvars_tracker::store_variable(void *ptr, void *data_ptr) if (!*node->test_load) { mysql_mutex_unlock(&LOCK_plugin); - return false; + continue; } sys_var *svar= node->m_svar; bool is_plugin= svar->cast_pluginvar(); @@ -795,11 +767,6 @@ my_bool Session_sysvars_tracker::store_variable(void *ptr, void *data_ptr) return false; } -bool Session_sysvars_tracker::vars_list::store(THD *thd, String *buf) -{ - st_store_variable_param data= {thd, buf}; - return my_hash_iterate(&m_registered_sysvars, &store_variable, &data); -} /** Store the data for changed system variables in the specified buffer. @@ -836,14 +803,13 @@ bool Session_sysvars_tracker::store(THD *thd, String *buf) void Session_sysvars_tracker::mark_as_changed(THD *thd, LEX_CSTRING *var) { - sysvar_node_st *node= NULL; + sysvar_node_st *node; sys_var *svar= (sys_var *)var; /* Check if the specified system variable is being tracked, if so mark it as changed and also set the class's m_changed flag. */ - if (orig_list->is_enabled() && - (node= (sysvar_node_st *) (orig_list->insert_or_search(node, svar)))) + if (orig_list->is_enabled() && (node= orig_list->insert_or_search(svar))) { node->m_changed= true; State_tracker::mark_as_changed(thd, var); @@ -870,18 +836,10 @@ uchar *Session_sysvars_tracker::sysvars_get_key(const char *entry, } -/* Function to support resetting hash nodes for the variables */ - -my_bool Session_sysvars_tracker::reset_variable(void *ptr, - void *data_ptr) -{ - ((Session_sysvars_tracker::sysvar_node_st *)ptr)->m_changed= false; - return false; -} - void Session_sysvars_tracker::vars_list::reset() { - my_hash_iterate(&m_registered_sysvars, &reset_variable, NULL); + for (ulong i= 0; i < m_registered_sysvars.records; i++) + at(i)->m_changed= false; } static Session_sysvars_tracker* sysvar_tracker(THD *thd) -- cgit v1.2.1 From 19d5ddccfde04c6b336bb4974407ecde4fb6fbc6 Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Wed, 13 Mar 2019 14:16:49 +0400 Subject: Cleanup session tracker redundancy - m_enabled is initialised by the base class constructor - removed unused schema_track_inited - moved Transaction_state_tracker constructor to declaration - common enable() - removed unused Session_sysvars_tracker::check_str() - classes are "private" by default - don't even try to compile for embedded Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 51 -------------------------------------------------- 1 file changed, 51 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 04a8b783837..14fc43e09e7 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -15,7 +15,6 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ -#ifndef EMBEDDED_LIBRARY #include "sql_plugin.h" #include "session_tracker.h" @@ -60,8 +59,6 @@ public: class Session_sysvars_tracker : public State_tracker { -private: - struct sysvar_node_st { sys_var *m_svar; bool *test_load; @@ -216,7 +213,6 @@ public: } bool enable(THD *thd); - bool check_str(THD *thd, LEX_STRING *val); bool update(THD *thd, set_var *var); bool store(THD *thd, String *buf); void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); @@ -239,18 +235,7 @@ public: class Current_schema_tracker : public State_tracker { -private: - bool schema_track_inited; - public: - - Current_schema_tracker() - { - schema_track_inited= false; - } - - bool enable(THD *thd) - { return update(thd, NULL); } bool update(THD *thd, set_var *var); bool store(THD *thd, String *buf); }; @@ -272,8 +257,6 @@ public: class Session_state_change_tracker : public State_tracker { public: - bool enable(THD *thd) - { return update(thd, NULL); }; bool update(THD *thd, set_var *var); bool store(THD *thd, String *buf); }; @@ -654,27 +637,6 @@ bool Session_sysvars_tracker::enable(THD *thd) } -/** - Check system variable name(s). - - @note This function is called from the ON_CHECK() function of the - session_track_system_variables' sys_var class. - - @param thd [IN] The thd handle. - @param var [IN] A pointer to set_var holding the specified list of - system variable names. - - @retval true Error - @retval false Success -*/ - -inline bool Session_sysvars_tracker::check_str(THD *thd, LEX_STRING *val) -{ - return Session_sysvars_tracker::check_var_list(thd, *val, true, - thd->charset(), true); -} - - /** Once the value of the @@session_track_system_variables has been successfully updated, this function calls @@ -936,17 +898,6 @@ bool Current_schema_tracker::store(THD *thd, String *buf) /////////////////////////////////////////////////////////////////////////////// - -Transaction_state_tracker::Transaction_state_tracker() -{ - m_enabled = false; - tx_changed = TX_CHG_NONE; - tx_curr_state = - tx_reported_state= TX_EMPTY; - tx_read_flags = TX_READ_INHERIT; - tx_isol_level = TX_ISOL_INHERIT; -} - /** Enable/disable the tracker based on @@session_track_transaction_info. @@ -1597,5 +1548,3 @@ void Session_tracker::store(THD *thd, String *buf) net_store_length(data - 1, length); } - -#endif //EMBEDDED_LIBRARY -- cgit v1.2.1 From 2be28a91b15010c5e6146e78e78fbe10a9b86153 Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Fri, 15 Mar 2019 11:52:26 +0400 Subject: Cleanup session tracker API - Session_sysvars_tracker::server_init_check() -> sysvartrack_validate_value() - Session_sysvars_tracker::check_var_list() -> sysvartrack_validate_value() - Session_sysvars_tracker::server_init_process() -> sysvartrack_global_update() - sysvartrack_reprint_value() -> sysvartrack_global_update() - sysvartrack_value_len() -> sysvartrack_session_value_ptr() - sysvartrack_value_construct() -> sysvartrack_session_value_ptr() - sysvartrack_update() -> Session_sysvars_tracker::update() - Session_tracker::server_boot_verify() -> session_tracker_init() - sysvar_tracker() -> /dev/null Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 136 +++++++++++++++++-------------------------------- 1 file changed, 46 insertions(+), 90 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 14fc43e09e7..7908c083446 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -190,28 +190,6 @@ public: return orig_list->construct_var_list(buf, buf_len); } - /** - Method used to check the validity of string provided - for session_track_system_variables during the server - startup. - */ - static bool server_init_check(THD *thd, CHARSET_INFO *char_set, - LEX_STRING var_list) - { - return check_var_list(thd, var_list, false, char_set, false); - } - - static bool server_init_process(THD *thd, CHARSET_INFO *char_set, - LEX_STRING var_list) - { - vars_list dummy; - bool result; - result= dummy.parse_var_list(thd, var_list, false, char_set, false); - if (!result) - dummy.construct_var_list(var_list.str, var_list.length + 1); - return result; - } - bool enable(THD *thd); bool update(THD *thd, set_var *var); bool store(THD *thd, String *buf); @@ -220,8 +198,7 @@ public: static uchar *sysvars_get_key(const char *entry, size_t *length, my_bool not_used __attribute__((unused))); - static bool check_var_list(THD *thd, LEX_STRING var_list, bool throw_error, - CHARSET_INFO *char_set, bool take_mutex); + friend bool sysvartrack_global_update(THD *thd, char *str, size_t len); }; @@ -442,12 +419,9 @@ error: } -bool Session_sysvars_tracker::check_var_list(THD *thd, - LEX_STRING var_list, - bool throw_error, - CHARSET_INFO *char_set, - bool take_mutex) +bool sysvartrack_validate_value(THD *thd, const char *str, size_t len) { + LEX_STRING var_list= { (char *) str, len }; const char separator= ','; char *token, *lasts= NULL; size_t rest= var_list.length; @@ -466,7 +440,7 @@ bool Session_sysvars_tracker::check_var_list(THD *thd, token value. Hence the mutex is handled here to avoid a performance overhead. */ - if (!thd || take_mutex) + if (!thd) mysql_mutex_lock(&LOCK_plugin); for (;;) { @@ -484,24 +458,14 @@ bool Session_sysvars_tracker::check_var_list(THD *thd, var.length= rest; /* Remove leading/trailing whitespace. */ - trim_whitespace(char_set, &var); + trim_whitespace(system_charset_info, &var); if(!strcmp(var.str, "*") && - !find_sys_var_ex(thd, var.str, var.length, throw_error, true)) + !find_sys_var_ex(thd, var.str, var.length, false, true)) { - if (throw_error && take_mutex && thd) - { - push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, - ER_WRONG_VALUE_FOR_VAR, - "%.*s is not a valid system variable and will" - "be ignored.", (int)var.length, token); - } - else - { - if (!thd || take_mutex) - mysql_mutex_unlock(&LOCK_plugin); - return true; - } + if (!thd) + mysql_mutex_unlock(&LOCK_plugin); + return true; } if (lasts) @@ -509,7 +473,7 @@ bool Session_sysvars_tracker::check_var_list(THD *thd, else break; } - if (!thd || take_mutex) + if (!thd) mysql_mutex_unlock(&LOCK_plugin); return false; @@ -804,38 +768,50 @@ void Session_sysvars_tracker::vars_list::reset() at(i)->m_changed= false; } -static Session_sysvars_tracker* sysvar_tracker(THD *thd) -{ - return (Session_sysvars_tracker*) - thd->session_tracker.get_tracker(SESSION_SYSVARS_TRACKER); -} -bool sysvartrack_validate_value(THD *thd, const char *str, size_t len) -{ - LEX_STRING tmp= {(char *)str, len}; - return Session_sysvars_tracker::server_init_check(thd, system_charset_info, - tmp); -} -bool sysvartrack_reprint_value(THD *thd, char *str, size_t len) +bool sysvartrack_global_update(THD *thd, char *str, size_t len) { - LEX_STRING tmp= {str, len}; - return Session_sysvars_tracker::server_init_process(thd, - system_charset_info, - tmp); -} -bool sysvartrack_update(THD *thd, set_var *var) -{ - return sysvar_tracker(thd)->update(thd, var); + LEX_STRING tmp= { str, len }; + Session_sysvars_tracker::vars_list dummy; + if (!dummy.parse_var_list(thd, tmp, false, system_charset_info, false)) + { + dummy.construct_var_list(str, len + 1); + return false; + } + return true; } -size_t sysvartrack_value_len(THD *thd) + + +uchar *sysvartrack_session_value_ptr(THD *thd, const LEX_CSTRING *base) { - return sysvar_tracker(thd)->get_buffer_length(); + Session_sysvars_tracker *tracker= static_cast + (thd->session_tracker.get_tracker(SESSION_SYSVARS_TRACKER)); + size_t len= tracker->get_buffer_length(); + char *res= (char*) thd->alloc(len + sizeof(char*)); + if (res) + { + char *buf= res + sizeof(char*); + *((char**) res)= buf; + tracker->construct_var_list(buf, len); + } + return (uchar*) res; } -bool sysvartrack_value_construct(THD *thd, char *val, size_t len) + + +int session_tracker_init() { - return sysvar_tracker(thd)->construct_var_list(val, len); + if (sysvartrack_validate_value(0, + global_system_variables.session_track_system_variables, + safe_strlen(global_system_variables.session_track_system_variables))) + { + sql_print_error("The variable session_track_system_variables has " + "invalid values."); + return 1; + } + return 0; } + /////////////////////////////////////////////////////////////////////////////// /** @@ -1477,26 +1453,6 @@ void Session_tracker::enable(THD *thd) } -/** - Method called during the server startup to verify the contents - of @@session_track_system_variables. - - @retval false Success - @retval true Failure -*/ - -bool Session_tracker::server_boot_verify(CHARSET_INFO *char_set) -{ - bool result; - LEX_STRING tmp; - tmp.str= global_system_variables.session_track_system_variables; - tmp.length= safe_strlen(tmp.str); - result= - Session_sysvars_tracker::server_init_check(NULL, char_set, tmp); - return result; -} - - /** @brief Store all change information in the specified buffer. -- cgit v1.2.1 From 55bdd7f7b4a0898dca2a2e7d457b06cb81df7d6a Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Wed, 13 Mar 2019 12:20:11 +0400 Subject: Get rid of not implemented SESSION_GTIDS_TRACKER One less new/delete per connection. Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 16 ---------------- 1 file changed, 16 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 7908c083446..13da65b417e 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -34,20 +34,6 @@ void State_tracker::mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name) } -class Not_implemented_tracker : public State_tracker -{ -public: - bool enable(THD *thd) - { return false; } - bool update(THD *, set_var *) - { return false; } - bool store(THD *, String *) - { return false; } - void mark_as_changed(THD *, LEX_CSTRING *tracked_item_name) - {} - -}; - /** Session_sysvars_tracker @@ -1443,8 +1429,6 @@ void Session_tracker::enable(THD *thd) new (std::nothrow) Current_schema_tracker; m_trackers[SESSION_STATE_CHANGE_TRACKER]= new (std::nothrow) Session_state_change_tracker; - m_trackers[SESSION_GTIDS_TRACKER]= - new (std::nothrow) Not_implemented_tracker; m_trackers[TRANSACTION_INFO_TRACKER]= new (std::nothrow) Transaction_state_tracker; -- cgit v1.2.1 From 01e8f3c52b349a0374c04f0765a625b5ac03a652 Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Wed, 13 Mar 2019 13:41:18 +0400 Subject: Allocate orig_list statically tool_list is a temporary list needed only for SET SESSION session_track_system_variables, so allocate it on stack instead. Sane reinit() calls. Saves 2 new/delete per connection. Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 57 +++++++++++++++----------------------------------- 1 file changed, 17 insertions(+), 40 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 13da65b417e..1af1c8de05a 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -98,8 +98,7 @@ class Session_sysvars_tracker : public State_tracker my_hash_element(&m_registered_sysvars, i)); } public: - vars_list() : - buffer_length(0) + vars_list(): buffer_length(0), track_all(false) { m_mem_flag= current_thd ? MY_THREAD_SPECIFIC : 0; init(); @@ -150,30 +149,16 @@ class Session_sysvars_tracker : public State_tracker Two objects of vars_list type are maintained to manage various operations. */ - vars_list *orig_list, *tool_list; + vars_list orig_list; public: - Session_sysvars_tracker() - { - orig_list= new (std::nothrow) vars_list(); - tool_list= new (std::nothrow) vars_list(); - } - - ~Session_sysvars_tracker() - { - if (orig_list) - delete orig_list; - if (tool_list) - delete tool_list; - } - size_t get_buffer_length() { - return orig_list->get_buffer_length(); + return orig_list.get_buffer_length(); } bool construct_var_list(char *buf, size_t buf_len) { - return orig_list->construct_var_list(buf, buf_len); + return orig_list.construct_var_list(buf, buf_len); } bool enable(THD *thd); @@ -249,7 +234,6 @@ void Session_sysvars_tracker::vars_list::reinit() void Session_sysvars_tracker::vars_list::copy(vars_list* from, THD *thd) { - reinit(); track_all= from->track_all; free_hash(); buffer_length= from->buffer_length; @@ -271,10 +255,7 @@ bool Session_sysvars_tracker::vars_list::insert(const sys_var *svar) sysvar_node_st *node; if (!(node= (sysvar_node_st *) my_malloc(sizeof(sysvar_node_st), MYF(MY_WME | m_mem_flag)))) - { - reinit(); return true; - } node->m_svar= (sys_var *)svar; node->test_load= node->m_svar->test_load; @@ -285,7 +266,6 @@ bool Session_sysvars_tracker::vars_list::insert(const sys_var *svar) if (!search((sys_var *)svar)) { //EOF (error is already reported) - reinit(); return true; } } @@ -322,7 +302,6 @@ bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, const char separator= ','; char *token, *lasts= NULL; size_t rest= var_list.length; - reinit(); if (!var_list.str || var_list.length == 0) { @@ -573,14 +552,13 @@ bool Session_sysvars_tracker::enable(THD *thd) LEX_STRING tmp; tmp.str= global_system_variables.session_track_system_variables; tmp.length= safe_strlen(tmp.str); - if (tool_list->parse_var_list(thd, tmp, - true, thd->charset(), false) == true) + if (orig_list.parse_var_list(thd, tmp, true, thd->charset(), false) == true) { mysql_mutex_unlock(&LOCK_plugin); + orig_list.reinit(); return true; } mysql_mutex_unlock(&LOCK_plugin); - orig_list->copy(tool_list, thd); m_enabled= true; return false; @@ -593,6 +571,9 @@ bool Session_sysvars_tracker::enable(THD *thd) Session_sysvars_tracker::vars_list::copy updating the hash in orig_list which represents the system variables to be tracked. + We are doing via tool list because there possible errors with memory + in this case value will be unchanged. + @note This function is called from the ON_UPDATE() function of the session_track_system_variables' sys_var class. @@ -604,15 +585,11 @@ bool Session_sysvars_tracker::enable(THD *thd) bool Session_sysvars_tracker::update(THD *thd, set_var *var) { - /* - We are doing via tool list because there possible errors with memory - in this case value will be unchanged. - */ - tool_list->reinit(); - if (tool_list->parse_var_list(thd, var->save_result.string_value, true, - thd->charset(), true)) + vars_list tool_list; + if (tool_list.parse_var_list(thd, var->save_result.string_value, true, + thd->charset(), true)) return true; - orig_list->copy(tool_list, thd); + orig_list.copy(&tool_list, thd); return false; } @@ -694,13 +671,13 @@ bool Session_sysvars_tracker::vars_list::store(THD *thd, String *buf) bool Session_sysvars_tracker::store(THD *thd, String *buf) { - if (!orig_list->is_enabled()) + if (!orig_list.is_enabled()) return false; - if (orig_list->store(thd, buf)) + if (orig_list.store(thd, buf)) return true; - orig_list->reset(); + orig_list.reset(); return false; } @@ -721,7 +698,7 @@ void Session_sysvars_tracker::mark_as_changed(THD *thd, Check if the specified system variable is being tracked, if so mark it as changed and also set the class's m_changed flag. */ - if (orig_list->is_enabled() && (node= orig_list->insert_or_search(svar))) + if (orig_list.is_enabled() && (node= orig_list.insert_or_search(svar))) { node->m_changed= true; State_tracker::mark_as_changed(thd, var); -- cgit v1.2.1 From 47bd06d55ec211bc4d05d616a0833629504c7edf Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Mon, 18 Mar 2019 17:13:00 +0400 Subject: Static current schema and state change trackers Saves 2 new/delete per connection. Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 47 ++++------------------------------------------- 1 file changed, 4 insertions(+), 43 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 1af1c8de05a..868e8295294 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -173,43 +173,6 @@ public: }; - -/** - Current_schema_tracker, - - This is a tracker class that enables & manages the tracking of current - schema for a particular connection. -*/ - -class Current_schema_tracker : public State_tracker -{ -public: - bool update(THD *thd, set_var *var); - bool store(THD *thd, String *buf); -}; - -/* - Session_state_change_tracker - - This is a boolean tracker class that will monitor any change that contributes - to a session state change. - Attributes that contribute to session state change include: - - Successful change to System variables - - User defined variables assignments - - temporary tables created, altered or deleted - - prepared statements added or removed - - change in current database - - change of current role -*/ - -class Session_state_change_tracker : public State_tracker -{ -public: - bool update(THD *thd, set_var *var); - bool store(THD *thd, String *buf); -}; - - /* To be used in expanding the buffer. */ static const unsigned int EXTRA_ALLOC= 1024; @@ -1379,8 +1342,10 @@ Session_tracker::Session_tracker() compile_time_assert((uint)SESSION_TRACK_always_at_the_end >= (uint)SESSION_TRACKER_END); - for (int i= 0; i < SESSION_TRACKER_END; i++) - m_trackers[i]= NULL; + m_trackers[SESSION_SYSVARS_TRACKER]= 0; + m_trackers[CURRENT_SCHEMA_TRACKER]= ¤t_schema; + m_trackers[SESSION_STATE_CHANGE_TRACKER]= &state_change; + m_trackers[TRANSACTION_INFO_TRACKER]= 0; } @@ -1402,10 +1367,6 @@ void Session_tracker::enable(THD *thd) deinit(); m_trackers[SESSION_SYSVARS_TRACKER]= new (std::nothrow) Session_sysvars_tracker(); - m_trackers[CURRENT_SCHEMA_TRACKER]= - new (std::nothrow) Current_schema_tracker; - m_trackers[SESSION_STATE_CHANGE_TRACKER]= - new (std::nothrow) Session_state_change_tracker; m_trackers[TRANSACTION_INFO_TRACKER]= new (std::nothrow) Transaction_state_tracker; -- cgit v1.2.1 From a7adc2ce1680f00635b8241202066fd5542d286f Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Mon, 18 Mar 2019 19:18:54 +0400 Subject: Allocate Transaction_state_tracker statically One less new/delete per connection. Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 868e8295294..2f72b7198f9 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -1345,7 +1345,7 @@ Session_tracker::Session_tracker() m_trackers[SESSION_SYSVARS_TRACKER]= 0; m_trackers[CURRENT_SCHEMA_TRACKER]= ¤t_schema; m_trackers[SESSION_STATE_CHANGE_TRACKER]= &state_change; - m_trackers[TRANSACTION_INFO_TRACKER]= 0; + m_trackers[TRANSACTION_INFO_TRACKER]= &transaction_info; } @@ -1367,8 +1367,6 @@ void Session_tracker::enable(THD *thd) deinit(); m_trackers[SESSION_SYSVARS_TRACKER]= new (std::nothrow) Session_sysvars_tracker(); - m_trackers[TRANSACTION_INFO_TRACKER]= - new (std::nothrow) Transaction_state_tracker; for (int i= 0; i < SESSION_TRACKER_END; i++) m_trackers[i]->enable(thd); -- cgit v1.2.1 From 554ac6f393941040cea6d45d57a298e900bff193 Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Tue, 19 Mar 2019 00:40:26 +0400 Subject: Allocate Session_sysvars_tracker statically One less new/delete per connection. Removed m_mem_flag since most allocs are thread specific. The only exception are allocs performed during initialization. Removed State_tracker and Session_tracker constructors as they don't make sense anymore. No reason to access session_sysvars_tracker via get_tracker(), so access it directly instead. Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 196 +++---------------------------------------------- 1 file changed, 11 insertions(+), 185 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 2f72b7198f9..81da43a0946 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -16,8 +16,6 @@ #include "sql_plugin.h" -#include "session_tracker.h" - #include "hash.h" #include "table.h" #include "rpl_gtid.h" @@ -34,145 +32,6 @@ void State_tracker::mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name) } -/** - Session_sysvars_tracker - - This is a tracker class that enables & manages the tracking of session - system variables. It internally maintains a hash of user supplied variable - references and a boolean field to store if the variable was changed by the - last statement. -*/ - -class Session_sysvars_tracker : public State_tracker -{ - struct sysvar_node_st { - sys_var *m_svar; - bool *test_load; - bool m_changed; - }; - - class vars_list - { - /** - Registered system variables. (@@session_track_system_variables) - A hash to store the name of all the system variables specified by the - user. - */ - HASH m_registered_sysvars; - /** Size of buffer for string representation */ - size_t buffer_length; - myf m_mem_flag; - /** - If TRUE then we want to check all session variable. - */ - bool track_all; - void init() - { - my_hash_init(&m_registered_sysvars, - &my_charset_bin, - 4, 0, 0, (my_hash_get_key) sysvars_get_key, - my_free, MYF(HASH_UNIQUE | - ((m_mem_flag & MY_THREAD_SPECIFIC) ? - HASH_THREAD_SPECIFIC : 0))); - } - void free_hash() - { - if (my_hash_inited(&m_registered_sysvars)) - { - my_hash_free(&m_registered_sysvars); - } - } - - sysvar_node_st *search(const sys_var *svar) - { - return reinterpret_cast( - my_hash_search(&m_registered_sysvars, - reinterpret_cast(&svar), - sizeof(sys_var*))); - } - - sysvar_node_st *at(ulong i) - { - DBUG_ASSERT(i < m_registered_sysvars.records); - return reinterpret_cast( - my_hash_element(&m_registered_sysvars, i)); - } - public: - vars_list(): buffer_length(0), track_all(false) - { - m_mem_flag= current_thd ? MY_THREAD_SPECIFIC : 0; - init(); - } - - size_t get_buffer_length() - { - DBUG_ASSERT(buffer_length != 0); // asked earlier then should - return buffer_length; - } - ~vars_list() - { - /* free the allocated hash. */ - if (my_hash_inited(&m_registered_sysvars)) - { - my_hash_free(&m_registered_sysvars); - } - } - - sysvar_node_st *insert_or_search(const sys_var *svar) - { - sysvar_node_st *res= search(svar); - if (!res) - { - if (track_all) - { - insert(svar); - return search(svar); - } - } - return res; - } - - bool insert(const sys_var *svar); - void reinit(); - void reset(); - inline bool is_enabled() - { - return track_all || m_registered_sysvars.records; - } - void copy(vars_list* from, THD *thd); - bool parse_var_list(THD *thd, LEX_STRING var_list, bool throw_error, - CHARSET_INFO *char_set, bool take_mutex); - bool construct_var_list(char *buf, size_t buf_len); - bool store(THD *thd, String *buf); - }; - /** - Two objects of vars_list type are maintained to manage - various operations. - */ - vars_list orig_list; - -public: - size_t get_buffer_length() - { - return orig_list.get_buffer_length(); - } - bool construct_var_list(char *buf, size_t buf_len) - { - return orig_list.construct_var_list(buf, buf_len); - } - - bool enable(THD *thd); - bool update(THD *thd, set_var *var); - bool store(THD *thd, String *buf); - void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); - /* callback */ - static uchar *sysvars_get_key(const char *entry, size_t *length, - my_bool not_used __attribute__((unused))); - - friend bool sysvartrack_global_update(THD *thd, char *str, size_t len); -}; - - /* To be used in expanding the buffer. */ static const unsigned int EXTRA_ALLOC= 1024; @@ -217,7 +76,9 @@ bool Session_sysvars_tracker::vars_list::insert(const sys_var *svar) { sysvar_node_st *node; if (!(node= (sysvar_node_st *) my_malloc(sizeof(sysvar_node_st), - MYF(MY_WME | m_mem_flag)))) + MYF(MY_WME | + (mysqld_server_initialized ? + MY_THREAD_SPECIFIC : 0))))) return true; node->m_svar= (sys_var *)svar; @@ -511,6 +372,7 @@ bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, bool Session_sysvars_tracker::enable(THD *thd) { + orig_list.reinit(); mysql_mutex_lock(&LOCK_plugin); LEX_STRING tmp; tmp.str= global_system_variables.session_track_system_variables; @@ -519,6 +381,7 @@ bool Session_sysvars_tracker::enable(THD *thd) { mysql_mutex_unlock(&LOCK_plugin); orig_list.reinit(); + m_enabled= false; return true; } mysql_mutex_unlock(&LOCK_plugin); @@ -1330,49 +1193,6 @@ bool Session_state_change_tracker::store(THD *thd, String *buf) /////////////////////////////////////////////////////////////////////////////// -/** - @brief Initialize session tracker objects. -*/ - -Session_tracker::Session_tracker() -{ - /* track data ID fit into one byte in net coding */ - compile_time_assert(SESSION_TRACK_always_at_the_end < 251); - /* one tracker could serv several tracking data */ - compile_time_assert((uint)SESSION_TRACK_always_at_the_end >= - (uint)SESSION_TRACKER_END); - - m_trackers[SESSION_SYSVARS_TRACKER]= 0; - m_trackers[CURRENT_SCHEMA_TRACKER]= ¤t_schema; - m_trackers[SESSION_STATE_CHANGE_TRACKER]= &state_change; - m_trackers[TRANSACTION_INFO_TRACKER]= &transaction_info; -} - - -/** - @brief Enables the tracker objects. - - @param thd [IN] The thread handle. - - @return void -*/ - -void Session_tracker::enable(THD *thd) -{ - /* - Originally and correctly this allocation was in the constructor and - deallocation in the destructor, but in this case memory counting - system works incorrectly (for example in INSERT DELAYED thread) - */ - deinit(); - m_trackers[SESSION_SYSVARS_TRACKER]= - new (std::nothrow) Session_sysvars_tracker(); - - for (int i= 0; i < SESSION_TRACKER_END; i++) - m_trackers[i]->enable(thd); -} - - /** @brief Store all change information in the specified buffer. @@ -1385,6 +1205,12 @@ void Session_tracker::store(THD *thd, String *buf) { size_t start; + /* track data ID fit into one byte in net coding */ + compile_time_assert(SESSION_TRACK_always_at_the_end < 251); + /* one tracker could serv several tracking data */ + compile_time_assert((uint) SESSION_TRACK_always_at_the_end >= + (uint) SESSION_TRACKER_END); + /* Probably most track result will fit in 251 byte so lets made it at least efficient. We allocate 1 byte for length and then will move -- cgit v1.2.1 From 1b5cf2f7e7995f8ee17da76eb28633f652852d8f Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Tue, 19 Mar 2019 20:04:10 +0400 Subject: Safe session_track_system_variables snapshot When enabling system variables tracker, make a copy of global_system_variables.session_track_system_variables under LOCK_global_system_variables. This protects from concurrent variable updates and potential freed memory access, as well as from variable reconstruction (which was previously protected by LOCK_plugin). We can also use this copy as a session variable pointer, so that we don't have to allocate memory and reconstruct it every time it is referenced. For this very reason we don't need buffer_length stuff anymore. As well as don't we need to take LOCK_plugin early in ::enable(). Unified ::parse_var_list() to acquire LOCK_plugin unconditionally. For no apparent reason it wasn't previously acquired for global variable update. Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 91 ++++++++++++++++++++++++++------------------------ 1 file changed, 48 insertions(+), 43 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 81da43a0946..8b0e247767c 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -38,7 +38,6 @@ static const unsigned int EXTRA_ALLOC= 1024; void Session_sysvars_tracker::vars_list::reinit() { - buffer_length= 0; track_all= 0; if (m_registered_sysvars.records) my_hash_reset(&m_registered_sysvars); @@ -58,7 +57,6 @@ void Session_sysvars_tracker::vars_list::copy(vars_list* from, THD *thd) { track_all= from->track_all; free_hash(); - buffer_length= from->buffer_length; m_registered_sysvars= from->m_registered_sysvars; from->init(); } @@ -111,7 +109,6 @@ bool Session_sysvars_tracker::vars_list::insert(const sys_var *svar) in case of invalid/duplicate values. @param char_set [IN] charecter set information used for string manipulations. - @param take_mutex [IN] take LOCK_plugin @return true Error @@ -120,27 +117,21 @@ bool Session_sysvars_tracker::vars_list::insert(const sys_var *svar) bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, LEX_STRING var_list, bool throw_error, - CHARSET_INFO *char_set, - bool take_mutex) + CHARSET_INFO *char_set) { const char separator= ','; char *token, *lasts= NULL; size_t rest= var_list.length; if (!var_list.str || var_list.length == 0) - { - buffer_length= 1; return false; - } if(!strcmp(var_list.str, "*")) { track_all= true; - buffer_length= 2; return false; } - buffer_length= var_list.length + 1; token= var_list.str; track_all= false; @@ -150,8 +141,7 @@ bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, token value. Hence the mutex is handled here to avoid a performance overhead. */ - if (!thd || take_mutex) - mysql_mutex_lock(&LOCK_plugin); + mysql_mutex_lock(&LOCK_plugin); for (;;) { sys_var *svar; @@ -196,14 +186,12 @@ bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, else break; } - if (!thd || take_mutex) - mysql_mutex_unlock(&LOCK_plugin); + mysql_mutex_unlock(&LOCK_plugin); return false; error: - if (!thd || take_mutex) - mysql_mutex_unlock(&LOCK_plugin); + mysql_mutex_unlock(&LOCK_plugin); return true; } @@ -361,6 +349,25 @@ bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, return false; } + +void Session_sysvars_tracker::init(THD *thd) +{ + DBUG_ASSERT(thd->variables.session_track_system_variables == + global_system_variables.session_track_system_variables); + DBUG_ASSERT(global_system_variables.session_track_system_variables); + thd->variables.session_track_system_variables= + my_strdup(global_system_variables.session_track_system_variables, + MYF(MY_WME | MY_THREAD_SPECIFIC)); +} + + +void Session_sysvars_tracker::deinit(THD *thd) +{ + my_free(thd->variables.session_track_system_variables); + thd->variables.session_track_system_variables= 0; +} + + /** Enable session tracker by parsing global value of tracked variables. @@ -372,21 +379,16 @@ bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, bool Session_sysvars_tracker::enable(THD *thd) { + LEX_STRING tmp= { thd->variables.session_track_system_variables, + safe_strlen(thd->variables.session_track_system_variables) }; orig_list.reinit(); - mysql_mutex_lock(&LOCK_plugin); - LEX_STRING tmp; - tmp.str= global_system_variables.session_track_system_variables; - tmp.length= safe_strlen(tmp.str); - if (orig_list.parse_var_list(thd, tmp, true, thd->charset(), false) == true) + if (orig_list.parse_var_list(thd, tmp, true, thd->charset()) == true) { - mysql_mutex_unlock(&LOCK_plugin); orig_list.reinit(); m_enabled= false; return true; } - mysql_mutex_unlock(&LOCK_plugin); m_enabled= true; - return false; } @@ -412,10 +414,28 @@ bool Session_sysvars_tracker::enable(THD *thd) bool Session_sysvars_tracker::update(THD *thd, set_var *var) { vars_list tool_list; + void *copy= var->save_result.string_value.str ? + my_memdup(var->save_result.string_value.str, + var->save_result.string_value.length + 1, + MYF(MY_WME | MY_THREAD_SPECIFIC)) : + my_strdup("", MYF(MY_WME | MY_THREAD_SPECIFIC)); + + if (!copy) + return true; + if (tool_list.parse_var_list(thd, var->save_result.string_value, true, - thd->charset(), true)) + thd->charset())) + { + my_free(copy); return true; + } + + my_free(thd->variables.session_track_system_variables); + thd->variables.session_track_system_variables= static_cast(copy); + orig_list.copy(&tool_list, thd); + orig_list.construct_var_list(thd->variables.session_track_system_variables, + var->save_result.string_value.length + 1); return false; } @@ -562,7 +582,7 @@ bool sysvartrack_global_update(THD *thd, char *str, size_t len) { LEX_STRING tmp= { str, len }; Session_sysvars_tracker::vars_list dummy; - if (!dummy.parse_var_list(thd, tmp, false, system_charset_info, false)) + if (!dummy.parse_var_list(thd, tmp, false, system_charset_info)) { dummy.construct_var_list(str, len + 1); return false; @@ -571,27 +591,12 @@ bool sysvartrack_global_update(THD *thd, char *str, size_t len) } -uchar *sysvartrack_session_value_ptr(THD *thd, const LEX_CSTRING *base) -{ - Session_sysvars_tracker *tracker= static_cast - (thd->session_tracker.get_tracker(SESSION_SYSVARS_TRACKER)); - size_t len= tracker->get_buffer_length(); - char *res= (char*) thd->alloc(len + sizeof(char*)); - if (res) - { - char *buf= res + sizeof(char*); - *((char**) res)= buf; - tracker->construct_var_list(buf, len); - } - return (uchar*) res; -} - - int session_tracker_init() { + DBUG_ASSERT(global_system_variables.session_track_system_variables); if (sysvartrack_validate_value(0, global_system_variables.session_track_system_variables, - safe_strlen(global_system_variables.session_track_system_variables))) + strlen(global_system_variables.session_track_system_variables))) { sql_print_error("The variable session_track_system_variables has " "invalid values."); -- cgit v1.2.1 From 53671a1fff8d4aa0978be2fb916f8e053c09424a Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Wed, 20 Mar 2019 01:32:10 +0400 Subject: Make connect speed great again Rather than parsing session_track_system_variables when thread starts, do it when first trackable event occurs. Benchmarked on a 2socket/20core/40threads Broadwell system using sysbench connect brencmark @40 threads (with select 1 disabled): 101379.77 -> 143016.68 CPS, whereas 10.2 is currently at 137766.31 CPS. Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 8b0e247767c..db82b7dffe9 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -379,16 +379,10 @@ void Session_sysvars_tracker::deinit(THD *thd) bool Session_sysvars_tracker::enable(THD *thd) { - LEX_STRING tmp= { thd->variables.session_track_system_variables, - safe_strlen(thd->variables.session_track_system_variables) }; orig_list.reinit(); - if (orig_list.parse_var_list(thd, tmp, true, thd->charset()) == true) - { - orig_list.reinit(); - m_enabled= false; - return true; - } - m_enabled= true; + m_parsed= false; + m_enabled= thd->variables.session_track_system_variables && + *thd->variables.session_track_system_variables; return false; } @@ -433,6 +427,7 @@ bool Session_sysvars_tracker::update(THD *thd, set_var *var) my_free(thd->variables.session_track_system_variables); thd->variables.session_track_system_variables= static_cast(copy); + m_parsed= true; orig_list.copy(&tool_list, thd); orig_list.construct_var_list(thd->variables.session_track_system_variables, var->save_result.string_value.length + 1); @@ -540,6 +535,20 @@ void Session_sysvars_tracker::mark_as_changed(THD *thd, { sysvar_node_st *node; sys_var *svar= (sys_var *)var; + + if (!m_parsed) + { + DBUG_ASSERT(thd->variables.session_track_system_variables); + LEX_STRING tmp= { thd->variables.session_track_system_variables, + strlen(thd->variables.session_track_system_variables) }; + if (orig_list.parse_var_list(thd, tmp, true, thd->charset())) + { + orig_list.reinit(); + return; + } + m_parsed= true; + } + /* Check if the specified system variable is being tracked, if so mark it as changed and also set the class's m_changed flag. -- cgit v1.2.1 From 894df7edb67b888c41eae5ffbe654ceba97c6b8f Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Thu, 21 Mar 2019 00:42:48 +0400 Subject: Adieu find_sys_var_ex() Only take LOCK_plugin for plugin system variables. Reverted optimisation that was originally done for session tracker: it makes much less sense now. Specifically only if connections would want to track plugin session variables changes and these changes would actually happen frequently. If this ever becomes an issue, there're much better ways to optimise this workload. Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 38 ++++---------------------------------- 1 file changed, 4 insertions(+), 34 deletions(-) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index db82b7dffe9..63a6770f7d1 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -135,13 +135,6 @@ bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, token= var_list.str; track_all= false; - /* - If Lock to the plugin mutex is not acquired here itself, it results - in having to acquire it multiple times in find_sys_var_ex for each - token value. Hence the mutex is handled here to avoid a performance - overhead. - */ - mysql_mutex_lock(&LOCK_plugin); for (;;) { sys_var *svar; @@ -165,11 +158,10 @@ bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, { track_all= true; } - else if ((svar= - find_sys_var_ex(thd, var.str, var.length, throw_error, true))) + else if ((svar= find_sys_var(thd, var.str, var.length, throw_error))) { if (insert(svar) == TRUE) - goto error; + return true; } else if (throw_error && thd) { @@ -179,20 +171,14 @@ bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, "be ignored.", (int)var.length, token); } else - goto error; + return true; if (lasts) token= lasts + 1; else break; } - mysql_mutex_unlock(&LOCK_plugin); - return false; - -error: - mysql_mutex_unlock(&LOCK_plugin); - return true; } @@ -211,14 +197,6 @@ bool sysvartrack_validate_value(THD *thd, const char *str, size_t len) token= var_list.str; - /* - If Lock to the plugin mutex is not acquired here itself, it results - in having to acquire it multiple times in find_sys_var_ex for each - token value. Hence the mutex is handled here to avoid a performance - overhead. - */ - if (!thd) - mysql_mutex_lock(&LOCK_plugin); for (;;) { LEX_CSTRING var; @@ -237,22 +215,14 @@ bool sysvartrack_validate_value(THD *thd, const char *str, size_t len) /* Remove leading/trailing whitespace. */ trim_whitespace(system_charset_info, &var); - if(!strcmp(var.str, "*") && - !find_sys_var_ex(thd, var.str, var.length, false, true)) - { - if (!thd) - mysql_mutex_unlock(&LOCK_plugin); + if (!strcmp(var.str, "*") && !find_sys_var(thd, var.str, var.length)) return true; - } if (lasts) token= lasts + 1; else break; } - if (!thd) - mysql_mutex_unlock(&LOCK_plugin); - return false; } -- cgit v1.2.1 From 779fb636daf4c127dbb90f75bab004ac1bbe12df Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Wed, 20 Mar 2019 18:35:20 +0400 Subject: Revert THD::THD(skip_global_sys_var_lock) argument Originally introduced by e972125f1 to avoid harmless wait for LOCK_global_system_variables in a newly created thread, which creation was initiated by system variable update. At the same time it opens dangerous hole, when system variable update thread already released LOCK_global_system_variables and ack_receiver thread haven't yet completed new THD construction. In this case THD constructor goes completely unprotected. Since ack_receiver.stop() waits for the thread to go down, we have to temporarily release LOCK_global_system_variables so that it doesn't deadlock with ack_receiver.run(). Unfortunately it breaks atomicity of rpl_semi_sync_master_enabled updates and makes them not serialized. LOCK_rpl_semi_sync_master_enabled was introduced to workaround the above. TODO: move ack_receiver start/stop into repl_semisync_master enable_master/disable_master under LOCK_binlog protection? Part of MDEV-14984 - regression in connect performance --- sql/session_tracker.cc | 1 + 1 file changed, 1 insertion(+) (limited to 'sql/session_tracker.cc') diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 63a6770f7d1..f4dab11bb42 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -322,6 +322,7 @@ bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, void Session_sysvars_tracker::init(THD *thd) { + mysql_mutex_assert_owner(&LOCK_global_system_variables); DBUG_ASSERT(thd->variables.session_track_system_variables == global_system_variables.session_track_system_variables); DBUG_ASSERT(global_system_variables.session_track_system_variables); -- cgit v1.2.1