db: clean up runDBDefineCmd

Consolidate the two variants of this function: we create a new `MemFS`
only if `opts.FS` is not set (which is much more intuitive).

Note that we no longer call `EnsureDefaults()` before modifying the
options passed to the `define` directive. This changes certain values
that have defaults that depends on other values, like
`FlushSplitBytes`. We add a `flush-split-byte` argument to correct
this.

Author: RaduBerinde

compaction_picker_test.go

@@ -1437,6 +1437,12 @@ type pausableCleaner struct {
 	cleaner Cleaner
 }
 
+func newPausableCleaner(cleaner Cleaner) *pausableCleaner {
+	p := &pausableCleaner{cleaner: cleaner}
+	p.cond.L = &p.mu
+	return p
+}
+
 func (c *pausableCleaner) Clean(fs vfs.FS, fileType base.FileType, path string) error {
 	c.mu.Lock()
 	defer c.mu.Unlock()
@@ -1460,9 +1466,7 @@ func (c *pausableCleaner) resume() {
 }
 
 func TestCompactionPickerScores(t *testing.T) {
-	fs := vfs.NewMem()
-	cleaner := pausableCleaner{cleaner: DeleteCleaner{}}
-	cleaner.cond.L = &cleaner.mu
+	cleaner := newPausableCleaner(DeleteCleaner{})
 
 	var d *DB
 	defer func() {
@@ -1497,11 +1501,10 @@ func TestCompactionPickerScores(t *testing.T) {
 				cleaner.pause()
 			}
 			opts := &Options{
-				Cleaner:                     &cleaner,
+				Cleaner:                     cleaner,
 				Comparer:                    testkeys.Comparer,
 				DisableAutomaticCompactions: true,
 				FormatMajorVersion:          FormatNewest,
-				FS:                          fs,
 				Logger:                      testutils.Logger{T: t},
 			}
 			opts.Experimental.CompactionScheduler = func() CompactionScheduler {

compaction_test.go

@@ -1452,31 +1452,38 @@ func TestCompaction(t *testing.T) {
 		"file_boundaries_delsized": {
 			minVersion: FormatDeleteSizedAndObsolete,
 			maxVersion: FormatFlushableIngestExcises,
+			cmp:        DefaultComparer,
 		},
 		"set_with_del_sstable_Pebblev4": {
 			minVersion: FormatDeleteSizedAndObsolete,
 			maxVersion: FormatFlushableIngestExcises,
+			cmp:        DefaultComparer,
 		},
 		"multilevel": {
 			minVersion: FormatMinSupported,
 			maxVersion: internalFormatNewest,
+			cmp:        DefaultComparer,
 		},
 		"set_with_del_sstable_Pebblev5": {
 			minVersion: FormatColumnarBlocks,
 			maxVersion: FormatColumnarBlocks,
+			cmp:        DefaultComparer,
 		},
 		"set_with_del_sstable_Pebblev6": {
 			minVersion: FormatTableFormatV6,
 			maxVersion: FormatTableFormatV6,
+			cmp:        DefaultComparer,
 		},
 		"set_with_del_sstable_Pebblev7": {
 			minVersion: formatFooterAttributes,
 			maxVersion: formatFooterAttributes,
+			cmp:        DefaultComparer,
 		},
 		"value_separation": {
 			minVersion: FormatValueSeparation,
 			maxVersion: FormatValueSeparation,
 			verbose:    true,
+			cmp:        DefaultComparer,
 		},
 		"mvcc_garbage_blob": {
 			minVersion: FormatValueSeparation,
@@ -1489,21 +1496,25 @@ func TestCompaction(t *testing.T) {
 			// since the test prints the compaction log which includes file sizes.
 			minVersion: formatDeprecatedExperimentalValueSeparation,
 			maxVersion: formatDeprecatedExperimentalValueSeparation,
+			cmp:        DefaultComparer,
 		},
 		"compaction_cancellation": {
 			// Run at a specific version, so that a single sstable format is used,
 			// since the test prints the compaction log which includes file sizes.
 			minVersion: formatDeprecatedExperimentalValueSeparation,
 			maxVersion: formatDeprecatedExperimentalValueSeparation,
+			cmp:        DefaultComparer,
 		},
 		"l0_to_lbase_compaction": {
 			minVersion: formatDeprecatedExperimentalValueSeparation,
 			maxVersion: formatDeprecatedExperimentalValueSeparation,
+			cmp:        DefaultComparer,
 		},
 		"backing_value_size": {
 			minVersion: FormatBackingValueSize,
 			maxVersion: FormatBackingValueSize,
 			verbose:    true,
+			cmp:        DefaultComparer,
 		},
 	}
 	datadriven.Walk(t, "testdata/compaction", func(t *testing.T, path string) {
@@ -2767,21 +2778,6 @@ func TestMarkedForCompaction(t *testing.T) {
 	if testing.Verbose() {
 		eventListener = TeeEventListener(eventListener, MakeLoggingEventListener(base.DefaultLogger))
 	}
-	mkOpts := func() *Options {
-		opts := &Options{
-			FS:                          vfs.NewMem(),
-			DebugCheck:                  DebugCheckLevels,
-			DisableAutomaticCompactions: true,
-			FormatMajorVersion:          internalFormatNewest,
-			EventListener:               &eventListener,
-			Logger:                      testutils.Logger{T: t},
-		}
-		opts.Experimental.CompactionScheduler = func() CompactionScheduler {
-			return NewConcurrencyLimitSchedulerWithNoPeriodicGrantingForTest()
-		}
-		opts.WithFSDefaults()
-		return opts
-	}
 
 	var opts *Options
 	var d *DB
@@ -2810,11 +2806,18 @@ func TestMarkedForCompaction(t *testing.T) {
 
 		case "define":
 			if d != nil {
-				if err := d.Close(); err != nil {
-					return err.Error()
-				}
+				require.NoError(t, d.Close())
+			}
+			opts = &Options{
+				DebugCheck:                  DebugCheckLevels,
+				DisableAutomaticCompactions: true,
+				FormatMajorVersion:          internalFormatNewest,
+				EventListener:               &eventListener,
+				Logger:                      testutils.Logger{T: t},
+			}
+			opts.Experimental.CompactionScheduler = func() CompactionScheduler {
+				return NewConcurrencyLimitSchedulerWithNoPeriodicGrantingForTest()
 			}
-			opts = mkOpts()
 			var err error
 			if d, err = runDBDefineCmd(td, opts); err != nil {
 				return err.Error()

data_test.go

@@ -838,22 +838,16 @@ func runCompactCmd(td *datadriven.TestData, d *DB) error {
 // The resulting version edit is then manipulated to write the files
 // to the indicated level.
 //
-// Because of it's low-level manipulation, runDBDefineCmd does allow the
+// Because of its low-level manipulation, runDBDefineCmd does allow the
 // creation of invalid database states. If opts.DebugCheck is set, the
 // level checker should detect the invalid state.
+//
+// The given opts are modified if the test directive contains any options. If
+// opts.FS is nil, it is initialized to a new MemFS.
 func runDBDefineCmd(td *datadriven.TestData, opts *Options) (*DB, error) {
-	if opts == nil {
-		opts = &Options{}
+	if opts.FS == nil {
+		opts.FS = vfs.NewMem()
 	}
-	opts.EnsureDefaults()
-	opts.FS = vfs.NewMem()
-	return runDBDefineCmdReuseFS(td, opts)
-}
-
-// runDBDefineCmdReuseFS is like runDBDefineCmd, but does not set opts.FS, expecting
-// the caller to have set an appropriate FS already.
-func runDBDefineCmdReuseFS(td *datadriven.TestData, opts *Options) (*DB, error) {
-	opts.EnsureDefaults()
 	if err := parseDBOptionsArgs(opts, td.CmdArgs); err != nil {
 		return nil, err
 	}
@@ -1005,13 +999,18 @@ func runDBDefineCmdReuseFS(td *datadriven.TestData, opts *Options) (*DB, error)
 		if len(parts) != 2 {
 			return nil, errors.Errorf("malformed table spec: %s", s)
 		}
-		m := (&manifest.TableMetadata{}).ExtendPointKeyBounds(
-			opts.Comparer.Compare,
+		var m manifest.TableMetadata
+		cmp := base.DefaultComparer.Compare
+		if opts.Comparer != nil {
+			cmp = opts.Comparer.Compare
+		}
+		m.ExtendPointKeyBounds(
+			cmp,
 			InternalKey{UserKey: []byte(parts[0])},
 			InternalKey{UserKey: []byte(parts[1])},
 		)
 		m.InitPhysicalBacking()
-		return m, nil
+		return &m, nil
 	}
 
 	// Example, compact: a-c.
@@ -1815,11 +1814,18 @@ func parseDBOptionsArgs(opts *Options, args []datadriven.CmdArg) error {
 				opts.TargetFileSizes[i] = size
 			}
 			// Set the remaining file sizes. Normally, EnsureDefaults() would do that
-			// for us but it was already called and the target file sizes for all
+			// for us but if it was already called the target file sizes for all
 			// levels are now set to the defaults.
 			for i := len(cmdArg.Vals); i < len(opts.TargetFileSizes); i++ {
 				opts.TargetFileSizes[i] = opts.TargetFileSizes[i-1] * 2
 			}
+		case "flush-split-bytes":
+			flushSplitBytes, err := strconv.ParseInt(cmdArg.Vals[0], 10, 64)
+			if err != nil {
+				return err
+			}
+			opts.FlushSplitBytes = flushSplitBytes
+
 		case "value-separation":
 			var policy ValueSeparationPolicy
 			if len(cmdArg.Vals) == 1 && cmdArg.Vals[0] == "off" || cmdArg.Vals[0] == "disabled" {

db_test.go

@@ -2272,7 +2272,7 @@ func TestDeterminism(t *testing.T) {
 					}
 					opts.Experimental.IngestSplit = func() bool { return rand.IntN(2) == 1 }
 					var err error
-					if d, err = runDBDefineCmdReuseFS(td, opts); err != nil {
+					if d, err = runDBDefineCmd(td, opts); err != nil {
 						return err.Error()
 					}
 					return d.mu.versions.currentVersion().String()

ingest_test.go

@@ -1609,7 +1609,6 @@ func TestIngestTargetLevel(t *testing.T) {
 			opts := Options{
 				FormatMajorVersion: internalFormatNewest,
 			}
-			opts.WithFSDefaults()
 			if d, err = runDBDefineCmd(td, &opts); err != nil {
 				return err.Error()
 			}

lsm_view_test.go

@@ -15,7 +15,7 @@ func TestLSMViewURL(t *testing.T) {
 		func(t *testing.T, td *datadriven.TestData) string {
 			switch td.Cmd {
 			case "define":
-				d, err := runDBDefineCmd(td, nil /* options */)
+				d, err := runDBDefineCmd(td, &Options{})
 				if err != nil {
 					td.Fatalf(t, "error: %s", err)
 				}

testdata/table_stats

@@ -262,7 +262,7 @@ range-deletions-bytes-estimate: 78
 
 # Test the interaction between point and range key deletions.
 
-define block-size=1 target-file-sizes=(100, 1)
+define block-size=1 target-file-sizes=(100, 1) flush-split-bytes=1000000
 ----
 
 # Start with a table that contains point and range keys, but no range dels or

version_set_test.go

@@ -375,7 +375,11 @@ func TestLargeKeys(t *testing.T) {
 	}()
 
 	var d *DB
-	defer func() { require.NoError(t, d.Close()) }()
+	defer func() {
+		if d != nil {
+			require.NoError(t, d.Close())
+		}
+	}()
 	datadriven.RunTest(t, "testdata/large_keys", func(t *testing.T, td *datadriven.TestData) string {
 		switch td.Cmd {
 		case "define":