From b3f4d2bd4d0c740a381439f8b048f9767e0d5025 Mon Sep 17 00:00:00 2001 From: sumeerbhola Date: Thu, 18 Feb 2021 09:12:17 -0500 Subject: [PATCH] storage,server: enable separated intents Separated intents are enabled by default, and the setting is randomized on the testing path for testserver.go. The intent resolution performance, which caused these to be disabled, has been improved due to https://github.com/cockroachdb/cockroach/pull/60622 and https://github.com/cockroachdb/cockroach/pull/60698 TestLogic/aggregate is tweaked to reduce the number of intents created, as explained in the TODO added where the change was made. The fk and upsert tests also use higher kv.transaction.max_intents_bytes. Informs #41720 Release note (ops change): The default value of the storage.transaction.separated_intents.enabled cluster setting is changed to true. --- Makefile | 11 +++++++--- pkg/server/settingswatcher/BUILD.bazel | 1 + .../settings_watcher_external_test.go | 5 +++++ pkg/server/testserver.go | 9 ++++---- .../logictest/testdata/logic_test/aggregate | 22 ++++++++++++++----- pkg/sql/logictest/testdata/logic_test/fk | 6 +++++ pkg/sql/logictest/testdata/logic_test/upsert | 6 +++++ pkg/storage/intent_reader_writer.go | 2 +- 8 files changed, 48 insertions(+), 14 deletions(-) diff --git a/Makefile b/Makefile index 0846f42d882b..26ec5774a208 100644 --- a/Makefile +++ b/Makefile @@ -124,7 +124,7 @@ SUBTESTS := LINTTIMEOUT := 30m ## Test timeout to use for regular tests. -TESTTIMEOUT := 30m +TESTTIMEOUT := 2h ## Test timeout to use for race tests. RACETIMEOUT := 30m @@ -1058,11 +1058,16 @@ bench benchshort: TESTTIMEOUT := $(BENCHTIMEOUT) # that longer running benchmarks can skip themselves. benchshort: override TESTFLAGS += -benchtime=1ns -short -.PHONY: check test testshort testrace testlogic testbaselogic testccllogic testoptlogic bench benchshort +.PHONY: test test: ## Run tests. -check test testshort testrace bench benchshort: +test: + $(xgo) test $(GOTESTFLAGS) $(GOFLAGS) $(GOMODVENDORFLAGS) -tags '$(TAGS)' -ldflags '$(LINKFLAGS)' -run "TestLogic" $(if $(BENCHES),-bench "$(BENCHES)") -cpuprofile ./artifacts/$@_logic_cpu.prof -memprofile ./artifacts/$@_logic_mem.prof -timeout $(TESTTIMEOUT) ./pkg/sql/logictest $(TESTFLAGS) + +.PHONY: check testshort testrace testlogic testbaselogic testccllogic testoptlogic bench benchshort +check testshort testrace bench benchshort: $(xgo) test $(GOTESTFLAGS) $(GOFLAGS) $(GOMODVENDORFLAGS) -tags '$(TAGS)' -ldflags '$(LINKFLAGS)' -run "$(TESTS)" $(if $(BENCHES),-bench "$(BENCHES)") -timeout $(TESTTIMEOUT) $(PKG) $(TESTFLAGS) + .PHONY: stress stressrace stress: ## Run tests under stress. stressrace: ## Run tests under stress with the race detector enabled. diff --git a/pkg/server/settingswatcher/BUILD.bazel b/pkg/server/settingswatcher/BUILD.bazel index 556e8f83d5e6..045134fa2b77 100644 --- a/pkg/server/settingswatcher/BUILD.bazel +++ b/pkg/server/settingswatcher/BUILD.bazel @@ -48,6 +48,7 @@ go_test( "//pkg/settings", "//pkg/settings/cluster", "//pkg/sql", + "//pkg/storage", "//pkg/testutils", "//pkg/testutils/serverutils", "//pkg/testutils/sqlutils", diff --git a/pkg/server/settingswatcher/settings_watcher_external_test.go b/pkg/server/settingswatcher/settings_watcher_external_test.go index 65a903bf1503..9bd804e8764a 100644 --- a/pkg/server/settingswatcher/settings_watcher_external_test.go +++ b/pkg/server/settingswatcher/settings_watcher_external_test.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" + "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" @@ -92,6 +93,10 @@ func TestSettingWatcher(t *testing.T) { s0.ExecutorConfig().(sql.ExecutorConfig).RangeFeedFactory, tc.Stopper()) require.NoError(t, sw.Start(ctx)) + // TestCluster randomizes the value of SeparatedIntentsEnabled, so set it to + // the same as in fakeSettings for the subsequent equality check. + storage.SeparatedIntentsEnabled.Override( + &s0.ClusterSettings().SV, storage.SeparatedIntentsEnabled.Get(&fakeSettings.SV)) require.NoError(t, checkSettingsValuesMatch(s0.ClusterSettings(), fakeSettings)) for k, v := range toSet { tdb.Exec(t, "SET CLUSTER SETTING "+k+" = $1", v[1]) diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go index fd2eb27b04f7..91b99d517e76 100644 --- a/pkg/server/testserver.go +++ b/pkg/server/testserver.go @@ -132,12 +132,11 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config { st := params.Settings if params.Settings == nil { st = cluster.MakeClusterSettings() - // TODO(sumeer): re-introduce this randomization. // enabledSeparated := rand.Intn(2) == 0 - // log.Infof(context.Background(), - // "test Config is randomly setting enabledSeparated: %t", - // enabledSeparated) - // storage.SeparatedIntentsEnabled.Override(&st.SV, enabledSeparated) + enabledSeparated := true + log.Infof(context.Background(), + "test Config is randomly setting enabledSeparated: %t", enabledSeparated) + storage.SeparatedIntentsEnabled.Override(&st.SV, enabledSeparated) } st.ExternalIODir = params.ExternalIODir cfg := makeTestConfig(st) diff --git a/pkg/sql/logictest/testdata/logic_test/aggregate b/pkg/sql/logictest/testdata/logic_test/aggregate index 63c1d4cfe00a..cd0c34f249ad 100644 --- a/pkg/sql/logictest/testdata/logic_test/aggregate +++ b/pkg/sql/logictest/testdata/logic_test/aggregate @@ -1,5 +1,11 @@ subtest other +# Reduce the need for ranged intent resolution, since the test environment +# tends to accumulate lots of un-compacted intent tombstones that can slow it +# down. +statement ok +SET CLUSTER SETTING kv.transaction.max_intents_bytes = '1048576' + statement ok CREATE TABLE kv ( k INT PRIMARY KEY, @@ -892,9 +898,15 @@ CREATE TABLE mnop ( p BIGINT ) +# TODO(sumeer): increase this series back to 2e4 once we have more performance +# improvements to ranged intent resolution. This test is somewhat artificial +# in that it accumulates a large number of deletion markers for intents for +# each key, in the LSM memtable, without them being flushed to ssts. This +# slows down ranged intent resolution. With 1e4 keys, the txn tracks each +# intent individually, and intent resolution is fast. statement ok INSERT INTO mnop (m, n) SELECT i, (1e9 + i/2e4)::float FROM - generate_series(1, 2e4) AS i(i) + generate_series(1, 5e3) AS i(i) statement ok UPDATE mnop SET o = n::decimal, p = (n * 10)::bigint @@ -902,22 +914,22 @@ UPDATE mnop SET o = n::decimal, p = (n * 10)::bigint query RRR SELECT round(variance(n), 2), round(variance(n), 2), round(variance(p)) FROM mnop ---- -0.08 0.08 8 +0.01 0.01 1 query RRR SELECT round(var_pop(n), 2), round(var_pop(n), 2), round(var_pop(p)) FROM mnop ---- -0.08 0.08 8 +0.01 0.01 1 query RRR SELECT round(stddev_samp(n), 2), round(stddev(n), 2), round(stddev_samp(p)) FROM mnop ---- -0.29 0.29 3 +0.07 0.07 1 query RRR SELECT round(stddev_pop(n), 2), round(stddev_pop(n), 2), round(stddev_pop(p)) FROM mnop ---- -0.29 0.29 3 +0.07 0.07 1 query RRR SELECT avg(1::int)::float, avg(2::float)::float, avg(3::decimal)::float diff --git a/pkg/sql/logictest/testdata/logic_test/fk b/pkg/sql/logictest/testdata/logic_test/fk index c4b1797e5653..56209b5ea566 100644 --- a/pkg/sql/logictest/testdata/logic_test/fk +++ b/pkg/sql/logictest/testdata/logic_test/fk @@ -1,5 +1,11 @@ # LogicTest: !3node-tenant(49854) +# Reduce the need for ranged intent resolution, since the test environment +# tends to accumulate lots of un-compacted intent tombstones that can slow it +# down. +statement ok +SET CLUSTER SETTING kv.transaction.max_intents_bytes = "1048576" + statement ok SET CLUSTER SETTING sql.cross_db_fks.enabled = TRUE diff --git a/pkg/sql/logictest/testdata/logic_test/upsert b/pkg/sql/logictest/testdata/logic_test/upsert index 4cb4386fcf55..d8dc13b3cb1b 100644 --- a/pkg/sql/logictest/testdata/logic_test/upsert +++ b/pkg/sql/logictest/testdata/logic_test/upsert @@ -1,5 +1,11 @@ subtest strict +# Reduce the need for ranged intent resolution, since the test environment +# tends to accumulate lots of un-compacted intent tombstones that can slow it +# down. +statement ok +SET CLUSTER SETTING kv.transaction.max_intents_bytes = "1048576" + statement ok CREATE TABLE ex( foo INT PRIMARY KEY, diff --git a/pkg/storage/intent_reader_writer.go b/pkg/storage/intent_reader_writer.go index 92d34f46f7ab..9bd6e8cb72be 100644 --- a/pkg/storage/intent_reader_writer.go +++ b/pkg/storage/intent_reader_writer.go @@ -46,7 +46,7 @@ var SeparatedIntentsEnabled = settings.RegisterBoolSetting( "storage.transaction.separated_intents.enabled", "if enabled, intents will be written to a separate lock table, instead of being "+ "interleaved with MVCC values", - false, + true, ) // This file defines wrappers for Reader and Writer, and functions to do the