Store error status into QueryResult object

The QueryResult-object remembers if a conversion failed. This makes checking
for errors more convenient, as just one check per row is required. The conversion
functions always return a valid value.

maxutils/maxsql/include/maxsql/mariadb.hh

@@ -117,26 +117,95 @@ public:
     std::string get_string(int64_t column_ind) const;
 
     /**
-     * Read a non-negative integer value from the current row and given column.
+     * Read an integer value from the current row and given column.
      *
      * @param column_ind Column index
-     * @return Value as integer. 0 or greater indicates success, -1 is returned if the data
+     * @return Value as integer. If the data could not be parsed an error flag is set.
-     * could not be parsed or the result was negative.
      */
-    int64_t get_uint(int64_t column_ind) const;
+    int64_t get_int(int64_t column_ind) const;
 
     /**
-     * Read a boolean value from the current row and given column.
+     * Check if field is null.
      *
      * @param column_ind Column index
-     * @return Value as boolean. Returns true if the text is either 'Y' or '1'.
+     * @return True if null
+     */
+    bool field_is_null(int64_t column_ind) const;
+
+    /**
+     * Read a boolean-like value from the current row and given column. The function expects the field to
+     * be either 1 or 0, any other value is an error.
+     *
+     * @param column_ind Column index
+     * @return Value as boolean. Returns true if the field contains '1'.
      */
     bool get_bool(int64_t column_ind) const;
 
+    /**
+     * Has a parsing error occurred during current row?
+     *
+     * @return True if parsing failed.
+     */
+    bool error() const;
+
+    /**
+     * Return error string.
+     *
+     * @return Error string
+     */
+    std::string error_string() const;
+
 private:
-    MYSQL_RES*                               m_resultset = NULL;    // Underlying result set, freed at dtor.
+
-    std::unordered_map<std::string, int64_t> m_col_indexes;         // Map of column name -> index
+    class ConversionError
-    MYSQL_ROW                                m_rowdata = NULL;      // Data for current row
+    {
-    int64_t                                  m_current_row_ind = -1;// Index of current row
+    public:
+
+        /**
+         * Is error set?
+         *
+         * @return True if set
+         */
+        bool error() const;
+
+        /**
+         * Set an error describing an invalid conversion. The error is only set if the error is
+         * currently empty.
+         *
+         * @param field_value The value of the accessed field
+         * @param target_type Conversion target datatype
+         */
+        void set_value_error(const std::string& field_value, const std::string& target_type);
+
+        /**
+         * Set an error describing a conversion from a null value. The error is only set if the error is
+         * currently empty.
+         *
+         * @param target_type Conversion target datatype
+         */
+        void set_null_value_error(const std::string& target_type);
+
+        /**
+         * Print error information to string.
+         *
+         * @return Error description, or empty if no error
+         */
+        std::string to_string() const;
+
+    private:
+        bool        m_field_was_null = false;   /**< Was the converted field null? */
+        std::string m_field_value;              /**< The value in the field if it was not null */
+        std::string m_target_type;              /**< The conversion target type */
+    };
+
+    int64_t parse_integer(int64_t column_ind, const std::string& target_type) const;
+    void    set_error(int64_t column_ind, const std::string& target_type) const;
+
+    MYSQL_RES* m_resultset = nullptr;   /**< Underlying result set, freed at dtor */
+    MYSQL_ROW  m_rowdata = nullptr;     /**< Data for current row */
+    int64_t    m_current_row_ind = -1;  /**< Index of current row */
+
+    mutable ConversionError                  m_error;       /**< Error information */
+    std::unordered_map<std::string, int64_t> m_col_indexes; /**< Map of column name -> index */
 };
 }

maxutils/maxsql/src/mariadb.cc

@@ -16,6 +16,7 @@
 #include <string>
 #include <string.h>
 #include <maxbase/assert.h>
+#include <maxbase/format.hh>
 
 using std::string;
 
@@ -116,6 +117,7 @@ bool QueryResult::next_row()
     if (m_rowdata)
     {
         m_current_row_ind++;
+        m_error = ConversionError(); // Reset error
         return true;
     }
     else
@@ -153,29 +155,147 @@ string QueryResult::get_string(int64_t column_ind) const
     return data ? data : "";
 }
 
-int64_t QueryResult::get_uint(int64_t column_ind) const
+int64_t QueryResult::get_int(int64_t column_ind) const
+{
+    return parse_integer(column_ind, "integer");
+}
+
+/**
+ * Parse a 64bit integer. On parse error an error flag is set.
+ *
+ * @param column_ind Column index
+ * @param target_type The final conversion target type.
+ * @return Conversion result
+ */
+int64_t QueryResult::parse_integer(int64_t column_ind, const std::string& target_type) const
 {
     mxb_assert(column_ind < get_col_count() && column_ind >= 0 && m_rowdata);
-    char* data = m_rowdata[column_ind];
+    int64_t rval = 0;
-    int64_t rval = -1;
+    char* data_elem = m_rowdata[column_ind];
-    if (data && *data)
+    if (data_elem == nullptr)
     {
-        errno = 0;      // strtoll sets this
+        set_error(column_ind, target_type);
-        char* endptr = NULL;
+    }
-        auto parsed = strtoll(data, &endptr, 10);
+    else
-        if (parsed >= 0 && errno == 0 && *endptr == '\0')
+    {
+        errno = 0;
+        char* endptr = nullptr;
+        auto parsed = strtoll(data_elem, &endptr, 10);
+        if (errno == 0 && *endptr == '\0')
         {
             rval = parsed;
         }
+        else
+        {
+            set_error(column_ind, target_type);
+        }
     }
     return rval;
 }
 
 bool QueryResult::get_bool(int64_t column_ind) const
