MXS-1503: Remove redundant code

Moved session command execution into the Backend class itself as the
session commands are defined as a related part of it. This allows all
connections to execute session commands if some are available.

Removed explicit SERVER_REF usage in the readwritesplit connection
creation code and replaced it with SRWBackend. This allows the removal of
the get_root_master_backend function which duplicated the functionality in
get_root_master.

include/maxscale/backend.hh

@@ -143,10 +143,11 @@ public:
      * @brief Create a new connection
      *
      * @param session The session to which the connection is linked
+     * @param sescmd  Pointer to a list of session commands to execute
      *
      * @return True if connection was successfully created
      */
-    bool connect(MXS_SESSION* session);
+    bool connect(MXS_SESSION* session, SessionCommandList* sescmd = NULL);
 
     /**
      * @brief Close the backend

server/core/backend.cc

@@ -176,7 +176,7 @@ void Backend::set_state(backend_state state)
     m_state |= state;
 }
 
-bool Backend::connect(MXS_SESSION* session)
+bool Backend::connect(MXS_SESSION* session, SessionCommandList* sescmd)
 {
     bool rval = false;
 
@@ -186,6 +186,16 @@ bool Backend::connect(MXS_SESSION* session)
         m_state = IN_USE;
         atomic_add(&m_backend->connections, 1);
         rval = true;
+
+        if (sescmd && sescmd->size())
+        {
+            append_session_command(*sescmd);
+
+            if (!execute_session_command())
+            {
+                rval = false;
+            }
+        }
     }
     else
     {

server/modules/routing/readwritesplit/readwritesplit.cc

@@ -441,8 +441,7 @@ static bool handle_error_new_connection(RWSplit *inst,
     }
     else
     {
-        succp = select_connect_backend_servers(myrses->rses_nbackends, max_nslaves,
+        succp = select_connect_backend_servers(inst, ses, myrses->backends,
-                                               ses, inst->config(), myrses->backends,
                                                myrses->current_master, &myrses->sescmd_list,
                                                &myrses->expected_responses,
                                                connection_type::SLAVE);
@@ -770,8 +769,7 @@ RWSplitSession* RWSplitSession::create(RWSplit* router, MXS_SESSION* session)
 
         SRWBackend master;
 
-        if (select_connect_backend_servers(router->service()->n_dbref, router->max_slave_count(),
+        if (select_connect_backend_servers(router, session, backends, master,
-                                           session, router->config(), backends, master,
                                            NULL, NULL, connection_type::ALL))
         {
             if ((rses = new RWSplitSession(router, session, backends, master)))

server/modules/routing/readwritesplit/rwsplit_internal.hh

@@ -93,15 +93,13 @@ enum connection_type
     SLAVE
 };
 
-bool select_connect_backend_servers(int router_nservers,
+bool select_connect_backend_servers(RWSplit *inst, MXS_SESSION *session,
-                                    int max_nslaves,
-                                    MXS_SESSION *session,
-                                    const Config& config,
                                     SRWBackendList& backends,
                                     SRWBackend& current_master,
                                     mxs::SessionCommandList* sescmd,
                                     int* expected_responses,
                                     connection_type type);
+SRWBackend get_root_master(const SRWBackendList& backends);
 /*
  * The following are implemented in rwsplit_tmp_table_multi.c
  */

server/modules/routing/readwritesplit/rwsplit_route_stmt.cc

@@ -35,8 +35,6 @@
 
 extern int (*criteria_cmpfun[LAST_CRITERIA])(const SRWBackend&, const SRWBackend&);
 
