Merge #153841 #153846

153841: base: clean up TempStorageConfig r=RaduBerinde a=RaduBerinde

#### server: clean up default TempStorageConfig

We currently create an invalid initial `TempStorageConfig` (the `Path`
isn't set). In practice, this config is overridden by
`cli.initTempStorageConfig()`.

We clean this up by leaving the initial `TempStorageConfig` empty.

Epic: none
Release note: None

#### base: clean up TempStorageConfig

`TempStorageConfig` is very confusing. It is very hard to follow the
exact mechanics of when `InMemory` differs from `StoreSpec.InMemory`,
how the paths differ, etc.

In this change:
 - we replace the `StoreSpec` field with the `EncryptionOptions` and
   the `temp-dirs-record.txt` file path.
 - we clean up and clarify the initialization code.
 - we improve the documentation for the `--temp-dir` flag.

Epic: none
Release note: None

153846: randgen: fix recent regression in tuple generation r=yuzefovich a=yuzefovich

In 1a751b5 we made a slight change to how we generate random tuples. Namely, we no longer call `ResolvedType` on each tuple element. This can be problematic since we might have generated an element with some Any modifier, which could later lead to "illegal AppendFloat/FormatFloat bitSize" panic when operating with floats.

This commit simply reverts the relevant changes from the commit mentioned above.

This required some minor adjustments to `TestCanonical` since there tuple with NULL elements will have their resolved type differ from the originally requested one.

Fixes: #153805.
Release note: None

Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: 3 people

pkg/base/config.go

@@ -17,7 +17,9 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
 )
@@ -904,19 +906,25 @@ type TempStorageConfig struct {
 	// InMemory specifies whether the temporary storage will remain
 	// in-memory or occupy a temporary subdirectory on-disk.
 	InMemory bool
-	// Path is the filepath of the temporary subdirectory created for
-	// the temp storage.
+	// Path is the filepath of the temporary subdirectory created for the temp
+	// storage. Empty if InMemory is true.
 	Path string
 	// Mon will be used by the temp storage to register all its capacity requests.
 	// It can be used to limit the disk or memory that temp storage is allowed to
 	// use. If InMemory is set, than this has to be a memory monitor; otherwise it
 	// has to be a disk monitor.
 	Mon *mon.BytesMonitor
-	// Spec stores the StoreSpec this TempStorageConfig will use.
-	Spec StoreSpec
+	// Encryption is set if encryption is enabled. We use the same encryption
+	// options as the store we chose for temp storage.
+	Encryption *storageconfig.EncryptionOptions
 	// Settings stores the cluster.Settings this TempStoreConfig will use. Must
 	// not be nil.
 	Settings *cluster.Settings
+	// If set, TempDirsRecordPath is the path to a temp-dirs-record.txt file in
+	// one of the stores (see server.TempDirsRecordFilename). Used when we create
+	// a new temporary storage directory for a new shared-process tenant. Empty if
+	// InMemory is false.
+	TempDirsRecordPath string
 }
 
 // ExternalIODirConfig describes various configuration options pertaining
@@ -948,32 +956,29 @@ type ExternalIODirConfig struct {
 	EnableNonAdminImplicitAndArbitraryOutbound bool
 }
 
-// TempStorageConfigFromEnv creates a TempStorageConfig.
-// If parentDir is not specified and the specified store is in-memory,
-// then the temp storage will also be in-memory.
-func TempStorageConfigFromEnv(
-	ctx context.Context,
-	st *cluster.Settings,
-	useStore StoreSpec,
-	parentDir string,
-	maxSizeBytes int64,
-) TempStorageConfig {
-	inMem := parentDir == "" && useStore.InMemory
-	return newTempStorageConfig(ctx, st, inMem, useStore, maxSizeBytes)
-}
-
 // InheritTempStorageConfig creates a new TempStorageConfig using the
 // configuration of the given TempStorageConfig. It assumes the given
 // TempStorageConfig has been fully initialized.
 func InheritTempStorageConfig(
 	ctx context.Context, st *cluster.Settings, parentConfig TempStorageConfig,
 ) TempStorageConfig {
-	return newTempStorageConfig(ctx, st, parentConfig.InMemory, parentConfig.Spec, parentConfig.Mon.Limit())
+	return NewTempStorageConfig(ctx, st, parentConfig.InMemory, parentConfig.Path, parentConfig.Encryption, parentConfig.Mon.Limit(), parentConfig.TempDirsRecordPath)
 }
 
