MXS-1865 Update server version only when (re)connecting

Updating it every iteration is needless.

include/maxscale/monitor.h

@@ -180,14 +180,14 @@ typedef enum
     MONITOR_STATE_FREED     = 0x08
 } monitor_state_t;
 
-/*
+/* Return type of mon_ping_or_connect_to_db(). */
- * Results of attempt at database connection for monitoring
- */
 typedef enum
 {
-    MONITOR_CONN_OK,
+    MONITOR_CONN_EXISTING_OK,   /* Existing connection was ok and server replied to ping. */
-    MONITOR_CONN_REFUSED,
+    MONITOR_CONN_NEWCONN_OK,    /* No existing connection or no ping reply. New connection created
-    MONITOR_CONN_TIMEOUT
+                                 * successfully. */
+    MONITOR_CONN_REFUSED,       /* No existing connection or no ping reply. Server refused new connection. */
+    MONITOR_CONN_TIMEOUT        /* No existing connection or no ping reply. Timeout on new connection. */
 } mxs_connect_result_t;
 
 /** Monitor events */
@@ -326,6 +326,7 @@ bool mon_status_changed(MXS_MONITORED_SERVER* mon_srv);
 bool mon_print_fail_status(MXS_MONITORED_SERVER* mon_srv);
 
 mxs_connect_result_t mon_ping_or_connect_to_db(MXS_MONITOR* mon, MXS_MONITORED_SERVER *database);
+bool mon_connection_is_ok(mxs_connect_result_t connect_result);
 void mon_log_connect_error(MXS_MONITORED_SERVER* database, mxs_connect_result_t rval);
 const char* mon_get_event_name(mxs_monitor_event_t event);
 

server/core/monitor.cc

