Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix max_transaction_time #4135

Merged
merged 8 commits into from
Mar 2, 2023
23 changes: 15 additions & 8 deletions lib/MySQL_Session.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4691,12 +4691,6 @@ int MySQL_Session::handler() {
//unsigned int j;
//unsigned char c;

if (active_transactions == 0) {
active_transactions=NumActiveTransactions();
if (active_transactions > 0) {
transaction_started_at = thread->curtime;
}
}
// FIXME: Sessions without frontend are an ugly hack
if (session_fast_forward==false) {
if (client_myds==NULL) {
Expand Down Expand Up @@ -4968,6 +4962,14 @@ int MySQL_Session::handler() {
gtid_hid = -1;
if (rc==0) {

if (active_transactions != 0) { // run this only if currently we think there is a transaction
if ((myconn->mysql->server_status & SERVER_STATUS_IN_TRANS) == 0) { // there is no transaction on the backend connection
active_transactions = NumActiveTransactions(); // we check all the hostgroups/backends
if (active_transactions == 0)
transaction_started_at = 0; // reset it
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@renecannao what do you think about putting this code inside a function and calling it here and also when rc==-1. This way, ProxySQL also reset the timer when it has an error in the middle of a transaction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you want to reset the timer if there is an error in the middle of a transaction?

I am going back to what I wrote before: [...] at the beginning of the query

handler_rc0_Process_GTID(myconn);

// if we are locked on hostgroup, the value of autocommit is copied from the backend connection
Expand Down Expand Up @@ -5075,6 +5077,12 @@ int MySQL_Session::handler() {
handler_minus1_HandleBackendConnection(myds, myconn);
}
} else {
if (active_transactions == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not setting this at the beginning of a query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the beginning of a query. When rc==0 is the end of the query and when rc==-1 the query do not begin because it has failed.

What I am realizing now is that if rc==-1 because proxysql failed to retrieve the result of a query, then the number of active transaction is not going to be updated and the transaction_started_at is not going to be reset.

active_transactions=NumActiveTransactions();
if (active_transactions > 0) {
transaction_started_at = thread->curtime;
}
}
switch (rc) {
// rc==1 , query is still running
// start sending to frontend if mysql_thread___threshold_resultset_size is reached
Expand Down Expand Up @@ -7206,8 +7214,7 @@ void MySQL_Session::MySQL_Result_to_MySQL_wire(MYSQL *mysql, MySQL_ResultSet *My
int myerrno=mysql_errno(mysql);
if (myerrno==0) {
unsigned int num_rows = mysql_affected_rows(mysql);
unsigned int nTrx=NumActiveTransactions();
uint16_t setStatus = (nTrx ? SERVER_STATUS_IN_TRANS : 0 );
uint16_t setStatus = (active_transactions ? SERVER_STATUS_IN_TRANS : 0);
if (autocommit) setStatus |= SERVER_STATUS_AUTOCOMMIT;
if (mysql->server_status & SERVER_MORE_RESULTS_EXIST)
setStatus |= SERVER_MORE_RESULTS_EXIST;
Expand Down
12 changes: 0 additions & 12 deletions lib/MySQL_Thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3746,18 +3746,6 @@ void MySQL_Thread::ProcessAllSessions_CompletedMirrorSession(unsigned int& n, My
// this function was inline in MySQL_Thread::process_all_sessions()
void MySQL_Thread::ProcessAllSessions_MaintenanceLoop(MySQL_Session *sess, unsigned long long sess_time, unsigned int& total_active_transactions_) {
unsigned int numTrx=0;
sess->active_transactions=sess->NumActiveTransactions();
{
sess->active_transactions=sess->NumActiveTransactions();
// in case we detected a new transaction just now
if (sess->active_transactions == 0) {
sess->transaction_started_at = 0;
} else {
if (sess->transaction_started_at == 0) {
sess->transaction_started_at = curtime;
}
}
}
total_active_transactions_ += sess->active_transactions;
sess->to_process=1;
if ( (sess_time/1000 > (unsigned long long)mysql_thread___max_transaction_idle_time) || (sess_time/1000 > (unsigned long long)mysql_thread___wait_timeout) ) {
Expand Down
67 changes: 67 additions & 0 deletions test/tap/tests/test_max_transaction_time-t.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* @file test_max_transaction_time-t.cpp
* @brief This test verifies that 'max_transaction_time' behaves properly.
*
* @details It verifies that connection with many transactions does not get
* killed by the max_transaction_time implementation if each individual
* transaction takes shorter than max_transaction_time.
*/

#include "mysql.h"

#include "proxysql_utils.h"
#include "tap.h"
#include "utils.h"

using std::string;

int main(int, char**) {
CommandLine cl;

if (cl.getEnv()) {
diag("Failed to get the required environmental variables.");
return EXIT_FAILURE;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

plan() is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I thought it was not needed when not using ok()

MYSQL* admin = mysql_init(NULL);
if (!mysql_real_connect(admin, cl.host, cl.admin_username, cl.admin_password, NULL, cl.admin_port, NULL, 0)) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(admin));
return EXIT_FAILURE;
}

diag("Configure the target server (non-existing) server to test connection failures");
javsanpar marked this conversation as resolved.
Show resolved Hide resolved
MYSQL_QUERY_T(
admin,
"UPDATE global_variables SET variable_value = 10000 "
"WHERE variable_name = 'mysql-max_transaction_time'"
);
MYSQL_QUERY_T(admin, "LOAD MYSQL VARIABLES TO RUNTIME");

mysql_close(admin);

MYSQL* proxy = mysql_init(NULL);
if (!mysql_real_connect(proxy, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxy));
return EXIT_FAILURE;
}

diag("Run fifteen 1-second transactions:");
MYSQL_RES* myres;
for (int i = 0; i < 15; i++) {
MYSQL_QUERY_T(proxy, "BEGIN");

MYSQL_QUERY_T(proxy, "SELECT SLEEP(1)");
myres = mysql_store_result(proxy);
if (myres == nullptr) {
fprintf(stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__, mysql_error(proxy));
return EXIT_FAILURE;
}
mysql_free_result(myres);

MYSQL_QUERY_T(proxy, "COMMIT");
}

mysql_close(proxy);

return exit_status();
}