From e633d5edb47b54a08905ae92d842625ba7805000 Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Mon, 22 May 2023 11:55:59 -0700 Subject: [PATCH] opt: disallow SELECT FOR UPDATE under weak isolation levels Temporarily disallow `SELECT FOR UPDATE` statements under all isolation levels that are not `SERIALIZABLE` (i.e. `SNAPSHOT` and `READ COMMITTED`). We will allow them again when the following issues are fixed: - #57031 - #75457 - #100193 - #100194 Fixes: #100144 Release note: None --- .../tests/3node-tenant/generated_test.go | 7 + pkg/sql/conn_executor.go | 1 + .../select_for_update_read_committed | 142 ++++++++++++++++++ .../tests/fakedist-disk/generated_test.go | 7 + .../tests/fakedist-vec-off/generated_test.go | 7 + .../tests/fakedist/generated_test.go | 7 + .../generated_test.go | 7 + .../tests/local-vec-off/generated_test.go | 7 + .../logictest/tests/local/generated_test.go | 7 + pkg/sql/opt/exec/execbuilder/BUILD.bazel | 1 + pkg/sql/opt/exec/execbuilder/relational.go | 73 +++++---- pkg/sql/sem/eval/BUILD.bazel | 1 + pkg/sql/sem/eval/context.go | 3 + 13 files changed, 240 insertions(+), 30 deletions(-) create mode 100644 pkg/sql/logictest/testdata/logic_test/select_for_update_read_committed diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index b66f60ca54d5..fed1f1e8c579 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -1606,6 +1606,13 @@ func TestTenantLogic_select_for_update( runLogicTest(t, "select_for_update") } +func TestTenantLogic_select_for_update_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "select_for_update_read_committed") +} + func TestTenantLogic_select_index( t *testing.T, ) { diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index 23683484b75c..d667dbf19dae 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -3512,6 +3512,7 @@ func (ex *connExecutor) resetEvalCtx(evalCtx *extendedEvalContext, txn *kv.Txn, evalCtx.TxnReadOnly = ex.state.readOnly evalCtx.TxnImplicit = ex.implicitTxn() evalCtx.TxnIsSingleStmt = false + evalCtx.TxnIsoLevel = ex.state.isolationLevel if newTxn || !ex.implicitTxn() { // Only update the stmt timestamp if in a new txn or an explicit txn. This is because this gets // called multiple times during an extended protocol implicit txn, but we diff --git a/pkg/sql/logictest/testdata/logic_test/select_for_update_read_committed b/pkg/sql/logictest/testdata/logic_test/select_for_update_read_committed new file mode 100644 index 000000000000..01eefc71685e --- /dev/null +++ b/pkg/sql/logictest/testdata/logic_test/select_for_update_read_committed @@ -0,0 +1,142 @@ +# LogicTest: !local-mixed-22.2-23.1 + +# SELECT FOR UPDATE is prohibited under weaker isolation levels until we improve +# locking. See #57031, #75457, #100144. + +statement ok +CREATE TABLE supermarket ( + person STRING PRIMARY KEY, + aisle INT NOT NULL, + starts_with STRING GENERATED ALWAYS AS (left(person, 1)) STORED, + ends_with STRING GENERATED ALWAYS AS (right(person, 3)) STORED, + INDEX (starts_with), + INDEX (ends_with) +) + +statement ok +INSERT INTO supermarket (person, aisle) + VALUES ('abbie', 1), ('gideon', 2), ('matilda', 3), ('michael', 4) + +# SELECT FOR UPDATE should still work under serializable isolation. +statement ok +BEGIN + +query I +SELECT aisle FROM supermarket WHERE person = 'gideon' FOR UPDATE +---- +2 + +statement ok +UPDATE supermarket SET aisle = 2 WHERE person = 'abbie' + +statement ok +COMMIT + +# It should fail under read committed isolation. +statement ok +BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED + +query error pgcode 0A000 cannot execute SELECT FOR UPDATE statements under ReadCommitted isolation +SELECT aisle FROM supermarket WHERE person = 'matilda' FOR UPDATE + +statement ok +ROLLBACK + +# It should also fail under snapshot isolation. +statement ok +SET CLUSTER SETTING sql.txn.snapshot_isolation.enabled = true + +statement ok +BEGIN TRANSACTION ISOLATION LEVEL SNAPSHOT + +query error pgcode 0A000 cannot execute SELECT FOR UPDATE statements under Snapshot isolation +SELECT aisle FROM supermarket WHERE person = 'matilda' FOR UPDATE + +statement ok +ROLLBACK + +statement ok +RESET CLUSTER SETTING sql.txn.snapshot_isolation.enabled + +# SELECT FOR UPDATE in a subquery should also fail under read committed. +statement ok +BEGIN TRANSACTION + +statement ok +SET TRANSACTION ISOLATION LEVEL READ COMMITTED; + +query error pgcode 0A000 cannot execute SELECT FOR UPDATE statements under ReadCommitted isolation +UPDATE supermarket + SET aisle = (SELECT aisle FROM supermarket WHERE person = 'matilda' FOR UPDATE) + WHERE person = 'michael' + +statement ok +ROLLBACK + +# It should also fail in a CTE. +statement ok +BEGIN TRANSACTION + +statement ok +SET TRANSACTION ISOLATION LEVEL READ COMMITTED; + +query error pgcode 0A000 cannot execute SELECT FOR UPDATE statements under ReadCommitted isolation +WITH s AS + (SELECT aisle FROM supermarket WHERE person = 'matilda' FOR UPDATE) +SELECT aisle + 1 FROM s + +statement ok +ROLLBACK + +statement ok +SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED + +# Creating a UDF using SELECT FOR UPDATE should succeed under read committed. +statement ok +CREATE FUNCTION wrangle (name STRING) RETURNS INT LANGUAGE SQL AS $$ + SELECT aisle FROM supermarket WHERE person = name FOR UPDATE +$$ + +# But calling that function should fail. +query error pgcode 0A000 cannot execute SELECT FOR UPDATE statements under ReadCommitted isolation +INSERT INTO supermarket (person, aisle) VALUES ('grandma', wrangle('matilda')) + +statement ok +DROP FUNCTION wrangle + +# Preparing a SELECT FOR UPDATE should succeed under read committed. +statement ok +PREPARE psa AS SELECT aisle FROM supermarket WHERE person = $1::STRING FOR UPDATE + +# But execution should fail. +query error pgcode 0A000 cannot execute SELECT FOR UPDATE statements under ReadCommitted isolation +EXECUTE psa('matilda') + +statement ok +DEALLOCATE psa + +# SELECT FOR UPDATE using a lookup join should also fail. +query error pgcode 0A000 cannot execute SELECT FOR UPDATE statements under ReadCommitted isolation +WITH names AS MATERIALIZED + (SELECT 'matilda' AS person) +SELECT aisle + FROM names + NATURAL INNER LOOKUP JOIN supermarket + FOR UPDATE + +# SELECT FOR UPDATE using an index join should also fail. +query error pgcode 0A000 cannot execute SELECT FOR UPDATE statements under ReadCommitted isolation +SELECT aisle + FROM supermarket@supermarket_starts_with_idx + WHERE starts_with = 'm' + FOR UPDATE + +# SELECT FOR UPDATE using a zigzag join should also fail. +query error pgcode 0A000 cannot execute SELECT FOR UPDATE statements under ReadCommitted isolation +SELECT aisle + FROM supermarket@{FORCE_ZIGZAG} + WHERE starts_with = 'm' AND ends_with = 'lda' + FOR UPDATE + +statement ok +SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL SERIALIZABLE diff --git a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go index 01da4d9ceb26..5a470be9fb1b 100644 --- a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go @@ -1584,6 +1584,13 @@ func TestLogic_select_for_update( runLogicTest(t, "select_for_update") } +func TestLogic_select_for_update_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "select_for_update_read_committed") +} + func TestLogic_select_index( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go index ef01a410b3f9..b6762a993ccb 100644 --- a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go @@ -1584,6 +1584,13 @@ func TestLogic_select_for_update( runLogicTest(t, "select_for_update") } +func TestLogic_select_for_update_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "select_for_update_read_committed") +} + func TestLogic_select_index( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist/generated_test.go b/pkg/sql/logictest/tests/fakedist/generated_test.go index 108c95646bf2..4ab35a0298e4 100644 --- a/pkg/sql/logictest/tests/fakedist/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist/generated_test.go @@ -1598,6 +1598,13 @@ func TestLogic_select_for_update( runLogicTest(t, "select_for_update") } +func TestLogic_select_for_update_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "select_for_update_read_committed") +} + func TestLogic_select_index( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go index 5bff5c0ddf61..95b98e1740b4 100644 --- a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go +++ b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go @@ -1563,6 +1563,13 @@ func TestLogic_select_for_update( runLogicTest(t, "select_for_update") } +func TestLogic_select_for_update_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "select_for_update_read_committed") +} + func TestLogic_select_index( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-vec-off/generated_test.go b/pkg/sql/logictest/tests/local-vec-off/generated_test.go index ce1df235e4d0..565a69fbfcf6 100644 --- a/pkg/sql/logictest/tests/local-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/local-vec-off/generated_test.go @@ -1591,6 +1591,13 @@ func TestLogic_select_for_update( runLogicTest(t, "select_for_update") } +func TestLogic_select_for_update_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "select_for_update_read_committed") +} + func TestLogic_select_index( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local/generated_test.go b/pkg/sql/logictest/tests/local/generated_test.go index 8bac2d446c19..9269558ff59e 100644 --- a/pkg/sql/logictest/tests/local/generated_test.go +++ b/pkg/sql/logictest/tests/local/generated_test.go @@ -1731,6 +1731,13 @@ func TestLogic_select_for_update( runLogicTest(t, "select_for_update") } +func TestLogic_select_for_update_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "select_for_update_read_committed") +} + func TestLogic_select_index( t *testing.T, ) { diff --git a/pkg/sql/opt/exec/execbuilder/BUILD.bazel b/pkg/sql/opt/exec/execbuilder/BUILD.bazel index 7a9a4cd6ec10..3e3dfbe5b702 100644 --- a/pkg/sql/opt/exec/execbuilder/BUILD.bazel +++ b/pkg/sql/opt/exec/execbuilder/BUILD.bazel @@ -15,6 +15,7 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/sql/opt/exec/execbuilder", visibility = ["//visibility:public"], deps = [ + "//pkg/kv/kvserver/concurrency/isolation", "//pkg/server/telemetry", "//pkg/sql/catalog/colinfo", "//pkg/sql/catalog/descpb", diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index 68783fbe01ee..78f3c6732149 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -17,6 +17,7 @@ import ( "math" "strings" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" @@ -618,18 +619,9 @@ func (b *Builder) scanParams( return exec.ScanParams{}, opt.ColMap{}, err } - locking := scan.Locking - if b.forceForUpdateLocking { - locking = locking.Max(forUpdateLocking) - } - b.ContainsNonDefaultKeyLocking = b.ContainsNonDefaultKeyLocking || locking.IsLocking() - - // Raise error if row-level locking is part of a read-only transaction. - // TODO(nvanbenschoten): this check should be shared across all expressions - // that can perform row-level locking. - if locking.IsLocking() && b.evalCtx.TxnReadOnly { - return exec.ScanParams{}, opt.ColMap{}, pgerror.Newf(pgcode.ReadOnlySQLTransaction, - "cannot execute %s in a read-only transaction", locking.Strength.String()) + locking, err := b.buildLocking(scan.Locking) + if err != nil { + return exec.ScanParams{}, opt.ColMap{}, err } needed, outputMap := b.getColumns(scan.Cols, scan.Table) @@ -2170,11 +2162,10 @@ func (b *Builder) buildIndexJoin(join *memo.IndexJoinExpr) (execPlan, error) { cols := join.Cols needed, output := b.getColumns(cols, join.Table) - locking := join.Locking - if b.forceForUpdateLocking { - locking = locking.Max(forUpdateLocking) + locking, err := b.buildLocking(join.Locking) + if err != nil { + return execPlan{}, err } - b.ContainsNonDefaultKeyLocking = b.ContainsNonDefaultKeyLocking || locking.IsLocking() res := execPlan{outputCols: output} b.recordJoinAlgorithm(exec.IndexJoin) @@ -2491,11 +2482,10 @@ func (b *Builder) buildLookupJoin(join *memo.LookupJoinExpr) (execPlan, error) { idx := tab.Index(join.Index) b.IndexesUsed = util.CombineUnique(b.IndexesUsed, []string{fmt.Sprintf("%d@%d", tab.ID(), idx.ID())}) - locking := join.Locking - if b.forceForUpdateLocking { - locking = locking.Max(forUpdateLocking) + locking, err := b.buildLocking(join.Locking) + if err != nil { + return execPlan{}, err } - b.ContainsNonDefaultKeyLocking = b.ContainsNonDefaultKeyLocking || locking.IsLocking() joinType, err := joinOpToJoinType(join.JoinType) if err != nil { @@ -2733,11 +2723,10 @@ func (b *Builder) buildInvertedJoin(join *memo.InvertedJoinExpr) (execPlan, erro return execPlan{}, err } - locking := join.Locking - if b.forceForUpdateLocking { - locking = locking.Max(forUpdateLocking) + locking, err := b.buildLocking(join.Locking) + if err != nil { + return execPlan{}, err } - b.ContainsNonDefaultKeyLocking = b.ContainsNonDefaultKeyLocking || locking.IsLocking() joinType, err := joinOpToJoinType(join.JoinType) if err != nil { @@ -2813,13 +2802,14 @@ func (b *Builder) buildZigzagJoin(join *memo.ZigzagJoinExpr) (execPlan, error) { leftOrdinals, leftColMap := b.getColumns(leftCols, join.LeftTable) rightOrdinals, rightColMap := b.getColumns(rightCols, join.RightTable) - leftLocking := join.LeftLocking - rightLocking := join.RightLocking - if b.forceForUpdateLocking { - leftLocking = leftLocking.Max(forUpdateLocking) - rightLocking = rightLocking.Max(forUpdateLocking) + leftLocking, err := b.buildLocking(join.LeftLocking) + if err != nil { + return execPlan{}, err + } + rightLocking, err := b.buildLocking(join.RightLocking) + if err != nil { + return execPlan{}, err } - b.ContainsNonDefaultKeyLocking = b.ContainsNonDefaultKeyLocking || leftLocking.IsLocking() || rightLocking.IsLocking() allCols := joinOutputMap(leftColMap, rightColMap) @@ -2884,6 +2874,29 @@ func (b *Builder) buildZigzagJoin(join *memo.ZigzagJoinExpr) (execPlan, error) { return b.applySimpleProject(res, join, join.Cols, join.ProvidedPhysical().Ordering) } +func (b *Builder) buildLocking(locking opt.Locking) (opt.Locking, error) { + if b.forceForUpdateLocking { + locking = locking.Max(forUpdateLocking) + } + if locking.IsLocking() { + // Raise error if row-level locking is part of a read-only transaction. + if b.evalCtx.TxnReadOnly { + return opt.Locking{}, pgerror.Newf(pgcode.ReadOnlySQLTransaction, + "cannot execute %s in a read-only transaction", locking.Strength.String(), + ) + } + if locking.Durability == tree.LockDurabilityGuaranteed && + b.evalCtx.TxnIsoLevel != isolation.Serializable { + return opt.Locking{}, unimplemented.NewWithIssuef( + 100144, "cannot execute SELECT FOR UPDATE statements under %s isolation", + b.evalCtx.TxnIsoLevel, + ) + } + b.ContainsNonDefaultKeyLocking = true + } + return locking, nil +} + func (b *Builder) buildMax1Row(max1Row *memo.Max1RowExpr) (execPlan, error) { input, err := b.buildRelational(max1Row.Input) if err != nil { diff --git a/pkg/sql/sem/eval/BUILD.bazel b/pkg/sql/sem/eval/BUILD.bazel index 6088b2d022cd..c94b0d4d1c84 100644 --- a/pkg/sql/sem/eval/BUILD.bazel +++ b/pkg/sql/sem/eval/BUILD.bazel @@ -44,6 +44,7 @@ go_library( "//pkg/keys", "//pkg/kv", "//pkg/kv/kvpb", + "//pkg/kv/kvserver/concurrency/isolation", "//pkg/kv/kvserver/kvserverbase", "//pkg/repstream/streampb", "//pkg/roachpb", diff --git a/pkg/sql/sem/eval/context.go b/pkg/sql/sem/eval/context.go index 60fca2cdd3a8..f72025adb269 100644 --- a/pkg/sql/sem/eval/context.go +++ b/pkg/sql/sem/eval/context.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase" "github.com/cockroachdb/cockroach/pkg/repstream/streampb" "github.com/cockroachdb/cockroach/pkg/roachpb" @@ -78,6 +79,8 @@ type Context struct { // TxnIsSingleStmt specifies the current implicit transaction consists of only // a single statement. TxnIsSingleStmt bool + // TxnIsoLevel is the isolation level of the current transaction. + TxnIsoLevel isolation.Level Settings *cluster.Settings // ClusterID is the logical cluster ID for this tenant.