Merge #152658 #154149 #154579 #154682 #154743 #154745

152658: jobs: avoid unnecessary writes r=dt a=stevendanna

See individual commits.

154149: azure: configure a default TryTimeout of 60 seconds r=jeffswenson a=jeffswenson

We have seen some evidence of stuck or slow requests to azure blob storage. Setting a try timeout allows us to retry these slow operations and may allow us to make forward progress if the underlying error is transient. Operations that run into the TryTimeout will be internally retried by the cloud package.

The TryTimeout can be controlled via the cloudstorage.azure.try.timeout setting. Setting it to zero disables the per-attempt timeout.

60 seconds is a relatively long timeout. The main reason it is set that long is the timeout is applied to read operations and CRDB sometimes performs long lived stream reads when it is merging many SSTs during a restore.

Release note: Add a default TryTimeout of 60 seconds for Azure Blob Storage to mitigate occasional stuck operations. 
Fixes: #154085

154579: storageconfig: encryption YAML support and validation r=RaduBerinde a=RaduBerinde

#### storageconfig: move StoreEncryptionSpec to cli

The separate encryption specs are a byproduct of having to use a
separate command-line flag. This change moves the related code to the
cli package.

Epic: none
Release note: None

#### storageconfig: move WAL failover flag code to cli

Move the pflag-specific part to cli.

Epic: none
Release note: None

#### storageconfig: use time.Duration for rotation period

Epic: none
Release note: None

#### storageconfig: encryption YAML support and validation

Support yaml for the encryption options and add validation.

Epic: none
Release note: None

154682: opt: move some assignment cast execbuilder logic to init-time r=mgartner a=mgartner

#### opt: move some assignment cast execbuilder logic to init-time

Lookups for the `crdb_internal.assignment_cast` function properties and
overload are now performed once at init-time instead of for every
assignment cast built at execbuild-time.

Release note: None

#### sql/sem/tree: remove always-nil error return from GetBuiltinFuncDefinition

Release note: None


154743: server: check for isMeta1Leaseholder later in loop r=arulajmani a=stevendanna

This checks for isMeta1Leaseholder after first checking the version, allowing the version checking goroutine to exit early for the common case of being on the most recent version.

Epic: none
Release note: None

154745: ui: update txn diagnostics history UI text and docs link r=jasonlmfong a=dhartunian

Point users to new docs link and builtin.

Epic: CRDB-53541
Resolves: CRDB-55077

Release note: None

Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: Jeff Swenson <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: David Hartunian <[email protected]>

Author: 6 people

pkg/base/BUILD.bazel

@@ -74,7 +74,6 @@ go_test(
         "//pkg/util/leaktest",
         "//pkg/util/log",
         "//pkg/util/uuid",
-        "@com_github_cockroachdb_datadriven//:datadriven",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_davecgh_go_spew//spew",
         "@com_github_stretchr_testify//require",

pkg/base/config_test.go

@@ -6,19 +6,15 @@
 package base_test
 
 import (
-	"bytes"
 	"fmt"
 	"math"
-	"strings"
 	"testing"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
-	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/echotest"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
-	"github.com/cockroachdb/datadriven"
 	"github.com/davecgh/go-spew/spew"
 	"github.com/stretchr/testify/require"
 )
@@ -211,20 +207,3 @@ func TestRaftMaxInflightBytes(t *testing.T) {
 		})
 	}
 }
-
-func TestWALFailoverConfigRoundtrip(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-
-	datadriven.RunTest(t, datapathutils.TestDataPath(t, "wal-failover-config"), func(t *testing.T, d *datadriven.TestData) string {
-		var buf bytes.Buffer
-		for _, l := range strings.Split(d.Input, "\n") {
-			var cfg storageconfig.WALFailover
-			if err := cfg.Set(l); err != nil {
-				fmt.Fprintf(&buf, "err: %s\n", err)
-				continue
-			}
-			fmt.Fprintln(&buf, cfg.String())
-		}
-		return buf.String()
-	})
-}

pkg/base/store_spec.go

@@ -333,64 +333,3 @@ func (ssl *StoreSpecList) Set(value string) error {
 	}
 	return nil
 }