-func newTempStorageConfig(
-	ctx context.Context, st *cluster.Settings, inMemory bool, useStore StoreSpec, maxSizeBytes int64,
+// NewTempStorageConfig creates a new TempStorageConfig.
+// The path should be empty iff inMemory is true.
+func NewTempStorageConfig(
+	ctx context.Context,
+	st *cluster.Settings,
+	inMemory bool,
+	path string,
+	encryption *storageconfig.EncryptionOptions,
+	maxSizeBytes int64,
+	tempDirsRecordPath string,
 ) TempStorageConfig {
+	if inMemory != (path == "") {
+		log.Dev.Fatalf(ctx, "inMemory (%t) must be true iff path is empty (%q)", inMemory, path)
+	}
 	var monitorName mon.Name
 	if inMemory {
 		monitorName = mon.MakeName("in-mem temp storage")
@@ -988,9 +993,11 @@ func newTempStorageConfig(
 	})
 	monitor.Start(ctx, nil /* pool */, mon.NewStandaloneBudget(maxSizeBytes))
 	return TempStorageConfig{
-		InMemory: inMemory,
-		Mon:      monitor,
-		Spec:     useStore,
-		Settings: st,
+		InMemory:           inMemory,
+		Path:               path,
+		Mon:                monitor,
+		Encryption:         encryption,
+		Settings:           st,
+		TempDirsRecordPath: tempDirsRecordPath,
 	}
 }

pkg/base/test_server_args.go

@@ -625,7 +625,6 @@ func DefaultTestTempStorageConfigWithSize(
 	return TempStorageConfig{
 		InMemory: true,
 		Mon:      monitor,
-		Spec:     DefaultTestStoreSpec,
 		Settings: st,
 	}
 }

pkg/cli/cliflags/flags.go

@@ -1197,10 +1197,14 @@ Verbose output.`,
 	TempDir = FlagInfo{
 		Name: "temp-dir",
 		Description: `
-The parent directory path where a temporary subdirectory will be created to be used for temporary files.
-This path must exist or the node will not start.
-The temporary subdirectory is used primarily as working memory for distributed computations
-and CSV importing.
+The parent directory path where a temporary subdirectory will be created to be
+used for temporary files. This path must exist or the node will not start.
+
+The encryption from one of the stores is used with the temporary directory
+(specifically, the first store that is on-disk and encrypted).
+
+The temporary subdirectory is used primarily as working memory for distributed
+computations and CSV importing.
 For example, the following will generate an arbitrary, temporary subdirectory
 "/mnt/ssd01/temp/cockroach-temp<NUMBER>":
 <PRE>
@@ -1209,7 +1213,8 @@ For example, the following will generate an arbitrary, temporary subdirectory
 
 </PRE>
 If this flag is unspecified, the temporary subdirectory will be located under
-the root of the first store.`,
+the root of one of the stores (with preference for on-disk, encrypted stores).
+`,
 	}
 
 	ExternalIODir = FlagInfo{

pkg/cli/start.go

@@ -264,40 +264,44 @@ func initTempStorageConfig(
 		}
 	}
 
-	// Initialize a base.TempStorageConfig based on first store's spec and
-	// cli flags.
-	tempStorageConfig := base.TempStorageConfigFromEnv(
-		ctx,
-		st,
-		useStore,
-		startCtx.tempDir,
-		tempStorageMaxSizeBytes,
-	)
+	// If all stores are in-memory and no temp dir was specified, the temp
+	// store will also be in memory. This is a testing scenario.
+	if startCtx.tempDir == "" && useStore.InMemory {
+		return base.NewTempStorageConfig(
+			ctx,
+			st,
+			true, // inMem
+			"",   // path
+			useStore.EncryptionOptions,
+			tempStorageMaxSizeBytes,
+			"", // tempDirsRecordPath
+		), nil
+	}
 
 	// Set temp directory to first store's path if the temp storage is not
 	// in memory.
