sql: remove testing.vectorize.batch_size setting

With the recent addition of the metamorphic build tag we no longer need to expose the `sql.testing.vectorize.batch_size` cluster setting in order to randomize `coldata.BatchSize()` for testing purposes, so this commit retires the setting. The only testing coverage we're losing is `tpchvec/smallbatchsize` test, but I don't think it ever found any issues, so it doesn't seem important to figure out how to use the metamorphic tag for it. Release note: None (removing non-public setting)
cockroachdb · Dec 3, 2020 · 6e6ec8e · 6e6ec8e
1 parent 4c61427
commit 6e6ec8e
Show file tree

Hide file tree

Showing 8 changed files with 3 additions and 111 deletions.
diff --git a/pkg/cmd/roachtest/tpchvec.go b/pkg/cmd/roachtest/tpchvec.go
@@ -17,7 +17,6 @@ import (
 	gosql "database/sql"
 	"fmt"
 	"math"
-	"math/rand"
 	"regexp"
 	"runtime"
 	"sort"
@@ -146,11 +145,6 @@ func (b tpchVecTestCaseBase) getRunConfig(
 		}},
 		setupNames: []string{"default"},
 	}
-	if version != tpchVecVersion19_2 {
-		runConfig.clusterSetups[0] = append(runConfig.clusterSetups[0],
-			"RESET CLUSTER SETTING sql.testing.vectorize.batch_size",
-		)
-	}
 	for queryNum := 1; queryNum <= tpch.NumQueries; queryNum++ {
 		if _, shouldSkip := queriesToSkip[queryNum]; !shouldSkip {
 			runConfig.queriesToRun = append(runConfig.queriesToRun, queryNum)
@@ -517,33 +511,6 @@ func (d tpchVecDiskTest) preTestRunHook(
 	}
 }
 
-// setSmallBatchSize sets a cluster setting to override the batch size to be in
-// [1, 5) range.
-func setSmallBatchSize(t *test, conn *gosql.DB, rng *rand.Rand) {
-	batchSize := 1 + rng.Intn(4)
-	t.Status(fmt.Sprintf("setting sql.testing.vectorize.batch_size to %d", batchSize))
-	if _, err := conn.Exec(fmt.Sprintf("SET CLUSTER SETTING sql.testing.vectorize.batch_size=%d", batchSize)); err != nil {
-		t.Fatal(err)
-	}
-}
-
-type tpchVecSmallBatchSizeTest struct {
-	tpchVecTestCaseBase
-}
-
-func (b tpchVecSmallBatchSizeTest) preTestRunHook(
-	ctx context.Context,
-	t *test,
-	c *cluster,
-	conn *gosql.DB,
-	version crdbVersion,
-	clusterSetup []string,
-) {
-	b.tpchVecTestCaseBase.preTestRunHook(t, conn, clusterSetup, true /* createStats */)
-	rng, _ := randutil.NewPseudoRand()
-	setSmallBatchSize(t, conn, rng)
-}
-
 func baseTestRun(
 	ctx context.Context, t *test, c *cluster, conn *gosql.DB, version crdbVersion, tc tpchVecTestCase,
 ) {
@@ -607,12 +574,6 @@ func (s tpchVecSmithcmpTest) preTestRunHook(
 		t.Fatal(err)
 	}
 	c.Put(ctx, smithcmp, "./"+tpchVecSmithcmp, node)
-	// To increase test coverage, we will be randomizing the batch size in 50%
-	// of the runs.
-	rng, _ := randutil.NewPseudoRand()
-	if rng.Float64() < 0.5 {
-		setSmallBatchSize(t, conn, rng)
-	}
 }
 
 func smithcmpTestRun(
@@ -698,18 +659,6 @@ func registerTPCHVec(r *testRegistry) {
 		},
 	})
 
