Skip to content

sessionctx: Error/warning on unsupported isolation levels#8625

Merged
ngaut merged 8 commits intopingcap:masterfrom
morgo:isolation
Dec 10, 2018
Merged

sessionctx: Error/warning on unsupported isolation levels#8625
ngaut merged 8 commits intopingcap:masterfrom
morgo:isolation

Conversation

@morgo
Copy link
Copy Markdown
Contributor

@morgo morgo commented Dec 8, 2018 •
edited
Loading

What problem does this PR solve?

Fixes #2712

What is changed and how it works?

For isolation levels, the following behavior is provided:

  • read-uncommitted: error (unsupported)
  • read-committed: clean
  • repeatable-read: clean
  • serializable: error (unsupported)

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Code changes

  • minimal

Side effects

  • Breaking backward compatibility (there may be some apps which depend on read-uncommitted or serializable, but they are less common than RR and RC)

Related changes

  • Need to update the documentation
  • Need to be included in the release note

This change is Reviewable

@morgo morgo mentioned this pull request Dec 8, 2018
Closed
@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Dec 8, 2018

/run-all-tests

winkyao
winkyao reviewed Dec 9, 2018
View reviewed changes
Comment thread sessionctx/variable/varsutil.go Outdated
@zhouqiang-cl
Copy link
Copy Markdown
Contributor

zhouqiang-cl commented Dec 9, 2018

/rebuild

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Dec 9, 2018

I have changed read-uncommitted to be an error now, and updated the description. read-committed also uses a new error class of NotRecommended.

@zhouqiang-cl
Copy link
Copy Markdown
Contributor

zhouqiang-cl commented Dec 9, 2018

/run-all-tests

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Dec 9, 2018

It looks like this does break the JDBC test:

Loading JDBC driver 'com.mysql.jdbc.Driver'

Connected to 5.7.10-TiDB-v2.1.0-rc.3-299-g0dc57f7

[ERROR] Tests run: 30, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 4.957 s <<< FAILURE! - in pingcap.tidb.test.simple.ConnectionTest

[ERROR] testIsolationLevel(pingcap.tidb.test.simple.ConnectionTest)  Time elapsed: 0.01 s  <<< ERROR!

java.sql.SQLException: variable 'tx_isolation' does not yet support value: READ-UNCOMMITTED

	at pingcap.tidb.test.simple.ConnectionTest.testIsolationLevel(ConnectionTest.java:553)

This is the side-effect mentioned in the PR. Some applications will break on warnings too, particularly since read-committed is frequently used.

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Dec 9, 2018

/run-all-tests

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Dec 9, 2018

I was concerned that a warning for read-committed may break too many applications, so I've changed it to be clean again. The description has been updated.

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Dec 9, 2018

/run-all-tests tidb-test=pr/683

@disksing
Copy link
Copy Markdown
Contributor

disksing commented Dec 10, 2018

LGTM.

@zz-jason zz-jason added status/LGT1 Indicates that a PR has LGTM 1. component/server labels Dec 10, 2018
winkyao
winkyao approved these changes Dec 10, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao winkyao added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 10, 2018
@ngaut
Copy link
Copy Markdown
Member

ngaut commented Dec 10, 2018

/run-all-tests

@ngaut ngaut merged commit c27705a into pingcap:master Dec 10, 2018
@morgo morgo deleted the isolation branch December 10, 2018 13:06
iamzhoug37 pushed a commit to iamzhoug37/tidb that referenced this pull request Dec 13, 2018
@morgo @iamzhoug37
sessionctx: Error/warning on unsupported isolation levels (pingcap#8625)
397c803
@morgo morgo mentioned this pull request Jan 22, 2019
Closed
jackysp added a commit that referenced this pull request Apr 3, 2019
@jackysp
Revert "sessionctx: Error/warning on unsupported isolation levels (#8625
3155040 
)"

This reverts commit c27705a.
@jackysp jackysp mentioned this pull request Apr 3, 2019
Closed
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@winkyao winkyao winkyao approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component/server contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

What transaction isolation is supported?

7 participants

@morgo @zhouqiang-cl @disksing @ngaut @winkyao @zz-jason @sre-bot
⚡