From c893e354a96b421e346808a7975a26f5198c7479 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 23 Jan 2018 08:37:51 +0200 Subject: [PATCH 01/54] Add missing variable The numlocks variable is used when older OpenSSL versions are used. --- server/core/gateway.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/server/core/gateway.cc b/server/core/gateway.cc index d7d726711..169595442 100644 --- a/server/core/gateway.cc +++ b/server/core/gateway.cc @@ -1362,6 +1362,7 @@ int main(int argc, char **argv) sigset_t saved_mask; bool to_stdout = false; void (*exitfunp[4])(void) = { mxs_log_finish, cleanup_process_datadir, write_footer, NULL }; + int numlocks = 0; bool pid_file_created = false; // NOTE: These are set here since global_defaults() is called inside config_load(). From c85f83fa2b1e9d5b51c062dd3427d0a1d58dbffd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 23 Jan 2018 09:26:22 +0200 Subject: [PATCH 02/54] Fix strcpy overlap in binlogrouter The source and destination buffers could overlap which is why an intermediate buffer is required. --- server/modules/routing/binlogrouter/blr_file.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/server/modules/routing/binlogrouter/blr_file.c b/server/modules/routing/binlogrouter/blr_file.c index 3950faf83..746cb98a6 100644 --- a/server/modules/routing/binlogrouter/blr_file.c +++ b/server/modules/routing/binlogrouter/blr_file.c @@ -421,7 +421,12 @@ blr_file_create(ROUTER_INSTANCE *router, char *file) { close(router->binlog_fd); spinlock_acquire(&router->binlog_lock); - strcpy(router->binlog_name, file); + + /// Use an intermediate buffer in case the source and destination overlap + char new_binlog[strlen(file) + 1]; + strcpy(new_binlog, file); + strcpy(router->binlog_name, new_binlog); + router->binlog_fd = fd; router->current_pos = BINLOG_MAGIC_SIZE; /* Initial position after the magic number */ router->binlog_position = BINLOG_MAGIC_SIZE; From 653b8429d492e812023b70edc983110373a401a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 23 Jan 2018 10:46:26 +0200 Subject: [PATCH 03/54] Extend cdc_datatypes test The test now also checks DATE, DATETIME and TIMESTAMP types. --- .../cdc_datatypes/cdc_datatypes.cpp | 41 +++++++++++++++++-- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp b/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp index f9a29f379..5695d6d7a 100644 --- a/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp +++ b/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp @@ -36,6 +36,7 @@ static const char* decimal_types[] = "FLOAT", "DOUBLE", "DECIMAL(10, 2)", + "DECIMAL(32, 2)", NULL }; @@ -89,16 +90,48 @@ static const char* binary_values[] = NULL }; +static const char* datetime_types[] = +{ + "DATETIME(1)", + "DATETIME(2)", + "DATETIME(3)", + "DATETIME(4)", + "DATETIME(5)", + "DATETIME(6)", + "TIMESTAMP", + NULL +}; + +static const char* datetime_values[] = +{ + "'2018-01-01 11:11:11'", + NULL +}; + +static const char* date_types[] = +{ + "DATE", + NULL +}; + +static const char* date_values[] = +{ + "'2018-01-01'", + NULL +}; + struct { const char** types; const char** values; } test_set[] { - { integer_types, integer_values }, - { decimal_types, decimal_values }, - { string_types, string_values }, - { binary_types, binary_values }, + { integer_types, integer_values }, + { decimal_types, decimal_values }, + { string_types, string_values }, + { binary_types, binary_values }, + { datetime_types, datetime_values }, + { date_types, date_values }, { 0 } }; From 4e9a5af9267fff0cdac4aaf4bdfb56678cf2ecbc Mon Sep 17 00:00:00 2001 From: Timofey Turenko Date: Fri, 26 Jan 2018 09:23:32 +0200 Subject: [PATCH 04/54] add execution of add_core_conf.sh to 'check_backend' to set up core dump saving --- maxscale-system-test/check_backend.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/maxscale-system-test/check_backend.cpp b/maxscale-system-test/check_backend.cpp index fb045c532..fcdb3926a 100644 --- a/maxscale-system-test/check_backend.cpp +++ b/maxscale-system-test/check_backend.cpp @@ -8,8 +8,21 @@ int main(int argc, char *argv[]) { + char src[1024]; + char dst[1024]; + TestConnections * Test = new TestConnections(argc, argv); + for (int i = 0; i < Test->maxscales->N; i++) + { + sprintf(src, "%s/mdbci/add_core_cnf.sh", test_dir); + Test->maxscales->ssh_node_f(i, false, "mkdir %s/ccore", Test->maxscales->access_homedir[i]); + sprintf(dst, "%s/ccore/", Test->maxscales->access_homedir[i]); + Test->maxscales->copy_to_node(i, src, dst); + sprintf(dst, "%s/ccore/", Test->maxscales->access_homedir[i]); + Test->maxscales->ssh_node_f(i, true, "%s/ccore/add_core_cnf.sh", Test->maxscales->access_homedir[i]); + } + /*Test->restart_maxscale(); sleep(5);*/ Test->set_timeout(10); From 3da4aa166515a84ab42aa01911781dae3baa6412 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Jan 2018 10:23:05 +0200 Subject: [PATCH 05/54] Remove trailing whitespace in check_backend Removed trailing whitespace in check_backend --- maxscale-system-test/check_backend.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maxscale-system-test/check_backend.cpp b/maxscale-system-test/check_backend.cpp index fcdb3926a..83579ae48 100644 --- a/maxscale-system-test/check_backend.cpp +++ b/maxscale-system-test/check_backend.cpp @@ -10,7 +10,7 @@ int main(int argc, char *argv[]) { char src[1024]; char dst[1024]; - + TestConnections * Test = new TestConnections(argc, argv); for (int i = 0; i < Test->maxscales->N; i++) From 4dc9b56d290d0ee6e25ba8c9b470efe8f8b8d386 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Jan 2018 10:41:34 +0200 Subject: [PATCH 06/54] Fix check_backend Fixed the core generation and removed obsolete files. --- maxscale-system-test/check_backend.cpp | 17 ++++------------- maxscale-system-test/mdbci/add_core_cnf.sh | 6 +++++- maxscale-system-test/mdbci/configure_core.sh | 11 ----------- 3 files changed, 9 insertions(+), 25 deletions(-) delete mode 100755 maxscale-system-test/mdbci/configure_core.sh diff --git a/maxscale-system-test/check_backend.cpp b/maxscale-system-test/check_backend.cpp index 83579ae48..e84c1ae82 100644 --- a/maxscale-system-test/check_backend.cpp +++ b/maxscale-system-test/check_backend.cpp @@ -8,23 +8,14 @@ int main(int argc, char *argv[]) { - char src[1024]; - char dst[1024]; TestConnections * Test = new TestConnections(argc, argv); - for (int i = 0; i < Test->maxscales->N; i++) - { - sprintf(src, "%s/mdbci/add_core_cnf.sh", test_dir); - Test->maxscales->ssh_node_f(i, false, "mkdir %s/ccore", Test->maxscales->access_homedir[i]); - sprintf(dst, "%s/ccore/", Test->maxscales->access_homedir[i]); - Test->maxscales->copy_to_node(i, src, dst); - sprintf(dst, "%s/ccore/", Test->maxscales->access_homedir[i]); - Test->maxscales->ssh_node_f(i, true, "%s/ccore/add_core_cnf.sh", Test->maxscales->access_homedir[i]); - } + std::string src = std::string(test_dir) + "/mdbci/add_core_cnf.sh"; + Test->copy_to_maxscale(src.c_str(), Test->maxscale_access_homedir); + Test->ssh_maxscale(true, "%s/add_core_cnf.sh %s", Test->maxscale_access_homedir, + Test->verbose ? "verbose" : ""); - /*Test->restart_maxscale(); - sleep(5);*/ Test->set_timeout(10); Test->tprintf("Connecting to Maxscale routers with Master/Slave backend\n"); diff --git a/maxscale-system-test/mdbci/add_core_cnf.sh b/maxscale-system-test/mdbci/add_core_cnf.sh index aa5db1828..21a69f460 100755 --- a/maxscale-system-test/mdbci/add_core_cnf.sh +++ b/maxscale-system-test/mdbci/add_core_cnf.sh @@ -1,4 +1,8 @@ -set -x +if [ "$1" == "verbose" ] +then + set -x +fi + chmod 777 /tmp/ echo 2 > /proc/sys/fs/suid_dumpable sed -i "s/start() {/start() { \n export DAEMON_COREFILE_LIMIT='unlimited'; ulimit -c unlimited; /" /etc/init.d/maxscale diff --git a/maxscale-system-test/mdbci/configure_core.sh b/maxscale-system-test/mdbci/configure_core.sh deleted file mode 100755 index 2c72b5ac9..000000000 --- a/maxscale-system-test/mdbci/configure_core.sh +++ /dev/null @@ -1,11 +0,0 @@ -#!/bin/bash -set -x - -ssh -i $maxscale_sshkey -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no $maxscale_access_user@$maxscale_IP '$maxscale_access_sudo service iptables stop' - -ssh -i $maxscale_sshkey -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no $maxscale_access_user@$maxscale_IP "$maxscale_access_sudo mkdir ccore; $maxscale_access_sudo chown $maxscale_access_user ccore" - -scp -i $maxscale_sshkey -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no ${script_dir}/add_core_cnf.sh $maxscale_access_user@$maxscale_IP:./ccore/ -ssh -i $maxscale_sshkey -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no $maxscale_access_user@$maxscale_IP "$maxscale_access_sudo /home/$maxscale_access_user/ccore/add_core_cnf.sh" - -set +x From 6068850b181e5af1451a6668a227eb97212965e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 25 Jan 2018 17:47:24 +0200 Subject: [PATCH 07/54] MXS-1627: Only load users that use default auth plugin The list of users that is used for authentication shoudl only consist of users that do not use an explicit authentication plugin. This way authentication fails before any connections to the backend servers are done. --- server/modules/authenticator/MySQLAuth/dbusers.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/modules/authenticator/MySQLAuth/dbusers.c b/server/modules/authenticator/MySQLAuth/dbusers.c index c1981272b..09f056a0a 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.c +++ b/server/modules/authenticator/MySQLAuth/dbusers.c @@ -44,11 +44,11 @@ #define NEW_LOAD_DBUSERS_QUERY "SELECT u.user, u.host, d.db, u.select_priv, u.%s \ FROM mysql.user AS u LEFT JOIN mysql.db AS d \ - ON (u.user = d.user AND u.host = d.host) %s \ + ON (u.user = d.user AND u.host = d.host) WHERE u.plugin = '' %s \ UNION \ SELECT u.user, u.host, t.db, u.select_priv, u.%s \ FROM mysql.user AS u LEFT JOIN mysql.tables_priv AS t \ - ON (u.user = t.user AND u.host = t.host) %s" + ON (u.user = t.user AND u.host = t.host) WHERE u.plugin = '' %s" static int get_users(SERV_LISTENER *listener, bool skip_local); static MYSQL *gw_mysql_init(void); @@ -59,7 +59,7 @@ static bool get_hostname(DCB *dcb, char *client_hostname, size_t size); static char* get_new_users_query(const char *server_version, bool include_root) { const char* password = strstr(server_version, "5.7.") ? MYSQL57_PASSWORD : MYSQL_PASSWORD; - const char *with_root = include_root ? "" : "WHERE u.user NOT IN ('root')"; + const char *with_root = include_root ? "" : " AND u.user NOT IN ('root')"; size_t n_bytes = snprintf(NULL, 0, NEW_LOAD_DBUSERS_QUERY, password, with_root, password, with_root); char *rval = MXS_MALLOC(n_bytes + 1); From 9ded5848364ae926509d949b693cead6bab5eb5c Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Thu, 25 Jan 2018 13:04:38 +0200 Subject: [PATCH 08/54] Check that all slaves use gtid replication before performing failover --- .../modules/monitor/mariadbmon/mysql_mon.cc | 367 +++++++++--------- 1 file changed, 194 insertions(+), 173 deletions(-) diff --git a/server/modules/monitor/mariadbmon/mysql_mon.cc b/server/modules/monitor/mariadbmon/mysql_mon.cc index ef42aadea..c3767b6e2 100644 --- a/server/modules/monitor/mariadbmon/mysql_mon.cc +++ b/server/modules/monitor/mariadbmon/mysql_mon.cc @@ -148,6 +148,172 @@ static const char CN_REPLICATION_PASSWORD[] = "replication_password"; /** Default master failure verification timeout */ #define DEFAULT_MASTER_FAILURE_TIMEOUT "10" +class Gtid +{ +public: + uint32_t domain; + uint32_t server_id; + uint64_t sequence; + Gtid() + : domain(0) + , server_id(0) + , sequence(0) + {} + + /** + * Parse a Gtid-triplet from a string. In case of a multi-triplet value, only the triplet with + * the given domain is returned. + * + * @param str Gtid string + * @param search_domain The Gtid domain whose triplet should be returned. Negative domain stands for + * autoselect, which is only allowed when the string contains one triplet. + */ + Gtid(const char* str, int64_t search_domain = -1) + : domain(0) + , server_id(0) + , sequence(0) + { + // Autoselect only allowed with one triplet + ss_dassert(search_domain >= 0 || strchr(str, ',') == NULL); + parse_triplet(str); + if (search_domain >= 0 && domain != search_domain) + { + // Search for the correct triplet. + bool found = false; + for (const char* next_triplet = strchr(str, ','); + next_triplet != NULL && !found; + next_triplet = strchr(next_triplet, ',')) + { + parse_triplet(++next_triplet); + if (domain == search_domain) + { + found = true; + } + } + ss_dassert(found); + } + } + bool operator == (const Gtid& rhs) const + { + return domain == rhs.domain && server_id == rhs.server_id && sequence == rhs.sequence; + } + string to_string() const + { + std::stringstream ss; + ss << domain << "-" << server_id << "-" << sequence; + return ss.str(); + } +private: + void parse_triplet(const char* str) + { + ss_debug(int rv = ) sscanf(str, "%" PRIu32 "-%" PRIu32 "-%" PRIu64, &domain, &server_id, &sequence); + ss_dassert(rv == 3); + } +}; + +// Contains data returned by one row of SHOW ALL SLAVES STATUS +class SlaveStatusInfo +{ +public: + int master_server_id; /**< The master's server_id value. */ + string master_host; /**< Master server host name. */ + int master_port; /**< Master server port. */ + bool slave_io_running; /**< Whether the slave I/O thread is running and connected. */ + bool slave_sql_running; /**< Whether or not the SQL thread is running. */ + string master_log_file; /**< Name of the master binary log file that the I/O thread is currently + * reading from. */ + uint64_t read_master_log_pos; /**< Position up to which the I/O thread has read in the current master + * binary log file. */ + Gtid gtid_io_pos; /**< Gtid I/O position of the slave thread. Only shows the triplet with + * the current master domain. */ + string last_error; /**< Last IO or SQL error encountered. */ + + SlaveStatusInfo() + : master_server_id(0) + , master_port(0) + , slave_io_running(false) + , slave_sql_running(false) + , read_master_log_pos(0) + {} +}; + +// This class groups some miscellaneous replication related settings together. +class ReplicationSettings +{ +public: + bool gtid_strict_mode; /**< Enable additional checks for replication */ + bool log_bin; /**< Is binary logging enabled */ + bool log_slave_updates; /**< Does the slave log replicated events to binlog */ + ReplicationSettings() + : gtid_strict_mode(false) + , log_bin(false) + , log_slave_updates(false) + {} +}; + +/** + * Monitor specific information about a server + * + * Note: These are initialized in @c init_server_info + */ +class MySqlServerInfo +{ +public: + int server_id; /**< Value of @@server_id */ + int group; /**< Multi-master group where this server belongs, + * 0 for servers not in groups */ + bool read_only; /**< Value of @@read_only */ + bool slave_configured; /**< Whether SHOW SLAVE STATUS returned rows */ + bool binlog_relay; /** Server is a Binlog Relay */ + int n_slaves_configured; /**< Number of configured slave connections*/ + int n_slaves_running; /**< Number of running slave connections */ + int slave_heartbeats; /**< Number of received heartbeats */ + double heartbeat_period; /**< The time interval between heartbeats */ + time_t latest_event; /**< Time when latest event was received from the master */ + int64_t gtid_domain_id; /**< The value of gtid_domain_id, the domain which is used for + * new non-replicated events. */ + Gtid gtid_current_pos; /**< Gtid of latest event. Only shows the triplet + * with the current master domain. */ + Gtid gtid_binlog_pos; /**< Gtid of latest event written to binlog. Only shows + * the triplet with the current master domain. */ + SlaveStatusInfo slave_status; /**< Data returned from SHOW SLAVE STATUS */ + ReplicationSettings rpl_settings; /**< Miscellaneous replication related settings */ + mysql_server_version version; /**< Server version, 10.X, 5.5 or 5.1 */ + + MySqlServerInfo() + : server_id(0) + , group(0) + , read_only(false) + , slave_configured(false) + , binlog_relay(false) + , n_slaves_configured(0) + , n_slaves_running(0) + , slave_heartbeats(0) + , heartbeat_period(0) + , latest_event(0) + , gtid_domain_id(-1) + , version(MYSQL_SERVER_VERSION_51) + {} + + /** + * Calculate how many events are left in the relay log. If gtid_current_pos is ahead of Gtid_IO_Pos, + * or a server_id is zero, an error value is returned. + * + * @return Number of events in relay log according to latest queried info. A negative value signifies + * an error in the gtid-values. + */ + int64_t relay_log_events() + { + if (slave_status.gtid_io_pos.server_id != 0 && gtid_current_pos.server_id != 0 && + slave_status.gtid_io_pos.domain == gtid_current_pos.domain && + slave_status.gtid_io_pos.sequence >= gtid_current_pos.sequence) + { + return slave_status.gtid_io_pos.sequence - gtid_current_pos.sequence; + } + return -1; + } +}; + /** * Check whether specified current master is acceptable. * @@ -303,10 +469,13 @@ bool mysql_switchover_check(MXS_MONITOR* mon, * @param error_out JSON error out * @return True if failover may proceed */ -bool mysql_failover_check(MYSQL_MONITOR* mon, json_t** error_out) +bool failover_check(MYSQL_MONITOR* mon, json_t** error_out) { // Check that there is no running master and that there is at least one running server in the cluster. + // Also, all slaves must be using gtid-replication. int slaves = 0; + bool error = false; + for (MXS_MONITORED_SERVER* mon_server = mon->monitor->monitored_servers; mon_server != NULL; mon_server = mon_server->next) @@ -322,19 +491,35 @@ bool mysql_failover_check(MYSQL_MONITOR* mon, json_t** error_out) master_up_msg += ", although in maintenance mode"; } master_up_msg += "."; - PRINT_MXS_JSON_ERROR(error_out, "%s Failover not allowed.", master_up_msg.c_str()); - return false; + PRINT_MXS_JSON_ERROR(error_out, "%s", master_up_msg.c_str()); + error = true; } else if (SERVER_IS_SLAVE(mon_server->server)) { - slaves++; + const MySqlServerInfo* info = get_server_info(mon, mon_server); + if (info->slave_status.gtid_io_pos.server_id == 0) + { + string slave_not_gtid_msg = string("Slave server ") + mon_server->server->unique_name + + " is not using gtid replication or master server id is 0."; + PRINT_MXS_JSON_ERROR(error_out, "%s", slave_not_gtid_msg.c_str()); + error = true; + } + else + { + slaves++; + } } } - if (slaves == 0) + + if (error) + { + PRINT_MXS_JSON_ERROR(error_out, "Failover not allowed due to errors."); + } + else if (slaves == 0) { PRINT_MXS_JSON_ERROR(error_out, "No running slaves, cannot failover."); } - return slaves > 0; + return !error && slaves > 0; } /** @@ -486,7 +671,7 @@ bool mysql_failover(MXS_MONITOR* mon, json_t** output) MXS_NOTICE("Monitor %s already stopped, failover can proceed.", mon->name); } - rv = mysql_failover_check(handle, output); + rv = failover_check(handle, output); if (rv) { rv = do_failover(handle, output); @@ -766,170 +951,6 @@ extern "C" } -class Gtid -{ -public: - uint32_t domain; - uint32_t server_id; - uint64_t sequence; - Gtid() - : domain(0) - , server_id(0) - , sequence(0) - {} - - /** - * Parse a Gtid-triplet from a string. In case of a multi-triplet value, only the triplet with - * the given domain is returned. - * - * @param str Gtid string - * @param search_domain The Gtid domain whose triplet should be returned. Negative domain stands for - * autoselect, which is only allowed when the string contains one triplet. - */ - Gtid(const char* str, int64_t search_domain = -1) - : domain(0) - , server_id(0) - , sequence(0) - { - // Autoselect only allowed with one triplet - ss_dassert(search_domain >= 0 || strchr(str, ',') == NULL); - parse_triplet(str); - if (search_domain >= 0 && domain != search_domain) - { - // Search for the correct triplet. - bool found = false; - for (const char* next_triplet = strchr(str, ','); - next_triplet != NULL && !found; - next_triplet = strchr(next_triplet, ',')) - { - parse_triplet(++next_triplet); - if (domain == search_domain) - { - found = true; - } - } - ss_dassert(found); - } - } - bool operator == (const Gtid& rhs) const - { - return domain == rhs.domain && server_id == rhs.server_id && sequence == rhs.sequence; - } - string to_string() const - { - std::stringstream ss; - ss << domain << "-" << server_id << "-" << sequence; - return ss.str(); - } -private: - void parse_triplet(const char* str) - { - ss_debug(int rv = ) sscanf(str, "%" PRIu32 "-%" PRIu32 "-%" PRIu64, &domain, &server_id, &sequence); - ss_dassert(rv == 3); - } -}; -// Contains data returned by one row of SHOW ALL SLAVES STATUS -class SlaveStatusInfo -{ -public: - int master_server_id; /**< The master's server_id value. */ - string master_host; /**< Master server host name. */ - int master_port; /**< Master server port. */ - bool slave_io_running; /**< Whether the slave I/O thread is running and connected. */ - bool slave_sql_running; /**< Whether or not the SQL thread is running. */ - string master_log_file; /**< Name of the master binary log file that the I/O thread is currently - * reading from. */ - uint64_t read_master_log_pos; /**< Position up to which the I/O thread has read in the current master - * binary log file. */ - Gtid gtid_io_pos; /**< Gtid I/O position of the slave thread. Only shows the triplet with - * the current master domain. */ - string last_error; /**< Last IO or SQL error encountered. */ - - SlaveStatusInfo() - : master_server_id(0), - master_port(0), - slave_io_running(false), - slave_sql_running(false), - read_master_log_pos(0) - {} -}; - -// This class groups some miscellaneous replication related settings together. -class ReplicationSettings -{ -public: - bool gtid_strict_mode; /**< Enable additional checks for replication */ - bool log_bin; /**< Is binary logging enabled */ - bool log_slave_updates;/**< Does the slave log replicated events to binlog */ - ReplicationSettings() - : gtid_strict_mode(false) - , log_bin(false) - , log_slave_updates(false) - {} -}; - -/** - * Monitor specific information about a server - * - * Note: These are initialized in @c init_server_info - */ -class MySqlServerInfo -{ -public: - int server_id; /**< Value of @@server_id */ - int group; /**< Multi-master group where this server belongs, 0 for servers not in groups */ - bool read_only; /**< Value of @@read_only */ - bool slave_configured; /**< Whether SHOW SLAVE STATUS returned rows */ - bool binlog_relay; /** Server is a Binlog Relay */ - int n_slaves_configured; /**< Number of configured slave connections*/ - int n_slaves_running; /**< Number of running slave connections */ - int slave_heartbeats; /**< Number of received heartbeats */ - double heartbeat_period; /**< The time interval between heartbeats */ - time_t latest_event; /**< Time when latest event was received from the master */ - int64_t gtid_domain_id; /**< The value of gtid_domain_id, the domain which is used for new non- - * replicated events. */ - Gtid gtid_current_pos; /**< Gtid of latest event. Only shows the triplet - * with the current master domain. */ - Gtid gtid_binlog_pos; /**< Gtid of latest event written to binlog. Only shows the triplet - * with the current master domain. */ - SlaveStatusInfo slave_status; /**< Data returned from SHOW SLAVE STATUS */ - ReplicationSettings rpl_settings; /**< Miscellaneous replication related settings */ - mysql_server_version version; /**< Server version, 10.X, 5.5 or 5.1 */ - - MySqlServerInfo() - : server_id(0), - group(0), - read_only(false), - slave_configured(false), - binlog_relay(false), - n_slaves_configured(0), - n_slaves_running(0), - slave_heartbeats(0), - heartbeat_period(0), - latest_event(0), - gtid_domain_id(-1), - version(MYSQL_SERVER_VERSION_51) - {} - - /** - * Calculate how many events are left in the relay log. If gtid_current_pos is ahead of Gtid_IO_Pos, - * or a server_id is zero, an error value is returned. - * - * @return Number of events in relay log according to latest queried info. A negative value signifies - * an error in the gtid-values. - */ - int64_t relay_log_events() - { - if (slave_status.gtid_io_pos.server_id != 0 && gtid_current_pos.server_id != 0 && - slave_status.gtid_io_pos.domain == gtid_current_pos.domain && - slave_status.gtid_io_pos.sequence >= gtid_current_pos.sequence) - { - return slave_status.gtid_io_pos.sequence - gtid_current_pos.sequence; - } - return -1; - } -}; - void* info_copy_func(const void *val) { ss_dassert(val); @@ -3258,7 +3279,7 @@ bool mon_process_failover(MYSQL_MONITOR* monitor, uint32_t failover_timeout, boo MXS_NOTICE("Performing automatic failover to replace failed master '%s'.", failed_master->server->unique_name); failed_master->new_event = false; - rval = mysql_failover_check(monitor, NULL) && do_failover(monitor, NULL); + rval = failover_check(monitor, NULL) && do_failover(monitor, NULL); if (rval) { *cluster_modified_out = true; @@ -3395,7 +3416,7 @@ MXS_MONITORED_SERVER* select_new_master(MYSQL_MONITOR* mon, if (check_replication_settings(cand, cand_info)) { bool select_this = false; - // If no candidate yet, accept any. + // If no candidate yet, accept any slave. Slaves have already been checked to use gtid. if (new_master == NULL) { select_this = true; From cf0d745c1481401d320ffcbefb2cc50d74afa035 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 29 Jan 2018 11:37:46 +0200 Subject: [PATCH 09/54] MXS-1583 Add test that exposes the behaviour This will fail with MaxScale 2.2.1. --- maxscale-system-test/.gitignore | 1 + maxscale-system-test/CMakeLists.txt | 1 + .../cnf/maxscale.cnf.template.mxs1583_fwf | 77 +++++++++++++++++++ maxscale-system-test/fw/rules_mxs1583 | 7 ++ maxscale-system-test/mxs1583_fwf.cpp | 65 ++++++++++++++++ 5 files changed, 151 insertions(+) create mode 100644 maxscale-system-test/cnf/maxscale.cnf.template.mxs1583_fwf create mode 100644 maxscale-system-test/fw/rules_mxs1583 create mode 100644 maxscale-system-test/mxs1583_fwf.cpp diff --git a/maxscale-system-test/.gitignore b/maxscale-system-test/.gitignore index 5d831b984..c84dcdb12 100644 --- a/maxscale-system-test/.gitignore +++ b/maxscale-system-test/.gitignore @@ -143,6 +143,7 @@ mxs1457_ignore_deleted mxs1468 mxs1476 mxs1509 +mxs1583_fwf mxs244_prepared_stmt_loop mxs280_select_outfile mxs314 diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index f497a20fc..df555690d 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -313,6 +313,7 @@ add_test_executable(fwf_logging.cpp fwf_logging fwf_logging LABELS dbfwfilter RE add_test_executable(fwf_reload.cpp fwf_reload fwf LABELS dbfwfilter REPL_BACKEND) add_test_executable(fwf_syntax.cpp fwf_syntax fwf_syntax LABELS dbfwfilter REPL_BACKEND) add_test_executable(fwf_com_ping.cpp fwf_com_ping fwf_com_ping LABELS dbfwfilter REPL_BACKEND) +add_test_executable(mxs1583_fwf.cpp mxs1583_fwf mxs1583_fwf LABELS dbfwfilter REPL_BACKEND) # Galera node priority test add_test_executable(galera_priority.cpp galera_priority galera_priority LABELS galeramon LIGHT GALERA_BACKEND) diff --git a/maxscale-system-test/cnf/maxscale.cnf.template.mxs1583_fwf b/maxscale-system-test/cnf/maxscale.cnf.template.mxs1583_fwf new file mode 100644 index 000000000..8c9e1213b --- /dev/null +++ b/maxscale-system-test/cnf/maxscale.cnf.template.mxs1583_fwf @@ -0,0 +1,77 @@ +[maxscale] +threads=###threads### +query_classifier_args=log_unrecognized_statements=3 + +[MySQL Monitor] +type=monitor +module=mysqlmon +###repl51### +servers=server1 +user=maxskysql +passwd=skysql +monitor_interval=100 + +[Database Firewall] +type=filter +module=dbfwfilter +rules=/###access_homedir###/rules/rules.txt +log_match=true +log_no_match=true + +[RW Split Router] +type=service +router=readconnroute +servers=server1 +user=maxskysql +passwd=skysql +filters=Database Firewall + +[Read Connection Router Slave] +type=service +router=readconnroute +router_options=slave +servers=server1 +user=maxskysql +passwd=skysql + +[Read Connection Router Master] +type=service +router=readconnroute +router_options=master +servers=server1 +user=maxskysql +passwd=skysql + +[RW Split Listener] +type=listener +service=RW Split Router +protocol=MySQLClient +port=4006 + +[Read Connection Listener Slave] +type=listener +service=Read Connection Router Slave +protocol=MySQLClient +port=4009 + +[Read Connection Listener Master] +type=listener +service=Read Connection Router Master +protocol=MySQLClient +port=4008 + +[CLI] +type=service +router=cli + +[CLI Listener] +type=listener +service=CLI +protocol=maxscaled +socket=default + +[server1] +type=server +address=###node_server_IP_1### +port=###node_server_port_1### +protocol=MySQLBackend diff --git a/maxscale-system-test/fw/rules_mxs1583 b/maxscale-system-test/fw/rules_mxs1583 new file mode 100644 index 000000000..ce87945c9 --- /dev/null +++ b/maxscale-system-test/fw/rules_mxs1583 @@ -0,0 +1,7 @@ +rule deny_functions_on_a match uses_function a + +users %@% match all rules deny_functions_on_a + +rule deny_functions_on_b match uses_function b + +users %@% match all rules deny_functions_on_b diff --git a/maxscale-system-test/mxs1583_fwf.cpp b/maxscale-system-test/mxs1583_fwf.cpp new file mode 100644 index 000000000..fa6c2e186 --- /dev/null +++ b/maxscale-system-test/mxs1583_fwf.cpp @@ -0,0 +1,65 @@ +/** + * Firewall filter multiple matching users + * + * Test it multiple matching user rows are handled in OR fashion. + */ + + +#include +#include +#include "testconnections.h" +#include "fw_copy_rules.h" + +int main(int argc, char** argv) +{ + TestConnections::skip_maxscale_start(true); + char rules_dir[4096]; + + TestConnections test(argc, argv); + test.stop_timeout(); + + test.tprintf("Creating rules\n"); + test.maxscales->stop_maxscale(0); + + sprintf(rules_dir, "%s/fw/", test_dir); + copy_rules(&test, (char*) "rules_mxs1583", rules_dir); + + test.set_timeout(60); + test.maxscales->start_maxscale(0); + + test.set_timeout(30); + test.maxscales->connect_maxscale(0); + + test.try_query(test.maxscales->conn_rwsplit[0], "drop table if exists t"); + test.try_query(test.maxscales->conn_rwsplit[0], "create table t (a text, b text)"); + + test.tprintf("Trying query that matches one 'user' row, expecting failure\n"); + test.set_timeout(30); + test.add_result(!execute_query(test.maxscales->conn_rwsplit[0], "select concat(a) from t"), + "Query that matches one 'user' row should fail.\n"); + + test.tprintf("Trying query that matches other 'user' row, expecting failure\n"); + test.set_timeout(30); + test.add_result(!execute_query(test.maxscales->conn_rwsplit[0], "select concat(b) from t"), + "Query that matches other 'user' row should fail.\n"); + + test.tprintf("Trying query that matches both 'user' rows, expecting failure\n"); + test.set_timeout(30); + test.add_result(!execute_query_silent(test.maxscales->conn_rwsplit[0], "select concat(a), concat(b) from t"), + "Query that matches both 'user' rows should fail.\n"); + + test.tprintf("Trying non-matching query to blacklisted RWSplit, expecting success\n"); + test.set_timeout(30); + test.add_result(execute_query_silent(test.maxscales->conn_rwsplit[0], "show status"), + "Non-matching query to blacklist service should succeed.\n"); + + test.stop_timeout(); + test.tprintf("Checking if MaxScale is alive\n"); + test.check_maxscale_processes(0, 1); + test.maxscales->stop_maxscale(0); + sleep(10); + test.tprintf("Checking if MaxScale was succesfully terminated\n"); + test.check_maxscale_processes(0, 0); + + return test.global_result; +} From 0db538db9abaff4c80bd42bd3679532ad1a616b8 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 29 Jan 2018 11:48:13 +0200 Subject: [PATCH 10/54] MXS-1583 Treat independent 'users' line in OR-fashion If there are several 'users' lines in a rule file, for a particular user, the rules each matching line will be checked independently until a rule match is found. That is, the rules of each 'users' line are treated in an OR-fashion with respect to each other. --- .../Filters/Database-Firewall-Filter.md | 6 ++ .../modules/filter/dbfwfilter/dbfwfilter.cc | 2 +- server/modules/filter/dbfwfilter/user.cc | 99 +++++++++++-------- server/modules/filter/dbfwfilter/user.hh | 16 +-- 4 files changed, 76 insertions(+), 47 deletions(-) diff --git a/Documentation/Filters/Database-Firewall-Filter.md b/Documentation/Filters/Database-Firewall-Filter.md index 18eb8dd2a..704b585c4 100644 --- a/Documentation/Filters/Database-Firewall-Filter.md +++ b/Documentation/Filters/Database-Firewall-Filter.md @@ -355,6 +355,12 @@ After the matching part comes the rules keyword after which a list of rule names is expected. This allows reusing of the rules and enables varying levels of query restriction. +If a particular _NAME_ appears on several `users` lines, then when an +actual user matches that name, the rules of each line are checked +independently until there is a match for the statement in question. That +is, the rules of each `users` line are treated in an _OR_ fashion with +respect to each other. + ## Module commands Read [Module Commands](../Reference/Module-Commands.md) documentation for diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.cc b/server/modules/filter/dbfwfilter/dbfwfilter.cc index fd5e85fb6..1c0aaa95e 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.cc +++ b/server/modules/filter/dbfwfilter/dbfwfilter.cc @@ -1029,7 +1029,7 @@ static bool process_user_templates(UserMap& users, const TemplateList& templates if (newrules.size() > 0) { - user->append_rules(ut->type, newrules); + user->add_rules(ut->type, newrules); } } diff --git a/server/modules/filter/dbfwfilter/user.cc b/server/modules/filter/dbfwfilter/user.cc index 0792283ce..ee225333c 100644 --- a/server/modules/filter/dbfwfilter/user.cc +++ b/server/modules/filter/dbfwfilter/user.cc @@ -31,20 +31,20 @@ const char* User::name() const return m_name.c_str(); } -void User::append_rules(match_type mode, const RuleList& rules) +void User::add_rules(match_type mode, const RuleList& rules) { switch (mode) { case FWTOK_MATCH_ANY: - rules_or.insert(rules_or.end(), rules.begin(), rules.end()); + rules_or_vector.push_back(rules); break; case FWTOK_MATCH_ALL: - rules_and.insert(rules_and.end(), rules.begin(), rules.end()); + rules_and_vector.push_back(rules); break; case FWTOK_MATCH_STRICT_ALL: - rules_strict_and.insert(rules_strict_and.end(), rules.begin(), rules.end()); + rules_strict_and_vector.push_back(rules); break; default: @@ -73,28 +73,39 @@ bool User::match_any(Dbfw* my_instance, DbfwSession* my_session, bool rval = false; - if (rules_or.size() > 0 && should_match(queue)) + for (RuleListVector::iterator i = rules_or_vector.begin(); i != rules_or_vector.end(); ++i) { - char *fullquery = modutil_get_SQL(queue); + RuleList& rules_or = *i; - if (fullquery) + if (rules_or.size() > 0 && should_match(queue)) { - for (RuleList::iterator it = rules_or.begin(); it != rules_or.end(); it++) + char *fullquery = modutil_get_SQL(queue); + + if (fullquery) { - if (rule_is_active(*it)) + for (RuleList::iterator j = rules_or.begin(); j != rules_or.end(); j++) { - if (rule_matches(my_instance, my_session, queue, *it, fullquery)) + if (rule_is_active(*j)) { - *rulename = MXS_STRDUP_A((*it)->name().c_str()); - rval = true; - break; + if (rule_matches(my_instance, my_session, queue, *j, fullquery)) + { + *rulename = MXS_STRDUP_A((*j)->name().c_str()); + rval = true; + break; + } } } - } - MXS_FREE(fullquery); + MXS_FREE(fullquery); + } + } + + if (rval) + { + break; } } + return rval; } @@ -116,44 +127,54 @@ bool User::do_match(Dbfw* my_instance, DbfwSession* my_session, bool rval = false; bool have_active_rule = false; std::string matching_rules; - RuleList& rules = mode == User::ALL ? rules_and : rules_strict_and; + RuleListVector& rules_vector = (mode == User::ALL ? rules_and_vector : rules_strict_and_vector); - if (rules.size() > 0 && should_match(queue)) + for (RuleListVector::iterator i = rules_vector.begin(); i != rules_vector.end(); ++i) { - char *fullquery = modutil_get_SQL(queue); + RuleList& rules = *i; - if (fullquery) + if (rules.size() > 0 && should_match(queue)) { - rval = true; - for (RuleList::iterator it = rules.begin(); it != rules.end(); it++) + char *fullquery = modutil_get_SQL(queue); + + if (fullquery) { - if (rule_is_active(*it)) + rval = true; + for (RuleList::iterator j = rules.begin(); j != rules.end(); j++) { - have_active_rule = true; - - if (rule_matches(my_instance, my_session, queue, *it, fullquery)) + if (rule_is_active(*j)) { - matching_rules += (*it)->name(); - matching_rules += " "; - } - else - { - rval = false; + have_active_rule = true; - if (mode == User::STRICT) + if (rule_matches(my_instance, my_session, queue, *j, fullquery)) { - break; + matching_rules += (*j)->name(); + matching_rules += " "; + } + else + { + rval = false; + + if (mode == User::STRICT) + { + break; + } } } } - } - if (!have_active_rule) - { - /** No active rules */ - rval = false; + if (!have_active_rule) + { + /** No active rules */ + rval = false; + } + MXS_FREE(fullquery); } - MXS_FREE(fullquery); + } + + if (rval) + { + break; } } diff --git a/server/modules/filter/dbfwfilter/user.hh b/server/modules/filter/dbfwfilter/user.hh index 5c537389b..f80eb5254 100644 --- a/server/modules/filter/dbfwfilter/user.hh +++ b/server/modules/filter/dbfwfilter/user.hh @@ -57,12 +57,12 @@ public: const char* name() const; /** - * Append new rules to existing rules + * Add new rules to existing rules * * @param mode Matching mode for the rule * @param rules Rules to append */ - void append_rules(match_type mode, const RuleList& rules); + void add_rules(match_type mode, const RuleList& rules); /** * Check if a query matches some rule @@ -84,11 +84,13 @@ private: STRICT }; - RuleList rules_or; /*< If any of these rules match the action is triggered */ - RuleList rules_and; /*< All of these rules must match for the action to trigger */ - RuleList rules_strict_and; /*< rules that skip the rest of the rules if one of them - * fails. This is only for rules paired with 'match strict_all'. */ - std::string m_name; /*< Name of the user */ + typedef std::vector RuleListVector; + + RuleListVector rules_or_vector; /*< If any of these rules match the action is triggered */ + RuleListVector rules_and_vector; /*< All of these rules must match for the action to trigger */ + RuleListVector rules_strict_and_vector;/*< rules that skip the rest of the rules if one of them + * fails. This is only for rules paired with 'match strict_all'. */ + std::string m_name; /*< Name of the user */ /** * Functions for matching rules From ef1ec2e524aecb88cc926a7e470555c5aba654a6 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 29 Jan 2018 15:15:57 +0200 Subject: [PATCH 11/54] MXS-1591 Mark GET_LOCK(...) et.al. as WRITE The follwing statements SELECT GET_LOCK('lock1',10); SELECT IS_FREE_LOCK('lock1'); SELECT IS_USED_LOCK('lock1'); SELECT RELEASE_LOCK('lock1'); are now classified as QUERY_TYPE_READ|QUERY_TYPE_WRITE. That will make cooperative locking work if these functions are used inside non-read-only transactions and outside transanctions. --- query_classifier/qc_sqlite/builtin_functions.c | 14 ++++++++++---- query_classifier/test/expected.sql | 4 ++++ query_classifier/test/input.sql | 4 ++++ 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/query_classifier/qc_sqlite/builtin_functions.c b/query_classifier/qc_sqlite/builtin_functions.c index af42e1f65..7839441ed 100644 --- a/query_classifier/qc_sqlite/builtin_functions.c +++ b/query_classifier/qc_sqlite/builtin_functions.c @@ -250,27 +250,33 @@ static const char* BUILTIN_FUNCTIONS[] = * https://mariadb.com/kb/en/mariadb/miscellaneous-functions/ */ "default", - "get_lock", "inet6_aton", "inet6_ntoa", "inet_aton", "inet_ntoa", - "is_free_lock", "is_ipv4", "is_ipv4_compat", "is_ipv4_mapped", "is_ipv6", - "is_used_lock", "last_value", "master_gtid_wait", "master_pos_wait", "name_const", - "release_lock", "sleep", "uuid", "uuid_short", "values", + /** + * Although conceptually non-updating, we classify these as WRITE as + * that will force them to be sent to _master_ outside transactions. + * + * "get_lock", + * "is_free_lock", + * "is_used_lock", + * "release_lock", + */ + /* * Numeric Functions * https://mariadb.com/kb/en/mariadb/numeric-functions/ diff --git a/query_classifier/test/expected.sql b/query_classifier/test/expected.sql index dbff01229..f57402db2 100644 --- a/query_classifier/test/expected.sql +++ b/query_classifier/test/expected.sql @@ -19,3 +19,7 @@ QUERY_TYPE_READ|QUERY_TYPE_SYSVAR_READ QUERY_TYPE_READ|QUERY_TYPE_WRITE QUERY_TYPE_READ|QUERY_TYPE_WRITE QUERY_TYPE_READ|QUERY_TYPE_WRITE +QUERY_TYPE_READ|QUERY_TYPE_WRITE +QUERY_TYPE_READ|QUERY_TYPE_WRITE +QUERY_TYPE_READ|QUERY_TYPE_WRITE +QUERY_TYPE_READ|QUERY_TYPE_WRITE diff --git a/query_classifier/test/input.sql b/query_classifier/test/input.sql index 48c5d6276..643e2c2d0 100644 --- a/query_classifier/test/input.sql +++ b/query_classifier/test/input.sql @@ -19,3 +19,7 @@ select if(@@hostname='box02','prod_mariadb02','n'); select next value for seq1; select nextval(seq1); select seq1.nextval; +SELECT GET_LOCK('lock1',10); +SELECT IS_FREE_LOCK('lock1'); +SELECT IS_USED_LOCK('lock1'); +SELECT RELEASE_LOCK('lock1'); From 5212b67bb2e64183b6e1c8d5406861714644b0ea Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 29 Jan 2018 15:33:29 +0200 Subject: [PATCH 12/54] Update 2.2 version number --- VERSION22.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION22.cmake b/VERSION22.cmake index 30e294c04..ff9093763 100644 --- a/VERSION22.cmake +++ b/VERSION22.cmake @@ -5,7 +5,7 @@ set(MAXSCALE_VERSION_MAJOR "2" CACHE STRING "Major version") set(MAXSCALE_VERSION_MINOR "2" CACHE STRING "Minor version") -set(MAXSCALE_VERSION_PATCH "1" CACHE STRING "Patch version") +set(MAXSCALE_VERSION_PATCH "2" CACHE STRING "Patch version") # This should only be incremented if a package is rebuilt set(MAXSCALE_BUILD_NUMBER 1 CACHE STRING "Release number") From e455e8d43c7173f026d08136f2357b4538d9babc Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Mon, 29 Jan 2018 14:13:39 +0200 Subject: [PATCH 13/54] Simplify monitor start and stop during switchover/failover If these operations failed, the monitor could be left stopped. --- .../modules/monitor/mariadbmon/mysql_mon.cc | 44 ++++++------------- 1 file changed, 13 insertions(+), 31 deletions(-) diff --git a/server/modules/monitor/mariadbmon/mysql_mon.cc b/server/modules/monitor/mariadbmon/mysql_mon.cc index c3767b6e2..a965212a8 100644 --- a/server/modules/monitor/mariadbmon/mysql_mon.cc +++ b/server/modules/monitor/mariadbmon/mysql_mon.cc @@ -534,14 +534,7 @@ bool failover_check(MYSQL_MONITOR* mon, json_t** error_out) */ bool mysql_switchover(MXS_MONITOR* mon, SERVER* new_master, SERVER* current_master, json_t** output) { - bool rv = true; - - MYSQL_MONITOR *handle = static_cast(mon->handle); - - *output = NULL; - bool stopped = stop_monitor(mon); - if (stopped) { MXS_NOTICE("Stopped the monitor %s for the duration of switchover.", mon->name); @@ -551,6 +544,8 @@ bool mysql_switchover(MXS_MONITOR* mon, SERVER* new_master, SERVER* current_mast MXS_NOTICE("Monitor %s already stopped, switchover can proceed.", mon->name); } + bool rv = true; + *output = NULL; MXS_MONITORED_SERVER* monitored_new_master = NULL; MXS_MONITORED_SERVER* monitored_current_master = NULL; @@ -562,6 +557,7 @@ bool mysql_switchover(MXS_MONITOR* mon, SERVER* new_master, SERVER* current_mast if (rv) { bool failover = config_get_bool(mon->parameters, CN_AUTO_FAILOVER); + MYSQL_MONITOR *handle = static_cast(mon->handle); rv = do_switchover(handle, monitored_current_master, monitored_new_master, output); if (rv) @@ -569,11 +565,6 @@ bool mysql_switchover(MXS_MONITOR* mon, SERVER* new_master, SERVER* current_mast MXS_NOTICE("Switchover %s -> %s performed.", current_master->unique_name ? current_master->unique_name : "none", new_master->unique_name); - - if (stopped) - { - startMonitor(mon, mon->parameters); - } } else { @@ -598,14 +589,11 @@ bool mysql_switchover(MXS_MONITOR* mon, SERVER* new_master, SERVER* current_mast } } } - else - { - if (stopped) - { - startMonitor(mon, mon->parameters); - } - } + if (stopped) + { + startMonitor(mon, mon->parameters); + } return rv; } @@ -659,8 +647,6 @@ bool mysql_handle_switchover(const MODULECMD_ARG* args, json_t** output) */ bool mysql_failover(MXS_MONITOR* mon, json_t** output) { - bool rv = true; - MYSQL_MONITOR *handle = static_cast(mon->handle); bool stopped = stop_monitor(mon); if (stopped) { @@ -671,6 +657,8 @@ bool mysql_failover(MXS_MONITOR* mon, json_t** output) MXS_NOTICE("Monitor %s already stopped, failover can proceed.", mon->name); } + bool rv = true; + MYSQL_MONITOR *handle = static_cast(mon->handle); rv = failover_check(handle, output); if (rv) { @@ -678,22 +666,16 @@ bool mysql_failover(MXS_MONITOR* mon, json_t** output) if (rv) { MXS_NOTICE("Failover performed."); - if (stopped) - { - startMonitor(mon, mon->parameters); - } } else { PRINT_MXS_JSON_ERROR(output, "Failover failed."); } } - else + + if (stopped) { - if (stopped) - { - startMonitor(mon, mon->parameters); - } + startMonitor(mon, mon->parameters); } return rv; } @@ -734,7 +716,6 @@ bool mysql_handle_failover(const MODULECMD_ARG* args, json_t** output) */ bool mysql_rejoin(MXS_MONITOR* mon, SERVER* rejoin_server, json_t** output) { - MYSQL_MONITOR *handle = static_cast(mon->handle); bool stopped = stop_monitor(mon); if (stopped) { @@ -746,6 +727,7 @@ bool mysql_rejoin(MXS_MONITOR* mon, SERVER* rejoin_server, json_t** output) } bool rval = false; + MYSQL_MONITOR *handle = static_cast(mon->handle); if (cluster_can_be_joined(handle)) { MXS_MONITORED_SERVER* mon_server = NULL; From b5a291bb78192c994ac14b26f504605037c5c4a1 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 29 Jan 2018 15:44:14 +0200 Subject: [PATCH 14/54] Update ChangeLog, Add Release Notes for 2.2 --- Documentation/Changelog.md | 1 + .../MaxScale-2.2.2-Release-Notes.md | 48 +++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 Documentation/Release-Notes/MaxScale-2.2.2-Release-Notes.md diff --git a/Documentation/Changelog.md b/Documentation/Changelog.md index e7e0109dc..ccf10bb10 100644 --- a/Documentation/Changelog.md +++ b/Documentation/Changelog.md @@ -28,6 +28,7 @@ the master. There is also limited capability for rejoining nodes. For more details, please refer to: +* [MariaDB MaxScale 2.2.2 Release Notes](Release-Notes/MaxScale-2.2.2-Release-Notes.md) * [MariaDB MaxScale 2.2.1 Release Notes](Release-Notes/MaxScale-2.2.1-Release-Notes.md) * [MariaDB MaxScale 2.2.0 Release Notes](Release-Notes/MaxScale-2.2.0-Release-Notes.md) diff --git a/Documentation/Release-Notes/MaxScale-2.2.2-Release-Notes.md b/Documentation/Release-Notes/MaxScale-2.2.2-Release-Notes.md new file mode 100644 index 000000000..a27d13a46 --- /dev/null +++ b/Documentation/Release-Notes/MaxScale-2.2.2-Release-Notes.md @@ -0,0 +1,48 @@ +# MariaDB MaxScale 2.2.2 Release Notes + +Release 2.2.2 is a XXX release. + +This document describes the changes in release 2.2.2, when compared to +release 2.2.1. + +For any problems you encounter, please consider submitting a bug +report at [Jira](https://jira.mariadb.org). + +## Changed Features + +## Dropped Features + +## New Features + +## Bug fixes + +[Here is a list of bugs fixed in MaxScale 2.2.2.](https://jira.mariadb.org/issues/?jql=project%20%3D%20MXS%20AND%20issuetype%20%3D%20Bug%20AND%20status%20%3D%20Closed%20AND%20fixVersion%20%3D%202.2.2) + +* [MXS-1630](https://jira.mariadb.org/browse/MXS-1630) MaxCtrl binary are not included by default in MaxScale package +* [MXS-1620](https://jira.mariadb.org/browse/MXS-1620) CentOS package symbols are stripped +* [MXS-1615](https://jira.mariadb.org/browse/MXS-1615) Masking filter accesses wrong command argument. +* [MXS-1614](https://jira.mariadb.org/browse/MXS-1614) MariaDBMon yet adding mysqlbackend as the protocol instead of mariadbbackend +* [MXS-1606](https://jira.mariadb.org/browse/MXS-1606) After enabling detect_replication_lag MariaDBMon does not create the maxscale_schema.replication_heartbeat database and table +* [MXS-1604](https://jira.mariadb.org/browse/MXS-1604) PamAuth Default Authentication is Different from MariaDB +* [MXS-1591](https://jira.mariadb.org/browse/MXS-1591) Adding get_lock and release_lock support +* [MXS-1539](https://jira.mariadb.org/browse/MXS-1539) Authentication data should be thread specific + +## Known Issues and Limitations + +There are some limitations and known issues within this version of MaxScale. +For more information, please refer to the [Limitations](../About/Limitations.md) document. + +## Packaging + +RPM and Debian packages are provided for the Linux distributions supported +by MariaDB Enterprise. + +Packages can be downloaded [here](https://mariadb.com/resources/downloads). + +## Source Code + +The source code of MaxScale is tagged at GitHub with a tag, which is identical +with the version of MaxScale. For instance, the tag of version X.Y.Z of MaxScale +is X.Y.Z. Further, *master* always refers to the latest released non-beta version. + +The source code is available [here](https://github.com/mariadb-corporation/MaxScale). From ab0ea417ae3aef098c2cff05c65b72b821bb4930 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 29 Jan 2018 17:45:47 +0200 Subject: [PATCH 15/54] Fix typo in maxctrl/CMakeLists.txt The correct command is `message`. --- maxctrl/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maxctrl/CMakeLists.txt b/maxctrl/CMakeLists.txt index 6516a82eb..3105cb14e 100644 --- a/maxctrl/CMakeLists.txt +++ b/maxctrl/CMakeLists.txt @@ -20,5 +20,5 @@ if (BUILD_MAXCTRL) message(FATAL_ERROR "Not building MaxCtrl: npm or Node.js >= 6.0.0 not found. Add the following to skip MaxCtrl: -DBUILD_MAXCTRL=N") endif() else() - messages(STATUS "Not building MaxCtrl: BUILD_MAXCTRL=N") + message(STATUS "Not building MaxCtrl: BUILD_MAXCTRL=N") endif() From 1a33c1caef4580614ea049415aca2dc20ebc19de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 30 Jan 2018 08:19:08 +0200 Subject: [PATCH 16/54] Build CDC connector into the core test library The CDC connector can be build directly into the core testing library for testing purposes. This way we remove an unnecessary dependency on a library. This commit fixes the linkage failure of the cdc_datatypes test. --- maxscale-system-test/CMakeLists.txt | 11 ++++++++--- maxscale-system-test/utilities.cmake | 4 ---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index df555690d..90899569e 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -50,10 +50,15 @@ add_library(testcore SHARED testconnections.cpp nodes.cpp mariadb_nodes.cpp maxs mariadb_func.cpp get_com_select_insert.cpp maxadmin_operations.cpp big_transaction.cpp sql_t1.cpp test_binlog_fnc.cpp get_my_ip.cpp big_load.cpp get_com_select_insert.cpp different_size.cpp fw_copy_rules maxinfo_func.cpp config_operations.cpp rds_vpc.cpp execute_cmd.cpp - blob_test.cpp) -target_link_libraries(testcore ${MYSQL_CLIENT} ${CDC_CONNECTOR_LIBRARIES} ${JANSSON_LIBRARIES} z nsl m pthread ssl dl rt crypto crypt) + blob_test.cpp + # Include the CDC connector in the core library + ${CMAKE_SOURCE_DIR}/../connectors/cdc-connector/cdc_connector.cpp) +target_link_libraries(testcore ${MYSQL_CLIENT} ${JANSSON_LIBRARIES} z nsl m pthread ssl dl rt crypto crypt) install(TARGETS testcore DESTINATION system-test) -add_dependencies(testcore connector-c cdc_connector jansson) +add_dependencies(testcore connector-c jansson) + +# Include the CDC connector headers +include_directories(${CMAKE_SOURCE_DIR}/../connectors/cdc-connector/) # Tool used to check backend state add_test_executable_notest(check_backend.cpp check_backend check_backend LABELS CONFIG) diff --git a/maxscale-system-test/utilities.cmake b/maxscale-system-test/utilities.cmake index a683b7fd0..a8f274166 100644 --- a/maxscale-system-test/utilities.cmake +++ b/maxscale-system-test/utilities.cmake @@ -103,10 +103,6 @@ ExternalProject_Add(connector-c include_directories(${CMAKE_BINARY_DIR}/include) set(MYSQL_CLIENT ${CMAKE_BINARY_DIR}/lib/mariadb/libmariadbclient.a CACHE INTERNAL "") -# Build the CDC connector -add_library(cdc_connector ${CMAKE_SOURCE_DIR}/../connectors/cdc-connector/cdc_connector.cpp) -include_directories(${CMAKE_SOURCE_DIR}/../connectors/cdc-connector/) - # # Check that all required components are present. To build even without them, # add e.g. -DHAVE_PHP=Y to the CMake invocation From b7af191f400d3519eb868ed4aa25aec0f2323c5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 30 Jan 2018 08:34:07 +0200 Subject: [PATCH 17/54] Improve CDC connector error messages A more precise error message is now returned if authentication times out. --- connectors/cdc-connector/cdc_connector.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/connectors/cdc-connector/cdc_connector.cpp b/connectors/cdc-connector/cdc_connector.cpp index fa9628f70..5aa2814fa 100644 --- a/connectors/cdc-connector/cdc_connector.cpp +++ b/connectors/cdc-connector/cdc_connector.cpp @@ -460,7 +460,8 @@ bool Connection::do_auth() { buf[bytes] = '\0'; m_error = "Authentication failed: "; - m_error += buf; + m_error += bytes > 0 ? buf : "Request timed out"; + } else { From cbfee5698fc74eca88e29cd7edccd07274159854 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 30 Jan 2018 09:00:47 +0200 Subject: [PATCH 18/54] Fix CDC::connect The function used the m_fd member variable before it was updated to point to the actual file descriptor of the connection. This caused the test to fail. Also fixed the nointr_write function to correctly process multiple consecutive writes. --- connectors/cdc-connector/cdc_connector.cpp | 55 +++++++++++++--------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/connectors/cdc-connector/cdc_connector.cpp b/connectors/cdc-connector/cdc_connector.cpp index 5aa2814fa..be751488a 100644 --- a/connectors/cdc-connector/cdc_connector.cpp +++ b/connectors/cdc-connector/cdc_connector.cpp @@ -254,28 +254,32 @@ bool Connection::connect(const std::string& table, const std::string& gtid) m_error = "Failed to set socket non-blocking: "; m_error += strerror_r(errno, err, sizeof(err)); } - else if (do_auth() && do_registration()) + else { - std::string req_msg(REQUEST_MSG); - req_msg += table; + m_fd = c_fd.release(); + m_connected = true; - if (gtid.length()) + if (do_auth() && do_registration()) { - req_msg += " "; - req_msg += gtid; - } + std::string req_msg(REQUEST_MSG); + req_msg += table; - if (nointr_write(req_msg.c_str(), req_msg.length()) == -1) - { - char err[ERRBUF_SIZE]; - m_error = "Failed to write request: "; - m_error += strerror_r(errno, err, sizeof(err)); - } - else if ((m_first_row = read())) - { - rval = true; - m_connected = true; - m_fd = c_fd.release(); + if (gtid.length()) + { + req_msg += " "; + req_msg += gtid; + } + + if (nointr_write(req_msg.c_str(), req_msg.length()) == -1) + { + char err[ERRBUF_SIZE]; + m_error = "Failed to write request: "; + m_error += strerror_r(errno, err, sizeof(err)); + } + else if ((m_first_row = read())) + { + rval = true; + } } } } @@ -438,11 +442,12 @@ bool Connection::do_auth() std::string auth_str = generateAuthString(m_user, m_password); /** Send the auth string */ - if (nointr_write(auth_str.c_str(), auth_str.length()) == -1) + int rc = nointr_write(auth_str.c_str(), auth_str.length()); + if (rc <= 0) { char err[ERRBUF_SIZE]; m_error = "Failed to write authentication data: "; - m_error += strerror_r(errno, err, sizeof(err)); + m_error += rc == -1 ? strerror_r(errno, err, sizeof(err)) : "Write timeout"; } else { @@ -688,11 +693,12 @@ int Connection::nointr_read(void *dest, size_t size) int Connection::nointr_write(const void *src, size_t size) { int rc = 0; - int n_bytes = 0; + size_t n_bytes = 0; + const uint8_t* ptr = static_cast(src); - if (wait_for_event(POLLOUT) > 0) + do { - while ((rc = ::write(m_fd, src, size)) < 0 && errno == EINTR) + while ((rc = ::write(m_fd, ptr, size)) < 0 && errno == EINTR) { ; } @@ -707,8 +713,11 @@ int Connection::nointr_write(const void *src, size_t size) else if (rc > 0) { n_bytes += rc; + ptr += rc; + size -= rc; } } + while (n_bytes < size && wait_for_event(POLLOUT) > 0); return n_bytes; } From 524e55bf524790aca1701ca71e7a3f65b2a8ce29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 30 Jan 2018 09:17:09 +0200 Subject: [PATCH 19/54] Sync slaves before checking MaxScale is alive Synchronizing the slaves before checking that MaxScale is still alive makes sure the slave servers have settled down to a known state. --- maxscale-system-test/different_size_rwsplit.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/maxscale-system-test/different_size_rwsplit.cpp b/maxscale-system-test/different_size_rwsplit.cpp index 33400406b..a3a1b804a 100644 --- a/maxscale-system-test/different_size_rwsplit.cpp +++ b/maxscale-system-test/different_size_rwsplit.cpp @@ -19,6 +19,7 @@ int main(int argc, char *argv[]) different_packet_size(Test, false); + Test->repl->sync_slaves(); Test->check_maxscale_alive(0); int rval = Test->global_result; delete Test; From 5bc945df3f9ab92f0f75c3d4c87a0fe881881091 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 30 Jan 2018 09:24:09 +0200 Subject: [PATCH 20/54] Allow monitor to stabilize in mxs1516 The test needs to give the monitor enough time to detect the change in the replication topology in order for it to work. --- maxscale-system-test/mxs1516.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/maxscale-system-test/mxs1516.cpp b/maxscale-system-test/mxs1516.cpp index c1081b4e2..340067374 100644 --- a/maxscale-system-test/mxs1516.cpp +++ b/maxscale-system-test/mxs1516.cpp @@ -17,6 +17,9 @@ int main(int argc, char** argv) test.repl->connect(); test.repl->change_master(1, 0); + // Give the monitor some time to detect it + sleep(5); + test.add_result(execute_query_silent(test.maxscales->conn_master[0], "SELECT 1") == 0, "Query should fail"); // Change the master back to the original one From 6410b4f19a0025a1ba16685c3a26b34a1ae03c3f Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Tue, 30 Jan 2018 13:27:41 +0200 Subject: [PATCH 21/54] MXS-1633 Turn off collecting of sqlite3 memstats According to customer reports collecting the statistics has a significant impact on the performance. As we don't need that information we can just as well turn off that. Further, since maxscale-common now links to the sqlite3-library, no module needs to do that explicitly. --- server/core/CMakeLists.txt | 1 + server/core/gateway.cc | 39 +++++++++++++++++++ .../authenticator/MySQLAuth/CMakeLists.txt | 2 +- .../modules/routing/avrorouter/CMakeLists.txt | 2 +- .../routing/binlogrouter/CMakeLists.txt | 4 +- .../routing/binlogrouter/test/CMakeLists.txt | 2 +- 6 files changed, 45 insertions(+), 5 deletions(-) diff --git a/server/core/CMakeLists.txt b/server/core/CMakeLists.txt index a76c021dc..a6c6e979d 100644 --- a/server/core/CMakeLists.txt +++ b/server/core/CMakeLists.txt @@ -73,6 +73,7 @@ target_link_libraries(maxscale-common z rt m + sqlite3 stdc++ gnutls gcrypt diff --git a/server/core/gateway.cc b/server/core/gateway.cc index 1d900d42f..d2caf5695 100644 --- a/server/core/gateway.cc +++ b/server/core/gateway.cc @@ -46,6 +46,7 @@ #include #include #include +#include #include #include #include @@ -189,6 +190,7 @@ static void disable_module_unloading(const char* arg); static void enable_module_unloading(const char* arg); static void redirect_output_to_file(const char* arg); static bool user_is_acceptable(const char* specified_user); +static bool init_sqlite3(); struct DEBUG_ARGUMENT { @@ -1957,6 +1959,13 @@ int main(int argc, char **argv) } } + if (!init_sqlite3()) + { + MXS_ERROR("Could not initialize sqlite3, exiting."); + rc = MAXSCALE_INTERNALERROR; + goto return_main; + } + MXS_NOTICE("MariaDB MaxScale %s started", MAXSCALE_VERSION); MXS_NOTICE("MaxScale is running in process %i", getpid()); @@ -3304,3 +3313,33 @@ static bool user_is_acceptable(const char* specified_user) return acceptable; } + +static bool init_sqlite3() +{ + bool rv = true; + + // Collecting the memstatus introduces locking that, according to customer reports, + // has a significant impact on the performance. + if (sqlite3_config(SQLITE_CONFIG_MEMSTATUS, (int)0) == SQLITE_OK) // 0 turns off. + { + MXS_NOTICE("The collection of SQLite memory allocation statistics turned off."); + } + else + { + MXS_WARNING("Could not turn off the collection of SQLite memory allocation statistics."); + // Non-fatal, we simply will take a small performance hit. + } + + if (sqlite3_config(SQLITE_CONFIG_MULTITHREAD) == SQLITE_OK) + { + MXS_NOTICE("Threading mode of SQLite set to Multi-thread."); + } + else + { + MXS_ERROR("Could not set the threading mode of SQLite to Multi-thread. " + "MaxScale will terminate."); + rv = false; + } + + return rv; +} diff --git a/server/modules/authenticator/MySQLAuth/CMakeLists.txt b/server/modules/authenticator/MySQLAuth/CMakeLists.txt index a4aa424f1..d6ac92947 100644 --- a/server/modules/authenticator/MySQLAuth/CMakeLists.txt +++ b/server/modules/authenticator/MySQLAuth/CMakeLists.txt @@ -2,7 +2,7 @@ if(SQLITE_VERSION VERSION_LESS 3.3) message(FATAL_ERROR "SQLite version 3.3 or higher is required") else() add_library(mysqlauth SHARED mysql_auth.c dbusers.c) - target_link_libraries(mysqlauth maxscale-common mysqlcommon sqlite3) + target_link_libraries(mysqlauth maxscale-common mysqlcommon) set_target_properties(mysqlauth PROPERTIES VERSION "1.0.0") install_module(mysqlauth core) endif() diff --git a/server/modules/routing/avrorouter/CMakeLists.txt b/server/modules/routing/avrorouter/CMakeLists.txt index d852862bb..c9be3ddfd 100644 --- a/server/modules/routing/avrorouter/CMakeLists.txt +++ b/server/modules/routing/avrorouter/CMakeLists.txt @@ -4,7 +4,7 @@ if(AVRO_FOUND AND JANSSON_FOUND) add_library(avrorouter SHARED avro.c ../binlogrouter/binlog_common.c avro_client.c avro_schema.c avro_rbr.c avro_file.c avro_index.c) set_target_properties(avrorouter PROPERTIES VERSION "1.0.0") set_target_properties(avrorouter PROPERTIES LINK_FLAGS -Wl,-z,defs) - target_link_libraries(avrorouter maxscale-common ${JANSSON_LIBRARIES} ${AVRO_LIBRARIES} maxavro sqlite3 lzma) + target_link_libraries(avrorouter maxscale-common ${JANSSON_LIBRARIES} ${AVRO_LIBRARIES} maxavro lzma) install_module(avrorouter core) else() message(STATUS "No Avro C or Jansson libraries found, not building avrorouter.") diff --git a/server/modules/routing/binlogrouter/CMakeLists.txt b/server/modules/routing/binlogrouter/CMakeLists.txt index d28ebae1a..a847e9792 100644 --- a/server/modules/routing/binlogrouter/CMakeLists.txt +++ b/server/modules/routing/binlogrouter/CMakeLists.txt @@ -1,11 +1,11 @@ add_library(binlogrouter SHARED blr.c blr_master.c blr_cache.c blr_slave.c blr_file.c) set_target_properties(binlogrouter PROPERTIES INSTALL_RPATH ${CMAKE_INSTALL_RPATH}:${MAXSCALE_LIBDIR} VERSION "2.0.0") set_target_properties(binlogrouter PROPERTIES LINK_FLAGS -Wl,-z,defs) -target_link_libraries(binlogrouter maxscale-common ${PCRE_LINK_FLAGS} uuid sqlite3) +target_link_libraries(binlogrouter maxscale-common ${PCRE_LINK_FLAGS} uuid) install_module(binlogrouter core) add_executable(maxbinlogcheck maxbinlogcheck.c blr_file.c blr_cache.c blr_master.c blr_slave.c blr.c) -target_link_libraries(maxbinlogcheck maxscale-common ${PCRE_LINK_FLAGS} uuid sqlite3) +target_link_libraries(maxbinlogcheck maxscale-common ${PCRE_LINK_FLAGS} uuid) install_executable(maxbinlogcheck core) diff --git a/server/modules/routing/binlogrouter/test/CMakeLists.txt b/server/modules/routing/binlogrouter/test/CMakeLists.txt index e93f9306a..d600418eb 100644 --- a/server/modules/routing/binlogrouter/test/CMakeLists.txt +++ b/server/modules/routing/binlogrouter/test/CMakeLists.txt @@ -1,5 +1,5 @@ if(BUILD_TESTS) add_executable(testbinlogrouter testbinlog.c ../blr.c ../blr_slave.c ../blr_master.c ../blr_file.c ../blr_cache.c) - target_link_libraries(testbinlogrouter maxscale-common ${PCRE_LINK_FLAGS} uuid sqlite3) + target_link_libraries(testbinlogrouter maxscale-common ${PCRE_LINK_FLAGS} uuid) add_test(NAME TestBinlogRouter COMMAND ./testbinlogrouter WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}) endif() From f9cc2d5bbbab0c571eb262a96d8fba63335bbb6a Mon Sep 17 00:00:00 2001 From: Timofey Turenko Date: Tue, 30 Jan 2018 15:48:05 +0200 Subject: [PATCH 22/54] use 'mdbci destroy' instead of 'vagrant destroy' (#163) use mdbci destroy instead of vagrant destroy --- BUILD/mdbci/build.sh | 12 ++---------- BUILD/mdbci/upgrade_test.sh | 12 +++--------- maxscale-system-test/mdbci/create_config.sh | 4 +--- maxscale-system-test/mdbci/destroy.sh | 10 ---------- maxscale-system-test/mdbci/run_test.sh | 7 +++---- maxscale-system-test/mdbci/run_test_snapshot.sh | 9 ++++----- 6 files changed, 13 insertions(+), 41 deletions(-) delete mode 100755 maxscale-system-test/mdbci/destroy.sh diff --git a/BUILD/mdbci/build.sh b/BUILD/mdbci/build.sh index abdb0f4b2..a18c16f92 100755 --- a/BUILD/mdbci/build.sh +++ b/BUILD/mdbci/build.sh @@ -66,9 +66,7 @@ $(<${script_dir}/templates/build.json.template) # destroying existing box if [ -d "$MDBCI_VM_PATH/${name}" ]; then - cd $MDBCI_VM_PATH/${name} - vagrant destroy -f - cd ${dir} + ${mdbci_dir}/mdbci destroy $name fi # starting VM for build @@ -78,9 +76,7 @@ $(<${script_dir}/templates/build.json.template) ${mdbci_dir}/mdbci up --attempts=1 $name if [ $? != 0 ] ; then echo "Error starting VM" - cd $MDBCI_VM_PATH/${name} rm ~/vagrant_lock - cd $dir exit 1 fi echo "copying public keys to VM" @@ -122,11 +118,7 @@ if [ "$try_already_running" == "yes" ] ; then fi if [[ "$do_not_destroy_vm" != "yes" && "$try_already_running" != "yes" ]] ; then echo "Destroying VM" - vagrant destroy -f - cd .. - rm -rf $name - rm -rf ${name}.json - rm -rf ${name}_netwotk_config + ${mdbci_dir}/mdbci destroy $name fi cd $dir diff --git a/BUILD/mdbci/upgrade_test.sh b/BUILD/mdbci/upgrade_test.sh index 9ac3efe1f..c30069dcb 100755 --- a/BUILD/mdbci/upgrade_test.sh +++ b/BUILD/mdbci/upgrade_test.sh @@ -30,9 +30,7 @@ echo $JOB_NAME-$BUILD_NUMBER >> ~/vagrant_lock # destroying existing box if [ -d "install_$box" ]; then - cd $MDBCI_VM_PATH/$name - vagrant destroy -f - cd $dir + ${mdbci_dir}/mdbci destroy $name fi ${mdbci_dir}/repository-config/generate_all.sh repo.d @@ -44,11 +42,9 @@ ${mdbci_dir}/mdbci up $name --attempts=1 if [ $? != 0 ] ; then if [ $? != 0 ] ; then echo "Error starting VM" - cd ${MDBCI_VM_PATH}/$name if [ "x$do_not_destroy_vm" != "xyes" ] ; then - vagrant destroy -f + ${mdbci_dir}/mdbci destroy $name fi - cd $dir rm ~/vagrant_lock exit 1 fi @@ -126,9 +122,7 @@ scp $scpopt $sshuser@$IP:/var/log/maxscale/* $logs_publish_dir chmod a+r $logs_publish_dir/* if [ "x$do_not_destroy_vm" != "xyes" ] ; then - cd $MDBCI_VM_PATH/$name - vagrant destroy -f - cd $dir + ${mdbci_dir}/mdbci destroy $name fi kill $pid_to_kill exit $res diff --git a/maxscale-system-test/mdbci/create_config.sh b/maxscale-system-test/mdbci/create_config.sh index 6f176e92f..12f899323 100755 --- a/maxscale-system-test/mdbci/create_config.sh +++ b/maxscale-system-test/mdbci/create_config.sh @@ -20,10 +20,8 @@ if [ "$product" == "mysql" ] ; then export cnf_path=${script_dir}/cnf/mysql56 fi +${mdbci_dir}/mdbci destroy $name mkdir -p ${MDBCI_VM_PATH}/$name -cd ${MDBCI_VM_PATH}/$name -vagrant destroy -f -cd $dir mkdir ${MDBCI_VM_PATH}/$name/cnf cp -r ${cnf_path}/* ${MDBCI_VM_PATH}/$name/cnf/ diff --git a/maxscale-system-test/mdbci/destroy.sh b/maxscale-system-test/mdbci/destroy.sh deleted file mode 100755 index d12ec0764..000000000 --- a/maxscale-system-test/mdbci/destroy.sh +++ /dev/null @@ -1,10 +0,0 @@ -#!/bin/bash - -dir=`pwd` -cd ${MDBCI_VM_PATH}/${name} -vagrant destroy -f -cd $dir - -rm -rf ${MDBCI_VM_PATH}/${name} -rm -rf ${MDBCI_VM_PATH}/${name}.json -rm -rf ${MDBCI_VM_PATH}/${name}_network_config diff --git a/maxscale-system-test/mdbci/run_test.sh b/maxscale-system-test/mdbci/run_test.sh index c74ab8dc9..609d35e7c 100755 --- a/maxscale-system-test/mdbci/run_test.sh +++ b/maxscale-system-test/mdbci/run_test.sh @@ -63,7 +63,6 @@ res=$? ulimit -c unlimited if [ $res == 0 ] ; then -# . ${script_dir}/configure_backend.sh . ${script_dir}/set_env.sh $name cd ${script_dir}/.. cmake . -DBUILDNAME=$name -DCMAKE_BUILD_TYPE=Debug @@ -76,7 +75,7 @@ if [ $res == 0 ] ; then if [ $? != 0 ]; then echo "Backend broken!" if [ "${do_not_destroy_vm}" != "yes" ] ; then - ${script_dir}/destroy.sh + ${mdbci_dir}/mdbci destroy $name fi rm ~/vagrant_lock exit 1 @@ -90,13 +89,13 @@ if [ $res == 0 ] ; then else echo "Failed to create VMs, exiting" if [ "${do_not_destroy_vm}" != "yes" ] ; then - ${script_dir}/destroy.sh + ${mdbci_dir}/mdbci destroy $name fi rm ~/vagrant_lock exit 1 fi if [ "${do_not_destroy_vm}" != "yes" ] ; then - ${script_dir}/destroy.sh + ${mdbci_dir}/mdbci destroy $name echo "clean up done!" fi diff --git a/maxscale-system-test/mdbci/run_test_snapshot.sh b/maxscale-system-test/mdbci/run_test_snapshot.sh index 77d319121..cff4ce3fe 100755 --- a/maxscale-system-test/mdbci/run_test_snapshot.sh +++ b/maxscale-system-test/mdbci/run_test_snapshot.sh @@ -48,15 +48,14 @@ export repo_dir=$dir/repo.d/ ${mdbci_dir}/mdbci snapshot revert --path-to-nodes $name --snapshot-name $snapshot_name if [ $? != 0 ]; then - ${script_dir}/destroy.sh + ${mdbci_dir}/mdbci destroy $name ${MDBCI_VM_PATH}/scripts/clean_vms.sh $name ${script_dir}/create_config.sh - checkExitStatus $? "Error creating configuration" $snapshot_lock_file - . ${script_dir}/configure_backend.sh - + checkExitStatus $? "Error creating configuration" $snapshot_lock_file + echo "Creating snapshot from new config" - $HOME/mdbci/mdbci snapshot take --path-to-nodes $name --snapshot-name $snapshot_name + ${mdbci_dir}/mdbci snapshot take --path-to-nodes $name --snapshot-name $snapshot_name fi . ${script_dir}/set_env.sh "$name" From 8dfb1d0113865d20325ea2790b57039504cde38a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 25 Jan 2018 11:39:03 +0200 Subject: [PATCH 23/54] MXS-1621: Add ALTER TABLE ... [FIRST | AFTER `col` ] parsing The parser checks whether the FIRST or AFTER keywords are used and, if AFTER is used, extracts the relevant column name. Added a test case that checks that the parsing works and detects the correct column names. --- .../modules/routing/avrorouter/CMakeLists.txt | 4 + server/modules/routing/avrorouter/avro_file.c | 3 +- .../modules/routing/avrorouter/avro_schema.c | 105 +++++++++++++++++- .../routing/avrorouter/test/CMakeLists.txt | 3 + .../avrorouter/test/test_alter_parsing.c | 99 +++++++++++++++++ 5 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 server/modules/routing/avrorouter/test/CMakeLists.txt create mode 100644 server/modules/routing/avrorouter/test/test_alter_parsing.c diff --git a/server/modules/routing/avrorouter/CMakeLists.txt b/server/modules/routing/avrorouter/CMakeLists.txt index d852862bb..d23a4e986 100644 --- a/server/modules/routing/avrorouter/CMakeLists.txt +++ b/server/modules/routing/avrorouter/CMakeLists.txt @@ -6,6 +6,10 @@ if(AVRO_FOUND AND JANSSON_FOUND) set_target_properties(avrorouter PROPERTIES LINK_FLAGS -Wl,-z,defs) target_link_libraries(avrorouter maxscale-common ${JANSSON_LIBRARIES} ${AVRO_LIBRARIES} maxavro sqlite3 lzma) install_module(avrorouter core) + + if (BUILD_TESTS) + add_subdirectory(test) + endif() else() message(STATUS "No Avro C or Jansson libraries found, not building avrorouter.") endif() diff --git a/server/modules/routing/avrorouter/avro_file.c b/server/modules/routing/avrorouter/avro_file.c index f7bcdaa09..793376e07 100644 --- a/server/modules/routing/avrorouter/avro_file.c +++ b/server/modules/routing/avrorouter/avro_file.c @@ -1040,12 +1040,13 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra db[dblen] = 0; size_t sqlsz = len, tmpsz = len; - char *tmp = MXS_MALLOC(len); + char *tmp = MXS_MALLOC(len + 1); MXS_ABORT_IF_NULL(tmp); remove_mysql_comments((const char**)&sql, &sqlsz, &tmp, &tmpsz); sql = tmp; len = tmpsz; unify_whitespace(sql, len); + sql[len] = '\0'; static bool warn_not_row_format = true; diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index 24fe0adf8..610c27480 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -930,7 +930,6 @@ static void remove_extras(char* str) ss_dassert(strlen(str) == len); } - static void remove_backticks(char* src) { char* dest = src; @@ -1134,6 +1133,110 @@ static const char* get_tok(const char* sql, int* toklen, const char* end) return NULL; } +static void rskip_whitespace(const char* sql, const char** end) +{ + const char* ptr = *end; + + while (ptr > sql && isspace(*ptr)) + { + ptr--; + } + + *end = ptr; +} + +static void rskip_token(const char* sql, const char** end) +{ + const char* ptr = *end; + + while (ptr > sql && !isspace(*ptr)) + { + ptr--; + } + + *end = ptr; +} + +static bool get_placement_specifier(const char* sql, const char* end, const char** tgt, int* tgt_len) +{ + bool rval = false; + ss_dassert(end > sql); + end--; + + *tgt = NULL; + *tgt_len = 0; + + // Skip any trailing whitespace + rskip_whitespace(sql, &end); + + if (*end == '`') + { + // Identifier, possibly AFTER `column` + const char* id_end = end; + end--; + + while (end > sql && *end != '`') + { + end--; + } + + const char* id_start = end + 1; + ss_dassert(*end == '`' && *id_end == '`'); + + end--; + + rskip_whitespace(sql, &end); + rskip_token(sql, &end); + + // end points to the character _before_ the token + end++; + + if (strncasecmp(end, "AFTER", 5) == 0) + { + // This column comes after the specified column + rval = true; + *tgt = id_start; + *tgt_len = id_end - id_start; + } + } + else + { + // Something else, possibly FIRST or un-backtick'd AFTER + const char* id_end = end + 1; // Points to either a trailing space or one-after-the-end + rskip_token(sql, &end); + + // end points to the character _before_ the token + end++; + + if (strncasecmp(end, "FIRST", 5) == 0) + { + // Put this column first + rval = true; + } + else + { + const char* id_start = end + 1; + + // Skip the whitespace and until the start of the current token + rskip_whitespace(sql, &end); + rskip_token(sql, &end); + + // end points to the character _before_ the token + end++; + + if (strncasecmp(end, "AFTER", 5) == 0) + { + // This column comes after the specified column + rval = true; + *tgt = id_start; + *tgt_len = id_end - id_start; + } + } + } + + return rval; +} + static bool tok_eq(const char *a, const char *b, size_t len) { size_t i = 0; diff --git a/server/modules/routing/avrorouter/test/CMakeLists.txt b/server/modules/routing/avrorouter/test/CMakeLists.txt new file mode 100644 index 000000000..016461f38 --- /dev/null +++ b/server/modules/routing/avrorouter/test/CMakeLists.txt @@ -0,0 +1,3 @@ +add_executable(test_alter_parsing test_alter_parsing.c) +target_link_libraries(test_alter_parsing maxscale-common ${JANSSON_LIBRARIES} ${AVRO_LIBRARIES} maxavro sqlite3 lzma) +add_test(test_alter_parsing test_alter_parsing) \ No newline at end of file diff --git a/server/modules/routing/avrorouter/test/test_alter_parsing.c b/server/modules/routing/avrorouter/test/test_alter_parsing.c new file mode 100644 index 000000000..c622d6e75 --- /dev/null +++ b/server/modules/routing/avrorouter/test/test_alter_parsing.c @@ -0,0 +1,99 @@ +#include "../avro_schema.c" + +static struct +{ + const char* statement; + const char* target; + bool rval; +} data[] = +{ + {"/*!40000 ALTER TABLE `t1` DISABLE KEYS */", NULL, false}, + {"/*!40000 ALTER TABLE `t1` ENABLE KEYS */", NULL, false}, + {"ADD COLUMN `a` INT", NULL, false}, + {"ADD COLUMN `a`", NULL, false}, + {"ALTER TABLE `t1` ADD `account_id` INT", NULL, false}, + {"ALTER TABLE `t1` ADD `amount` INT", NULL, false}, + {"ALTER TABLE `t1` ADD `app_id` VARCHAR(64)", NULL, false}, + {"ALTER TABLE `t1` ADD `create_time` DATETIME", NULL, false}, + {"alter TABLE t1 add `end_time` varchar(10) DEFAULT NULL COMMENT 'this is a comment'", NULL, false}, + {"ALTER TABLE `t1` ADD `expire_time` DATETIME", NULL, false}, + {"ALTER TABLE `t1` ADD `id_a` VARCHAR(128)", NULL, false}, + {"ALTER TABLE `t1` ADD `id` BIGINT(20)", NULL, false}, + {"ALTER TABLE `t1` ADD `id` VARCHAR(64)", NULL, false}, + {"ALTER TABLE `t1` ADD `node_state` INT(4)", NULL, false}, + {"ALTER TABLE `t1` ADD `no` INT", NULL, false}, + {"ALTER TABLE `t1` ADD `order_id` INT", NULL, false}, + {"alter TABLE t1 add `start_time` varchar(10) DEFAULT NULL COMMENT 'this is a comment'", NULL, false}, + {"ALTER TABLE `t1` ADD `status` INT", NULL, false}, + {"ALTER TABLE `t1` ADD `task_id` BIGINT(20)", NULL, false}, + {"alter TABLE t1 add `undo` int(1) DEFAULT '0' COMMENT 'this is a comment'", NULL, false}, + {"alter table `t1` add unique (`a`,`id`)", NULL, false}, + {"alter table `t1` add unique (`a`)", NULL, false}, + {"alter table `t1` add UNIQUE(`a`)", NULL, false}, + {"ALTER TABLE `t1` ADD UNIQUE `idx_id` USING BTREE (`id`, `result`)", NULL, false}, + {"ALTER TABLE `t1` ADD `update_time` INT", NULL, false}, + {"ALTER TABLE `t1` ADD `username` VARCHAR(16)", NULL, false}, + {"ALTER TABLE `t1` AUTO_INCREMENT = 1", NULL, false}, + {"ALTER TABLE `t1` CHANGE `account_id` `account_id` BIGINT(20)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `amount` `amount` DECIMAL(32,2)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `app_id` `app_id` VARCHAR(64)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `business_id` `business_id` VARCHAR(128)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `business_id` `business_id` VARCHAR(64)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `business_unique_no` `business_unique_no` VARCHAR(64)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `expire_time` `expire_time` DATETIME", NULL, false}, + {"ALTER TABLE `t1` CHANGE `id_a` `id_a` VARCHAR(128)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `id` `id` BIGINT(20)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `node_state` `node_state` INT(4)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `order_id` `order_id` BIGINT(20)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `status` `status` INT(1)", NULL, false}, + {"ALTER TABLE `t1` CHANGE `update_time` `update_time` TIMESTAMP", NULL, false}, + {"ALTER TABLE `t1` CHANGE `username` `username` VARCHAR(16)", NULL, false}, + {"ALTER TABLE `t1` COMMENT = 'a comment'", NULL, false}, + {"alter table `t1` drop index a", NULL, false}, + {"alter table t1 drop index t1_idx", NULL, false}, + {"alter table t1 index(account_id, business_id)", NULL, false}, + {"ALTER TABLE `t1` MODIFY COLUMN `expire_time` DATETIME DEFAULT NULL COMMENT 'this is a comment' AFTER `update_time`", "update_time", true}, + {"ALTER TABLE `t1` MODIFY COLUMN `id_a` VARCHAR(128) CHARACTER SET utf8 COLLATE utf8_general_ci COMMENT 'this is a comment' AFTER `username`", "username", true}, + {"ALTER TABLE `t1` MODIFY COLUMN `number` VARCHAR(64) CHARACTER SET utf8 COLLATE utf8_general_ci DEFAULT NULL COMMENT 'this is a comment' AFTER `business_id`", "business_id", true}, + {"ALTER TABLE `t1` MODIFY COLUMN `task_id` BIGINT(20) DEFAULT NULL COMMENT 'this is a comment' AFTER `business_id`", "business_id", true}, + {"ALTER TABLE `t1` MODIFY COLUMN `username` VARCHAR(16) CHARACTER SET utf8 COLLATE utf8_general_ci NOT NULL COMMENT 'this is a comment' AFTER `business_id`", "business_id", true}, + {"ALTER TABLE `t1` RENAME `t2`", NULL, false}, + {"ALTER TABLE `db1`.`t1` ADD COLUMN `num` varchar(32) COMMENT 'this is a comment' AFTER `bank_name`", "bank_name", true}, + {"ALTER TABLE `db1`.`t1` ADD INDEX `idx_node_state` USING BTREE (`node_state`) comment ''", NULL, false}, + {"ALTER TABLE `db1`.`t1` CHANGE COLUMN `num` `code` varchar(32) DEFAULT NULL COMMENT 'this is a comment'", NULL, false}, + {"ALTER TABLE `db1`.`t1` DROP INDEX `a`, ADD INDEX `a` USING BTREE (`a`) comment ''", NULL, false}, + {"ALTER TABLE `db1`.`t1` DROP INDEX `a`, ADD INDEX `idx_a` USING BTREE (`a`) comment ''", NULL, false}, + {"ALTER TABLE `t1` CHANGE COLUMN `a` `c` INT AFTER `b`", "b", true}, + {"ALTER TABLE `t1` CHANGE COLUMN `a` `c` INT first", NULL, true}, + {"ALTER TABLE `t1` CHANGE COLUMN `a` `c` INT", NULL, false}, + {"ALTER TABLE `t1` MODIFY COLUMN `a` INT PRIMARY KEY", NULL, false}, + {NULL} +}; + +int main(int argc, char** argv) +{ + int rval = 0; + + for (int i = 0; data[i].statement; i++) + { + const char* target = NULL; + int len = 0; + const char* stmt = data[i].statement; + const char* end = data[i].statement + strlen(data[i].statement); + + if (get_placement_specifier(stmt, end, &target, &len) != data[i].rval) + { + const char* a = data[i].rval ? "true" : "false"; + const char* b = data[i].rval ? "false" : "true"; + printf("Expected '%s', got '%s' for '%s'\n", a, b, data[i].statement); + rval++; + } + else if (((bool)data[i].target != (bool)target) || (strncmp(target, data[i].target, len) != 0)) + { + printf("Expected '%s', got '%.*s' for '%s'\n", data[i].target, len, target, data[i].statement); + rval++; + } + } + + return rval; +} From c000b3186c61c8cd4398806398837067d7076420 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 25 Jan 2018 12:49:21 +0200 Subject: [PATCH 24/54] MXS-1575: Fix optional COLUMN keywork handling The COLUMN keyword is optional and cannot be assumed to exist. --- server/modules/routing/avrorouter/avro_schema.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index 610c27480..8fcfd1b94 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -1334,9 +1334,14 @@ bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end) if (tok) { - if (tok_eq(ptok, "add", plen) && tok_eq(tok, "column", len)) + if (tok_eq(tok, "column", len)) { + // Skip the optional COLUMN keyword tok = get_tok(tok + len, &len, end); + } + + if (tok_eq(ptok, "add", plen)) + { char avro_token[len + 1]; make_avro_token(avro_token, tok, len); bool is_new = true; @@ -1367,10 +1372,8 @@ bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end) tok = get_next_def(tok, end); len = 0; } - else if (tok_eq(ptok, "drop", plen) && tok_eq(tok, "column", len)) + else if (tok_eq(ptok, "drop", plen)) { - tok = get_tok(tok + len, &len, end); - int idx = get_column_index(create, tok, len); if (idx != -1) @@ -1394,10 +1397,8 @@ bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end) tok = get_next_def(tok, end); len = 0; } - else if (tok_eq(ptok, "change", plen) && tok_eq(tok, "column", len)) + else if (tok_eq(ptok, "change", plen)) { - tok = get_tok(tok + len, &len, end); - int idx = get_column_index(create, tok, len); if (idx != -1 && (tok = get_tok(tok + len, &len, end))) From b7e475f316c92fb0e4325c118dc1156d79cbeb82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 25 Jan 2018 16:37:49 +0200 Subject: [PATCH 25/54] =?UTF-8?q?MXS-1621:=20Detect=20TABLE=5FMAP=20?= =?UTF-8?q?=E2=86=94=20TABLE=5FCREATE=20column=20count=20mismatch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the TABLE_MAP and TABLE_CREATE have different column counts, an error is logged and the row events are skipped. --- server/core/mysql_binlog.c | 1 + server/modules/routing/avrorouter/avro_rbr.c | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/server/core/mysql_binlog.c b/server/core/mysql_binlog.c index 33f6dd395..86ea4065b 100644 --- a/server/core/mysql_binlog.c +++ b/server/core/mysql_binlog.c @@ -649,6 +649,7 @@ size_t unpack_numeric_field(uint8_t *src, uint8_t type, uint8_t *metadata, uint8 break; } + ss_dassert(size > 0); memcpy(dest, src, size); return size; } diff --git a/server/modules/routing/avrorouter/avro_rbr.c b/server/modules/routing/avrorouter/avro_rbr.c index 8af455076..d3b5c6a87 100644 --- a/server/modules/routing/avrorouter/avro_rbr.c +++ b/server/modules/routing/avrorouter/avro_rbr.c @@ -210,6 +210,7 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) { bool rval = false; uint8_t *start = ptr; + uint8_t *end = ptr + hdr->event_size - BINLOG_EVENT_HDR_LEN; uint8_t table_id_size = router->event_type_hdr_lens[hdr->event_type] == 6 ? 4 : 6; uint64_t table_id = 0; @@ -274,8 +275,9 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) snprintf(table_ident, sizeof(table_ident), "%s.%s", map->database, map->table); AVRO_TABLE* table = hashtable_fetch(router->open_tables, table_ident); TABLE_CREATE* create = map->table_create; + ss_dassert(hashtable_fetch(router->created_tables, table_ident) == create); - if (table && create && ncolumns == map->columns) + if (table && create && ncolumns == map->columns && create->columns == map->columns) { avro_value_t record; avro_generic_value_new(table->avro_writer_iface, &record); @@ -292,7 +294,6 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) MXS_INFO("Row %lu", total_row_count++); /** Add the current GTID and timestamp */ - uint8_t *end = ptr + hdr->event_size - BINLOG_EVENT_HDR_LEN; int event_type = get_event_type(hdr->event_type); prepare_record(router, hdr, event_type, &record); ptr = process_row_event_data(map, create, &record, ptr, col_present, end); @@ -334,6 +335,12 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) "binary logs or the stored schema was not correct.", map->database, map->table); } + else if (ncolumns == map->columns && create->columns != map->columns) + { + MXS_ERROR("Table map event has a different column " + "count for table %s.%s than the CREATE TABLE statement.", + map->database, map->table); + } else { MXS_ERROR("Row event and table map event have different column " From 9146a215f7d1dc1e9508c8ba419899a12d9fa566 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Jan 2018 13:50:06 +0200 Subject: [PATCH 26/54] Fix DDL table identifier parsing The parsing was inadequate and did not support all forms of quoted identifiers. --- server/modules/routing/avrorouter/avro_file.c | 30 +-- .../modules/routing/avrorouter/avro_schema.c | 173 ++++++++++++++---- .../modules/routing/avrorouter/avrorouter.h | 4 +- 3 files changed, 142 insertions(+), 65 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_file.c b/server/modules/routing/avrorouter/avro_file.c index 793376e07..dffb375d4 100644 --- a/server/modules/routing/avrorouter/avro_file.c +++ b/server/modules/routing/avrorouter/avro_file.c @@ -1067,6 +1067,9 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra } } + char ident[MYSQL_TABLE_MAXLEN + MYSQL_DATABASE_MAXLEN + 2]; + read_table_identifier(db, sql, sql + len, ident, sizeof(ident)); + if (is_create_table_statement(router, sql, len)) { TABLE_CREATE *created = NULL; @@ -1086,7 +1089,7 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra } else { - created = table_create_alloc(sql, len, db); + created = table_create_alloc(ident, sql, len, db); } if (created && !save_and_replace_table_create(router, created)) @@ -1096,30 +1099,7 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra } else if (is_alter_table_statement(router, sql, len)) { - char ident[MYSQL_TABLE_MAXLEN + MYSQL_DATABASE_MAXLEN + 2]; - read_alter_identifier(sql, sql + len, ident, sizeof(ident)); - - bool combine = (strnlen(db, 1) && strchr(ident, '.') == NULL); - - size_t ident_len = strlen(ident) + 1; // + 1 for the NULL - - if (combine) - { - ident_len += (strlen(db) + 1); // + 1 for the "." - } - - char full_ident[ident_len]; - full_ident[0] = 0; // Set full_ident to "". - - if (combine) - { - strcat(full_ident, db); - strcat(full_ident, "."); - } - - strcat(full_ident, ident); - - TABLE_CREATE *created = hashtable_fetch(router->created_tables, full_ident); + TABLE_CREATE *created = hashtable_fetch(router->created_tables, ident); if (created) { diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index 8fcfd1b94..efc1bb832 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -720,46 +720,28 @@ int resolve_table_version(const char* db, const char* table) /** * @brief Handle a query event which contains a CREATE TABLE statement - * @param sql Query SQL - * @param db Database where this query was executed + * + * @param ident Table identifier in database.table format + * @param sql The CREATE TABLE statement + * @param len Length of @c sql + * * @return New CREATE_TABLE object or NULL if an error occurred */ -TABLE_CREATE* table_create_alloc(const char* sql, int len, const char* db) +TABLE_CREATE* table_create_alloc(const char* ident, const char* sql, int len) { /** Extract the table definition so we can get the column names from it */ int stmt_len = 0; const char* statement_sql = get_table_definition(sql, len, &stmt_len); ss_dassert(statement_sql); + + char* tbl_start = strchr(ident, '.'); + ss_dassert(tbl_start); + *tbl_start++ = '\0'; + char table[MYSQL_TABLE_MAXLEN + 1]; char database[MYSQL_DATABASE_MAXLEN + 1]; - const char* err = NULL; - MXS_INFO("Create table: %.*s", len, sql); - - if (!statement_sql) - { - err = "table definition"; - } - else if (!get_table_name(sql, table)) - { - err = "table name"; - } - - if (get_database_name(sql, database)) - { - // The CREATE statement contains the database name - db = database; - } - else if (*db == '\0') - { - // No explicit or current database - err = "database name"; - } - - if (err) - { - MXS_ERROR("Malformed CREATE TABLE statement, could not extract %s: %.*s", err, len, sql); - return NULL; - } + strcpy(database, ident); + strcpy(table, tbl_start); int* lengths = NULL; char **names = NULL; @@ -1253,15 +1235,130 @@ static bool tok_eq(const char *a, const char *b, size_t len) return true; } -void read_alter_identifier(const char *sql, const char *end, char *dest, int size) +static void skip_whitespace(const char** saved) { - int len = 0; - const char *tok = get_tok(sql, &len, end); // ALTER - if (tok && (tok = get_tok(tok + len, &len, end)) // TABLE - && (tok = get_tok(tok + len, &len, end))) // Table identifier + const char* ptr = *saved; + + while (*ptr && isspace(*ptr)) { - snprintf(dest, size, "%.*s", len, tok); - remove_backticks(dest); + ptr++; + } + + *saved = ptr; +} + +static void skip_token(const char** saved) +{ + const char* ptr = *saved; + + while (*ptr && !isspace(*ptr)) + { + ptr++; + } + + *saved = ptr; +} + +static void skip_non_backtick(const char** saved) +{ + const char* ptr = *saved; + + while (*ptr && *ptr != '`') + { + ptr++; + } + + *saved = ptr; +} + +const char* keywords[] = +{ + "CREATE", + "DROP", + "ALTER", + "IF", + "EXISTS", + "REPLACE", + "OR", + "TABLE", + "NOT", + NULL +}; + +static bool token_is_keyword(const char* tok, int len) +{ + for (int i = 0; keywords[i]; i++) + { + if (strncasecmp(keywords[i], tok, len) == 0) + { + return true; + } + } + + return false; +} + +void read_table_identifier(const char* db, const char *sql, const char *end, char *dest, int size) +{ + const char* start; + int len = 0; + bool is_keyword = true; + + while (is_keyword) + { + skip_whitespace(&sql); // Leading whitespace + + if (*sql == '`') + { + // Quoted identifier, not a keyword + is_keyword = false; + sql++; + start = sql; + skip_non_backtick(&sql); + len = sql - start; + sql++; + } + else + { + start = sql; + skip_token(&sql); + len = sql - start; + is_keyword = token_is_keyword(start, len); + } + } + + skip_whitespace(&sql); // Space after first identifier + + if (*sql != '.') + { + // No explicit database + snprintf(dest, size, "%s.%.*s", db, len, start); + } + else + { + // Explicit database, skip the period + sql++; + skip_whitespace(&sql); // Space after first identifier + + const char* id_start; + int id_len = 0; + + if (*sql == '`') + { + sql++; + id_start = sql; + skip_non_backtick(&sql); + id_len = sql - id_start; + sql++; + } + else + { + id_start = sql; + skip_token(&sql); + id_len = sql - id_start; + } + + snprintf(dest, size, "%.*s.%.*s", len, start, id_len, id_start); } } diff --git a/server/modules/routing/avrorouter/avrorouter.h b/server/modules/routing/avrorouter/avrorouter.h index 9f4ebb1a5..02f1b8d6c 100644 --- a/server/modules/routing/avrorouter/avrorouter.h +++ b/server/modules/routing/avrorouter/avrorouter.h @@ -302,12 +302,12 @@ extern void read_table_info(uint8_t *ptr, uint8_t post_header_len, uint64_t *tab char* dest, size_t len); extern TABLE_MAP *table_map_alloc(uint8_t *ptr, uint8_t hdr_len, TABLE_CREATE* create); extern void table_map_free(TABLE_MAP *map); -extern TABLE_CREATE* table_create_alloc(const char* sql, int len, const char* db); +extern TABLE_CREATE* table_create_alloc(const char* ident, const char* sql, int len, const char* db); extern TABLE_CREATE* table_create_copy(AVRO_INSTANCE *router, const char* sql, size_t len, const char* db); extern void table_create_free(TABLE_CREATE* value); extern bool table_create_save(TABLE_CREATE *create, const char *filename); extern bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end); -extern void read_alter_identifier(const char *sql, const char *end, char *dest, int size); +extern void read_table_identifier(const char* db, const char *sql, const char *end, char *dest, int size); extern int avro_client_handle_request(AVRO_INSTANCE *, AVRO_CLIENT *, GWBUF *); extern void avro_client_rotate(AVRO_INSTANCE *router, AVRO_CLIENT *client, uint8_t *ptr); extern bool avro_open_binlog(const char *binlogdir, const char *file, int *fd); From df86ee35791143d29625c9d87ca4f3a25f972f5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Jan 2018 14:02:53 +0200 Subject: [PATCH 27/54] Fix buffer overflow assertions The buffer overflow assertions were off by one: The data pointer can be equal to the end pointer when the last column of the row is processed. --- server/modules/routing/avrorouter/avro_rbr.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_rbr.c b/server/modules/routing/avrorouter/avro_rbr.c index d3b5c6a87..4c506f284 100644 --- a/server/modules/routing/avrorouter/avro_rbr.c +++ b/server/modules/routing/avrorouter/avro_rbr.c @@ -564,7 +564,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value avro_value_set_string(&field, strval); sprintf(trace[i], "[%ld] ENUM: %lu bytes", i, bytes); ptr += bytes; - check_overflow(ptr < end); + check_overflow(ptr <= end); } else { @@ -600,7 +600,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value str[bytes] = '\0'; avro_value_set_string(&field, str); ptr += bytes; - check_overflow(ptr < end); + check_overflow(ptr <= end); } } else if (column_is_bit(map->column_types[i])) @@ -619,7 +619,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value avro_value_set_int(&field, value); sprintf(trace[i], "[%ld] BIT", i); ptr += bytes; - check_overflow(ptr < end); + check_overflow(ptr <= end); } else if (column_is_decimal(map->column_types[i])) { @@ -627,7 +627,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value ptr += unpack_decimal_field(ptr, metadata + metadata_offset, &f_value); avro_value_set_double(&field, f_value); sprintf(trace[i], "[%ld] DECIMAL", i); - check_overflow(ptr < end); + check_overflow(ptr <= end); } else if (column_is_variable_string(map->column_types[i])) { @@ -650,7 +650,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value buf[sz] = '\0'; ptr += sz; avro_value_set_string(&field, buf); - check_overflow(ptr < end); + check_overflow(ptr <= end); } else if (column_is_blob(map->column_types[i])) { @@ -669,7 +669,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value uint8_t nullvalue = 0; avro_value_set_bytes(&field, &nullvalue, 1); } - check_overflow(ptr < end); + check_overflow(ptr <= end); } else if (column_is_temporal(map->column_types[i])) { @@ -681,7 +681,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value format_temporal_value(buf, sizeof(buf), map->column_types[i], &tm); avro_value_set_string(&field, buf); sprintf(trace[i], "[%ld] %s: %s", i, column_type_to_string(map->column_types[i]), buf); - check_overflow(ptr < end); + check_overflow(ptr <= end); } /** All numeric types (INT, LONG, FLOAT etc.) */ else @@ -692,7 +692,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value &metadata[metadata_offset], lval); set_numeric_field_value(&field, map->column_types[i], &metadata[metadata_offset], lval); sprintf(trace[i], "[%ld] %s", i, column_type_to_string(map->column_types[i])); - check_overflow(ptr < end); + check_overflow(ptr <= end); } ss_dassert(metadata_offset <= map->column_metadata_size); metadata_offset += get_metadata_len(map->column_types[i]); From e14710ab2bfd073ad31b394cb02a8e89a2514e63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 26 Jan 2018 14:04:19 +0200 Subject: [PATCH 28/54] Fix ALTER TABLE detection regex The regular expression expected that the COLUMN keyword was always present. --- server/modules/routing/avrorouter/avro.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/modules/routing/avrorouter/avro.c b/server/modules/routing/avrorouter/avro.c index 2932b19b6..e9458c764 100644 --- a/server/modules/routing/avrorouter/avro.c +++ b/server/modules/routing/avrorouter/avro.c @@ -65,7 +65,7 @@ static const char* avro_index_name = "avro.index"; static const char* create_table_regex = "(?i)create[a-z0-9[:space:]_]+table"; static const char* alter_table_regex = - "(?i)alter[[:space:]]+table.*column"; + "(?i)alter[[:space:]]+table"; /* The router entry points */ static MXS_ROUTER *createInstance(SERVICE *service, char **options); From 93923acafb7b8dc6716a200deefde19e4a87fa99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 29 Jan 2018 14:05:52 +0200 Subject: [PATCH 29/54] MXS-1621: Skip unneeded ALTER TABLE operations Some ALTER TABLE operations (e.g. ADD INDEX) are not useful to the avrorouter. These need to be detected and skipped. --- server/modules/routing/avrorouter/avro_file.c | 37 +++++++++++++++- server/modules/routing/avrorouter/avro_rbr.c | 6 +-- .../modules/routing/avrorouter/avro_schema.c | 44 ++++++++++++++++++- 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_file.c b/server/modules/routing/avrorouter/avro_file.c index dffb375d4..ee9fd04f2 100644 --- a/server/modules/routing/avrorouter/avro_file.c +++ b/server/modules/routing/avrorouter/avro_file.c @@ -1021,6 +1021,40 @@ void unify_whitespace(char *sql, int len) } } +/** + * A very simple function for stripping auto-generated executable comments + * + * Note that the string will not strip the trailing part of the comment, making + * the SQL invalid. + * + * @param sql String to modify + * @param len Pointer to current length of string, updated to new length if + * @c sql is modified + */ +static void strip_executable_comments(char *sql, int* len) +{ + if (strncmp(sql, "/*!", 3) == 0 || strncmp(sql, "/*M!", 4) == 0) + { + // Executable comment, remove it + char* p = sql + 3; + if (*p == '!') + { + p++; + } + + // Skip the versioning part + while (*p && isdigit(*p)) + { + p++; + } + + int n_extra = p - sql; + int new_len = *len - n_extra; + memmove(sql, sql + n_extra, new_len); + *len = new_len; + } +} + /** * @brief Handling of query events * @@ -1046,6 +1080,7 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra sql = tmp; len = tmpsz; unify_whitespace(sql, len); + strip_executable_comments(sql, &len); sql[len] = '\0'; static bool warn_not_row_format = true; @@ -1107,7 +1142,7 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra } else { - MXS_ERROR("Alter statement to a table with no create statement."); + MXS_ERROR("Alter statement to table '%s' has no preceding create statement.", ident); } } /* A transaction starts with this event */ diff --git a/server/modules/routing/avrorouter/avro_rbr.c b/server/modules/routing/avrorouter/avro_rbr.c index 4c506f284..4b9f5d177 100644 --- a/server/modules/routing/avrorouter/avro_rbr.c +++ b/server/modules/routing/avrorouter/avro_rbr.c @@ -337,9 +337,9 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) } else if (ncolumns == map->columns && create->columns != map->columns) { - MXS_ERROR("Table map event has a different column " - "count for table %s.%s than the CREATE TABLE statement.", - map->database, map->table); + MXS_ERROR("Table map event has a different column count for table " + "%s.%s than the CREATE TABLE statement. Possible " + "unsupported DDL detected.", map->database, map->table); } else { diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index efc1bb832..aaacc4a32 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -1392,6 +1392,14 @@ int get_column_index(TABLE_CREATE *create, const char *tok, int len) char safe_tok[len + 2]; memcpy(safe_tok, tok, len); safe_tok[len] = '\0'; + + if (*safe_tok == '`') + { + int toklen = strlen(safe_tok) - 2; // Token length without backticks + memmove(safe_tok, safe_tok + 1, toklen); // Overwrite first backtick + safe_tok[toklen] = '\0'; // Null-terminate the string before the second backtick + } + fix_reserved_word(safe_tok); for (int x = 0; x < create->columns; x++) @@ -1406,6 +1414,35 @@ int get_column_index(TABLE_CREATE *create, const char *tok, int len) return idx; } +static bool not_column_operation(const char* tok, int len) +{ + const char* keywords[] = + { + "PRIMARY", + "UNIQUE", + "FULLTEXT", + "SPATIAL", + "PERIOD", + "PRIMARY", + "KEY", + "KEYS", + "INDEX", + "FOREIGN", + "CONSTRAINT", + NULL + }; + + for (int i = 0; keywords[i]; i++) + { + if (tok_eq(tok, keywords[i], strlen(keywords[i]))) + { + return true; + } + } + + return false; +} + bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end) { const char *tbl = strcasestr(sql, "table"), *def; @@ -1431,7 +1468,12 @@ bool table_create_alter(TABLE_CREATE *create, const char *sql, const char *end) if (tok) { - if (tok_eq(tok, "column", len)) + if (not_column_operation(tok, len)) + { + MXS_INFO("Statement doesn't affect columns, not processing: %s", sql); + return true; + } + else if (tok_eq(tok, "column", len)) { // Skip the optional COLUMN keyword tok = get_tok(tok + len, &len, end); From 66ec4792cd571b56578d1837583f750cb83a258a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 29 Jan 2018 17:01:20 +0200 Subject: [PATCH 30/54] MXS-1575: Fix DATETIME handling DATETIME values in old formats should always be 8 bytes long. This is how MariaDB 10.2 stores them and only DATETIME2 values are stored with a fractional part. --- server/core/mysql_binlog.c | 55 +++++++++----------------------------- 1 file changed, 13 insertions(+), 42 deletions(-) diff --git a/server/core/mysql_binlog.c b/server/core/mysql_binlog.c index 86ea4065b..fbd07179c 100644 --- a/server/core/mysql_binlog.c +++ b/server/core/mysql_binlog.c @@ -262,47 +262,18 @@ static void unpack_datetime(uint8_t *ptr, int length, struct tm *dest) uint64_t val = 0; uint32_t second, minute, hour, day, month, year; - if (length == -1) - { - val = gw_mysql_get_byte8(ptr); - second = val - ((val / 100) * 100); - val /= 100; - minute = val - ((val / 100) * 100); - val /= 100; - hour = val - ((val / 100) * 100); - val /= 100; - day = val - ((val / 100) * 100); - val /= 100; - month = val - ((val / 100) * 100); - val /= 100; - year = val; - } - else - { - // TODO: Figure out why DATETIME(0) doesn't work like it others do - val = unpack_bytes(ptr, datetime_sizes[length]); - val *= log_10_values[6 - length]; - - if (val < 0) - { - val = -val; - } - - int subsecond = val % 1000000; - val /= 1000000; - - second = val % 60; - val /= 60; - minute = val % 60; - val /= 60; - hour = val % 24; - val /= 24; - day = val % 32; - val /= 32; - month = val % 13; - val /= 13; - year = val; - } + val = gw_mysql_get_byte8(ptr); + second = val - ((val / 100) * 100); + val /= 100; + minute = val - ((val / 100) * 100); + val /= 100; + hour = val - ((val / 100) * 100); + val /= 100; + day = val - ((val / 100) * 100); + val /= 100; + month = val - ((val / 100) * 100); + val /= 100; + year = val; memset(dest, 0, sizeof(struct tm)); dest->tm_year = year - 1900; @@ -502,7 +473,7 @@ static size_t temporal_field_size(uint8_t type, uint8_t decimals, int length) return 3 + ((decimals + 1) / 2); case TABLE_COL_TYPE_DATETIME: - return length < 0 || length > 6 ? 8 : datetime_sizes[length]; + return 8; case TABLE_COL_TYPE_TIMESTAMP: return 4; From 6dcc71d8622295d6d6d7f8702d5a01683b3dc27a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 29 Jan 2018 20:17:48 +0200 Subject: [PATCH 31/54] MXS-1621: Fix minor bugs caused by previous changes Used the correct value in table_create_alloc and remove unused parameter. Use the pre-calculated end pointer when looking for events. Always use the column count of the TABLE_MAP event as all mismatches are detected earlier. --- server/modules/routing/avrorouter/avro_file.c | 2 +- server/modules/routing/avrorouter/avro_rbr.c | 4 ++-- server/modules/routing/avrorouter/avro_schema.c | 4 ++-- server/modules/routing/avrorouter/avrorouter.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_file.c b/server/modules/routing/avrorouter/avro_file.c index ee9fd04f2..cbb011f85 100644 --- a/server/modules/routing/avrorouter/avro_file.c +++ b/server/modules/routing/avrorouter/avro_file.c @@ -1124,7 +1124,7 @@ void handle_query_event(AVRO_INSTANCE *router, REP_HEADER *hdr, int *pending_tra } else { - created = table_create_alloc(ident, sql, len, db); + created = table_create_alloc(ident, sql, len); } if (created && !save_and_replace_table_create(router, created)) diff --git a/server/modules/routing/avrorouter/avro_rbr.c b/server/modules/routing/avrorouter/avro_rbr.c index 4b9f5d177..dc72be06a 100644 --- a/server/modules/routing/avrorouter/avro_rbr.c +++ b/server/modules/routing/avrorouter/avro_rbr.c @@ -288,7 +288,7 @@ bool handle_row_event(AVRO_INSTANCE *router, REP_HEADER *hdr, uint8_t *ptr) int rows = 0; MXS_INFO("Row Event for '%s' at %lu", table_ident, router->current_pos); - while (ptr - start < hdr->event_size - BINLOG_EVENT_HDR_LEN) + while (ptr < end) { static uint64_t total_row_count = 1; MXS_INFO("Row %lu", total_row_count++); @@ -514,7 +514,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value { int npresent = 0; avro_value_t field; - long ncolumns = MXS_MIN(map->columns, create->columns); + long ncolumns = map->columns; uint8_t *metadata = map->column_metadata; size_t metadata_offset = 0; diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index aaacc4a32..962a394f7 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -755,13 +755,13 @@ TABLE_CREATE* table_create_alloc(const char* ident, const char* sql, int len) { if ((rval = MXS_MALLOC(sizeof(TABLE_CREATE)))) { - rval->version = resolve_table_version(db, table); + rval->version = resolve_table_version(database, table); rval->was_used = false; rval->column_names = names; rval->column_lengths = lengths; rval->column_types = types; rval->columns = n_columns; - rval->database = MXS_STRDUP(db); + rval->database = MXS_STRDUP(database); rval->table = MXS_STRDUP(table); } diff --git a/server/modules/routing/avrorouter/avrorouter.h b/server/modules/routing/avrorouter/avrorouter.h index 02f1b8d6c..41a4f93fa 100644 --- a/server/modules/routing/avrorouter/avrorouter.h +++ b/server/modules/routing/avrorouter/avrorouter.h @@ -302,7 +302,7 @@ extern void read_table_info(uint8_t *ptr, uint8_t post_header_len, uint64_t *tab char* dest, size_t len); extern TABLE_MAP *table_map_alloc(uint8_t *ptr, uint8_t hdr_len, TABLE_CREATE* create); extern void table_map_free(TABLE_MAP *map); -extern TABLE_CREATE* table_create_alloc(const char* ident, const char* sql, int len, const char* db); +extern TABLE_CREATE* table_create_alloc(const char* ident, const char* sql, int len); extern TABLE_CREATE* table_create_copy(AVRO_INSTANCE *router, const char* sql, size_t len, const char* db); extern void table_create_free(TABLE_CREATE* value); extern bool table_create_save(TABLE_CREATE *create, const char *filename); From c7474e439e82363f079eb3a712625ee902606189 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Mon, 29 Jan 2018 15:17:12 +0200 Subject: [PATCH 32/54] Print new parameters during diagnostics Also, copy using strdup instead since config_copy_string() returns null for empty strings. --- server/modules/filter/qlafilter/qlafilter.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/server/modules/filter/qlafilter/qlafilter.cc b/server/modules/filter/qlafilter/qlafilter.cc index c576824c4..d44b04076 100644 --- a/server/modules/filter/qlafilter/qlafilter.cc +++ b/server/modules/filter/qlafilter/qlafilter.cc @@ -364,8 +364,8 @@ createInstance(const char *name, char **options, MXS_CONFIG_PARAMETER *params) my_instance->flush_writes = config_get_bool(params, PARAM_FLUSH); my_instance->log_file_data_flags = config_get_enum(params, PARAM_LOG_DATA, log_data_values); my_instance->log_mode_flags = config_get_enum(params, PARAM_LOG_TYPE, log_type_values); - my_instance->query_newline = config_copy_string(params, PARAM_NEWLINE); - my_instance->separator = config_copy_string(params, PARAM_SEPARATOR); + my_instance->query_newline = MXS_STRDUP_A(config_get_string(params, PARAM_NEWLINE)); + my_instance->separator = MXS_STRDUP_A(config_get_string(params, PARAM_SEPARATOR)); my_instance->match = config_copy_string(params, PARAM_MATCH); my_instance->exclude = config_copy_string(params, PARAM_EXCLUDE); @@ -768,6 +768,10 @@ diagnostic(MXS_FILTER *instance, MXS_FILTER_SESSION *fsession, DCB *dcb) dcb_printf(dcb, "\t\tExclude queries that match %s\n", my_instance->exclude); } + dcb_printf(dcb, "\t\tColumn separator %s\n", + my_instance->separator); + dcb_printf(dcb, "\t\tNewline replacement %s\n", + my_instance->query_newline); } /** @@ -811,6 +815,8 @@ static json_t* diagnostic_json(const MXS_FILTER *instance, const MXS_FILTER_SESS { json_object_set_new(rval, PARAM_EXCLUDE, json_string(my_instance->exclude)); } + json_object_set_new(rval, PARAM_SEPARATOR, json_string(my_instance->separator)); + json_object_set_new(rval, PARAM_NEWLINE, json_string(my_instance->query_newline)); return rval; } From ea58b16a7aaa4b34e854bac428addd3519f35d56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 30 Jan 2018 16:31:12 +0200 Subject: [PATCH 33/54] Add missing include for vector The vector header was not included in dbfwfilter.hh. --- server/modules/filter/dbfwfilter/dbfwfilter.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.hh b/server/modules/filter/dbfwfilter/dbfwfilter.hh index e7181159d..738ef128d 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.hh +++ b/server/modules/filter/dbfwfilter/dbfwfilter.hh @@ -18,6 +18,7 @@ #include #include #include +#include #include #include From 255250652df607d717b0663d9879b3477dbcdf22 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Fri, 26 Jan 2018 09:58:10 +0200 Subject: [PATCH 34/54] Refactor pre-switchover, add similar checks as in failover Now detects some erroneous situations before starting switchover. Switchover can be activated without specifying current master. In this case, the cluster master server is selected. --- include/maxscale/monitor.h | 9 + server/core/monitor.cc | 13 + .../modules/monitor/mariadbmon/mysql_mon.cc | 341 ++++++++---------- 3 files changed, 171 insertions(+), 192 deletions(-) diff --git a/include/maxscale/monitor.h b/include/maxscale/monitor.h index 676f4a494..b8d9450d8 100644 --- a/include/maxscale/monitor.h +++ b/include/maxscale/monitor.h @@ -375,4 +375,13 @@ void store_server_journal(MXS_MONITOR *monitor, MXS_MONITORED_SERVER *master); */ void load_server_journal(MXS_MONITOR *monitor, MXS_MONITORED_SERVER **master); +/** + * Find the monitored server representing the server. + * + * @param mon Cluster monitor + * @param search_server Server to search for + * @return Found monitored server or NULL if not found + */ +MXS_MONITORED_SERVER* mon_get_monitored_server(MXS_MONITOR* mon, SERVER* search_server); + MXS_END_DECLS diff --git a/server/core/monitor.cc b/server/core/monitor.cc index 7c9936e0f..c8f0b450b 100644 --- a/server/core/monitor.cc +++ b/server/core/monitor.cc @@ -2427,3 +2427,16 @@ static bool journal_is_stale(MXS_MONITOR *monitor, time_t max_age) return is_stale; } + +MXS_MONITORED_SERVER* mon_get_monitored_server(MXS_MONITOR* mon, SERVER* search_server) +{ + ss_dassert(mon && search_server); + for (MXS_MONITORED_SERVER* iter = mon->monitored_servers; iter != NULL; iter = iter->next) + { + if (iter->server == search_server) + { + return iter; + } + } + return NULL; +} \ No newline at end of file diff --git a/server/modules/monitor/mariadbmon/mysql_mon.cc b/server/modules/monitor/mariadbmon/mysql_mon.cc index a965212a8..b89574c2a 100644 --- a/server/modules/monitor/mariadbmon/mysql_mon.cc +++ b/server/modules/monitor/mariadbmon/mysql_mon.cc @@ -314,152 +314,95 @@ public: } }; +bool uses_gtid(MYSQL_MONITOR* mon, MXS_MONITORED_SERVER* mon_server, json_t** error_out) +{ + bool rval = false; + const MySqlServerInfo* info = get_server_info(mon, mon_server); + if (info->slave_status.gtid_io_pos.server_id == 0) + { + string slave_not_gtid_msg = string("Slave server ") + mon_server->server->unique_name + + " is not using gtid replication or master server id is 0."; + PRINT_MXS_JSON_ERROR(error_out, "%s", slave_not_gtid_msg.c_str()); + } + else + { + rval = true; + } + return rval; +} /** - * Check whether specified current master is acceptable. + * Check that the given server is a master and it's the only master. * - * @param current_master The specified current master. - * @param monitored_server The server to check against. - * @param monitored_current_master On output, @c monitored_server, if @c monitored_server - * is the same server as @c current_master. - * @param error On output, error object if function failed. + * @param mon Cluster monitor. + * @param suggested_curr_master The server to check, given by user. + * @param error_out On output, error object if function failed. * * @return False, if there is some error with the specified current master, * True otherwise. */ -bool mysql_switchover_check_current(SERVER* current_master, - MXS_MONITORED_SERVER* monitored_server, - MXS_MONITORED_SERVER** monitored_current_master, - json_t** error) +bool mysql_switchover_check_current(const MYSQL_MONITOR* mon, + const MXS_MONITORED_SERVER* suggested_curr_master, + json_t** error_out) { - bool rv = true; - bool is_master = SERVER_IS_MASTER(monitored_server->server); - - if (current_master == monitored_server->server) + bool server_is_master = false; + MXS_MONITORED_SERVER* extra_master = NULL; // A master server which is not the suggested one + for (MXS_MONITORED_SERVER* mon_serv = mon->monitor->monitored_servers; + mon_serv != NULL && extra_master == NULL; + mon_serv = mon_serv->next) { - if (is_master) + if (SERVER_IS_MASTER(mon_serv->server)) { - *monitored_current_master = monitored_server; - } - else - { - *error = mxs_json_error("Specified %s is a server, but not the current master.", - current_master->unique_name); - rv = false; + if (mon_serv == suggested_curr_master) + { + server_is_master = true; + } + else + { + extra_master = mon_serv; + } } } - else if (is_master) - { - *error = mxs_json_error("Current master not specified, even though there is " - "a master (%s).", monitored_server->server->unique_name); - rv = false; - } - return rv; + if (!server_is_master) + { + PRINT_MXS_JSON_ERROR(error_out, "Server '%s' is not the current master or it's in maintenance.", + suggested_curr_master->server->unique_name); + } + else if (extra_master) + { + PRINT_MXS_JSON_ERROR(error_out, "Cluster has an additional master server '%s'.", + extra_master->server->unique_name); + } + return server_is_master && !extra_master; } /** * Check whether specified new master is acceptable. * - * @param new_master The specified new master. * @param monitored_server The server to check against. - * @param monitored_new_master On output, @c monitored_server, if @c monitored_server - * is the same server as @c new_master. * @param error On output, error object if function failed. * - * @return False, if there is some error with the specified current master, - * True otherwise. + * @return True, if suggested new master is a viable promotion candidate. */ -bool mysql_switchover_check_new(SERVER* new_master, - MXS_MONITORED_SERVER* monitored_server, - MXS_MONITORED_SERVER** monitored_new_master, - json_t** error) +bool mysql_switchover_check_new(const MXS_MONITORED_SERVER* monitored_server, json_t** error) { - bool rv = true; - bool is_master = SERVER_IS_MASTER(monitored_server->server); + SERVER* server = monitored_server->server; + const char* name = server->unique_name; + bool is_master = SERVER_IS_MASTER(server); + bool is_slave = SERVER_IS_SLAVE(server); - if (new_master == monitored_server->server) + if (is_master) { - if (!is_master) - { - *monitored_new_master = monitored_server; - } - else - { - *error = mxs_json_error("Specified new master %s is already the current master.", - new_master->unique_name); - rv = false; - } + const char IS_MASTER[] = "Specified new master '%s' is already the current master."; + PRINT_MXS_JSON_ERROR(error, IS_MASTER, name); + } + else if (!is_slave) + { + const char NOT_SLAVE[] = "Specified new master '%s' is not a slave."; + PRINT_MXS_JSON_ERROR(error, NOT_SLAVE, name); } - return rv; -} - -/** - * Check whether specified current and new master are acceptable. - * - * @param mon The monitor. - * @param new_master The specified new master. - * @param current_master The specifiec current master (may be NULL). - * @param monitored_new_master On output, the monitored server corresponding to - * @c new_master. - * @param monitored_current_master On output, the monitored server corresponding to - * @c current_master. - * @param error On output, error object if function failed. - * - * @return True if switchover can proceeed, false otherwise. - */ -bool mysql_switchover_check(MXS_MONITOR* mon, - SERVER* new_master, - SERVER* current_master, - MXS_MONITORED_SERVER** monitored_new_master, - MXS_MONITORED_SERVER** monitored_current_master, - json_t** error) -{ - bool rv = true; - - *monitored_new_master = NULL; - *monitored_current_master = NULL; - *error = NULL; - - MXS_MONITORED_SERVER* monitored_server = mon->monitored_servers; - - while (rv && monitored_server && (!*monitored_current_master || !*monitored_new_master)) - { - if (!*monitored_current_master) - { - rv = mysql_switchover_check_current(current_master, - monitored_server, - monitored_current_master, - error); - } - - if (rv) - { - rv = mysql_switchover_check_new(new_master, monitored_server, monitored_new_master, error); - } - - monitored_server = monitored_server->next; - } - - if (rv && ((current_master && !*monitored_current_master) || !*monitored_new_master)) - { - if (current_master && !*monitored_current_master) - { - *error = mxs_json_error("Specified current master %s is not found amongst " - "existing servers.", current_master->unique_name); - } - - if (!*monitored_new_master) - { - *error = mxs_json_error_append(*error, - "Specified new master %s is not found amongst " - "existing servers.", new_master->unique_name); - } - - rv = false; - } - - return rv; + return !is_master && is_slave; } /** @@ -496,17 +439,13 @@ bool failover_check(MYSQL_MONITOR* mon, json_t** error_out) } else if (SERVER_IS_SLAVE(mon_server->server)) { - const MySqlServerInfo* info = get_server_info(mon, mon_server); - if (info->slave_status.gtid_io_pos.server_id == 0) + if (uses_gtid(mon, mon_server, error_out)) { - string slave_not_gtid_msg = string("Slave server ") + mon_server->server->unique_name + - " is not using gtid replication or master server id is 0."; - PRINT_MXS_JSON_ERROR(error_out, "%s", slave_not_gtid_msg.c_str()); - error = true; + slaves++; } else { - slaves++; + error = true; } } } @@ -532,7 +471,7 @@ bool failover_check(MYSQL_MONITOR* mon, json_t** error_out) * * @return True, if switchover was performed, false otherwise. */ -bool mysql_switchover(MXS_MONITOR* mon, SERVER* new_master, SERVER* current_master, json_t** output) +bool mysql_switchover(MXS_MONITOR* mon, MXS_MONITORED_SERVER* new_master, MXS_MONITORED_SERVER* current_master, json_t** error_out) { bool stopped = stop_monitor(mon); if (stopped) @@ -544,49 +483,47 @@ bool mysql_switchover(MXS_MONITOR* mon, SERVER* new_master, SERVER* current_mast MXS_NOTICE("Monitor %s already stopped, switchover can proceed.", mon->name); } - bool rv = true; - *output = NULL; - MXS_MONITORED_SERVER* monitored_new_master = NULL; - MXS_MONITORED_SERVER* monitored_current_master = NULL; + bool rval = false; + MYSQL_MONITOR* handle = static_cast(mon->handle); - rv = mysql_switchover_check(mon, - new_master, current_master, - &monitored_new_master, &monitored_current_master, - output); - - if (rv) + bool current_ok = mysql_switchover_check_current(handle, current_master, error_out); + bool new_ok = mysql_switchover_check_new(new_master, error_out); + // Check that all slaves are using gtid-replication + bool gtid_ok = true; + for (MXS_MONITORED_SERVER* mon_serv = mon->monitored_servers; mon_serv != NULL; mon_serv = mon_serv->next) { - bool failover = config_get_bool(mon->parameters, CN_AUTO_FAILOVER); - MYSQL_MONITOR *handle = static_cast(mon->handle); - rv = do_switchover(handle, monitored_current_master, monitored_new_master, output); - - if (rv) + if (SERVER_IS_SLAVE(mon_serv->server)) { - MXS_NOTICE("Switchover %s -> %s performed.", - current_master->unique_name ? current_master->unique_name : "none", - new_master->unique_name); + if (!uses_gtid(handle, mon_serv, error_out)) + { + gtid_ok = false; + } + } + } + + if (current_ok && new_ok && gtid_ok) + { + bool switched = do_switchover(handle, current_master, new_master, error_out); + + const char* curr_master_name = current_master->server->unique_name; + const char* new_master_name = new_master->server->unique_name; + + if (switched) + { + MXS_NOTICE("Switchover %s -> %s performed.", curr_master_name, new_master_name); + rval = true; } else { + string format = "Switchover %s -> %s failed"; + bool failover = config_get_bool(mon->parameters, CN_AUTO_FAILOVER); if (failover) { - // TODO: There could be a more convenient way for this. - MXS_CONFIG_PARAMETER p = {}; - p.name = const_cast(CN_AUTO_FAILOVER); - p.value = const_cast("false"); - - monitorAddParameters(mon, &p); - - MXS_ALERT("Switchover %s -> %s failed, failover has been disabled.", - current_master->unique_name ? current_master->unique_name : "none", - new_master->unique_name); - } - else - { - MXS_ERROR("Switchover %s -> %s failed.", - current_master->unique_name ? current_master->unique_name : "none", - new_master->unique_name); + disable_setting(handle, CN_AUTO_FAILOVER); + format += ", failover has been disabled."; } + format += "."; + PRINT_MXS_JSON_ERROR(error_out, format.c_str(), curr_master_name, new_master_name); } } @@ -594,7 +531,7 @@ bool mysql_switchover(MXS_MONITOR* mon, SERVER* new_master, SERVER* current_mast { startMonitor(mon, mon->parameters); } - return rv; + return rval; } /** @@ -605,37 +542,68 @@ bool mysql_switchover(MXS_MONITOR* mon, SERVER* new_master, SERVER* current_mast * * @return True, if the command was executed, false otherwise. */ -bool mysql_handle_switchover(const MODULECMD_ARG* args, json_t** output) +bool mysql_handle_switchover(const MODULECMD_ARG* args, json_t** error_out) { ss_dassert((args->argc == 2) || (args->argc == 3)); ss_dassert(MODULECMD_GET_TYPE(&args->argv[0].type) == MODULECMD_ARG_MONITOR); ss_dassert(MODULECMD_GET_TYPE(&args->argv[1].type) == MODULECMD_ARG_SERVER); - ss_dassert((args->argc == 2) || - (MODULECMD_GET_TYPE(&args->argv[2].type) == MODULECMD_ARG_SERVER)); + ss_dassert((args->argc == 2) || (MODULECMD_GET_TYPE(&args->argv[2].type) == MODULECMD_ARG_SERVER)); MXS_MONITOR* mon = args->argv[0].value.monitor; - MYSQL_MONITOR* mysql_mon = static_cast(mon->handle); SERVER* new_master = args->argv[1].value.server; SERVER* current_master = (args->argc == 3) ? args->argv[2].value.server : NULL; + bool error = false; - bool rv = false; - - if (!config_get_global_options()->passive) + const char NO_SERVER[] = "Server '%s' is not a member of monitor '%s'."; + MXS_MONITORED_SERVER* mon_new_master = mon_get_monitored_server(mon, new_master); + if (mon_new_master == NULL) { - rv = mysql_switchover(mon, new_master, current_master, output); + PRINT_MXS_JSON_ERROR(error_out, NO_SERVER, new_master->unique_name, mon->name); + error = true; + } + + MXS_MONITORED_SERVER* mon_curr_master = NULL; + if (current_master) + { + mon_curr_master = mon_get_monitored_server(mon, current_master); + if (mon_curr_master == NULL) + { + PRINT_MXS_JSON_ERROR(error_out, NO_SERVER, current_master->unique_name, mon->name); + error = true; + } } else { - MXS_WARNING("Attempt to perform switchover %s -> %s, even though " - "MaxScale is in passive mode.", - current_master ? current_master->unique_name : "none", - new_master->unique_name); - *output = mxs_json_error("Switchover %s -> %s not performed, as MaxScale is in passive mode.", - current_master ? current_master->unique_name : "none", - new_master->unique_name); + // Autoselect current master + MYSQL_MONITOR* handle = static_cast(mon->handle); + if (handle->master) + { + mon_curr_master = handle->master; + } + else + { + const char NO_MASTER[] = "Monitor '%s' has no master server."; + PRINT_MXS_JSON_ERROR(error_out, NO_MASTER, mon->name); + error = true; + } + } + if (error) + { + return false; } - return rv; + bool rval = false; + if (!config_get_global_options()->passive) + { + rval = mysql_switchover(mon, mon_new_master, mon_curr_master, error_out); + } + else + { + const char MSG[] = "Switchover attempted but not performed, as MaxScale is in passive mode."; + PRINT_MXS_JSON_ERROR(error_out, MSG); + } + + return rval; } /** @@ -730,18 +698,7 @@ bool mysql_rejoin(MXS_MONITOR* mon, SERVER* rejoin_server, json_t** output) MYSQL_MONITOR *handle = static_cast(mon->handle); if (cluster_can_be_joined(handle)) { - MXS_MONITORED_SERVER* mon_server = NULL; - // Search for the MONITORED_SERVER. Could this be a general monitor function? - for (MXS_MONITORED_SERVER* iterator = mon->monitored_servers; - iterator != NULL && mon_server == NULL; - iterator = iterator->next) - { - if (iterator->server == rejoin_server) - { - mon_server = iterator; - } - } - + MXS_MONITORED_SERVER* mon_server = mon_get_monitored_server(mon, rejoin_server); if (mon_server) { MXS_MONITORED_SERVER* master = handle->master; @@ -840,7 +797,7 @@ extern "C" ARG_MONITOR_DESC }, { MODULECMD_ARG_SERVER, "New master" }, - { MODULECMD_ARG_SERVER | MODULECMD_ARG_OPTIONAL, "Current master (obligatory if exists)" } + { MODULECMD_ARG_SERVER | MODULECMD_ARG_OPTIONAL, "Current master (optional)" } }; modulecmd_register_command(MXS_MODULE_NAME, "switchover", MODULECMD_TYPE_ACTIVE, From f53e112bf49766f1cc55516c2d7ee571461d483f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 30 Jan 2018 19:19:04 +0200 Subject: [PATCH 35/54] Don't write errors to dummy sessions If a DCB is closed before a response to the handshake packet is received, the DCB's session will point to the dummy session. In this case no error should be written to the DCB. --- server/modules/protocol/MySQL/mariadbclient/mysql_client.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 0055b528b..d9cf55327 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -1416,7 +1416,7 @@ static int gw_client_hangup_event(DCB *dcb) goto retblock; } - if (!session_valid_for_pool(session)) + if (session->state != SESSION_STATE_DUMMY && !session_valid_for_pool(session)) { // The client did not send a COM_QUIT packet modutil_send_mysql_err_packet(dcb, 0, 0, 1927, "08S01", "Connection killed by MaxScale"); From 4b7c334c1b9f6cb23eca5ae765ec90cb8f4777ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 30 Jan 2018 11:59:47 +0200 Subject: [PATCH 36/54] Add a timeout to mxs1585 The test will hang if one of the threads doesn't exit. To detect failures faster, a timeout is needed. --- maxscale-system-test/mxs1585.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/maxscale-system-test/mxs1585.cpp b/maxscale-system-test/mxs1585.cpp index d6656846d..ee259830a 100644 --- a/maxscale-system-test/mxs1585.cpp +++ b/maxscale-system-test/mxs1585.cpp @@ -63,6 +63,7 @@ int main(int argc, char** argv) } running = false; + test.set_timeout(120); for (auto& a: threads) { From d506f01de971751c8b11051f57fc208dd4c50309 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 30 Jan 2018 15:55:58 +0200 Subject: [PATCH 37/54] Add basic MaxCtrl test The test does a very minimal check for MaxCtrl functionality. --- maxscale-system-test/CMakeLists.txt | 3 +++ maxscale-system-test/maxctrl_basic.cpp | 31 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 maxscale-system-test/maxctrl_basic.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 90899569e..7cc81e0c9 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -254,6 +254,9 @@ add_test_executable(different_size_rwsplit.cpp different_size_rwsplit replicatio # Tries to use 'maxkeys', 'maxpasswrd' add_test_executable(encrypted_passwords.cpp encrypted_passwords replication LABELS maxscale LIGHT REPL_BACKEND) +# Basic MaxCtrl test +add_test_executable(maxctrl_basic.cpp maxctrl_basic replication LABELS maxctrl REPL_BACKEND) + # MySQL Monitor Failover Test add_test_executable(mysqlmon_detect_standalone_master.cpp mysqlmon_detect_standalone_master mysqlmon_detect_standalone_master LABELS mysqlmon REPL_BACKEND) diff --git a/maxscale-system-test/maxctrl_basic.cpp b/maxscale-system-test/maxctrl_basic.cpp new file mode 100644 index 000000000..6322d989a --- /dev/null +++ b/maxscale-system-test/maxctrl_basic.cpp @@ -0,0 +1,31 @@ +/** + * Minimal MaxCtrl sanity check + */ + +#include "testconnections.h" + +int main(int argc, char** argv) +{ + TestConnections test(argc, argv); + + int rc = test.maxscales->ssh_node_f(0, false, "maxctrl help list servers"); + test.assert(rc == 0, "`help list servers` should work"); + + rc = test.maxscales->ssh_node_f(0, false, "maxctrl --tsv list servers|grep 'Master, Running'"); + test.assert(rc == 0, "`list servers` should return at least one row with: Master, Running"); + + rc = test.maxscales->ssh_node_f(0, false, "maxctrl set server server1 maintenance"); + test.assert(rc == 0, "`set server` should work"); + + rc = test.maxscales->ssh_node_f(0, false, "maxctrl --tsv list servers|grep 'Maintenance'"); + test.assert(rc == 0, "`list servers` should return at least one row with: Maintanance"); + + rc = test.maxscales->ssh_node_f(0, false, "maxctrl clear server server1 maintenance"); + test.assert(rc == 0, "`clear server` should work"); + + rc = test.maxscales->ssh_node_f(0, false, "maxctrl --tsv list servers|grep 'Maintenance'"); + test.assert(rc != 0, "`list servers` should have no rows with: Maintanance"); + + test.check_maxscale_alive(); + return test.global_result; +} From 26d3de7a3ba6ffa4bf9c84d97561095b013639e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 29 Jan 2018 15:29:19 +0200 Subject: [PATCH 38/54] Skip parsing when in read-only transaction As chages to the transaction state are detected by the protocol level mini-parser, there's no need to fully classify queries inside read-only transactions. This should be a good performance boost for loads that heavily use read-only transactions. --- .../readwritesplit/rwsplit_route_stmt.cc | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc index 95dbf8fe0..196464d05 100644 --- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc +++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc @@ -96,13 +96,26 @@ route_target_t get_target_type(RWSplitSession *rses, GWBUF *buffer, uint8_t* command, uint32_t* type, uint32_t* stmt_id) { route_target_t route_target = TARGET_MASTER; + bool in_read_only_trx = rses->target_node && session_trx_is_read_only(rses->client_dcb->session); if (gwbuf_length(buffer) > MYSQL_HEADER_LEN) { *command = mxs_mysql_get_command(buffer); - *type = determine_query_type(buffer, *command); - handle_multi_temp_and_load(rses, buffer, *command, type); + /** + * If the session is inside a read-only transaction, we trust that the + * server acts properly even when non-read-only queries are executed. + * For this reason, we can skip the parsing of the statement completely. + */ + if (in_read_only_trx) + { + *type = QUERY_TYPE_READ; + } + else + { + *type = determine_query_type(buffer, *command); + handle_multi_temp_and_load(rses, buffer, *command, type); + } if (MXS_LOG_PRIORITY_IS_ENABLED(LOG_INFO)) { @@ -139,7 +152,8 @@ route_target_t get_target_type(RWSplitSession *rses, GWBUF *buffer, } else { - if (*command == MXS_COM_QUERY && + if (!in_read_only_trx && + *command == MXS_COM_QUERY && qc_get_operation(buffer) == QUERY_OP_EXECUTE) { std::string id = get_text_ps_id(buffer); From 1febafabf34d357f3329235943f0d6fe81bdd1c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 31 Jan 2018 09:40:29 +0200 Subject: [PATCH 39/54] Crash after double close on debug builds When a double close is detected in a debug build, a debug assertion is triggered. This will generate a core dump which should help investigate the double close. --- server/core/dcb.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/server/core/dcb.cc b/server/core/dcb.cc index 161df9dad..701159efd 100644 --- a/server/core/dcb.cc +++ b/server/core/dcb.cc @@ -1123,6 +1123,7 @@ void dcb_close(DCB *dcb) ++dcb->n_close; // TODO: Will this happen on a regular basis? MXS_WARNING("dcb_close(%p) called %u times.", dcb, dcb->n_close); + ss_dassert(!true); } } From 12f5cabc5027886808d8b5c6f8edd80bba738235 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 31 Jan 2018 10:35:20 +0200 Subject: [PATCH 40/54] Discard fake events for closed DCBs If a fake event is sent to a DCB that has been closed, it should be discarded. --- server/core/dcb.cc | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/server/core/dcb.cc b/server/core/dcb.cc index 701159efd..151e990eb 100644 --- a/server/core/dcb.cc +++ b/server/core/dcb.cc @@ -3209,6 +3209,26 @@ static uint32_t dcb_poll_handler(MXS_POLL_DATA *data, int thread_id, uint32_t ev return dcb_handler(dcb, events); } +static bool dcb_is_still_valid(DCB* target, int id) +{ + bool rval = false; + + for (DCB *dcb = this_unit.all_dcbs[id]; + dcb; dcb = dcb->thread.next) + { + if (target == dcb) + { + if (dcb->n_close == 0) + { + rval = true; + } + break; + } + } + + return rval; +} + class FakeEventTask: public mxs::WorkerDisposableTask { FakeEventTask(const FakeEventTask&); @@ -3224,8 +3244,15 @@ public: void execute(Worker& worker) { - m_dcb->fakeq = m_buffer; - dcb_handler(m_dcb, m_ev); + if (dcb_is_still_valid(m_dcb, worker.get_current_id())) + { + m_dcb->fakeq = m_buffer; + dcb_handler(m_dcb, m_ev); + } + else + { + gwbuf_free(m_buffer); + } } private: From 8ebac86406c307e4e3f0d6aa8510698c0092b248 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 31 Jan 2018 11:03:17 +0200 Subject: [PATCH 41/54] Simplify mxs431 The test unnecessarily restarted MaxScale for no obvious reason. Cleaned up SQL queries to use new functionality. --- maxscale-system-test/mxs431.cpp | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/maxscale-system-test/mxs431.cpp b/maxscale-system-test/mxs431.cpp index 62e0dcb5d..f179f26ef 100644 --- a/maxscale-system-test/mxs431.cpp +++ b/maxscale-system-test/mxs431.cpp @@ -10,37 +10,26 @@ int main(int argc, char *argv[]) { TestConnections test(argc, argv); - char str[256]; - int iterations = 100; - test.repl->execute_query_all_nodes((char *) "set global max_connections = 600;"); - test.set_timeout(200); - test.repl->stop_slaves(); - test.set_timeout(200); - test.maxscales->restart_maxscale(0); - test.set_timeout(200); + test.repl->connect(); - test.stop_timeout(); /** Create a database on each node */ for (int i = 0; i < test.repl->N; i++) { test.set_timeout(20); - sprintf(str, "DROP DATABASE IF EXISTS shard_db%d", i); - test.tprintf("%s\n", str); - execute_query(test.repl->nodes[i], str); - test.set_timeout(20); - sprintf(str, "CREATE DATABASE shard_db%d", i); - test.tprintf("%s\n", str); - execute_query(test.repl->nodes[i], str); + execute_query(test.repl->nodes[i], "set global max_connections = 600"); + execute_query(test.repl->nodes[i], "DROP DATABASE IF EXISTS shard_db%d", i); + execute_query(test.repl->nodes[i], "CREATE DATABASE shard_db%d", i); test.stop_timeout(); } - test.repl->close_connections(); + int iterations = 100; for (int j = 0; j < iterations && test.global_result == 0; j++) { for (int i = 0; i < test.repl->N && test.global_result == 0; i++) { + char str[256]; sprintf(str, "shard_db%d", i); test.set_timeout(30); MYSQL *conn = open_conn_db(test.maxscales->rwsplit_port[0], test.maxscales->IP[0], @@ -52,5 +41,13 @@ int main(int argc, char *argv[]) } } + /** Create a database on each node */ + for (int i = 0; i < test.repl->N; i++) + { + test.set_timeout(20); + execute_query(test.repl->nodes[i], "DROP DATABASE shard_db%d", i); + test.stop_timeout(); + } + return test.global_result; } From 2c04adafd192b7e44c1b66b743e3d1449b944ef5 Mon Sep 17 00:00:00 2001 From: Timofey Turenko Date: Tue, 8 Aug 2017 19:13:37 +0300 Subject: [PATCH 42/54] fix kerberos test and mxs1516 test --- maxscale-system-test/kerberos_setup.cpp | 35 +++++++++++++++++-------- maxscale-system-test/mxs1516.cpp | 3 +++ 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/maxscale-system-test/kerberos_setup.cpp b/maxscale-system-test/kerberos_setup.cpp index 7ec96d82b..cc4f19291 100644 --- a/maxscale-system-test/kerberos_setup.cpp +++ b/maxscale-system-test/kerberos_setup.cpp @@ -14,6 +14,7 @@ int main(int argc, char *argv[]) TestConnections * Test = new TestConnections(argc, argv); Test->set_timeout(1000); char str[1024]; + char str1[1024]; int i; @@ -32,23 +33,27 @@ int main(int argc, char *argv[]) sprintf(str, "%s/krb5.conf", test_dir); for (i = 0; i < Test->repl->N; i++) { + Test->repl->ssh_node(i, true, "yum clean all"); Test->repl->ssh_node(i, true, "yum install -y MariaDB-gssapi-server MariaDB-gssapi-client krb5-workstation pam_krb5"); - Test->repl->copy_to_node(str, (char *) "~/", i); - Test->repl->ssh_node(i, true, "cp ~/krb5.conf /etc/"); + Test->repl->copy_to_node(str, Test->repl->access_homedir[i], i); + sprintf(str1, "cp %s/krb5.conf /etc/", Test->repl->access_homedir[i]); + Test->repl->ssh_node(i, true, str1); Test->repl->copy_to_node((char *) "hosts", (char *) "~/", i); - Test->repl->ssh_node(i, true, "cp ~/hosts /etc/"); + sprintf(str1, "cp %s/hosts /etc/", Test->repl->access_homedir[i]); + Test->repl->ssh_node(i, true, str1); } Test->tprintf("Copying 'hosts' and krb5.conf files to Maxscale node\n"); - Test->copy_to_maxscale((char *) "hosts", (char *) "~/"); - Test->ssh_maxscale(true, (char *) "cp ~/hosts /etc/"); + Test->copy_to_maxscale((char *) "hosts", Test->maxscale_access_homedir); + Test->ssh_maxscale(true, (char *) "cp %s/hosts /etc/", Test->maxscale_access_homedir); - Test->copy_to_maxscale(str, (char *) "~/"); - Test->ssh_maxscale(true, (char *) "cp ~/krb5.conf /etc/"); + Test->copy_to_maxscale(str, Test->maxscale_access_homedir); + Test->ssh_maxscale(true, (char *) "cp %s/krb5.conf /etc/", Test->maxscale_access_homedir); Test->tprintf("Instaling Kerberos server packages to Maxscale node\n"); + Test->ssh_maxscale(true, (char *) "yum clean all"); Test->ssh_maxscale(true, (char *) "yum install rng-tools -y"); Test->ssh_maxscale(true, (char *) "rngd -r /dev/urandom -o /dev/random"); @@ -84,6 +89,7 @@ int main(int argc, char *argv[]) Test->ssh_maxscale(true, (char *) "chmod a+r /etc/krb5.keytab;"); Test->ssh_maxscale(false, (char *) "kinit mariadb/maxscale.test@MAXSCALE.TEST -k -t /etc/krb5.keytab"); + Test->ssh_maxscale(true, (char *) "mkdir -p /home/maxscale"); Test->ssh_maxscale(true, (char *) "su maxscale --login -s /bin/sh -c \"kinit mariadb/maxscale.test@MAXSCALE.TEST -k -t /etc/krb5.keytab\""); @@ -94,11 +100,13 @@ int main(int argc, char *argv[]) for (i = 0; i < Test->repl->N; i++) { sprintf(str, "%s/kerb.cnf", test_dir); - Test->repl->copy_to_node(str, (char *) "~/", i); - Test->repl->ssh_node(i, true, "cp ~/kerb.cnf /etc/my.cnf.d/"); + Test->repl->copy_to_node(str, Test->repl->access_homedir[i], i); + sprintf(str, "cp %s/kerb.cnf /etc/my.cnf.d/", Test->repl->access_homedir[i]); + Test->repl->ssh_node(i, true, str); - Test->repl->copy_to_node((char *) "krb5.keytab", (char *) "~/", i); - Test->repl->ssh_node(i, true, "cp ~/krb5.keytab /etc/"); + Test->repl->copy_to_node((char *) "krb5.keytab", Test->repl->access_homedir[i], i); + sprintf(str, "cp %s/krb5.keytab /etc/", Test->repl->access_homedir[i]); + Test->repl->ssh_node(i, true, str); Test->repl->ssh_node(i, false, "kinit mariadb/maxscale.test@MAXSCALE.TEST -k -t /etc/krb5.keytab"); } @@ -131,6 +139,11 @@ int main(int argc, char *argv[]) "echo select User,Host from mysql.user | mysql -uusr1 -h maxscale.maxscale.test -P 4009"), "Error executing query against Read Connection Slave\n"); + for (int i = 0; i < Test->repl->N; i++) + { + Test->repl->ssh_node(i, true, "sudo rm -f /etc/my.cnf.d/kerb.cnf"); + } + int rval = Test->global_result; delete Test; return rval; diff --git a/maxscale-system-test/mxs1516.cpp b/maxscale-system-test/mxs1516.cpp index af9c0e84f..887013373 100644 --- a/maxscale-system-test/mxs1516.cpp +++ b/maxscale-system-test/mxs1516.cpp @@ -17,6 +17,9 @@ int main(int argc, char** argv) test.repl->connect(); test.repl->change_master(1, 0); + // Give the monitor some time to detect it + sleep(5); + test.add_result(execute_query_silent(test.conn_master, "SELECT 1") == 0, "Query should fail"); // Change the master back to the original one From a83b36ca45823ef3a9f7d8cded8c9fb9ae649dd9 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Thu, 1 Feb 2018 11:44:17 +0200 Subject: [PATCH 43/54] Use 64 bits for storing server id In debug mode, when scanning the server id from a string, check that resulting number is 32bit. Also, when querying the server id, query the global version. Now, if a super user modifies the server id the monitor will notice it. Server id:s in gtid:s are handled similarly. --- .../modules/monitor/mariadbmon/mysql_mon.cc | 112 +++++++++++------- 1 file changed, 71 insertions(+), 41 deletions(-) diff --git a/server/modules/monitor/mariadbmon/mysql_mon.cc b/server/modules/monitor/mariadbmon/mysql_mon.cc index b89574c2a..e4dd9c5f2 100644 --- a/server/modules/monitor/mariadbmon/mysql_mon.cc +++ b/server/modules/monitor/mariadbmon/mysql_mon.cc @@ -18,10 +18,11 @@ #define MXS_MODULE_NAME "mariadbmon" #include "../mysqlmon.h" +#include +#include #include #include #include -#include #include #include #include @@ -121,6 +122,7 @@ static bool can_replicate_from(MYSQL_MONITOR* mon, static bool wait_cluster_stabilization(MYSQL_MONITOR* mon, MXS_MONITORED_SERVER* new_master, const ServerVector& slaves, int seconds_remaining); static string get_connection_errors(const ServerVector& servers); +static int64_t scan_server_id(const char* id_string); static bool report_version_err = true; static const char* hb_table_name = "maxscale_schema.replication_heartbeat"; @@ -148,15 +150,18 @@ static const char CN_REPLICATION_PASSWORD[] = "replication_password"; /** Default master failure verification timeout */ #define DEFAULT_MASTER_FAILURE_TIMEOUT "10" +/** Server id default value */ +static const int64_t SERVER_ID_UNKNOWN = -1; + class Gtid { public: uint32_t domain; - uint32_t server_id; + int64_t server_id; // Is actually 32bit unsigned. 0 is only used by server versions <= 10.1 uint64_t sequence; Gtid() : domain(0) - , server_id(0) + , server_id(SERVER_ID_UNKNOWN) , sequence(0) {} @@ -170,7 +175,7 @@ public: */ Gtid(const char* str, int64_t search_domain = -1) : domain(0) - , server_id(0) + , server_id(SERVER_ID_UNKNOWN) , sequence(0) { // Autoselect only allowed with one triplet @@ -195,18 +200,23 @@ public: } bool operator == (const Gtid& rhs) const { - return domain == rhs.domain && server_id == rhs.server_id && sequence == rhs.sequence; + return domain == rhs.domain && + server_id != SERVER_ID_UNKNOWN && server_id == rhs.server_id && + sequence == rhs.sequence; } string to_string() const { std::stringstream ss; - ss << domain << "-" << server_id << "-" << sequence; + if (server_id != SERVER_ID_UNKNOWN) + { + ss << domain << "-" << server_id << "-" << sequence; + } return ss.str(); } private: void parse_triplet(const char* str) { - ss_debug(int rv = ) sscanf(str, "%" PRIu32 "-%" PRIu32 "-%" PRIu64, &domain, &server_id, &sequence); + ss_debug(int rv = ) sscanf(str, "%" PRIu32 "-%" PRId64 "-%" PRIu64, &domain, &server_id, &sequence); ss_dassert(rv == 3); } }; @@ -215,21 +225,22 @@ private: class SlaveStatusInfo { public: - int master_server_id; /**< The master's server_id value. */ - string master_host; /**< Master server host name. */ - int master_port; /**< Master server port. */ - bool slave_io_running; /**< Whether the slave I/O thread is running and connected. */ - bool slave_sql_running; /**< Whether or not the SQL thread is running. */ - string master_log_file; /**< Name of the master binary log file that the I/O thread is currently - * reading from. */ - uint64_t read_master_log_pos; /**< Position up to which the I/O thread has read in the current master - * binary log file. */ - Gtid gtid_io_pos; /**< Gtid I/O position of the slave thread. Only shows the triplet with - * the current master domain. */ - string last_error; /**< Last IO or SQL error encountered. */ + int64_t master_server_id; /**< The master's server_id value. Valid ids are 32bit unsigned. -1 is + * unread/error. */ + string master_host; /**< Master server host name. */ + int master_port; /**< Master server port. */ + bool slave_io_running; /**< Whether the slave I/O thread is running and connected. */ + bool slave_sql_running; /**< Whether or not the SQL thread is running. */ + string master_log_file; /**< Name of the master binary log file that the I/O thread is currently + * reading from. */ + uint64_t read_master_log_pos; /**< Position up to which the I/O thread has read in the current master + * binary log file. */ + Gtid gtid_io_pos; /**< Gtid I/O position of the slave thread. Only shows the triplet with + * the current master domain. */ + string last_error; /**< Last IO or SQL error encountered. */ SlaveStatusInfo() - : master_server_id(0) + : master_server_id(SERVER_ID_UNKNOWN) , master_port(0) , slave_io_running(false) , slave_sql_running(false) @@ -259,7 +270,7 @@ public: class MySqlServerInfo { public: - int server_id; /**< Value of @@server_id */ + int64_t server_id; /**< Value of @@server_id. Valid values are 32bit unsigned. */ int group; /**< Multi-master group where this server belongs, * 0 for servers not in groups */ bool read_only; /**< Value of @@read_only */ @@ -281,7 +292,7 @@ public: mysql_server_version version; /**< Server version, 10.X, 5.5 or 5.1 */ MySqlServerInfo() - : server_id(0) + : server_id(SERVER_ID_UNKNOWN) , group(0) , read_only(false) , slave_configured(false) @@ -297,14 +308,15 @@ public: /** * Calculate how many events are left in the relay log. If gtid_current_pos is ahead of Gtid_IO_Pos, - * or a server_id is zero, an error value is returned. + * or a server_id is unknown, an error value is returned. * * @return Number of events in relay log according to latest queried info. A negative value signifies * an error in the gtid-values. */ int64_t relay_log_events() { - if (slave_status.gtid_io_pos.server_id != 0 && gtid_current_pos.server_id != 0 && + if (slave_status.gtid_io_pos.server_id != SERVER_ID_UNKNOWN && + gtid_current_pos.server_id != SERVER_ID_UNKNOWN && slave_status.gtid_io_pos.domain == gtid_current_pos.domain && slave_status.gtid_io_pos.sequence >= gtid_current_pos.sequence) { @@ -318,10 +330,10 @@ bool uses_gtid(MYSQL_MONITOR* mon, MXS_MONITORED_SERVER* mon_server, json_t** er { bool rval = false; const MySqlServerInfo* info = get_server_info(mon, mon_server); - if (info->slave_status.gtid_io_pos.server_id == 0) + if (info->slave_status.gtid_io_pos.server_id == SERVER_ID_UNKNOWN) { string slave_not_gtid_msg = string("Slave server ") + mon_server->server->unique_name + - " is not using gtid replication or master server id is 0."; + " is not using gtid replication."; PRINT_MXS_JSON_ERROR(error_out, "%s", slave_not_gtid_msg.c_str()); } else @@ -1147,12 +1159,12 @@ static void diagnostics(DCB *dcb, const MXS_MONITOR *mon) { MySqlServerInfo *serv_info = get_server_info(handle, db); dcb_printf(dcb, "Server: %s\n", db->server->unique_name); - dcb_printf(dcb, "Server ID: %d\n", serv_info->server_id); + dcb_printf(dcb, "Server ID: %" PRId64 "\n", serv_info->server_id); dcb_printf(dcb, "Read only: %s\n", serv_info->read_only ? "ON" : "OFF"); dcb_printf(dcb, "Slave configured: %s\n", serv_info->slave_configured ? "YES" : "NO"); dcb_printf(dcb, "Slave IO running: %s\n", serv_info->slave_status.slave_io_running ? "YES" : "NO"); dcb_printf(dcb, "Slave SQL running: %s\n", serv_info->slave_status.slave_sql_running ? "YES" : "NO"); - dcb_printf(dcb, "Master ID: %d\n", serv_info->slave_status.master_server_id); + dcb_printf(dcb, "Master ID: %" PRId64 "\n", serv_info->slave_status.master_server_id); dcb_printf(dcb, "Master binlog file: %s\n", serv_info->slave_status.master_log_file.c_str()); dcb_printf(dcb, "Master binlog position: %lu\n", serv_info->slave_status.read_master_log_pos); @@ -1280,7 +1292,7 @@ static bool do_show_slave_status(MYSQL_MONITOR* mon, } MYSQL_RES* result; - int master_server_id = -1; + int64_t master_server_id = SERVER_ID_UNKNOWN; int nconfigured = 0; int nrunning = 0; @@ -1336,11 +1348,7 @@ static bool do_show_slave_status(MYSQL_MONITOR* mon, if (serv_info->slave_status.slave_io_running && server_version != MYSQL_SERVER_VERSION_51) { /* Get Master_Server_Id */ - master_server_id = atoi(row[i_master_server_id]); - if (master_server_id == 0) - { - master_server_id = -1; - } + master_server_id = scan_server_id(row[i_master_server_id]); } if (server_version == MYSQL_SERVER_VERSION_100) @@ -1418,7 +1426,7 @@ static bool slave_receiving_events(MYSQL_MONITOR* handle) { ss_dassert(handle->master); bool received_event = false; - long master_id = handle->master->server->node_id; + int64_t master_id = handle->master->server->node_id; for (MXS_MONITORED_SERVER* server = handle->monitor->monitored_servers; server; server = server->next) { MySqlServerInfo* info = get_server_info(handle, server); @@ -4230,7 +4238,7 @@ static bool do_switchover(MYSQL_MONITOR* mon, MXS_MONITORED_SERVER* current_mast */ static void read_server_variables(MXS_MONITORED_SERVER* database, MySqlServerInfo* serv_info) { - string query = "SELECT @@server_id, @@read_only;"; + string query = "SELECT @@global.server_id, @@read_only;"; int columns = 2; if (serv_info->version == MYSQL_SERVER_VERSION_100) { @@ -4245,16 +4253,16 @@ static void read_server_variables(MXS_MONITORED_SERVER* database, MySqlServerInf StringVector row; if (query_one_row(database, query.c_str(), columns, &row)) { - uint32_t server_id = 0; - ss_debug(int rv = ) sscanf(row[ind_id].c_str(), "%" PRIu32, &server_id); - ss_dassert(rv == 1 && (row[ind_ro] == "0" || row[ind_ro] == "1")); + int64_t server_id = scan_server_id(row[ind_id].c_str()); database->server->node_id = server_id; serv_info->server_id = server_id; + + ss_dassert(row[ind_ro] == "0" || row[ind_ro] == "1"); serv_info->read_only = (row[ind_ro] == "1"); if (columns == 3) { uint32_t domain = 0; - ss_debug(rv = ) sscanf(row[ind_domain].c_str(), "%" PRIu32, &domain); + ss_debug(int rv = ) sscanf(row[ind_domain].c_str(), "%" PRIu32, &domain); ss_dassert(rv == 1); serv_info->gtid_domain_id = domain; } @@ -4284,7 +4292,7 @@ static bool can_replicate_from(MYSQL_MONITOR* mon, // The following are not sufficient requirements for replication to work, they only cover the basics. // If the servers have diverging histories, the redirection will seem to succeed but the slave IO // thread will stop in error. - if (slave_gtid.server_id != 0 && master_gtid.server_id != 0 && + if (slave_gtid.server_id != SERVER_ID_UNKNOWN && master_gtid.server_id != SERVER_ID_UNKNOWN && slave_gtid.domain == master_gtid.domain && slave_gtid.sequence <= master_info->gtid_current_pos.sequence) { @@ -4486,3 +4494,25 @@ static bool cluster_can_be_joined(MYSQL_MONITOR* mon) { return (mon->master != NULL && SERVER_IS_MASTER(mon->master->server) && mon->master_gtid_domain >= 0); } + +/** + * Scan a server id from a string. + * + * @param id_string + * @return Server id, or -1 if scanning fails + */ +static int64_t scan_server_id(const char* id_string) +{ + int64_t server_id = SERVER_ID_UNKNOWN; + ss_debug(int rv = ) sscanf(id_string, "%" PRId64, &server_id); + ss_dassert(rv == 1); + // Server id can be 0, which was even the default value until 10.2.1. + // KB is a bit hazy on this, but apparently when replicating, the server id should not be 0. Not sure, + // so MaxScale allows this. +#if defined(SS_DEBUG) + const int64_t SERVER_ID_MIN = std::numeric_limits::min(); + const int64_t SERVER_ID_MAX = std::numeric_limits::max(); +#endif + ss_dassert(server_id >= SERVER_ID_MIN && server_id <= SERVER_ID_MAX); + return server_id; +} From 943c82b33be16859575b6fe79295991e1e77f7dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 1 Feb 2018 12:55:30 +0200 Subject: [PATCH 44/54] Extend cdc_datatypes test The test now also creates TIME type values and checks that they are converted correctly. Also added NULL value tests for all values and made required adjustments to the code. --- .../cdc_datatypes/cdc_datatypes.cpp | 32 ++++++++++++++++--- .../cdc_datatypes/cdc_result.h | 6 ++-- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp b/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp index 5695d6d7a..c749fa88d 100644 --- a/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp +++ b/maxscale-system-test/cdc_datatypes/cdc_datatypes.cpp @@ -28,6 +28,7 @@ static const char* integer_values[] = "-1", "20", "-20", + "NULL", NULL }; @@ -47,6 +48,7 @@ static const char* decimal_values[] = "-1.5", "20.5", "-20.5", + "NULL", NULL }; @@ -65,7 +67,7 @@ static const char* string_values[] = { "\"Hello world!\"", "\"The quick brown fox jumps over the lazy dog\"", -// "\"The Unicode should work: äöåǢ\"", + "NULL", NULL }; @@ -85,26 +87,27 @@ static const char* binary_values[] = "\"Hello world!\"", "\"The quick brown fox jumps over the lazy dog\"", "NULL", -// "\"The Unicode should work: äöåǢ\"", -// "\"These should work for binary types: ⦿☏☃☢😤😂\"", NULL }; static const char* datetime_types[] = { + "DATETIME", "DATETIME(1)", "DATETIME(2)", "DATETIME(3)", "DATETIME(4)", "DATETIME(5)", "DATETIME(6)", - "TIMESTAMP", + // TODO: Fix test setup to use same timezone + // "TIMESTAMP", NULL }; static const char* datetime_values[] = { "'2018-01-01 11:11:11'", + "NULL", NULL }; @@ -117,6 +120,26 @@ static const char* date_types[] = static const char* date_values[] = { "'2018-01-01'", + "NULL", + NULL +}; + +static const char* time_types[] = +{ + "TIME", + "TIME(1)", + "TIME(2)", + "TIME(3)", + "TIME(4)", + "TIME(5)", + "TIME(6)", + NULL +}; + +static const char* time_values[] = +{ + "'12:00:00'", + "NULL", NULL }; @@ -132,6 +155,7 @@ struct { binary_types, binary_values }, { datetime_types, datetime_values }, { date_types, date_values }, + { time_types, time_values }, { 0 } }; diff --git a/maxscale-system-test/cdc_datatypes/cdc_result.h b/maxscale-system-test/cdc_datatypes/cdc_result.h index 2475230d6..03c209855 100644 --- a/maxscale-system-test/cdc_datatypes/cdc_result.h +++ b/maxscale-system-test/cdc_datatypes/cdc_result.h @@ -39,8 +39,10 @@ public: bool operator ==(const TestOutput& output) const { return m_value == output.getValue() || - (m_type.find("BLOB") != std::string::npos && - output.getValue().length() == 0); + (m_type.find("BLOB") != std::string::npos && output.getValue().length() == 0) || + // A NULL timestamp appears to be inserted as NOW() by default in 10.2, a NULL INT is + // inserted as 0 and a NULL string gets converted into an empty string by the CDC system + (m_value == "NULL" && (output.getValue().empty() || m_type == "TIMESTAMP" || output.getValue() == "0")); } bool operator !=(const TestOutput& output) const From 7093a5bdf82bd4ecefe6d3ebb3e7188599227658 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 1 Feb 2018 13:46:28 +0200 Subject: [PATCH 45/54] Fix CREATE TABLE tokenization The token skipping function did not check for a period or an opening parenthesis when parsing the test. Also fixed a debug assertion when only NULL values were inserted. --- server/modules/routing/avrorouter/avro_rbr.c | 19 ++++++++++++++++++- .../modules/routing/avrorouter/avro_schema.c | 2 +- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_rbr.c b/server/modules/routing/avrorouter/avro_rbr.c index dc72be06a..b34b0f83c 100644 --- a/server/modules/routing/avrorouter/avro_rbr.c +++ b/server/modules/routing/avrorouter/avro_rbr.c @@ -498,6 +498,23 @@ int get_metadata_len(uint8_t type) } \ }while(false) +// Debug function for checking whether a row event consists of only NULL values +static bool all_fields_null(uint8_t* null_bitmap, int ncolumns) +{ + bool rval = true; + + for (long i = 0; i < ncolumns; i++) + { + if (!bit_is_set(null_bitmap, ncolumns, i)) + { + rval = false; + break; + } + } + + return rval; +} + /** * @brief Extract the values from a single row in a row event * @@ -525,7 +542,7 @@ uint8_t* process_row_event_data(TABLE_MAP *map, TABLE_CREATE *create, avro_value /** Store the null value bitmap */ uint8_t *null_bitmap = ptr; ptr += (ncolumns + 7) / 8; - ss_dassert(ptr < end); + ss_dassert(ptr < end || (bit_is_set(null_bitmap, ncolumns, 0))); char trace[ncolumns][768]; memset(trace, 0, sizeof(trace)); diff --git a/server/modules/routing/avrorouter/avro_schema.c b/server/modules/routing/avrorouter/avro_schema.c index 962a394f7..21b91e038 100644 --- a/server/modules/routing/avrorouter/avro_schema.c +++ b/server/modules/routing/avrorouter/avro_schema.c @@ -1251,7 +1251,7 @@ static void skip_token(const char** saved) { const char* ptr = *saved; - while (*ptr && !isspace(*ptr)) + while (*ptr && !isspace(*ptr) && *ptr != '(' && *ptr != '.') { ptr++; } From ebf0d6fc5fc8e6862fd3e0b8da413884c4a57c68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 1 Feb 2018 00:39:45 +0200 Subject: [PATCH 46/54] Close client DCB with a hangup in the backend protocol Directly closing the client DCB in the backend protocol modules is not correct anymore as the state of the session doesn't change when the client DCB is closed. By propagating the shutdown of the session with a fake hangup to the client DCB, the closing of the DCB is done only once. Added debug assertions that make sure all DCBs are closed only once. Removed redundant code in the backend protocol error handling code. --- server/core/backend.cc | 2 ++ .../protocol/MySQL/mariadbbackend/mysql_backend.c | 11 +++-------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/server/core/backend.cc b/server/core/backend.cc index 3dd915ca6..6ffa3777d 100644 --- a/server/core/backend.cc +++ b/server/core/backend.cc @@ -43,6 +43,8 @@ Backend::~Backend() void Backend::close(close_type type) { + ss_dassert(m_dcb->n_close == 0); + if (!m_closed) { m_closed = true; diff --git a/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.c b/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.c index 02a428348..6769d503f 100644 --- a/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.c +++ b/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.c @@ -599,13 +599,8 @@ static void gw_reply_on_error(DCB *dcb, mxs_auth_state_t state) MXS_SESSION *session = dcb->session; CHK_SESSION(session); - if (!dcb->dcb_errhandle_called) - { - do_handle_error(dcb, ERRACT_REPLY_CLIENT, - "Authentication with backend failed. Session will be closed."); - session->state = SESSION_STATE_STOPPING; - dcb->dcb_errhandle_called = true; - } + do_handle_error(dcb, ERRACT_REPLY_CLIENT, + "Authentication with backend failed. Session will be closed."); } /** @@ -1348,7 +1343,7 @@ static int gw_backend_close(DCB *dcb) session->state == SESSION_STATE_STOPPING && session->client_dcb->state == DCB_STATE_POLLING) { - dcb_close(session->client_dcb); + poll_fake_hangup_event(session->client_dcb); } return 1; From 9f0a6912330d03bfd27011d2a4f40cc97f58215f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 1 Feb 2018 01:32:22 +0200 Subject: [PATCH 47/54] Always stop the session by closing the client DCB By always starting the session shutdown process by stopping the client DCB, the manipulation of the session state can be removed from the backend protocol modules and replaced with a fake hangup event. Delivering this event via the core allows the actual dcb_close call on the client DCB to be done only when the client DCB is being handled by a worker. --- server/core/session.cc | 5 +--- .../MySQL/mariadbbackend/mysql_backend.c | 2 +- .../MySQL/mariadbclient/mysql_client.cc | 25 ++++++------------- 3 files changed, 10 insertions(+), 22 deletions(-) diff --git a/server/core/session.cc b/server/core/session.cc index 23af09b38..ad10d9b24 100644 --- a/server/core/session.cc +++ b/server/core/session.cc @@ -320,10 +320,7 @@ void session_close(MXS_SESSION *session) { if (session->router_session) { - if (session->state != SESSION_STATE_STOPPING) - { - session->state = SESSION_STATE_STOPPING; - } + session->state = SESSION_STATE_STOPPING; MXS_ROUTER_OBJECT* router = session->service->router; MXS_ROUTER* router_instance = session->service->router_instance; diff --git a/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.c b/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.c index 6769d503f..7917d8889 100644 --- a/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.c +++ b/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.c @@ -583,7 +583,7 @@ static void do_handle_error(DCB *dcb, mxs_error_action_t action, const char *err */ if (!succp) { - session->state = SESSION_STATE_STOPPING; + poll_fake_hangup_event(session->client_dcb); } } diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index d9cf55327..77f1368f0 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -1401,29 +1401,20 @@ static int gw_client_close(DCB *dcb) */ static int gw_client_hangup_event(DCB *dcb) { - MXS_SESSION* session; - CHK_DCB(dcb); - session = dcb->session; + MXS_SESSION* session = dcb->session; - if (session != NULL && session->state == SESSION_STATE_ROUTER_READY) + if (session) { CHK_SESSION(session); + if (session->state != SESSION_STATE_DUMMY && !session_valid_for_pool(session)) + { + // The client did not send a COM_QUIT packet + modutil_send_mysql_err_packet(dcb, 0, 0, 1927, "08S01", "Connection killed by MaxScale"); + } + dcb_close(dcb); } - if (session != NULL && session->state == SESSION_STATE_STOPPING) - { - goto retblock; - } - - if (session->state != SESSION_STATE_DUMMY && !session_valid_for_pool(session)) - { - // The client did not send a COM_QUIT packet - modutil_send_mysql_err_packet(dcb, 0, 0, 1927, "08S01", "Connection killed by MaxScale"); - } - dcb_close(dcb); - -retblock: return 1; } From 8c8aaeae8e786b53bf27ffab64b6d20a58acdabd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 1 Feb 2018 10:16:25 +0200 Subject: [PATCH 48/54] Prevent double closing of client DCB If the client is closing the connection but routing the COM_QUIT fails, the DCB would be closed twice. --- server/modules/protocol/MySQL/mariadbclient/mysql_client.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 77f1368f0..69ac28c1d 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -1086,8 +1086,7 @@ gw_read_finish_processing(DCB *dcb, GWBUF *read_buffer, uint64_t capabilities) dcb_close(dcb); MXS_ERROR("Routing the query failed. Session will be closed."); } - - if (proto->current_command == MXS_COM_QUIT) + else if (proto->current_command == MXS_COM_QUIT) { /** Close router session which causes closing of backends */ dcb_close(dcb); From 0d037b2c24add551d854ca375f59fec58f56b8aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 1 Feb 2018 11:05:38 +0200 Subject: [PATCH 49/54] Add configuration reset command Resetting the settings makes it possible to start a test from a known good configuration. --- maxscale-system-test/mariadb_nodes.cpp | 19 +++++++++++++++++++ maxscale-system-test/mariadb_nodes.h | 12 ++++++++++++ 2 files changed, 31 insertions(+) diff --git a/maxscale-system-test/mariadb_nodes.cpp b/maxscale-system-test/mariadb_nodes.cpp index 8d417376f..6071f7a63 100644 --- a/maxscale-system-test/mariadb_nodes.cpp +++ b/maxscale-system-test/mariadb_nodes.cpp @@ -1324,6 +1324,25 @@ void Mariadb_nodes::add_server_setting(int node, const char* setting) ssh_node_f(node, true, "sudo sed -i '$a %s' /etc/my.cnf.d/server.cnf", setting); } +void Mariadb_nodes::reset_server_settings(int node) +{ + std::stringstream ss; + ss << test_dir << "/mdbci/cnf/server" << node + 1 << ".cnf"; + + // Note: This is a CentOS specific path + ssh_node(node, "sudo rm -rf /etc/my.cnf.d/*", true); + copy_to_node(node, ss.str().c_str(), "~/"); + ssh_node_f(node, false, "sudo mv ~/server%d.cnf /etc/my.cnf.d/", node + 1); +} + +void Mariadb_nodes::reset_server_settings() +{ + for (int i = 0; i < N; i++) + { + reset_server_settings(i); + } +} + /** * @brief extract_version_from_string Tries to find MariaDB server version number in the output of 'mysqld --version' * Function does not allocate any memory diff --git a/maxscale-system-test/mariadb_nodes.h b/maxscale-system-test/mariadb_nodes.h index 1cf00c1ae..fb0517ae9 100644 --- a/maxscale-system-test/mariadb_nodes.h +++ b/maxscale-system-test/mariadb_nodes.h @@ -396,6 +396,18 @@ public: */ void add_server_setting(int node, const char* setting); + /** + * Restore the original configuration for this server + * + * @param node Node to restore + */ + void reset_server_settings(int node); + + /** + * Restore the original configuration for all servers + */ + void reset_server_settings(); + /** * @brief revert_nodes_snapshot Execute MDBCI snapshot revert command for all nodes * @return true in case of success From 84845a57320ddea6f5fb2f47d049844a335faeff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 1 Feb 2018 11:18:12 +0200 Subject: [PATCH 50/54] Use basic configuration for mxs812_2 The test repeatedly blocks and unblocks a master which goes unnoticed by the monitor due to the 10 second read timeouts in the monitor configuration in the longblob template. The replication template uses the default timeouts which makes the test actually exercise the functionality that it is intended to test. --- maxscale-system-test/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 7cc81e0c9..ec32c6e71 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -483,7 +483,7 @@ add_test_script(mxs791_galera.sh mxs791_galera.sh galera LABELS UNSTABLE HEAVY G add_test_executable(mxs812_1.cpp mxs812_1 longblob LABELS readwritesplit REPL_BACKEND) # Checks "Current no. of conns" maxadmin output after long blob inserting -add_test_executable(mxs812_2.cpp mxs812_2 longblob LABELS readwritesplit REPL_BACKEND) +add_test_executable(mxs812_2.cpp mxs812_2 replication LABELS readwritesplit REPL_BACKEND) # Execute prepared statements while master is blocked, checks "Current no. of conns" after the test add_test_executable(mxs822_maxpasswd.cpp mxs822_maxpasswd maxpasswd LABELS maxscale REPL_BACKEND) From d7d2d3349deab4a2e18e79cc236cc07f6cf8deb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 1 Feb 2018 11:47:25 +0200 Subject: [PATCH 51/54] Reset server configuration in check_backend By resetting the configuration in the check_backend test, the backend servers are guaranteed to use the correct configuration when started. --- maxscale-system-test/check_backend.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/maxscale-system-test/check_backend.cpp b/maxscale-system-test/check_backend.cpp index e78cafbdc..f9a62372c 100644 --- a/maxscale-system-test/check_backend.cpp +++ b/maxscale-system-test/check_backend.cpp @@ -11,6 +11,9 @@ int main(int argc, char *argv[]) TestConnections * Test = new TestConnections(argc, argv); + // Reset server settings by replacing the config files + Test->repl->reset_server_settings(); + std::string src = std::string(test_dir) + "/mdbci/add_core_cnf.sh"; Test->maxscales->copy_to_node(0, src.c_str(), Test->maxscales->access_homedir[0]); Test->maxscales->ssh_node_f(0, true, "%s/add_core_cnf.sh %s", Test->maxscales->access_homedir[0], From 7ae931ce9c1b615477bd68c1947b87bf1e57de82 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Wed, 31 Jan 2018 16:14:14 +0200 Subject: [PATCH 52/54] MXS-1635 Allow using specific address when connecting In some cases you might want to use a specific address/interface when connecting to a server instead of the default one. With the global parameter 'local_address' it can now be specified which address to use. --- .../Getting-Started/Configuration-Guide.md | 11 ++++ .../MaxScale-2.1.14-Release-Notes.md | 63 +++++++++++++++++++ include/maxscale/config.h | 1 + server/core/config.c | 4 ++ server/core/utils.c | 37 ++++++++++- 5 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 Documentation/Release-Notes/MaxScale-2.1.14-Release-Notes.md diff --git a/Documentation/Getting-Started/Configuration-Guide.md b/Documentation/Getting-Started/Configuration-Guide.md index f367176a6..947741d2d 100644 --- a/Documentation/Getting-Started/Configuration-Guide.md +++ b/Documentation/Getting-Started/Configuration-Guide.md @@ -537,6 +537,17 @@ This will log all statements that cannot be parsed completely. This may be useful if you suspect that MariaDB MaxScale routes statements to the wrong server (e.g. to a slave instead of to a master). +#### `local_address` + +What specific local address/interface to use when connecting to servers. + +This can be used for ensuring that MaxScale uses a particular interface +when connecting to servers, in case the computer MaxScale is running on +has multiple interfaces. +``` +local_address=192.168.1.254 +``` + ### Service A service represents the database service that MariaDB MaxScale offers to the diff --git a/Documentation/Release-Notes/MaxScale-2.1.14-Release-Notes.md b/Documentation/Release-Notes/MaxScale-2.1.14-Release-Notes.md new file mode 100644 index 000000000..eb9f4426f --- /dev/null +++ b/Documentation/Release-Notes/MaxScale-2.1.14-Release-Notes.md @@ -0,0 +1,63 @@ +# MariaDB MaxScale 2.1.14 Release Notes + +Release 2.1.14 is a GA release. + +This document describes the changes in release 2.1.14, when compared +to release [2.1.13](MaxScale-2.1.13-Release-Notes.md). + +If you are upgrading from release 2.0, please also read the following +release notes: + +* [2.1.13](./MaxScale-2.1.13-Release-Notes.md) +* [2.1.12](./MaxScale-2.1.12-Release-Notes.md) +* [2.1.11](./MaxScale-2.1.11-Release-Notes.md) +* [2.1.10](./MaxScale-2.1.10-Release-Notes.md) +* [2.1.9](./MaxScale-2.1.9-Release-Notes.md) +* [2.1.8](./MaxScale-2.1.8-Release-Notes.md) +* [2.1.7](./MaxScale-2.1.7-Release-Notes.md) +* [2.1.6](./MaxScale-2.1.6-Release-Notes.md) +* [2.1.5](./MaxScale-2.1.5-Release-Notes.md) +* [2.1.4](./MaxScale-2.1.4-Release-Notes.md) +* [2.1.3](./MaxScale-2.1.3-Release-Notes.md) +* [2.1.2](./MaxScale-2.1.2-Release-Notes.md) +* [2.1.1](./MaxScale-2.1.1-Release-Notes.md) +* [2.1.0](./MaxScale-2.1.0-Release-Notes.md) + +For any problems you encounter, please consider submitting a bug report at +[Jira](https://jira.mariadb.org). + +## New Features + +### Local Address + +It is now possible to specify what local address MaxScale should +use when connecting to servers. Please refer to the documentation +for [details](../Getting-Started/Configuration-Guide.md#local_address). + +## Bug fixes + +[Here is a list of bugs fixed in MaxScale 2.1.14.](https://jira.mariadb.org/issues/?jql=project%20%3D%20MXS%20AND%20issuetype%20%3D%20Bug%20AND%20status%20%3D%20Closed%20AND%20fixVersion%20%3D%202.1.14) + +* [MXS-1627](https://jira.mariadb.org/browse/MXS-1627) MySQLAuth loads users that use authentication plugins +* [MXS-1620](https://jira.mariadb.org/browse/MXS-1620) CentOS package symbols are stripped +* [MXS-1602](https://jira.mariadb.org/browse/MXS-1602) cannot connect to maxinfo with python client +* [MXS-1601](https://jira.mariadb.org/browse/MXS-1601) maxinfo crash at execute query 'flush;' +* [MXS-1600](https://jira.mariadb.org/browse/MXS-1600) maxscale it seen to not coop well with lower-case-table-names=1 on cnf +* [MXS-1576](https://jira.mariadb.org/browse/MXS-1576) Maxscale crashes when starting if .avro and .avsc files are present +* [MXS-1543](https://jira.mariadb.org/browse/MXS-1543) Avrorouter doesn't detect MIXED or STATEMENT format replication +* [MXS-1416](https://jira.mariadb.org/browse/MXS-1416) maxscale should not try to do anything when started with --config-check + +## Packaging + +RPM and Debian packages are provided for the Linux distributions supported by +MariaDB Enterprise. + +Packages can be downloaded [here](https://mariadb.com/resources/downloads). + +## Source Code + +The source code of MaxScale is tagged at GitHub with a tag, which is identical +with the version of MaxScale. For instance, the tag of version X.Y.Z of MaxScale +is maxscale-X.Y.Z. + +The source code is available [here](https://github.com/mariadb-corporation/MaxScale). diff --git a/include/maxscale/config.h b/include/maxscale/config.h index 98930914b..7a1265470 100644 --- a/include/maxscale/config.h +++ b/include/maxscale/config.h @@ -77,6 +77,7 @@ typedef struct char* qc_args; /**< Arguments for the query classifier */ int query_retries; /**< Number of times a interrupted query is retried */ time_t query_retry_timeout; /**< Timeout for query retries */ + char* local_address; /**< Local address to use when connecting */ } MXS_CONFIG; /** diff --git a/server/core/config.c b/server/core/config.c index 19132f2c8..bd31b3f6b 100644 --- a/server/core/config.c +++ b/server/core/config.c @@ -1401,6 +1401,10 @@ handle_global_item(const char *name, const char *value) MXS_FREE(v); } } + else if (strcmp(name, "local_address") == 0) + { + gateway.local_address = MXS_STRDUP_A(value); + } else { for (i = 0; lognames[i].name; i++) diff --git a/server/core/utils.c b/server/core/utils.c index 47570d48f..194a43e4f 100644 --- a/server/core/utils.c +++ b/server/core/utils.c @@ -41,6 +41,7 @@ #include #include +#include #include #include #include @@ -986,15 +987,47 @@ int open_network_socket(enum mxs_socket_type type, struct sockaddr_storage *addr memcpy(addr, ai->ai_addr, ai->ai_addrlen); set_port(addr, port); + freeaddrinfo(ai); + if ((type == MXS_SOCKET_NETWORK && !configure_network_socket(so)) || (type == MXS_SOCKET_LISTENER && !configure_listener_socket(so))) { close(so); so = -1; } - } + else if (type == MXS_SOCKET_NETWORK) + { + MXS_CONFIG* config = config_get_global_options(); - freeaddrinfo(ai); + if (config->local_address) + { + if ((rc = getaddrinfo(config->local_address, NULL, &hint, &ai)) == 0) + { + struct sockaddr_storage local_address = {}; + + memcpy(&local_address, ai->ai_addr, ai->ai_addrlen); + freeaddrinfo(ai); + + if (bind(so, (struct sockaddr*)&local_address, sizeof(local_address)) == 0) + { + MXS_INFO("Bound connecting socket to \"%s\".", config->local_address); + } + else + { + MXS_ERROR("Could not bind connecting socket to local address \"%s\", " + "connecting to server using default local address: %s", + config->local_address, mxs_strerror(errno)); + } + } + else + { + MXS_ERROR("Could not get address information for local address \"%s\", " + "connecting to server using default local address: %s", + config->local_address, mxs_strerror(errno)); + } + } + } + } } #else From facb8d60f7676fdac3115170926e797dd16ad24d Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 2 Feb 2018 14:54:21 +0200 Subject: [PATCH 53/54] MXS-1635 Test program for local_address Tests that local_address is taken into account. However, at the time of writing the maxscale VM does not have two usable IP addresses, so we only test that explicitly specifying an IP-address does not break things. Locally it has been confirmed that this indeed works the way it is supposed to. --- maxscale-system-test/.gitignore | 1 + maxscale-system-test/CMakeLists.txt | 3 + .../cnf/maxscale.cnf.template.local_address | 90 +++++ maxscale-system-test/local_address.cpp | 331 ++++++++++++++++++ 4 files changed, 425 insertions(+) create mode 100644 maxscale-system-test/cnf/maxscale.cnf.template.local_address create mode 100644 maxscale-system-test/local_address.cpp diff --git a/maxscale-system-test/.gitignore b/maxscale-system-test/.gitignore index 78424a324..c291ed6ba 100644 --- a/maxscale-system-test/.gitignore +++ b/maxscale-system-test/.gitignore @@ -97,6 +97,7 @@ kill_master large_insert_hang load_balancing load_balancing_galera +local_address long_sysbench longblob lots_of_rows diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 9177d7daf..aaf91c21a 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -761,6 +761,9 @@ add_test_executable_notest(create_rds.cpp create_rds replication LABELS EXTERN_B # start sysbench ageints RWSplit for infinite execution add_test_executable_notest(long_sysbench.cpp long_sysbench replication LABELS readwritesplit REPL_BACKEND) +# test effect of local_address in configuration file +add_test_executable(local_address.cpp local_address local_address LABELS REPL_BACKEND) + configure_file(templates.h.in templates.h @ONLY) include(CTest) diff --git a/maxscale-system-test/cnf/maxscale.cnf.template.local_address b/maxscale-system-test/cnf/maxscale.cnf.template.local_address new file mode 100644 index 000000000..44bf3ae38 --- /dev/null +++ b/maxscale-system-test/cnf/maxscale.cnf.template.local_address @@ -0,0 +1,90 @@ +[maxscale] +threads=###threads### +###local_address### + +[MySQL Monitor] +type=monitor +module=mysqlmon +###repl51### +servers=server1,server2,server3,server4 +user=maxskysql +passwd=skysql +monitor_interval=1000 +detect_stale_master=false + +[RW Split Router] +type=service +router=readwritesplit +servers=server1,server2,server3,server4 +user=maxskysql +passwd=skysql +router_options=slave_selection_criteria=LEAST_GLOBAL_CONNECTIONS +max_slave_connections=1 + +[Read Connection Router Slave] +type=service +router=readconnroute +router_options=slave +servers=server1,server2,server3,server4 +user=maxskysql +passwd=skysql + +[Read Connection Router Master] +type=service +router=readconnroute +router_options=master +servers=server1,server2,server3,server4 +user=maxskysql +passwd=skysql + +[RW Split Listener] +type=listener +service=RW Split Router +protocol=MySQLClient +port=4006 + +[Read Connection Listener Slave] +type=listener +service=Read Connection Router Slave +protocol=MySQLClient +port=4009 + +[Read Connection Listener Master] +type=listener +service=Read Connection Router Master +protocol=MySQLClient +port=4008 + +[CLI] +type=service +router=cli + +[CLI Listener] +type=listener +service=CLI +protocol=maxscaled +socket=default + +[server1] +type=server +address=###node_server_IP_1### +port=###node_server_port_1### +protocol=MySQLBackend + +[server2] +type=server +address=###node_server_IP_2### +port=###node_server_port_2### +protocol=MySQLBackend + +[server3] +type=server +address=###node_server_IP_3### +port=###node_server_port_3### +protocol=MySQLBackend + +[server4] +type=server +address=###node_server_IP_4### +port=###node_server_port_4### +protocol=MySQLBackend diff --git a/maxscale-system-test/local_address.cpp b/maxscale-system-test/local_address.cpp new file mode 100644 index 000000000..9eafb184c --- /dev/null +++ b/maxscale-system-test/local_address.cpp @@ -0,0 +1,331 @@ +/* + * Copyright (c) 2018 MariaDB Corporation Ab + * + * Use of this software is governed by the Business Source License included + * in the LICENSE.TXT file and at www.mariadb.com/bsl11. + * + * Change Date: 2019-07-01 + * + * On the date above, in accordance with the Business Source License, use + * of this software will be governed by version 2 or later of the General + * Public License. + */ + +#include "testconnections.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace std; + +namespace +{ + +template +void to_collection(string s, const string& delimiter, T* pT) +{ + size_t pos; + + while ((pos = s.find(delimiter)) != std::string::npos) + { + pT->push_back(s.substr(0, pos)); + s.erase(0, pos + delimiter.length()); + } + + if (s.length() != 0) + { + pT->push_back(s); + } +} + +string& ltrim(std::string& s) +{ + s.erase(s.begin(), std::find_if(s.begin(), s.end(), + std::not1(std::ptr_fun(std::isspace)))); + return s; +} + +string& rtrim(std::string& s) +{ + s.erase(std::find_if(s.rbegin(), s.rend(), + std::not1(std::ptr_fun(std::isspace))).base(), s.end()); + return s; +} + +string& trim(std::string& s) +{ + return ltrim(rtrim(s)); +} + +string extract_ip(string s) +{ + // 's' looks something like: " inet 127.0.0.1/..."; + s = s.substr(9); // => "127.0.0.1/..."; + s = s.substr(0, s.find_first_of('/')); // => "127.0.0.1" + return s; +} + +void get_maxscale_ips(TestConnections& test, vector* pIps) +{ + string output(test.ssh_maxscale_output(false, "ip addr|fgrep inet|fgrep -v ::")); + + to_collection(output, "\n", pIps); + transform(pIps->begin(), pIps->end(), pIps->begin(), extract_ip); + + pIps->erase(find(pIps->begin(), pIps->end(), "127.0.0.1")); +} + +} + +namespace +{ + +void drop_user(TestConnections& test, const string& user, const string& host) +{ + string stmt("DROP USER IF EXISTS "); + + stmt += "'"; + stmt += user; + stmt += "'@'"; + stmt += host; + stmt += "'"; + test.try_query(test.conn_rwsplit, stmt.c_str()); +} + +void create_user(TestConnections& test, const string& user, const string& password, const string& host) +{ + string stmt("CREATE USER "); + + stmt += "'"; + stmt += user; + stmt += "'@'"; + stmt += host; + stmt += "'"; + stmt += " IDENTIFIED BY "; + stmt += "'"; + stmt += password; + stmt += "'"; + test.try_query(test.conn_rwsplit, stmt.c_str()); +} + +void grant_access(TestConnections& test, const string& user, const string& host) +{ + string stmt("GRANT SELECT, INSERT, UPDATE ON *.* TO "); + + stmt += "'"; + stmt += user; + stmt += "'@'"; + stmt += host; + stmt += "'"; + test.try_query(test.conn_rwsplit, stmt.c_str()); + + test.try_query(test.conn_rwsplit, "FLUSH PRIVILEGES"); +} + +void create_user_and_grants(TestConnections& test, + const string& user, const string& password, const string& host) +{ + test.tprintf("Creating user: %s@%s", user.c_str(), host.c_str()); + + drop_user(test, user, host); + create_user(test, user, password, host); + grant_access(test, user, host); +} + +bool select_user(MYSQL* pMysql, string* pUser) +{ + bool rv = false; + + if (mysql_query(pMysql, "SELECT USER()") == 0) + { + MYSQL_RES* pRes = mysql_store_result(pMysql); + + if (mysql_num_rows(pRes) == 1) + { + MYSQL_ROW row = mysql_fetch_row(pRes); + *pUser = row[0]; + rv = true; + } + + mysql_free_result(pRes); + + while (mysql_next_result(pMysql) == 0) + { + MYSQL_RES* pRes = mysql_store_result(pMysql); + mysql_free_result(pRes); + } + } + + return rv; +} + +bool can_connect_to_maxscale(const char* zHost, int port, const char* zUser, const char* zPassword) +{ + bool could_connect = false; + + MYSQL* pMysql = mysql_init(NULL); + + if (pMysql) + { + unsigned int timeout = 5; + mysql_options(pMysql, MYSQL_OPT_CONNECT_TIMEOUT, &timeout); + mysql_options(pMysql, MYSQL_OPT_READ_TIMEOUT, &timeout); + mysql_options(pMysql, MYSQL_OPT_WRITE_TIMEOUT, &timeout); + + if (mysql_real_connect(pMysql, zHost, zUser, zPassword, NULL, port, NULL, 0)) + { + string user; + if (select_user(pMysql, &user)) + { + could_connect = true; + } + else + { + cout << "Could not 'SELECT USER()' as '" << zUser << "': " << mysql_error(pMysql) << endl; + } + } + else + { + cout << "Could not connect as '" << zUser << "': " << mysql_error(pMysql) << endl; + } + + mysql_close(pMysql); + } + + return could_connect; +} + +string get_local_ip(TestConnections& test) +{ + string output(test.ssh_maxscale_output(false, "nslookup maxscale|fgrep Server:|sed s/Server://")); + return trim(output); +} + +void start_maxscale_with_local_address(TestConnections& test, + const string& replace, + const string& with) +{ + string command("sed -i s/"); + command += replace; + command += "/"; + command += with; + command += "/ "; + command += "/etc/maxscale.cnf"; + + test.ssh_maxscale_output(true, command.c_str()); + + test.start_maxscale(); +} + +void test_connecting(TestConnections& test, + const char* zUser, const char* zPassword, const char* zHost, + bool should_be_able_to) +{ + bool could_connect = can_connect_to_maxscale(test.maxscale_IP, test.rwsplit_port, zUser, zPassword); + + if (!could_connect && should_be_able_to) + { + test.assert(false, "%s@%s should have been able to connect, but wasn't.", zUser, zHost); + } + else if (could_connect && !should_be_able_to) + { + test.assert(false, "%s@%s should NOT have been able to connect, but was.", zUser, zHost); + } + else + { + if (could_connect) + { + test.tprintf("%s@%s could connect, as expected.", zUser, zHost); + } + else + { + test.tprintf("%s@%s could NOT connect, as expected.", zUser, zHost); + } + } +} + +void run_test(TestConnections& test, const string& ip1, const string& ip2) +{ + test.connect_maxscale(); + + string local_ip = get_local_ip(test); + + const char* zUser1 = "alice"; + const char* zUser2 = "bob"; + const char* zPassword1 = "alicepwd"; + const char* zPassword2 = "bobpwd"; + + create_user_and_grants(test, zUser1, zPassword1, ip1); + create_user_and_grants(test, zUser1, zPassword1, local_ip); + create_user_and_grants(test, zUser2, zPassword2, ip2); + create_user_and_grants(test, zUser2, zPassword2, local_ip); + + test.tprintf("\n"); + test.tprintf("Testing default; alice should be able to access, bob not."); + + test_connecting(test, zUser1, zPassword1, ip1.c_str(), true); + test_connecting(test, zUser2, zPassword2, ip2.c_str(), false); + + test.close_maxscale_connections(); + test.stop_maxscale(); + + test.tprintf("\n"); + test.tprintf("Testing with local_address=%s; alice should be able to access, bob not.", + ip1.c_str()); + + string local_address_ip1 = "local_address=" + ip1; + start_maxscale_with_local_address(test, "###local_address###", local_address_ip1); + test.connect_maxscale(); + + test_connecting(test, zUser1, zPassword1, ip1.c_str(), true); + test_connecting(test, zUser2, zPassword2, ip2.c_str(), false); + + test.close_maxscale_connections(); + test.stop_maxscale(); + + test.tprintf("\n"); + test.tprintf("WARNING: Other IP-address not tested, as usable IP-address not available."); + +#ifdef USABLE_SECOND_IP_ADDRESS_ON_MAXSCALE_NODE_IS_AVAILABLE + test.tprintf("\n"); + test.tprintf("\nTesting with local_address=%s, bob should be able to access, alice not.", + ip2.c_str()); + + string local_address_ip2 = "local_address=" + ip2; + start_maxscale_with_local_address(test, local_address_ip1, local_address_ip2); + test.connect_maxscale(); + + test_connecting(test, zUser1, zPassword1, ip1.c_str(), false); + test_connecting(test, zUser2, zPassword2, ip2.c_str(), true); + + test.close_maxscale_connections(); + test.stop_maxscale(); +#endif +} + +} + +int main(int argc, char** argv) +{ + TestConnections test(argc, argv); + + vector ips; + get_maxscale_ips(test, &ips); + + if (ips.size() >= 2) + { + run_test(test, ips[0], ips[1]); + } + else + { + test.assert(false, "MaxScale node does not have at least two IP-addresses."); + } + + return test.global_result; +} From e670596486857e65c4140cfac47d0fa4c8c39622 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 5 Feb 2018 09:33:40 +0200 Subject: [PATCH 54/54] Fix local_address It included the list instead of the vector header. --- maxscale-system-test/local_address.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maxscale-system-test/local_address.cpp b/maxscale-system-test/local_address.cpp index 9eafb184c..442876daf 100644 --- a/maxscale-system-test/local_address.cpp +++ b/maxscale-system-test/local_address.cpp @@ -15,7 +15,7 @@ #include #include #include -#include +#include #include #include #include