+{
+    const string target_type = "boolean";
+    bool rval = false;
+    auto val = parse_integer(column_ind, target_type);
+    if (!error())
+    {
+        if (val < 0 || val > 1)
+        {
+            set_error(column_ind, target_type);
+        }
+        else
+        {
+            rval = (val == 1);
+        }
+    }
+    return rval;
+}
+
+bool QueryResult::field_is_null(int64_t column_ind) const
 {
     mxb_assert(column_ind < get_col_count() && column_ind >= 0 && m_rowdata);
-    char* data = m_rowdata[column_ind];
+    return m_rowdata[column_ind] == nullptr;
-    return data ? (strcmp(data, "Y") == 0 || strcmp(data, "1") == 0) : false;
+}
+
+void QueryResult::set_error(int64_t column_ind, const string& target_type) const
+{
+    string col_name;
+    // Find the column name.
+    for (const auto& elem : m_col_indexes)
+    {
+        if (elem.second == column_ind)
+        {
+            col_name = elem.first;
+            break;
+        }
+    }
+
+    mxb_assert(!col_name.empty());
+    // If the field value is null, then that is the cause of the error.
+    char* field_value = m_rowdata[column_ind];
+    if (field_value == nullptr)
+    {
+        m_error.set_null_value_error(target_type);
+    }
+    else
+    {
+        m_error.set_value_error(field_value, target_type);
+    }
+}
+
+bool QueryResult::error() const
+{
+    return m_error.error();
+}
+
+string QueryResult::error_string() const
+{
+    return m_error.to_string();
+}
+
+void QueryResult::ConversionError::set_value_error(const string& field_value, const string& target_type)
+{
+    mxb_assert(!target_type.empty());
+    // The error container only has space for one error.
+    if (m_target_type.empty())
+    {
+        m_field_value = field_value;
+        m_target_type = target_type;
+    }
+}
+
+void QueryResult::ConversionError::set_null_value_error(const string& target_type)
+{
+    mxb_assert(!target_type.empty());
+    if (m_target_type.empty())
+    {
+        m_field_was_null = true;
+        m_target_type = target_type;
+    }
+}
+
+string QueryResult::ConversionError::to_string() const
+{
+    string rval;
+    if (!m_target_type.empty())
+    {
+        rval = "Cannot convert ";
+        if (m_field_was_null)
+        {
+            rval += mxb::string_printf("a null field to %s.", m_target_type.c_str());
+        }
+        else
+        {
+            rval += mxb::string_printf("field '%s' to %s.", m_field_value.c_str(), m_target_type.c_str());
+        }
+    }
+    return rval;
+}
+
+bool QueryResult::ConversionError::error() const
+{
+    return !m_target_type.empty();
 }
 
 }

server/modules/monitor/mariadbmon/mariadbserver.cc

@@ -302,12 +302,13 @@ bool MariaDBServer::do_show_slave_status(string* errmsg_out)
     }
 
     SlaveStatusArray slave_status_new;
+    bool parse_error = false;
     while (result->next_row())
     {
         SlaveStatus new_row;
         new_row.owning_server = name();
         new_row.master_host = result->get_string(i_master_host);
-        new_row.master_port = result->get_uint(i_master_port);
+        new_row.master_port = result->get_int(i_master_port);
         string last_io_error = result->get_string(i_last_io_error);
         string last_sql_error = result->get_string(i_last_sql_error);
         new_row.last_error = !last_io_error.empty() ? last_io_error : last_sql_error;
@@ -315,17 +316,24 @@ bool MariaDBServer::do_show_slave_status(string* errmsg_out)
         new_row.slave_io_running =
             SlaveStatus::slave_io_from_string(result->get_string(i_slave_io_running));
         new_row.slave_sql_running = (result->get_string(i_slave_sql_running) == "Yes");
-        new_row.master_server_id = result->get_uint(i_master_server_id);
+        new_row.master_server_id = result->get_int(i_master_server_id);
 
-        auto rlag = result->get_uint(i_seconds_behind_master);
+        // If slave connection is stopped, the value given by the backend is null.
-        // If slave connection is stopped, the value given by the backend is null -> -1.
+        if (result->field_is_null(i_seconds_behind_master))
-        new_row.seconds_behind_master = (rlag < 0) ? SERVER::RLAG_UNDEFINED :
+        {
-            (rlag > INT_MAX) ? INT_MAX : rlag;
+            new_row.seconds_behind_master = SERVER::RLAG_UNDEFINED;
+        }
+        else
+        {
+            // Seconds_Behind_Master is actually uint64, but it will take a long time until it goes over
+            // int64 limit.
+            new_row.seconds_behind_master = result->get_int(i_seconds_behind_master);
+        }
 
         if (all_slaves_status)
         {
             new_row.name = result->get_string(i_connection_name);
-            new_row.received_heartbeats = result->get_uint(i_slave_rec_hbs);
+            new_row.received_heartbeats = result->get_int(i_slave_rec_hbs);
 
             string using_gtid = result->get_string(i_using_gtid);
             string gtid_io_pos = result->get_string(i_gtid_io_pos);
@@ -335,6 +343,14 @@ bool MariaDBServer::do_show_slave_status(string* errmsg_out)
             }
         }
 
