Bug#31968366 SET PERSIST FOR COMPONENTS GENERATES A FLOOD OF WARNINGS

Description:
-----------
Persisting component-defined system variables causes a flood of "Currently
unknown variable" messages when the component is installed and registering
its system variables. This also results in repeated and unnecessary
invocations of the check() and update() callback functions for each system
variable, persisted or otherwise.

Analysis:
---------
While registering component system variable it will add the variable into
system_variable_hash and then in Persisted_variables_cache::set_persist_options()
the persisted variables will be find in system_variable_hash if not found, then it
will be stored in m_persist_plugin_variables.
In the current case while loading the validate password component we have
validate_password.check_user_name which is 5th variable while loading the component.
So while registering first variable validate_password.check_user_name will not
be found in system_variable_hash, and hence that variable is stored in
m_persist_plugin_variables and the storing happens for multiple times because
m_persist_plugin_variables is a vector type.

Fix:
----
changed m_persist_plugin_variables type to unordered_set from vector type,
so that variables duplication will not be allowed.

RB#26373

Author: vsmurthys

mysql-test/r/persisted_variables_for_component.result

@@ -49,8 +49,8 @@ validate_password.policy	STRONG
 SET PERSIST validate_password.length= 13;
 SELECT * FROM performance_schema.persisted_variables;
 VARIABLE_NAME	VARIABLE_VALUE
-validate_password.policy	STRONG
 validate_password.length	13
+validate_password.policy	STRONG
 RESET PERSIST `validate_password.length`;
 SELECT * FROM performance_schema.persisted_variables;
 VARIABLE_NAME	VARIABLE_VALUE
@@ -64,3 +64,23 @@ validate_password.policy	STRONG
 RESET PERSIST;
 SELECT * FROM performance_schema.persisted_variables;
 VARIABLE_NAME	VARIABLE_VALUE
+#
+# BUG#31968366: SET PERSIST FOR COMPONENTS GENERATES A FLOOD OF WARNINGS
+#
+INSTALL COMPONENT 'file://component_validate_password';
+SET @@persist.validate_password.length=10;
+SET @@persist.validate_password.check_user_name=OFF;
+SELECT * FROM performance_schema.persisted_variables;
+VARIABLE_NAME	VARIABLE_VALUE
+validate_password.check_user_name	OFF
+validate_password.length	10
+UNINSTALL COMPONENT 'file://component_validate_password';
+INSTALL COMPONENT 'file://component_validate_password';
+SELECT COUNT(*) FROM performance_schema.error_log WHERE ERROR_CODE = "MY-013185";
+COUNT(*)
+0
+UNINSTALL COMPONENT 'file://component_validate_password';
+# Test RESET PERSIST after component is uninstalled
+RESET PERSIST;
+SELECT * FROM performance_schema.persisted_variables;
+VARIABLE_NAME	VARIABLE_VALUE

mysql-test/r/read_only_persisted_plugin_variables.result

@@ -118,9 +118,9 @@ CALL mtr.add_suppression("Plugin audit_log reported *");
 # Both queries must return 101 rows.
 SELECT * FROM performance_schema.persisted_variables;
 VARIABLE_NAME	VARIABLE_VALUE
+replica_type_conversions	
 innodb_log_buffer_size	16777216
 slave_type_conversions	
-replica_type_conversions	
 back_log	80
 binlog_gtid_simple_recovery	ON
 disabled_storage_engines	

mysql-test/t/persisted_variables_for_component.test

@@ -90,3 +90,22 @@ RESET PERSIST;
 # return 0 rows
 SELECT * FROM performance_schema.persisted_variables;
 
+
+--echo #
+--echo # BUG#31968366: SET PERSIST FOR COMPONENTS GENERATES A FLOOD OF WARNINGS
+--echo #
+
+INSTALL COMPONENT 'file://component_validate_password';
+SET @@persist.validate_password.length=10;
+SET @@persist.validate_password.check_user_name=OFF;
+# return 2 rows
+SELECT * FROM performance_schema.persisted_variables;
+UNINSTALL COMPONENT 'file://component_validate_password';
+INSTALL COMPONENT 'file://component_validate_password';
+# with fix should return 0 rows, without fix it will return 63 rows
+SELECT COUNT(*) FROM performance_schema.error_log WHERE ERROR_CODE = "MY-013185";
+UNINSTALL COMPONENT 'file://component_validate_password';
+--echo # Test RESET PERSIST after component is uninstalled
+RESET PERSIST;
+# return 0 rows
+SELECT * FROM performance_schema.persisted_variables;

