Do configuration outside of constructors

Configuration errors can be resolved before the instance is created. This
avoids the unnecessary throws that were generated when an error occured.

As the configuration is stored in the router, the router sessions can use
a pointer to it instead of copying it locally. This should avoid some
unnecessary copying if more complex configuration parameters are added.

server/modules/routing/schemarouter/schemarouter.hh

@@ -20,30 +20,70 @@
 
 #include <maxscale/cdefs.h>
 
+#include <limits>
+#include <set>
+#include <string>
+
+#include <maxscale/pcre2.h>
+
+using std::set;
+using std::string;
+
+namespace schemarouter
+{
 /**
  * Configuration values
  */
-typedef struct schemarouter_config_st
+struct Config
 {
-    double refresh_min_interval; /**< Minimum required interval between refreshes of databases */
+    double            refresh_min_interval; /**< Minimum required interval between
-    bool   refresh_databases;    /**< Are databases refreshed when they are not found in the hashtable */
+                                             * refreshes of databases */
-    bool   debug;                /**< Enable verbose debug messages to clients */
+    bool              refresh_databases;    /**< Are databases refreshed when
-} schemarouter_config_t;
+                                             * they are not found in the hashtable */
+    bool              debug;                /**< Enable verbose debug messages to clients */
+    pcre2_code*       ignore_regex;         /**< Regular expression used to ignore databases */
+    pcre2_match_data* ignore_match_data;    /**< Match data for @c ignore_regex */
+    set<string>       ignored_dbs;          /**< Set of ignored databases */
+
+    Config():
+        refresh_min_interval(0.0),
+        refresh_databases(false),
+        debug(false),
+        ignore_regex(NULL),
+        ignore_match_data(NULL)
+    {
+    }
+};
 
 /**
- * The statistics for this router instance
+ * Router statistics
  */
-typedef struct
+struct Stats
 {
     int    n_queries;        /*< Number of queries forwarded    */
     int    n_sescmd;         /*< Number of session commands */
     int    longest_sescmd;   /*< Longest chain of stored session commands */
     int    n_hist_exceeded;  /*< Number of sessions that exceeded session
                               * command history limit */
-    int    sessions;
+    int    sessions;         /*< Number of sessions */
+    int    shmap_cache_hit;  /*< Shard map was found from the cache */
+    int    shmap_cache_miss; /*< No shard map found from the cache */
     double ses_longest;      /*< Longest session */
     double ses_shortest;     /*< Shortest session */
     double ses_average;      /*< Average session length */
-    int    shmap_cache_hit;  /*< Shard map was found from the cache */
+
-    int    shmap_cache_miss; /*< No shard map found from the cache */
+    Stats():
-} ROUTER_STATS;
+        n_queries(0),
+        n_sescmd(0),
+        longest_sescmd(0),
+        n_hist_exceeded(0),
+        sessions(0),
+        shmap_cache_hit(0),
+        shmap_cache_miss(0),
+        ses_longest(0.0),
+        ses_shortest(std::numeric_limits<double>::max()),
+        ses_average(0.0)
+    {
+    }
+};
+}

server/modules/routing/schemarouter/schemarouterinstance.cc

@@ -40,39 +40,45 @@ using std::map;
  * @file schemarouter.c The entry points for the simple sharding router module.
  */
 
-SchemaRouter::SchemaRouter(SERVICE *service, char **options):
+SchemaRouter::SchemaRouter(SERVICE *service, Config& config):
     mxs::Router<SchemaRouter, SchemaRouterSession>(service),
+    m_config(config),
     m_service(service)
 {
-    MXS_CONFIG_PARAMETER* conf;
-    MXS_CONFIG_PARAMETER* param;
-
-    /** Add default system databases to ignore */
-    m_ignored_dbs.insert("mysql");
-    m_ignored_dbs.insert("information_schema");
-    m_ignored_dbs.insert("performance_schema");
-
-    m_stats.longest_sescmd = 0;
-    m_stats.n_hist_exceeded = 0;
-    m_stats.n_queries = 0;
-    m_stats.n_sescmd = 0;
-    m_stats.ses_longest = 0;
-    m_stats.ses_shortest = (double)((unsigned long)(~0));
     spinlock_init(&m_lock);
+}
 
-    conf = service->svc_config_param;
+SchemaRouter::~SchemaRouter()
+{
+    if (m_config.ignore_regex)
+    {
+        pcre2_code_free(m_config.ignore_regex);
+    }
+}
 
