store transaction isolation level in upper case #12099
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Harshit Gangal <[email protected]>
harshit-gangal
requested review from
systay,
frouioui,
GuptaManan100 and
arthurschreiber
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
If a new flag is being introduced:
If a workflow is added or modified:
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
vitess-bot
bot
added
NeedsDescriptionUpdate
harshit-gangal
removed
NeedsDescriptionUpdate
systay
reviewed
Jan 16, 2023
| utils.AssertContains(t, conn, "select @@transaction_isolation, connection_id()", `REPEATABLE-READ`) | ||
|
|
||
| // setting to different value. | ||
| utils.Exec(t, conn, "set @@transaction_isolation = 'read-committed'") | ||
| utils.AssertMatches(t, conn, "select @@transaction_isolation", `[[VARCHAR("read-committed")]]`) | ||
| utils.Exec(t, conn, "set @@transaction_isolation = 'READ-COMMITTED'") |
There was a problem hiding this comment.
we want to store them with upper case and return them with uppercase, but we want to allow for both cases for input, right?
There was a problem hiding this comment.
currently, we do not change the case when set <system_variable> is used and store it as-is.
This is only for the case when set transaction isolation level <value> is provided we have the option as we convert the statement to set @@session.transaction_isolation = <value>
Do you suggest that I make that change as well, to store the user-provided value in upper case for set <system_varable>?
There was a problem hiding this comment.
Do we use the stored value for printing things, not the constants defined in sql.y?
There was a problem hiding this comment.
Changed as discussed offline.
systay
reviewed
Jan 16, 2023
GuptaManan100
approved these changes
Jan 16, 2023
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
systay
reviewed
Jan 17, 2023
| type StorageCase int | ||
|
|
||
| const ( | ||
| SCSame StorageCase = iota |
There was a problem hiding this comment.
nitpick: Couldn't we skip the SC and just use sysvars.AlwaysUpper,sysvars.AlwaysLower and sysvars.FromUser? really not important, if you want to merge, go ahead :)
systay
approved these changes
Jan 17, 2023
dbussink
pushed a commit
that referenced
this pull request
Jan 30, 2023
* add support for transaction isolation level and make it vitess aware setting (#11673) * feat: add support for tx_isolation and make it vitess aware setting Signed-off-by: Harshit Gangal <[email protected]> * feat: rewrite tx_isolation and transaction_isolation in select query Signed-off-by: Harshit Gangal <[email protected]> * feat: added support for set session transaction isolation level <value> Signed-off-by: Harshit Gangal <[email protected]> * feat: add nextTxScope for transaction variables Signed-off-by: Harshit Gangal <[email protected]> * refactor: use constants over magic literals Signed-off-by: Harshit Gangal <[email protected]> * kept old support for next transaction scope transaction system variables Signed-off-by: Harshit Gangal <[email protected]> * test: added e2e test Signed-off-by: Harshit Gangal <[email protected]> Signed-off-by: Harshit Gangal <[email protected]> * Fix: return allowed transaction isolation level value on select query (#11804) * test: added e2e test Signed-off-by: Harshit Gangal <[email protected]> * test: added unit test Signed-off-by: Harshit Gangal <[email protected]> * fix: transaction_isolation to return connection default when not set Signed-off-by: Harshit Gangal <[email protected]> * test: addressed review comments Signed-off-by: Harshit Gangal <[email protected]> Signed-off-by: Harshit Gangal <[email protected]> * feat: support transaction isolation through reserved connection (#11987) Signed-off-by: Harshit Gangal <[email protected]> Signed-off-by: Harshit Gangal <[email protected]> * store transaction isolation level in upper case (#12099) * store transaction isolation level in upper case Signed-off-by: Harshit Gangal <[email protected]> * store system variables in the case as provided in StorageCase type Signed-off-by: Harshit Gangal <[email protected]> Signed-off-by: Harshit Gangal <[email protected]> Signed-off-by: Harshit Gangal <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a follow-up PR from #11987 for comment #11987 (comment). The only possible solution was to store it as upper case as how MySQL returns the value for the
transaction_isolationsystem variable.Related Issue(s)
Checklist
Deployment Notes