sql/persisted_variable.cc

@@ -395,24 +395,15 @@ bool Persisted_variables_cache::set_variable(THD *thd, set_var *setvar) {
       m_persist_ro_variables[tmp_var.key] = tmp_var;
     else {
       /*
-       if element is present remove from current position and insert
-       at end of vector to restore insertion order.
+       if element is present remove it and insert
+       it again with new value.
       */
-      string str = tmp_var.key;
-      auto itt =
-          std::find_if(m_persist_variables.begin(), m_persist_variables.end(),
-                       [str](st_persist_var const &s) { return s.key == str; });
-      if (itt != m_persist_variables.end()) m_persist_variables.erase(itt);
-      m_persist_variables.push_back(tmp_var);
+      m_persist_variables.erase(tmp_var);
+      m_persist_variables.insert(tmp_var);
       /* for plugin variables update m_persist_plugin_variables */
       if (setvar->var->cast_pluginvar()) {
-        auto it = std::find_if(
-            m_persist_plugin_variables.begin(),
-            m_persist_plugin_variables.end(),
-            [str](st_persist_var const &s) { return s.key == str; });
-        if (it != m_persist_plugin_variables.end())
-          m_persist_plugin_variables.erase(it);
-        m_persist_plugin_variables.push_back(tmp_var);
+        m_persist_plugin_variables.erase(tmp_var);
+        m_persist_plugin_variables.insert(tmp_var);
       }
     }
     unlock();
@@ -698,7 +689,8 @@ bool Persisted_variables_cache::set_persist_options(bool plugin_options) {
   THD *thd;
   LEX lex_tmp, *sav_lex = nullptr;
   List<set_var_base> tmp_var_list;
-  vector<st_persist_var> *persist_variables = nullptr;
+  std::unordered_set<st_persist_var, st_persist_var_hash> *persist_variables =
+      nullptr;
   bool result = false, new_thd = false;
   const std::vector<std::string> priv_list = {
       "ENCRYPTION_KEY_ADMIN", "ROLE_ADMIN", "SYSTEM_VARIABLES_ADMIN",
@@ -779,9 +771,10 @@ bool Persisted_variables_cache::set_persist_options(bool plugin_options) {
         keep track of this variable so that it is set when plugin
         is loaded and continue with remaining persisted variables
       */
-      m_persist_plugin_variables.push_back(*iter);
-      LogErr(WARNING_LEVEL, ER_UNKNOWN_VARIABLE_IN_PERSISTED_CONFIG_FILE,
-             var_name.c_str());
+      auto ret = m_persist_plugin_variables.insert(*iter);
+      if (ret.second == true)
+        LogErr(WARNING_LEVEL, ER_UNKNOWN_VARIABLE_IN_PERSISTED_CONFIG_FILE,
+               var_name.c_str());
       continue;
     }
     /*
@@ -867,9 +860,7 @@ bool Persisted_variables_cache::set_persist_options(bool plugin_options) {
       Once persisted variables are SET in the server,
       update variables source/user/timestamp/host from m_persist_variables.
     */
-    auto it = std::find_if(
-        m_persist_variables.begin(), m_persist_variables.end(),
-        [var_name](st_persist_var const &s) { return s.key == var_name; });
+    auto it = m_persist_variables.find(*iter);
     if (it != m_persist_variables.end()) {
       /* persisted variable is found */
       sysvar->set_source(enum_variable_source::PERSISTED);
@@ -1047,7 +1038,7 @@ bool Persisted_variables_cache::extract_variables_from_json(const Json_dom *dom,
     if (is_read_only)
       m_persist_ro_variables[var_name] = persist_var;
     else
-      m_persist_variables.push_back(persist_var);
+      m_persist_variables.insert(persist_var);
     unlock();
   }
   return false;
@@ -1092,14 +1083,12 @@ void Persisted_variables_cache::load_aliases() {
         }
       };
   lock();
-  size_t size = m_persist_variables.size();
-  for (size_t i = 0; i < size; i++) {
-    auto var = m_persist_variables[i];
+  for (auto iter : m_persist_variables) {
     insert_alias(
         [&](const char *name) -> bool {
           return var_set.find(name) != var_set.end();
         },
-        [&](st_persist_var &v) { m_persist_variables.push_back(v); }, var);
+        [&](st_persist_var &v) { m_persist_variables.insert(v); }, iter);
   }
   for (auto pair : m_persist_ro_variables) {
     insert_alias(
@@ -1375,7 +1364,8 @@ bool Persisted_variables_cache::reset_persisted_variables(THD *thd,
 /**
   Return in-memory copy persist_variables_
 */
-vector<st_persist_var> *Persisted_variables_cache::get_persisted_variables() {
+std::unordered_set<st_persist_var, st_persist_var_hash>
+    *Persisted_variables_cache::get_persisted_variables() {
   return &m_persist_variables;
 }
 

sql/persisted_variable.h

@@ -26,7 +26,7 @@
 #include <stddef.h>
 #include <map>
 #include <string>
-#include <vector>
+#include <unordered_set>
 
 #include "my_alloc.h"
 #include "my_inttypes.h"
@@ -60,6 +60,21 @@ struct st_persist_var {
   st_persist_var(const std::string key, const std::string value,
                  const ulonglong timestamp, const std::string user,
                  const std::string host, const bool is_null);
+  /* This is custom comparision function used to make the unordered_set
+     to work with the default std::hash for userdefined types. */
+  bool operator==(const st_persist_var &persist_var) const {
+    return key == persist_var.key;
+  }
+};
+
+/**
+  STRUCT st_persist_var_hash
+
+  This structure has a custom hasher function used to make the unordered_set
+  to work with the default std::hash for userdefined types.
+*/
+struct st_persist_var_hash {
+  size_t operator()(const st_persist_var &pv) const { return pv.key.length(); }
 };
 
 /**
@@ -71,7 +86,7 @@ struct st_persist_var {
   --------
   When first SET PERSIST statement is executed we instantiate
   Persisted_variables_cache which loads the config file if present into
-  m_persist_variables vector. This is a singleton operation. m_persist_variables
+  m_persist_variables set. This is a singleton operation. m_persist_variables
   is an in-memory copy of config file itself. If the SET statement passes then
   this in-memory is updated and flushed to file as an atomic operation.
 
@@ -114,7 +129,8 @@ class Persisted_variables_cache {
   /**
     Get persisted variables
   */
-  std::vector<st_persist_var> *get_persisted_variables();
+  std::unordered_set<st_persist_var, st_persist_var_hash>
+      *get_persisted_variables();
   /**
     Get persisted static variables
   */
@@ -174,9 +190,10 @@ class Persisted_variables_cache {
 
  private:
   /* In memory copy of persistent config file */
-  std::vector<st_persist_var> m_persist_variables;
+  std::unordered_set<st_persist_var, st_persist_var_hash> m_persist_variables;
   /* copy of plugin variables whose plugin is not yet installed */
-  std::vector<st_persist_var> m_persist_plugin_variables;
+  std::unordered_set<st_persist_var, st_persist_var_hash>
+      m_persist_plugin_variables;
   /* In memory copy of read only persistent variables */
   std::map<std::string, st_persist_var> m_persist_ro_variables;
 

storage/perfschema/pfs_variable.cc

@@ -490,7 +490,7 @@ int PFS_system_persisted_variables_cache::do_materialize_all(THD *unsafe_thd) {
   if ((m_safe_thd = get_THD(unsafe_thd)) != nullptr) {
     Persisted_variables_cache *pv = Persisted_variables_cache::get_instance();
     if (pv) {
-      vector<st_persist_var> *persist_variables = pv->get_persisted_variables();
+      auto *persist_variables = pv->get_persisted_variables();
       pv->lock();
       for (auto iter = persist_variables->begin();
            iter != persist_variables->end(); iter++) {