-    m_config.refresh_databases = config_get_bool(conf, "refresh_databases");
+SchemaRouter* SchemaRouter::create(SERVICE* pService, char** pzOptions)
-    m_config.refresh_min_interval = config_get_integer(conf, "refresh_interval");
+{
-    m_config.debug = config_get_bool(conf, "debug");
+    MXS_CONFIG_PARAMETER* conf = pService->svc_config_param;
 
     if ((config_get_param(conf, "auth_all_servers")) == NULL)
     {
         MXS_NOTICE("Authentication data is fetched from all servers. To disable this "
                    "add 'auth_all_servers=0' to the service.");
-        service->users_from_all = true;
+        pService->users_from_all = true;
     }
 
+    Config config;
+    MXS_CONFIG_PARAMETER* param;
+
+    config.refresh_databases = config_get_bool(conf, "refresh_databases");
+    config.refresh_min_interval = config_get_integer(conf, "refresh_interval");
+    config.debug = config_get_bool(conf, "debug");
+
+    /** Add default system databases to ignore */
+    config.ignored_dbs.insert("mysql");
+    config.ignored_dbs.insert("information_schema");
+    config.ignored_dbs.insert("performance_schema");
+
     if ((param = config_get_param(conf, "ignore_databases_regex")))
     {
         int errcode;
@@ -86,7 +92,7 @@ SchemaRouter::SchemaRouter(SERVICE *service, char **options):
             pcre2_get_error_message(errcode, errbuf, sizeof(errbuf));
             MXS_ERROR("Regex compilation failed at %d for regex '%s': %s",
                       (int)erroffset, param->value, errbuf);
-            throw std::runtime_error("Regex compilation failed");
+            return NULL;
         }
 
         pcre2_match_data* match_data = pcre2_match_data_create_from_pattern(re, NULL);
@@ -94,11 +100,11 @@ SchemaRouter::SchemaRouter(SERVICE *service, char **options):
         if (match_data == NULL)
         {
             pcre2_code_free(re);
-            throw std::bad_alloc();
+            return NULL;
         }
 
-        m_ignore_regex = re;
+        config.ignore_regex = re;
-        m_ignore_match_data = match_data;
+        config.ignore_match_data = match_data;
     }
 
     if ((param = config_get_param(conf, "ignore_databases")))
@@ -112,77 +118,56 @@ SchemaRouter::SchemaRouter(SERVICE *service, char **options):
 
         while (tok)
         {
-            m_ignored_dbs.insert(tok);
+            config.ignored_dbs.insert(tok);
             tok = strtok_r(NULL, sep, &sptr);
         }
     }
 
-    bool failure = false;
+    bool success = true;
 
-    for (int i = 0; options && options[i]; i++)
+    for (int i = 0; pzOptions && pzOptions[i]; i++)
     {
-        char* value = strchr(options[i], '=');
+        char* value = strchr(pzOptions[i], '=');
 
         if (value == NULL)
         {
-            MXS_ERROR("Unknown router options for %s", options[i]);
+            MXS_ERROR("Unknown router options for %s", pzOptions[i]);
-            failure = true;
+            success = false;
             break;
         }
 
         *value = '\0';
         value++;
 
-        if (strcmp(options[i], "max_sescmd_history") == 0)
+        if (strcmp(pzOptions[i], "max_sescmd_history") == 0)
         {
             MXS_WARNING("Use of 'max_sescmd_history' is deprecated");
         }
-        else if (strcmp(options[i], "disable_sescmd_history") == 0)
+        else if (strcmp(pzOptions[i], "disable_sescmd_history") == 0)
         {
             MXS_WARNING("Use of 'disable_sescmd_history' is deprecated");
         }
-        else if (strcmp(options[i], "refresh_databases") == 0)
+        else if (strcmp(pzOptions[i], "refresh_databases") == 0)
         {
-            m_config.refresh_databases = config_truth_value(value);
+            config.refresh_databases = config_truth_value(value);
         }
-        else if (strcmp(options[i], "refresh_interval") == 0)
+        else if (strcmp(pzOptions[i], "refresh_interval") == 0)
         {
-            m_config.refresh_min_interval = atof(value);
+            config.refresh_min_interval = atof(value);
         }
-        else if (strcmp(options[i], "debug") == 0)
+        else if (strcmp(pzOptions[i], "debug") == 0)
         {
-            m_config.debug = config_truth_value(value);
+            config.debug = config_truth_value(value);
         }
         else
         {
-            MXS_ERROR("Unknown router options for %s", options[i]);
+            MXS_ERROR("Unknown router options for %s", pzOptions[i]);
-            failure = true;
+            success = false;
             break;
         }
     }
 
-    if (failure)
+    return success ? new SchemaRouter(pService, config) : NULL;
-    {
-        throw std::runtime_error("Failed to create schemarouter instance.");
-    }
-}
-
-SchemaRouter::~SchemaRouter()
-{
-    if (m_ignore_regex)
-    {
-        pcre2_code_free(m_ignore_regex);
-    }
-
-    if (m_ignore_match_data)
-    {
-        pcre2_match_data_free(m_ignore_match_data);
-    }
-}
-
-SchemaRouter* SchemaRouter::create(SERVICE* pService, char** pzOptions)
-{
-    return new SchemaRouter(pService, pzOptions);
 }
 
 SchemaRouterSession* SchemaRouter::newSession(MXS_SESSION* pSession)

