Skip to content

Remove SNAPSHOT support#3958

Merged
sploiselle merged 6 commits intomasterfrom
cerealizable
Nov 19, 2018
Merged

Remove SNAPSHOT support#3958
sploiselle merged 6 commits intomasterfrom
cerealizable

Conversation

@sploiselle
Copy link
Copy Markdown
Contributor

@sploiselle sploiselle commented Oct 31, 2018

Mostly Rich's work, but includes BNF modification

Closes #1585

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@cockroach-teamcity
Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Contributor

@rmloveland rmloveland left a comment

Choose a reason for hiding this comment

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

Looking good except for some "deprecation" terminology that I think is slightly incorrect.

Thanks for taking this one over Sean! (PS you are my hero for updating all those diagrams.)

Reviewed 1 of 3 files at r1, 6 of 6 files at r2, 3 of 3 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


v2.1/begin-transaction.md, line 38 at r3 (raw file):

In previous versions of CockroachDB, you could set transactions to SNAPSHOT isolation, but that feature has been deprecated.

"Deprecated" means the usage is still supported but is not preferred and is likely to be removed in a future release . In this case we have removed the ability to set any other isolation levels.

Suggest s/deprecated/removed/g

Cite: https://en.wikipedia.org/wiki/Deprecation

PS I'm sorry if this comes across as annoying in text! Just trying to communicate clearly and stuff. :-}


v2.1/demo-serializable.md, line 7 at r3 (raw file):

CockroachDB v2.1 always

While we're in here anyway, I think it would be good to remove this mention of version numbers, since this will apply to 2.1++ and friends as well.


v2.1/transactions.md, line 213 at r3 (raw file):

