From 16299445caf00269baec599f9c8c84240c0d92ce Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Thu, 22 Aug 2024 18:49:07 -0400 Subject: [PATCH] sql: don't consider repeatable read to snapshot an upgrade One isolation level is not stronger than the other. We consider them aliases of each other. Epic: None Release note: None --- pkg/sql/conn_executor.go | 21 +++++++------------ pkg/sql/set_session_characteristics.go | 11 ++++------ pkg/sql/testdata/telemetry/isolation_level | 4 ---- .../logging/stmts_per_txn_limit | 1 - pkg/sql/vars.go | 15 ++++++------- 5 files changed, 18 insertions(+), 34 deletions(-) diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index 5ff8f24370a1..456e137160cb 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -3529,9 +3529,7 @@ var allowReadCommittedIsolation = settings.RegisterBoolSetting( settings.WithPublic, ) -// TODO(nvanbenschoten): rename this variable and update uses of it to favor -// "repeatable read" over "snapshot". -var allowSnapshotIsolation = settings.RegisterBoolSetting( +var allowRepeatableReadIsolation = settings.RegisterBoolSetting( settings.ApplicationLevel, "sql.txn.snapshot_isolation.enabled", "set to true to allow transactions to use the REPEATABLE READ isolation "+ @@ -3589,20 +3587,17 @@ func (ex *connExecutor) txnIsolationLevelToKV( upgradedDueToLicense = true } } - case tree.RepeatableReadIsolation: - // REPEATABLE READ is mapped to SNAPSHOT. - upgraded = true - fallthrough - case tree.SnapshotIsolation: - // SNAPSHOT is only allowed if the cluster setting is enabled and the - // cluster has a license. Otherwise it is mapped to SERIALIZABLE. - allowSnapshot := allowSnapshotIsolation.Get(&ex.server.cfg.Settings.SV) - if allowSnapshot && hasLicense { + case tree.RepeatableReadIsolation, tree.SnapshotIsolation: + // REPEATABLE READ and SNAPSHOT are considered aliases. The isolation levels + // are only allowed if the cluster setting is enabled and the cluster has a + // license. Otherwise, they are mapped to SERIALIZABLE. + allowRepeatableRead := allowRepeatableReadIsolation.Get(&ex.server.cfg.Settings.SV) + if allowRepeatableRead && hasLicense { ret = isolation.Snapshot } else { upgraded = true ret = isolation.Serializable - if allowSnapshot && !hasLicense { + if allowRepeatableRead && !hasLicense { upgradedDueToLicense = true } } diff --git a/pkg/sql/set_session_characteristics.go b/pkg/sql/set_session_characteristics.go index 0171a8e95a87..a95ab1cf54b8 100644 --- a/pkg/sql/set_session_characteristics.go +++ b/pkg/sql/set_session_characteristics.go @@ -28,7 +28,7 @@ func (p *planner) SetSessionCharacteristics( upgradedLevel := false upgradedDueToLicense := false allowReadCommitted := allowReadCommittedIsolation.Get(&p.execCfg.Settings.SV) - allowSnapshot := allowSnapshotIsolation.Get(&p.execCfg.Settings.SV) + allowRepeatableRead := allowRepeatableReadIsolation.Get(&p.execCfg.Settings.SV) hasLicense := base.CCLDistributionAndEnterpriseEnabled(p.ExecCfg().Settings) if err := p.sessionDataMutatorIterator.applyOnEachMutatorError(func(m sessionDataMutator) error { // Note: We also support SET DEFAULT_TRANSACTION_ISOLATION TO ' .... '. @@ -49,16 +49,13 @@ func (p *planner) SetSessionCharacteristics( } } m.SetDefaultTransactionIsolationLevel(level) - case tree.RepeatableReadIsolation: - upgradedLevel = true - fallthrough - case tree.SnapshotIsolation: + case tree.RepeatableReadIsolation, tree.SnapshotIsolation: level := tree.SerializableIsolation - if allowSnapshot && hasLicense { + if allowRepeatableRead && hasLicense { level = tree.SnapshotIsolation } else { upgradedLevel = true - if allowSnapshot && !hasLicense { + if allowRepeatableRead && !hasLicense { upgradedDueToLicense = true } } diff --git a/pkg/sql/testdata/telemetry/isolation_level b/pkg/sql/testdata/telemetry/isolation_level index 7e9cce9ac6f3..611b7501190c 100644 --- a/pkg/sql/testdata/telemetry/isolation_level +++ b/pkg/sql/testdata/telemetry/isolation_level @@ -112,7 +112,6 @@ feature-usage SET default_transaction_isolation = 'repeatable read' ---- sql.txn.isolation.executed_at.read_committed -sql.txn.isolation.upgraded_from.repeatable_read feature-usage SET default_transaction_isolation = 'snapshot' @@ -134,7 +133,6 @@ feature-usage SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL REPEATABLE READ ---- sql.txn.isolation.executed_at.read_committed -sql.txn.isolation.upgraded_from.repeatable_read feature-usage SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL SNAPSHOT @@ -156,7 +154,6 @@ feature-usage BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ; COMMIT ---- sql.txn.isolation.executed_at.snapshot -sql.txn.isolation.upgraded_from.repeatable_read feature-usage BEGIN TRANSACTION ISOLATION LEVEL SNAPSHOT; COMMIT @@ -180,7 +177,6 @@ feature-usage BEGIN; SET TRANSACTION ISOLATION LEVEL REPEATABLE READ; COMMIT ---- sql.txn.isolation.executed_at.snapshot -sql.txn.isolation.upgraded_from.repeatable_read feature-usage BEGIN; SET TRANSACTION ISOLATION LEVEL SNAPSHOT; COMMIT diff --git a/pkg/sql/testdata/telemetry_logging/logging/stmts_per_txn_limit b/pkg/sql/testdata/telemetry_logging/logging/stmts_per_txn_limit index 52662c965ad8..cd0c61429585 100644 --- a/pkg/sql/testdata/telemetry_logging/logging/stmts_per_txn_limit +++ b/pkg/sql/testdata/telemetry_logging/logging/stmts_per_txn_limit @@ -228,4 +228,3 @@ BEGIN; SELECT 1; SELECT 2; SELECT 3; SELECT 4; COMMIT; "TransactionFingerprintID": "2831371359051261045", "User": "root" } - diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go index d65c44ad5cb2..1c3d3acb30db 100644 --- a/pkg/sql/vars.go +++ b/pkg/sql/vars.go @@ -418,10 +418,10 @@ var varGen = map[string]sessionVar{ `default_transaction_isolation`: { Set: func(ctx context.Context, m sessionDataMutator, s string) error { allowReadCommitted := allowReadCommittedIsolation.Get(&m.settings.SV) - allowSnapshot := allowSnapshotIsolation.Get(&m.settings.SV) + allowRepeatableRead := allowRepeatableReadIsolation.Get(&m.settings.SV) hasLicense := base.CCLDistributionAndEnterpriseEnabled(m.settings) var allowedValues = []string{"serializable"} - if allowSnapshot { + if allowRepeatableRead { // TODO(nvanbenschoten): switch to "repeatable read". allowedValues = append(allowedValues, "snapshot") } @@ -449,16 +449,13 @@ var varGen = map[string]sessionVar{ upgradedDueToLicense = true } } - case tree.RepeatableReadIsolation: - upgraded = true - fallthrough - case tree.SnapshotIsolation: + case tree.RepeatableReadIsolation, tree.SnapshotIsolation: level = tree.SerializableIsolation - if allowSnapshot && hasLicense { + if allowRepeatableRead && hasLicense { level = tree.SnapshotIsolation } else { upgraded = true - if allowSnapshot && !hasLicense { + if allowRepeatableRead && !hasLicense { upgradedDueToLicense = true } } @@ -1615,7 +1612,7 @@ var varGen = map[string]sessionVar{ level, ok := tree.IsolationLevelMap[strings.ToLower(s)] if !ok { var allowedValues = []string{"serializable"} - if allowSnapshotIsolation.Get(&evalCtx.ExecCfg.Settings.SV) { + if allowRepeatableReadIsolation.Get(&evalCtx.ExecCfg.Settings.SV) { // TODO(nvanbenschoten): switch to "repeatable read". allowedValues = append(allowedValues, "snapshot") }