-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Fix commit after statement fail in non-autocommit transaction #24960
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
base: master
Are you sure you want to change the base?
Conversation
0b40406 to
1d0cf14
Compare
ZacBlanco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. I have a question about the tests
| assertQueryFails(txnSession, "create table test_table(a int, b varchar)", "Current transaction is aborted, commands ignored until end of transaction block"); | ||
|
|
||
| // `commit` statement fail, but abort the transaction successfully | ||
| assertQueryFails(txnSession, "commit", "Current transaction has already been aborted"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we still call rollback here before or after this query? Can we add that to verify what the comment above states?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this statement, the transaction is completely aborted. So, if we calling ROLLBACK again, the behavior should be the same as when no transaction was started at all, and an error message will be reported as follows:
Query xxx failed: No transaction in progress
In CLI console, it indeed behavior as above. However, currently in the testing framework, since the flag clearTransactionId cannot be returned to the client's session, executing rollback again will report the following error message:
Expected exception message 'Unknown transaction ID: cddea4f9-4dd9-437f-bab5-0cc17c76dee5. Possibly expired? ...
Now I have added all these scenarios to the test case. I thinks this failure message can demonstrate that the transaction has been aborted by the server.
| assertQueryFails(txnSession, "commit", "Current transaction has already been aborted"); | ||
| }); | ||
|
|
||
| assertQuery("select count(*) from test_non_autocommit_table_v2", "values(0)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so I understand what this PR fixes: Before this change, would this query have failed due to the previous transaction failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, before this change, the query would always fail after a transaction is corrupted by previous failure. However, the test framework can't accurately mimic the real world behavior, so in the test case transaction(...).execute(...) itself will finally fail because of the corrupted transaction if we didn't abort it properly.
I have refactored the test case and added some comments to clarify the expected behaviors of subsequent statements. Please take a look when available, thanks.
1d0cf14 to
f57324d
Compare
Description
Following PR #23247 , this PR handle the session corruption cause by a failed statement in non-autocommit transaction as well.
When executing
commitin above scenario, it should fail while aborting the whole corrupted transaction successfully. Thus we can keep running following statements within the same session.Motivation and Context
Fix: #23246
Test Plan
AbstractTestDistributedQueriesto simulate the scenarios described in Session is completely corrupted by the failed statement in a non-autocommit transaction #23246Contributor checklist
Release Notes