@@ -817,7 +817,7 @@ bool check_monitor_permissions(MXS_MONITOR* monitor, const char* query)
 
     for (MXS_MONITORED_SERVER *mondb = monitor->monitored_servers; mondb; mondb = mondb->next)
     {
-        if (mon_ping_or_connect_to_db(monitor, mondb) != MONITOR_CONN_OK)
+        if (!mon_connection_is_ok(mon_ping_or_connect_to_db(monitor, mondb)))
         {
             MXS_ERROR("[%s] Failed to connect to server '%s' ([%s]:%d) when"
                       " checking monitor user credentials and permissions: %s",
@@ -1449,29 +1449,28 @@ int monitor_launch_script(MXS_MONITOR* mon, MXS_MONITORED_SERVER* ptr, const cha
 }
 
 /**
- * Ping or, if connection does not exist or ping fails, connect to a database. This
+ * Ping or connect to a database. If connection does not exist or ping fails, a new connection is created.
- * will always leave a valid database handle in the database->con pointer, allowing
+ * This will always leave a valid database handle in the database->con pointer, allowing the user to call
- * the user to call MySQL C API functions to find out the reason of the failure.
+ * MySQL C API functions to find out the reason of the failure.
  *
  * @param mon Monitor
  * @param database Monitored database
- * @return MONITOR_CONN_OK if the connection is OK, else the reason for the failure
+ * @return Connection status.
  */
-mxs_connect_result_t
+mxs_connect_result_t mon_ping_or_connect_to_db(MXS_MONITOR* mon, MXS_MONITORED_SERVER *database)
-mon_ping_or_connect_to_db(MXS_MONITOR* mon, MXS_MONITORED_SERVER *database)
 {
-    /** Return if the connection is OK */
-    if (database->con && mysql_ping(database->con) == 0)
-    {
-        return MONITOR_CONN_OK;
-    }
-
     if (database->con)
     {
+        /** Return if the connection is OK */
+        if (mysql_ping(database->con) == 0)
+        {
+            return MONITOR_CONN_EXISTING_OK;
+        }
+        /** Otherwise close the handle. */
         mysql_close(database->con);
     }
 
-    mxs_connect_result_t rval = MONITOR_CONN_REFUSED;
+    mxs_connect_result_t conn_result = MONITOR_CONN_REFUSED;
     if ((database->con = mysql_init(NULL)))
     {
         char *uname = mon->user;
@@ -1500,36 +1499,46 @@ mon_ping_or_connect_to_db(MXS_MONITOR* mon, MXS_MONITORED_SERVER *database)
 
             if (result)
             {
-                rval = MONITOR_CONN_OK;
+                conn_result = MONITOR_CONN_NEWCONN_OK;
                 break;
             }
         }
 
-        if (rval == MONITOR_CONN_REFUSED &&
+        if (conn_result == MONITOR_CONN_REFUSED && (int)difftime(end, start) >= mon->connect_timeout)
-            (int)difftime(end, start) >= mon->connect_timeout)
         {
-            rval = MONITOR_CONN_TIMEOUT;
+            conn_result = MONITOR_CONN_TIMEOUT;
         }
         MXS_FREE(dpwd);
     }
 
-    return rval;
+    return conn_result;
 }
 
 /**
- * Log an error about the failure to connect to a backend server
+ * Is the return value one of the 'OK' values.
- * and why it happened.
+ *
- * @param database Backend database
+ * @param connect_result Return value of mon_ping_or_connect_to_db
- * @param rval Return value of mon_connect_to_db
+ * @return True of connection is ok
  */
-void
+bool mon_connection_is_ok(mxs_connect_result_t connect_result)
-mon_log_connect_error(MXS_MONITORED_SERVER* database, mxs_connect_result_t rval)
 {
-    MXS_ERROR(rval == MONITOR_CONN_TIMEOUT ?
+    return (connect_result == MONITOR_CONN_EXISTING_OK || connect_result == MONITOR_CONN_NEWCONN_OK);
-              "Monitor timed out when connecting to server [%s]:%d : \"%s\"" :
+}
-              "Monitor was unable to connect to server [%s]:%d : \"%s\"",
+
-              database->server->address, database->server->port,
+/**
-              mysql_error(database->con));
+ * Log an error about the failure to connect to a backend server and why it happened.
+ *
+ * @param database Backend database
+ * @param rval Return value of mon_ping_or_connect_to_db
+ */
+void mon_log_connect_error(MXS_MONITORED_SERVER* database, mxs_connect_result_t rval)
+{
+    ss_dassert(!mon_connection_is_ok(rval) && database);
+    const char TIMED_OUT[] = "Monitor timed out when connecting to server %s[%s:%d] : '%s'";
+    const char REFUSED[] = "Monitor was unable to connect to server %s[%s:%d] : '%s'";
+    auto srv = database->server;
+    MXS_ERROR(rval == MONITOR_CONN_TIMEOUT ? TIMED_OUT : REFUSED,
+              srv->name, srv->address, srv->port, mysql_error(database->con));
 }
 
 static void mon_log_state_change(MXS_MONITORED_SERVER *ptr)

server/modules/monitor/auroramon/auroramon.cc

@@ -57,7 +57,7 @@ void update_server_status(MXS_MONITOR *monitor, MXS_MONITORED_SERVER *database)
         /** Try to connect to or ping the database */
         mxs_connect_result_t rval = mon_ping_or_connect_to_db(monitor, database);
 
-        if (rval == MONITOR_CONN_OK)
+        if (mon_connection_is_ok(rval))
         {
             server_set_status_nolock(&temp_server, SERVER_RUNNING);
             MYSQL_RES *result;

server/modules/monitor/galeramon/galeramon.cc

@@ -355,7 +355,7 @@ monitorDatabase(MXS_MONITOR *mon, MXS_MONITORED_SERVER *database)
     database->mon_prev_status = database->server->status;
 
     mxs_connect_result_t rval = mon_ping_or_connect_to_db(mon, database);
-    if (rval != MONITOR_CONN_OK)
+    if (!mon_connection_is_ok(rval))
     {
         if (mysql_errno(database->con) == ER_ACCESS_DENIED_ERROR)
         {

server/modules/monitor/grmon/grmon.cc

@@ -193,7 +193,7 @@ static void update_server_status(MXS_MONITOR* monitor, MXS_MONITORED_SERVER* ser
 
     mxs_connect_result_t rval = mon_ping_or_connect_to_db(monitor, server);
 
-    if (rval != MONITOR_CONN_OK)
+    if (!mon_connection_is_ok(rval))
     {
         if (mysql_errno(server->con) == ER_ACCESS_DENIED_ERROR)
         {

server/modules/monitor/mariadbmon/cluster_discovery.cc

@@ -453,7 +453,7 @@ void MariaDBMonitor::check_maxscale_schema_replication()
     while (database)
     {
         mxs_connect_result_t rval = mon_ping_or_connect_to_db(m_monitor_base, database);
-        if (rval == MONITOR_CONN_OK)
+        if (mon_connection_is_ok(rval))
         {
             if (!check_replicate_ignore_table(database) ||
                 !check_replicate_do_table(database) ||

server/modules/monitor/mariadbmon/mariadbserver.cc

@@ -685,10 +685,18 @@ void MariaDBServer::monitor_server(MXS_MONITOR* base_monitor)
 
     SERVER* srv = mon_srv->server;
     MYSQL* conn = mon_srv->con; // mon_ping_or_connect_to_db() may have reallocated the MYSQL struct.
-    if (rval == MONITOR_CONN_OK)
+    if (mon_connection_is_ok(rval))
     {
         server_clear_status_nolock(srv, SERVER_AUTH_ERROR);
         monitor_clear_pending_status(mon_srv, SERVER_AUTH_ERROR);
+        /* Store current status in both server and monitor server pending struct */
+        server_set_status_nolock(srv, SERVER_RUNNING);
+        monitor_set_pending_status(mon_srv, SERVER_RUNNING);
+
+        if (rval == MONITOR_CONN_NEWCONN_OK)
+        {
+            update_server_info();
+        }
     }
     else
     {
@@ -713,39 +721,6 @@ void MariaDBServer::monitor_server(MXS_MONITOR* base_monitor)
         return;
     }
 
-    /* Store current status in both server and monitor server pending struct */
-    server_set_status_nolock(srv, SERVER_RUNNING);
-    monitor_set_pending_status(mon_srv, SERVER_RUNNING);
-
-    /* Check whether current server is MaxScale Binlog Server */
-    MYSQL_RES *result;
-    if (mxs_mysql_query(conn, "SELECT @@maxscale_version") == 0 &&
-        (result = mysql_store_result(conn)) != NULL)
-    {
-        m_binlog_relay = true;
-        mysql_free_result(result);
-    }
-    else
-    {
-        m_binlog_relay = false;
-    }
-
-    /* Get server version string, also get/set numeric representation. */
-    mxs_mysql_set_server_version(conn, srv);
-    /* Set monitor version enum. */
-    uint64_t version_num = server_get_version(srv);
-    if (version_num >= 100000)
-    {
-        m_version = MariaDBServer::MARIADB_VERSION_100;
-    }
-    else if (version_num >= 5 * 10000 + 5 * 100)
-    {
-        m_version = MariaDBServer::MARIADB_VERSION_55;
-    }
-    else
-    {
-        m_version = MariaDBServer::MARIADB_VERSION_UNKNOWN;
-    }
     /* Query a few settings. */
     read_server_variables();
     /* If gtid domain exists and server is 10.0, update gtid:s */
@@ -790,6 +765,45 @@ void MariaDBServer::update_slave_status()
     }
 }
 
+/**
+ * Update information which changes rarely. This method should be called after (re)connecting to a backend.
+ * Calling this every monitoring loop is overkill.
+ */
+void MariaDBServer::update_server_info()
+{
+    auto conn = m_server_base->con;
+    // Check whether this server is a MaxScale Binlog Server.
+    MYSQL_RES *result;
+    if (mxs_mysql_query(conn, "SELECT @@maxscale_version") == 0 &&
+        (result = mysql_store_result(conn)) != NULL)
+    {
+        m_binlog_relay = true;
+        mysql_free_result(result);
+    }
+    else
+    {
+        m_binlog_relay = false;
+    }
+
+    /* Get server version string, also get/set numeric representation. These functions do not actually
+     * query the server, since the data was obtained when connecting. */
+    mxs_mysql_set_server_version(conn, m_server_base->server);
+    /* Set monitor version enum. */
+    uint64_t version_num = server_get_version(m_server_base->server);
+    if (version_num >= 100000)
+    {
+        m_version = MariaDBServer::MARIADB_VERSION_100;
+    }
+    else if (version_num >= 5 * 10000 + 5 * 100)
+    {
+        m_version = MariaDBServer::MARIADB_VERSION_55;
+    }
+    else
+    {
+        m_version = MariaDBServer::MARIADB_VERSION_UNKNOWN;
+    }
+}
+
 SlaveStatus::slave_io_running_t SlaveStatus::slave_io_from_string(const std::string& str)
 {
     slave_io_running_t rval = SLAVE_IO_NO;

server/modules/monitor/mariadbmon/mariadbserver.hh

@@ -295,6 +295,7 @@ public:
 private:
     void monitor_server(MXS_MONITOR* base_monitor);
     void update_slave_status();
+    void update_server_info();
 };
 
 /**

server/modules/monitor/mmmon/mmmon.cc

@@ -259,7 +259,7 @@ monitorDatabase(MXS_MONITOR* mon, MXS_MONITORED_SERVER *database)
     database->mon_prev_status = database->server->status;
     mxs_connect_result_t rval = mon_ping_or_connect_to_db(mon, database);
 
-    if (rval != MONITOR_CONN_OK)
+    if (!mon_connection_is_ok(rval))
     {
         if (mysql_errno(database->con) == ER_ACCESS_DENIED_ERROR)
         {

server/modules/monitor/ndbclustermon/ndbclustermon.cc

@@ -237,7 +237,7 @@ monitorDatabase(MXS_MONITORED_SERVER *database, char *defaultUser, char *default
     }
 
     mxs_connect_result_t rval = mon_ping_or_connect_to_db(mon, database);
-    if (rval != MONITOR_CONN_OK)
+    if (!mon_connection_is_ok(rval))
     {
         server_clear_status_nolock(database->server, SERVER_RUNNING);