db: modify Options.DebugCheck to be a function

Previously, pebble.Options had a boolean DebugCheck field that when set to true caused Pebble to run (*DB).CheckLevels when a new version was installed. This commit refactors that Options field to be a function of the signature `func(*DB) error`, allowing custom functions to be called instead. In addition a `DebugCheckLevels` function with the matching signature is exposed so the existing use is still ergonomic for callers. This was motivated by #438. When injecting errors into filesystem operations, CheckLevels may fail with an injected error. With this change, the metamorphic tests can provide a custom DebugCheck function that wraps `DebugCheckLevels` with a retry loop for injected errors.
cockroachdb · Mar 30, 2020 · a1718be82f5fb8655d6561df16e4d3dbd721ba61 · a1718be
1 parent d6182fe
commit a1718be82f5fb8655d6561df16e4d3dbd721ba61
Unified Split

Showing with 27 additions and 36 deletions.

+2 −10 compaction.go

+3 −3 compaction_test.go

+2 −6 ingest.go

+1 −1 ingest_test.go

+1 −1 internal/metamorphic/test.go

+1 −5 open.go

+11 −4 options.go

+4 −4 range_del_test.go

+2 −2 read_state.go
diff --git a/compaction.go b/compaction.go
@@ -1002,11 +1002,7 @@ func (d *DB) flush1() error {
 	if err == nil {
 		flushed = d.mu.mem.queue[:n]
 		d.mu.mem.queue = d.mu.mem.queue[n:]
-		var checker func() error
-		if d.opts.DebugCheck {
-			checker = func() error { return d.CheckLevels(nil) }
-		}
-		d.updateReadStateLocked(checker)
+		d.updateReadStateLocked(d.opts.DebugCheck)
 	}
 
 	d.deleteObsoleteFiles(jobID)
@@ -1178,11 +1174,7 @@ func (d *DB) compact1(c *compaction, errChannel chan error) (err error) {
 	// there are no references obsolete tables will be added to the obsolete
 	// table list.
 	if err == nil {
-		var checker func() error
-		if d.opts.DebugCheck {
-			checker = func() error { return d.CheckLevels(nil) }
-		}
-		d.updateReadStateLocked(checker)
+		d.updateReadStateLocked(d.opts.DebugCheck)
 	}
 	d.deleteObsoleteFiles(jobID)
 

diff --git a/compaction_test.go b/compaction_test.go
@@ -795,7 +795,7 @@ func TestCompaction(t *testing.T) {
 	d, err := Open("", &Options{
 		FS:           mem,
 		MemTableSize: memTableSize,
-		DebugCheck:   true,
+		DebugCheck:   DebugCheckLevels,
 		enablePacing: true,
 	})
 	if err != nil {
@@ -929,7 +929,7 @@ func TestManualCompaction(t *testing.T) {
 		var err error
 		d, err = Open("", &Options{
 			FS:         mem,
-			DebugCheck: true,
+			DebugCheck: DebugCheckLevels,
 		})
 		require.NoError(t, err)
 	}
@@ -1700,7 +1700,7 @@ func TestFlushInvariant(t *testing.T) {
 								}
 							},
 						},
-						DebugCheck: true,
+						DebugCheck: DebugCheckLevels,
 					})
 					require.NoError(t, err)
 

diff --git a/ingest.go b/ingest.go
@@ -216,7 +216,7 @@ func ingestLink(
 	for i := range paths {
 		target := base.MakeFilename(fs, dirname, fileTypeTable, meta[i].FileNum)
 		var err error
-		if _, ok := opts.FS.(*vfs.MemFS); ok && opts.DebugCheck {
+		if _, ok := opts.FS.(*vfs.MemFS); ok && opts.DebugCheck != nil {
 			// The combination of MemFS+Ingest+DebugCheck produces awkwardness around
 			// the subsequent deletion of files. The problem is that MemFS implements
 			// the Windows semantics of disallowing removal of an open file. This is
@@ -647,11 +647,7 @@ func (d *DB) ingestApply(jobID int, meta []*fileMetadata) (*versionEdit, error)
 	}); err != nil {
 		return nil, err
 	}
-	var checker func() error
-	if d.opts.DebugCheck {
-		checker = func() error { return d.CheckLevels(nil) }
-	}
-	d.updateReadStateLocked(checker)
+	d.updateReadStateLocked(d.opts.DebugCheck)
 	d.deleteObsoleteFiles(jobID)
 	// The ingestion may have pushed a level over the threshold for compaction,
 	// so check to see if one is necessary and schedule it.

diff --git a/ingest_test.go b/ingest_test.go
@@ -500,7 +500,7 @@ func TestIngest(t *testing.T) {
 			FS:                    mem,
 			L0CompactionThreshold: 100,
 			L0StopWritesThreshold: 100,
-			DebugCheck:            true,
+			DebugCheck:            DebugCheckLevels,
 		})
 		require.NoError(t, err)
 	}

diff --git a/internal/metamorphic/test.go b/internal/metamorphic/test.go
@@ -49,7 +49,7 @@ func (t *test) init(h *history, dir string, testOpts *testOptions) error {
 	t.opts = testOpts.opts.EnsureDefaults()
 	t.opts.Logger = h.Logger()
 	t.opts.EventListener = pebble.MakeLoggingEventListener(t.opts.Logger)
-	t.opts.DebugCheck = true
+	t.opts.DebugCheck = pebble.DebugCheckLevels
 
 	defer t.opts.Cache.Unref()
 

diff --git a/open.go b/open.go
@@ -285,11 +285,7 @@ func Open(dirname string, opts *Options) (db *DB, _ error) {
 			return nil, err
 		}
 	}
-	var checker func() error
-	if d.opts.DebugCheck {
-		checker = func() error { return d.CheckLevels(nil) }
-	}
-	d.updateReadStateLocked(checker)
+	d.updateReadStateLocked(d.opts.DebugCheck)
 
 	if !d.opts.ReadOnly {
 		// Write the current options to disk.

diff --git a/options.go b/options.go
@@ -252,10 +252,10 @@ type Options struct {
 	// The default value uses the same ordering as bytes.Compare.
 	Comparer *Comparer
 
-	// Setting this to true causes DB.CheckLevels() to be called whenever a new version is being
-	// installed. DB.CheckLevels() iterates over all the data in the DB, so set this to true only
-	// in tests.
-	DebugCheck bool
+	// DebugCheck is invoked, if non-nil, whenever a new version is being
+	// installed. Typically, this is set to pebble.DebugCheckLevels in tests
+	// or tools only, to check invariants over all the data in the database.
+	DebugCheck func(*DB) error
 
 	// Disable the write-ahead log (WAL). Disabling the write-ahead log prohibits
 	// crash recovery, but can improve performance if crash recovery is not
@@ -399,6 +399,13 @@ type Options struct {
 	enablePacing bool
 }
 
+// DebugCheckLevels calls CheckLevels on the provided database.
+// It may be set in the DebugCheck field of Options to check
+// level invariants whenever a new version is installed.
+func DebugCheckLevels(db *DB) error {
+	return db.CheckLevels(nil)
+}
+
 // EnsureDefaults ensures that the default values for all options are set if a
 // valid value was not already specified. Returns the new options.
 func (o *Options) EnsureDefaults() *Options {

diff --git a/range_del_test.go b/range_del_test.go
@@ -106,7 +106,7 @@ func TestRangeDelCompactionTruncation(t *testing.T) {
 			{TargetFileSize: 100},
 			{TargetFileSize: 1},
 		},
-		DebugCheck: true,
+		DebugCheck: DebugCheckLevels,
 	})
 	require.NoError(t, err)
 	defer d.Close()
@@ -237,7 +237,7 @@ func TestRangeDelCompactionTruncation2(t *testing.T) {
 			{TargetFileSize: 100},
 			{TargetFileSize: 1},
 		},
-		DebugCheck: true,
+		DebugCheck: DebugCheckLevels,
 	})
 	require.NoError(t, err)
 	defer d.Close()
@@ -299,7 +299,7 @@ func TestRangeDelCompactionTruncation3(t *testing.T) {
 			{TargetFileSize: 100},
 			{TargetFileSize: 1},
 		},
-		DebugCheck: true,
+		DebugCheck: DebugCheckLevels,
 	})
 	require.NoError(t, err)
 	defer d.Close()
@@ -401,7 +401,7 @@ func BenchmarkRangeDelIterate(b *testing.B) {
 					d, err := Open("", &Options{
 						Cache:      cache,
 						FS:         mem,
-						DebugCheck: true,
+						DebugCheck: DebugCheckLevels,
 					})
 					if err != nil {
 						b.Fatal(err)

diff --git a/read_state.go b/read_state.go
@@ -79,7 +79,7 @@ func (d *DB) loadReadState() *readState {
 // updateReadStateLocked creates a new readState from the current version and
 // list of memtables. Requires DB.mu is held. If checker is not nil, it is called after installing
 // the new readState
-func (d *DB) updateReadStateLocked(checker func() error) {
+func (d *DB) updateReadStateLocked(checker func(*DB) error) {
 	s := &readState{
 		db:        d,
 		refcnt:    1,
@@ -96,7 +96,7 @@ func (d *DB) updateReadStateLocked(checker func() error) {
 	d.readState.val = s
 	d.readState.Unlock()
 	if checker != nil {
-		if err := checker(); err != nil {
+		if err := checker(d); err != nil {
 			d.opts.Logger.Fatalf("checker failed with error: %s", err)
 		}
 	}