sql: Remove SNAPSHOT isolation #27040

bdarnell · 2018-06-27T22:30:25Z

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.

cockroach-teamcity · 2018-06-27T22:30:30Z

This change is

knz · 2018-06-28T10:58:51Z

Probably @andreimatei can review the changed test.

Reviewed 13 of 13 files at r1.
Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

Comments from Reviewable

andreimatei · 2018-06-28T15:08:36Z

LGTM

Reviewed 1 of 13 files at r1.
Review status: complete! 1 of 0 LGTMs obtained

pkg/sql/lease_test.go, line 865 at r1 (raw file):

		t.Fatal(err)
	}
	// Set the isolation level to Snapshot. This is because the test wants to

wasn't this comment relevant? Does the test test what it's supposed to test now? Like, does the error the txn receives on the first release have anything to do with the deadline, or is it a regular serialization restart? I'm thinking it's the latter cause I don't think we're smart enough to turn deadline errors into retriable errors.
If that's the case, I think you could turn the way the txn is pushed such that the span refresh would allow it to succeed if it wasn't for the deadline.
I have to admit though that I'm not entirely sure how the txn currently gets pushed. This test would benefit from more explanations.

pkg/sql/lease_test.go, line 870 at r1 (raw file):

	}

	// Insert an entry so that the transaction is guaranteed to be

this can go away since you're here. Timestamps are now assigned when the txn is created.

pkg/sql/sem/tree/txn.go, line 40 at r1 (raw file):

