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
Merged

Fix max_transaction_time #4135

merged 8 commits into from
Mar 2, 2023

Conversation

javsanpar
Copy link
Contributor

Fixes #3957

lib/MySQL_Session.cpp Outdated Show resolved Hide resolved
@@ -5075,6 +5078,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.

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

javsanpar and others added 2 commits February 27, 2023 17:49
This commit also introduces 2 output for PROXYSQL INTERNAL SESSION:
- active_transactions
- transaction_started_at

Enhanced the TAP tests to also verify the above metrics
transaction_started_at = 0; // reset it
}
}

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

@@ -4968,6 +4968,9 @@ int MySQL_Session::handler() {
if (active_transactions == 0)
transaction_started_at = 0; // reset it
}
} else {
transaction_started_at = thread->curtime;
Copy link
Contributor

Choose a reason for hiding this comment

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

This code means that when a query completes, and active_transactions==0 , transaction_started_at is set to current time even if there is no transaction.

test/tap/tests/test_max_transaction_time-t.cpp Outdated Show resolved Hide resolved
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()

@renecannao renecannao merged commit 7ef4cc0 into v2.x Mar 2, 2023
@matus-chordify
Copy link

Great, thank you @javsanpar and @renecannao! 🎉

@renecannao renecannao deleted the v2.x-3957 branch August 29, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connections killed by incorrect implementation of max_transaction_time
3 participants