clarify isolation level upgrades #19490
Conversation
Files changed:
|
✅ Deploy Preview for cockroachdb-api-docs canceled.
|
✅ Deploy Preview for cockroachdb-interactivetutorials-docs canceled.
|
✅ Netlify Preview
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
This LGTM! I have a couple of suggestions, but you can take or leave them as you see fit. Thanks for doing this!
Eventually we need to document sql.txn.snapshot_isolation.enabled but I'll leave it up to @dikshant on the timing of that (I don't think 25.2 is the time).
src/current/v23.2/transactions.md
Outdated
|
|
||
| {% include_cached new-in.html version="v23.2" %} If [`READ COMMITTED` isolation is enabled]({% link {{ page.version.version }}/read-committed.md %}#enable-read-committed-isolation) using the `sql.txn.read_committed_isolation.enabled` [cluster setting]({% link {{ page.version.version }}/cluster-settings.md %}), `READ COMMITTED` is no longer an alias for `SERIALIZABLE`, and `READ UNCOMMITTED` becomes an alias for `READ COMMITTED`. | ||
| If `sql.txn.read_committed_isolation.enabled` is set to `false` (disabling `READ COMMITTED` isolation), all isolation levels upgrade to `SERIALIZABLE`. |
There was a problem hiding this comment.
suggestion: It might be clearer to make "transactions" the subject of the sentence rather than "isolation levels", something like:
If
sql.txn.read_committed_isolation.enabledis set tofalse(disablingREAD COMMITTEDisolation), all transactions run underSERIALIZABLEisolation regardless of the isolation level requested.
src/current/v23.2/transactions.md
Outdated
|
|
||
| #### Comparison | ||
| If a transaction includes [DDL]({% link {{ page.version.version }}/sql-statements.md %}#data-definition-statements) or [`AOST`]({% link {{ page.version.version }}/as-of-system-time.md %}) statements, all isolation levels upgrade to `SERIALIZABLE`. |
There was a problem hiding this comment.
I played around with this a bit, and I think we should not say anything about AOST. I believe the upgrade to serializable behavior for AOST transactions happens entirely in the KV layer. The SQL layer still considers AOST transactions to have read committed isolation, so it probably just confuses things to say they're upgraded.
@rafiss is this correct about DDL? Do we silently commit the transaction and start a new one for DDL?
There was a problem hiding this comment.
I agree with not mentioning AOST here.
As for DDL, yes that's correct, we will upgrade the transaction to SERIALIZABLE if possible -- meaning as long as the transaction hasn't performed any work yet, it will be upgraded. (https://github.com/cockroachdb/cockroach/blob/3acb6e287300ad4df232e2e4028af2f2435c5220/pkg/sql/conn_executor_ddl.go#L76)
Let's not mention autocommit behavior around DDLs here though. That's a separate piece of functionality, controlled by the autocommit_before_ddl setting.
There was a problem hiding this comment.
Thanks both for clarifying these points. I'm just going to avoid talking about AOST and DDL txns altogether, since we have this open issue that could update the behavior again at some point.
src/current/v23.2/transactions.md
Outdated
| | `READ COMMITTED` | -- | | ||
| | `REPEATABLE READ` | `SNAPSHOT` | | ||
| | `SNAPSHOT` | `SERIALIZABLE` | | ||
| | `SERIALIZABLE` | -- | |
There was a problem hiding this comment.
(Sorry, this is bikeshedding, you can ignore this comment unless you feel convinced 🙂.)
This table isn't incorrect, but IMO the REPEATABLE READ -> SNAPSHOT -> SERIALIZABLE upgrade chain idea is a little confusing and puts more emphasis on "upgrades" than I think they're worth.
If I were writing this I think I would say first (or last) that READ UNCOMMITTED is an alias for READ COMMITTED and REPEATABLE READ is an alias for SNAPSHOT and then I wouldn't mention those levels again. And then I would only describe the upgrades in terms of READ COMMITTED, SNAPSHOT, and SERIALIZABLE, which are the three "real" isolation levels we support. I think that would put the emphasis in the right place.
There was a problem hiding this comment.
Thanks for this! The reason I thought to highlight READ UNCOMMITTED as an 'upgrade' to READ COMMITTED is because the Upgrades to SQL Isolation Level chart seems to track that exact 'upgrade,' which became part of a customer issue recently. That's why I've been confused about "alias" vs "upgrade". cc @rafiss
There was a problem hiding this comment.
Ah, thanks for explaining. Now I understand.
From some experimentation, looks like:
- READ UNCOMMITTED -> READ COMMITTED does increment the metric
- REPEATABLE READ -> SNAPSHOT does not increment the metric
- READ COMMITTED -> SERIALIZABLE does increment the metric
- SNAPSHOT -> SERIALIZABLE does increment the metric
So I guess only REPEATABLE READ could be considered an alias for SNAPSHOT, and the rest are upgrades?
Here's how I figured that out:
[email protected]:26257/demoapp/defaultdb> \set show_times=false
[email protected]:26257/demoapp/defaultdb> SET CLUSTER SETTING sql.txn.repeatable_read_isolation.enabled = true;
SET CLUSTER SETTING
[email protected]:26257/demoapp/defaultdb> SELECT DISTINCT variable, value FROM [SHOW ALL CLUSTER SETTINGS] WHERE variable LIKE '%isolation%';
variable | value
--------------------------------------------+--------
sql.txn.read_committed_isolation.enabled | true
sql.txn.repeatable_read_isolation.enabled | true
(2 rows)
[email protected]:26257/demoapp/defaultdb> SELECT * FROM crdb_internal.node_metrics WHERE name LIKE '%iso_level%' ORDER BY name;
store_id | name | value
-----------+-------------------------------------------+--------
NULL | sql.txn.upgraded_iso_level.count | 0
NULL | sql.txn.upgraded_iso_level.count.internal | 0
(2 rows)
[email protected]:26257/demoapp/defaultdb> BEGIN TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; EXPLAIN ANALYZE SELECT 1; COMMIT;
BEGIN
info
-------------------------------------------
planning time: 332µs
execution time: 357µs
distribution: local
vectorized: true
plan type: custom
maximum memory usage: 10 KiB
DistSQL network usage: 0 B (0 messages)
regions: us-east1
sql cpu time: 50µs
estimated RUs consumed: 0
isolation level: read committed
priority: normal
quality of service: regular
• values
sql nodes: n1
regions: us-east1
actual row count: 1
execution time: 49µs
sql cpu time: 50µs
size: 1 column, 1 row
(21 rows)
COMMIT
[email protected]:26257/demoapp/defaultdb> SELECT * FROM crdb_internal.node_metrics WHERE name LIKE '%iso_level%' ORDER BY name;
store_id | name | value
-----------+-------------------------------------------+--------
NULL | sql.txn.upgraded_iso_level.count | 1
NULL | sql.txn.upgraded_iso_level.count.internal | 0
(2 rows)
[email protected]:26257/demoapp/defaultdb> BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ; EXPLAIN ANALYZE SELECT 1; COMMIT;
BEGIN
info
-------------------------------------------
planning time: 263µs
execution time: 272µs
distribution: local
vectorized: true
plan type: generic, re-optimized
maximum memory usage: 10 KiB
DistSQL network usage: 0 B (0 messages)
regions: us-east1
sql cpu time: 46µs
estimated RUs consumed: 0
isolation level: snapshot
priority: normal
quality of service: regular
• values
sql nodes: n1
regions: us-east1
actual row count: 1
execution time: 46µs
sql cpu time: 46µs
size: 1 column, 1 row
(21 rows)
COMMIT
[email protected]:26257/demoapp/defaultdb> SELECT * FROM crdb_internal.node_metrics WHERE name LIKE '%iso_level%' ORDER BY name;
store_id | name | value
-----------+-------------------------------------------+--------
NULL | sql.txn.upgraded_iso_level.count | 1
NULL | sql.txn.upgraded_iso_level.count.internal | 0
(2 rows)
[email protected]:26257/demoapp/defaultdb> SET CLUSTER SETTING sql.txn.repeatable_read_isolation.enabled = false;
SET CLUSTER SETTING
[email protected]:26257/demoapp/defaultdb> SET CLUSTER SETTING sql.txn.read_committed_isolation.enabled = false;
SET CLUSTER SETTING
[email protected]:26257/demoapp/defaultdb> SELECT DISTINCT variable, value FROM [SHOW ALL CLUSTER SETTINGS] WHERE variable LIKE '%isolation%';
variable | value
--------------------------------------------+--------
sql.txn.read_committed_isolation.enabled | false
sql.txn.repeatable_read_isolation.enabled | false
(2 rows)
[email protected]:26257/demoapp/defaultdb> BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED; EXPLAIN ANALYZE SELECT 1; COMMIT;
BEGIN
info
-------------------------------------------
planning time: 349µs
execution time: 362µs
distribution: local
vectorized: true
plan type: generic, re-optimized
maximum memory usage: 10 KiB
DistSQL network usage: 0 B (0 messages)
regions: us-east1
sql cpu time: 67µs
estimated RUs consumed: 0
isolation level: serializable
priority: normal
quality of service: regular
• values
sql nodes: n1
regions: us-east1
actual row count: 1
execution time: 67µs
sql cpu time: 67µs
size: 1 column, 1 row
(21 rows)
COMMIT
[email protected]:26257/demoapp/defaultdb> SELECT * FROM crdb_internal.node_metrics WHERE name LIKE '%iso_level%' ORDER BY name;
store_id | name | value
-----------+-------------------------------------------+--------
NULL | sql.txn.upgraded_iso_level.count | 2
NULL | sql.txn.upgraded_iso_level.count.internal | 0
(2 rows)
[email protected]:26257/demoapp/defaultdb> BEGIN TRANSACTION ISOLATION LEVEL SNAPSHOT; EXPLAIN ANALYZE SELECT 1; COMMIT;
BEGIN
info
-------------------------------------------
planning time: 81µs
execution time: 148µs
distribution: local
vectorized: true
plan type: generic, reused
maximum memory usage: 10 KiB
DistSQL network usage: 0 B (0 messages)
regions: us-east1
sql cpu time: 24µs
estimated RUs consumed: 0
isolation level: serializable
priority: normal
quality of service: regular
• values
sql nodes: n1
regions: us-east1
actual row count: 1
execution time: 24µs
sql cpu time: 24µs
size: 1 column, 1 row
(21 rows)
COMMIT
[email protected]:26257/demoapp/defaultdb> SELECT * FROM crdb_internal.node_metrics WHERE name LIKE '%iso_level%' ORDER BY name;
store_id | name | value
-----------+-------------------------------------------+--------
NULL | sql.txn.upgraded_iso_level.count | 3
NULL | sql.txn.upgraded_iso_level.count.internal | 0
(2 rows)
There was a problem hiding this comment.
+1, let's treat READ UNCOMMITTED -> READ COMMITTED as an upgrade of the isolation level, since they are clearly defined as different levels in PG docs and in the SQL standard.
And also +1 to treating SNAPSHOT and REPEATABLE READ as pure aliases of each other. However, the caveat with this is that this isolation level is not enabled by default. It's gated behind the cluster setting sql.txn.repeatable_read_isolation.enabled, which defaults to false (and this setting also has an alias sql.txn.snapshot_isolation.enabled). I'm just pointing this out in case we want to mention that setting here.
There was a problem hiding this comment.
oops, i see the setting is mentioned already. lgtm!
There was a problem hiding this comment.
nevermind, i got myself confused again. the current PR mentions sql.txn.read_committed_isolation.enabled which is true by default, but not sql.txn.repeatable_read_isolation.enabled which is false by default. given that we don't really mention repeatable read anywhere else in the docs, i'm not really sure how we want to discuss it here. i recall @dikshant mentioning he wanted to wait a bit before advertising that we support it (https://cockroachlabs.slack.com/archives/C051DMXEGMV/p1733255496192269?thread_ts=1733251940.956479&cid=C051DMXEGMV)
There was a problem hiding this comment.
Makes sense -- I'm removing the row with REPEATABLE READ but leaving the one with SNAPSHOT.
There was a problem hiding this comment.
In terms of the product decision to not mention REPEATABLE READ here, the same concerns would apply to mentioning SNAPSHOT. The usage of either of them is gated behind the sql.txn.repeatable_read_isolation.enabled/sql.txn.snapshot_isolation.enabled cluster setting, which are aliases of each other.
There was a problem hiding this comment.
OK, got it! Amending.
There was a problem hiding this comment.
Thanks! Yeah good to leave out repeatable read for now.
|
deploy-preview is failing with I think this is the line in Please fix the link. thanks |
Sorry! Fixed. |
DOC-13027