-
-// PopulateWithEncryptionOpts iterates through the EncryptionSpecList and looks
-// for matching paths in the StoreSpecList and WAL failover config. Any
-// unmatched EncryptionSpec causes an error.
-func PopulateWithEncryptionOpts(
-	storeSpecs StoreSpecList,
-	walFailoverConfig *storageconfig.WALFailover,
-	encryptionSpecs storageconfig.EncryptionSpecList,
-) error {
-	for _, es := range encryptionSpecs.Specs {
-		var storeMatched bool
-		for i := range storeSpecs.Specs {
-			if !es.PathMatches(storeSpecs.Specs[i].Path) {
-				continue
-			}
-
-			// Found a matching path.
-			if storeSpecs.Specs[i].EncryptionOptions != nil {
-				return fmt.Errorf("store with path %s already has an encryption setting",
-					storeSpecs.Specs[i].Path)
-			}
-
-			storeSpecs.Specs[i].EncryptionOptions = &es.Options
-			storeMatched = true
-			break
-		}
-		pathMatched, err := maybeSetExternalPathEncryption(&walFailoverConfig.Path, es)
-		if err != nil {
-			return err
-		}
-		prevPathMatched, err := maybeSetExternalPathEncryption(&walFailoverConfig.PrevPath, es)
-		if err != nil {
-			return err
-		}
-		if !storeMatched && !pathMatched && !prevPathMatched {
-			return fmt.Errorf("no usage of path %s found for encryption setting: %v", es.Path, es)
-		}
-	}
-	return nil
-}
-
-// maybeSetExternalPathEncryption updates an ExternalPath to contain the provided
-// encryption options if the path matches. The ExternalPath must not already have
-// an encryption setting.
-func maybeSetExternalPathEncryption(
-	externalPath *storageconfig.ExternalPath, es storageconfig.StoreEncryptionSpec,
-) (found bool, err error) {
-	if !externalPath.IsSet() || !es.PathMatches(externalPath.Path) {
-		return false, nil
-	}
-	// NB: The external paths WALFailoverConfig.Path and
-	// WALFailoverConfig.PrevPath are only ever set in single-store
-	// configurations. In multi-store with among-stores failover mode, these
-	// will be empty (so we won't encounter the same path twice).
-	if externalPath.Encryption != nil {
-		return false, fmt.Errorf("WAL failover path %s already has an encryption setting",
-			externalPath.Path)
-	}
-	externalPath.Encryption = &es.Options
-	return true, nil
-}

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -61,16 +61,16 @@ exp,benchmark
 8,Grant/grant_all_on_3_tables
 12,GrantRole/grant_1_role
 16,GrantRole/grant_2_roles
-12-15,Jobs/cancel_job
+9-12,Jobs/cancel_job
 3,Jobs/crdb_internal.system_jobs
 3-5,Jobs/jobs_page_default
 3,Jobs/jobs_page_latest_50
 3,Jobs/jobs_page_type_filtered
 1-3,Jobs/jobs_page_type_filtered_no_matches
 2,Jobs/non_admin_crdb_internal.system_jobs
 2,Jobs/non_admin_show_jobs
-12-15,Jobs/pause_job
-12-15,Jobs/resume_job
+9-12,Jobs/pause_job
+9-12,Jobs/resume_job
 3,Jobs/show_job
 3-5,Jobs/show_jobs
 3,ORMQueries/activerecord_type_introspection_query

pkg/ccl/storageccl/engineccl/encrypted_fs.go