// IsolationLevelMap is a map from string isolation level name to isolation
// level, in the lowercase format that set isolation_level supports.
var IsolationLevelMap = map[string]IsolationLevel{

consider getting rid of this map, or at least turning it into a set

Comments from Reviewable

bdarnell · 2018-06-28T18:20:27Z

Review status: complete! 1 of 0 LGTMs obtained

pkg/sql/lease_test.go, line 865 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

wasn't this comment relevant? Does the test test what it's supposed to test now? Like, does the error the txn receives on the first release have anything to do with the deadline, or is it a regular serialization restart? I'm thinking it's the latter cause I don't think we're smart enough to turn deadline errors into retriable errors.
If that's the case, I think you could turn the way the txn is pushed such that the span refresh would allow it to succeed if it wasn't for the deadline.
I have to admit though that I'm not entirely sure how the txn currently gets pushed. This test would benefit from more explanations.

You're right, this test isn't testing what it used to. I was having a hard time figuring out how to generate the deadline-exceeded error in a serializable transaction, but fortunately @vivekmenezes already did in #23681 (which never got reviewed/merged). I'm going to cherry-pick that change into this branch.

Comments from Reviewable

bdarnell · 2018-06-28T18:37:49Z

OK, I've replaced the lease_test change with #23681 by @vivekmenezes (with one small change to fix a typo that was breaking the build) and it looks like it's doing the right thing now.

Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

Comments from Reviewable

andreimatei · 2018-06-28T20:43:44Z

Review status: complete! 0 of 0 LGTMs obtained (and 2 stale)

pkg/sql/lease_test.go, line 888 at r2 (raw file):

	}

	checkSelectResults := func() {

make this take rows as an arg, and also close them and check the err on close

pkg/sql/lease_test.go, line 931 at r2 (raw file):

	// The transaction read at one timestamp and wrote at another so it
	// has to be restarted because the spans read were modified by the backfill.
	if err := txReadWrite.Commit(); !testutils.IsError(err, "TransactionRetryError: retry txn \\(RETRY_SERIALIZABLE\\)") {

long line

pkg/sql/lease_test.go, line 948 at r2 (raw file):

	// Test the deadline exceeded error with a CREATE/DROP INDEX.
	testCases := []struct{ sql string }{

nit: testCases is not a great name since the tests are not independent. schemaChanges.

pkg/sql/lease_test.go, line 959 at r2 (raw file):

			t.Fatal(err)
		}
		txUpdate, err := sqlDB.Begin()

pls say something about this txnUpdate. Why is it necessary / what is it testing? Cause I don't get it.

pkg/sql/lease_test.go, line 970 at r2 (raw file):

		// This INSERT will cause the transaction to be pushed transparently,
		// which will be detected when we attempt to Commit() below only because

I don't get what's the difference between this txn that gets the deadline error (so span refresh succeeds) and txReadWrite above for whom the refresh doesn't succeed. That one says something about the backfill writing data. How come this one doesn't have the same problem?

Comments from Reviewable

removes the use of SNAPSHOT ISOLATION in the test. Tests read-only, write-only, and read-write transaction scenarios. Release note: None

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 · 2018-06-28T21:41:47Z

Review status: complete! 0 of 0 LGTMs obtained (and 2 stale)

pkg/sql/lease_test.go, line 888 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

make this take rows as an arg, and also close them and check the err on close

Done.

pkg/sql/lease_test.go, line 931 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

long line

Done.

pkg/sql/lease_test.go, line 959 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

pls say something about this txnUpdate. Why is it necessary / what is it testing? Cause I don't get it.

I think this txWrite and txUpdate are both doing the same thing, for INSERT and UPDATE statements respectively: Do a transaction that spans a schema change and see that it gets a deadline error. I'm not sure, though. We may need an answer from @vivekmenezes.

pkg/sql/lease_test.go, line 970 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I don't get what's the difference between this txn that gets the deadline error (so span refresh succeeds) and txReadWrite above for whom the refresh doesn't succeed. That one says something about the backfill writing data. How come this one doesn't have the same problem?

I think the difference is in the schema changes: the backfill process/state machine for index changes is different from the one for adding a column. If I change testCases to add a column, I see the serializable failure as above.

This test needs some more documentation and should probably be refactored so all the different schema changes are handled through one loop with different expectations. But I'm not confident I understand what's going on here well enough to do that, so I'd like to go ahead and merge this and leave the cleanup to @vivekmenezes.

Comments from Reviewable

andreimatei · 2018-06-28T22:18:56Z

merge at will

Review status: complete! 0 of 0 LGTMs obtained (and 2 stale)

Comments from Reviewable

bdarnell · 2018-06-29T01:37:14Z

bors r+

craig · 2018-06-29T03:23:32Z

Timed out (retrying...)

craig · 2018-06-29T03:39:42Z

Build failed (retrying...)

GitHub CI (Cockroach)

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 · 2018-06-29T05:03:24Z

Build succeeded

GitHub CI (Cockroach)

bdarnell requested review from a team June 27, 2018 22:30

knz mentioned this pull request Jun 28, 2018

sql-2.0: ensure that serializable means serializable #27047

Merged

knz requested a review from andreimatei June 28, 2018 10:59

bdarnell mentioned this pull request Jun 28, 2018

sql: improve TestTxnObeysTableModificationTime #23681

Closed

bdarnell force-pushed the all-txns-are-serializable branch from ea06958 to 5354e8e Compare June 28, 2018 18:30

vivekmenezes and others added 2 commits June 28, 2018 17:37

sql: improve TestTxnObeysTableModificationTime

411b689

removes the use of SNAPSHOT ISOLATION in the test. Tests read-only, write-only, and read-write transaction scenarios. Release note: None

bdarnell force-pushed the all-txns-are-serializable branch from 5354e8e to 07f583e Compare June 28, 2018 21:41

craig bot merged commit 07f583e into cockroachdb:master Jun 29, 2018

jseldess mentioned this pull request Jul 31, 2018

sql: Remove SNAPSHOT isolation cockroachdb/docs#3447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Remove SNAPSHOT isolation #27040

sql: Remove SNAPSHOT isolation #27040

bdarnell commented Jun 27, 2018 •

edited

cockroach-teamcity commented Jun 27, 2018

knz commented Jun 28, 2018

andreimatei commented Jun 28, 2018

bdarnell commented Jun 28, 2018

bdarnell commented Jun 28, 2018

andreimatei commented Jun 28, 2018

bdarnell commented Jun 28, 2018

andreimatei commented Jun 28, 2018

bdarnell commented Jun 29, 2018

craig bot commented Jun 29, 2018

craig bot commented Jun 29, 2018

craig bot commented Jun 29, 2018

sql: Remove SNAPSHOT isolation #27040

sql: Remove SNAPSHOT isolation #27040

Conversation

bdarnell commented Jun 27, 2018 • edited

cockroach-teamcity commented Jun 27, 2018

knz commented Jun 28, 2018

andreimatei commented Jun 28, 2018

bdarnell commented Jun 28, 2018

bdarnell commented Jun 28, 2018

andreimatei commented Jun 28, 2018

bdarnell commented Jun 28, 2018

andreimatei commented Jun 28, 2018

bdarnell commented Jun 29, 2018

craig bot commented Jun 29, 2018

Timed out (retrying...)

craig bot commented Jun 29, 2018

Build failed (retrying...)

craig bot commented Jun 29, 2018

Build succeeded

bdarnell commented Jun 27, 2018 •

edited