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.

Sign up for GitHub

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

Jump to bottom

*: Remove SNAPSHOT isolation #26475

Closed
bdarnell opened this issue Jun 6, 2018 · 0 comments · Fixed by #27040
Closed

*: Remove SNAPSHOT isolation #26475

bdarnell opened this issue Jun 6, 2018 · 0 comments · Fixed by #27040
Assignees
@bdarnell
Labels
A-kv-transactions Relating to MVCC and the transactional model.
Milestone
2.1

Comments

@bdarnell
Copy link
Member

bdarnell commented Jun 6, 2018

Our implementation of SNAPSHOT isolation is buggy (#23176, #26460) and under-tested. Since the introduction of RefreshSpans in 2.0, it doesn't even improve performance significantly in most cases. We have discouraged its use by mapping all four ANSI isolation levels to SERIALIZABLE, so users wouldn't get SNAPSHOT unless they specifically requested it. We should go the rest of the way and remove it completely.

We could remove the WriteTooOld and RetryOnPush fields of Transaction when SNAPSHOT isolation is gone. I think we might also be able to eliminate the distinction between the read and write timestamp cache.

In 2.1, we should map the SQL SNAPSHOT isolation level to SERIALIZABLE, but we'll need to leave all the supporting machinery in place until 2.2.
@bdarnell bdarnell mentioned this issue Jun 11, 2018
Merged
@tbg tbg added the A-kv-transactions Relating to MVCC and the transactional model. label Jun 12, 2018
@tbg tbg added this to Backlog in KV Jun 14, 2018
@bdarnell bdarnell added this to the 2.1 milestone Jun 27, 2018
@bdarnell bdarnell self-assigned this Jun 27, 2018
bdarnell added a commit to bdarnell/cockroach that referenced this issue Jun 27, 2018
@bdarnell
sql: Remove SNAPSHOT isolation
ea06958 
The SNAPSHOT isolation level is no longer accessible from SQL.
Requests for SNAPSHOT will be remapped to SERIALIZABLE (in the same
way that the other ANSI levels already are).

KV-level support will remain through the next release cycle to support
transactions that began during a rolling upgrade.

Fixes cockroachdb#26475

Release note (sql change): The SNAPSHOT isolation level has been
removed. Transactions that request to use it are now mapped to
SERIALIZABLE.
@bdarnell bdarnell mentioned this issue Jun 27, 2018
Merged
bdarnell added a commit to bdarnell/cockroach that referenced this issue Jun 28, 2018
@bdarnell
sql: Remove SNAPSHOT isolation
07f583e 
The SNAPSHOT isolation level is no longer accessible from SQL.
Requests for SNAPSHOT will be remapped to SERIALIZABLE (in the same
way that the other ANSI levels already are).

KV-level support will remain through the next release cycle to support
transactions that began during a rolling upgrade.

Fixes cockroachdb#26475

Release note (sql change): The SNAPSHOT isolation level has been
removed. Transactions that request to use it are now mapped to
SERIALIZABLE.
craig bot pushed a commit that referenced this issue Jun 29, 2018
@BramGruneir @vivekmenezes @bdarnell
Merge #27034 #27040
fd5743e 
27034: sql: turn on the optimizer for PREPARE statements r=benesch a=BramGruneir

The optimizer, when enabled, was not used for prepare statements. This patch
fixes that.

It should be noted that PREPARE has a limited subset of statements it can be
run with. Postgres only allows SELECT, INSERT, UPDATE, DELETE and VALUES
statements to be prepared.
See: https://www.postgresql.org/docs/current/static/sql-prepare.html

However, we allow a large number of additional statements.
As of right now, the optimizer only works on SELECT statements and will fallback
for all others, so this change should be safe for the foreseeable future.

Closes #26958.

Release note (sql change): When the cost based optimizer is enabled, it will
also affect prepared queries.

27040: sql: Remove SNAPSHOT isolation r=bdarnell a=bdarnell

The SNAPSHOT isolation level is no longer accessible from SQL.
Requests for SNAPSHOT will be remapped to SERIALIZABLE (in the same
way that the other ANSI levels already are).

KV-level support will remain through the next release cycle to support
transactions that began during a rolling upgrade.

Fixes #26475

Release note (sql change): The SNAPSHOT isolation level has been
removed. Transactions that request to use it are now mapped to
SERIALIZABLE.

The change in `lease_test.go` is the most complicated part here - I tried to adapt the test to a serializable transaction, but it's possible I misunderstood the intent of the test.

I left tests that used `BEGIN TRANSACTION SNAPSHOT` in place to ensure this syntax still works, and just changed the tests to reflect the new behavior of `SHOW transaction_isolation`.

While making this change I found that the previous change to map all the ANSI isolation levels to SERIALIZABLE was incomplete. There were some paths (such as `SET default_transaction_isolation`) that still used the old mapping of `READ COMMITTED` to `SNAPSHOT`. 

Also fixes a bug in `SET transaction_isolation='serializable'`, which was previously setting `snapshot` instead. 

Co-authored-by: Bram Gruneir <bram@cockroachlabs.com>
Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>
Co-authored-by: Ben Darnell <ben@cockroachlabs.com>
@craig craig bot closed this as completed in #27040 Jun 29, 2018
KV automation moved this from Backlog to Finished (milestone 3, ends 7/20) Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bdarnell bdarnell

Labels
A-kv-transactions Relating to MVCC and the transactional model.
Projects
None yet
Milestone
2.1
Development

Successfully merging a pull request may close this issue.

sql: Remove SNAPSHOT isolation bdarnell/cockroach
2 participants
@bdarnell @tbg