db: fix virtual table race

We initialize the virtual params in the file cache, but this is racy.

To fix this:
 - we change Virtual to a bool, and change `VirtualReaderParams` to a
   non-pointer field.
 - we add `AttachVirtualBacking` and use that instead of setting
   `FileBacking` directly. This function also initializes the
   `VirtualParams`.

Fixes #4536

Author: RaduBerinde

checkpoint.go

@@ -283,7 +283,7 @@ func (d *DB) Checkpoint(
 			}
 
 			fileBacking := f.FileBacking
-			if f.Virtual != nil {
+			if f.Virtual {
 				if _, ok := requiredVirtualBackingFiles[fileBacking.DiskFileNum]; ok {
 					continue
 				}

compaction.go

@@ -424,7 +424,7 @@ func newCompaction(
 		if mustCopy {
 			// If the source is virtual, it's best to just rewrite the file as all
 			// conditions in the above comment are met.
-			if meta.Virtual == nil {
+			if !meta.Virtual {
 				c.kind = compactionKindCopy
 			}
 		} else {
@@ -2401,7 +2401,7 @@ func (d *DB) cleanupVersionEdit(ve *versionEdit) {
 		deletedFiles[key.FileNum] = struct{}{}
 	}
 	for i := range ve.NewTables {
-		if ve.NewTables[i].Meta.Virtual != nil {
+		if ve.NewTables[i].Meta.Virtual {
 			// We handle backing files separately.
 			continue
 		}
@@ -2562,7 +2562,7 @@ func (d *DB) runCopyCompaction(
 		}
 		// Note that based on logic in the compaction picker, we're guaranteed
 		// inputMeta.Virtual is nil.
-		if inputMeta.Virtual != nil {
+		if inputMeta.Virtual {
 			panic(errors.AssertionFailedf("cannot do a copy compaction of a virtual sstable across local/remote storage"))
 		}
 	}
@@ -2700,7 +2700,7 @@ func (d *DB) runCopyCompaction(
 		Level: c.outputLevel.level,
 		Meta:  newMeta,
 	}}
-	if newMeta.Virtual != nil {
+	if newMeta.Virtual {
 		ve.CreatedBackingTables = []*fileBacking{newMeta.FileBacking}
 	}
 	c.metrics[c.outputLevel.level] = &LevelMetrics{
@@ -2904,7 +2904,7 @@ func (d *DB) runDeleteOnlyCompaction(
 	// NewFiles.
 	usedBackingFiles := make(map[base.DiskFileNum]struct{})
 	for _, e := range ve.NewTables {
-		if e.Meta.Virtual != nil {
+		if e.Meta.Virtual {
 			usedBackingFiles[e.Meta.FileBacking.DiskFileNum] = struct{}{}
 		}
 	}

compaction_picker.go

@@ -1186,7 +1186,7 @@ func pickCompactionSeedFile(
 func responsibleForGarbageBytes(
 	virtualBackings *manifest.VirtualBackings, m *tableMetadata,
 ) uint64 {
-	if m.Virtual == nil {
+	if !m.Virtual {
 		return 0
 	}
 	useCount, virtualizedSize := virtualBackings.Usage(m.FileBacking.DiskFileNum)

compaction_test.go

@@ -600,7 +600,7 @@ func TestCompaction(t *testing.T) {
 		defer provider.Close()
 		for _, levelMetadata := range v.Levels {
 			for meta := range levelMetadata.All() {
-				if meta.Virtual != nil {
+				if meta.Virtual {
 					continue
 				}
 				f, err := provider.OpenForReading(context.Background(), base.FileTypeTable, meta.FileBacking.DiskFileNum, objstorage.OpenOptions{})
@@ -3001,7 +3001,7 @@ func hasExternalFiles(d *DB) bool {
 	for level := 0; level < manifest.NumLevels; level++ {
 		iter := v.Levels[level].Iter()
 		for m := iter.First(); m != nil; m = iter.Next() {
-			if m.Virtual != nil {
+			if m.Virtual {
 				meta, err := d.objProvider.Lookup(base.FileTypeTable, m.FileBacking.DiskFileNum)
 				if err != nil {
 					panic(err)

data_test.go

@@ -1288,9 +1288,8 @@ func runSSTablePropertiesCmd(t *testing.T, td *datadriven.TestData, d *DB) strin
 
 	props := r.Properties.String()
 	env := sstable.ReadEnv{}
-	if m != nil && m.Virtual != nil {
-		m.InitVirtual()
-		env.Virtual = m.Virtual
+	if m != nil && m.Virtual {
+		env.Virtual = &m.VirtualParams
 		scaledProps := r.Properties.GetScaledProperties(m.FileBacking.Size, m.Size)
 		props = scaledProps.String()
 	}

db.go

@@ -2248,7 +2248,7 @@ func (d *DB) SSTables(opts ...SSTablesOption) ([][]SSTableInfo, error) {
 				}
 				destTables[j].Properties = p
 			}
-			destTables[j].Virtual = m.Virtual != nil
+			destTables[j].Virtual = m.Virtual
 			destTables[j].BackingSSTNum = m.FileBacking.DiskFileNum
 			objMeta, err := d.objProvider.Lookup(base.FileTypeTable, m.FileBacking.DiskFileNum)
 			if err != nil {

download.go

@@ -403,7 +403,7 @@ func firstExternalFileInLevelIter(
 		f = it.Next()
 	}
 	for ; f != nil && endBound.IsUpperBoundFor(cmp, f.Smallest.UserKey); f = it.Next() {
-		if f.Virtual != nil && objstorage.IsExternalTable(objProvider, f.FileBacking.DiskFileNum) {
+		if f.Virtual && objstorage.IsExternalTable(objProvider, f.FileBacking.DiskFileNum) {
 			return f
 		}
 	}

download_test.go

@@ -181,7 +181,7 @@ func TestDownloadTask(t *testing.T) {
 					ch <- ErrCancelledCompaction
 				} else {
 					fmt.Fprintf(&buf, "downloading %s\n", f.FileNum)
-					f.Virtual = nil
+					f.Virtual = false
 					f.FileBacking.DiskFileNum = base.DiskFileNum(f.FileNum)
 					ch <- nil
 				}

excise.go

@@ -12,7 +12,6 @@ import (
 	"github.com/cockroachdb/pebble/internal/base"
 	"github.com/cockroachdb/pebble/internal/invariants"
 	"github.com/cockroachdb/pebble/internal/manifest"
-	"github.com/cockroachdb/pebble/sstable/virtual"
 )
 
 // Excise atomically deletes all data overlapping with the provided span. All
@@ -103,9 +102,8 @@ func (d *DB) exciseTable(
 	// https://github.com/cockroachdb/pebble/issues/2112 .
 	if d.cmp(m.Smallest.UserKey, exciseSpan.Start) < 0 {
 		leftTable = &tableMetadata{
-			Virtual:     &virtual.VirtualReaderParams{},
-			FileBacking: m.FileBacking,
-			FileNum:     d.mu.versions.getNextFileNum(),
+			Virtual: true,
+			FileNum: d.mu.versions.getNextFileNum(),
 			// Note that these are loose bounds for smallest/largest seqnums, but they're
 			// sufficient for maintaining correctness.
 			SmallestSeqNum:           m.SmallestSeqNum,
@@ -118,6 +116,7 @@ func (d *DB) exciseTable(
 		}
 
 		if leftTable.HasRangeKeys || leftTable.HasPointKeys {
+			leftTable.AttachVirtualBacking(m.FileBacking)
 			if err := determineExcisedTableSize(d.fileCache, m, leftTable); err != nil {
 				return nil, nil, err
 			}
@@ -136,9 +135,8 @@ func (d *DB) exciseTable(
 		// See comment before the definition of leftFile for the motivation behind
 		// calculating tight user-key bounds.
 		rightTable = &tableMetadata{
-			Virtual:     &virtual.VirtualReaderParams{},
-			FileBacking: m.FileBacking,
-			FileNum:     d.mu.versions.getNextFileNum(),
+			Virtual: true,
+			FileNum: d.mu.versions.getNextFileNum(),
 			// Note that these are loose bounds for smallest/largest seqnums, but they're
 			// sufficient for maintaining correctness.
 			SmallestSeqNum:           m.SmallestSeqNum,
@@ -150,6 +148,7 @@ func (d *DB) exciseTable(
 			return nil, nil, err
 		}
 		if rightTable.HasRangeKeys || rightTable.HasPointKeys {
+			rightTable.AttachVirtualBacking(m.FileBacking)
 			if err := determineExcisedTableSize(d.fileCache, m, rightTable); err != nil {
 				return nil, nil, err
 			}
@@ -352,7 +351,7 @@ func applyExciseToVersionEdit(
 	if leftTable == nil && rightTable == nil {
 		return
 	}
-	if originalTable.Virtual == nil {
+	if !originalTable.Virtual {
 		// If the original table was virtual, then its file backing is already known
 		// to the manifest; we don't need to create another file backing. Note that
 		// there must be only one CreatedBackingTables entry per backing sstable.

file_cache.go

@@ -311,13 +311,17 @@ func (h *fileCacheHandle) estimateSize(
 	return size, err
 }
 
-func createReader(v *fileCacheValue, file *tableMetadata) (*sstable.Reader, sstable.ReadEnv) {
+func createReader(v *fileCacheValue, meta *tableMetadata) (*sstable.Reader, sstable.ReadEnv) {
 	r := v.mustSSTableReader()
 	env := sstable.ReadEnv{}
-	if file.Virtual != nil {
-		file.InitVirtual()
-		env.Virtual = file.Virtual
-		env.IsSharedIngested = v.isShared && file.SyntheticSeqNum() != 0
+	if meta.Virtual {
+		if invariants.Enabled {
+			if meta.VirtualParams.FileNum == 0 || meta.VirtualParams.Lower.UserKey == nil || meta.VirtualParams.Upper.UserKey == nil {
+				panic("virtual params not initialized")
+			}
+		}
+		env.Virtual = &meta.VirtualParams
+		env.IsSharedIngested = v.isShared && meta.SyntheticSeqNum() != 0
 	}
 	return r, env
 }
@@ -339,9 +343,13 @@ func (h *fileCacheHandle) withReader(
 	env := sstable.ReadEnv{Block: blockEnv}
 
 	r := v.mustSSTableReader()
-	if meta.Virtual != nil {
-		meta.InitVirtual()
-		env.Virtual = meta.Virtual
+	if meta.Virtual {
+		if invariants.Enabled {
+			if meta.VirtualParams.FileNum == 0 || meta.VirtualParams.Lower.UserKey == nil || meta.VirtualParams.Upper.UserKey == nil {
+				panic("virtual params not initialized")
+			}
+		}
+		env.Virtual = &meta.VirtualParams
 		env.IsSharedIngested = v.isShared && meta.SyntheticSeqNum() != 0
 	}