Merge pull request #4306 from sysown/v2.x-4264

Fix `COMMIT|ROLLBACK` forwarding to backend connections - Closes #4264

include/MySQL_Session.h

@@ -127,6 +127,16 @@ class MySQL_Session
 
 	void return_proxysql_internal(PtrSize_t *);
 	bool handler_special_queries(PtrSize_t *);
+	/**
+	 * @brief Handles 'COMMIT|ROLLBACK' commands.
+	 * @details Forwarding the packet is required when there are active transactions. Since we are limited to
+	 *  forwarding just one 'COMMIT|ROLLBACK', we work under the assumption that we only have one active
+	 *  transaction. If more transactions are simultaneously open for the session, more 'COMMIT|ROLLBACK'.
+	 *  commands are required to be issued by the client, so they could be forwarded to the corresponding
+	 *  backend connections.
+	 * @param The received packet to be handled.
+	 * @return 'true' if the packet is intercepted and never forwarded to the client, 'false' otherwise.
+	 */
 	bool handler_CommitRollback(PtrSize_t *);
 	bool handler_SetAutocommit(PtrSize_t *);
 	/**
@@ -329,6 +339,18 @@ class MySQL_Session
 	unsigned int NumActiveTransactions(bool check_savpoint=false);
 	bool HasOfflineBackends();
 	bool SetEventInOfflineBackends();
+	/**
+	 * @brief Finds one active transaction in the current backend connections.
+	 * @details Since only one connection is returned, if the session holds multiple backend connections with
+	 *  potential transactions, the priority is:
+	 *   1. Connections flagged with 'SERVER_STATUS_IN_TRANS', or 'autocommit=0' in combination with
+	 *      'autocommit_false_is_transaction'.
+	 *   2. Connections with 'autocommit=0' holding a 'SAVEPOINT'.
+	 *   3. Connections with 'unknown transaction status', e.g: connections with errors.
+	 * @param check_savepoint Used to also check for connections holding savepoints. See MySQL bug
+	 *  https://bugs.mysql.com/bug.php?id=107875.
+	 * @returns The hostgroup in which the connection was found, -1 in case no connection is found.
+	 */
 	int FindOneActiveTransaction(bool check_savepoint=false);
 	unsigned long long IdleTime();
 

include/mysql_connection.h

@@ -209,16 +209,21 @@ class MySQL_Connection {
 	void process_rows_in_ASYNC_STMT_EXECUTE_STORE_RESULT_CONT(unsigned long long& processed_bytes);
 
 	void async_free_result();
-	bool IsActiveTransaction(); /* {
-		bool ret=false;
-			if (mysql) {
-				ret = (mysql->server_status & SERVER_STATUS_IN_TRANS);
-				if (ret == false && (mysql)->net.last_errno) {
-					ret = true;
-				}
-			}
-		return ret;
-	} */
+	/**
+	 * @brief Returns if the connection is **for sure**, known to be in an active transaction.
+	 * @details The function considers two things:
+	 *   1. If 'server_status' is flagged with 'SERVER_STATUS_IN_TRANS'.
+	 *   2. If the connection has 'autcommit=0' and 'autocommit_false_is_transaction' is set.
+	 * @return True if the connection is known to be in a transaction, or equivalent state.
+	 */
+	bool IsKnownActiveTransaction();
+	/**
+	 * @brief Returns if the connection is in a **potential transaction**.
+	 * @details This function is a more strict version of 'IsKnownActiveTransaction', which also considers
+	 *  connections which holds 'unknown_transaction_status' as potentially active transactions.
+	 * @return True if the connection is in potentially in an active transaction.
+	 */
+	bool IsActiveTransaction();
 	bool IsServerOffline();
 	bool IsAutoCommit();
 	bool AutocommitFalse_AndSavepoint();

lib/MySQL_Session.cpp

@@ -917,9 +917,16 @@ bool MySQL_Session::handler_CommitRollback(PtrSize_t *pkt) {
 	// in this part of the code (as at release 2.4.3) where we call
 	// NumActiveTransactions() with the check_savepoint flag .
 	// This to try to handle MySQL bug https://bugs.mysql.com/bug.php?id=107875
-	unsigned int nTrx=NumActiveTransactions(true);
-	if (nTrx) {
+	//
+	// Since we are limited to forwarding just one 'COMMIT|ROLLBACK', we work under the assumption that we
+	// only have one active transaction. Under this premise, we should execute this command under that
+	// specific connection, for that, we update 'current_hostgroup' with the first active transaction we are
+	// able to find. If more transactions are simultaneously open for the session, more 'COMMIT|ROLLBACK'
+	// commands are required to be issued by the client to continue ending transactions.
+	unsigned int hg = FindOneActiveTransaction(true);
+	if (hg != -1) {
 		// there is an active transaction, we must forward the request
+		current_hostgroup = hg;
 		return false;
 	} else {
 		// there is no active transaction, we will just reply OK
@@ -7497,8 +7504,10 @@ int MySQL_Session::FindOneActiveTransaction(bool check_savepoint) {
 		_mybe=(MySQL_Backend *)mybes->index(i);
 		if (_mybe->server_myds) {
 			if (_mybe->server_myds->myconn) {
-				if (_mybe->server_myds->myconn->IsActiveTransaction()) {
+				if (_mybe->server_myds->myconn->IsKnownActiveTransaction()) {
 					return (int)_mybe->server_myds->myconn->parent->myhgc->hid;
+				} else if (_mybe->server_myds->myconn->IsActiveTransaction()) {
+					ret = (int)_mybe->server_myds->myconn->parent->myhgc->hid;
 				} else {
 					// we use check_savepoint to check if we shouldn't ignore COMMIT or ROLLBACK due
 					// to MySQL bug https://bugs.mysql.com/bug.php?id=107875 related to

lib/mysql_connection.cpp

@@ -2385,6 +2385,16 @@ bool MySQL_Connection::AutocommitFalse_AndSavepoint() {
 	return ret;
 }
 
+bool MySQL_Connection::IsKnownActiveTransaction() {
+	bool in_trx = mysql ? mysql->server_status & SERVER_STATUS_IN_TRANS : false;
+
+	if (in_trx == false) {
+		in_trx = mysql_thread___autocommit_false_is_transaction && (IsAutoCommit() == false);
+	}
+
+	return in_trx;
+}
+
 bool MySQL_Connection::IsActiveTransaction() {
 	bool ret=false;
 	if (mysql) {