From 17fc2ba88a26133a950ce0b421e2d650f2c67be3 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Mon, 14 Jan 2019 12:54:19 +0200 Subject: [PATCH] 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 | 89 +++++++++-- maxutils/maxsql/src/mariadb.cc | 140 ++++++++++++++++-- .../monitor/mariadbmon/mariadbserver.cc | 117 +++++++++------ 3 files changed, 281 insertions(+), 65 deletions(-) diff --git a/maxutils/maxsql/include/maxsql/mariadb.hh b/maxutils/maxsql/include/maxsql/mariadb.hh index 44f243ac8..e7628f80e 100644 --- a/maxutils/maxsql/include/maxsql/mariadb.hh +++ b/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 - * could not be parsed or the result was negative. + * @return Value as integer. If the data could not be parsed an error flag is set. */ - 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 m_col_indexes; // Map of column name -> index - MYSQL_ROW m_rowdata = NULL; // Data for current row - int64_t m_current_row_ind = -1;// Index of current row + + class ConversionError + { + 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 m_col_indexes; /**< Map of column name -> index */ }; } diff --git a/maxutils/maxsql/src/mariadb.cc b/maxutils/maxsql/src/mariadb.cc index 12d3d3f59..dc69c2cd1 100644 --- a/maxutils/maxsql/src/mariadb.cc +++ b/maxutils/maxsql/src/mariadb.cc @@ -16,6 +16,7 @@ #include #include #include +#include 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 = -1; - if (data && *data) + int64_t rval = 0; + char* data_elem = m_rowdata[column_ind]; + if (data_elem == nullptr) { - errno = 0; // strtoll sets this - char* endptr = NULL; - auto parsed = strtoll(data, &endptr, 10); - if (parsed >= 0 && errno == 0 && *endptr == '\0') + set_error(column_ind, target_type); + } + else + { + 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 data ? (strcmp(data, "Y") == 0 || strcmp(data, "1") == 0) : false; + return m_rowdata[column_ind] == nullptr; +} + +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(); } } diff --git a/server/modules/monitor/mariadbmon/mariadbserver.cc b/server/modules/monitor/mariadbmon/mariadbserver.cc index e2cbf3772..3b5cfc640 100644 --- a/server/modules/monitor/mariadbmon/mariadbserver.cc +++ b/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 -> -1. - new_row.seconds_behind_master = (rlag < 0) ? SERVER::RLAG_UNDEFINED : - (rlag > INT_MAX) ? INT_MAX : rlag; + // If slave connection is stopped, the value given by the backend is null. + if (result->field_is_null(i_seconds_behind_master)) + { + 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 (!sstatus_array_topology_equal(slave_status_new)) + if (!parse_error) { - 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, - // gtid:s etc may have changed. - m_slave_status = std::move(slave_status_new); - return true; + return !parse_error; } 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; - 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. + if (!result->next_row()) { - server_id_parsed = SERVER_ID_UNKNOWN; - rval = false; - } - 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; + // This should not really happen, means that server is buggy. + *errmsg_out = string_printf("Query '%s' did not return any rows.", query.c_str()); } 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;