From 9d24366d164b87c33f9ced6268323c9b1e0c0ed6 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Mon, 13 Feb 2023 12:18:55 -0800 Subject: [PATCH] storage: never set a too-old format version This change asserts that we never set a pebble FormatVersion that is older than the one minimum supported by our code. Benchmarks that set a lower value are updated (and BenchmarkTimeBoundIterate is reenabled). These benchmarks can fail when the code exercises a feature like range keys that is not supported by the older format. Fixes #97061 Release note: None Epic: none --- pkg/ccl/storageccl/engineccl/BUILD.bazel | 1 - pkg/ccl/storageccl/engineccl/bench_test.go | 3 --- pkg/kv/kvserver/batcheval/BUILD.bazel | 1 - .../batcheval/cmd_refresh_range_bench_test.go | 2 -- pkg/kv/kvserver/rangefeed/BUILD.bazel | 1 - .../rangefeed/catchup_scan_bench_test.go | 2 -- pkg/storage/metamorphic/options.go | 4 ---- pkg/storage/mvcc_incremental_iterator_test.go | 10 +++------- pkg/storage/pebble.go | 16 ++++++++++++++-- 9 files changed, 17 insertions(+), 23 deletions(-) diff --git a/pkg/ccl/storageccl/engineccl/BUILD.bazel b/pkg/ccl/storageccl/engineccl/BUILD.bazel index 36bbc4bcc927..bbaed697b3f8 100644 --- a/pkg/ccl/storageccl/engineccl/BUILD.bazel +++ b/pkg/ccl/storageccl/engineccl/BUILD.bazel @@ -50,7 +50,6 @@ go_test( "//pkg/storage/enginepb", "//pkg/testutils", "//pkg/testutils/datapathutils", - "//pkg/testutils/skip", "//pkg/testutils/storageutils", "//pkg/util/encoding", "//pkg/util/hlc", diff --git a/pkg/ccl/storageccl/engineccl/bench_test.go b/pkg/ccl/storageccl/engineccl/bench_test.go index 12fcd4c38cff..08cde2494f20 100644 --- a/pkg/ccl/storageccl/engineccl/bench_test.go +++ b/pkg/ccl/storageccl/engineccl/bench_test.go @@ -21,7 +21,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/testutils" - "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/util/encoding" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -167,8 +166,6 @@ func runIterate( } func BenchmarkTimeBoundIterate(b *testing.B) { - skip.WithIssue(b, 95530, "bump minBinary to 22.2. Skip 22.2 mixed-version tests for future cleanup") - for _, loadFactor := range []float32{1.0, 0.5, 0.1, 0.05, 0.0} { b.Run(fmt.Sprintf("LoadFactor=%.2f", loadFactor), func(b *testing.B) { b.Run("NormalIterator", func(b *testing.B) { diff --git a/pkg/kv/kvserver/batcheval/BUILD.bazel b/pkg/kv/kvserver/batcheval/BUILD.bazel index 9ad5de919cb2..2878aee6cb8d 100644 --- a/pkg/kv/kvserver/batcheval/BUILD.bazel +++ b/pkg/kv/kvserver/batcheval/BUILD.bazel @@ -173,7 +173,6 @@ go_test( "//pkg/util/uuid", "@com_github_cockroachdb_errors//:errors", "@com_github_cockroachdb_errors//oserror", - "@com_github_cockroachdb_pebble//:pebble", "@com_github_cockroachdb_pebble//vfs", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", diff --git a/pkg/kv/kvserver/batcheval/cmd_refresh_range_bench_test.go b/pkg/kv/kvserver/batcheval/cmd_refresh_range_bench_test.go index c40cebf6a81c..e77ca18dd056 100644 --- a/pkg/kv/kvserver/batcheval/cmd_refresh_range_bench_test.go +++ b/pkg/kv/kvserver/batcheval/cmd_refresh_range_bench_test.go @@ -30,7 +30,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/randutil" "github.com/cockroachdb/errors/oserror" - "github.com/cockroachdb/pebble" "github.com/cockroachdb/pebble/vfs" "github.com/stretchr/testify/require" ) @@ -183,7 +182,6 @@ func setupMVCCPebble(b testing.TB, dir string, lBaseMaxBytes int64, readOnly boo opts.FS = vfs.Default opts.LBaseMaxBytes = lBaseMaxBytes opts.ReadOnly = readOnly - opts.FormatMajorVersion = pebble.FormatBlockPropertyCollector peb, err := storage.NewPebble( context.Background(), storage.PebbleConfig{ diff --git a/pkg/kv/kvserver/rangefeed/BUILD.bazel b/pkg/kv/kvserver/rangefeed/BUILD.bazel index 084b10d96504..8c6a9a265804 100644 --- a/pkg/kv/kvserver/rangefeed/BUILD.bazel +++ b/pkg/kv/kvserver/rangefeed/BUILD.bazel @@ -75,7 +75,6 @@ go_test( "//pkg/util/uuid", "@com_github_cockroachdb_errors//:errors", "@com_github_cockroachdb_errors//oserror", - "@com_github_cockroachdb_pebble//:pebble", "@com_github_cockroachdb_pebble//vfs", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", diff --git a/pkg/kv/kvserver/rangefeed/catchup_scan_bench_test.go b/pkg/kv/kvserver/rangefeed/catchup_scan_bench_test.go index 30684c509c09..f22ba5c3bc32 100644 --- a/pkg/kv/kvserver/rangefeed/catchup_scan_bench_test.go +++ b/pkg/kv/kvserver/rangefeed/catchup_scan_bench_test.go @@ -30,7 +30,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/randutil" "github.com/cockroachdb/errors/oserror" - "github.com/cockroachdb/pebble" "github.com/cockroachdb/pebble/vfs" "github.com/stretchr/testify/require" ) @@ -202,7 +201,6 @@ func setupMVCCPebble(b testing.TB, dir string, lBaseMaxBytes int64, readOnly boo opts.FS = vfs.Default opts.LBaseMaxBytes = lBaseMaxBytes opts.ReadOnly = readOnly - opts.FormatMajorVersion = pebble.FormatRangeKeys peb, err := storage.NewPebble( context.Background(), storage.PebbleConfig{ diff --git a/pkg/storage/metamorphic/options.go b/pkg/storage/metamorphic/options.go index a6017437c1e1..c203fbec4663 100644 --- a/pkg/storage/metamorphic/options.go +++ b/pkg/storage/metamorphic/options.go @@ -100,15 +100,11 @@ func standardOptions(i int) *pebble.Options { if err := opts.Parse(stdOpts[i], nil); err != nil { panic(err) } - // Enable range keys in all options. - opts.FormatMajorVersion = pebble.FormatRangeKeys return opts } func randomOptions() *pebble.Options { opts := storage.DefaultPebbleOptions() - // Enable range keys in all options. - opts.FormatMajorVersion = pebble.FormatRangeKeys rng, _ := randutil.NewTestRand() opts.BytesPerSync = 1 << rngIntRange(rng, 8, 30) diff --git a/pkg/storage/mvcc_incremental_iterator_test.go b/pkg/storage/mvcc_incremental_iterator_test.go index f869edff3c84..4e47183e81c1 100644 --- a/pkg/storage/mvcc_incremental_iterator_test.go +++ b/pkg/storage/mvcc_incremental_iterator_test.go @@ -33,7 +33,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/randutil" "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/cockroachdb/errors" - "github.com/cockroachdb/pebble" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/sync/errgroup" @@ -1568,7 +1567,7 @@ func BenchmarkMVCCIncrementalIteratorForOldData(b *testing.B) { // day of keys. The old keys are uniformly distributed in the key space, // which is the worst case for block property filters. keyAgeInterval := 400 - setupMVCCPebbleWithBlockProperties := func(b *testing.B) Engine { + setupMVCCPebble := func(b *testing.B) Engine { eng, err := Open( context.Background(), InMemory(), @@ -1577,10 +1576,7 @@ func BenchmarkMVCCIncrementalIteratorForOldData(b *testing.B) { // will mostly miss the cache (especially since the block cache is meant // to be scan resistant). CacheSize(1<<10), - func(cfg *engineConfig) error { - cfg.Opts.FormatMajorVersion = pebble.FormatBlockPropertyCollector - return nil - }) + ) if err != nil { b.Fatal(err) } @@ -1618,7 +1614,7 @@ func BenchmarkMVCCIncrementalIteratorForOldData(b *testing.B) { } for _, valueSize := range []int{100, 500, 1000, 2000} { - eng := setupMVCCPebbleWithBlockProperties(b) + eng := setupMVCCPebble(b) setupData(b, eng, valueSize) b.Run(fmt.Sprintf("valueSize=%d", valueSize), func(b *testing.B) { startKey := roachpb.Key(encoding.EncodeUvarintAscending([]byte("key-"), uint64(0))) diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go index af395f6bd9f6..510b66cef369 100644 --- a/pkg/storage/pebble.go +++ b/pkg/storage/pebble.go @@ -533,6 +533,12 @@ var PebbleBlockPropertyCollectors = []func() pebble.BlockPropertyCollector{ }, } +// MinimumSupportedFormatVersion is the version that provides features that the +// Cockroach code relies on unconditionally (like range keys). New stores are by +// default created with this version. It should correspond to the minimum +// supported binary version. +const MinimumSupportedFormatVersion = pebble.FormatPrePebblev1Marked + // DefaultPebbleOptions returns the default pebble options. func DefaultPebbleOptions() *pebble.Options { // In RocksDB, the concurrency setting corresponds to both flushes and @@ -556,7 +562,7 @@ func DefaultPebbleOptions() *pebble.Options { Merger: MVCCMerger, BlockPropertyCollectors: PebbleBlockPropertyCollectors, // Minimum supported format. - FormatMajorVersion: pebble.FormatPrePebblev1Marked, + FormatMajorVersion: MinimumSupportedFormatVersion, } // Automatically flush 10s after the first range tombstone is added to a // memtable. This ensures that we can reclaim space even when there's no @@ -870,6 +876,12 @@ func NewPebble(ctx context.Context, cfg PebbleConfig) (p *Pebble, err error) { // Clone the given options so that we are free to modify them. opts = cfg.Opts.Clone() } + if opts.FormatMajorVersion < MinimumSupportedFormatVersion { + return nil, errors.AssertionFailedf( + "FormatMajorVersion is %d, should be at least %d", + opts.FormatMajorVersion, MinimumSupportedFormatVersion, + ) + } // pebble.Open also calls EnsureDefaults, but only after doing a clone. Call // EnsureDefaults here to make sure we have a working FS. @@ -1965,7 +1977,7 @@ func (p *Pebble) SetMinVersion(version roachpb.Version) error { // This should never happen in production. But we tolerate tests creating // imaginary older versions; we must still use the earliest supported // format. - formatVers = pebble.FormatPrePebblev1Marked + formatVers = MinimumSupportedFormatVersion } if p.db.FormatMajorVersion() < formatVers {