diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_read_committed b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_read_committed new file mode 100644 index 000000000000..d69664f86553 --- /dev/null +++ b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row_read_committed @@ -0,0 +1,118 @@ +# tenant-cluster-setting-override-opt: sql.zone_configs.allow_for_secondary_tenant.enabled=true sql.multi_region.allow_abstractions_for_secondary_tenants.enabled=true +# LogicTest: multiregion-9node-3region-3azs multiregion-9node-3region-3azs-vec-off multiregion-9node-3region-3azs-tenant multiregion-9node-3region-3azs-no-los + +statement ok +SET CLUSTER SETTING sql.txn.read_committed_syntax.enabled = true + +statement ok +SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED + +statement ok +CREATE DATABASE multi_region_test_db PRIMARY REGION "ca-central-1" REGIONS "ap-southeast-2", "us-east-1" SURVIVE REGION FAILURE + +statement ok +USE multi_region_test_db + +# Create a table with a computed region column. + +statement ok +CREATE TABLE university ( + name STRING NOT NULL, + mascot STRING NOT NULL, + postal_code STRING NOT NULL, + region crdb_internal_region NOT NULL AS ( + CASE + WHEN left(postal_code, 2) = '97' THEN 'ca-central-1' -- Oregon + WHEN left(postal_code, 2) = '98' THEN 'ap-southeast-2' -- Washington + ELSE 'us-east-1' -- British Columbia + END + ) STORED, + PRIMARY KEY (name), + UNIQUE INDEX (mascot), + FAMILY (name, mascot, postal_code, region) +) +LOCALITY REGIONAL BY ROW AS region + +# Create a table with a non-computed region column. + +statement ok +CREATE TABLE volcano ( + name STRING NOT NULL, + origin STRING NOT NULL, + location GEOGRAPHY NOT NULL, + region crdb_internal_region NOT NULL, + PRIMARY KEY (name), + UNIQUE INDEX (origin), + INVERTED INDEX (location), + FAMILY (name, origin, location, region) +) +LOCALITY REGIONAL BY ROW AS region + +# TODO(michae2): statement ok +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO university (name, mascot, postal_code) VALUES ('Western Oregon', 'wolves', '97361') + +# TODO(michae2): statement ok +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO volcano +VALUES ('Mount Hood', 'Fought over Loowit and was transformed by Saghalie.', 'POINT(-121.695833 45.373611)', 'ca-central-1') + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "university_mascot_key"\nDETAIL: Key \(mascot\)=\('wolves'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO university (name, mascot, postal_code) VALUES ('Thompson Rivers', 'wolves', 'V2C 0C8') + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "volcano_origin_key"\nDETAIL: Key \(origin\)=\('Fought over Loowit and was transformed by Saghalie.'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO volcano VALUES +('Mount Adams', 'Fought over Loowit and was transformed by Saghalie.', 'POINT(-121.490895 46.202412)', 'ap-southeast-2') + +# TODO(michae2): statement ok +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO university (name, mascot, postal_code) +VALUES ('Thompson Rivers', 'wolves', 'V2C 0C8'), ('Evergreen State', 'geoducks', '98505') +ON CONFLICT (mascot) DO NOTHING + +# TODO(michae2): statement ok +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO volcano VALUES +('Mount Adams', 'Fought over Loowit and was transformed by Saghalie.', 'POINT(-121.490895 46.202412)', 'ap-southeast-2'), +('Mount St. Helens', 'Fair maiden Loowit could not choose between Wyeast and Pahto and was transformed by Saghalie.', 'POINT(-122.1944 46.1912)', 'ap-southeast-2') +ON CONFLICT (origin) DO NOTHING + +query TTT +SELECT name, mascot, postal_code FROM university ORDER BY name +---- + +query TTT +SELECT name, origin, location FROM volcano ORDER BY name +---- + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "university_mascot_key"\nDETAIL: Key \(mascot\)=\('wolves'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +UPSERT INTO university (name, mascot, postal_code) VALUES ('Thompson Rivers', 'wolves', 'V2C 0C8') + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "volcano_origin_key"\nDETAIL: Key \(origin\)=\('Fought over Loowit and was transformed by Saghalie.'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +UPSERT INTO volcano VALUES +('Mount Adams', 'Fought over Loowit and was transformed by Saghalie.', 'POINT(-121.490895 46.202412)', 'ap-southeast-2') + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "university_mascot_key"\nDETAIL: Key \(mascot\)=\('wolves'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +UPDATE university SET mascot = 'wolves' WHERE name = 'Evergreen State' + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "volcano_origin_key"\nDETAIL: Key \(origin\)=\('Fought over Loowit and was transformed by Saghalie.'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +UPDATE volcano SET origin = 'Fought over Loowit and was transformed by Saghalie.' WHERE name = 'Mount St. Helens' + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "university_pkey"\nDETAIL: Key \(name\)=\('Evergreen State'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO university (name, mascot, postal_code) +VALUES ('Thompson Rivers', 'wolves', 'V2C 0C8'), ('Oregon Tech', 'owls', '97601') +ON CONFLICT (mascot) DO UPDATE SET name = 'Evergreen State', mascot = 'banana slugs' + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "volcano_pkey"\nDETAIL: Key \(name\)=\('Mount St. Helens'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO volcano VALUES +('Mount Adams', 'Fought over Loowit and was transformed by Saghalie.', 'POINT(-121.490895 46.202412)', 'ap-southeast-2'), +('Mount Garibaldi', 'Lightning from thunderbird eyes struck the ground.', 'POINT(-123.004722 49.850278)', 'us-east-1') +ON CONFLICT (origin) DO UPDATE SET name = 'Mount St. Helens', origin = 'Discovered by the Vancouver expedition in 1792.', region = 'us-east-1' diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index c1002b805cad..672f5f674b54 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -2312,6 +2312,13 @@ func TestTenantLogic_unique( runLogicTest(t, "unique") } +func TestTenantLogic_unique_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "unique_read_committed") +} + func TestTenantLogic_update( t *testing.T, ) { diff --git a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-no-los/BUILD.bazel b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-no-los/BUILD.bazel index 4da0f10b27c2..0926b52f68a2 100644 --- a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-no-los/BUILD.bazel +++ b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-no-los/BUILD.bazel @@ -15,7 +15,7 @@ go_test( exec_properties = { "Pool": "large", }, - shard_count = 19, + shard_count = 20, tags = [ "ccl_test", "cpu:4", diff --git a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-no-los/generated_test.go b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-no-los/generated_test.go index 926a56b10a04..a4f575f3f1b0 100644 --- a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-no-los/generated_test.go +++ b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-no-los/generated_test.go @@ -197,6 +197,13 @@ func TestCCLLogic_regional_by_row_placement_restricted( runCCLLogicTest(t, "regional_by_row_placement_restricted") } +func TestCCLLogic_regional_by_row_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runCCLLogicTest(t, "regional_by_row_read_committed") +} + func TestCCLLogic_regional_by_row_rename_column( t *testing.T, ) { diff --git a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-tenant/BUILD.bazel b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-tenant/BUILD.bazel index 5e8914d18dea..302de29e4b8b 100644 --- a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-tenant/BUILD.bazel +++ b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-tenant/BUILD.bazel @@ -15,7 +15,7 @@ go_test( exec_properties = { "Pool": "large", }, - shard_count = 15, + shard_count = 16, tags = [ "ccl_test", "cpu:4", diff --git a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-tenant/generated_test.go index 7dd968908959..b3d602d7f12a 100644 --- a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-tenant/generated_test.go @@ -169,6 +169,13 @@ func TestCCLLogic_regional_by_row_placement_restricted( runCCLLogicTest(t, "regional_by_row_placement_restricted") } +func TestCCLLogic_regional_by_row_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runCCLLogicTest(t, "regional_by_row_read_committed") +} + func TestCCLLogic_regional_by_row_rename_column( t *testing.T, ) { diff --git a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-vec-off/BUILD.bazel b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-vec-off/BUILD.bazel index 546a9047317e..bba82a35cc7e 100644 --- a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-vec-off/BUILD.bazel +++ b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-vec-off/BUILD.bazel @@ -15,7 +15,7 @@ go_test( exec_properties = { "Pool": "large", }, - shard_count = 8, + shard_count = 9, tags = [ "ccl_test", "cpu:4", diff --git a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-vec-off/generated_test.go b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-vec-off/generated_test.go index 5efa44c2fafd..366feee839d6 100644 --- a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-vec-off/generated_test.go +++ b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs-vec-off/generated_test.go @@ -120,6 +120,13 @@ func TestCCLLogic_regional_by_row_placement_restricted( runCCLLogicTest(t, "regional_by_row_placement_restricted") } +func TestCCLLogic_regional_by_row_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runCCLLogicTest(t, "regional_by_row_read_committed") +} + func TestCCLLogic_regional_by_row_rename_column( t *testing.T, ) { diff --git a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs/BUILD.bazel b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs/BUILD.bazel index 975423ef25bf..b3391ddae2f2 100644 --- a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs/BUILD.bazel +++ b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs/BUILD.bazel @@ -15,7 +15,7 @@ go_test( exec_properties = { "Pool": "large", }, - shard_count = 26, + shard_count = 27, tags = [ "ccl_test", "cpu:4", diff --git a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs/generated_test.go b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs/generated_test.go index dd0f3ae0af10..0d5ace49e455 100644 --- a/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs/generated_test.go +++ b/pkg/ccl/logictestccl/tests/multiregion-9node-3region-3azs/generated_test.go @@ -246,6 +246,13 @@ func TestCCLLogic_regional_by_row_query_behavior( runCCLLogicTest(t, "regional_by_row_query_behavior") } +func TestCCLLogic_regional_by_row_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runCCLLogicTest(t, "regional_by_row_read_committed") +} + func TestCCLLogic_regional_by_row_rename_column( t *testing.T, ) { diff --git a/pkg/sql/logictest/testdata/logic_test/unique_read_committed b/pkg/sql/logictest/testdata/logic_test/unique_read_committed new file mode 100644 index 000000000000..86c9c35a44f3 --- /dev/null +++ b/pkg/sql/logictest/testdata/logic_test/unique_read_committed @@ -0,0 +1,108 @@ +# LogicTest: !local-mixed-22.2-23.1 + +statement ok +SET CLUSTER SETTING sql.txn.read_committed_syntax.enabled = true + +statement ok +SET experimental_enable_unique_without_index_constraints = true + +# Test UNIQUE WITHOUT INDEX with an enum PK. Under read committed isolation this +# should work, using single-key predicate locks. + +statement ok +CREATE TYPE region AS ENUM ('adriatic', 'aegean', 'black', 'caspian', 'mediterranean', 'persian', 'red') + +statement ok +CREATE TABLE voyage ( + sea region NOT NULL DEFAULT 'aegean', + hero STRING NOT NULL, + crew STRING NULL, + quest STRING NOT NULL, + PRIMARY KEY (sea, hero), + UNIQUE INDEX (sea, quest, crew), + UNIQUE WITHOUT INDEX (hero), + UNIQUE WITHOUT INDEX (quest, crew), + FAMILY (sea, hero, crew, quest) +) + +statement ok +SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED + +# TODO(michae2): statement ok +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO voyage VALUES ('caspian', 'hercules', 'argonauts', 'golden fleece') + +# The Argonauts searching for the golden fleece should fail the (quest, crew) +# uniqueness check, even with a different sea. +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "unique_quest_crew"\nDETAIL: Key \(crew, quest\)=\('argonauts', 'golden fleece'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO voyage +VALUES (DEFAULT, 'odysseus', 'nobody', 'penelope'), ('black', 'jason', 'argonauts', 'golden fleece') + +# Only Odysseus should be inserted. +# TODO(michae2): statement ok +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO voyage +VALUES ('mediterranean', 'odysseus', 'nobody', 'penelope'), ('black', 'jason', 'argonauts', 'golden fleece') +ON CONFLICT (quest, crew) DO NOTHING + +query TTTT +SELECT * FROM voyage ORDER BY hero, crew, quest +---- + +# Hercules should fail the (hero) uniqueness check, even with a different sea. +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "unique_hero"\nDETAIL: Key \(hero\)=\('hercules'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO voyage (hero, quest) VALUES ('perseus', 'medusa'), ('hercules', 'geryon') + +# Only Perseus should be inserted. +# TODO(michae2): statement ok +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO voyage (hero, quest) VALUES ('perseus', 'medusa'), ('hercules', 'geryon') +ON CONFLICT (hero) DO NOTHING + +query TTTT +SELECT * FROM voyage ORDER BY hero, crew, quest +---- + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "unique_quest_crew"\nDETAIL: Key \(crew, quest\)=\('argonauts', 'golden fleece'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +UPSERT INTO voyage VALUES ('black', 'jason', 'argonauts', 'golden fleece') + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "unique_hero"\nDETAIL: Key \(hero\)=\('hercules'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +UPSERT INTO voyage (hero, quest) VALUES ('hercules', 'geryon') + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "unique_quest_crew"\nDETAIL: Key \(crew, quest\)=\('argonauts', 'golden fleece'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +UPDATE voyage SET crew = 'argonauts', quest = 'golden fleece' WHERE hero = 'perseus' + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "unique_hero"\nDETAIL: Key \(hero\)=\('hercules'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +UPDATE voyage SET hero = 'hercules' WHERE hero = 'odysseus' + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "unique_quest_crew"\nDETAIL: Key \(crew, quest\)=\('nobody', 'penelope'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO voyage VALUES ('black', 'jason', 'argonauts', 'golden fleece') +ON CONFLICT (quest, crew) DO UPDATE SET quest = 'penelope', crew = 'nobody' + +# TODO(michae2): statement error pgcode 23505 pq: duplicate key value violates unique constraint "unique_hero"\nDETAIL: Key \(hero\)=\('perseus'\) already exists. +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO voyage (hero, quest) VALUES ('hercules', 'geryon') +ON CONFLICT (hero) DO UPDATE SET hero = 'perseus' + +# Test UNIQUE WITHOUT INDEX with a non-enum PK. Under read committed isolation +# this will not work until predicate locks are supported on multi-key scans. + +statement ok +CREATE TABLE titan ( + name STRING NOT NULL, + domain STRING NOT NULL, + children STRING[], + PRIMARY KEY (name), + UNIQUE WITHOUT INDEX (domain), + FAMILY (name, domain, children) +) + +statement error pgcode 0A000 guaranteed-durable locking not yet implemented +INSERT INTO titan VALUES ('cronus', 'time', ARRAY['zeus', 'hera', 'hades', 'poseidon', 'demeter', 'hestia']) diff --git a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go index ab9fe772d083..18c77c64334e 100644 --- a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go @@ -2276,6 +2276,13 @@ func TestLogic_unique( runLogicTest(t, "unique") } +func TestLogic_unique_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "unique_read_committed") +} + func TestLogic_update( 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 ddeb970b1c69..19a45c67f706 100644 --- a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go @@ -2283,6 +2283,13 @@ func TestLogic_unique( runLogicTest(t, "unique") } +func TestLogic_unique_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "unique_read_committed") +} + func TestLogic_update( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist/generated_test.go b/pkg/sql/logictest/tests/fakedist/generated_test.go index 485f13541301..dc141136e173 100644 --- a/pkg/sql/logictest/tests/fakedist/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist/generated_test.go @@ -2297,6 +2297,13 @@ func TestLogic_unique( runLogicTest(t, "unique") } +func TestLogic_unique_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "unique_read_committed") +} + func TestLogic_update( 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 5d9447d5ba1f..b96a9cb5f9de 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 @@ -2276,6 +2276,13 @@ func TestLogic_unique( runLogicTest(t, "unique") } +func TestLogic_unique_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "unique_read_committed") +} + func TestLogic_update( 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 e51ec03b145f..a2b689de5ac3 100644 --- a/pkg/sql/logictest/tests/local-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/local-vec-off/generated_test.go @@ -2304,6 +2304,13 @@ func TestLogic_unique( runLogicTest(t, "unique") } +func TestLogic_unique_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "unique_read_committed") +} + func TestLogic_update( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local/generated_test.go b/pkg/sql/logictest/tests/local/generated_test.go index 37eb48917b3d..65a9a2283bea 100644 --- a/pkg/sql/logictest/tests/local/generated_test.go +++ b/pkg/sql/logictest/tests/local/generated_test.go @@ -2500,6 +2500,13 @@ func TestLogic_unique( runLogicTest(t, "unique") } +func TestLogic_unique_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "unique_read_committed") +} + func TestLogic_update( t *testing.T, ) { diff --git a/pkg/sql/opt/exec/execbuilder/testdata/explain b/pkg/sql/opt/exec/execbuilder/testdata/explain index 01f6559d6afb..55da80f69ec4 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/explain +++ b/pkg/sql/opt/exec/execbuilder/testdata/explain @@ -2241,7 +2241,7 @@ quality of service: regular query T EXPLAIN (OPT, MEMO) SELECT * FROM tc JOIN t ON k=a ---- -memo (optimized, ~17KB, required=[presentation: info:10] [distribution: test]) +memo (optimized, ~18KB, required=[presentation: info:10] [distribution: test]) ├── G1: (explain G2 [presentation: a:1,b:2,k:6,v:7] [distribution: test]) │ └── [presentation: info:10] [distribution: test] │ ├── best: (explain G2="[presentation: a:1,b:2,k:6,v:7] [distribution: test]" [presentation: a:1,b:2,k:6,v:7] [distribution: test]) @@ -2318,7 +2318,7 @@ TABLE t ├── tableoid oid [hidden] [system] └── PRIMARY INDEX t_pkey └── k int not null -memo (optimized, ~17KB, required=[presentation: info:10] [distribution: test]) +memo (optimized, ~18KB, required=[presentation: info:10] [distribution: test]) ├── G1: (explain G2 [presentation: a:1,b:2,k:6,v:7] [distribution: test]) │ └── [presentation: info:10] [distribution: test] │ ├── best: (explain G2="[presentation: a:1,b:2,k:6,v:7] [distribution: test]" [presentation: a:1,b:2,k:6,v:7] [distribution: test]) diff --git a/pkg/sql/opt/exec/execbuilder/testdata/unique_read_committed b/pkg/sql/opt/exec/execbuilder/testdata/unique_read_committed new file mode 100644 index 000000000000..5de9f8ab983b --- /dev/null +++ b/pkg/sql/opt/exec/execbuilder/testdata/unique_read_committed @@ -0,0 +1,203 @@ +# LogicTest: local + +statement ok +SET CLUSTER SETTING sql.txn.read_committed_syntax.enabled = true + +statement ok +SET experimental_enable_unique_without_index_constraints = true + +# Test UNIQUE WITHOUT INDEX with an enum PK. Under read committed isolation this +# should work, using single-key predicate locks. + +statement ok +CREATE TYPE region AS ENUM ('us-east', 'us-west', 'eu-west') + +statement ok +CREATE TABLE uniq_enum ( + r region DEFAULT CASE (random()*3)::int WHEN 0 THEN 'us-east' WHEN 1 THEN 'us-west' ELSE 'eu-west' END, + s STRING, + i INT, + j INT DEFAULT NULL, + PRIMARY KEY (r, i), + UNIQUE INDEX (r, s, j), + UNIQUE WITHOUT INDEX (i), + UNIQUE WITHOUT INDEX (s, j), + FAMILY (r, s, i, j) +) + +statement ok +SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED + +query T +EXPLAIN (OPT) INSERT INTO uniq_enum VALUES ('us-west', 'foo', 1, 1), ('us-east', 'bar', 2, 2) +---- +insert uniq_enum + ├── project + │ ├── values + │ │ ├── ('us-west', 'foo', 1, 1) + │ │ └── ('us-east', 'bar', 2, 2) + │ └── projections + │ └── column1 IN ('us-east', 'us-west', 'eu-west') + └── unique-checks + ├── unique-checks-item: uniq_enum(i) + │ └── project + │ └── semi-join (lookup uniq_enum) + │ ├── flags: prefer lookup join (into right side) + │ ├── locking: for-share,predicate,durability-guaranteed + │ ├── with-scan &1 + │ └── filters + │ └── r != uniq_enum.r + └── unique-checks-item: uniq_enum(s,j) + └── project + └── semi-join (lookup uniq_enum@uniq_enum_r_s_j_key) + ├── flags: prefer lookup join (into right side) + ├── locking: for-share,predicate,durability-guaranteed + ├── with-scan &1 + └── filters + └── (r != uniq_enum.r) OR (i != uniq_enum.i) + +query T +EXPLAIN (OPT) INSERT INTO uniq_enum (s, i) VALUES ('foo', 1), ('bar', 2) +---- +insert uniq_enum + ├── project + │ ├── project + │ │ ├── values + │ │ │ ├── ('foo', 1) + │ │ │ └── ('bar', 2) + │ │ └── projections + │ │ ├── CASE (random() * 3.0)::INT8 WHEN 0 THEN 'us-east' WHEN 1 THEN 'us-west' ELSE 'eu-west' END + │ │ └── CAST(NULL AS INT8) + │ └── projections + │ └── r_default IN ('us-east', 'us-west', 'eu-west') + └── unique-checks + └── unique-checks-item: uniq_enum(i) + └── project + └── semi-join (lookup uniq_enum) + ├── flags: prefer lookup join (into right side) + ├── locking: for-share,predicate,durability-guaranteed + ├── with-scan &1 + └── filters + └── r != uniq_enum.r + +query T +EXPLAIN (OPT) INSERT INTO uniq_enum VALUES ('us-west', 'foo', 1, 1), ('us-east', 'bar', 2, 2) +ON CONFLICT DO NOTHING +---- +insert uniq_enum + ├── arbiter constraints: unique_i unique_s_j + └── project + ├── anti-join (lookup uniq_enum@uniq_enum_r_s_j_key) + │ ├── flags: prefer lookup join (into right side) + │ ├── lookup columns are key + │ ├── locking: for-share,predicate,durability-guaranteed + │ ├── anti-join (lookup uniq_enum) + │ │ ├── flags: prefer lookup join (into right side) + │ │ ├── lookup columns are key + │ │ ├── locking: for-share,predicate,durability-guaranteed + │ │ ├── values + │ │ │ ├── ('us-west', 'foo', 1, 1) + │ │ │ └── ('us-east', 'bar', 2, 2) + │ │ └── filters (true) + │ └── filters (true) + └── projections + └── column1 IN ('us-east', 'us-west', 'eu-west') + +query T +EXPLAIN (OPT) UPDATE uniq_enum SET r = DEFAULT, s = 'baz', i = 3 WHERE r = 'eu-west' AND i > 10 AND i <= 20 +---- +update uniq_enum + ├── project + │ ├── project + │ │ ├── scan uniq_enum + │ │ │ └── constraint: /7/9: [/'eu-west'/11 - /'eu-west'/20] + │ │ └── projections + │ │ ├── CASE (random() * 3.0)::INT8 WHEN 0 THEN 'us-east' WHEN 1 THEN 'us-west' ELSE 'eu-west' END + │ │ ├── 'baz' + │ │ └── 3 + │ └── projections + │ └── r_new IN ('us-east', 'us-west', 'eu-west') + └── unique-checks + ├── unique-checks-item: uniq_enum(i) + │ └── project + │ └── semi-join (lookup uniq_enum) + │ ├── flags: prefer lookup join (into right side) + │ ├── locking: for-share,predicate,durability-guaranteed + │ ├── with-scan &1 + │ └── filters + │ └── r != uniq_enum.r + └── unique-checks-item: uniq_enum(s,j) + └── project + └── semi-join (lookup uniq_enum@uniq_enum_r_s_j_key) + ├── flags: prefer lookup join (into right side) + ├── locking: for-share,predicate,durability-guaranteed + ├── with-scan &1 + └── filters + └── (r != uniq_enum.r) OR (i != uniq_enum.i) + +query T +EXPLAIN (OPT) UPSERT INTO uniq_enum VALUES ('us-west', 'foo', 1, 1), ('us-east', 'bar', 2, 2) +---- +upsert uniq_enum + ├── arbiter indexes: uniq_enum_pkey + ├── project + │ ├── project + │ │ ├── left-join (lookup uniq_enum) + │ │ │ ├── lookup columns are key + │ │ │ ├── values + │ │ │ │ ├── ('us-west', 'foo', 1, 1) + │ │ │ │ └── ('us-east', 'bar', 2, 2) + │ │ │ └── filters (true) + │ │ └── projections + │ │ ├── CASE WHEN uniq_enum.r IS NULL THEN column1 ELSE uniq_enum.r END + │ │ └── CASE WHEN uniq_enum.r IS NULL THEN column3 ELSE uniq_enum.i END + │ └── projections + │ └── upsert_r IN ('us-east', 'us-west', 'eu-west') + └── unique-checks + ├── unique-checks-item: uniq_enum(i) + │ └── project + │ └── semi-join (lookup uniq_enum) + │ ├── flags: prefer lookup join (into right side) + │ ├── locking: for-share,predicate,durability-guaranteed + │ ├── with-scan &1 + │ └── filters + │ └── r != uniq_enum.r + └── unique-checks-item: uniq_enum(s,j) + └── project + └── semi-join (lookup uniq_enum@uniq_enum_r_s_j_key) + ├── flags: prefer lookup join (into right side) + ├── locking: for-share,predicate,durability-guaranteed + ├── with-scan &1 + └── filters + └── (r != uniq_enum.r) OR (i != uniq_enum.i) + +query T +EXPLAIN (OPT) INSERT INTO uniq_enum VALUES ('us-west', 'foo', 1, 1), ('us-east', 'bar', 2, 2) +ON CONFLICT (s, j) DO UPDATE SET i = 3 +---- +upsert uniq_enum + ├── arbiter constraints: unique_s_j + ├── project + │ ├── project + │ │ ├── left-join (lookup uniq_enum@uniq_enum_r_s_j_key) + │ │ │ ├── flags: prefer lookup join (into right side) + │ │ │ ├── lookup columns are key + │ │ │ ├── locking: for-update,predicate,durability-guaranteed + │ │ │ ├── values + │ │ │ │ ├── ('us-west', 'foo', 1, 1) + │ │ │ │ └── ('us-east', 'bar', 2, 2) + │ │ │ └── filters (true) + │ │ └── projections + │ │ ├── CASE WHEN uniq_enum.r IS NULL THEN column1 ELSE uniq_enum.r END + │ │ └── CASE WHEN uniq_enum.r IS NULL THEN column3 ELSE 3 END + │ └── projections + │ └── upsert_r IN ('us-east', 'us-west', 'eu-west') + └── unique-checks + └── unique-checks-item: uniq_enum(i) + └── project + └── semi-join (lookup uniq_enum) + ├── flags: prefer lookup join (into right side) + ├── locking: for-share,predicate,durability-guaranteed + ├── with-scan &1 + └── filters + └── r != uniq_enum.r diff --git a/pkg/sql/opt/exec/execbuilder/tests/local/generated_test.go b/pkg/sql/opt/exec/execbuilder/tests/local/generated_test.go index 045a1344f52f..d5f878b822ef 100644 --- a/pkg/sql/opt/exec/execbuilder/tests/local/generated_test.go +++ b/pkg/sql/opt/exec/execbuilder/tests/local/generated_test.go @@ -643,6 +643,13 @@ func TestExecBuild_unique( runExecBuildLogicTest(t, "unique") } +func TestExecBuild_unique_read_committed( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runExecBuildLogicTest(t, "unique_read_committed") +} + func TestExecBuild_update( t *testing.T, ) { diff --git a/pkg/sql/opt/exec/explain/emit.go b/pkg/sql/opt/exec/explain/emit.go index b24520ed5194..bd3134cf12e3 100644 --- a/pkg/sql/opt/exec/explain/emit.go +++ b/pkg/sql/opt/exec/explain/emit.go @@ -1074,6 +1074,7 @@ func (e *emitter) emitLockingPolicy(locking opt.Locking) { func (e *emitter) emitLockingPolicyWithPrefix(keyPrefix string, locking opt.Locking) { strength := descpb.ToScanLockingStrength(locking.Strength) waitPolicy := descpb.ToScanLockingWaitPolicy(locking.WaitPolicy) + form := locking.Form durability := locking.Durability if strength != descpb.ScanLockingStrength_FOR_NONE { e.ob.Attr(keyPrefix+"locking strength", strength.PrettyString()) @@ -1081,6 +1082,9 @@ func (e *emitter) emitLockingPolicyWithPrefix(keyPrefix string, locking opt.Lock if waitPolicy != descpb.ScanLockingWaitPolicy_BLOCK { e.ob.Attr(keyPrefix+"locking wait policy", waitPolicy.PrettyString()) } + if form != tree.LockRecord { + e.ob.Attr(keyPrefix+"locking form", form.String()) + } if durability != tree.LockDurabilityBestEffort { e.ob.Attr(keyPrefix+"locking durability", durability.String()) } diff --git a/pkg/sql/opt/locking.go b/pkg/sql/opt/locking.go index 8c6ee41e6f69..9b12a5b3e315 100644 --- a/pkg/sql/opt/locking.go +++ b/pkg/sql/opt/locking.go @@ -13,7 +13,7 @@ package opt import "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" // Locking represents the row-level locking properties of a relational operator. -// Each relational operator clause consists of three different row-level locking +// Each relational operator clause consists of four different row-level locking // properties. type Locking struct { // The first property is locking strength (see tree.LockingStrength). Locking @@ -44,14 +44,28 @@ type Locking struct { // WaitPolicy tree.LockingWaitPolicy - // The third property is the durability of the locking. A guaranteed-durable - // lock always persists until commit time, while a best-effort lock may - // sometimes be lost before commit (for example, during a lease transfer). We - // currently only require guaranteed-durable locks for SELECT FOR UPDATE - // statements and system-maintained constraint checks (e.g. FK checks) under - // SNAPSHOT and READ COMMITTED isolation. Other locking statements, such as - // UPDATE, rely on the durability of intents for correctness, rather than the - // durability of locks. + // The third property is the form of locking, either record locking or + // predicate locking (see tree.LockingForm). Record locking prevents + // modification of existing rows, but does not prevent insertion of new + // rows. Predicate locking prevents both modification of existing rows and + // insertion of new rows. Unlike locking strength, locking form is optional to + // specify in a locking clause. If not specified, the form defaults to record + // locking. We currently only use predicate locking for uniqueness checks + // under snapshot and read committed isolation, and only support predicate + // locking on single-key spans. + Form tree.LockingForm + + // The fourth property is the durability of the locking (see + // tree.LockingDurability). A guaranteed-durable lock always persists until + // commit time, while a best-effort lock may sometimes be lost before commit + // (for example, during a lease transfer). Unlike locking strength, locking + // durability is optional to specify in a locking clause. If not specified, + // the durability defaults to best-effort. We currently only require + // guaranteed-durable locks for SELECT FOR UPDATE statements and + // system-maintained constraint checks (e.g. FK checks) under snapshot and + // read commited isolation. Other locking statements, such as UPDATE, rely on + // the durability of intents for correctness, rather than the durability of + // locks. Durability tree.LockingDurability } @@ -61,6 +75,7 @@ func (l Locking) Max(l2 Locking) Locking { return Locking{ Strength: l.Strength.Max(l2.Strength), WaitPolicy: l.WaitPolicy.Max(l2.WaitPolicy), + Form: l.Form.Max(l2.Form), Durability: l.Durability.Max(l2.Durability), } } diff --git a/pkg/sql/opt/memo/expr_format.go b/pkg/sql/opt/memo/expr_format.go index ff14feeaa2f5..1cf4b6030dbd 100644 --- a/pkg/sql/opt/memo/expr_format.go +++ b/pkg/sql/opt/memo/expr_format.go @@ -1581,6 +1581,14 @@ func (f *ExprFmtCtx) formatLockingWithPrefix( default: panic(errors.AssertionFailedf("unexpected wait policy")) } + form := "" + switch locking.Form { + case tree.LockRecord: + case tree.LockPredicate: + form = ",predicate" + default: + panic(errors.AssertionFailedf("unexpected form")) + } durability := "" switch locking.Durability { case tree.LockDurabilityBestEffort: @@ -1589,7 +1597,7 @@ func (f *ExprFmtCtx) formatLockingWithPrefix( default: panic(errors.AssertionFailedf("unexpected durability")) } - tp.Childf("%slocking: %s%s%s", labelPrefix, strength, wait, durability) + tp.Childf("%slocking: %s%s%s%s", labelPrefix, strength, wait, form, durability) } // formatDependencies adds a new treeprinter child for schema dependencies. diff --git a/pkg/sql/opt/optbuilder/arbiter_set.go b/pkg/sql/opt/optbuilder/arbiter_set.go index 8c3ffd40da76..836093ffa620 100644 --- a/pkg/sql/opt/optbuilder/arbiter_set.go +++ b/pkg/sql/opt/optbuilder/arbiter_set.go @@ -105,8 +105,12 @@ func (a *arbiterSet) ContainsUniqueConstraint(uniq cat.UniqueOrdinal) bool { // pred is nil. // - canaryOrd is the table column ordinal of a not-null column in the // constraint's table. +// - uniqueWithoutIndex is true if this is a unique constraint enforced +// without an index. func (a *arbiterSet) ForEach( - f func(name string, conflictOrds intsets.Fast, pred tree.Expr, canaryOrd int), + f func( + name string, conflictOrds intsets.Fast, pred tree.Expr, canaryOrd int, uniqueWithoutIndex bool, + ), ) { // Call the callback for each index arbiter. a.indexes.ForEach(func(i int) { @@ -120,7 +124,7 @@ func (a *arbiterSet) ForEach( pred = a.mb.parsePartialIndexPredicateExpr(i) } - f(string(index.Name()), conflictOrds, pred, canaryOrd) + f(string(index.Name()), conflictOrds, pred, canaryOrd, false) }) // Call the callback for each unique constraint arbiter. @@ -135,7 +139,7 @@ func (a *arbiterSet) ForEach( pred = a.mb.parseUniqueConstraintPredicateExpr(i) } - f(uniqueConstraint.Name(), conflictOrds, pred, canaryOrd) + f(uniqueConstraint.Name(), conflictOrds, pred, canaryOrd, uniqueConstraint.WithoutIndex()) }) } diff --git a/pkg/sql/opt/optbuilder/insert.go b/pkg/sql/opt/optbuilder/insert.go index 0385d59c2282..ec2cc9fddc56 100644 --- a/pkg/sql/opt/optbuilder/insert.go +++ b/pkg/sql/opt/optbuilder/insert.go @@ -711,14 +711,18 @@ func (mb *mutationBuilder) buildInputForDoNothing(inScope *scope, onConflict *tr mb.outScope.ordering = nil // Create an anti-join for each arbiter. - mb.arbiters.ForEach(func(name string, conflictOrds intsets.Fast, pred tree.Expr, canaryOrd int) { - mb.buildAntiJoinForDoNothingArbiter(inScope, conflictOrds, pred) + mb.arbiters.ForEach(func( + name string, conflictOrds intsets.Fast, pred tree.Expr, canaryOrd int, uniqueWithoutIndex bool, + ) { + mb.buildAntiJoinForDoNothingArbiter(inScope, conflictOrds, pred, uniqueWithoutIndex) }) // Create an UpsertDistinctOn for each arbiter. This must happen after all // conflicting rows are removed with the anti-joins created above, to avoid // removing valid rows (see #59125). - mb.arbiters.ForEach(func(name string, conflictOrds intsets.Fast, pred tree.Expr, canaryOrd int) { + mb.arbiters.ForEach(func( + name string, conflictOrds intsets.Fast, pred tree.Expr, canaryOrd int, uniqueWithoutIndex bool, + ) { // If the arbiter has a partial predicate, project a new column that // allows the UpsertDistinctOn to only de-duplicate insert rows that // satisfy the predicate. See projectPartialArbiterDistinctColumn for @@ -764,7 +768,9 @@ func (mb *mutationBuilder) buildInputForUpsert( // Create an UpsertDistinctOn and a left-join for the single arbiter. var canaryCol *scopeColumn - mb.arbiters.ForEach(func(name string, conflictOrds intsets.Fast, pred tree.Expr, canaryOrd int) { + mb.arbiters.ForEach(func( + name string, conflictOrds intsets.Fast, pred tree.Expr, canaryOrd int, uniqueWithoutIndex bool, + ) { // If the arbiter has a partial predicate, project a new column that // allows the UpsertDistinctOn to only de-duplicate insert rows that // satisfy the predicate. See projectPartialArbiterDistinctColumn for @@ -794,7 +800,7 @@ func (mb *mutationBuilder) buildInputForUpsert( // Create a left-join for the arbiter. mb.buildLeftJoinForUpsertArbiter( - inScope, conflictOrds, pred, + inScope, conflictOrds, pred, uniqueWithoutIndex, ) // Record a not-null "canary" column. After the left-join, this will be diff --git a/pkg/sql/opt/optbuilder/locking.go b/pkg/sql/opt/optbuilder/locking.go index 8c56336a03b7..f4234b99be0a 100644 --- a/pkg/sql/opt/optbuilder/locking.go +++ b/pkg/sql/opt/optbuilder/locking.go @@ -78,6 +78,7 @@ func (lm lockingSpec) get() opt.Locking { return opt.Locking{ Strength: spec.Strength, WaitPolicy: spec.WaitPolicy, + Form: spec.Form, } } return opt.Locking{} @@ -134,6 +135,8 @@ func (lm lockingSpec) filter(alias tree.Name) lockingSpec { // > any of the clauses affecting it. Otherwise, it is processed as SKIP // > LOCKED if that is specified in any of the clauses affecting it. ret[0].WaitPolicy = ret[0].WaitPolicy.Max(li.WaitPolicy) + // We assume the same behavior for locking form. + ret[0].Form = ret[0].Form.Max(li.Form) } for i, li := range lm { diff --git a/pkg/sql/opt/optbuilder/mutation_builder_arbiter.go b/pkg/sql/opt/optbuilder/mutation_builder_arbiter.go index 88ac7980c065..845da03ecd49 100644 --- a/pkg/sql/opt/optbuilder/mutation_builder_arbiter.go +++ b/pkg/sql/opt/optbuilder/mutation_builder_arbiter.go @@ -13,6 +13,7 @@ package optbuilder import ( "fmt" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/cat" @@ -277,8 +278,32 @@ func (mb *mutationBuilder) inferArbitersFromConflictOrds( // - pred is the partial index or constraint predicate. If the arbiter is // not a partial index or constraint, pred is nil. func (mb *mutationBuilder) buildAntiJoinForDoNothingArbiter( - inScope *scope, conflictOrds intsets.Fast, pred tree.Expr, + inScope *scope, conflictOrds intsets.Fast, pred tree.Expr, uniqueWithoutIndex bool, ) { + locking := noRowLocking + // If we're using a weaker isolation level, we must lock the right side of the + // anti-join to prevent concurrent inserts from other transactions from + // violating the unique constraint. This is only necessary when there is no + // index directly enforcing the unique constraint. (With an index, concurrent + // transactions will always conflict on the same KV key.) + if mb.b.evalCtx.TxnIsoLevel != isolation.Serializable && uniqueWithoutIndex { + locking = lockingSpec{ + &tree.LockingItem{ + // TODO(michae2): Change this to ForKeyShare when it is supported. + // Actually, for INSERT ON CONFLICT DO NOTHING, I think this could be + // ForNone if we supported predicate locking at that strength. I'm + // pretty sure we don't need to lock existing rows at *any* locking + // strength, only need to prevent insertion of new rows. + Strength: tree.ForShare, + Targets: []tree.TableName{tree.MakeUnqualifiedTableName(mb.tab.Name())}, + WaitPolicy: tree.LockWaitBlock, + // Unique arbiters must ensure the non-existence of certain rows, so we + // use predicate locks instead of record locks to prevent insertion of + // new rows into the locked span(s) by other concurrent transactions. + Form: tree.LockPredicate, + }, + } + } // Build the right side of the anti-join. Use a new metadata instance // of the mutation table so that a different set of column IDs are used for // the two tables in the self-join. @@ -290,7 +315,7 @@ func (mb *mutationBuilder) buildAntiJoinForDoNothingArbiter( includeInverted: false, }), nil, /* indexFlags */ - noRowLocking, + locking, inScope, true, /* disableNotVisibleIndex */ ) @@ -337,12 +362,22 @@ func (mb *mutationBuilder) buildAntiJoinForDoNothingArbiter( on = append(on, mb.b.factory.ConstructFiltersItem(predScalar)) } + joinPrivate := memo.EmptyJoinPrivate + // If we're using a weaker isolation level, the anti-joined scan needs to + // obtain predicate locks. We must use a lookup anti-join for predicate locks + // to work. + if mb.b.evalCtx.TxnIsoLevel != isolation.Serializable && uniqueWithoutIndex { + joinPrivate = &memo.JoinPrivate{ + Flags: memo.PreferLookupJoinIntoRight, + } + } + // Construct the anti-join. mb.outScope.expr = mb.b.factory.ConstructAntiJoin( mb.outScope.expr, fetchScope.expr, on, - memo.EmptyJoinPrivate, + joinPrivate, ) } @@ -358,9 +393,31 @@ func (mb *mutationBuilder) buildAntiJoinForDoNothingArbiter( // - partialIndexDistinctCol is a column that allows the UpsertDistinctOn to // only de-duplicate insert rows that satisfy the partial index predicate. // If the arbiter is not a partial index, partialIndexDistinctCol is nil. +// - uniqueWithoutIndex is true if the arbiter is a unique constraint without +// an enforcing index. func (mb *mutationBuilder) buildLeftJoinForUpsertArbiter( - inScope *scope, conflictOrds intsets.Fast, pred tree.Expr, + inScope *scope, conflictOrds intsets.Fast, pred tree.Expr, uniqueWithoutIndex bool, ) { + locking := noRowLocking + // If we're using a weaker isolation level, we must lock the right side of the + // left join to prevent concurrent inserts from other transactions from + // violating the unique constraint. This is only necessary when there is no + // index directly enforcing the unique constraint. (With an index, concurrent + // transactions will always conflict on the same KV key.) + if mb.b.evalCtx.TxnIsoLevel != isolation.Serializable && uniqueWithoutIndex { + locking = lockingSpec{ + &tree.LockingItem{ + // We're about to update the row, so take an exclusive lock. + Strength: tree.ForUpdate, + Targets: []tree.TableName{tree.MakeUnqualifiedTableName(mb.tab.Name())}, + WaitPolicy: tree.LockWaitBlock, + // Unique arbiters must ensure the non-existence of certain rows, so we + // use predicate locks instead of record locks to prevent insertion of + // new rows into the locked span(s) by other concurrent transactions. + Form: tree.LockPredicate, + }, + } + } // Build the right side of the left outer join. Use a different instance of // table metadata so that col IDs do not overlap. // @@ -375,7 +432,7 @@ func (mb *mutationBuilder) buildLeftJoinForUpsertArbiter( includeInverted: false, }), nil, /* indexFlags */ - noRowLocking, + locking, inScope, true, /* disableNotVisibleIndex */ ) @@ -427,12 +484,22 @@ func (mb *mutationBuilder) buildLeftJoinForUpsertArbiter( // mutationBuilder, and which are no longer needed for any other purpose. mb.outScope.appendColumnsFromScope(mb.fetchScope) + joinPrivate := memo.EmptyJoinPrivate + // If we're using a weaker isolation level, the left-joined scan needs to + // obtain predicate locks. We must use a lookup left-join for predicate locks + // to work. + if mb.b.evalCtx.TxnIsoLevel != isolation.Serializable && uniqueWithoutIndex { + joinPrivate = &memo.JoinPrivate{ + Flags: memo.PreferLookupJoinIntoRight, + } + } + // Construct the left join. mb.outScope.expr = mb.b.factory.ConstructLeftJoin( mb.outScope.expr, mb.fetchScope.expr, on, - memo.EmptyJoinPrivate, + joinPrivate, ) } diff --git a/pkg/sql/opt/optbuilder/mutation_builder_fk.go b/pkg/sql/opt/optbuilder/mutation_builder_fk.go index e1d14f8dbfc4..c16f1425bdd9 100644 --- a/pkg/sql/opt/optbuilder/mutation_builder_fk.go +++ b/pkg/sql/opt/optbuilder/mutation_builder_fk.go @@ -641,10 +641,10 @@ func (h *fkCheckHelper) buildOtherTableScan(parent bool) (outScope *scope, tabMe locking := noRowLocking // For insertion-side checks, if enable_implicit_fk_locking_for_serializable // is true or we're using a weaker isolation level, we lock the parent row(s) - // to prevent concurrent mutations of the parent from violating the FK - // constraint. Deletion-side checks don't need to lock because they can rely - // on the deletion intent conflicting with locks from any concurrent inserts - // or updates of the child. + // to prevent concurrent mutations of the parent from other transactions from + // violating the FK constraint. Deletion-side checks don't need to lock + // because they can rely on the deletion intent conflicting with locks from + // any concurrent inserts or updates of the child. if parent && (h.mb.b.evalCtx.TxnIsoLevel != isolation.Serializable || h.mb.b.evalCtx.SessionData().ImplicitFKLockingForSerializable) { locking = lockingSpec{ diff --git a/pkg/sql/opt/optbuilder/mutation_builder_unique.go b/pkg/sql/opt/optbuilder/mutation_builder_unique.go index e8dc532e3642..a58d4811d684 100644 --- a/pkg/sql/opt/optbuilder/mutation_builder_unique.go +++ b/pkg/sql/opt/optbuilder/mutation_builder_unique.go @@ -11,6 +11,7 @@ package optbuilder import ( + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/sql/opt" @@ -373,7 +374,17 @@ func (h *uniqueCheckHelper) buildInsertionCheck() memo.UniqueChecksItem { } semiJoinFilters = append(semiJoinFilters, f.ConstructFiltersItem(pkFilter)) - semiJoin := f.ConstructSemiJoin(withScanScope.expr, h.scanScope.expr, semiJoinFilters, memo.EmptyJoinPrivate) + joinPrivate := memo.EmptyJoinPrivate + // If we're using a weaker isolation level, the semi-joined scan needs to + // obtain predicate locks. We must use a lookup semi-join for predicate locks + // to work. + if h.mb.b.evalCtx.TxnIsoLevel != isolation.Serializable { + joinPrivate = &memo.JoinPrivate{ + Flags: memo.PreferLookupJoinIntoRight, + } + } + + semiJoin := f.ConstructSemiJoin(withScanScope.expr, h.scanScope.expr, semiJoinFilters, joinPrivate) // Collect the key columns that will be shown in the error message if there // is a duplicate key violation resulting from this uniqueness check. @@ -404,13 +415,31 @@ func (h *uniqueCheckHelper) buildTableScan() (outScope *scope, ordinals []int) { includeSystem: false, includeInverted: false, }) + locking := noRowLocking + // If we're using a weaker isolation level, we lock the checked predicate(s) + // to prevent concurrent inserts from other transactions from violating the + // unique constraint. + if h.mb.b.evalCtx.TxnIsoLevel != isolation.Serializable { + locking = lockingSpec{ + &tree.LockingItem{ + // TODO(michae2): Change this to ForKeyShare when it is supported. + Strength: tree.ForShare, + Targets: []tree.TableName{tree.MakeUnqualifiedTableName(h.mb.tab.Name())}, + WaitPolicy: tree.LockWaitBlock, + // Unique checks must ensure the non-existence of certain rows, so we + // use predicate locks instead of record locks to prevent insertion of + // new rows into the locked span(s) by other concurrent transactions. + Form: tree.LockPredicate, + }, + } + } return h.mb.b.buildScan( tabMeta, ordinals, // After the update we can't guarantee that the constraints are unique // (which is why we need the uniqueness checks in the first place). &tree.IndexFlags{IgnoreUniqueWithoutIndexKeys: true}, - noRowLocking, + locking, h.mb.b.allocScope(), true, /* disableNotVisibleIndex */ ), ordinals diff --git a/pkg/sql/opt/optbuilder/select.go b/pkg/sql/opt/optbuilder/select.go index 70867917946e..7ee9b6338750 100644 --- a/pkg/sql/opt/optbuilder/select.go +++ b/pkg/sql/opt/optbuilder/select.go @@ -1470,14 +1470,18 @@ func (b *Builder) validateLockingInFrom( case tree.ForNone: // AST nodes should not be created with this locking strength. panic(errors.AssertionFailedf("locking item without strength")) - case tree.ForUpdate, tree.ForNoKeyUpdate, tree.ForShare, tree.ForKeyShare: - // CockroachDB treats all of the FOR LOCKED modes as no-ops. Since all - // transactions are serializable in CockroachDB, clients can't observe - // whether or not FOR UPDATE (or any of the other weaker modes) actually - // created a lock. This behavior may improve as the transaction model gains - // more capabilities. + case tree.ForUpdate: + // Exclusive locking on the entire row. + case tree.ForNoKeyUpdate: + // Exclusive locking on only non-key(s) of the row. Currently + // unimplemented and treated identically to ForUpdate. + case tree.ForShare: + // Shared locking on the entire row. + case tree.ForKeyShare: + // Shared locking on only key(s) of the row. Currently unimplemented and + // treated identically to ForShare. default: - panic(errors.AssertionFailedf("unknown locking strength: %s", li.Strength)) + panic(errors.AssertionFailedf("unknown locking strength: %d", li.Strength)) } // Validating locking wait policy. @@ -1489,7 +1493,17 @@ func (b *Builder) validateLockingInFrom( case tree.LockWaitError: // Raise an error on conflicting locks. default: - panic(errors.AssertionFailedf("unknown locking wait policy: %s", li.WaitPolicy)) + panic(errors.AssertionFailedf("unknown locking wait policy: %d", li.WaitPolicy)) + } + + // Validate locking form. + switch li.Form { + case tree.LockRecord: + // Default. Only lock existing rows. + case tree.LockPredicate: + // Lock both existing rows and gaps between rows. + default: + panic(errors.AssertionFailedf("unknown locking form: %d", li.Form)) } // Validate locking targets by checking that all targets are well-formed diff --git a/pkg/sql/opt/xform/testdata/rules/join b/pkg/sql/opt/xform/testdata/rules/join index 3d269029b927..1fea36524452 100644 --- a/pkg/sql/opt/xform/testdata/rules/join +++ b/pkg/sql/opt/xform/testdata/rules/join @@ -227,7 +227,7 @@ inner-join (merge) memo expect=ReorderJoins SELECT * FROM abc, stu, xyz WHERE abc.a=stu.s AND stu.s=xyz.x ---- -memo (optimized, ~44KB, required=[presentation: a:1,b:2,c:3,s:7,t:8,u:9,x:12,y:13,z:14]) +memo (optimized, ~45KB, required=[presentation: a:1,b:2,c:3,s:7,t:8,u:9,x:12,y:13,z:14]) ├── G1: (inner-join G2 G3 G4) (inner-join G3 G2 G4) (inner-join G5 G6 G7) (inner-join G6 G5 G7) (inner-join G8 G9 G7) (inner-join G9 G8 G7) (merge-join G2 G3 G10 inner-join,+1,+7) (merge-join G3 G2 G10 inner-join,+7,+1) (lookup-join G3 G10 abc@ab,keyCols=[7],outCols=(1-3,7-9,12-14)) (merge-join G5 G6 G10 inner-join,+7,+12) (merge-join G6 G5 G10 inner-join,+12,+7) (lookup-join G6 G10 stu,keyCols=[12],outCols=(1-3,7-9,12-14)) (merge-join G8 G9 G10 inner-join,+7,+12) (lookup-join G8 G10 xyz@xy,keyCols=[7],outCols=(1-3,7-9,12-14)) (merge-join G9 G8 G10 inner-join,+12,+7) │ └── [presentation: a:1,b:2,c:3,s:7,t:8,u:9,x:12,y:13,z:14] │ ├── best: (merge-join G5="[ordering: +7]" G6="[ordering: +(1|12)]" G10 inner-join,+7,+12) diff --git a/pkg/sql/sem/tree/select.go b/pkg/sql/sem/tree/select.go index 2ea127bcc232..2413324fe4e2 100644 --- a/pkg/sql/sem/tree/select.go +++ b/pkg/sql/sem/tree/select.go @@ -1100,6 +1100,7 @@ type LockingItem struct { Strength LockingStrength Targets TableNames WaitPolicy LockingWaitPolicy + Form LockingForm } // Format implements the NodeFormatter interface. @@ -1110,6 +1111,7 @@ func (f *LockingItem) Format(ctx *FmtCtx) { ctx.FormatNode(&f.Targets) } ctx.FormatNode(f.WaitPolicy) + // Form and durability are not currently exposed through SQL. } // LockingStrength represents the possible row-level lock modes for a SELECT @@ -1199,11 +1201,53 @@ func (p LockingWaitPolicy) Max(p2 LockingWaitPolicy) LockingWaitPolicy { return LockingWaitPolicy(max(byte(p), byte(p2))) } -// LockingDurability represents the durability of a lock. It is currently not -// exposed through SQL, but is instead set according to statement type and +// LockingForm represents the form of locking to use, record locking or +// predicate locking. It is not currently exposed through SQL, but could be once +// more fully supported. +type LockingForm byte + +// The ordering of the variants is important, because the highest numerical +// value takes precedence when row-level locking is specified multiple ways. +const ( + // LockRecord represents the default: lock existing rows within the specified + // span(s), which prevents modification of those rows but does not prevent + // insertion of new rows (phantoms) into the span(s). + LockRecord LockingForm = iota + // LockPredicate represents locking the logical predicate defined by the + // span(s), preventing modification of existing rows as well as insertion of + // new rows (phantoms). This is similar to the behavior of "next-key locks" in + // InnoDB, "key-range locks" in SQL Server, "phantom locks" in Sybase, etc. + // (Postgres also has predicate locks, which it uses under serializable + // isolation, but these are used to detect serializable violations rather than + // for mutual exclusion.) + // + // We currently only use predicate locks for uniqueness checks under snapshot + // and read committed isolation, and only support predicate locks on + // single-key spans. + LockPredicate +) + +var lockingClassName = [...]string{ + LockRecord: "record", + LockPredicate: "predicate", +} + +func (p LockingForm) String() string { + return lockingClassName[p] +} + +// Max returns the maximum of the two locking forms. +func (p LockingForm) Max(p2 LockingForm) LockingForm { + return LockingForm(max(byte(p), byte(p2))) +} + +// LockingDurability represents the durability of a lock. It is not exposed +// through SQL, but is instead set by the system according to statement type and // isolation level. It is included here for completeness. type LockingDurability byte +// The ordering of the variants is important, because the highest numerical +// value takes precedence when row-level locking is specified multiple ways. const ( // LockDurabilityBestEffort represents the default: make a best-effort attempt // to hold the lock until commit while keeping it unreplicated and in-memory