[MXS-2583] Make optimistic_trx=true work for SELECTs after set autocommit=0 Created: 2019-07-01  Updated: 2019-10-18  Resolved: 2019-10-16

Status: Closed
Project: MariaDB MaxScale
Component/s: readwritesplit
Affects Version/s: 2.3
Fix Version/s: 2.5.0

Type: Bug Priority: Major
Reporter: Valerii Kravchuk Assignee: markus makela
Resolution: Fixed Votes: 0
Labels: None

Epic Link: Router Improvements
Sprint: MXS-SPRINT-92

 Description   

It seems optimistic_trx works only for transactions explicitly started with BEGIN ... or START TRANSACTION.

It would be great to add read scaling support for sets of SELECTs executed after set autocommit=0;



 Comments   
Comment by markus makela [ 2019-07-01 ]

Took a quick look at the code and evaluated what needs to be done for this to work.

The information about implicit transactions must be retained so that the next statement after a SET autocommit=0 or COMMIT is treated as the first statement of a new transaction. Since the first statement after the implicit transaction starts can be a write, the transaction should only be attempted on a slave if this statements is a read-only one.

Here's a crude patch that seems to work based on the few minutes of testing I did:

diff --git a/include/maxscale/queryclassifier.hh b/include/maxscale/queryclassifier.hh
index 78ea4fed6..48af49eb8 100644
--- a/include/maxscale/queryclassifier.hh
+++ b/include/maxscale/queryclassifier.hh
@@ -245,6 +245,17 @@ public:
         return m_ps_continuation;
     }
 
+    /**
+     * Whether the statement alone is read-only
+     *
+     * This disregards the transaction state and other factors that might affect the target type
+     * selection done by the query classifier.
+     */
+    bool stmt_is_read_only() const
+    {
+        return query_type_is_read_only(m_route_info.type_mask());
+    }
+
     /**
      * @brief Store and process a prepared statement
      *
diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc
index 4a62c693e..ffc5382c9 100644
--- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc
+++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc
@@ -199,6 +199,11 @@ bool RWSplitSession::route_single_stmt(GWBUF* querybuf)
     uint8_t command = info.command();
     uint32_t qtype = info.type_mask();
     route_target_t route_target = info.target();
+    bool was_implicit_trx = m_implicit_trx;
+
+    m_implicit_trx = !session_is_autocommit(m_client->session)
+        && (qc_query_is_type(qtype, QUERY_TYPE_DISABLE_AUTOCOMMIT)
+            || qc_query_is_type(qtype, QUERY_TYPE_COMMIT));
 
     SRWBackend target;
 
@@ -210,9 +215,10 @@ bool RWSplitSession::route_single_stmt(GWBUF* querybuf)
     {
         update_trx_statistics();
 
-        if (m_qc.is_trx_starting()                          // A transaction is starting
-            && !session_trx_is_read_only(m_client->session) // Not explicitly read-only
-            && should_try_trx_on_slave(route_target))       // Qualifies for speculative routing
+        if ((m_qc.is_trx_starting()                             // A transaction is starting
+             || (was_implicit_trx && m_qc.stmt_is_read_only())) // Implicit trx start with autocommit=0
+            && !session_trx_is_read_only(m_client->session)     // Not explicitly read-only
+            && should_try_trx_on_slave(route_target))           // Qualifies for speculative routing
         {
             // Speculatively start routing the transaction to a slave
             m_otrx_state = OTRX_STARTING;
diff --git a/server/modules/routing/readwritesplit/rwsplitsession.hh b/server/modules/routing/readwritesplit/rwsplitsession.hh
index 9da56dcf2..5630279e6 100644
--- a/server/modules/routing/readwritesplit/rwsplitsession.hh
+++ b/server/modules/routing/readwritesplit/rwsplitsession.hh
@@ -173,6 +173,8 @@ public:
                                      * This avoids the lookup involved in getting the worker-local value from
                                      * the worker's container.*/
 
+    bool m_implicit_trx = false;
+
 private:
     RWSplitSession(RWSplit* instance,
                    MXS_SESSION* session,

I think it would be better to store the implicit transaction start inside the query classifier which means minor adjustments are still needed.

Generated at Thu Feb 08 04:15:11 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.