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

What transaction isolation is supported? #2712

Closed
vadimtk opened this issue Feb 22, 2017 · 14 comments
Closed

What transaction isolation is supported? #2712

vadimtk opened this issue Feb 22, 2017 · 14 comments
Assignees

Comments

@vadimtk
Copy link

@vadimtk vadimtk commented Feb 22, 2017

As TiDB declares strong consistency , I expect it should support SERIALIZABLE isolation level.

When I run test from "Hermitage" test suite
https://github.com/vadimtk/hermitage/blob/master/mysql-innodb.md

both Read Skew and Write Skew test fails in SERIALIZABLE level.

The example of queries, see:
MySQL "serializable" prevents Write Skew (G2-item)

@shenli
Copy link
Member

@shenli shenli commented Feb 23, 2017

@vadimtk Thanks for your feedback!
TiDB support serializable isolation level. But its behavior is a little different from MySQL. TiDB does not has global lock when you update rows. We use row-level optimistic lock to resolve write conflict. When two session updating the same row, one of them will success and the other one will meet write conflict and retry the whole transaction automatically.

In your case, you can use "select for ... update" to prevent write skew. There is one thing you should notice. We also use optimistic lock to detect conflict at the commit stage. Unlike update/delete/insert, SelectForUpdate statement will not be retried automatically. The client will receive an error with message "can not retry select for update statement" and it should decide retry the statement or not.

@vadimtk
Copy link
Author

@vadimtk vadimtk commented Feb 23, 2017

Hi,

"select for .. update" helps in some cases, but now I am trying serializable and Anti-Dependency Cycles (G2):
https://github.com/ept/hermitage/blob/master/mysql.md#anti-dependency-cycles-g2

In TiDB Anti-Dependency Cycles (G2) is not prevented in serializable level.

@vadimtk
Copy link
Author

@vadimtk vadimtk commented Feb 23, 2017

more on why G2 must be prevented you can find there cockroachdb/cockroach#10030

@disksing
Copy link
Member

@disksing disksing commented Feb 23, 2017

Hi @vadimtk ,

Strictly speaking, the isolation level we are providing should be Snapshot Isolation, you may also refer to the FAQ. In some cases, users can use SELECT ... FOR UPDATE to explicitly specify the rows that need to be locked to avoid write skew.

If we want to support Serializable Isolation, we have to introduce table lock or range lock, which will affect the system's concurrent performance.

In the future, we might provide configurable different isolation levels, allowing users to balance isolation level and performance according to their own situation.

@vadimtk
Copy link
Author

@vadimtk vadimtk commented Feb 23, 2017

Then if you do not provide serializable, I recommend you should return an error on the following statement

set session transaction isolation level serializable;

@ngaut
Copy link
Member

@ngaut ngaut commented Feb 23, 2017

Good idea. Thank you @vadimtk
Would you like to send a PR to return an error?

@vadimtk
Copy link
Author

@vadimtk vadimtk commented Feb 23, 2017

I am not familiar with your source code base, so I can't make a PR

@coocood coocood added this to the rc3 milestone Feb 27, 2017
@coocood coocood added the easy label Feb 27, 2017
@breeswish
Copy link
Member

@breeswish breeswish commented Jul 7, 2017

I think it would be better to print warning messages in console about ignoring the isolation level instead of fail this operation so that common applications won't break due to this change.

@tshqin
Copy link
Member

@tshqin tshqin commented Aug 21, 2017

Based on my tests and understanding, TiDB provides 2 isolation levels, the default isolation level is SI, can anyone help on below questions?

  1. SelectForUpdate statement is not designed for providing DML abilities after select, the only difference between SI's default behavior and SelectForUpdate behavior is that SelectForUpdate provides an error msg in some cases. pls point it out if I'm wrong.
  2. how to change the isolation level to Read Committed, I tried below statement, got an "Query OK", but it has no effect.
    "set session transaction isolation level read committed;"

TiDB doesn't support the other 3 isolation level which supports by MySQL - read uncommitted, repeatable read, and serializable, but by using "set session transaction isolation level" statement you always get a "Query OK" msg, this is quite misleading, compared to warning msg, I suppose an error msg will be more actual.

@zimulala zimulala removed the easy label Aug 23, 2017
@ngaut ngaut removed this from the 1.0 GA milestone Oct 28, 2017
@ngaut ngaut added this to the 1.1 milestone Oct 28, 2017
@ngaut ngaut removed this from the 2.0 milestone Apr 9, 2018
@kocoten1992
Copy link

@kocoten1992 kocoten1992 commented Sep 6, 2018

sometime retry can't be an option, like a transaction would last 4 hours (long background jobs, transaction just for keeping sure it done when it done), wouldn't expect a failed trans if I've select for update {skip locked,no wait} from the start.

@morgo
Copy link
Member

@morgo morgo commented Dec 7, 2018

I have the following proposal to help fix this issue:

  • Provide a warning when trying to set the isolation level to read-uncommitted or read-committed.
  • Provide an error when trying to set the isolation level to serializable.

The rationale being that providing a stronger level of consistency is less likely to produce issues and that some common ORMs do explicitly set read-committed. Producing an error could break compatibility unnecessarily.

(I will assign the issue to myself. I am happy to provide the patch.)

@morgo morgo self-assigned this Dec 7, 2018
@shenli
Copy link
Member

@shenli shenli commented Dec 7, 2018

@morgo I agree with you.

@morgo
Copy link
Member

@morgo morgo commented Dec 8, 2018

I have a patch in #8625 which addresses this problem as described by my earlier comment. But having thought about the issue more, I have two suggested enhancements I would like feedback on:

  1. Also provide an error for read-uncommitted.
  2. Add a new error class for read-committed, which is to produce a "not recommended" warning.

@ngaut ngaut closed this in #8625 Dec 10, 2018
@morgo
Copy link
Member

@morgo morgo commented Dec 10, 2018

Just a quick note, the final behavior is:

  • Error on read uncommitted, serializable.
  • No error or warning on repeatable read, read-committed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

10 participants