Prevent flushing nonsuper_connections and Reference count nonsuper_co…

…nnections

Summary:
nonsuper_connections was getting underflows because it was getting reset
during "flush status". In 8.0, the flags should have been updated to
SHOW_LONG_NOFLUSH.

It is also possible for privileges to be granted/revoked after
connection setup, but the nonsuper_connections count were not adjusted
appropriately. On cleanup, the new privileges were used to determine
if nonsuper_connections needed to be decremented.

Improve the robustness of tracking nonsuper_connections references
by storing counts in the THD.  Allow each THD to increment
nonsuper_connections at most once and track nonsuper_connections
increments within the THD. Apply the appropriate decrements when the
structure is freed or reinitialized.

Switching to atomic counters for nonsuper_connections removes the
LOCK_user_conn requirements and reduces the risk of deadlocks when
cleaning up THD's.

Squash with D13987618

Reviewed By: pradeep1288

Differential Revision: D21562003

fbshipit-source-id: 9e4b8eb

sql/auth/sql_authentication.cc

@@ -3523,12 +3523,9 @@ int acl_authenticate(THD *thd, enum_server_command command) {
                         mpvio.db.str));
 
     if (!thd->m_main_security_ctx.check_access(SUPER_ACL)) {
-      mysql_mutex_lock(&LOCK_user_conn);
       // this is non-super user, increment nonsuper_connections
-      nonsuper_connections++;
       const bool limit_reached =
-          nonsuper_connections > max_nonsuper_connections;
-      mysql_mutex_unlock(&LOCK_user_conn);
+          thd->add_nonsuper_connections_ref() > max_nonsuper_connections;
 
       if (max_nonsuper_connections &&
           limit_reached) {  // max_nonsuper_connections limit reached

sql/mysqld.cc

@@ -1262,7 +1262,8 @@ ulong specialflag = 0;
 ulong binlog_cache_use = 0, binlog_cache_disk_use = 0;
 ulong binlog_stmt_cache_use = 0, binlog_stmt_cache_disk_use = 0;
 ulong max_connections, max_connect_errors;
-ulong max_nonsuper_connections = 0, nonsuper_connections = 0;
+ulong max_nonsuper_connections = 0;
+std::atomic<ulong> nonsuper_connections(0);
 ulong opt_max_running_queries, opt_max_waiting_queries;
 ulong rpl_stop_slave_timeout = LONG_TIMEOUT;
 bool rpl_skip_tx_api = false;
@@ -9130,7 +9131,7 @@ SHOW_VAR status_vars[] = {
      SHOW_SCOPE_GLOBAL},
     {"Max_used_connections_time", (char *)&show_max_used_connections_time,
      SHOW_FUNC, SHOW_SCOPE_GLOBAL},
-    {"Non_super_connections", (char *)&nonsuper_connections, SHOW_LONG,
+    {"Non_super_connections", (char *)&nonsuper_connections, SHOW_LONG_NOFLUSH,
      SHOW_SCOPE_GLOBAL},
     {"Not_flushed_delayed_rows", (char *)&delayed_rows_in_use,
      SHOW_LONG_NOFLUSH, SHOW_SCOPE_GLOBAL},

sql/mysqld.h

@@ -331,7 +331,8 @@ extern MYSQL_PLUGIN_IMPORT ulong max_connections;
 extern ulong opt_max_running_queries, opt_max_waiting_queries;
 extern ulong max_digest_length;
 extern ulong max_connect_errors, connect_timeout;
-extern ulong max_nonsuper_connections, nonsuper_connections;
+extern ulong max_nonsuper_connections;
+extern std::atomic<ulong> nonsuper_connections;
 extern bool opt_slave_allow_batching;
 extern ulong slave_trans_retries;
 extern uint slave_net_timeout;

sql/sql_class.cc

@@ -1135,6 +1135,9 @@ void THD::release_resources() {
     get_protocol_classic()->end_net();
   }
 
+  /* Free nonsuper connection reference if needed */
+  remove_nonsuper_connections_ref(true);
+
   /* modification plan for UPDATE/DELETE should be freed. */
   DBUG_ASSERT(query_plan.get_modification_plan() == NULL);
   mysql_mutex_unlock(&LOCK_query_plan);
@@ -1198,6 +1201,8 @@ THD::~THD() {
 
   if (!m_release_resources_done) release_resources();
 
+  DBUG_ASSERT(nonsuper_ref == 0);
+
   clear_next_event_pos();
 
   /* Ensure that no one is using THD */
@@ -2492,6 +2497,40 @@ void THD::increment_questions_counter() {
   DBUG_VOID_RETURN;
 }
 
+ulong THD::add_nonsuper_connections_ref() {
+  DBUG_ENTER("THD::add_nonsuper_connections_ref");
+
+  // Changing user temporarily is considered two references
+  // If max_nonsuper_connections is 1, then this could be a problem
+  nonsuper_ref++;
+  ulong count = ++nonsuper_connections;
+
+  DBUG_RETURN(count);
+}
+
+void THD::remove_nonsuper_connections_ref(bool clear) {
+  DBUG_ENTER("THD::remove_nonsuper_connections_ref");
+
+  if (nonsuper_ref == 0) {
+    DBUG_VOID_RETURN;
+  }
+
+  bool underflow;
+  if (clear) {
+    underflow = nonsuper_connections < nonsuper_ref;
+    nonsuper_connections -= nonsuper_ref;
+    nonsuper_ref = 0;
+  } else {
+    underflow = ((nonsuper_connections--) == 0);
+    nonsuper_ref--;
+  }
+
+  DBUG_ASSERT(!underflow);
+  (void)underflow;  // prevent compiler warning about unused on non-debug builds
+
+  DBUG_VOID_RETURN;
+}
+
 /*
   Reset per-hour user resource limits when it has been more than
   an hour since they were last checked

sql/sql_class.h

@@ -930,6 +930,14 @@ class THD : public MDL_context_owner,
   bool should_write_hlc = false;
   database_container databases;
 
+  /* Nonsuper_connections reference counting */
+ private:
+  ulong nonsuper_ref = 0;
+
+ public:
+  ulong add_nonsuper_connections_ref();
+  void remove_nonsuper_connections_ref(bool clear = false);
+
   /**
     The function checks whether the thread is processing queries from binlog,
     as automatically generated by mysqlbinlog.

sql/sql_connect.cc

@@ -276,11 +276,15 @@ void release_user_connection(THD *thd) {
   const USER_CONN *uc = thd->get_user_connect();
   DBUG_ENTER("release_user_connection");
 
+  // In 8.0, uc might be null this connection.
+  // It is also possible for the user's privilege access to have changed
+  // due to REVOKE after the connection was established. This means
+  // add_nonsuper_connections_ref was not called (at the time, it had
+  // SUPER access), but remove_nonsuper_connections_ref() is called here
+  // during cleanup because the it no longer has SUPER access.
   if (!thd->m_main_security_ctx.check_access(SUPER_ACL)) {
-    mysql_mutex_lock(&LOCK_user_conn);
     // this is non-super user, decrement nonsuper_connections
-    nonsuper_connections--;
-    mysql_mutex_unlock(&LOCK_user_conn);
+    thd->remove_nonsuper_connections_ref();
   }
 
   if (uc) {

sql/sql_parse.cc

@@ -1782,10 +1782,8 @@ bool dispatch_command(THD *thd, const COM_DATA *com_data,
 #endif /* HAVE_PSI_THREAD_INTERFACE */
 
         if (!save_security_ctx.check_access(SUPER_ACL)) {
-          mysql_mutex_lock(&LOCK_user_conn);
           // previous user was a non-super user, decrement nonsuper_connections
-          nonsuper_connections--;
-          mysql_mutex_unlock(&LOCK_user_conn);
+          thd->remove_nonsuper_connections_ref();
         }
         if (save_user_connect) decrease_user_connections(save_user_connect);
         mysql_mutex_lock(&thd->LOCK_thd_data);