-	r.Add(testSpec{
-		Name:    "tpchvec/smallbatchsize",
-		Owner:   OwnerSQLExec,
-		Cluster: makeClusterSpec(tpchVecNodeCount),
-		// 19.2 version doesn't have the testing cluster setting to change the batch
-		// size, so only run on versions >= 20.1.0.
-		MinVersion: "v20.1.0",
-		Run: func(ctx context.Context, t *test, c *cluster) {
-			runTPCHVec(ctx, t, c, tpchVecSmallBatchSizeTest{}, baseTestRun)
-		},
-	})
-
 	r.Add(testSpec{
 		Name:       "tpchvec/smithcmp",
 		Owner:      OwnerSQLExec,
@@ -742,6 +691,8 @@ func registerTPCHVec(r *testRegistry) {
 			// that modify the cluster settings for all configs to benchmark
 			// like in the example below. The example benchmarks three values
 			// of coldata.BatchSize() variable against each other.
+			// NOTE: the setting has been removed since the example was written,
+			// but it still serves the purpose of showing how to use the config.
 			var clusterSetups [][]string
 			var setupNames []string
 			for _, batchSize := range []int{512, 1024, 1536} {

diff --git a/pkg/col/coldata/batch.go b/pkg/col/coldata/batch.go
@@ -105,12 +105,6 @@ func SetBatchSizeForTests(newBatchSize int) error {
 	return nil
 }
 
-// ResetBatchSizeForTests resets the batchSize variable to the default batch
-// size. It should only be used in tests.
-func ResetBatchSizeForTests() {
-	atomic.SwapInt64(&batchSize, defaultBatchSize)
-}
-
 // NewMemBatch allocates a new in-memory Batch.
 // TODO(jordan): pool these allocations.
 func NewMemBatch(typs []*types.T, factory ColumnFactory) Batch {

diff --git a/pkg/settings/registry.go b/pkg/settings/registry.go
@@ -73,6 +73,7 @@ var retiredSettings = map[string]struct{}{
 	"sql.parallel_scans.enabled":                                       {},
 	// removed as of 21.1.
 	"sql.distsql.interleaved_joins.enabled": {},
+	"sql.testing.vectorize.batch_size":      {},
 }
 
 // register adds a setting to the registry.

diff --git a/pkg/sql/colflow/BUILD.bazel b/pkg/sql/colflow/BUILD.bazel
@@ -9,7 +9,6 @@ go_library(
         "//pkg/col/coldata",
         "//pkg/col/coldataext",
         "//pkg/rpc/nodedialer",
-        "//pkg/settings",
         "//pkg/sql/catalog/descs",
         "//pkg/sql/colcontainer",
         "//pkg/sql/colexec",
@@ -21,8 +20,6 @@ go_library(
         "//pkg/sql/execinfra",
         "//pkg/sql/execinfrapb",
         "//pkg/sql/flowinfra",
-        "//pkg/sql/pgwire/pgcode",
-        "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/rowexec",
         "//pkg/sql/sessiondatapb",
         "//pkg/sql/types",

diff --git a/pkg/sql/colflow/vectorized_flow.go b/pkg/sql/colflow/vectorized_flow.go
@@ -12,7 +12,6 @@ package colflow
 
 import (
 	"context"
-	"fmt"
 	"math"
 	"path/filepath"
 	"strconv"
@@ -24,7 +23,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/col/coldata"
 	"github.com/cockroachdb/cockroach/pkg/col/coldataext"
 	"github.com/cockroachdb/cockroach/pkg/rpc/nodedialer"
-	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/colcontainer"
 	"github.com/cockroachdb/cockroach/pkg/sql/colexec"
@@ -36,8 +34,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfra"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/flowinfra"
-	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
-	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowexec"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
@@ -167,21 +163,6 @@ func NewVectorizedFlow(base *flowinfra.FlowBase) flowinfra.Flow {
 	return vf
 }
 
-// VectorizeTestingBatchSize is a testing cluster setting that sets the default
-// batch size used by the vectorized execution engine. A low batch size is
-// useful to test batch reuse.
-var VectorizeTestingBatchSize = settings.RegisterValidatedIntSetting(
-	"sql.testing.vectorize.batch_size",
-	fmt.Sprintf("the size of a batch of rows in the vectorized engine (0=default, value must be less than %d)", coldata.MaxBatchSize),
-	0,
-	func(newBatchSize int64) error {
-		if newBatchSize > coldata.MaxBatchSize {
-			return pgerror.Newf(pgcode.InvalidParameterValue, "batch size %d may not be larger than %d", newBatchSize, coldata.MaxBatchSize)
-		}
-		return nil
-	},
-)
-
 // Setup is part of the flowinfra.Flow interface.
 func (f *vectorizedFlow) Setup(
 	ctx context.Context, spec *execinfrapb.FlowSpec, opt flowinfra.FuseOpt,
@@ -200,18 +181,6 @@ func (f *vectorizedFlow) Setup(
 	}
 	helper := newVectorizedFlowCreatorHelper(f.FlowBase)
 
-	testingBatchSize := int64(0)
-	if f.FlowCtx.Cfg.Settings != nil {
-		testingBatchSize = VectorizeTestingBatchSize.Get(&f.FlowCtx.Cfg.Settings.SV)
-	}
-	if testingBatchSize != 0 {
-		if err := coldata.SetBatchSizeForTests(int(testingBatchSize)); err != nil {
-			return ctx, err
-		}
-	} else {
-		coldata.ResetBatchSizeForTests()
-	}
-
 	diskQueueCfg := colcontainer.DiskQueueCfg{
 		FS:        f.Cfg.TempFS,
 		GetPather: f,

diff --git a/pkg/sql/logictest/BUILD.bazel b/pkg/sql/logictest/BUILD.bazel
@@ -8,7 +8,6 @@ go_library(
     deps = [
         "//pkg/base",
         "//pkg/build",
-        "//pkg/col/coldata",
         "//pkg/kv/kvserver",
         "//pkg/roachpb",
         "//pkg/security",

diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go
@@ -39,7 +39,6 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/build"
-	"github.com/cockroachdb/cockroach/pkg/col/coldata"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security"
@@ -1109,11 +1108,6 @@ type logicTest struct {
 	curPath   string
 	curLineNo int
 
-	// randomizedVectorizedBatchSize stores the randomized batch size for
-	// vectorized engine if it is not turned off. The batch size will randomly be
-	// set to 1 with 25% probability, {2, 3} with 25% probability or default batch
-	// size with 50% probability.
-	randomizedVectorizedBatchSize int
 	// randomizedMutationsMaxBatchSize stores the randomized max batch size for
 	// the mutation operations. The max batch size will randomly be set to 1
 	// with 25% probability, a random value in [2, 100] range with 25%
@@ -1462,12 +1456,6 @@ func (t *logicTest) setup(cfg testClusterConfig, serverArgs TestServerArgs) {
 			t.Fatal(err)
 		}
 
-		if _, err := conn.Exec(
-			"SET CLUSTER SETTING sql.testing.vectorize.batch_size = $1", t.randomizedVectorizedBatchSize,
-		); err != nil {
-			t.Fatal(err)
-		}
-
 		if _, err := conn.Exec(
 			"SET CLUSTER SETTING sql.testing.mutations.max_batch_size = $1", t.randomizedMutationsMaxBatchSize,
 		); err != nil {
@@ -3059,12 +3047,7 @@ func RunLogicTestWithDefaultConfig(
 		}
 	}
 
-	// Determining whether or not to randomize vectorized batch size.
 	rng, _ := randutil.NewPseudoRand()
-	randomizedVectorizedBatchSize := randomValue(rng, []int{1, 2, 3}, []float64{0.25, 0.125, 0.125}, coldata.BatchSize())
-	if randomizedVectorizedBatchSize != coldata.BatchSize() {
-		t.Log(fmt.Sprintf("randomize coldata.BatchSize to %d", randomizedVectorizedBatchSize))
-	}
 	randomizedMutationsMaxBatchSize := mutations.MaxBatchSize()
 	// Temporarily disable this randomization because of #54948.
 	// TODO(yuzefovich): re-enable it once the issue is figured out.
@@ -3133,7 +3116,6 @@ func RunLogicTestWithDefaultConfig(
 						verbose:                         verbose,
 						perErrorSummary:                 make(map[string][]string),
 						rng:                             rng,
-						randomizedVectorizedBatchSize:   randomizedVectorizedBatchSize,
 						randomizedMutationsMaxBatchSize: randomizedMutationsMaxBatchSize,
 					}
 					if *printErrorSummary {

diff --git a/pkg/sql/logictest/testdata/logic_test/system b/pkg/sql/logictest/testdata/logic_test/system
@@ -581,7 +581,6 @@ FROM system.settings
 WHERE name NOT IN ('version', 'sql.defaults.distsql', 'cluster.secret',
   'sql.stats.automatic_collection.enabled', 'sql.defaults.vectorize',
   'sql.defaults.vectorize_row_count_threshold',
-  'sql.testing.vectorize.batch_size',
   'sql.defaults.experimental_distsql_planning',
   'sql.testing.mutations.max_batch_size')
 ORDER BY name