Merge #55877 #55907 #55982 #55994 #56006

55877: rowflow: account for more memory usage of the row buffer r=yuzefovich a=yuzefovich

Previously, we accounted for the memory usage of the row buffer from
which the rows are pushed from in the routers only when copying the rows
from the row container, but we also have another in-memory row buffer
that we can move the rows from which was missing the memory accounting.
This is now fixed which also deflakes the router disk spill test.

Fixes: #55848.

Release note: None

55907: sql: remove vectorize=201auto option r=yuzefovich a=yuzefovich

`201auto` option of `vectorize` setting has been removed since it no
longer makes sense to keep (it was introduced as an escape hatch for
20.2 release). Note that we don't need to bump the distsql version
because of this change since it is backwards compatible - if the gateway
is running the old version that has `vectorize=201auto` set, then we
will check whether flows for all nodes don't have non-streaming
operators and possibly use `off` option on the flow setup request, then
the newer version remote node will check the vectorize setting on the
request whether it is not `off` and setup the vectorized flow if it is
not.

Release note (sql change): `201auto` value for `vectorize` session
variable and the corresponding cluster setting has been removed.

55982: roachtest: use IMPORT for all TPC-C tests r=nvanbenschoten a=nvanbenschoten

Fixes #55541.
Fixes #55580.

In #54880 and #55050, we switched to using IMPORT for most TPC-C tests,
in favor of BACKUP, which requires fixtures to be regenerated whenever
we changed the workload. In this PR, we switch over the remaining uses
of `fixtures load tpcc` to `fixtures import tpcc` to avoid compatibility
issues on older release branches.

55994: settings: delete StateMachineSetting, introduce VersionSetting r=irfansharif a=irfansharif

We introduced the custom StateMachineSetting type in #17216 to power the
underlying machinery for version upgrades:

  SET CLUSTER SETTING version = <major>-<minor>;

At the time we left it generalizable enough to make room for future
settings with arbitrary internal transitions. For cluster versions this
meant only allowing transitions from one major version to the immediate
next, all the while making sure that each binary in the cluster was able
to activate the targeted version (the proposed version fell within the
allowable range ["binary min supported version", "binary version"]).

In the three years since we haven't added any state-machine validated
settings that fit the bill, and the layers of abstractions to get to
validated cluster version updates are one too many for (my) comfort.
This PR introduces a new VersionSetting type that is more tightly
coupled with the ClusterVersion type itself.

VersionSetting uses language that only appropriate for cluster versions,
hopefully clarifying things as we go. We'll depend on this clarification
in future PRs when we remove the use of gossip in disseminating cluster
version bumps.

Release note (sql/cli change): The underlying type for the version
cluster setting has been changed. Previously it was of an internal type
representing "state machine", but now it's simply "version". This has no
operational implications, but it does reflect differently in a few
spots:
  - The `Type` column in `cockroach gen settings-list` will now show
    "version" instead of "custom validation"
  - The `setting_type` column for `version` in `SHOW CLUSTER SETTINGS`
    will now show a "v" instead of an "m"
  - The `valueType` column for `version` in `system.settings` will now
    show a "v" instead of an "m"


56006: logictest: deflake a test r=yuzefovich a=yuzefovich

We've recently merged a test that in very rare circumstances could
produce a float result that differs from the expected one by 1 in the
15th significant digit (after rounding). I believe that could occur,
e.g. when the 15th and 16th significant digits were `35`, and we matched
the spec of supporting 15 significant digits for floats, yet the
rounding makes us return an unexpected result. This commit rounds to the
precision of 1 digit less which should make the test non-flaky.

Release note: None

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>

docs/generated/settings/settings.html

@@ -90,6 +90,6 @@
 <tr><td><code>trace.debug.enable</code></td><td>boolean</td><td><code>false</code></td><td>if set, traces for recent requests can be seen in the /debug page</td></tr>
 <tr><td><code>trace.lightstep.token</code></td><td>string</td><td><code></code></td><td>if set, traces go to Lightstep using this token</td></tr>
 <tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>if set, traces go to the given Zipkin instance (example: '127.0.0.1:9411'); ignored if trace.lightstep.token is set</td></tr>
