-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
Moving transaction initialization before query analysis #1013
Conversation
…n to before invoking analyzer.
…sactions if any errors occur while executing a query.
…r automatic transaction management (not for explicitly created transactions).
…ql-server into fulghum/dolt-3402
// don't need to do anything here to clear implicit transaction state. | ||
// | ||
// TODO: This logic would probably read more clearly if we could just ask the session/ctx if the | ||
// current transaction is automatically created or explicitly created by the caller. |
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.
I considered adding something to Session or Transaction's interface to show whether the current transaction is automatically created by the system (i.e. autocommit) or is explicitly requested by the client, but wanted to keep this fix as small as possible. I think the code that needs to use GetIgnoreAutoCommit would read a little clearer with that refactor, so happy to follow up with that if you think it's worth it.
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.
LGTM!
Moves transaction creation ahead of query analysis, so that queries can be executed with the latest committed state from other transactions.
Fixes: dolthub/dolt#3402