server/modules/routing/schemarouter/schemarouterinstance.hh

@@ -24,6 +24,8 @@
 
 using std::string;
 using std::set;
+using schemarouter::Config;
+using schemarouter::Stats;
 
 class SchemaRouterSession;
 
@@ -43,19 +45,12 @@ private:
     friend class SchemaRouterSession;
 
     /** Internal functions */
-    SchemaRouter(SERVICE *service, char **options);
+    SchemaRouter(SERVICE *service, Config& config);
 
     /** Member variables */
-    schemarouter_config_t m_config;        /*< expanded config info from SERVICE */
+    Config                m_config;        /*< expanded config info from SERVICE */
     ShardManager          m_shard_manager; /*< Shard maps hashed by user name */
     SERVICE*              m_service;       /*< Pointer to service */
     SPINLOCK              m_lock;          /*< Lock for the instance data */
-    ROUTER_STATS          m_stats;         /*< Statistics for this router */
+    Stats                 m_stats;         /*< Statistics for this router */
-    set<string>           m_ignored_dbs;   /*< List of databases to ignore when the
-                                            * database mapping finds multiple servers
-                                            * with the same database */
-    pcre2_code*           m_ignore_regex;  /*< Databases matching this regex will
-                                            * not cause the session to be terminated
-                                            * if they are found on more than one server. */
-    pcre2_match_data*     m_ignore_match_data;
 };

server/modules/routing/schemarouter/schemaroutersession.cc

@@ -42,10 +42,10 @@ SchemaRouterSession::SchemaRouterSession(MXS_SESSION* session, SchemaRouter* rou
     m_client(session->client_dcb),
     m_mysql_session((MYSQL_session*)session->client_dcb->data),
     m_backends(NULL),
-    m_config(m_router->m_config),
+    m_config(&m_router->m_config),
     m_backend_count(0),
     m_router(router),
-    m_shard(m_router->m_shard_manager.get_shard(m_client->user, m_config.refresh_min_interval)),
+    m_shard(m_router->m_shard_manager.get_shard(m_client->user, m_config->refresh_min_interval)),
     m_state(0),
     m_sent_sescmd(0),
     m_replied_sescmd(0)
@@ -107,6 +107,7 @@ SchemaRouterSession::SchemaRouterSession(MXS_SESSION* session, SchemaRouter* rou
 
     if (!connect_backend_servers(backend_ref, router_nservers, session))
     {
+        // TODO: Figure out how to avoid this throw
         throw std::runtime_error("Failed to connect to backend servers");
     }
 
@@ -395,7 +396,7 @@ int32_t SchemaRouterSession::routeQuery(GWBUF* pPacket)
             char errbuf[128 + MYSQL_DATABASE_MAXLEN];
             snprintf(errbuf, sizeof(errbuf), "Unknown database: %s", db);
 
-            if (m_config.debug)
+            if (m_config->debug)
             {
                 sprintf(errbuf + strlen(errbuf),
                         " ([%lu]: DB change failed)",
@@ -1148,7 +1149,7 @@ bool SchemaRouterSession::handle_default_db()
         MXS_INFO("Connecting to a non-existent database '%s'", m_connect_db.c_str());
         char errmsg[128 + MYSQL_DATABASE_MAXLEN + 1];
         sprintf(errmsg, "Unknown database '%s'", m_connect_db.c_str());
-        if (m_config.debug)
+        if (m_config->debug)
         {
             sprintf(errmsg + strlen(errmsg), " ([%lu]: DB not found on connect)",
                     m_client->session->ses_id);

server/modules/routing/schemarouter/schemaroutersession.hh

@@ -25,6 +25,8 @@
 
 using std::string;
 using std::list;
+using schemarouter::Config;
+using schemarouter::Stats;
 
 /**
  * Bitmask values for the router session's initialization. These values are used
@@ -186,7 +188,7 @@ private:
     DCB*                  m_client;         /**< The client DCB */
     MYSQL_session*        m_mysql_session;  /**< Session client data (username, password, SHA1). */
     backend_ref_t*        m_backends;       /**< Pointer to backend reference array */
-    schemarouter_config_t m_config;         /**< Copied config info from router instance */
+    Config*               m_config;         /**< Pointer to router config */
     int                   m_backend_count;  /**< Number of backends */
     SchemaRouter*         m_router;         /**< The router instance */
     Shard                 m_shard;          /**< Database to server mapping */
@@ -194,7 +196,7 @@ private:
     string                m_current_db;     /**< Current active database */
     int                   m_state;          /**< Initialization state bitmask */
     list<Buffer>          m_queue;          /**< Query that was received before the session was ready */
-    ROUTER_STATS          m_stats;          /**< Statistics for this router */
+    Stats                 m_stats;          /**< Statistics for this router */
     uint64_t              m_sent_sescmd;    /**< The latest session command being executed */
     uint64_t              m_replied_sescmd; /**< The last session command reply that was sent to the client */
 };