Conversation
rmloveland
left a comment
There was a problem hiding this comment.
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: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
SNAPSHOTisolation, 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.)
sploiselle
left a comment
There was a problem hiding this comment.
Reviewable status:
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."
jseldess
left a comment
There was a problem hiding this comment.
Reviewable status:
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.
3d40b37 to
370bf91
Compare
sploiselle
left a comment
There was a problem hiding this comment.
Reviewable status:
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
SNAPSHOTisolation, 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/gCite: 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.
|
@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 |
jseldess
left a comment
There was a problem hiding this comment.
LGTM still. I'd lean toward removing from set-vars.
Reviewable status:
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?
370bf91 to
c5e6e10
Compare
3a638ab to
1c0a062
Compare
1c0a062 to
27525ed
Compare
|
@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. |
jseldess
left a comment
There was a problem hiding this comment.
, with a nit about
NOP. Thanks, @sploiselle.
Reviewable status:
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.
27525ed to
e49cbae
Compare
Mostly Rich's work, but includes BNF modification
Closes #1585