From c88aa11e11a6e1b59c764d2ab658230b5ccd9512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 28 May 2018 11:38:59 +0300 Subject: [PATCH 1/6] Copy FDE events in avrorouter Commit 67386980e327ad063b24cb55971cf44f4930e241 caused the actual events to be ignored. This meant that the larger event size was assumed for all events. In most cases this works but it is not the correct way to do it. --- server/modules/routing/avrorouter/avro_file.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/modules/routing/avrorouter/avro_file.c b/server/modules/routing/avrorouter/avro_file.c index d37da88ce..404737d72 100644 --- a/server/modules/routing/avrorouter/avro_file.c +++ b/server/modules/routing/avrorouter/avro_file.c @@ -647,6 +647,10 @@ avro_binlog_end_t avro_read_all_events(AVRO_INSTANCE *router) int n_events = hdr.event_size - event_header_length - BLRM_FDE_EVENT_TYPES_OFFSET - FDE_EXTRA_BYTES; uint8_t* checksum = ptr + hdr.event_size - event_header_length - FDE_EXTRA_BYTES; + // Precaution to prevent writing too much in case new events are added + int real_len = MXS_MIN(n_events, sizeof(router->event_type_hdr_lens)); + memcpy(router->event_type_hdr_lens, ptr + BLRM_FDE_EVENT_TYPES_OFFSET, real_len); + router->event_types = n_events; router->binlog_checksum = checksum[0]; } From cddcc6d7d56f282c4da2d7013e4dd77989331e5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Sat, 2 Jun 2018 22:24:51 +0300 Subject: [PATCH 2/6] MXS-1896: Distinct LOAD DATA LOCAL INFILE from LOAD DATA INFILE The two operations return different types of results and need to be treated differently in order for them to be handled correctly in 2.2. This fixes the unexpected internal state errors that happened in all 2.2 versions due to a wrong assumption made by readwritesplit. This fix is not necessary for newer versions as the LOAD DATA LOCAL INFILE processing is done with a simpler, and more robust, method. --- include/maxscale/query_classifier.h | 1 + maxscale-system-test/CMakeLists.txt | 4 +++ .../mxs1896_load_data_infile.cpp | 32 +++++++++++++++++++ .../qc_mysqlembedded/qc_mysqlembedded.cc | 2 +- query_classifier/qc_sqlite/qc_sqlite.cc | 10 +++--- .../qc_sqlite/sqlite-src-3110100/src/parse.y | 12 ++++--- server/core/query_classifier.cc | 3 ++ .../modules/filter/dbfwfilter/dbfwfilter.hh | 3 +- .../readwritesplit/rwsplit_route_stmt.cc | 2 +- .../schemarouter/schemaroutersession.cc | 2 +- 10 files changed, 57 insertions(+), 14 deletions(-) create mode 100644 maxscale-system-test/mxs1896_load_data_infile.cpp diff --git a/include/maxscale/query_classifier.h b/include/maxscale/query_classifier.h index fce557878..116e59a1b 100644 --- a/include/maxscale/query_classifier.h +++ b/include/maxscale/query_classifier.h @@ -104,6 +104,7 @@ typedef enum qc_query_op QUERY_OP_EXPLAIN, QUERY_OP_GRANT, QUERY_OP_INSERT, + QUERY_OP_LOAD_LOCAL, QUERY_OP_LOAD, QUERY_OP_REVOKE, QUERY_OP_SELECT, diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 827a588fc..ff121b91a 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -658,6 +658,10 @@ add_test_executable(mxs1831_unknown_param.cpp mxs1831_unknown_param replication # https://jira.mariadb.org/browse/MXS-1873 add_test_executable(mxs1873_large_sescmd.cpp mxs1873_large_sescmd replication LABELS readwritesplit REPL_BACKEND) +# MXS-1896: LOAD DATA INFILE is mistaken for LOAD DATA LOCAL INFILE +# https://jira.mariadb.org/browse/MXS-1896 +add_test_executable(mxs1896_load_data_infile.cpp mxs1896_load_data_infile replication LABELS readwritesplit REPL_BACKEND) + # 'namedserverfilter' test add_test_executable(namedserverfilter.cpp namedserverfilter namedserverfilter LABELS namedserverfilter LIGHT REPL_BACKEND) diff --git a/maxscale-system-test/mxs1896_load_data_infile.cpp b/maxscale-system-test/mxs1896_load_data_infile.cpp new file mode 100644 index 000000000..31bf863b5 --- /dev/null +++ b/maxscale-system-test/mxs1896_load_data_infile.cpp @@ -0,0 +1,32 @@ +/** + * MXS-1896: LOAD DATA INFILE is mistaken for LOAD DATA LOCAL INFILE + * + * https://jira.mariadb.org/browse/MXS-1896 + */ + +#include "testconnections.h" + +int main(int argc, char** argv) +{ + TestConnections test(argc, argv); + + test.set_timeout(30); + test.maxscales->connect(); + + test.try_query(test.maxscales->conn_rwsplit[0], "DROP TABLE IF EXISTS test.t1"); + test.try_query(test.maxscales->conn_rwsplit[0], "CREATE TABLE test.t1(id INT)"); + test.try_query(test.maxscales->conn_rwsplit[0], "INSERT INTO test.t1 VALUES (1), (2), (3)"); + test.try_query(test.maxscales->conn_rwsplit[0], "SELECT * FROM test.t1 INTO OUTFILE '/tmp/test.csv'"); + test.try_query(test.maxscales->conn_rwsplit[0], "LOAD DATA INFILE '/tmp/test.csv' INTO TABLE test.t1"); + test.try_query(test.maxscales->conn_rwsplit[0], "DROP TABLE test.t1"); + + test.maxscales->disconnect(); + + // Clean up the generated files + for (int i = 0; i < 4; i++) + { + test.repl->ssh_node_f(i, true, "rm -f /tmp/test.csv"); + } + + return test.global_result; +} diff --git a/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc b/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc index 8ea9ce177..07c813cf2 100644 --- a/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc +++ b/query_classifier/qc_mysqlembedded/qc_mysqlembedded.cc @@ -2001,7 +2001,7 @@ int32_t qc_mysql_get_operation(GWBUF* querybuf, int32_t* operation) break; case SQLCOM_LOAD: - *operation = QUERY_OP_LOAD; + *operation = QUERY_OP_LOAD_LOCAL; break; case SQLCOM_GRANT: diff --git a/query_classifier/qc_sqlite/qc_sqlite.cc b/query_classifier/qc_sqlite/qc_sqlite.cc index af49d29d8..a71e501e2 100644 --- a/query_classifier/qc_sqlite/qc_sqlite.cc +++ b/query_classifier/qc_sqlite/qc_sqlite.cc @@ -2327,13 +2327,13 @@ public: exposed_sqlite3SrcListDelete(pParse->db, pFullName); } - void maxscaleLoadData(Parse* pParse, SrcList* pFullName) + void maxscaleLoadData(Parse* pParse, SrcList* pFullName, int local) { ss_dassert(this_thread.initialized); m_status = QC_QUERY_PARSED; m_type_mask = QUERY_TYPE_WRITE; - m_operation = QUERY_OP_LOAD; + m_operation = local ? QUERY_OP_LOAD_LOCAL: QUERY_OP_LOAD; if (pFullName) { @@ -3299,7 +3299,7 @@ extern void maxscaleExecuteImmediate(Parse*, Token* pName, ExprSpan* pExprSpan, extern void maxscaleExplain(Parse*, Token* pNext); extern void maxscaleFlush(Parse*, Token* pWhat); extern void maxscaleHandler(Parse*, mxs_handler_t, SrcList* pFullName, Token* pName); -extern void maxscaleLoadData(Parse*, SrcList* pFullName); +extern void maxscaleLoadData(Parse*, SrcList* pFullName, int local); extern void maxscaleLock(Parse*, mxs_lock_t, SrcList*); extern void maxscalePrepare(Parse*, Token* pName, Expr* pStmt); extern void maxscalePrivileges(Parse*, int kind); @@ -4201,14 +4201,14 @@ void maxscaleHandler(Parse* pParse, mxs_handler_t type, SrcList* pFullName, Toke QC_EXCEPTION_GUARD(pInfo->maxscaleHandler(pParse, type, pFullName, pName)); } -void maxscaleLoadData(Parse* pParse, SrcList* pFullName) +void maxscaleLoadData(Parse* pParse, SrcList* pFullName, int local) { QC_TRACE(); QcSqliteInfo* pInfo = this_thread.pInfo; ss_dassert(pInfo); - QC_EXCEPTION_GUARD(pInfo->maxscaleLoadData(pParse, pFullName)); + QC_EXCEPTION_GUARD(pInfo->maxscaleLoadData(pParse, pFullName, local)); } void maxscaleLock(Parse* pParse, mxs_lock_t type, SrcList* pTables) 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 21707bee2..bd08e3219 100644 --- a/query_classifier/qc_sqlite/sqlite-src-3110100/src/parse.y +++ b/query_classifier/qc_sqlite/sqlite-src-3110100/src/parse.y @@ -120,7 +120,7 @@ extern void maxscaleExecuteImmediate(Parse*, Token* pName, ExprSpan* pExprSpan, extern void maxscaleExplain(Parse*, Token* pNext); extern void maxscaleFlush(Parse*, Token* pWhat); extern void maxscaleHandler(Parse*, mxs_handler_t, SrcList* pFullName, Token* pName); -extern void maxscaleLoadData(Parse*, SrcList* pFullName); +extern void maxscaleLoadData(Parse*, SrcList* pFullName, int local); extern void maxscaleLock(Parse*, mxs_lock_t, SrcList*); extern void maxscalePrepare(Parse*, Token* pName, Expr* pStmt); extern void maxscalePrivileges(Parse*, int kind); @@ -2913,19 +2913,21 @@ handler ::= HANDLER nm(X) CLOSE. { //////////////////////// The LOAD DATA INFILE statement //////////////////////////////////// // +%type ld_local_opt {int} + cmd ::= load_data. ld_priority_opt ::= . ld_priority_opt ::= LOW_PRIORITY. ld_priority_opt ::= CONCURRENT. -ld_local_opt ::= . -ld_local_opt ::= LOCAL. +ld_local_opt(A) ::= . {A = 0;} +ld_local_opt(A) ::= LOCAL. {A = 1;} ld_charset_opt ::= . ld_charset_opt ::= CHARACTER SET ids. -load_data ::= LOAD DATA ld_priority_opt ld_local_opt +load_data ::= LOAD DATA ld_priority_opt ld_local_opt(Y) INFILE STRING ignore_or_replace_opt INTO TABLE fullname(X) /* ld_partition_opt */ @@ -2935,7 +2937,7 @@ load_data ::= LOAD DATA ld_priority_opt ld_local_opt /* ld_col_name_or_user_var_opt */ /* ld_set */. { - maxscaleLoadData(pParse, X); + maxscaleLoadData(pParse, X, Y); } //////////////////////// The LOCK/UNLOCK statement //////////////////////////////////// diff --git a/server/core/query_classifier.cc b/server/core/query_classifier.cc index 2cf3dfa96..787abd21b 100644 --- a/server/core/query_classifier.cc +++ b/server/core/query_classifier.cc @@ -389,6 +389,9 @@ const char* qc_op_to_string(qc_query_op_t op) case QUERY_OP_LOAD: return "QUERY_OP_LOAD"; + case QUERY_OP_LOAD_LOCAL: + return "QUERY_OP_LOAD_LOCAL"; + case QUERY_OP_REVOKE: return "QUERY_OP_REVOKE"; diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.hh b/server/modules/filter/dbfwfilter/dbfwfilter.hh index 738ef128d..1cc3dec9a 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.hh +++ b/server/modules/filter/dbfwfilter/dbfwfilter.hh @@ -85,6 +85,7 @@ static inline fw_op_t qc_op_to_fw_op(qc_query_op_t op) case QUERY_OP_INSERT: return FW_OP_INSERT; + case QUERY_OP_LOAD_LOCAL: case QUERY_OP_LOAD: return FW_OP_LOAD; @@ -288,4 +289,4 @@ char* create_error(const char* format, ...); */ bool rule_matches(Dbfw* my_instance, DbfwSession* my_session, GWBUF *queue, SRule rule, char* query); -bool rule_is_active(SRule rule); \ No newline at end of file +bool rule_is_active(SRule rule); diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc index b0ba764af..3b2059e8e 100644 --- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc +++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc @@ -881,7 +881,7 @@ handle_multi_temp_and_load(RWSplitSession *rses, GWBUF *querybuf, else if (is_packet_a_query(packet_type)) { qc_query_op_t queryop = qc_get_operation(querybuf); - if (queryop == QUERY_OP_LOAD) + if (queryop == QUERY_OP_LOAD_LOCAL) { rses->load_data_state = LOAD_DATA_START; rses->rses_load_data_sent = 0; diff --git a/server/modules/routing/schemarouter/schemaroutersession.cc b/server/modules/routing/schemarouter/schemaroutersession.cc index 092cb1ecd..dd77f2d91 100644 --- a/server/modules/routing/schemarouter/schemaroutersession.cc +++ b/server/modules/routing/schemarouter/schemaroutersession.cc @@ -424,7 +424,7 @@ int32_t SchemaRouterSession::routeQuery(GWBUF* pPacket) /** We know where to route this query */ SSRBackend bref = get_bref_from_dcb(target_dcb); - if (op == QUERY_OP_LOAD) + if (op == QUERY_OP_LOAD_LOCAL) { m_load_target = bref->backend()->server; } From da4397a50132e9d5189b6968bb83f6c4950abcbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 4 Jun 2018 06:32:30 +0300 Subject: [PATCH 3/6] MXS-1743: Expand test case to cover load balancing The test case now verifies that the servers are actually load balanced correctly. This test reveals a problem in the readconnroute; the master is always preferred over slaves if one is available with router_options=master,slave. --- .../mxs1743_rconn_bitmask.cpp | 40 ++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/maxscale-system-test/mxs1743_rconn_bitmask.cpp b/maxscale-system-test/mxs1743_rconn_bitmask.cpp index 3d3bba969..949783662 100644 --- a/maxscale-system-test/mxs1743_rconn_bitmask.cpp +++ b/maxscale-system-test/mxs1743_rconn_bitmask.cpp @@ -4,12 +4,12 @@ * https://jira.mariadb.org/browse/MXS-1743 */ #include "testconnections.h" +#include int main(int argc, char** argv) { TestConnections test(argc, argv); - test.tprintf("Testing with both master and slave up"); test.maxscales->connect(); test.try_query(test.maxscales->conn_master[0], "SELECT 1"); @@ -31,6 +31,44 @@ int main(int argc, char** argv) test.try_query(test.maxscales->conn_master[0], "SELECT 1"); test.maxscales->disconnect(); test.repl->unblock_node(1); + sleep(5); + + test.tprintf("Checking that both the master and slave are used"); + std::vector connections; + + test.repl->connect(); + execute_query_silent(test.repl->nodes[0], "DROP USER IF EXISTS 'mxs1743'@'%'"); + test.try_query(test.repl->nodes[0], "%s", "CREATE USER 'mxs1743'@'%' IDENTIFIED BY 'mxs1743'"); + test.try_query(test.repl->nodes[0], "%s", "GRANT ALL ON *.* TO 'mxs1743'@'%'"); + test.repl->sync_slaves(); + + for (int i = 0; i < 20; i++) + { + connections.push_back(open_conn(test.maxscales->readconn_master_port[0], + test.maxscales->IP[0], "mxs1743", + "mxs1743", false)); + } + + // Give the connections a few seconds to establish + sleep(5); + + std::string query = "SELECT COUNT(*) AS connections FROM information_schema.processlist WHERE user = 'mxs1743'"; + char master_connections[1024]; + char slave_connections[1024]; + find_field(test.repl->nodes[0], query.c_str(), "connections", master_connections); + find_field(test.repl->nodes[1], query.c_str(), "connections", slave_connections); + + test.assert(strcmp(master_connections, slave_connections) == 0, + "Master and slave shoud have the same amount of connections: %s != %s", + master_connections, slave_connections); + + for (auto a: connections) + { + mysql_close(a); + } + + execute_query_silent(test.repl->nodes[0], "DROP USER 'mxs1743'@'%'"); + test.repl->disconnect(); return test.global_result; } From d22f6d7b1c29b30141cc122e3f6e119ff7a98825 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 4 Jun 2018 06:46:38 +0300 Subject: [PATCH 4/6] MXS-1743: Fix load balancing with router_options=master,slave The code that selects the candidate backend always returned the root master if the server bitmask contained the master bit. This should only be done if the master bit is the only bit in the bitmask and when there are other bits, the normal candidate selection code should be used. Also added a query to the expanded test case to make sure the connection actually works. --- maxscale-system-test/mxs1743_rconn_bitmask.cpp | 8 +++++--- server/modules/routing/readconnroute/readconnroute.c | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/maxscale-system-test/mxs1743_rconn_bitmask.cpp b/maxscale-system-test/mxs1743_rconn_bitmask.cpp index 949783662..4543e37cd 100644 --- a/maxscale-system-test/mxs1743_rconn_bitmask.cpp +++ b/maxscale-system-test/mxs1743_rconn_bitmask.cpp @@ -44,9 +44,11 @@ int main(int argc, char** argv) for (int i = 0; i < 20; i++) { - connections.push_back(open_conn(test.maxscales->readconn_master_port[0], - test.maxscales->IP[0], "mxs1743", - "mxs1743", false)); + // Open a connection and make sure it works + MYSQL* conn = open_conn(test.maxscales->readconn_master_port[0], test.maxscales->IP[0], + "mxs1743", "mxs1743", false); + test.try_query(conn, "SELECT 1"); + connections.push_back(conn); } // Give the connections a few seconds to establish diff --git a/server/modules/routing/readconnroute/readconnroute.c b/server/modules/routing/readconnroute/readconnroute.c index 278c65d24..1987fb38e 100644 --- a/server/modules/routing/readconnroute/readconnroute.c +++ b/server/modules/routing/readconnroute/readconnroute.c @@ -344,7 +344,7 @@ newSession(MXS_ROUTER *instance, MXS_SESSION *session) continue; } - if (ref == master_host && (inst->bitvalue & SERVER_MASTER)) + if (ref == master_host && inst->bitvalue == SERVER_MASTER) { /* If option is "master" return only the root Master as there could be * intermediate masters (Relay Servers) and they must not be selected. From 306beb05cdd0373d92c615089a1897427e2ab875 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 4 Jun 2018 11:30:39 +0300 Subject: [PATCH 5/6] MXS-1899: Allow generated [maxscale] sections The generated configuration file contains a [maxscale] section which should not be treated as an error. --- server/core/config.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/core/config.cc b/server/core/config.cc index 48133db96..ea7ad9aaa 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -526,7 +526,7 @@ static int ini_handler(void *userdata, const char *section, const char *name, co if (strcmp(section, CN_GATEWAY) == 0 || strcasecmp(section, CN_MAXSCALE) == 0) { - if (is_root_config_file) + if (is_root_config_file || is_persisted_config) { return handle_global_item(name, value); } From 9d0b1be08b34f67e7dcd0f2177774970285e9072 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 4 Jun 2018 11:58:13 +0300 Subject: [PATCH 6/6] Improve unexpected response error message The message now logs the current command and the query if it is available. --- .../routing/readwritesplit/readwritesplit.cc | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.cc b/server/modules/routing/readwritesplit/readwritesplit.cc index 8ef88e23c..3d8e59f69 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.cc +++ b/server/modules/routing/readwritesplit/readwritesplit.cc @@ -1123,7 +1123,7 @@ static json_t* diagnostics_json(const MXS_ROUTER *instance) return rval; } -static void log_unexpected_response(DCB* dcb, GWBUF* buffer) +static void log_unexpected_response(SRWBackend& backend, GWBUF* buffer) { if (mxs_mysql_is_err_packet(buffer)) { @@ -1138,21 +1138,27 @@ static void log_unexpected_response(DCB* dcb, GWBUF* buffer) if (errcode == ER_CONNECTION_KILLED) { MXS_INFO("Connection from '%s'@'%s' to '%s' was killed", - dcb->session->client_dcb->user, - dcb->session->client_dcb->remote, - dcb->server->unique_name); + backend->dcb()->session->client_dcb->user, + backend->dcb()->session->client_dcb->remote, + backend->name()); } else { MXS_WARNING("Server '%s' sent an unexpected error: %hu, %s", - dcb->server->unique_name, errcode, errstr.c_str()); + backend->name(), errcode, errstr.c_str()); } } else { + char* sql = session_have_stmt(backend->dcb()->session) ? + modutil_get_SQL(backend->dcb()->session->stmt.buffer) : + NULL; MXS_ERROR("Unexpected internal state: received response 0x%02hhx from " - "server '%s' when no response was expected", - mxs_mysql_get_command(buffer), dcb->server->unique_name); + "server '%s' when no response was expected. Command: 0x%02hhx " + "Query: %s", mxs_mysql_get_command(buffer), backend->name(), + backend->current_command(), sql ? sql : ""); + MXS_FREE(sql); + ss_dassert(false); } } @@ -1188,7 +1194,7 @@ static void clientReply(MXS_ROUTER *instance, /** If we receive an unexpected response from the server, the internal * logic cannot handle this situation. Routing the reply straight to * the client should be the safest thing to do at this point. */ - log_unexpected_response(backend_dcb, writebuf); + log_unexpected_response(backend, writebuf); MXS_SESSION_ROUTE_REPLY(backend_dcb->session, writebuf); return; }