MXS-1603: get session transation state from backend via session track mechanism #158

ybbct · 2018-01-14T04:27:37Z

parse session track info( autocommit and transaction state) from ok packet in mysql_backend, and save it to gwbuf property;
when reply to client in mysql_client, mapping all the information provided by session track info to mxs_session_trx_state_t
add config parameter to enable/disable this behavior

ybbct · 2018-01-14T04:28:47Z

cmake/BuildMariaDBConnector.cmake

@@ -9,7 +9,7 @@ set(MARIADB_CONNECTOR_C_REPO "https://github.com/MariaDB/mariadb-connector-c.git
  CACHE STRING "MariaDB Connector-C Git repository")

 # Release 2.3.3 (preliminary) of the Connector-C
-set(MARIADB_CONNECTOR_C_TAG "v3.0.2"


https://jira.mariadb.org/browse/MDEV-14647 this bug will lead monitor crash so current need master version

The bug is fixed but not yet released. Once the next Connector-C 3.0 version is released, this should be updated.

I suggest that this change is reverted and only updated once we know the tag.

markus456 · 2018-01-15T08:10:12Z

cmake/BuildMariaDBConnector.cmake

@@ -9,7 +9,7 @@ set(MARIADB_CONNECTOR_C_REPO "https://github.com/MariaDB/mariadb-connector-c.git
  CACHE STRING "MariaDB Connector-C Git repository")

 # Release 2.3.3 (preliminary) of the Connector-C
-set(MARIADB_CONNECTOR_C_TAG "v3.0.2"


The bug is fixed but not yet released. Once the next Connector-C 3.0 version is released, this should be updated.

I suggest that this change is reverted and only updated once we know the tag.

markus456 · 2018-01-15T08:38:36Z

server/core/config.cc

@@ -233,6 +234,7 @@ const char *config_listener_params[] =
    CN_SSL_VERSION,
    CN_SSL_CERT_VERIFY_DEPTH,
    CN_SSL_VERIFY_PEER_CERTIFICATE,
+    CN_SESSION_TRACK_TRX_STATE,


This should be set as a service parameter. Then the DCB and Listener would not have to be modified.

The listeners are a lower level object that are intended to represent a network socket. Services define higher level attributes so the transaction tracking should be put there.

markus456 · 2018-01-15T08:40:37Z

server/modules/protocol/MySQL/mariadbclient/mysql_client.cc

@@ -411,6 +411,11 @@ int MySQLSendHandshake(DCB* dcb)
 */
 int gw_MySQLWrite_client(DCB *dcb, GWBUF *queue)
 {
+    MySQLProtocol *protocol = DCB_PROTOCOL(dcb, MySQLProtocol);
+    if (GWBUF_IS_REPLY_OK(queue) && protocol->session_track_trx_state) 


The DCB has a service pointer which can be used to access the service parameters. Storing the session_track_trx_state in the service would make this easier.

markus456 · 2018-01-15T08:40:57Z

server/modules/protocol/MySQL/mariadbclient/mysql_client.cc

@@ -1279,6 +1285,9 @@ int gw_MySQLAccept(DCB *listener)
        while ((client_dcb = dcb_accept(listener)) != NULL)
        {
            gw_process_one_new_client(client_dcb);
+            cli_proto = DCB_PROTOCOL(client_dcb, MySQLProtocol);
+            listener_proto = (SERV_LISTENER *)listener->listener;
+            cli_proto->session_track_trx_state = listener_proto->session_track_trx_state;


This can be removed if the parameter is stored in the service.

markus456 · 2018-01-15T08:43:54Z

server/modules/protocol/MySQL/mysql_common.cc

-
-    // get capabilities part 2 (2 bytes)
-    memcpy(&capab_ptr[2], &mysql_server_capabilities_two, 2);
+    conn->server_capabilities = mysql_server_capabilities_one | mysql_server_capabilities_two << 16;


Although this is better than the original, we try to avoid doing unrelated changes when they are not required. Please remove this change and submit it in a separate commit and pull request.

server_capabilities is needed in function mxs_mysql_get_session_track_info to determine whether the server support extended ok packet;

Ah, this was my mistake. I didn't realize that the capab_ptr was not used for anything. This looks good now.

markus456 · 2018-01-15T08:56:08Z

server/modules/protocol/MySQL/mysql_common.cc

+
+    uint8_t* ptr = GWBUF_DATA(buff);
+
+    ptr += (MYSQL_COM_OFFSET + 1); // Header and Command type


Use the MYSQL_HEADER_LEN macro from <maxscale/protocol/mysql.h>, this is what is used elsewhere.

markus456 · 2018-01-15T08:57:07Z

server/modules/protocol/MySQL/mysql_common.cc

+
+    if (server_capabilities & GW_MYSQL_CAPABILITIES_SESSION_TRACK)
+    {
+        if (ptr < buff->end) {


MaxScale code style places the opening curly brace on the next line.

markus456 · 2018-01-15T08:59:32Z

server/modules/protocol/MySQL/mysql_common.cc

+mysql_tx_state_t parse_trx_state(char *str)
+{
+    int s = TX_EMPTY;
+    assert(str);


Use ss_dassert from <maxscale/debug.h>

markus456 · 2018-01-15T09:00:12Z

server/modules/protocol/MySQL/mysql_common.cc

+    } while(*(str++) != 0);
+
+    return (mysql_tx_state_t)s;
+}


Add newline at end of file.

markus456 · 2018-01-15T09:00:56Z

server/modules/protocol/MySQL/mysql_common.cc

+    {
+        switch (*str)
+        {
+            case 'T':


Are these documented somewhere? If so, add a comment and a link to the documentation or document the meaning of these here.

Tell Beyonce come get me

jhnwkmn · 2018-01-15T09:15:56Z

server/modules/protocol/MySQL/mariadbclient/mysql_client.cc

+        {
+            session_set_trx_state(ses, SESSION_TRX_INACTIVE);
+        }
+        else if(s & TX_READ_TRX)


Space between if and (.

jhnwkmn · 2018-01-15T09:17:41Z

server/modules/protocol/MySQL/mariadbclient/mysql_client.cc

+        {
+            session_set_trx_state(ses, SESSION_TRX_READ_WRITE);
+        }
+        else if((s & TX_EXPLICIT) |  (s & TX_IMPLICIT))


As it's a logical comparison || would be more appropriate.

jhnwkmn · 2018-01-15T09:23:03Z

server/modules/protocol/MySQL/mysql_common.cc

+    }
+}
+
+mysql_tx_state_t parse_trx_state(char *str)


const char*

…service, fix code style, ...

ybbct · 2018-01-16T05:51:59Z

I have manually tested following cases, get the result from debug log (server/modules/protocol/MySQL/mariadbclient/mysql_client.cc:1953)

case1:

STMT	Result
BEGIN	SESSION_TRX_ACTIVE
COMMIT	SESSION_TRX_INACTIVE
BEGIN WORK	SESSION_TRX_ACTIVE
COMMIT WORK	SESSION_TRX_INACTIVE

case2:

STMT	Result
BEGIN	SESSION_TRX_ACTIVE
ROLLBACK	SESSION_TRX_INACTIVE
BEGIN WORK	SESSION_TRX_ACTIVE
ROLLBACK WORK	SESSION_TRX_INACTIVE

case3:

STMT	Result
START TRANSACTION	SESSION_TRX_ACTIVE
COMMIT	SESSION_TRX_INACTIVE
START TRANSACTION	SESSION_TRX_ACTIVE
START TRANSACTION READ ONLY	SESSION_TRX_READ_ONLY
START TRANSACTION READ WRITE	SESSION_TRX_READ_WRITE
START TRANSACTION WITH CONSISTENT SNAPSHOT	SESSION_TRX_ACTIVE
COMMIT	SESSION_TRX_INACTIVE

case4:

STMT	Value of AUTOCOMMIT
SET AUTOCOMMIT=true	ON
SET @@autocommit=0	OFF
SET GLOBAL AUTOCOMMIT=1	OFF
SET SESSION AUTOCOMMIT=0	OFF
SET @@SESSION . AUTOCOMMIT=1	ON
SET @@global . AUTOCOMMIT=0	ON

case5:

STMT	TRANSACTION STATE	Value of AUTOCOMMIT
SET @@autocommit=0	SESSION_TRX_INACTIVE	OFF
SELECT 1	NA	NA
INSERT INTO t1 VALUES(1)	SESSION_TRX_ACTIVE	OFF
COMMIT	SESSION_TRX_INACTIVE	OFF

##config
[maxscale]
threads=24
log_info=0
log_debug=1

[MySQL Monitor]
type=monitor
module=mysqlmon
servers=server1,server2
user=###user###
passwd=###password##
monitor_interval=1000

[RwSplitRouter]
type=service
router=readwritesplit
servers=server1,server2
user=###user###
passwd=###password##
session_track_trx_state=1

[RwSplit Listener]
type=listener
address=###ip0###
service=RwSplitRouter
protocol=MySQLClient
port=###port1###

[server1]
type=server
address=###ip1###
port=###port2###
protocol=MySQLBackend
serv_weight=1

[server2]
type=server
address=###ip2###
port=###port2###
protocol=MySQLBackend
serv_weight=3

##my.cnf

add these two

session_track_state_change = on
session_track_transaction_info = CHARACTERISTICS

… wrong state mapping

jhnwkmn · 2018-01-16T06:25:18Z

server/modules/protocol/MySQL/mysql_common.cc

@@ -1798,13 +1798,13 @@ void mxs_mysql_get_session_track_info(GWBUF *buff, uint32_t server_capabilities)

    if (server_capabilities & GW_MYSQL_CAPABILITIES_SESSION_TRACK)
    {
-        if (ptr < buff->end) 
+        if (ptr < (local_buf+len)) 


Space on both sides of binary operator.
local_buff + len

markus456 · 2018-01-16T07:04:41Z

I think that scanning the debug log would be an OK solution for a test case implementation. It's somewhat hard to unit test code that is inside the protocol modules.

markus456 · 2018-01-16T07:07:30Z

server/modules/protocol/MySQL/mysql_common.cc

+ * As described in https://dev.mysql.com/worklog/task/?id=6631
+ * When session transation state changed
+ * SESSION_TRACK_TRANSACTION_TYPE (or SESSION_TRACK_TRANSACTION_STATE in MySQL) will
+ * return an 8 bytes string to indicate the transaction state details


I believe it would be OK to refer to that document and add the following excerpt from it:

The following states may be signaled: Place 1: Transaction. T explicitly started transaction ongoing I implicitly started transaction (@autocommit=0) ongoing _ no active transaction Place 2: unsafe read r one/several non-transactional tables were read in the context of the current transaction _ no non-transactional tables were read within the current transaction so far Place 3: transactional read R one/several transactional tables were read _ no transactional tables were read yet Place 4: unsafe write w one/several non-transactional tables were written _ no non-transactional tables were written yet Place 5: transactional write W one/several transactional tables were written to _ no transactional tables were written to yet Place 6: unsafe statements s one/several unsafe statements (such as UUID()) were used. _ no such statements were used yet. Place 7: result-set S a result set was sent to the client _ statement had no result-set Place 8: LOCKed TABLES L tables were explicitly locked using LOCK TABLES _ LOCK TABLES is not active in this session

markus456 · 2018-01-19T06:50:26Z

As is required by our contribution policy, please indicate that you submit this pull request under BSD-new by posting a comment with the following text:

I am contributing the new code of the whole pull request, including one or several files that are either new files or modified ones, under the BSD-new license.

ybbct · 2018-01-19T12:22:27Z

I am contributing the new code of the whole pull request, including one or several files that are either new files or modified ones, under the BSD-new license.

markus456 · 2018-01-19T18:24:56Z

Executing the following SQL through readwritesplit seems to hang with this patch applied:

create table t1(id int);
insert into t1 select seq from seq_0_to_10000;
select '' from t1;

The log contains the following warning:

2018-01-19 20:12:30   warning: (13) recieved unexpecting session track type:256

ybbct · 2018-01-20T06:12:17Z

Hi markus, could you give more detail about the problem, I cannot recurrent it with mysql 5.7 and mariadb 10.2

markus456 · 2018-01-20T09:58:42Z

Seems that a non-OK packet interpreted as an OK packet which causes a debug assertion to be hit. This was spotted when we ran our test suite on the patched code but right now I can't think of a consistent way to reproduce it.

Another problem is that the server can send multiple packets at once and the code only checks whether the first packet is an OK packet (this happens when MULTI_RESULTS capability is in use). This is why I suggested moving the call to mxs_mysql_get_session_track_info from mysql_backend.c:774 inside the modutil_get_complete_packets function.

ybbct · 2018-01-21T10:25:56Z

THX, I know where is wrong now, I taken the output of modutil_get_complete_packets as a single packet, so in mxs_mysql_get_session_track_info the packets after ok packet be treated as ok packet's session track info;

because need server_capablitities to decode ok packet, cannot move mxs_mysql_get_session_track_info into modutil_get_complete_packets, I have tried get server_capablitities from this way "((MysqlProtocol *)buff->server->protocol)->server_capabilities, but it not works, so I keep mxs_mysql_get_session_track_info behind mxs_mysql_get_session_track_info;

ybbct · 2018-01-21T10:32:03Z

imitate test_modutil.c write a ut for session_track;

ybbct · 2018-01-21T10:33:49Z

server/modules/protocol/MySQL/mysql_common.cc

+                    enum_session_state_type type = 
+                        (enum enum_session_state_type)mxs_leint_consume(&ptr);
+#if defined(SS_DEBUG)
+                    ss_dassert(type <= SESSION_TRACK_TRANSACTION_TYPE);


markus456 · 2018-01-21T16:56:15Z

server/modules/protocol/MySQL/mysql_common.cc

+    ptr += 2; // status      
+    ptr += 2; // number of warnings
+
+    if (server_capabilities & GW_MYSQL_CAPABILITIES_SESSION_TRACK)


This should be moved before the while loop in mxs_mysql_get_session_track_info so that if the server doesn't support the capability, the code won't be executed.

markus456 · 2018-01-21T16:56:44Z

server/modules/protocol/MySQL/mysql_common.cc

+    size_t offset = 0;
+    uint8_t header_and_command[MYSQL_HEADER_LEN+1];
+
+    while(gwbuf_copy_data(buff, offset, MYSQL_HEADER_LEN+1, header_and_command) == (MYSQL_HEADER_LEN+1))


Space between while and (. The MaxScale style guide can be found here.

markus456 · 2018-01-21T17:04:02Z

server/modules/protocol/MySQL/mysql_common.cc

+    {
+        size_t packet_len = gw_mysql_get_byte3(header_and_command) + MYSQL_HEADER_LEN;
+        uint8_t cmd = header_and_command[MYSQL_COM_OFFSET];
+        if (packet_len > MYSQL_OK_PACKET_MIN_LEN && cmd == MYSQL_REPLY_OK)


The following set of SQL statements should cause a false positive to be triggered.

CREATE TABLE t1(a VARCHAR(20), b INT, c INT, d INT); INSERT INTO t1 VALUES ('', 100, 200, 300); SELECT * FROM t1;

The result set row will be wrongly interpreted due to the fact that the MariaDB protocol is stateful. To work around this, the number of EOF packets could be tracked inside the protocol object stored in the DCB. To see whether a result set is in progress, we can just check if num_eof_packets % 2 == 0.

jhnwkmn · 2018-01-22T12:44:31Z

server/core/test/test_session_track.cc

+    proto.num_eof_packets = 0;
+    ss_dfprintf(stderr,"test_session_track : Functional tests.\n");
+    //BEGIN
+    buffer = gwbuf_alloc_and_load(PACKET_1_LEN, resultset1+PACKET_1_IDX);


Space around binary operators.

...resultset1 + PACKET_1_IDX...

jhnwkmn · 2018-01-22T12:53:29Z

server/modules/protocol/MySQL/mysql_common.cc

+    uint8_t header_and_command[MYSQL_HEADER_LEN+1];
+    if (proto->server_capabilities & GW_MYSQL_CAPABILITIES_SESSION_TRACK)
+    {
+        while (gwbuf_copy_data(buff, offset, MYSQL_HEADER_LEN+1, header_and_command) == (MYSQL_HEADER_LEN+1))


Space around binary operators. Please fix all places.

ekorh475 · 2018-01-22T13:25:12Z

include/maxscale/protocol/mysql.h

@@ -339,6 +353,7 @@ typedef struct
    int                    ignore_replies;               /*< How many replies should be discarded */
    GWBUF*                 stored_query;                 /*< Temporarily stored queries */
    bool                   collect_result;               /*< Collect the next result set as one buffer */
+    uint32_t               num_eof_packets;              /*< Signal number to indicate is current packet is ok packet*/


The comment is a bit unclear. What is "signal" here?

ybbct · 2018-01-22T14:44:39Z

Thanks for all your patience, currently the develop branch build failed on 'qlafilter.cc:1169:64: error: cannot pass objects of non-trivially-copyable type', so this pr also failed; I tested for my local branch, passed all test;

markus456 · 2018-01-22T15:05:54Z

Ah, that would be caused by something we did. I think the pull request is OK and we'll begin merging it into the develop branch (we have to fix the branch first).

We would like to thank you for contributing to the MaxScale project, it's really nice to see that external contributions work well.

markus456 · 2018-01-24T15:01:13Z

@ybbct I've merged the pull request into develop. I also realized that there's no documentation for the feature. If you could create a new pull request that adds a short description of what the parameter does into the configuration guide, we would really appreciate it.

get session transation state from backend via session track mechanism

e1aeac8

ybbct commented Jan 14, 2018

View reviewed changes

markus456 requested changes Jan 15, 2018

View reviewed changes

jhnwkmn reviewed Jan 15, 2018

View reviewed changes

server/modules/protocol/MySQL/mysql_common.cc Outdated

}

}

mysql_tx_state_t parse_trx_state(char *str)

Copy link

Contributor

jhnwkmn Jan 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const char*

ybbct added 2 commits January 15, 2018 20:25

refactor, check every packet before parser ok packet, move config to …

d234b13

…service, fix code style, ...

remove noneed protocol and the comparison corrent

6d3c60e

add session tracker:SESSION_TRACK_TRANSACTION_CHARACTERISTICS and fix…

a52e1ac

… wrong state mapping

jhnwkmn reviewed Jan 16, 2018

View reviewed changes

fix code style

f1aa1d7

markus456 approved these changes Jan 16, 2018

View reviewed changes

markus456 reviewed Jan 16, 2018

View reviewed changes

ybbct added 2 commits January 16, 2018 15:14

add doc, more detail about trx state

f7e08fd

update mariadb connector version for MDEV-14647

0363b85

Merge branch 'develop' into MXS-1603

7682967

fix parse session track info out of packet boundary

8ce7683

ybbct commented Jan 21, 2018

View reviewed changes

markus456 reviewed Jan 21, 2018

View reviewed changes

ybbct added 2 commits January 22, 2018 14:52

check protocol state to determine real ok packet

81f3035

support deprecated eof and fix eof count

f41f3a1

revert deprecate eof

1729d73

jhnwkmn reviewed Jan 22, 2018

View reviewed changes

ekorh475 reviewed Jan 22, 2018

View reviewed changes

misc fix and remove all tail spaces

73e5fd5

markus456 merged commit 73e5fd5 into mariadb-corporation:develop Jan 24, 2018

ybbct deleted the MXS-1603 branch January 26, 2018 01:59


		uint8_t* ptr = GWBUF_DATA(buff);

		ptr += (MYSQL_COM_OFFSET + 1); // Header and Command type

MXS-1603: get session transation state from backend via session track mechanism #158

MXS-1603: get session transation state from backend via session track mechanism #158

Conversation

ybbct commented Jan 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ybbct commented Jan 16, 2018 • edited

add these two

Choose a reason for hiding this comment

markus456 commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus456 commented Jan 19, 2018

ybbct commented Jan 19, 2018

markus456 commented Jan 19, 2018

ybbct commented Jan 20, 2018

markus456 commented Jan 20, 2018

ybbct commented Jan 21, 2018 • edited

ybbct commented Jan 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhnwkmn Jan 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ybbct commented Jan 22, 2018

markus456 commented Jan 22, 2018 • edited

markus456 commented Jan 24, 2018

ybbct commented Jan 16, 2018 •

edited

ybbct commented Jan 21, 2018 •

edited

jhnwkmn Jan 22, 2018 •

edited

markus456 commented Jan 22, 2018 •

edited