From e327282e82754c69ccddabf930e4ba62f45565ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 10 Apr 2018 21:39:17 +0300 Subject: [PATCH 01/16] Don't log warnings for valid SQL The warnings are about what the parser expects, not something the end user should know. --- server/core/internal/trxboundaryparser.hh | 2 +- server/modules/protocol/MySQL/mariadbclient/setsqlmodeparser.hh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/core/internal/trxboundaryparser.hh b/server/core/internal/trxboundaryparser.hh index 8dc3310db..3bfc00fd1 100644 --- a/server/core/internal/trxboundaryparser.hh +++ b/server/core/internal/trxboundaryparser.hh @@ -607,7 +607,7 @@ private: if (m_pI != m_pEnd) { - MXS_WARNING("Non-space data found after semi-colon: '%.*s'.", + MXS_INFO("Non-space data found after semi-colon: '%.*s'.", (int)(m_pEnd - m_pI), m_pI); } diff --git a/server/modules/protocol/MySQL/mariadbclient/setsqlmodeparser.hh b/server/modules/protocol/MySQL/mariadbclient/setsqlmodeparser.hh index 73f63e711..f0400584d 100644 --- a/server/modules/protocol/MySQL/mariadbclient/setsqlmodeparser.hh +++ b/server/modules/protocol/MySQL/mariadbclient/setsqlmodeparser.hh @@ -562,7 +562,7 @@ private: if (m_pI != m_pEnd) { - MXS_WARNING("Non-space data found after semi-colon: '%.*s'.", + MXS_INFO("Non-space data found after semi-colon: '%.*s'.", (int)(m_pEnd - m_pI), m_pI); } From 1eefb46e682d96239646a94f390e5456cd735585 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 10 Apr 2018 22:51:16 +0300 Subject: [PATCH 02/16] MXS-1773: Update internal state when LOAD DATA LOCAL INFILE fails When a LOAD DATA LOCAL INFILE is actively rejected by the server, the server sends an error to the client. This error was not detected and the router was stuck in the special mode that handles LOAD DATA LOCAL INFILE. --- server/modules/routing/readwritesplit/readwritesplit.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/modules/routing/readwritesplit/readwritesplit.cc b/server/modules/routing/readwritesplit/readwritesplit.cc index 1d08000b5..0897eae45 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.cc +++ b/server/modules/routing/readwritesplit/readwritesplit.cc @@ -1150,6 +1150,12 @@ static void clientReply(MXS_ROUTER *instance, SRWBackend& backend = get_backend_from_dcb(rses, backend_dcb); + if (rses->load_data_state == LOAD_DATA_ACTIVE && mxs_mysql_is_err_packet(writebuf)) + { + // Server responded with an error to the LOAD DATA LOCAL INFILE + rses->load_data_state = LOAD_DATA_INACTIVE; + } + if (backend->get_reply_state() == REPLY_STATE_DONE) { /** If we receive an unexpected response from the server, the internal From 8e2208b957694b543befdc26d4083c23c6b0ab54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 11 Apr 2018 08:00:47 +0300 Subject: [PATCH 03/16] Update limitations document Removed old limitations from the document and cleaned up some of the text. --- Documentation/About/Limitations.md | 89 +++++++++--------------------- 1 file changed, 27 insertions(+), 62 deletions(-) diff --git a/Documentation/About/Limitations.md b/Documentation/About/Limitations.md index e7ee13c94..fff8ceaf5 100644 --- a/Documentation/About/Limitations.md +++ b/Documentation/About/Limitations.md @@ -106,10 +106,6 @@ usernames in MariaDB MaxScale. ## Filter limitations -Filters are not guaranteed to receive complete MySQL packets if they are used -with the readconnroute router. This can be fixed by using the readwritesplit -router. - ### Database Firewall limitations (dbfwfilter) The Database Firewall filter does not support multi-statements. Using them will @@ -119,13 +115,18 @@ result in an error being sent to the client. The Tee filter does not support binary protocol prepared statements. The execution of a prepared statements through a service that uses the tee filter is -not guaranteed to produce the same result on the service where the filter -branches to as it does on the original service. +not guaranteed to succeed on the service where the filter branches to as it does +on the original service. + +This possibility exists due to the fact that the binary protocol prepared +statements are identified by a server-generated ID. The ID sent to the client +from the main service is not guaranteed to be the same that is sent by the +branch service. ## Monitor limitations -A server can only be monitored by one monitor. If multiple monitors monitor the -same server, the state of the server is non-deterministic. +A server can only be monitored by one monitor. Two or more monitors monitoring +the same server is considered an error. ### Limitations with Galera Cluster Monitoring (galeramon) @@ -156,9 +157,6 @@ on this issue. ### Limitations in the connection router (readconnroute) -If Master changes (ie. new Master promotion) during current connection, the -router cannot check the change. - Sending of binary data with `LOAD DATA LOCAL INFILE` is not supported. ### Limitations in the Read/Write Splitter (readwritesplit) @@ -172,14 +170,9 @@ LAST_INSERT_ID();` #### JDBC Batched Statements -Readwritesplit does not support execution of JDBC batched statements with -non-INSERT statements mixed in it. This is caused by the fact that -readwritesplit expects that the protocol is idle before another command is sent. - -Most clients conform to this expectation but some JDBC drivers send multiple -requests without waiting for the protocol to be idle. If you are using the -MariaDB Connector/J, add `useBatchMultiSend=false` to the JDBC connection string -to disable batched statement execution. +Readwritesplit does not support pipelining of JDBC batched statements. This is +caused by the fact that readwritesplit executes the statements one at a time to +track the state of the response. #### Prepared Statement Limitations @@ -193,18 +186,9 @@ statements to stall. #### Limitations in multi-statement handling When a multi-statement query is executed through the readwritesplit router, it -will always be routed to the master. With the default configuration, all queries -after a multi-statement query will be routed to the master to prevent possible -reads of false data. - -You can override this behavior with the `strict_multi_stmt=false` router option. -In this mode, the multi-statement queries will still be routed to the master but -individual statements are routed normally. If you use multi-statements and you -know they don't modify the session state in any relevant way, you can disable -this option for better performance. - -For more information, read the -[ReadWriteSplit](../Routers/ReadWriteSplit.md) router documentation. +will always be routed to the master. See +[`strict_multi_stmt`](../Routers/ReadWriteSplit.md#strict_multi_stmt) for more +details. #### Limitations in client session handling @@ -234,22 +218,21 @@ SET autocommit=1|0 ``` There is a possibility for misbehavior. If `USE mytable` is executed in one of -the slaves and fails, it may be due to replication lag rather than the -database not existing. Thus, the same command may produce different result in -different backend servers. The slaves which fail to execute a session command -will be dropped from the active list of slaves for this session to guarantee a -consistent session state across all the servers used by the session. +the slaves and fails, it may be due to replication lag rather than the database +not existing. Thus, the same command may produce different result in different +backend servers. The slaves which fail to execute a session command will be +dropped from the active list of slaves for this session to guarantee a +consistent session state across all the servers used by the session. In +addition, the server will not used again for routing for the duration of the +session. + +The above-mentioned behavior for user variables can be partially controlled with +the configuration parameter `use_sql_variables_in`: -The above-mentioned behavior can be partially controlled with the configuration -parameter `use_sql_variables_in`: ``` use_sql_variables_in=[master|all] (default: all) ``` -Server-side session variables are handled similar to SQL variables. If "master" -is set, SQL variables are read and written in master only. Autocommit values and -prepared statements are routed to all nodes always. - **WARNING** If a SELECT query modifies a user variable when the `use_sql_variables_in` @@ -266,28 +249,10 @@ MySQL [(none)]> SELECT @id := @id + 1 FROM test.t1; ERROR 1064 (42000): Routing query to backend failed. See the error log for further details. ``` -Allow user variable modification in SELECT queries by setting the value of -`use_sql_variables_in` to `master`. This will route all queries that use user +Allow user variable modification in SELECT queries by setting +`use_sql_variables_in=master`. This will route all queries that use user variables to the master. -#### Examples of session command limitations - -In a situation where a new database `db` is created, immediately after which a client executes `USE db`, it is possible that the command is routed -to a slave before the `CREATE DATABASE` clause is replicated to all slaves. In this case a query may be executed in the wrong database. Similarly, if any response -that ReadWriteSplit sends back to the client differ from that of the master, -there is a risk for misbehavior. To prevent this, any failures in session -command execution are treated as fatal errors and all connections by the session -to that particular slave server will be closed. In addition, the server will not -used again for routing for the duration of the session. - -The most likely reasons are related to replication lag but it could be possible -that a slave fails to execute something because of some non-fatal, temporary -failure, while the execution of the same command succeeds in other backends. - -The preparation of a prepared statement is routed to all servers. The execution -of a prepared statement is routed to the first available server or to the server -pointed by a routing hint attached to the query. - ### Schemarouter limitations (schemarouter) The schemarouter currently has some limitations due to the nature of the From c0a3e6ba377793bf2174034e1d165c591a7c59e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 11 Apr 2018 09:44:53 +0300 Subject: [PATCH 04/16] MXS-1773: Add test case Added a test case that reproduces the problem and verifies that it is fixed. --- maxscale-system-test/CMakeLists.txt | 4 ++++ maxscale-system-test/mxs1773_failing_ldli.cpp | 22 +++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 maxscale-system-test/mxs1773_failing_ldli.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 607be92f1..61c97da2e 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -629,6 +629,10 @@ add_test_executable(mxs1743_rconn_bitmask.cpp mxs1743_rconn_bitmask mxs1743_rcon # https://jira.mariadb.org/browse/MXS-1751 add_test_executable(mxs1751_available_when_donor_crash.cpp mxs1751_available_when_donor_crash mxs1751_available_when_donor_crash LABELS GALERA_BACKEND) +# MXS-1773: Failing LOAD DATA LOCAL INFILE confuses readwritesplit +# https://jira.mariadb.org/browse/MXS-1773 +add_test_executable(mxs1773_failing_ldli.cpp mxs1773_failing_ldli 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/mxs1773_failing_ldli.cpp b/maxscale-system-test/mxs1773_failing_ldli.cpp new file mode 100644 index 000000000..f8e113608 --- /dev/null +++ b/maxscale-system-test/mxs1773_failing_ldli.cpp @@ -0,0 +1,22 @@ +/** + * MXS-1773: Failing LOAD DATA LOCAL INFILE confuses readwritesplit + * + * https://jira.mariadb.org/browse/MXS-1773 + */ +#include "testconnections.h" +#include + +int main(int argc, char** argv) +{ + TestConnections test(argc, argv); + + test.maxscales->connect(); + auto q = std::bind(execute_query, test.maxscales->conn_rwsplit[0], std::placeholders::_1); + q("LOAD DATA LOCAL INFILE '/tmp/this-file-does-not-exist.txt' INTO TABLE this_table_does_not_exist"); + q("SELECT 1"); + q("SELECT 2"); + q("SELECT 3"); + test.maxscales->disconnect(); + + return test.global_result; +} From 252475cdc5be590aa75e161e0aeb452853c02c32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 11 Apr 2018 15:13:20 +0300 Subject: [PATCH 05/16] MXS-1776: Add test case Added test case that reproduces the problem. --- maxscale-system-test/CMakeLists.txt | 4 + maxscale-system-test/mxs1776_ps_exec_hang.cpp | 193 ++++++++++++++++++ 2 files changed, 197 insertions(+) create mode 100644 maxscale-system-test/mxs1776_ps_exec_hang.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 61c97da2e..1d4626333 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -633,6 +633,10 @@ add_test_executable(mxs1751_available_when_donor_crash.cpp mxs1751_available_whe # https://jira.mariadb.org/browse/MXS-1773 add_test_executable(mxs1773_failing_ldli.cpp mxs1773_failing_ldli replication LABELS readwritesplit REPL_BACKEND) +# MXS-1776: recursive COM_STMT_EXECUTE execution +# https://jira.mariadb.org/browse/MXS-1776 +add_test_executable(mxs1776_ps_exec_hang.cpp mxs1776_ps_exec_hang 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/mxs1776_ps_exec_hang.cpp b/maxscale-system-test/mxs1776_ps_exec_hang.cpp new file mode 100644 index 000000000..b98c116ba --- /dev/null +++ b/maxscale-system-test/mxs1776_ps_exec_hang.cpp @@ -0,0 +1,193 @@ +#include "testconnections.h" +#include +#include +#include + +using namespace std; + +struct Bind +{ + Bind() + { + bind.buffer = buffer; + bind.buffer_type = MYSQL_TYPE_LONG; + bind.error = &err; + bind.is_null = &is_null; + bind.length = &length; + } + + MYSQL_BIND bind; + char err = 0; + char is_null = 0; + char is_unsigned = 0; + uint8_t buffer[1024]; + unsigned long length = 0; +}; + +struct TestCase +{ + std::string name; + std::function func; +}; + +bool run_test(TestConnections& test, TestCase test_case) +{ + test.maxscales->connect(); + + MYSQL_STMT* stmt = mysql_stmt_init(test.maxscales->conn_rwsplit[0]); + std::string query = "SELECT * FROM test.t1"; + unsigned long cursor_type = CURSOR_TYPE_READ_ONLY; + mysql_stmt_attr_set(stmt, STMT_ATTR_CURSOR_TYPE, &cursor_type); + + Bind bind; + + test.set_timeout(30); + + if (mysql_stmt_prepare(stmt, query.c_str(), query.size())) + { + test.assert(false, "Prepared statement failure: %s", mysql_stmt_error(stmt)); + } + + cout << test_case.name << endl; + test.assert(test_case.func(test.maxscales->conn_rwsplit[0], stmt, bind), "Test '%s' failed", + test_case.name.c_str()); + + mysql_stmt_close(stmt); + + test.assert(mysql_query(test.maxscales->conn_rwsplit[0], "SELECT 1") == 0, "Normal queries should work"); + + test.maxscales->disconnect(); +} + + +int main(int argc, char* argv[]) +{ + TestConnections test(argc, argv); + + test.maxscales->connect(); + + test.try_query(test.maxscales->conn_rwsplit[0], "CREATE OR REPLACE TABLE test.t1(id INT)"); + test.try_query(test.maxscales->conn_rwsplit[0], "BEGIN"); + + for (int i = 0; i < 100; i++) + { + execute_query(test.maxscales->conn_rwsplit[0], "INSERT INTO test.t1 VALUES (%d)", i); + } + + test.try_query(test.maxscales->conn_rwsplit[0], "COMMIT"); + test.maxscales->disconnect(); + + vector tests = + { + { + "Simple execute and fetch", + [](MYSQL * conn, MYSQL_STMT * stmt, Bind & bind) + { + bool rval = true; + + if (mysql_stmt_execute(stmt) || + mysql_stmt_bind_result(stmt, &bind.bind)) + { + rval = false; + } + + while (mysql_stmt_fetch(stmt) == 0) + { + ; + } + + return rval; + } + }, + { + "Multiple overlapping executions without fetch", + [](MYSQL * conn, MYSQL_STMT * stmt, Bind & bind) + { + bool rval = true; + + if (mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt)) + { + rval = false; + } + + return rval; + } + }, + { + "Multiple overlapping executions with fetch", + [](MYSQL * conn, MYSQL_STMT * stmt, Bind & bind) + { + bool rval = true; + + if (mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_bind_result(stmt, &bind.bind)) + { + rval = false; + } + + while (mysql_stmt_fetch(stmt) == 0) + { + ; + } + + return rval; + } + }, + { + "Execution of queries while fetching", + [](MYSQL * conn, MYSQL_STMT * stmt, Bind & bind) + { + bool rval = true; + + if (mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_bind_result(stmt, &bind.bind)) + { + rval = false; + } + + while (mysql_stmt_fetch(stmt) == 0) + { + mysql_query(conn, "SELECT 1"); + } + + return rval; + } + }, + { + "Multiple overlapping executions and a query", + [](MYSQL * conn, MYSQL_STMT * stmt, Bind & bind) + { + bool rval = true; + + if (mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_stmt_execute(stmt) || + mysql_query(conn, "SET @a = 1")) + { + rval = false; + } + + return rval; + } + } + }; + + for (auto a : tests) + { + run_test(test, a); + } + + return test.global_result; +} From 311adf817f8a5434c1605628b5ac80dbd9d67402 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 11 Apr 2018 15:16:22 +0300 Subject: [PATCH 06/16] MXS-1776: Handle recursive COM_STMT_EXECUTE commands Readwritesplit would not handle multiple overlapping COM_STMT_EXECUTE commands properly if they opened cursors. This was due to the fact that the result would not be marked as complete and COM_STMT_FETCH commands were executed as if they did not return results. The correct implementation is to consider a COM_STMT_EXECUTE that opens a cursor complete only when the first EOF packet is read (that is, when the resultset header is read). This allows subsequent COM_STMT_FETCH commands to be handled separately. The separate COM_STMT_FETCH handling must count the number of packets that are being fetched. This allows correct tracking of the state of a COM_STMT_FETCH by checking that the number of packets is correct or the second EOF/ERR packet is read. --- include/maxscale/modutil.h | 1 + server/core/modutil.cc | 15 +++++++++ .../routing/readwritesplit/readwritesplit.cc | 23 +++++++++++-- .../readwritesplit/rwsplit_route_stmt.cc | 3 +- .../routing/readwritesplit/rwsplitsession.cc | 32 ++++++++++++++++++- .../routing/readwritesplit/rwsplitsession.hh | 10 ++++++ 6 files changed, 79 insertions(+), 5 deletions(-) diff --git a/include/maxscale/modutil.h b/include/maxscale/modutil.h index 99c01f59e..ba5cd8425 100644 --- a/include/maxscale/modutil.h +++ b/include/maxscale/modutil.h @@ -45,6 +45,7 @@ int modutil_MySQL_query_len(GWBUF* buf, int* nbytes_missing); void modutil_reply_parse_error(DCB* backend_dcb, char* errstr, uint32_t flags); void modutil_reply_auth_error(DCB* backend_dcb, char* errstr, uint32_t flags); int modutil_count_statements(GWBUF* buffer); +int modutil_count_packets(GWBUF* buffer); GWBUF* modutil_create_query(const char* query); GWBUF* modutil_create_mysql_err_msg(int packet_number, int affected_rows, diff --git a/server/core/modutil.cc b/server/core/modutil.cc index 568fa99cf..9741cafec 100644 --- a/server/core/modutil.cc +++ b/server/core/modutil.cc @@ -1068,6 +1068,21 @@ int modutil_count_statements(GWBUF* buffer) return num; } +int modutil_count_packets(GWBUF* buffer) +{ + int packets = 0; + size_t offset = 0; + uint8_t len[3]; + + while (gwbuf_copy_data(buffer, offset, 3, len) == 3) + { + ++packets; + offset += gw_mysql_get_byte3(len) + MYSQL_HEADER_LEN; + } + + return packets; +} + /** * Initialize the PCRE2 patterns used when converting MySQL wildcards to PCRE syntax. */ diff --git a/server/modules/routing/readwritesplit/readwritesplit.cc b/server/modules/routing/readwritesplit/readwritesplit.cc index 0897eae45..392170189 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.cc +++ b/server/modules/routing/readwritesplit/readwritesplit.cc @@ -520,7 +520,20 @@ static inline bool have_next_packet(GWBUF* buffer) */ bool reply_is_complete(SRWBackend& backend, GWBUF *buffer) { - if (backend->get_reply_state() == REPLY_STATE_START && + if (backend->current_command() == MXS_COM_STMT_FETCH) + { + bool more = false; + modutil_state state = {backend->is_large_packet()}; + int n_eof = modutil_count_signal_packets(buffer, 0, &more, &state); + backend->set_large_packet(state.state); + + if (n_eof > 0 || backend->consume_fetched_rows(buffer)) + { + LOG_RS(backend, REPLY_STATE_DONE); + backend->set_reply_state(REPLY_STATE_DONE); + } + } + else if (backend->get_reply_state() == REPLY_STATE_START && (!mxs_mysql_is_result_set(buffer) || GWBUF_IS_COLLECTED_RESULT(buffer))) { if (GWBUF_IS_COLLECTED_RESULT(buffer) || @@ -575,6 +588,13 @@ bool reply_is_complete(SRWBackend& backend, GWBUF *buffer) /** Waiting for the EOF packet after the rows */ LOG_RS(backend, REPLY_STATE_RSET_ROWS); backend->set_reply_state(REPLY_STATE_RSET_ROWS); + + if (backend->cursor_is_open()) + { + MXS_INFO("Cursor successfully opened"); + LOG_RS(backend, REPLY_STATE_DONE); + backend->set_reply_state(REPLY_STATE_DONE); + } } else { @@ -927,7 +947,6 @@ static int routeQuery(MXS_ROUTER *instance, MXS_ROUTER_SESSION *router_session, { if (rses->query_queue == NULL && (rses->expected_responses == 0 || - mxs_mysql_get_command(querybuf) == MXS_COM_STMT_FETCH || rses->load_data_state == LOAD_DATA_ACTIVE || rses->large_query)) { diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc index a9d6890c9..f7d62484f 100644 --- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc +++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc @@ -1111,8 +1111,7 @@ static inline bool query_creates_reply(uint8_t cmd) { return cmd != MXS_COM_QUIT && cmd != MXS_COM_STMT_SEND_LONG_DATA && - cmd != MXS_COM_STMT_CLOSE && - cmd != MXS_COM_STMT_FETCH; // Fetch is done mid-result + cmd != MXS_COM_STMT_CLOSE; } static inline bool is_large_query(GWBUF* buf) diff --git a/server/modules/routing/readwritesplit/rwsplitsession.cc b/server/modules/routing/readwritesplit/rwsplitsession.cc index 88456f4c6..69db728fc 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.cc +++ b/server/modules/routing/readwritesplit/rwsplitsession.cc @@ -17,7 +17,9 @@ RWBackend::RWBackend(SERVER_REF* ref): mxs::Backend(ref), m_reply_state(REPLY_STATE_DONE), - m_large_packet(false) + m_large_packet(false), + m_open_cursor(false), + m_expected_rows(0) { } @@ -72,12 +74,40 @@ bool RWBackend::write(GWBUF* buffer, response_type type) /** Replace the client handle with the real PS handle */ uint8_t* ptr = GWBUF_DATA(buffer) + MYSQL_PS_ID_OFFSET; gw_mysql_set_byte4(ptr, it->second); + uint8_t buf[4]; + + if (cmd == MXS_COM_STMT_EXECUTE && + // Skip header and command byte for the 4 byte ID + gwbuf_copy_data(buffer, 5, 4, buf) == sizeof(buf) && + // A flag value of 0 is no cursor + gw_mysql_get_byte4(buf) != 0) + { + m_open_cursor = true; + } + else if (cmd == MXS_COM_STMT_FETCH) + { + ss_dassert(m_open_cursor); + // Number of rows to fetch is a 4 byte integer after the ID + gwbuf_copy_data(buffer, 5 + 4, 4, buf); + m_expected_rows = gw_mysql_get_byte4(buf); + } + else + { + m_open_cursor = false; + } } } return mxs::Backend::write(buffer); } +bool RWBackend::consume_fetched_rows(GWBUF* buffer) +{ + m_expected_rows -= modutil_count_packets(buffer); + ss_dassert(m_expected_rows >= 0); + return m_expected_rows == 0; +} + uint32_t get_internal_ps_id(RWSplitSession* rses, GWBUF* buffer) { uint32_t rval = 0; diff --git a/server/modules/routing/readwritesplit/rwsplitsession.hh b/server/modules/routing/readwritesplit/rwsplitsession.hh index 1de6fb696..46cbec554 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.hh +++ b/server/modules/routing/readwritesplit/rwsplitsession.hh @@ -59,6 +59,9 @@ public: bool execute_session_command(); bool write(GWBUF* buffer, response_type type = EXPECT_RESPONSE); + // For COM_STMT_FETCH processing + bool consume_fetched_rows(GWBUF* buffer); + inline void set_large_packet(bool value) { m_large_packet = value; @@ -74,6 +77,11 @@ public: return m_command; } + inline bool cursor_is_open() const + { + return m_open_cursor; + } + private: reply_state_t m_reply_state; BackendHandleMap m_ps_handles; /**< Internal ID to backend PS handle mapping */ @@ -81,6 +89,8 @@ private: *calculation for result sets when the result * contains very large rows */ uint8_t m_command; + bool m_open_cursor; /**< Whether we have an open cursor */ + uint32_t m_expected_rows; /**< Number of rows a COM_STMT_FETCH is retrieving */ }; typedef std::tr1::shared_ptr SRWBackend; From ad5458f0e71259c8ad5513014a3c0f2a630746ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 12 Apr 2018 09:43:38 +0300 Subject: [PATCH 07/16] MXS-1776: Initialize RWBackend::m_command The variable was not initialized which caused COM_CHANGE_USER to produce unexpected behavior. --- server/modules/routing/readwritesplit/rwsplitsession.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/server/modules/routing/readwritesplit/rwsplitsession.cc b/server/modules/routing/readwritesplit/rwsplitsession.cc index 69db728fc..4e14c4e9c 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.cc +++ b/server/modules/routing/readwritesplit/rwsplitsession.cc @@ -18,6 +18,7 @@ RWBackend::RWBackend(SERVER_REF* ref): mxs::Backend(ref), m_reply_state(REPLY_STATE_DONE), m_large_packet(false), + m_command(0), m_open_cursor(false), m_expected_rows(0) { From fab8477c05e334a1bada506d2667a1fe06fde2c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 12 Apr 2018 09:44:28 +0300 Subject: [PATCH 08/16] MXS-1776: Fix utility functions The COM_STMT_FETCH command will create a response. This was a readwritesplit-specific interpretation of the command and it was wrong. Also record the currently executed command event for session commands. --- server/modules/protocol/MySQL/mysql_common.cc | 3 +-- server/modules/routing/readwritesplit/readwritesplit.cc | 1 + server/modules/routing/readwritesplit/rwsplitsession.cc | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/server/modules/protocol/MySQL/mysql_common.cc b/server/modules/protocol/MySQL/mysql_common.cc index d0e9e4a15..f818ef4df 100644 --- a/server/modules/protocol/MySQL/mysql_common.cc +++ b/server/modules/protocol/MySQL/mysql_common.cc @@ -1657,8 +1657,7 @@ bool mxs_mysql_command_will_respond(uint8_t cmd) { return cmd != MXS_COM_STMT_SEND_LONG_DATA && cmd != MXS_COM_QUIT && - cmd != MXS_COM_STMT_CLOSE && - cmd != MXS_COM_STMT_FETCH; + cmd != MXS_COM_STMT_CLOSE; } typedef std::vector< std::pair > TargetList; diff --git a/server/modules/routing/readwritesplit/readwritesplit.cc b/server/modules/routing/readwritesplit/readwritesplit.cc index 392170189..8396a8e7f 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.cc +++ b/server/modules/routing/readwritesplit/readwritesplit.cc @@ -527,6 +527,7 @@ bool reply_is_complete(SRWBackend& backend, GWBUF *buffer) int n_eof = modutil_count_signal_packets(buffer, 0, &more, &state); backend->set_large_packet(state.state); + // If the server responded with an error, n_eof > 0 if (n_eof > 0 || backend->consume_fetched_rows(buffer)) { LOG_RS(backend, REPLY_STATE_DONE); diff --git a/server/modules/routing/readwritesplit/rwsplitsession.cc b/server/modules/routing/readwritesplit/rwsplitsession.cc index 4e14c4e9c..c33d04f2c 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.cc +++ b/server/modules/routing/readwritesplit/rwsplitsession.cc @@ -30,7 +30,8 @@ RWBackend::~RWBackend() bool RWBackend::execute_session_command() { - bool expect_response = mxs_mysql_command_will_respond(next_session_command()->get_command()); + m_command = next_session_command()->get_command(); + bool expect_response = mxs_mysql_command_will_respond(m_command); bool rval = mxs::Backend::execute_session_command(); if (rval && expect_response) From da03c7337330e83b2d2405c7ac0a3a6d2cd8a4ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 12 Apr 2018 10:25:10 +0300 Subject: [PATCH 09/16] MXS-1776: Fix COM_STMT_EXECUTE flag extraction The code extracted the statement id, not the flags. --- .../routing/readwritesplit/rwsplitsession.cc | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/server/modules/routing/readwritesplit/rwsplitsession.cc b/server/modules/routing/readwritesplit/rwsplitsession.cc index c33d04f2c..7f43eac11 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.cc +++ b/server/modules/routing/readwritesplit/rwsplitsession.cc @@ -76,21 +76,22 @@ bool RWBackend::write(GWBUF* buffer, response_type type) /** Replace the client handle with the real PS handle */ uint8_t* ptr = GWBUF_DATA(buffer) + MYSQL_PS_ID_OFFSET; gw_mysql_set_byte4(ptr, it->second); - uint8_t buf[4]; - if (cmd == MXS_COM_STMT_EXECUTE && - // Skip header and command byte for the 4 byte ID - gwbuf_copy_data(buffer, 5, 4, buf) == sizeof(buf) && - // A flag value of 0 is no cursor - gw_mysql_get_byte4(buf) != 0) + if (cmd == MXS_COM_STMT_EXECUTE) { - m_open_cursor = true; + // Extract the flag byte after the statement ID + uint8_t flags = 0; + gwbuf_copy_data(buffer, MYSQL_PS_ID_OFFSET + MYSQL_PS_ID_SIZE, 1, &flags); + + // Any non-zero flag value means that we have an open cursor + m_open_cursor = flags != 0; } else if (cmd == MXS_COM_STMT_FETCH) { ss_dassert(m_open_cursor); // Number of rows to fetch is a 4 byte integer after the ID - gwbuf_copy_data(buffer, 5 + 4, 4, buf); + uint8_t buf[4]; + gwbuf_copy_data(buffer, MYSQL_PS_ID_OFFSET + MYSQL_PS_ID_SIZE, 4, buf); m_expected_rows = gw_mysql_get_byte4(buf); } else From a663ea2e80b1c7da56757d5b88d87efe2cf17e62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 12 Apr 2018 10:29:26 +0300 Subject: [PATCH 10/16] Sync slaves in mxs1071_maxrows The first test could fail due to replication lag. --- maxscale-system-test/mxs1071_maxrows.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/maxscale-system-test/mxs1071_maxrows.cpp b/maxscale-system-test/mxs1071_maxrows.cpp index deec523e0..7aaa84dc7 100644 --- a/maxscale-system-test/mxs1071_maxrows.cpp +++ b/maxscale-system-test/mxs1071_maxrows.cpp @@ -297,8 +297,7 @@ int main(int argc, char *argv[]) create_t1(Test->maxscales->conn_rwsplit[0]); insert_into_t1(Test->maxscales->conn_rwsplit[0], 1); Test->stop_timeout(); - sleep(5); - + Test->repl->sync_slaves(); Test->tprintf("**** Test 1 ****\n"); @@ -316,7 +315,8 @@ int main(int argc, char *argv[]) create_t1(Test->maxscales->conn_rwsplit[0]); insert_into_t1(Test->maxscales->conn_rwsplit[0], 3); Test->stop_timeout(); - sleep(5); + Test->repl->sync_slaves(); + Test->tprintf("**** Test 2 ****\n"); exp_rows[0] = 0; From 1a293c0093bf1da68e1274ccd7dc717f8ca58264 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 12 Apr 2018 19:36:34 +0300 Subject: [PATCH 11/16] MXS-1785: Don't assume empty packet is for LDLI When a LOAD DATA LOCAL INFILE finishes, the client sends an empty packet. The second case when the client sends an empty packet when the previous packet was exactly 0xffffff bytes long. These two packets were confused which caused the internal state to temporarily flip from inactive to ending and back to inactive. The aforementioned flip-flopping didn't have any practical differences but it was caught by a debug assertion. --- server/modules/routing/readwritesplit/rwsplit_route_stmt.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc index f7d62484f..cca0e5663 100644 --- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc +++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc @@ -168,7 +168,7 @@ route_target_t get_target_type(RWSplitSession *rses, GWBUF *buffer, route_target = get_route_target(rses, *command, *type, buffer->hint); } } - else + else if (rses->load_data_state == LOAD_DATA_ACTIVE) { /** Empty packet signals end of LOAD DATA LOCAL INFILE, send it to master*/ rses->load_data_state = LOAD_DATA_END; From 802b16f7094d5cd64a22a5cd748c1f711d4b64d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 12 Apr 2018 20:00:00 +0300 Subject: [PATCH 12/16] MXS-1786: Fix hang on COM_STATISTICS The commands needs to be handled separately from the rest of the result types. Added a test case that reproduces the problem and verifies that the change in code fixes it. --- maxscale-system-test/CMakeLists.txt | 4 ++++ maxscale-system-test/mxs1786_statistics.cpp | 24 +++++++++++++++++++ .../routing/readwritesplit/readwritesplit.cc | 6 +++++ 3 files changed, 34 insertions(+) create mode 100644 maxscale-system-test/mxs1786_statistics.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 1d4626333..6d7915feb 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -637,6 +637,10 @@ add_test_executable(mxs1773_failing_ldli.cpp mxs1773_failing_ldli replication LA # https://jira.mariadb.org/browse/MXS-1776 add_test_executable(mxs1776_ps_exec_hang.cpp mxs1776_ps_exec_hang replication LABELS readwritesplit REPL_BACKEND) +# MXS-1786: Hang with COM_STATISTICS +# https://jira.mariadb.org/browse/MXS-1786 +add_test_executable(mxs1786_statistics.cpp mxs1786_statistics 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/mxs1786_statistics.cpp b/maxscale-system-test/mxs1786_statistics.cpp new file mode 100644 index 000000000..0ed4c49b8 --- /dev/null +++ b/maxscale-system-test/mxs1786_statistics.cpp @@ -0,0 +1,24 @@ +/** + * MXS-1786: Hang with COM_STATISTICS + */ + +#include "testconnections.h" + +int main(int argc, char* argv[]) +{ + TestConnections test(argc, argv); + + test.maxscales->connect(); + + for (int i = 0; i < 10; i++) + { + test.set_timeout(10); + mysql_stat(test.maxscales->conn_rwsplit[0]); + test.try_query(test.maxscales->conn_rwsplit[0], "SELECT 1"); + } + + test.maxscales->disconnect(); + + + return test.global_result; +} diff --git a/server/modules/routing/readwritesplit/readwritesplit.cc b/server/modules/routing/readwritesplit/readwritesplit.cc index 8396a8e7f..0eb70abcc 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.cc +++ b/server/modules/routing/readwritesplit/readwritesplit.cc @@ -534,6 +534,12 @@ bool reply_is_complete(SRWBackend& backend, GWBUF *buffer) backend->set_reply_state(REPLY_STATE_DONE); } } + else if (backend->current_command() == MXS_COM_STATISTICS) + { + // COM_STATISTICS returns a single string and thus requires special handling + LOG_RS(backend, REPLY_STATE_DONE); + backend->set_reply_state(REPLY_STATE_DONE); + } else if (backend->get_reply_state() == REPLY_STATE_START && (!mxs_mysql_is_result_set(buffer) || GWBUF_IS_COLLECTED_RESULT(buffer))) { From b060e3a28995e83513781adb4ebbbef8ab927acd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 12 Apr 2018 20:36:46 +0300 Subject: [PATCH 13/16] MXS-1787: Add test case Added a test case that reproduces the problem. --- maxscale-system-test/CMakeLists.txt | 4 ++ maxscale-system-test/mxs1787_call_ps.cpp | 62 ++++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 maxscale-system-test/mxs1787_call_ps.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 6d7915feb..442a255fc 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -641,6 +641,10 @@ add_test_executable(mxs1776_ps_exec_hang.cpp mxs1776_ps_exec_hang replication LA # https://jira.mariadb.org/browse/MXS-1786 add_test_executable(mxs1786_statistics.cpp mxs1786_statistics replication LABELS readwritesplit REPL_BACKEND) +# MXS-1787: Crash with PS: CALL p1((SELECT f1()), ?) +# https://jira.mariadb.org/browse/MXS-1787 +add_test_executable(mxs1787_call_ps.cpp mxs1787_call_ps 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/mxs1787_call_ps.cpp b/maxscale-system-test/mxs1787_call_ps.cpp new file mode 100644 index 000000000..364026874 --- /dev/null +++ b/maxscale-system-test/mxs1787_call_ps.cpp @@ -0,0 +1,62 @@ +/** + * MXS-1787: Crash with PS: CALL p1((SELECT f1()), ?) + */ + +#include "testconnections.h" + +using namespace std; + +struct Bind +{ + Bind() + { + bind.buffer = &data; + bind.buffer_type = MYSQL_TYPE_LONG; + bind.error = &err; + bind.is_null = &is_null; + bind.length = &length; + } + + MYSQL_BIND bind; + char err = 0; + char is_null = 0; + char is_unsigned = 0; + uint32_t data = 1234; + unsigned long length = sizeof(data); +}; + +int main(int argc, char* argv[]) +{ + TestConnections test(argc, argv); + + test.maxscales->connect(); + + execute_query(test.maxscales->conn_rwsplit[0], "USE test"); + execute_query(test.maxscales->conn_rwsplit[0], "CREATE OR REPLACE TABLE t1 AS SELECT 1 AS id"); + execute_query(test.maxscales->conn_rwsplit[0], "CREATE OR REPLACE FUNCTION f1() RETURNS INT DETERMINISTIC BEGIN RETURN 1; END"); + execute_query(test.maxscales->conn_rwsplit[0], "CREATE OR REPLACE PROCEDURE p1(IN i INT, IN j INT) BEGIN SELECT i + j; END"); + + test.maxscales->disconnect(); + + test.maxscales->connect(); + + MYSQL_STMT* stmt = mysql_stmt_init(test.maxscales->conn_rwsplit[0]); + std::string query = "CALL p1((SELECT f1()), ?)"; + Bind bind; + + test.set_timeout(30); + + test.assert(mysql_stmt_prepare(stmt, query.c_str(), query.size()) == 0, + "Prepared statement failure: %s", mysql_stmt_error(stmt)); + test.assert(mysql_stmt_bind_param(stmt, &bind.bind) == 0, + "Bind failure: %s", mysql_stmt_error(stmt)); + test.assert(mysql_stmt_execute(stmt) == 0, + "Execute failure: %s", mysql_stmt_error(stmt)); + + mysql_stmt_close(stmt); + + test.assert(mysql_query(test.maxscales->conn_rwsplit[0], "SELECT 1") == 0, "Normal queries should work"); + test.maxscales->disconnect(); + + return test.global_result; +} From 3d09c836c5dec5de3864d04e0dcfdf32679112a4 Mon Sep 17 00:00:00 2001 From: dapeng huang <7xerocha@gmail.com> Date: Fri, 13 Apr 2018 17:58:02 +0800 Subject: [PATCH 14/16] init inst->sessions for maxinfo (#173) * init inst->sessions for maxinfo * misc fix --- server/modules/routing/maxinfo/maxinfo.c | 1 + 1 file changed, 1 insertion(+) diff --git a/server/modules/routing/maxinfo/maxinfo.c b/server/modules/routing/maxinfo/maxinfo.c index bd1ac2bbe..4792ab6dd 100644 --- a/server/modules/routing/maxinfo/maxinfo.c +++ b/server/modules/routing/maxinfo/maxinfo.c @@ -154,6 +154,7 @@ createInstance(SERVICE *service, char **options) return NULL; } + inst->sessions = NULL; inst->service = service; spinlock_init(&inst->lock); From 94af85b9484cb25afb96a8ff45b6ce6b8d266f9a Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 13 Apr 2018 10:56:05 +0300 Subject: [PATCH 15/16] MXS-1787 Add test that exposes problem --- query_classifier/test/maxscale.test | 3 +++ 1 file changed, 3 insertions(+) diff --git a/query_classifier/test/maxscale.test b/query_classifier/test/maxscale.test index a0d037df2..63fed3f4c 100644 --- a/query_classifier/test/maxscale.test +++ b/query_classifier/test/maxscale.test @@ -100,3 +100,6 @@ SELECT '2005-01-01' - INTERVAL 1 SECOND; # MXS-1730 SELECT id as engine FROM users WHERE id = 1; + +# MXS-1787 +CALL p1((SELECT f1()), ?); From 3d8d2beaaaa33e808ea4f417e3ed4ff644714cfd Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Fri, 13 Apr 2018 11:24:52 +0300 Subject: [PATCH 16/16] MXS-1787 Provide alias map when parsing an expression list A statement like "CALL p1((SELECT f1()), ?);" needs an collection for storing aliases when being parsed. --- query_classifier/qc_sqlite/qc_sqlite.cc | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/query_classifier/qc_sqlite/qc_sqlite.cc b/query_classifier/qc_sqlite/qc_sqlite.cc index 713586cdc..af49d29d8 100644 --- a/query_classifier/qc_sqlite/qc_sqlite.cc +++ b/query_classifier/qc_sqlite/qc_sqlite.cc @@ -1074,7 +1074,8 @@ public: if (pExpr->flags & EP_xIsSelect) { - update_field_infos_from_subselect(pAliases, pExpr->x.pSelect, pExclude); + ss_dassert(pAliases); + update_field_infos_from_subselect(*pAliases, pExpr->x.pSelect, pExclude); if (zName) @@ -1299,7 +1300,7 @@ public: while (pPrior) { - update_field_infos_from_subselect(&aliases, pPrior, pExclude, + update_field_infos_from_subselect(aliases, pPrior, pExclude, IGNORE_COMPOUND_SELECTS); pPrior = pPrior->pPrior; } @@ -1307,12 +1308,12 @@ public: } } - void update_field_infos_from_subselect(QcAliases* pAliases, + void update_field_infos_from_subselect(const QcAliases& existing_aliases, const Select* pSelect, const ExprList* pExclude, compound_approach_t compound_approach = ANALYZE_COMPOUND_SELECTS) { - QcAliases aliases(*pAliases); + QcAliases aliases(existing_aliases); update_field_infos_from_select(aliases, pSelect, pExclude, compound_approach); } @@ -1325,7 +1326,8 @@ public: if (pCte->pSelect) { - update_field_infos_from_subselect(pAliases, pCte->pSelect, NULL); + ss_dassert(pAliases); + update_field_infos_from_subselect(*pAliases, pCte->pSelect, NULL); } } } @@ -2064,7 +2066,8 @@ public: if (pExprList) { - update_field_infos_from_exprlist(NULL, pExprList, NULL); + QcAliases aliases; + update_field_infos_from_exprlist(&aliases, pExprList, NULL); } exposed_sqlite3SrcListDelete(pParse->db, pName);