CockroachDB efficiently supports the strongest ANSI transaction isolation level: `SERIALIZABLE`. All other ANSI transaction isolaton levels (e.g., `READ UNCOMMITTED`, `READ COMMITTED`, and `REPEATABLE READ`) are automatically upgraded to `SERIALIZABLE`. Weaker isolation levels have historically been used to maximize transaction throughput. However, [recent research](http://www.bailis.org/papers/acidrain-sigmod2017.pdf) has demonstrated that the use of weak isolation levels results in substantial vulnerability to concurrency-based attacks.

<span class="version-tag">New in v2.1:</span> CockroachDB now only supports `SERIALIZABLE` isolation. Previous versions supported `SNAPSHOT` isolation, but that feature has been deprecated.

Same comment as above re: s/deprecated/removed/g. (Although perhaps there is some nuance around the fact that you can set the setting but it will be a no-op (AFAICT)? Your call.)

Copy link
Copy Markdown
Contributor Author

@sploiselle sploiselle left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


v2.1/transactions.md, line 213 at r3 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Same comment as above re: s/deprecated/removed/g. (Although perhaps there is some nuance around the fact that you can set the setting but it will be a no-op (AFAICT)? Your call.)

Ah, I was have my cake and eating it, too. It is a valid setting but a no-op. I'll take your suggestion and make more clear with simply saying it's been "removed."

Copy link
Copy Markdown
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

:lgtm:, with some suggestions

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


v2.1/begin-transaction.md, line 36 at r3 (raw file):

-----------|------------- 
`PRIORITY` | If you do not want the transaction to run with `NORMAL` priority, you can set it to `LOW` or `HIGH`.<br/><br/>Transactions with higher priority are less likely to need to be retried.<br/><br/>For more information, see [Transactions: Priorities](transactions.html#transaction-priorities).<br/><br/>**Default**: `NORMAL`
`READ` | Set the transaction access mode to `READ ONLY` or `READ WRITE`. The current transaction access mode is also exposed as the [session variable](show-vars.html) `transaction_read_only`.<br><br>**Default**: `READ WRITE`

Thanks for covering this as well. I think it fixes #1585?


v2.1/begin-transaction.md, line 114 at r3 (raw file):

~~~

You can also set a transaction's isolation level and priority with [`SET TRANSACTION`](set-transaction.html).

Do we want to revise this as well?


v2.1/demo-serializable.md, line 7 at r3 (raw file):

---

In contrast to most databases, CockroachDB v2.1 always uses `SERIALIZABLE` isolation, which is the strongest of the four [transaction isolation levels](https://en.wikipedia.org/wiki/Isolation_(database_systems)) defined by the SQL standard and is stronger than the `SNAPSHOT` isolation level developed later. `SERIALIZABLE` isolation guarantees that even though transactions may execute in parallel, the end result is the same as if they had executed one at a time, without any concurrency. This ensures data correctness by preventing all "anomalies" allowed by weaker isolation levels.

I don't think we should remove this clause, actually. We're not talking about snapshot support in cockroach, just the fact that serializable is strong than this level, which was introduced later than the four SQL standard levels.


v2.1/set-transaction.md, line 26 at r3 (raw file):

`READ` | Set the transaction access mode to `READ ONLY` or `READ WRITE`. The current transaction access mode is also exposed as the [session variable](show-vars.html) `transaction_read_only`.<br><br>**Default**: `READ WRITE`

<span class="version-tag">New in v2.1:</span> CockroachDB now only supports `SERIALIZABLE` isolation, so transactions can no longer be meaningfully set to any other `ISOLATION LEVEL`. In previous versions of CockroachDB, you could set transactions to `SNAPSHOT` isolation, but that feature has been deprecated.

deprecated > removed


v2.1/transactions.md, line 223 at r3 (raw file):

With `SERIALIZABLE` isolation, a transaction behaves as though it has the entire database all to itself for the duration of its execution. This means that no concurrent writers can affect the transaction unless they commit before it starts, and no concurrent readers can be affected by the transaction until it has successfully committed. This is the strongest level of isolation provided by CockroachDB and it's the default.

`SERIALIZABLE` isolation permits no anomalies. To prevent [write skew](https://en.wikipedia.org/wiki/Snapshot_isolation) anomalies, `SERIALIZABLE` isolation may require transaction restarts.

Just an idea, but we might take the opportunity to link to the new serializable transactions tutorial, which focuses on how serializable prevents write skew.


v2.1/architecture/transaction-layer.md, line 135 at r3 (raw file):

CockroachDB efficiently supports the strongest ANSI transaction isolation level: `SERIALIZABLE`. All other ANSI transaction isolation levels (e.g., `SNAPSHOT`, `READ UNCOMMITTED`, `READ COMMITTED`, and `REPEATABLE READ`) are automatically upgraded to `SERIALIZABLE`. Weaker isolation levels have historically been used to maximize transaction throughput. However, [recent research](http://www.bailis.org/papers/acidrain-sigmod2017.pdf) has demonstrated that the use of weak isolation levels results in substantial vulnerability to concurrency-based attacks.

Specifically, CockroachDB's transactions default to **serializable snapshot isolation** (equivalent to ANSI SQL's strongest isolation level, `SERIALIZABLE`). This isolation level does not allow any anomalies in your data, and is enforced by requiring the client to retry transactions if serializability violations are possible.

Here or in the paragraph above, we could probably be clearer that cockroach supports only serializable. It's the default and only option.

@cockroach-teamcity
Copy link
Copy Markdown
Member

@cockroach-teamcity
Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Contributor Author

@sploiselle sploiselle left a comment

Choose a reason for hiding this comment

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

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


v2.1/begin-transaction.md, line 36 at r3 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Thanks for covering this as well. I think it fixes #1585?

Done.


v2.1/begin-transaction.md, line 38 at r3 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

In previous versions of CockroachDB, you could set transactions to SNAPSHOT isolation, but that feature has been deprecated.

"Deprecated" means the usage is still supported but is not preferred and is likely to be removed in a future release . In this case we have removed the ability to set any other isolation levels.

Suggest s/deprecated/removed/g

Cite: https://en.wikipedia.org/wiki/Deprecation

PS I'm sorry if this comes across as annoying in text! Just trying to communicate clearly and stuff. :-}

Done.


v2.1/begin-transaction.md, line 114 at r3 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Do we want to revise this as well?

@jseldress @rmloveland Whoa. Good catch there. I actually found a whole rash of additional docs that needed additional clarification because of this note.


v2.1/demo-serializable.md, line 7 at r3 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

CockroachDB v2.1 always

While we're in here anyway, I think it would be good to remove this mention of version numbers, since this will apply to 2.1++ and friends as well.

Done.


v2.1/demo-serializable.md, line 7 at r3 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

I don't think we should remove this clause, actually. We're not talking about snapshot support in cockroach, just the fact that serializable is strong than this level, which was introduced later than the four SQL standard levels.

Done.


v2.1/set-transaction.md, line 26 at r3 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

deprecated > removed

Done.


v2.1/transactions.md, line 223 at r3 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Just an idea, but we might take the opportunity to link to the new serializable transactions tutorial, which focuses on how serializable prevents write skew.

Done.


v2.1/architecture/transaction-layer.md, line 135 at r3 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

Here or in the paragraph above, we could probably be clearer that cockroach supports only serializable. It's the default and only option.

Done.

@sploiselle
Copy link
Copy Markdown
Contributor Author

@jseldess @rmloveland Found a bunch of mentions of "isolation level" that I hadn't thought to look up in the first pass. PTAL. One of the big questions I have is the treatment of set-vars.md given that there's a kind of impedance mismatch between the interface and the operation––you have any thoughts/suggestions there?

Copy link
Copy Markdown
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

LGTM still. I'd lean toward removing from set-vars.

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


v2.1/set-vars.md, line 52 at r4 (raw file):

 `timezone` | The default time zone for the current session.<br><br>This value can be a string representation of a local system-defined time zone (e.g., `'EST'`, `'America/New_York'`) or a positive or negative numeric offset from UTC (e.g., `-7`, `+7`). Also, `DEFAULT`, `LOCAL`, or `0` sets the session time zone to `UTC`.</br><br>See [Setting the Time Zone](#set-time-zone) for more details. <br><br>This session variable was named `"time zone"` (with a space) in CockroachDB 1.x. It has been renamed for compatibility with PostgreSQL. | `UTC` | Yes
 `tracing` | The trace recording state.<br><br>See [`SET TRACING`](#set-tracing) for more details. | `off` | Yes
 `transaction_isolation` | _Read Only_: All transactions execute with `SERIALIZABLE` isolation, meaning you cannot `SET transaction_isolation` to any value. See [Transactions: Isolation levels](transactions.html#isolation-levels).<br><br>This session variable was called `transaction isolation level` (with spaces) in CockroachDB 1.x. It has been renamed for compatibility with PostgreSQL. | `SERIALIZABLE` | Yes

Perhaps we should just remove these variables from this doc, since they're not set-able, and leave them in the show-vars doc? Is there a precedent for that? A variable that's only viewable that we list for show but not set?

@sploiselle sploiselle force-pushed the cerealizable branch 2 times, most recently from 3a638ab to 1c0a062 Compare November 14, 2018 16:38
@sploiselle
Copy link
Copy Markdown
Contributor Author

@jseldess PTAL. I think that identifying these setting as NOP is the most accurate.

After a brief discussion in cockroachdb/cockroach#32115, the setting is potentially important for ORM compatibility. My apprehension with removing it is that it could appear like the setting isn't available for the next generation of ORM developers that want to consider integrating CockroachDB support.

@cockroach-teamcity
Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Contributor

@jseldess jseldess left a comment

Choose a reason for hiding this comment

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

:lgtm:, with a nit about NOP. Thanks, @sploiselle.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


v2.1/set-vars.md, line 41 at r8 (raw file):

 `application_name` | The current application name for statistics collection. | Empty string | Yes
 `database` | The [current database](sql-name-resolution.html#current-database).  Database in connection string, or empty if not specified | Yes
 `default_transaction_isolation` | _NOP:_ All transactions execute with [`SERIALIZABLE` isolation](transactions.html#isolation-levels), meaning you cannot `SET default_transaction_isolation...` to any value. However, this setting remains available for ORM compatibility. | `SERIALIZABLE` | Yes

I'd like to avoid using this term NOP in our docs. Although familiar to many engineers, it's very jargony. Can you find another more descriptive way to tell users this can't be changed?


v2.1/set-vars.md, line 52 at r8 (raw file):

 `timezone` | The default time zone for the current session.<br><br>This value can be a string representation of a local system-defined time zone (e.g., `'EST'`, `'America/New_York'`) or a positive or negative numeric offset from UTC (e.g., `-7`, `+7`). Also, `DEFAULT`, `LOCAL`, or `0` sets the session time zone to `UTC`.</br><br>See [Setting the Time Zone](#set-time-zone) for more details. <br><br>This session variable was named `"time zone"` (with a space) in CockroachDB 1.x. It has been renamed for compatibility with PostgreSQL. | `UTC` | Yes
 `tracing` | The trace recording state.<br><br>See [`SET TRACING`](#set-tracing) for more details. | `off` | Yes
 `transaction_isolation` | _NOP:_ All transactions execute with [`SERIALIZABLE` isolation](transactions.html#isolation-levels), meaning you cannot `SET transaction_isolation...` to any value. However, this  available for ORM compatibility.<br><br>This session variable was called `transaction isolation level` (with spaces) in CockroachDB 1.x. It has been renamed for compatibility with PostgreSQL. | `SERIALIZABLE` | Yes

Same as above.

@cockroach-teamcity
Copy link
Copy Markdown
Member

@sploiselle sploiselle merged commit 80e2025 into master Nov 19, 2018
@knz knz removed the in progress label Nov 19, 2018
@sploiselle sploiselle deleted the cerealizable branch November 19, 2018 19:15
jseldess added a commit that referenced this pull request Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants