sstable: consolidate reader/writer options

There are currently three mechanisms to configure sstable.Reader or
Writer:
 - most settings use `ReaderOptions`/`WriterOptions`
 - some settings use `ReaderOption`/`WriterOption`; some of these are
   kept private using a hook through the private package
 - some settings call a function that modifies a Writer, also through
   the private package

This commit moves all settings to `ReaderOption`/`WriterOption`
structs. Settings which are not allowed to be used outside Pebble are
defined in `sstableinternal`, which prevents outside callers from
constructing values for them.

compaction.go

@@ -20,7 +20,7 @@ import (
 	"github.com/cockroachdb/pebble/internal/keyspan"
 	"github.com/cockroachdb/pebble/internal/keyspan/keyspanimpl"
 	"github.com/cockroachdb/pebble/internal/manifest"
-	"github.com/cockroachdb/pebble/internal/private"
+	"github.com/cockroachdb/pebble/internal/sstableinternal"
 	"github.com/cockroachdb/pebble/objstorage"
 	"github.com/cockroachdb/pebble/objstorage/objstorageprovider/objiotracing"
 	"github.com/cockroachdb/pebble/objstorage/remote"
@@ -2712,7 +2712,13 @@ func (d *DB) newCompactionOutput(
 		FileNum: diskFileNum,
 	})
 
-	cacheOpts := private.SSTableCacheOpts(d.cacheID, diskFileNum).(sstable.WriterOption)
+	writerOpts.SetInternal(sstableinternal.WriterOptions{
+		CacheOpts: sstableinternal.CacheOptions{
+			Cache:   d.opts.Cache,
+			CacheID: d.cacheID,
+			FileNum: diskFileNum,
+		},
+	})
 
 	const MaxFileWriteAdditionalCPUTime = time.Millisecond * 100
 	cpuWorkHandle := d.opts.Experimental.CPUWorkPermissionGranter.GetPermission(
@@ -2722,7 +2728,7 @@ func (d *DB) newCompactionOutput(
 		d.opts.Experimental.MaxWriterConcurrency > 0 &&
 			(cpuWorkHandle.Permitted() || d.opts.Experimental.ForceWriterParallelism)
 
-	tw := sstable.NewWriter(writable, writerOpts, cacheOpts)
+	tw := sstable.NewWriter(writable, writerOpts)
 	return objMeta, tw, cpuWorkHandle, nil
 }
 

data_test.go

@@ -23,9 +23,9 @@ import (
 	"github.com/cockroachdb/pebble/internal/base"
 	"github.com/cockroachdb/pebble/internal/humanize"
 	"github.com/cockroachdb/pebble/internal/keyspan"
-	"github.com/cockroachdb/pebble/internal/private"
 	"github.com/cockroachdb/pebble/internal/rangedel"
 	"github.com/cockroachdb/pebble/internal/rangekey"
+	"github.com/cockroachdb/pebble/internal/sstableinternal"
 	"github.com/cockroachdb/pebble/internal/testkeys"
 	"github.com/cockroachdb/pebble/objstorage/objstorageprovider"
 	"github.com/cockroachdb/pebble/objstorage/remote"
@@ -1223,11 +1223,16 @@ func runSSTablePropertiesCmd(t *testing.T, td *datadriven.TestData, d *DB) strin
 	if err != nil {
 		return err.Error()
 	}
+	readerOpts := d.opts.MakeReaderOptions()
 	// TODO(bananabrick): cacheOpts is used to set the file number on a Reader,
 	// and virtual sstables expect this file number to be set. Split out the
 	// opts into fileNum opts, and cache opts.
-	cacheOpts := private.SSTableCacheOpts(0, backingFileNum).(sstable.ReaderOption)
-	r, err := sstable.NewReader(readable, d.opts.MakeReaderOptions(), cacheOpts)
+	readerOpts.SetInternalCacheOpts(sstableinternal.CacheOptions{
+		Cache:   d.opts.Cache,
+		CacheID: 0,
+		FileNum: backingFileNum,
+	})
+	r, err := sstable.NewReader(readable, readerOpts)
 	if err != nil {
 		return err.Error()
 	}

external_iterator.go

@@ -14,34 +14,6 @@ import (
 	"github.com/cockroachdb/pebble/sstable"
 )
 
-// ExternalIterOption provide an interface to specify open-time options to
-// NewExternalIter.
-type ExternalIterOption interface {
-	// iterApply is called on the iterator during opening in order to set internal
-	// parameters.
-	iterApply(*Iterator)
-	// readerOptions returns any reader options added by this iter option.
-	readerOptions() []sstable.ReaderOption
-}
-
-type externalIterReaderOptions struct {
-	opts []sstable.ReaderOption
-}
-
-func (e *externalIterReaderOptions) iterApply(iterator *Iterator) {
-	// Do nothing.
-}
-
-func (e *externalIterReaderOptions) readerOptions() []sstable.ReaderOption {
-	return e.opts
-}
-
-// ExternalIterReaderOptions returns an ExternalIterOption that specifies
-// sstable.ReaderOptions to be applied on sstable readers in NewExternalIter.
-func ExternalIterReaderOptions(opts ...sstable.ReaderOption) ExternalIterOption {
-	return &externalIterReaderOptions{opts: opts}
-}
-
 // NewExternalIter takes an input 2d array of sstable files which may overlap
 // across subarrays but not within a subarray (at least as far as points are
 // concerned; range keys are allowed to overlap arbitrarily even within a
@@ -59,22 +31,15 @@ func ExternalIterReaderOptions(opts ...sstable.ReaderOption) ExternalIterOption
 // options, including block-property and table filters. NewExternalIter errors
 // if an incompatible option is set.
 func NewExternalIter(
-	o *Options,
-	iterOpts *IterOptions,
-	files [][]sstable.ReadableFile,
-	extraOpts ...ExternalIterOption,
+	o *Options, iterOpts *IterOptions, files [][]sstable.ReadableFile,
 ) (it *Iterator, err error) {
-	return NewExternalIterWithContext(context.Background(), o, iterOpts, files, extraOpts...)
+	return NewExternalIterWithContext(context.Background(), o, iterOpts, files)
 }
 
 // NewExternalIterWithContext is like NewExternalIter, and additionally
 // accepts a context for tracing.
 func NewExternalIterWithContext(
-	ctx context.Context,
-	o *Options,
-	iterOpts *IterOptions,
-	files [][]sstable.ReadableFile,
-	extraOpts ...ExternalIterOption,
+	ctx context.Context, o *Options, iterOpts *IterOptions, files [][]sstable.ReadableFile,
 ) (it *Iterator, err error) {
 	if iterOpts != nil {
 		if err := validateExternalIterOpts(iterOpts); err != nil {
@@ -85,16 +50,12 @@ func NewExternalIterWithContext(
 	var readers [][]*sstable.Reader
 
 	seqNumOffset := 0
-	var extraReaderOpts []sstable.ReaderOption
-	for i := range extraOpts {
-		extraReaderOpts = append(extraReaderOpts, extraOpts[i].readerOptions()...)
-	}
 	for _, levelFiles := range files {
 		seqNumOffset += len(levelFiles)
 	}
 	for _, levelFiles := range files {
 		seqNumOffset -= len(levelFiles)
-		subReaders, err := openExternalTables(o, levelFiles, seqNumOffset, o.MakeReaderOptions(), extraReaderOpts...)
+		subReaders, err := openExternalTables(o, levelFiles, seqNumOffset, o.MakeReaderOptions())
 		readers = append(readers, subReaders)
 		if err != nil {
 			// Close all the opened readers.
@@ -138,9 +99,6 @@ func NewExternalIterWithContext(
 		dbi.opts = *iterOpts
 		dbi.processBounds(iterOpts.LowerBound, iterOpts.UpperBound)
 	}
-	for i := range extraOpts {
-		extraOpts[i].iterApply(dbi)
-	}
 	if err := finishInitializingExternal(ctx, dbi); err != nil {
 		dbi.Close()
 		return nil, err
@@ -295,19 +253,15 @@ func finishInitializingExternal(ctx context.Context, it *Iterator) error {
 }
 
 func openExternalTables(
-	o *Options,
-	files []sstable.ReadableFile,
-	seqNumOffset int,
-	readerOpts sstable.ReaderOptions,
-	extraReaderOpts ...sstable.ReaderOption,
+	o *Options, files []sstable.ReadableFile, seqNumOffset int, readerOpts sstable.ReaderOptions,
 ) (readers []*sstable.Reader, err error) {
 	readers = make([]*sstable.Reader, 0, len(files))
 	for i := range files {
 		readable, err := sstable.NewSimpleReadable(files[i])
 		if err != nil {
 			return readers, err
 		}
-		r, err := sstable.NewReader(readable, readerOpts, extraReaderOpts...)
+		r, err := sstable.NewReader(readable, readerOpts)
 		if err != nil {
 			return readers, err
 		}

external_iterator_test.go

@@ -41,7 +41,6 @@ func TestExternalIterator(t *testing.T) {
 			return ""
 		case "iter":
 			opts := IterOptions{KeyTypes: IterKeyTypePointsAndRanges}
-			var externalIterOpts []ExternalIterOption
 			var files [][]sstable.ReadableFile
 			for _, arg := range td.CmdArgs {
 				switch arg.Key {
@@ -59,7 +58,7 @@ func TestExternalIterator(t *testing.T) {
 					}
 				}
 			}
-			it, err := NewExternalIter(o, &opts, files, externalIterOpts...)
+			it, err := NewExternalIter(o, &opts, files)
 			require.NoError(t, err)
 			return runIterCmd(td, it, true /* close iter */)
 		default:

ingest.go

@@ -16,7 +16,7 @@ import (
 	"github.com/cockroachdb/pebble/internal/invariants"
 	"github.com/cockroachdb/pebble/internal/manifest"
 	"github.com/cockroachdb/pebble/internal/overlap"
-	"github.com/cockroachdb/pebble/internal/private"
+	"github.com/cockroachdb/pebble/internal/sstableinternal"
 	"github.com/cockroachdb/pebble/objstorage"
 	"github.com/cockroachdb/pebble/objstorage/remote"
 	"github.com/cockroachdb/pebble/sstable"
@@ -253,8 +253,13 @@ func ingestLoad1(
 	cacheID uint64,
 	fileNum base.FileNum,
 ) (*fileMetadata, error) {
-	cacheOpts := private.SSTableCacheOpts(cacheID, base.PhysicalTableDiskFileNum(fileNum)).(sstable.ReaderOption)
-	r, err := sstable.NewReader(readable, opts.MakeReaderOptions(), cacheOpts)
+	o := opts.MakeReaderOptions()
+	o.SetInternalCacheOpts(sstableinternal.CacheOptions{
+		Cache:   opts.Cache,
+		CacheID: cacheID,
+		FileNum: base.PhysicalTableDiskFileNum(fileNum),
+	})
+	r, err := sstable.NewReader(readable, o)
 	if err != nil {
 		return nil, err
 	}

internal/sstableinternal/options.go

@@ -0,0 +1,37 @@
+// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
+// of this source code is governed by a BSD-style license that can be found in
+// the LICENSE file.
+
+package sstableinternal
+
+import (
+	"github.com/cockroachdb/pebble/internal/base"
+	"github.com/cockroachdb/pebble/internal/cache"
+)
+
+// CacheOptions contains the information needed to interact with the block
+// cache.
+type CacheOptions struct {
+	// Cache can be nil, in which case no cache is used. When non-nil, the other
+	// fields must be set accordingly.
+	Cache   *cache.Cache
+	CacheID uint64
+	FileNum base.DiskFileNum
+}
+
+// ReaderOptions are fields of sstable.ReaderOptions that can only be set from
+// within the pebble package.
+type ReaderOptions struct {
+	CacheOpts CacheOptions
+}
+
+// WriterOptions are fields of sstable.ReaderOptions that can only be set from
+// within the pebble package.
+type WriterOptions struct {
+	CacheOpts CacheOptions
+
+	// DisableKeyOrderChecks disables the checks that keys are added to an sstable
+	// in order. It is intended for use only in the construction of invalid
+	// sstables for testing. See tool/make_test_sstables.go.
+	DisableKeyOrderChecks bool
+}

level_checker_test.go

@@ -20,8 +20,8 @@ import (
 	"github.com/cockroachdb/pebble/internal/invariants"
 	"github.com/cockroachdb/pebble/internal/keyspan"
 	"github.com/cockroachdb/pebble/internal/manifest"
-	"github.com/cockroachdb/pebble/internal/private"
 	"github.com/cockroachdb/pebble/internal/rangedel"
+	"github.com/cockroachdb/pebble/internal/sstableinternal"
 	"github.com/cockroachdb/pebble/internal/testkeys"
 	"github.com/cockroachdb/pebble/objstorage/objstorageprovider"
 	"github.com/cockroachdb/pebble/sstable"
@@ -165,20 +165,25 @@ func TestCheckLevelsCornerCases(t *testing.T) {
 					return err.Error()
 				}
 				writeUnfragmented := false
-				w := sstable.NewWriter(objstorageprovider.NewFileWritable(f), sstable.WriterOptions{
-					TableFormat: FormatNewest.MaxTableFormat(),
-					Comparer:    testkeys.Comparer,
-				})
+				disableKeyOrderChecks := false
 				for _, arg := range d.CmdArgs {
 					switch arg.Key {
 					case "disable-key-order-checks":
-						private.SSTableWriterDisableKeyOrderChecks(w)
+						disableKeyOrderChecks = true
 					case "write-unfragmented":
 						writeUnfragmented = true
 					default:
 						return fmt.Sprintf("unknown arg: %s", arg.Key)
 					}
 				}
+				writerOpts := sstable.WriterOptions{
+					TableFormat: FormatNewest.MaxTableFormat(),
+					Comparer:    testkeys.Comparer,
+				}
+				writerOpts.SetInternal(sstableinternal.WriterOptions{
+					DisableKeyOrderChecks: disableKeyOrderChecks,
+				})
+				w := sstable.NewWriter(objstorageprovider.NewFileWritable(f), writerOpts)
 				var tombstones []keyspan.Span
 				frag := keyspan.Fragmenter{
 					Cmp:    testkeys.Comparer.Compare,
@@ -224,8 +229,10 @@ func TestCheckLevelsCornerCases(t *testing.T) {
 				if err != nil {
 					return err.Error()
 				}
-				cacheOpts := private.SSTableCacheOpts(0, base.DiskFileNum(fileNum-1)).(sstable.ReaderOption)
-				r, err := sstable.NewReader(readable, sstable.ReaderOptions{Comparer: testkeys.Comparer}, cacheOpts)
+				// Set FileNum for logging purposes.
+				readerOpts := sstable.ReaderOptions{Comparer: testkeys.Comparer}
+				readerOpts.SetInternalCacheOpts(sstableinternal.CacheOptions{FileNum: base.DiskFileNum(fileNum - 1)})
+				r, err := sstable.NewReader(readable, readerOpts)
 				if err != nil {
 					return err.Error()
 				}

level_iter_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/pebble/internal/keyspan"
 	"github.com/cockroachdb/pebble/internal/manifest"
 	"github.com/cockroachdb/pebble/internal/rangedel"
+	"github.com/cockroachdb/pebble/internal/sstableinternal"
 	"github.com/cockroachdb/pebble/internal/testkeys"
 	"github.com/cockroachdb/pebble/objstorage/objstorageprovider"
 	"github.com/cockroachdb/pebble/sstable"
@@ -525,9 +526,12 @@ func buildLevelIterTables(
 			b.Fatal(err)
 		}
 	}
-
-	opts := sstable.ReaderOptions{Cache: NewCache(128 << 20), Comparer: DefaultComparer}
-	defer opts.Cache.Unref()
+	c := NewCache(128 << 20 /* 128MB */)
+	defer c.Unref()
+	opts := sstable.ReaderOptions{Comparer: DefaultComparer}
+	opts.SetInternalCacheOpts(sstableinternal.CacheOptions{
+		Cache: c,
+	})
 	readers := make([]*sstable.Reader, len(files))
 	for i := range files {
 		f, err := mem.Open(fmt.Sprintf("bench%d", i))

merging_iter_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/pebble/internal/keyspan"
 	"github.com/cockroachdb/pebble/internal/manifest"
 	"github.com/cockroachdb/pebble/internal/rangedel"
+	"github.com/cockroachdb/pebble/internal/sstableinternal"
 	"github.com/cockroachdb/pebble/internal/testkeys"
 	"github.com/cockroachdb/pebble/objstorage/objstorageprovider"
 	"github.com/cockroachdb/pebble/sstable"
@@ -372,9 +373,13 @@ func buildMergingIterTables(
 			b.Fatal(err)
 		}
 	}
+	c := NewCache(128 << 20 /* 128MB */)
+	defer c.Unref()
 
-	opts := sstable.ReaderOptions{Cache: NewCache(128 << 20)}
-	defer opts.Cache.Unref()
+	var opts sstable.ReaderOptions
+	opts.SetInternalCacheOpts(sstableinternal.CacheOptions{
+		Cache: c,
+	})
 
 	readers := make([]*sstable.Reader, len(files))
 	for i := range files {
@@ -608,12 +613,18 @@ func buildLevelsForMergingIterSeqSeek(
 		}
 	}
 
-	opts := sstable.ReaderOptions{Cache: NewCache(128 << 20), Comparer: DefaultComparer}
+	c := NewCache(128 << 20 /* 128MB */)
+	defer c.Unref()
+
+	opts := sstable.ReaderOptions{Comparer: DefaultComparer}
+	opts.SetInternalCacheOpts(sstableinternal.CacheOptions{
+		Cache: c,
+	})
+
 	if writeBloomFilters {
 		opts.Filters = make(map[string]FilterPolicy)
 		opts.Filters[filterPolicy.Name()] = filterPolicy
 	}
-	defer opts.Cache.Unref()
 
 	readers = make([][]*sstable.Reader, levelCount)
 	for i := range files {