+        // If parsing fails, discard all query results.
+        if (result->error())
+        {
+            parse_error = true;
+            MXB_ERROR("Query '%s' returned invalid data: %s", query.c_str(), result->error_string().c_str());
+            break;
+        }
+
         // Before adding this row to the SlaveStatus array, compare the row to the one from the previous
         // monitor tick and fill in the last pieces of data.
         auto old_row = sstatus_find_previous_row(new_row, slave_status_new.size());
@@ -370,16 +386,20 @@ bool MariaDBServer::do_show_slave_status(string* errmsg_out)
         slave_status_new.push_back(new_row);
     }
 
-    // Compare the previous array to the new one.
+    if (!parse_error)
-    if (!sstatus_array_topology_equal(slave_status_new))
     {
-        m_topology_changed = true;
+        // Compare the previous array to the new one.
+        if (!sstatus_array_topology_equal(slave_status_new))
+        {
+            m_topology_changed = true;
+        }
+
+        // Always write to m_slave_status. Even if the new status is equal by topology,
+        // gtid:s etc may have changed.
+        m_slave_status = std::move(slave_status_new);
     }
 
-    // Always write to m_slave_status. Even if the new status is equal by topology,
+    return !parse_error;
-    // gtid:s etc may have changed.
-    m_slave_status = std::move(slave_status_new);
-    return true;
 }
 
 bool MariaDBServer::update_gtids(string* errmsg_out)
@@ -454,42 +474,49 @@ bool MariaDBServer::read_server_variables(string* errmsg_out)
     int i_domain = 2;
     bool rval = false;
     auto result = execute_query(query, errmsg_out);
-    if (result.get() != NULL && result->next_row())
+    if (result != nullptr)
     {
-        rval = true;
+        if (!result->next_row())
-        int64_t server_id_parsed = result->get_uint(i_id);
-        if (server_id_parsed < 0)   // This is very unlikely, requiring an error in server or connector.
         {
-            server_id_parsed = SERVER_ID_UNKNOWN;
+            // This should not really happen, means that server is buggy.
-            rval = false;
+            *errmsg_out = string_printf("Query '%s' did not return any rows.", query.c_str());
-        }
-        if (server_id_parsed != m_server_id)
-        {
-            m_server_id = server_id_parsed;
-            m_topology_changed = true;
-        }
-        m_server_base->server->node_id = server_id_parsed;
-
-        bool read_only_parsed = result->get_bool(i_ro);
-        if (read_only_parsed != m_read_only)
-        {
-            m_read_only = read_only_parsed;
-            m_topology_changed = true;
-        }
-
-        if (use_gtid)
-        {
-            int64_t domain_id_parsed = result->get_uint(i_domain);
-            if (domain_id_parsed < 0)   // Same here.
-            {
-                domain_id_parsed = GTID_DOMAIN_UNKNOWN;
-                rval = false;
-            }
-            m_gtid_domain_id = domain_id_parsed;
         }
         else
         {
-            m_gtid_domain_id = GTID_DOMAIN_UNKNOWN;
+            int64_t server_id_parsed = result->get_int(i_id);
+            bool read_only_parsed = result->get_bool(i_ro);
+            int64_t domain_id_parsed = GTID_DOMAIN_UNKNOWN;
+            if (use_gtid)
+            {
+                domain_id_parsed = result->get_int(i_domain);
+            }
+
+            if (result->error())
+            {
+                // This is unlikely as well.
+                *errmsg_out = string_printf("Query '%s' returned invalid data: %s",
+                                            query.c_str(), result->error_string().c_str());
+            }
+            else
+            {
+                // All values parsed and within expected limits.
+                rval = true;
+                if (server_id_parsed != m_server_id)
+                {
+                    m_server_id = server_id_parsed;
+                    m_topology_changed = true;
+                }
+                m_server_base->server->node_id = server_id_parsed;
+
+                if (read_only_parsed != m_read_only)
+                {
+                    m_read_only = read_only_parsed;
+                    m_topology_changed = true;
+                }
+
+                m_gtid_domain_id = domain_id_parsed;
+            }
+
         }
     }
     return rval;