-<tr><td><code>version</code></td><td>custom validation</td><td><code>20.2-1</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
+<tr><td><code>version</code></td><td>version</td><td><code>20.2-1</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
 </tbody>
 </table>

pkg/cli/gen.go

@@ -213,7 +213,7 @@ Output the list of cluster settings known to this binary.
 				panic(fmt.Sprintf("unknown setting type %q", setting.Typ()))
 			}
 			var defaultVal string
-			if sm, ok := setting.(*settings.StateMachineSetting); ok {
+			if sm, ok := setting.(*settings.VersionSetting); ok {
 				defaultVal = sm.SettingsListDefault()
 			} else {
 				defaultVal = setting.String(&s.SV)

pkg/clusterversion/clusterversion.go

@@ -44,6 +44,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/redact"
 )
@@ -212,3 +213,19 @@ func (cv ClusterVersion) String() string { return redact.StringWithoutMarkers(cv
 func (cv ClusterVersion) SafeFormat(p redact.SafePrinter, _ rune) {
 	p.Print(cv.Version)
 }
+
+// ClusterVersionImpl implements the settings.ClusterVersionImpl interface.
+func (cv ClusterVersion) ClusterVersionImpl() {}
+
+var _ settings.ClusterVersionImpl = ClusterVersion{}
+
+// EncodingFromVersionStr is a shorthand to generate an encoded cluster version
+// from a version string.
+func EncodingFromVersionStr(v string) ([]byte, error) {
+	newV, err := roachpb.ParseVersion(v)
+	if err != nil {
+		return nil, err
+	}
+	newCV := ClusterVersion{Version: newV}
+	return protoutil.Marshal(&newCV)
+}

pkg/clusterversion/setting.go

@@ -57,27 +57,22 @@ var version = registerClusterVersionSetting()
 // setting structs, it is immutable, as Version is a global; all the state is
 // maintained in a Handle instance.
 type clusterVersionSetting struct {
-	settings.StateMachineSetting
+	settings.VersionSetting
 }
 
-var _ settings.StateMachineSettingImpl = &clusterVersionSetting{}
+var _ settings.VersionSettingImpl = &clusterVersionSetting{}
 
 // registerClusterVersionSetting creates a clusterVersionSetting and registers
 // it with the cluster settings registry.
 func registerClusterVersionSetting() *clusterVersionSetting {
-	s := makeClusterVersionSetting()
-	s.StateMachineSetting.SetReportable(true)
-	settings.RegisterStateMachineSetting(
+	s := &clusterVersionSetting{}
+	s.VersionSetting = settings.MakeVersionSetting(s)
+	settings.RegisterVersionSetting(
 		KeyVersionSetting,
 		"set the active cluster version in the format '<major>.<minor>'", // hide optional `-<unstable>,
-		&s.StateMachineSetting)
+		&s.VersionSetting)
 	s.SetVisibility(settings.Public)
-	return s
-}
-
-func makeClusterVersionSetting() *clusterVersionSetting {
-	s := &clusterVersionSetting{}
-	s.StateMachineSetting = settings.MakeStateMachineSetting(s)
+	s.SetReportable(true)
 	return s
 }
 
@@ -106,7 +101,7 @@ func (cv *clusterVersionSetting) initialize(
 		}
 		// Now version > ver.Version.
 	}
-	if err := cv.validateSupportedVersionInner(ctx, version, sv); err != nil {
+	if err := cv.validateBinaryVersions(version, sv); err != nil {
 		return err
 	}
 
@@ -176,69 +171,60 @@ func (cv *clusterVersionSetting) setBeforeChange(
 	h.beforeClusterVersionChangeMu.cb = cb
 }
 
-// Decode is part of the StateMachineSettingImpl interface.
-func (cv *clusterVersionSetting) Decode(val []byte) (interface{}, error) {
+// Decode is part of the VersionSettingImpl interface.
+func (cv *clusterVersionSetting) Decode(val []byte) (settings.ClusterVersionImpl, error) {
 	var clusterVersion ClusterVersion
 	if err := protoutil.Unmarshal(val, &clusterVersion); err != nil {
-		return "", err
+		return nil, err
 	}
 	return clusterVersion, nil
 }
 
-// DecodeToString is part of the StateMachineSettingImpl interface.
-func (cv *clusterVersionSetting) DecodeToString(val []byte) (string, error) {
-	clusterVersion, err := cv.Decode(val)
-	if err != nil {
-		return "", err
-	}
-	return clusterVersion.(ClusterVersion).Version.String(), nil
-}
-
-// ValidateLogical is part of the StateMachineSettingImpl interface.
-func (cv *clusterVersionSetting) ValidateLogical(
-	ctx context.Context, sv *settings.Values, curRawProto []byte, newVal string,
+// Validate is part of the VersionSettingImpl interface.
+func (cv *clusterVersionSetting) Validate(
+	_ context.Context, sv *settings.Values, curRawProto, newRawProto []byte,
 ) ([]byte, error) {
-	newVersion, err := roachpb.ParseVersion(newVal)
-	if err != nil {
+	var newCV ClusterVersion
+	if err := protoutil.Unmarshal(newRawProto, &newCV); err != nil {
 		return nil, err
 	}
-	if err := cv.validateSupportedVersionInner(ctx, newVersion, sv); err != nil {
+
+	if err := cv.validateBinaryVersions(newCV.Version, sv); err != nil {
 		return nil, err
 	}
 
-	var oldV ClusterVersion
-	if err := protoutil.Unmarshal(curRawProto, &oldV); err != nil {
+	var oldCV ClusterVersion
+	if err := protoutil.Unmarshal(curRawProto, &oldCV); err != nil {
 		return nil, err
 	}
 
 	// Versions cannot be downgraded.
-	if newVersion.Less(oldV.Version) {
+	if newCV.Version.Less(oldCV.Version) {
 		return nil, errors.Errorf(
 			"versions cannot be downgraded (attempting to downgrade from %s to %s)",
-			oldV.Version, newVersion)
+			oldCV.Version, newCV.Version)
 	}
 
-	// Prevent cluster version upgrade until cluster.preserve_downgrade_option is reset.
+	// Prevent cluster version upgrade until cluster.preserve_downgrade_option
+	// is reset.
 	if downgrade := preserveDowngradeVersion.Get(sv); downgrade != "" {
 		return nil, errors.Errorf(
 			"cannot upgrade to %s: cluster.preserve_downgrade_option is set to %s",
-			newVersion, downgrade)
+			newCV.Version, downgrade)
 	}
 
 	// Return the serialized form of the new version.
-	newV := ClusterVersion{Version: newVersion}
-	return protoutil.Marshal(&newV)
+	return protoutil.Marshal(&newCV)
 }
 
-// ValidateGossipVersion is part of the StateMachineSettingImpl interface.
-func (cv *clusterVersionSetting) ValidateGossipUpdate(
+// ValidateBinaryVersions is part of the VersionSettingImpl interface.
+func (cv *clusterVersionSetting) ValidateBinaryVersions(
 	ctx context.Context, sv *settings.Values, rawProto []byte,
 ) (retErr error) {
-
 	defer func() {
-		// This implementation of ValidateGossipUpdate never returns errors. Instead,
-		// we crash. Not being able to update our version to what the rest of the cluster is running
-		// is a serious issue.
+		// This implementation of ValidateBinaryVersions never returns errors.
+		// Instead, we crash. Not being able to update our version to what the
+		// rest of the cluster is running is a serious issue.
 		if retErr != nil {
 			log.Fatalf(ctx, "failed to validate version upgrade: %s", retErr)
 		}
@@ -248,15 +234,15 @@ func (cv *clusterVersionSetting) ValidateGossipUpdate(
 	if err := protoutil.Unmarshal(rawProto, &ver); err != nil {
 		return err
 	}
-	return cv.validateSupportedVersionInner(ctx, ver.Version, sv)
+	return cv.validateBinaryVersions(ver.Version, sv)
 }
 
-// SettingsListDefault is part of the StateMachineSettingImpl interface.
+// SettingsListDefault is part of the VersionSettingImpl interface.
 func (cv *clusterVersionSetting) SettingsListDefault() string {
 	return binaryVersion.String()
 }
 
-// BeforeChange is part of the StateMachineSettingImpl interface
+// BeforeChange is part of the VersionSettingImpl interface.
 func (cv *clusterVersionSetting) BeforeChange(
 	ctx context.Context, encodedVal []byte, sv *settings.Values,
 ) {
@@ -274,8 +260,8 @@ func (cv *clusterVersionSetting) BeforeChange(
 	h.beforeClusterVersionChangeMu.Unlock()
 }
 
-func (cv *clusterVersionSetting) validateSupportedVersionInner(
-	ctx context.Context, ver roachpb.Version, sv *settings.Values,
+func (cv *clusterVersionSetting) validateBinaryVersions(
+	ver roachpb.Version, sv *settings.Values,
 ) error {
 	vh := sv.Opaque().(Handle)
 	if vh.BinaryMinSupportedVersion() == (roachpb.Version{}) {

pkg/cmd/roachtest/cancel.go

@@ -43,8 +43,7 @@ func registerCancel(r *testRegistry) {
 		m := newMonitor(ctx, c, c.All())
 		m.Go(func(ctx context.Context) error {
 			t.Status("importing TPCC fixture")
-			c.Run(ctx, c.Node(1), fmt.Sprintf(
-				"./workload fixtures load tpcc --warehouses=%d {pgurl:1}", warehouses))
+			c.Run(ctx, c.Node(1), tpccImportCmd(warehouses))
 
 			conn := c.Conn(ctx, 1)
 			defer conn.Close()

pkg/cmd/roachtest/drop.go

@@ -37,8 +37,7 @@ func registerDrop(r *testRegistry) {
 		m := newMonitor(ctx, c, c.Range(1, nodes))
 		m.Go(func(ctx context.Context) error {
 			t.WorkerStatus("importing TPCC fixture")
-			c.Run(ctx, c.Node(1), fmt.Sprintf(
-				"./workload fixtures load tpcc --warehouses=%d --db tpcc {pgurl:1}", warehouses))
+			c.Run(ctx, c.Node(1), tpccImportCmd(warehouses))
 
 			// Don't open the DB connection until after the data has been imported.
 			// Otherwise the ALTER TABLE query below might fail to find the

pkg/cmd/roachtest/mixed_version_jobs.go

@@ -104,15 +104,10 @@ func (s *backgroundStepper) stop(ctx context.Context, t *test, u *versionUpgrade
 	}
 }
 
-func backgroundTPCCWorkload(t *test, warehouses int, tpccDB string) backgroundStepper {
+func backgroundTPCCWorkload(t *test, warehouses int) backgroundStepper {
 	return makeBackgroundStepper(func(ctx context.Context, u *versionUpgradeTest) error {
-		cmd := []string{
-			"./workload fixtures load tpcc",
-			fmt.Sprintf("--warehouses=%d", warehouses),
-			fmt.Sprintf("--db=%s", tpccDB),
-		}
 		// The workload has to run on one of the nodes of the cluster.
-		err := u.c.RunE(ctx, u.c.Node(1), cmd...)
+		err := u.c.RunE(ctx, u.c.Node(1), tpccImportCmd(warehouses))
 		if ctx.Err() != nil {
 			// If the context is canceled, that's probably why the workload returned
 			// so swallow error. (This is how the harness tells us to shut down the
@@ -220,7 +215,7 @@ func runJobsMixedVersions(
 	// `cockroach` will be used.
 	const mainVersion = ""
 	roachNodes := c.All()
-	backgroundTPCC := backgroundTPCCWorkload(t, warehouses, "tpcc")
+	backgroundTPCC := backgroundTPCCWorkload(t, warehouses)
 	resumeAllJobsAndWaitStep := makeResumeAllJobsAndWaitStep(10 * time.Second)
 	c.Put(ctx, workload, "./workload", c.Node(1))
 

pkg/cmd/roachtest/network.go

@@ -108,9 +108,7 @@ func runNetworkTPCC(ctx context.Context, t *test, origC *cluster, nodes int) {
 
 	const warehouses = 1
 	c.Start(ctx, t, serverNodes)
-	c.Run(ctx, workerNode, fmt.Sprintf(
-		`./workload fixtures load tpcc --warehouses=%d {pgurl:1}`, warehouses,
-	))
+	c.Run(ctx, c.Node(1), tpccImportCmd(warehouses))
 
 	db := c.Conn(ctx, 1)
 	defer db.Close()

pkg/cmd/roachtest/tpcc.go

@@ -71,9 +71,9 @@ type tpccOptions struct {
 // specify pgurl to ensure that it is run on a node with a running cockroach
 // instance to ensure that the workload version matches the gateway version in a
 // mixed version cluster.
-func tpccImportCmd(t *test, warehouses int, extraArgs string) string {
+func tpccImportCmd(warehouses int, extraArgs ...string) string {
 	return fmt.Sprintf("./cockroach workload fixtures import tpcc --warehouses=%d %s",
-		warehouses, extraArgs)
+		warehouses, strings.Join(extraArgs, " "))
 }
 
 func setupTPCC(
@@ -125,7 +125,7 @@ func setupTPCC(
 		switch opts.SetupType {
 		case usingImport:
 			t.Status("loading fixture")
-			c.Run(ctx, crdbNodes[:1], tpccImportCmd(t, opts.Warehouses, opts.ExtraSetupArgs))
+			c.Run(ctx, crdbNodes[:1], tpccImportCmd(opts.Warehouses, opts.ExtraSetupArgs))
 		case usingInit:
 			t.Status("initializing tables")
 			extraArgs := opts.ExtraSetupArgs
@@ -639,7 +639,7 @@ func loadTPCCBench(
 	// Load the corresponding fixture.
 	t.l.Printf("restoring tpcc fixture\n")
 	waitForFullReplication(t, db)
-	cmd := tpccImportCmd(t, b.LoadWarehouses, loadArgs)
+	cmd := tpccImportCmd(b.LoadWarehouses, loadArgs)
 	if err := c.RunE(ctx, roachNodes[:1], cmd); err != nil {
 		return err
 	}

pkg/settings/BUILD.bazel

@@ -13,9 +13,9 @@ go_library(
         "masked.go",
         "registry.go",
         "setting.go",
-        "statemachine.go",
         "string.go",
         "updater.go",
+        "version.go",
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/settings",
     visibility = ["//visibility:public"],

pkg/settings/registry.go

@@ -144,7 +144,7 @@ var ReadableTypes = map[string]string{
 	"z": "byte size",
 	"d": "duration",
 	"e": "enumeration",
-	"m": "custom validation",
+	"v": "version",
 }
 
 // RedactedValue returns a string representation of the value for settings

pkg/settings/setting.go

@@ -190,19 +190,35 @@ func (sv *Values) setOnChange(slotIdx int, fn func()) {
 type Setting interface {
 	// Typ returns the short (1 char) string denoting the type of setting.
 	Typ() string
+	// String returns the string representation of the setting's current value.
+	// It's used when materializing results for `SHOW CLUSTER SETTINGS` or `SHOW
+	// CLUSTER SETTING <setting-name>`.
 	String(sv *Values) string
+	// Description contains a helpful text explaining what the specific cluster
+	// setting is for.
 	Description() string
+	// Visibility controls whether or not the setting is made publicly visible.
+	// Reserved settings are still accessible to users, but they don't get
+	// listed out when retrieving all settings.
 	Visibility() Visibility
 }
 
 // WritableSetting is the exported interface of non-masked settings.
 type WritableSetting interface {
 	Setting
 
-	// Encoded returns the encoded value of the current value of the setting.
+	// Encoded returns the encoded representation of the current value of the
+	// setting.
 	Encoded(sv *Values) string
+	// EncodedDefault returns the encoded representation of the default value of
+	// the setting.
 	EncodedDefault() string
+	// SetOnChange installs a callback to be called when a setting's value
+	// changes. `fn` should avoid doing long-running or blocking work as it is
+	// called on the goroutine which handles all settings updates.
 	SetOnChange(sv *Values, fn func())
+	// ErrorHint returns a hint message to be displayed to the user when there's
+	// an error.
 	ErrorHint() (bool, string)
 }