-static SRWBackend get_root_master_backend(RWSplitSession *rses);
-
 /**
  * Find out which of the two backend servers has smaller value for select
  * criteria property.
@@ -345,9 +343,6 @@ SRWBackend get_target_backend(RWSplitSession *rses, backend_type_t btype,
         return rses->target_node;
     }
 
-    /** get root master from available servers */
-    SRWBackend master = get_root_master_backend(rses);
-
     if (name) /*< Choose backend by name from a hint */
     {
         ss_dassert(btype != BE_MASTER); /*< Master dominates and no name should be passed with it */
@@ -462,34 +457,26 @@ SRWBackend get_target_backend(RWSplitSession *rses, backend_type_t btype,
      */
     else if (btype == BE_MASTER)
     {
-        if (master)
+        /** get root master from available servers */
-        {
+        SRWBackend master = get_root_master(rses->backends);
-            /** It is possible for the server status to change at any point in time
-             * so copying it locally will make possible error messages
-             * easier to understand */
-            SERVER server;
-            server.status = master->server()->status;
 
-            if (master->in_use())
+        if (master && master->in_use())
+        {
+            if (master->is_master())
             {
-                if (SERVER_IS_MASTER(&server))
+                rval = master;
-                {
-                    rval = master;
-                }
-                else
-                {
-                    MXS_ERROR("Server '%s' should be master but is %s instead "
-                              "and can't be chosen as the master.",
-                              master->name(),
-                              STRSRVSTATUS(&server));
-                }
             }
             else
             {
-                MXS_ERROR("Server '%s' is not in use and can't be chosen as the master.",
+                MXS_ERROR("Server '%s' does not have the master state and "
-                          master->name());
+                          "can't be chosen as the master.", master->name());
             }
         }
+        else
+        {
+            MXS_ERROR("Server '%s' is not in use and can't be chosen as the master.",
+                      master->name());
+        }
     }
 
     return rval;
@@ -943,52 +930,3 @@ bool handle_got_target(RWSplit *inst, RWSplitSession *rses,
         return false;
     }
 }
-
-/**
- * @brief Get the root master server from MySQL replication tree
- *
- * Finds the server with the lowest replication depth level which has the master
- * status. Servers are checked even if they are in 'maintenance'.
- *
- * @param rses Router client session
- *
- * @return The backend that points to the master server or an empty reference
- * if the master cannot be found
- */
-static SRWBackend get_root_master_backend(RWSplitSession *rses)
-{
-    SRWBackend candidate;
-    SERVER master = {};
-
-    for (SRWBackendList::iterator it = rses->backends.begin();
-         it != rses->backends.end(); it++)
-    {
-        SRWBackend& backend = *it;
-        if (backend->in_use())
-        {
-            if (backend == rses->current_master)
-            {
-                /** Store master state for better error reporting */
-                master.status = backend->server()->status;
-            }
-
-            if (backend->is_master())
-            {
-                if (!candidate ||
-                    (backend->server()->depth < candidate->server()->depth))
-                {
-                    candidate = backend;
-                }
-            }
-        }
-    }
-
-    if (!candidate && rses->rses_config.master_failure_mode == RW_FAIL_INSTANTLY &&
-        rses->current_master && rses->current_master->in_use())
-    {
-        MXS_ERROR("Could not find master among the backend servers. "
-                  "Previous master's state : %s", STRSRVSTATUS(&master));
-    }
-
-    return candidate;
-}

server/modules/routing/readwritesplit/rwsplit_select_backends.cc

@@ -36,10 +36,10 @@
  *
  * @return True if this server is a valid slave candidate
  */
-static bool valid_for_slave(const SRWBackend& backend, const SERVER_REF *master)
+static bool valid_for_slave(const SRWBackend& backend, const SRWBackend& master)
 {
     return (backend->is_slave() || backend->is_relay()) &&
-           (master == NULL || (backend->server() != master->server));
+           (!master || backend != master);
 }
 
 /**
@@ -54,7 +54,7 @@ static bool valid_for_slave(const SRWBackend& backend, const SERVER_REF *master)
  *
  * @return The best slave backend reference or NULL if no candidates could be found
  */
-static SRWBackend get_slave_candidate(const SRWBackendList& backends, const SERVER_REF *master,
+static SRWBackend get_slave_candidate(const SRWBackendList& backends, const SRWBackend& master,
                                       int (*cmpfun)(const SRWBackend&, const SRWBackend&))
 {
     SRWBackend candidate;
@@ -239,44 +239,25 @@ static void log_server_connections(select_criteria_t criteria, const SRWBackendL
         }
     }
 }
-/**
+
- * @brief Find the master server that is at the root of the replication tree
+SRWBackend get_root_master(const SRWBackendList& backends)
- *
- * @param rses Router client session
- *
- * @return The root master reference or NULL if no master is found
- */
-static SERVER_REF* get_root_master(const SRWBackendList& backends)
 {
-    SERVER_REF *master_host = NULL;
+    SRWBackend master;
 
-    for (SRWBackendList::const_iterator it = backends.begin();
+    for (auto it = backends.begin(); it != backends.end(); it++)
-         it != backends.end(); it++)
     {
-        SERVER_REF* b = (*it)->backend();
+        auto b = *it;
 
-        if (SERVER_IS_MASTER(b->server))
+        if (b->is_master() && (!master || b->server()->depth < master->server()->depth))
         {
-            if (master_host == NULL ||
+            master = b;
-                (b->server->depth < master_host->server->depth))
-            {
-                master_host = b;
-            }
         }
     }
 
-    return master_host;
+    return master;
 }
 
-/**
+std::pair<int, int> get_slave_counts(SRWBackendList& backends, SRWBackend& master)
- * Get the total number of slaves and number of connected slaves
- *
- * @param backends List of backend servers
- * @param master   The master server or NULL for no master
- *
- * @return the total number of slaves and the connected slave count
- */
-std::pair<int, int> get_slave_counts(SRWBackendList& backends, SERVER_REF* master)
 {
     int slaves_found = 0;
     int slaves_connected = 0;
@@ -301,55 +282,35 @@ std::pair<int, int> get_slave_counts(SRWBackendList& backends, SERVER_REF* maste
 }
 
 /**
- * @brief Search suitable backend servers from those of router instance
+ * Select and connect to backend servers
  *
- * It is assumed that there is only one master among servers of a router instance.
+ * @param inst               Router instance
- * As a result, the first master found is chosen. There will possibly be more
+ * @param session            Client session
- * backend references than connected backends because only those in correct state
+ * @param backends           List of backend servers
- * are connected to.
+ * @param current_master     The current master server
+ * @param sescmd_list        List of session commands to execute
+ * @param expected_responses Pointer where number of expected responses are written
+ * @param type               Connection type, ALL for all types, SLAVE for slaves only
  *
- * @param router_nservers Number of backend servers
+ * @return True if session can continue
- * @param max_nslaves     Upper limit for the number of slaves
- * @param select_criteria Slave selection criteria
- * @param session         Client session
- * @param router          Router instance
- * @param rses            Router client session
- * @param type            Connection type, ALL for all types, SLAVE for slaves only
- *
- * @return True if at least one master and one slave was found
  */
-bool select_connect_backend_servers(int router_nservers,
+bool select_connect_backend_servers(RWSplit *inst, MXS_SESSION *session,
-                                    int max_nslaves,
-                                    MXS_SESSION *session,
-                                    const Config& config,
                                     SRWBackendList& backends,
                                     SRWBackend& current_master,
                                     mxs::SessionCommandList* sescmd_list,
                                     int* expected_responses,
                                     connection_type type)
 {
-    SERVER_REF *master = get_root_master(backends);
+    SRWBackend master = get_root_master(backends);
 
-    if (!master && config.master_failure_mode == RW_FAIL_INSTANTLY)
+    if (!master && inst->config().master_failure_mode == RW_FAIL_INSTANTLY)
     {
-        MXS_ERROR("Couldn't find suitable Master from %d candidates.", router_nservers);
+        MXS_ERROR("Couldn't find suitable Master from %lu candidates.", backends.size());
         return false;
     }
 
-    /**
-     * New session:
-     *
-     * Connect to both master and slaves
-     *
-     * Existing session:
-     *
-     * Master is already connected or we don't have a master. The function was
-     * called because new slaves must be selected to replace failed ones.
-     */
-    bool master_connected = type == SLAVE || current_master;
-
     /** Check slave selection criteria and set compare function */
-    select_criteria_t select_criteria = config.slave_selection_criteria;
+    select_criteria_t select_criteria = inst->config().slave_selection_criteria;
     int (*cmpfun)(const SRWBackend&, const SRWBackend&) = criteria_cmpfun[select_criteria];
     ss_dassert(cmpfun);
 
@@ -358,14 +319,14 @@ bool select_connect_backend_servers(int router_nservers,
         log_server_connections(select_criteria, backends);
     }
 
-    if (!master_connected)
+    if (type == ALL)
     {
         /** Find a master server */
         for (SRWBackendList::const_iterator it = backends.begin(); it != backends.end(); it++)
         {
             const SRWBackend& backend = *it;
 
-            if (backend->can_connect() && master && backend->server() == master->server)
+            if (backend->can_connect() && master && backend == master)
             {
                 if (backend->connect(session))
                 {
@@ -379,6 +340,7 @@ bool select_connect_backend_servers(int router_nservers,
     auto counts = get_slave_counts(backends, master);
     int slaves_found = counts.first;
     int slaves_connected = counts.second;
+    int max_nslaves = inst->max_slave_count();
 
     ss_dassert(slaves_connected < max_nslaves || max_nslaves == 0);
 
@@ -387,25 +349,17 @@ bool select_connect_backend_servers(int router_nservers,
          backend && slaves_connected < max_nslaves;
          backend = get_slave_candidate(backends, master, cmpfun))
     {
-        if (backend->can_connect() && backend->connect(session))
+        if (backend->can_connect() && backend->connect(session, sescmd_list))
         {
             if (sescmd_list && sescmd_list->size())
             {
-                backend->append_session_command(*sescmd_list);
+                if (expected_responses)
-
-                if (backend->execute_session_command())
                 {
-                    if (expected_responses)
+                    (*expected_responses)++;
-                    {
-                        (*expected_responses)++;
-                    }
-                    slaves_connected++;
                 }
             }
-            else
+
-            {
+            slaves_connected++;
-                slaves_connected++;
-            }
         }
     }