From f3e00431de0cb4bb545921bdc62e11f60a09ded6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 20 Feb 2018 15:35:52 +0200 Subject: [PATCH 1/6] Fix MXS-1418 regression If a server is removed from a service, readconnroute will not verify that the server it is connected to is still the same root master. This fixes the regression of MXS-1418. --- server/modules/routing/readconnroute/readconnroute.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/server/modules/routing/readconnroute/readconnroute.c b/server/modules/routing/readconnroute/readconnroute.c index 788da87d2..82f61ed42 100644 --- a/server/modules/routing/readconnroute/readconnroute.c +++ b/server/modules/routing/readconnroute/readconnroute.c @@ -551,12 +551,20 @@ static inline bool connection_is_valid(ROUTER_INSTANCE* inst, ROUTER_CLIENT_SES* if (SERVER_IS_RUNNING(router_cli_ses->backend->server) && (router_cli_ses->backend->server->status & inst->bitmask & inst->bitvalue)) { - if (inst->bitvalue & SERVER_MASTER) + if ((inst->bitvalue & SERVER_MASTER) && router_cli_ses->backend->active) { + // If we're using an active master server, verify that it is still a master rval = router_cli_ses->backend == get_root_master(inst->service->dbref); } else { + /** + * Either we don't use master type servers or the server reference + * is deactivated. We let deactivated connection close gracefully + * so we simply assume it is OK. This allows a server to be taken + * out of use in a manner that won't cause errors to the connected + * clients. + */ rval = true; } } From 1ecd791887994209eb29e56e1271f8c407cd0cdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 21 Feb 2018 09:35:42 +0200 Subject: [PATCH 2/6] MXS-1678: Store master_id even when IO thread is stopped When the IO thread of a relay master is stopped, the knowledge that it is not a real master but a relay master is lost. To prevent this loss of information, the master server's server_id value should always be stored if it is available. --- server/modules/monitor/mysqlmon/mysql_mon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/modules/monitor/mysqlmon/mysql_mon.c b/server/modules/monitor/mysqlmon/mysql_mon.c index 14226c14b..c8bb5bd22 100644 --- a/server/modules/monitor/mysqlmon/mysql_mon.c +++ b/server/modules/monitor/mysqlmon/mysql_mon.c @@ -457,7 +457,7 @@ static inline void monitor_mysql_db(MXS_MONITOR_SERVERS* database, MYSQL_SERVER_ * root master server. * Please note, there could be no slaves at all if Slave_SQL_Running == 'No' */ - if (serv_info->slave_io && server_version != MYSQL_SERVER_VERSION_51) + if (server_version != MYSQL_SERVER_VERSION_51) { /* Get Master_Server_Id */ master_id = atol(row[i_master_id]); From 8e31b30d19e14854f30e5a618f74c161172677d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 21 Feb 2018 10:43:12 +0200 Subject: [PATCH 3/6] MXS-1678: Add test case Added test case that checks that relay master status is not lost when IO thread is stopped. --- maxscale-system-test/CMakeLists.txt | 4 + maxscale-system-test/mxs1678_relay_master.cpp | 86 +++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 maxscale-system-test/mxs1678_relay_master.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index aaf91c21a..73d22d729 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -530,6 +530,10 @@ add_test_executable(mxs1543.cpp mxs1543 avro LABELS REPL_BACKEND) # https://jira.mariadb.org/browse/MXS-1585 add_test_executable(mxs1585.cpp mxs1585 mxs1585 LABELS REPL_BACKEND) +# MXS-1678: Stopping IO thread on relay master causes it to be promoted as master +# https://jira.mariadb.org/browse/MXS-1678 +add_test_executable(mxs1678_relay_master.cpp mxs1678_relay_master replication LABELS REPL_BACKEND) + # 'namedserverfilter' test add_test_executable(namedserverfilter.cpp namedserverfilter namedserverfilter LABELS namedserverfilter LIGHT REPL_BACKEND) diff --git a/maxscale-system-test/mxs1678_relay_master.cpp b/maxscale-system-test/mxs1678_relay_master.cpp new file mode 100644 index 000000000..bee2a0a97 --- /dev/null +++ b/maxscale-system-test/mxs1678_relay_master.cpp @@ -0,0 +1,86 @@ +/** + * MXS-1678: Stopping IO thread on relay master causes it to be promoted as master + * + * https://jira.mariadb.org/browse/MXS-1678 + */ +#include "testconnections.h" +#include +#include + +typedef std::set StringSet; + +// Note: This is backported from 2.2 and can be replaced with MaxScales::get_server_status once merged +StringSet state(TestConnections& test, const char* name) +{ + StringSet rval; + char* res = test.ssh_maxscale_output(true, "maxadmin list servers|grep \'%s\'", name); + char* pipe = strrchr(res, '|'); + + if (res && pipe) + { + pipe++; + char* tok = strtok(pipe, ","); + + while (tok) + { + char* p = tok; + char *end = strchr(tok, '\n'); + if (!end) + { + end = strchr(tok, '\0'); + } + + // Trim leading whitespace + while (p < end && isspace(*p)) + { + p++; + } + + // Trim trailing whitespace + while (end > tok && isspace(*end)) + { + *end-- = '\0'; + } + + rval.insert(p); + tok = strtok(NULL, ",\n"); + } + } + + free(res); + + return rval; +} + + +int main(int argc, char** argv) +{ + TestConnections test(argc, argv); + test.repl->connect(); + execute_query(test.repl->nodes[3], "STOP SLAVE"); + execute_query(test.repl->nodes[3], "CHANGE MASTER TO MASTER_HOST='%s', MASTER_PORT=%d", + test.repl->IP_private[2], test.repl->port[2]); + execute_query(test.repl->nodes[3], "START SLAVE"); + + StringSet master = {"Master", "Running"}; + StringSet slave = {"Slave", "Running"}; + StringSet relay_master = {"Relay Master", "Slave", "Running"}; + + test.tprintf("Checking before stopping IO thread"); + test.add_result(state(test, "server1") != master, "server1 is not a master"); + test.add_result(state(test, "server2") != slave, "server2 is not a slave"); + test.add_result(state(test, "server3") != relay_master, "server3 is not a relay master"); + test.add_result(state(test, "server4") != slave, "server4 is not a slave"); + + execute_query(test.repl->nodes[2], "STOP SLAVE IO_THREAD"); + sleep(10); + + test.tprintf("Checking after stopping IO thread"); + test.add_result(state(test, "server1") != master, "server1 is not a master"); + test.add_result(state(test, "server2") != slave, "server2 is not a slave"); + test.add_result(state(test, "server3") != relay_master, "server3 is not a relay master"); + test.add_result(state(test, "server4") != slave, "server4 is not a slave"); + + test.repl->fix_replication(); + return test.global_result; +} From 03eb30fbc62500a69e0cd4b6d4831e144a4a87b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 22 Feb 2018 10:06:29 +0200 Subject: [PATCH 4/6] Check SHOW DATABASES privilege on startup MySQLAuth requires the SHOW DATABASES privilege to see all the databases so it should be checked that the current user has the permission. A missing permission will cause errors that are hard to resolve. --- .../modules/authenticator/MySQLAuth/dbusers.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/server/modules/authenticator/MySQLAuth/dbusers.c b/server/modules/authenticator/MySQLAuth/dbusers.c index 0310ff928..8829e5170 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.c +++ b/server/modules/authenticator/MySQLAuth/dbusers.c @@ -627,6 +627,25 @@ static bool check_server_permissions(SERVICE *service, SERVER* server, } } + // Check whether the current user has the SHOW DATABASES privilege + if (mxs_mysql_query(mysql, "SELECT show_db_priv FROM mysql.user " + "WHERE CONCAT(user, '@', host) = CURRENT_USER()") == 0) + { + MYSQL_RES* res = mysql_use_result(mysql); + if (res) + { + MYSQL_ROW row = mysql_fetch_row(res); + + if (row && strcasecmp(row[0], "Y") != 0) + { + MXS_WARNING("[%s] User '%s' is missing the SHOW DATABASES privilege.", + service->name, user); + } + + mysql_free_result(res); + } + } + mysql_close(mysql); return rval; From b7cc793c400672a0faef668c239dcc0d98553e90 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Tue, 27 Feb 2018 00:03:01 +0200 Subject: [PATCH 5/6] MXS-1688 Add test that reveals DATE_ADD problem --- query_classifier/test/maxscale.test | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/query_classifier/test/maxscale.test b/query_classifier/test/maxscale.test index 6b9b59449..e2fda41d4 100644 --- a/query_classifier/test/maxscale.test +++ b/query_classifier/test/maxscale.test @@ -86,4 +86,12 @@ select * from db1.t1 union select * from db2.t2; select names from t; call p1(); -call p1(@var); \ No newline at end of file +call p1(@var); + +# MXS-1688 +select id from db2.t1 where DATE_SUB("2017-06-15", INTERVAL 10 DAY) < "2017-06-15"; +select id from db2.t1 where SUBDATE("2017-06-15", INTERVAL 10 DAY) < "2017-06-15"; +select id from db2.t1 where DATE_ADD("2017-06-15", INTERVAL 10 DAY) < "2017-06-15"; +select id from db2.t1 where ADDDATE("2017-06-15", INTERVAL 10 DAY) < "2017-06-15"; +SELECT '2008-12-31 23:59:59' + INTERVAL 1 SECOND; +SELECT '2005-01-01' - INTERVAL 1 SECOND; \ No newline at end of file From 0c206ff42843558f13d17d78f112913d27f8a91b Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Tue, 27 Feb 2018 02:55:05 +0200 Subject: [PATCH 6/6] MXS-1688 Handle ...INTERVAL N "INTERVAL N " is now handled as an expression in itself and as asuch will cause both statements such as "SELECT '2008-12-31 23:59:59' + INTERVAL 1 SECOND;" and "select id from db2.t1 where DATE_ADD("2017-06-15", INTERVAL 10 DAY) < "2017-06-15";" to be handled correctly. The compare test program contains some heuristic checking, as the the embedded parser will in all cases report date manipulation as the use of the add_date_interval() function. --- .../qc_sqlite/sqlite-src-3110100/src/parse.y | 4 +- query_classifier/test/compare.cc | 60 ++++++++++++++++++- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/query_classifier/qc_sqlite/sqlite-src-3110100/src/parse.y b/query_classifier/qc_sqlite/sqlite-src-3110100/src/parse.y index f3f7e0007..fad997933 100644 --- a/query_classifier/qc_sqlite/sqlite-src-3110100/src/parse.y +++ b/query_classifier/qc_sqlite/sqlite-src-3110100/src/parse.y @@ -1948,10 +1948,10 @@ expr(A) ::= expr(X) BITAND|BITOR|LSHIFT|RSHIFT(OP) expr(Y). expr(A) ::= expr(X) PLUS|MINUS(OP) expr(Y). {spanBinaryExpr(&A,pParse,@OP,&X,&Y);} %ifdef MAXSCALE -expr(A) ::= expr(X) PLUS|MINUS INTERVAL INTEGER id. { +expr(A) ::= INTERVAL INTEGER(X) id. { // Here we could check that id is one of MICROSECOND, SECOND, MINUTE // HOUR, DAY, WEEK, etc. - A=X; + spanExpr(&A, pParse, @X, &X); } %endif expr(A) ::= expr(X) STAR|SLASH|REM(OP) expr(Y). diff --git a/query_classifier/test/compare.cc b/query_classifier/test/compare.cc index 7b6fc611e..35185a0fd 100644 --- a/query_classifier/test/compare.cc +++ b/query_classifier/test/compare.cc @@ -1069,6 +1069,11 @@ public: return rv; } + const std::string& name() const + { + return m_name; + } + void print(ostream& out) const { out << m_name; @@ -1118,6 +1123,19 @@ bool operator == (const QcFunctionInfo& lhs, const QcFunctionInfo& rhs) return lhs.eq(rhs); } +void collect_missing_function_names(const std::set& one, + const std::set& other, + std::set* pNames) +{ + for (std::set::const_iterator i = one.begin(); i != one.end(); ++i) + { + if (other.count(*i) == 0) + { + pNames->insert(i->name()); + } + } +} + bool compare_get_function_info(QUERY_CLASSIFIER* pClassifier1, GWBUF* pCopy1, QUERY_CLASSIFIER* pClassifier2, GWBUF* pCopy2) { @@ -1151,7 +1169,47 @@ bool compare_get_function_info(QUERY_CLASSIFIER* pClassifier1, GWBUF* pCopy1, } else { - ss << "ERR: " << f1 << " != " << f2; + std::set names1; + collect_missing_function_names(f1, f2, &names1); + + std::set names2; + collect_missing_function_names(f2, f1, &names2); + + bool real_error = false; + + // We assume that names1 are from the qc_mysqlembedded and names2 from qc_sqlite. + // The embedded parser reports all date_add(), adddate(), date_sub() and subdate() + // functions as date_add_interval(). Further, all "DATE + INTERVAL ..." cases become + // use of date_add_interval() functions. + for (std::set::iterator i = names1.begin(); i != names1.end(); ++i) + { + if (*i == "date_add_interval") + { + if ((names2.count("date_add") == 0) && + (names2.count("adddate") == 0) && + (names2.count("date_sub") == 0) && + (names2.count("subdate") == 0) && + (names2.count("+") == 0) && + (names2.count("-") == 0)) + { + real_error = true; + } + } + else + { + real_error = true; + } + } + + if (real_error) + { + ss << "ERR: " << f1 << " != " << f2; + } + else + { + ss << "Ok : " << f1 << " != " << f2; + success = true; + } } report(success, ss.str());