From c73f14366b91f45dca136f0027233d1ffaf6a9f5 Mon Sep 17 00:00:00 2001 From: Nick Travers Date: Mon, 27 Feb 2023 16:04:04 -0800 Subject: [PATCH] clusterversion,storage: use range tombstones unconditionally in 23.1 Currently, MVCC range tombstones are only enabled when the `storage.mvcc.range_tombstones.enabled` cluster setting is `true`. Originally, the plan for 23.1 was to remove this cluster setting entirely, which would enable the feature unconditionally. However, it was decided that for 23.1, the setting would remain, along with the utility methods that determine whether or not the feature is enabled. As there is still an expectation that the feature is enabled in 23.1, add an internal cluster version that is used as a feature gate. If the cluster is at least of version `V23_1_MVCCRangeTombstonesUnconditionallyEnabled`, MVCC range tombstones are _unconditionally_ enabled, irrespective of whether the cluster setting is enabled or not. Fix #91147. Release note: None. --- .../settings/settings-for-tenants.txt | 2 +- docs/generated/settings/settings.html | 2 +- pkg/ccl/backupccl/datadriven_test.go | 3 ++- .../in-progress-import-rollback | 2 +- pkg/cli/testdata/declarative-rules/deprules | 2 +- .../declarative-rules/invalid_version | 2 +- pkg/cli/testdata/declarative-rules/oprules | 2 +- pkg/clusterversion/cockroach_versions.go | 13 +++++++++ pkg/sql/gcjob/gcjobnotifier/notifier.go | 2 +- pkg/sql/gcjob_test/gc_job_test.go | 2 +- pkg/sql/importer/import_stmt_test.go | 2 +- pkg/storage/mvcc.go | 27 ++++++++++++------- .../wait_for_del_range_in_gc_job_test.go | 2 +- 13 files changed, 43 insertions(+), 20 deletions(-) diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt index 5024cb438c76..f2f0679df820 100644 --- a/docs/generated/settings/settings-for-tenants.txt +++ b/docs/generated/settings/settings-for-tenants.txt @@ -297,4 +297,4 @@ trace.jaeger.agent string the address of a Jaeger agent to receive traces using trace.opentelemetry.collector string address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as :. If no port is specified, 4317 will be used. trace.span_registry.enabled boolean true if set, ongoing traces can be seen at https:///#/debug/tracez trace.zipkin.collector string the address of a Zipkin instance to receive traces, as :. If no port is specified, 9411 will be used. -version version 1000022.2-56 set the active cluster version in the format '.' +version version 1000022.2-58 set the active cluster version in the format '.' diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index f0ce6b518a95..72681ff886fe 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -238,6 +238,6 @@
trace.opentelemetry.collector
stringaddress of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as <host>:<port>. If no port is specified, 4317 will be used.
trace.span_registry.enabled
booleantrueif set, ongoing traces can be seen at https://<ui>/#/debug/tracez
trace.zipkin.collector
stringthe address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used. -
version
version1000022.2-56set the active cluster version in the format '<major>.<minor>' +
version
version1000022.2-58set the active cluster version in the format '<major>.<minor>' diff --git a/pkg/ccl/backupccl/datadriven_test.go b/pkg/ccl/backupccl/datadriven_test.go index 9e8b466bdf70..ca0f51359315 100644 --- a/pkg/ccl/backupccl/datadriven_test.go +++ b/pkg/ccl/backupccl/datadriven_test.go @@ -81,7 +81,8 @@ var localityCfgs = map[string]roachpb.Locality{ } var clusterVersionKeys = map[string]clusterversion.Key{ - "Start22_2": clusterversion.TODODelete_V22_2Start, + "Start22_2": clusterversion.TODODelete_V22_2Start, + "23_1_MVCCTombstones": clusterversion.V23_1_MVCCRangeTombstonesUnconditionallyEnabled, } type sqlDBKey struct { diff --git a/pkg/ccl/backupccl/testdata/backup-restore/in-progress-import-rollback b/pkg/ccl/backupccl/testdata/backup-restore/in-progress-import-rollback index 5cd89c3e29ab..13b002c26bcb 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/in-progress-import-rollback +++ b/pkg/ccl/backupccl/testdata/backup-restore/in-progress-import-rollback @@ -9,7 +9,7 @@ # disabled to run within tenant as they don't have access to the # storage.mvcc.range_tombstones.enabled cluster setting -new-cluster name=s1 disable-tenant +new-cluster name=s1 beforeVersion=23_1_MVCCTombstones disable-tenant ---- ########### diff --git a/pkg/cli/testdata/declarative-rules/deprules b/pkg/cli/testdata/declarative-rules/deprules index 3d0a36fd586d..54a726c5e79a 100644 --- a/pkg/cli/testdata/declarative-rules/deprules +++ b/pkg/cli/testdata/declarative-rules/deprules @@ -1,6 +1,6 @@ dep ---- -debug declarative-print-rules 1000022.2-56 dep +debug declarative-print-rules 1000022.2-58 dep deprules ---- - name: 'CheckConstraint transitions to ABSENT uphold 2-version invariant: PUBLIC->VALIDATED' diff --git a/pkg/cli/testdata/declarative-rules/invalid_version b/pkg/cli/testdata/declarative-rules/invalid_version index c392a03717ea..01340981d10b 100644 --- a/pkg/cli/testdata/declarative-rules/invalid_version +++ b/pkg/cli/testdata/declarative-rules/invalid_version @@ -3,5 +3,5 @@ invalid_version ---- debug declarative-print-rules 1.1 op unsupported version number, the supported versions are: - 1000022.2-56 + 1000022.2-58 1000022.2 diff --git a/pkg/cli/testdata/declarative-rules/oprules b/pkg/cli/testdata/declarative-rules/oprules index 5ac33e087110..917b0872c1cf 100644 --- a/pkg/cli/testdata/declarative-rules/oprules +++ b/pkg/cli/testdata/declarative-rules/oprules @@ -1,6 +1,6 @@ op ---- -debug declarative-print-rules 1000022.2-56 op +debug declarative-print-rules 1000022.2-58 op rules ---- [] diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index 985502b8bd35..17cf6f4afbd6 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -444,6 +444,15 @@ const ( // backfilled. V23_1DatabaseRoleSettingsRoleIDColumnBackfilled + // V23_1_MVCCRangeTombstonesUnconditionallyEnabled is a version gate at and + // after which Cockroach will always write MVCC Range Tombstones, regardless + // of the value of the storage.mvcc.range_tombstones.enabled cluster setting. + // Prior to this version, it was possible for a cluster to be writing MVCC + // Range Tombstones, but only if the cluster had been opted in manually, under + // a specific set of circumstances (i.e. appropriate 22.2.x version, Cockroach + // Cloud cluster, etc.). + V23_1_MVCCRangeTombstonesUnconditionallyEnabled + // ************************************************* // Step (1): Add new versions here. // Do not add new versions to a patch release. @@ -769,6 +778,10 @@ var rawVersionsSingleton = keyedVersions{ Key: V23_1DatabaseRoleSettingsRoleIDColumnBackfilled, Version: roachpb.Version{Major: 22, Minor: 2, Internal: 56}, }, + { + Key: V23_1_MVCCRangeTombstonesUnconditionallyEnabled, + Version: roachpb.Version{Major: 22, Minor: 2, Internal: 58}, + }, // ************************************************* // Step (2): Add new versions here. diff --git a/pkg/sql/gcjob/gcjobnotifier/notifier.go b/pkg/sql/gcjob/gcjobnotifier/notifier.go index 1e2a5e86f80f..55af1757ef2f 100644 --- a/pkg/sql/gcjob/gcjobnotifier/notifier.go +++ b/pkg/sql/gcjob/gcjobnotifier/notifier.go @@ -156,7 +156,7 @@ func (n *Notifier) run(_ context.Context) { } }) tombstonesEnableChanges := make(chan struct{}, 1) - storage.MVCCRangeTombstonesEnabled.SetOnChange(&n.settings.SV, func(ctx context.Context) { + storage.MVCCRangeTombstonesEnabledInMixedClusters.SetOnChange(&n.settings.SV, func(ctx context.Context) { select { case tombstonesEnableChanges <- struct{}{}: default: diff --git a/pkg/sql/gcjob_test/gc_job_test.go b/pkg/sql/gcjob_test/gc_job_test.go index 070f901b9e48..d5ef64aef6f3 100644 --- a/pkg/sql/gcjob_test/gc_job_test.go +++ b/pkg/sql/gcjob_test/gc_job_test.go @@ -271,7 +271,7 @@ func TestGCJobRetry(t *testing.T) { failed.Store(false) cs := cluster.MakeTestingClusterSettings() gcjob.EmptySpanPollInterval.Override(ctx, &cs.SV, 100*time.Millisecond) - storage.MVCCRangeTombstonesEnabled.Override(ctx, &cs.SV, true) + storage.MVCCRangeTombstonesEnabledInMixedClusters.Override(ctx, &cs.SV, true) params := base.TestServerArgs{Settings: cs} params.Knobs.JobsTestingKnobs = jobs.NewTestingKnobsWithShortIntervals() params.Knobs.Store = &kvserver.StoreTestingKnobs{ diff --git a/pkg/sql/importer/import_stmt_test.go b/pkg/sql/importer/import_stmt_test.go index 68cc09ee25dd..489582ed333e 100644 --- a/pkg/sql/importer/import_stmt_test.go +++ b/pkg/sql/importer/import_stmt_test.go @@ -6262,7 +6262,7 @@ func TestImportPgDumpSchemas(t *testing.T) { baseDir := datapathutils.TestDataPath(t, "pgdump") mkArgs := func() base.TestServerArgs { s := cluster.MakeTestingClusterSettings() - storage.MVCCRangeTombstonesEnabled.Override(ctx, &s.SV, true) + storage.MVCCRangeTombstonesEnabledInMixedClusters.Override(ctx, &s.SV, true) return base.TestServerArgs{ Settings: s, ExternalIODir: baseDir, diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index 76df4a2b279e..b8583820a7ae 100644 --- a/pkg/storage/mvcc.go +++ b/pkg/storage/mvcc.go @@ -22,6 +22,7 @@ import ( "sync" "time" + "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvnemesis/kvnemesisutil" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" @@ -74,8 +75,9 @@ var minWALSyncInterval = settings.RegisterDurationSetting( settings.NonNegativeDurationWithMaximum(1*time.Second), ) -// MVCCRangeTombstonesEnabled enables writing of MVCC range tombstones. -// Currently, this is used for schema GC and import cancellation rollbacks. +// MVCCRangeTombstonesEnabledInMixedClusters enables writing of MVCC range +// tombstones. Currently, this is used for schema GC and import cancellation +// rollbacks. // // Note that any executing jobs may not pick up this change, so these need to be // waited out before being certain that the setting has taken effect. @@ -83,18 +85,25 @@ var minWALSyncInterval = settings.RegisterDurationSetting( // If disabled after being enabled, this will prevent new range tombstones from // being written, but already written tombstones will remain until GCed. The // above note on jobs also applies in this case. -var MVCCRangeTombstonesEnabled = settings.RegisterBoolSetting( +// +// If the version of the cluster is at or beyond the version +// V23_1_MVCCRangeTombstonesUnconditionallyEnabled, the feature is +// unconditionally enabled. +var MVCCRangeTombstonesEnabledInMixedClusters = settings.RegisterBoolSetting( settings.TenantReadOnly, "storage.mvcc.range_tombstones.enabled", - "enables the use of MVCC range tombstones", + "controls the use of MVCC range tombstones in mixed version clusters; range tombstones are always on in 23.1 clusters", true) -// CanUseMVCCRangeTombstones returns true if the caller can begin writing -// MVCC range tombstones, by setting DeleteRangeRequest.UseRangeTombstone. -// It requires the MVCCRangeTombstones version gate to be active, and the -// setting storage.mvcc.range_tombstones.enabled to be enabled. +// CanUseMVCCRangeTombstones returns true if the caller can begin writing MVCC +// range tombstones, by setting DeleteRangeRequest.UseRangeTombstone. It +// requires the storage.mvcc.range_tombstones.enabled cluster setting to be +// enabled, OR the cluster version is at or beyond the +// V23_1_MVCCRangeTombstonesUnconditionallyEnabled version (i.e. in 23.1, the +// feature is unconditionally enabled). func CanUseMVCCRangeTombstones(ctx context.Context, st *cluster.Settings) bool { - return MVCCRangeTombstonesEnabled.Get(&st.SV) + return st.Version.IsActive(ctx, clusterversion.V23_1_MVCCRangeTombstonesUnconditionallyEnabled) || + MVCCRangeTombstonesEnabledInMixedClusters.Get(&st.SV) } // MaxIntentsPerWriteIntentError sets maximum number of intents returned in diff --git a/pkg/upgrade/upgrades/wait_for_del_range_in_gc_job_test.go b/pkg/upgrade/upgrades/wait_for_del_range_in_gc_job_test.go index 00fd9e7a584a..8128140256b8 100644 --- a/pkg/upgrade/upgrades/wait_for_del_range_in_gc_job_test.go +++ b/pkg/upgrade/upgrades/wait_for_del_range_in_gc_job_test.go @@ -40,7 +40,7 @@ func TestWaitForDelRangeInGCJob(t *testing.T) { ctx := context.Background() settings := cluster.MakeTestingClusterSettingsWithVersions(v1, v0, false /* initializeVersion */) require.NoError(t, clusterversion.Initialize(ctx, v0, &settings.SV)) - storage.MVCCRangeTombstonesEnabled.Override(ctx, &settings.SV, true) + storage.MVCCRangeTombstonesEnabledInMixedClusters.Override(ctx, &settings.SV, true) testServer, sqlDB, kvDB := serverutils.StartServer(t, base.TestServerArgs{ Settings: settings, Knobs: base.TestingKnobs{