@@ -8,6 +8,7 @@ package engineccl
 import (
 	"context"
 	"fmt"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
@@ -319,7 +320,7 @@ func newEncryptedEnv(
 	dataKeyManager := &DataKeyManager{
 		fs:             storeFS,
 		dbDir:          dbDir,
-		rotationPeriod: options.DataKeyRotationPeriod,
+		rotationPeriod: int64((options.RotationPeriod + time.Second - 1) / time.Second),
 		readOnly:       readOnly,
 	}
 	if err := dataKeyManager.Load(context.TODO()); err != nil {

pkg/ccl/storageccl/engineccl/encrypted_fs_test.go

@@ -16,6 +16,7 @@ import (
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -242,7 +243,7 @@ func TestPebbleEncryption(t *testing.T) {
 			CurrentKey: "16.key",
 			OldKey:     "plain",
 		},
-		DataKeyRotationPeriod: 1000, // arbitrary seconds
+		RotationPeriod: time.Hour,
 	}
 
 	func() {
@@ -385,7 +386,7 @@ func TestPebbleEncryption2(t *testing.T) {
 				CurrentKey: encKeyFile,
 				OldKey:     oldEncFileKey,
 			},
-			DataKeyRotationPeriod: 1000,
+			RotationPeriod: time.Hour,
 		}
 
 		// Initialize the filesystem env.
@@ -580,7 +581,7 @@ func makeEncryptedTestFS(t *testing.T, errorProb float64, errorRand *rand.Rand)
 	//
 	// TODO(sumeer): Do deterministic data key rotation. Inject kmTimeNow and
 	// operations that advance time.
-	encOptions.DataKeyRotationPeriod = 100000
+	encOptions.RotationPeriod = 100000 * time.Second
 	etfs := &encryptedTestFS{
 		mem:        mem,
 		encOptions: &encOptions,

pkg/cli/context.go

@@ -720,23 +720,3 @@ var userfileCtx struct {
 func setUserfileContextDefaults() {
 	userfileCtx.recursive = false
 }
-
-// GetServerCfgStores provides direct public access to the StoreSpecList inside
-// serverCfg. This is used by CCL code to populate some fields.
-//
-// WARNING: consider very carefully whether you should be using this.
-// If you are not writing CCL code that performs command-line flag
-// parsing, you probably should not be using this.
-func GetServerCfgStores() base.StoreSpecList {
-	return serverCfg.Stores
-}
-
-// GetWALFailoverConfig provides direct public access to the WALFailoverConfig
-// inside serverCfg. This is used by CCL code to populate some fields.
-//
-// WARNING: consider very carefully whether you should be using this.
-// If you are not writing CCL code that performs command-line flag
-// parsing, you probably should not be using this.
-func GetWALFailoverConfig() *storageconfig.WALFailover {
-	return &serverCfg.StorageConfig.WALFailover
-}

pkg/cli/debug_ear.go

@@ -20,7 +20,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
-	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -169,7 +168,7 @@ with their env type and encryption settings (if applicable).
 // fillEncryptionOptionsForStore fills the EnvConfig fields
 // based on the --enterprise-encryption flag value.
 func fillEncryptionOptionsForStore(dir string, cfg *fs.EnvConfig) error {
-	opts, err := storageconfig.EncryptionOptionsForStore(dir, encryptionSpecs)
+	opts, err := encryptionOptionsForStore(dir, encryptionSpecs)
 	if err != nil {
 		return err
 	}

pkg/cli/ear_test.go

@@ -18,7 +18,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
-	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -44,7 +43,7 @@ func TestDecrypt(t *testing.T) {
 
 	// Spin up a new encrypted store.
 	encSpecStr := fmt.Sprintf("path=%s,key=%s,old-key=plain", dir, keyPath)
-	encSpec, err := storageconfig.NewStoreEncryptionSpec(encSpecStr)
+	encSpec, err := parseStoreEncryptionSpec(encSpecStr)
 	require.NoError(t, err)
 	encOpts := &encSpec.Options
 
@@ -126,7 +125,7 @@ func TestList(t *testing.T) {
 
 	// Spin up a new encrypted store.
 	encSpecStr := fmt.Sprintf("path=%s,key=%s,old-key=plain", dir, keyPath)
-	encSpec, err := storageconfig.NewStoreEncryptionSpec(encSpecStr)
+	encSpec, err := parseStoreEncryptionSpec(encSpecStr)
 	require.NoError(t, err)
 	encOpts := &encSpec.Options
 	env, err := fs.InitEnv(ctx, vfs.Default, dir, fs.EnvConfig{EncryptionOptions: encOpts}, nil /* statsCollector */)

pkg/cli/flags.go

@@ -62,7 +62,7 @@ var storeSpecs base.StoreSpecList
 var goMemLimit int64
 var tenantIDFile string
 var localityFile string
-var encryptionSpecs storageconfig.EncryptionSpecList
+var encryptionSpecs encryptionSpecList
 
 // initPreFlagsDefaults initializes the values of the global variables
 // defined above.
@@ -431,7 +431,11 @@ func init() {
 
 		// Add a new pre-run command to match encryption specs to store specs.
 		AddPersistentPreRunE(cmd, func(cmd *cobra.Command, _ []string) error {
-			return populateStoreSpecsEncryption()
+			return populateWithEncryptionOpts(
+				serverCfg.Stores,
+				&serverCfg.StorageConfig.WALFailover,
+				encryptionSpecs,
+			)
 		})
 	}
 
@@ -525,7 +529,7 @@ func init() {
 		cliflagcfg.StringFlag(f, &deprecatedStorageEngine, cliflags.StorageEngine)
 		_ = pf.MarkHidden(cliflags.StorageEngine.Name)
 
-		cliflagcfg.VarFlag(f, &serverCfg.StorageConfig.WALFailover, cliflags.WALFailover)
+		cliflagcfg.VarFlag(f, &walFailoverWrapper{cfg: &serverCfg.StorageConfig.WALFailover}, cliflags.WALFailover)
 		// TODO(storage): Consider combining the uri and cache manual settings.
 		// Alternatively remove the ability to configure shared storage without
 		// passing a bootstrap configuration file.
@@ -1485,17 +1489,6 @@ func mtStartSQLFlagsInit(cmd *cobra.Command) error {
 	return nil
 }
 
-// populateStoreSpecsEncryption is a PreRun hook that matches store encryption
-// specs with the parsed stores and populates some fields in the StoreSpec and
-// WAL failover config.
-func populateStoreSpecsEncryption() error {
-	return base.PopulateWithEncryptionOpts(
-		GetServerCfgStores(),
-		GetWALFailoverConfig(),
-		encryptionSpecs,
-	)
-}
-
 // sizeFlagVal is a pflag.Value wrapper for storageconfig.Size. It can be
 // set to an absolute bytes amount or a percentage.
 type sizeFlagVal struct {