-	tempDir := startCtx.tempDir
-	if tempDir == "" && !tempStorageConfig.InMemory {
-		tempDir = useStore.Path
+	parentDir := startCtx.tempDir
+	if parentDir == "" {
+		parentDir = useStore.Path
 	}
 
-	tmpPath, unlockDirFn, err := fs.CreateTempDir(tempDir, server.TempDirPrefix)
+	tmpPath, unlockDirFn, err := fs.CreateTempDir(parentDir, server.TempDirPrefix)
 	if err != nil {
 		return base.TempStorageConfig{}, errors.Wrap(err, "could not create temporary directory for temp storage")
 	}
-	tempStorageConfig.Path = tmpPath
 
+	recordPath := ""
 	if useStore.InMemory {
 		stopper.AddCloser(stop.CloserFn(func() {
 			unlockDirFn()
 			// Remove the temp directory directly since there is no record file.
-			if err := os.RemoveAll(tempStorageConfig.Path); err != nil {
+			if err := os.RemoveAll(tmpPath); err != nil {
 				log.Dev.Errorf(ctx, "could not remove temporary store directory: %v", err.Error())
 			}
 		}))
 	} else {
-		recordPath := filepath.Join(useStore.Path, server.TempDirsRecordFilename)
-		if err := fs.RecordTempDir(recordPath, tempStorageConfig.Path); err != nil {
+		recordPath = filepath.Join(useStore.Path, server.TempDirsRecordFilename)
+		if err := fs.RecordTempDir(recordPath, tmpPath); err != nil {
 			return base.TempStorageConfig{}, errors.Wrapf(
 				err,
 				"could not record temporary directory path to record file: %s",
@@ -312,7 +316,18 @@ func initTempStorageConfig(
 			}
 		}))
 	}
-	return tempStorageConfig, nil
+
+	// Initialize a base.TempStorageConfig based on first store's spec and
+	// cli flags.
+	return base.NewTempStorageConfig(
+		ctx,
+		st,
+		false, // inMem
+		tmpPath,
+		useStore.EncryptionOptions,
+		tempStorageMaxSizeBytes,
+		recordPath,
+	), nil
 }
 
 type newServerFn func(ctx context.Context, serverCfg server.Config, stopper *stop.Stopper) (serverctl.ServerStartupInterface, error)

pkg/kv/kvclient/kvstreamer/results_buffer_test.go

@@ -35,12 +35,7 @@ func TestInOrderResultsBuffer(t *testing.T) {
 	ctx := context.Background()
 	rng, _ := randutil.NewTestRand()
 	st := cluster.MakeTestingClusterSettings()
-	tempEngine, _, err := storage.NewTempEngine(
-		ctx,
-		base.DefaultTestTempStorageConfig(st),
-		base.DefaultTestStoreSpec,
-		nil, /* statsCollector */
-	)
+	tempEngine, _, err := storage.NewTempEngine(ctx, base.DefaultTestTempStorageConfig(st), nil /* statsCollector */)
 	require.NoError(t, err)
 	defer tempEngine.Close()
 	memMonitor := mon.NewMonitor(mon.Options{

pkg/server/config.go

@@ -608,9 +608,9 @@ func SetOpenFileLimitForOneStore() (uint64, error) {
 
 // MakeConfig returns a Config for the system tenant with default values.
 func MakeConfig(ctx context.Context, st *cluster.Settings) Config {
-	storeSpec, tempStorageCfg := makeStorageCfg(ctx, st)
+	storeSpec := makeStorageCfg(ctx, st)
 	sqlCfg := MakeSQLConfig(roachpb.SystemTenantID,
-		roachpb.TenantName(roachpb.SystemTenantID.String()), tempStorageCfg)
+		roachpb.TenantName(roachpb.SystemTenantID.String()), base.TempStorageConfig{})
 	tr := tracing.NewTracerWithOpt(ctx, tracing.WithClusterSettings(&st.SV))
 	baseCfg := MakeBaseConfig(st, tr, storeSpec)
 	kvCfg := MakeKVConfig()
@@ -628,23 +628,19 @@ func MakeConfig(ctx context.Context, st *cluster.Settings) Config {
 // preserving the base.Config reference. Enables running tests
 // multiple times.
 func (cfg *Config) SetDefaults(ctx context.Context, st *cluster.Settings) {
-	storeSpec, tempStorageCfg := makeStorageCfg(ctx, st)
-	cfg.SQLConfig.SetDefaults(tempStorageCfg)
+	storeSpec := makeStorageCfg(ctx, st)
+	cfg.SQLConfig.SetDefaults(base.TempStorageConfig{})
 	cfg.KVConfig.SetDefaults()
 	tr := tracing.NewTracerWithOpt(ctx, tracing.WithClusterSettings(&st.SV))
 	cfg.BaseConfig.SetDefaults(st, tr, storeSpec)
 }
 
-func makeStorageCfg(
-	ctx context.Context, st *cluster.Settings,
-) (base.StoreSpec, base.TempStorageConfig) {
+func makeStorageCfg(ctx context.Context, st *cluster.Settings) base.StoreSpec {
 	storeSpec, err := base.NewStoreSpec(DefaultStorePath)
 	if err != nil {
 		panic(err)
 	}
-	tempStorageCfg := base.TempStorageConfigFromEnv(
-		ctx, st, storeSpec, "" /* parentDir */, base.DefaultTempStorageMaxSizeBytes)
-	return storeSpec, tempStorageCfg
+	return storeSpec
 }
 
 // String implements the fmt.Stringer interface.

pkg/server/server_controller_new_server.go

@@ -329,17 +329,19 @@ func makeSharedProcessTenantServerConfig(
 
 	tempStorageCfg := base.InheritTempStorageConfig(ctx, st, kvServerCfg.SQLConfig.TempStorageConfig)
 	if !tempStorageCfg.InMemory {
-		useStore := tempStorageCfg.Spec
-		// TODO(knz): Make tempDir configurable.
-		tempDir := useStore.Path
-		var unlockDirFn func()
-		if tempStorageCfg.Path, unlockDirFn, err = fs.CreateTempDir(tempDir, TempDirPrefix); err != nil {
+		// We create another temp directory alongside the existing one.
+		parentDir := filepath.Dir(tempStorageCfg.Path)
+		if parentDir == "" {
+			return BaseConfig{}, SQLConfig{}, errors.Newf("invalid temp storage config path %q", tempStorageCfg.Path)
+		}
+		tmpDir, unlockDirFn, err := fs.CreateTempDir(parentDir, TempDirPrefix)
+		if err != nil {
 			return BaseConfig{}, SQLConfig{}, errors.Wrap(err, "could not create temporary directory for temp storage")
 		}
 		stopper.AddCloser(stop.CloserFn(unlockDirFn))
-		if useStore.Path != "" {
-			recordPath := filepath.Join(useStore.Path, TempDirsRecordFilename)
-			if err := fs.RecordTempDir(recordPath, tempStorageCfg.Path); err != nil {
+		tempStorageCfg.Path = tmpDir
+		if tempStorageCfg.TempDirsRecordPath != "" {
+			if err := fs.RecordTempDir(tempStorageCfg.TempDirsRecordPath, tempStorageCfg.Path); err != nil {
 				return BaseConfig{}, SQLConfig{}, errors.Wrap(err, "could not record temp dir")
 			}
 		}

pkg/server/server_sql.go

@@ -725,8 +725,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 
 	// Set up the DistSQL temp engine.
 
-	useStoreSpec := cfg.TempStorageConfig.Spec
-	tempEngine, tempFS, err := storage.NewTempEngine(ctx, cfg.TempStorageConfig, useStoreSpec, cfg.DiskWriteStats)
+	tempEngine, tempFS, err := storage.NewTempEngine(ctx, cfg.TempStorageConfig, cfg.DiskWriteStats)
 	if err != nil {
 		return nil, errors.Wrap(err, "creating temp storage")
 	}

pkg/sql/colflow/draining_test.go

@@ -43,7 +43,7 @@ func TestDrainingAfterRemoteError(t *testing.T) {
 	diskMonitor.Start(ctx, nil /* pool */, mon.NewStandaloneBudget(1))
 
 	// Set up a two node cluster.
-	tempStorageConfig := base.TempStorageConfig{InMemory: true, Mon: diskMonitor, Settings: st, Spec: base.DefaultTestStoreSpec}
+	tempStorageConfig := base.TempStorageConfig{InMemory: true, Mon: diskMonitor, Settings: st}
 	args := base.TestClusterArgs{
 		ServerArgs: base.TestServerArgs{
 			Settings:          st,

pkg/sql/conn_executor_internal_test.go

@@ -280,7 +280,7 @@ func startConnExecutor(
 	nodeID := base.TestingIDContainer
 	distSQLMetrics := execinfra.MakeDistSQLMetrics(time.Hour /* histogramWindow */)
 	gw := gossip.MakeOptionalGossip(nil)
-	tempEngine, tempFS, err := storage.NewTempEngine(ctx, base.DefaultTestTempStorageConfig(st), base.DefaultTestStoreSpec, nil /* statsCollector */)
+	tempEngine, tempFS, err := storage.NewTempEngine(ctx, base.DefaultTestTempStorageConfig(st), nil /* statsCollector */)
 	if err != nil {
 		return nil, nil, nil, nil, nil, err
 	}