manifest: remove BlobReference.OriginalMetadata

Previously a TableMetadata's BlobReferences maintained a pointer to the
original PhysicalBlobFile that existed when the reference was created. This was
confusing state and only existed for the purpose of calculating the estimated
physical size of the referenced values.

This commit replaces the *PhysicalBlobFile pointer with a EstimatedPhysicalSize
field that's calculated once upfront. Some of the version edit accumulation and
application logic is also simplified, removing the need to track referenced
*PhysicalBlobFiles.

Informs #112.

Author: jbowens

internal/manifest/blob_metadata.go

@@ -32,50 +32,40 @@ type BlobReference struct {
 	// blob file for which there exists a reference in the sstable. Note that if
 	// any of the referencing tables are virtualized tables, the ValueSize may
 	// be approximate.
-	//
-	// INVARIANT: ValueSize <= Metadata.ValueSize
 	ValueSize uint64
-
-	// OriginalMetadata is the metadata for the original physical blob file. It
-	// is non-nil for blob references contained within active Versions. It is
-	// expected to initially be nil when decoding a version edit as a part of
-	// manfiest replay. When the version edit is accumulated into a bulk version
-	// edit, the metadata is populated.
-	//
-	// The OriginalMetadata describes the physical blob file that existed when
-	// the reference was originally created. The original physical blob file may
-	// no longer exist. The BlobReference.FileID should be translated using a
-	// Version's BlobFileSet.
-	//
-	// TODO(jackson): We should either remove the OriginalMetadata from the
-	// BlobReference or use it to infer when a blob file has definitely NOT been
-	// replaced and a lookup in Version.BlobFileSet is unnecessary.
-	OriginalMetadata *PhysicalBlobFile
-}
-
-// EstimatedPhysicalSize returns an estimate of the physical size of the blob
-// reference, in bytes. It's calculated by scaling the blob file's physical size
-// according to the ValueSize of the blob reference relative to the total
-// ValueSize of the blob file.
-func (br *BlobReference) EstimatedPhysicalSize() uint64 {
+	// EstimatedPhysicalSize is an estimate of the physical size of the blob
+	// reference, in bytes. It's calculated by scaling the blob file's physical
+	// size according to the ValueSize of the blob reference relative to the
+	// total ValueSize of the blob file.
+	EstimatedPhysicalSize uint64
+}
+
+// MakeBlobReference creates a BlobReference from the given file ID, value size,
+// and physical blob file.
+func MakeBlobReference(
+	fileID base.BlobFileID, valueSize uint64, phys *PhysicalBlobFile,
+) BlobReference {
 	if invariants.Enabled {
-		if br.ValueSize > br.OriginalMetadata.ValueSize {
+		switch {
+		case valueSize > phys.ValueSize:
 			panic(errors.AssertionFailedf("pebble: blob reference value size %d is greater than the blob file's value size %d",
-				br.ValueSize, br.OriginalMetadata.ValueSize))
-		}
-		if br.ValueSize == 0 {
-			panic(errors.AssertionFailedf("pebble: blob reference value size %d is zero", br.ValueSize))
-		}
-		if br.OriginalMetadata.ValueSize == 0 {
-			panic(errors.AssertionFailedf("pebble: blob file value size %d is zero", br.OriginalMetadata.ValueSize))
+				valueSize, phys.ValueSize))
+		case valueSize == 0:
+			panic(errors.AssertionFailedf("pebble: blob reference value size %d is zero", valueSize))
+		case phys.ValueSize == 0:
+			panic(errors.AssertionFailedf("pebble: blob file value size %d is zero", phys.ValueSize))
 		}
 	}
-	//                         br.ValueSize
-	//   Reference size =  -----------------------------  ×  br.OriginalMetadata.Size
-	//                     br.OriginalMetadata.ValueSize
-	//
-	// We perform the multiplication first to avoid floating point arithmetic.
-	return (br.ValueSize * br.OriginalMetadata.Size) / br.OriginalMetadata.ValueSize
+	return BlobReference{
+		FileID:    fileID,
+		ValueSize: valueSize,
+		//                        valueSize
+		//   Reference size =  -----------------  ×  phys.Size
+		//                      phys.ValueSize
+		//
+		// We perform the multiplication first to avoid floating point arithmetic.
+		EstimatedPhysicalSize: (valueSize * phys.Size) / phys.ValueSize,
+	}
 }
 
 // BlobFileMetadata encapsulates a blob file ID used to identify a particular
@@ -475,7 +465,7 @@ type currentBlobFile struct {
 	// referencing table number. This would likely be more memory efficient,
 	// reduce overall number of pointers to chase and suffer fewer allocations
 	// (and we can pool the B-Tree nodes to further reduce allocs)
-	references map[*TableMetadata]struct{}
+	references map[base.TableNum]struct{}
 	// referencedValueSize is the sum of the length of uncompressed values in
 	// this blob file that are still live.
 	referencedValueSize uint64
@@ -491,7 +481,7 @@ func (s *CurrentBlobFileSet) Init(bve *BulkVersionEdit) {
 	for blobFileID, pbf := range bve.BlobFiles.Added {
 		s.files[blobFileID] = &currentBlobFile{
 			metadata:   BlobFileMetadata{FileID: blobFileID, Physical: pbf},
-			references: make(map[*TableMetadata]struct{}),
+			references: make(map[base.TableNum]struct{}),
 		}
 		s.stats.Count++
 		s.stats.PhysicalSize += pbf.Size
@@ -506,7 +496,7 @@ func (s *CurrentBlobFileSet) Init(bve *BulkVersionEdit) {
 				if !ok {
 					panic(errors.AssertionFailedf("pebble: referenced blob file %d not found", ref.FileID))
 				}
-				cbf.references[table] = struct{}{}
+				cbf.references[table.TableNum] = struct{}{}
 				cbf.referencedValueSize += ref.ValueSize
 				s.stats.ReferencedValueSize += ref.ValueSize
 				s.stats.ReferencesCount++
@@ -559,7 +549,7 @@ func (s *CurrentBlobFileSet) ApplyAndUpdateVersionEdit(ve *VersionEdit) error {
 		}
 
 		blobFileID := m.FileID
-		cbf := &currentBlobFile{references: make(map[*TableMetadata]struct{})}
+		cbf := &currentBlobFile{references: make(map[base.TableNum]struct{})}
 		cbf.metadata = BlobFileMetadata{FileID: blobFileID, Physical: m.Physical}
 		s.files[blobFileID] = cbf
 		s.stats.Count++
@@ -577,7 +567,7 @@ func (s *CurrentBlobFileSet) ApplyAndUpdateVersionEdit(ve *VersionEdit) error {
 			if !ok {
 				return errors.AssertionFailedf("pebble: referenced blob file %d not found", ref.FileID)
 			}
-			cbf.references[e.Meta] = struct{}{}
+			cbf.references[e.Meta.TableNum] = struct{}{}
 			cbf.referencedValueSize += ref.ValueSize
 			s.stats.ReferencedValueSize += ref.ValueSize
 			s.stats.ReferencesCount++
@@ -600,13 +590,15 @@ func (s *CurrentBlobFileSet) ApplyAndUpdateVersionEdit(ve *VersionEdit) error {
 					return errors.AssertionFailedf("pebble: referenced value size %d for blob file %s is greater than the referenced value size %d",
 						ref.ValueSize, cbf.metadata.FileID, cbf.referencedValueSize)
 				}
-				if _, ok := cbf.references[meta]; !ok {
-					return errors.AssertionFailedf("pebble: deleted table %s's reference to blob file %d not known",
+				if _, ok := cbf.references[meta.TableNum]; !ok {
+					return errors.AssertionFailedf("pebble: deleted table %s's reference to blob file %s not known",
 						meta.TableNum, ref.FileID)
 				}
 			}
 
-			// Decrement the stats for this reference.
+			// Decrement the stats for this reference. We decrement even if the
+			// table is being moved, because we incremented the stats when we
+			// iterated over the version edit's new tables.
 			cbf.referencedValueSize -= ref.ValueSize
 			s.stats.ReferencedValueSize -= ref.ValueSize
 			s.stats.ReferencesCount--
@@ -619,7 +611,7 @@ func (s *CurrentBlobFileSet) ApplyAndUpdateVersionEdit(ve *VersionEdit) error {
 				continue
 			}
 			// Remove the reference of this table to this blob file.
-			delete(cbf.references, meta)
+			delete(cbf.references, meta.TableNum)
 
 			// If there are no more references to the blob file, remove it from
 			// the set and add the removal of the blob file to the version edit.

internal/manifest/table_metadata.go

@@ -328,7 +328,7 @@ func (m *TableMetadata) VirtualMeta() *TableMetadata {
 func (m *TableMetadata) EstimatedReferenceSize() uint64 {
 	var size uint64
 	for i := range m.BlobReferences {
-		size += m.BlobReferences[i].EstimatedPhysicalSize()
+		size += m.BlobReferences[i].EstimatedPhysicalSize
 	}
 	return size
 }

internal/manifest/testdata/version_edit_apply

@@ -233,7 +233,7 @@ L2:
 apply v8
   add-table:     L0 000004:[a#9,SET-z#9,DEL] seqnums:[9-9] points:[a#9,SET-z#9,DEL] blobrefs:[(B000003: 25935); depth:1]
 ----
-error during Accumulate: blob file B000003 referenced by L0.000004 not found
+pebble: blob file B000003 referenced by L0.000004 not found
 
 # Apply a version edit that introduces a new blob file and a corresponding table
 # that references its entirety.
@@ -258,7 +258,7 @@ Blob files:
 apply v8
   add-table:     L0 000005:[a#10,SET-a#10,SET] seqnums:[10-10] points:[a#10,SET-a#10,SET] blobrefs:[(B000003: 205); depth:1]
 ----
-error during Accumulate: blob file B000003 referenced by L0.000005 not found
+pebble: blob file B000003 referenced by L0.000005 not found
 
 # We can add new tables referencing the blob file if we delete an existing table
 # with a reference to the file.

internal/manifest/version_edit.go

@@ -984,13 +984,6 @@ type BulkVersionEdit struct {
 		//
 		// Deleted is keyed by blob file ID and points to the physical blob file.
 		Deleted map[base.BlobFileID]*PhysicalBlobFile
-		// DeletedReferences holds metadata of blob files referenced by tables
-		// deleted in the accumulated version edits. This is used during replay
-		// to populate the *BlobFileMetadata pointers of new blob references,
-		// making use of the invariant that new blob references must correspond
-		// to a file introduced in VersionEdit.AddedBlobFiles or a file
-		// referenced by a deleted sstable.
-		DeletedReferences map[base.BlobFileID]*PhysicalBlobFile
 	}
 
 	// AddedFileBacking is a map to support lookup so that we can populate the
@@ -1013,26 +1006,6 @@ type BulkVersionEdit struct {
 	MarkedForCompactionCountDiff int
 }
 
-// getBlobFileMetadata returns the metadata for the specified blob file. When a
-// new sstable with a reference to a blob file is accumulated to the
-// BulkVersionEdit, it must be the case that either an already-accumulated
-// version edit added the blob file to the BlobFiles.Added map, or the blob file
-// was referenced by a file that was deleted (and the blob file was added to
-// BlobFiles.DeletedReferences).
-func (b *BulkVersionEdit) getBlobFileMetadata(fileID base.BlobFileID) *PhysicalBlobFile {
-	if b.BlobFiles.Added != nil {
-		if md, ok := b.BlobFiles.Added[fileID]; ok {
-			return md
-		}
-	}
-	if b.BlobFiles.DeletedReferences != nil {
-		if md, ok := b.BlobFiles.DeletedReferences[fileID]; ok {
-			return md
-		}
-	}
-	return nil
-}
-
 // Accumulate adds the file addition and deletions in the specified version
 // edit to the bulk edit's internal state.
 //
@@ -1106,17 +1079,6 @@ func (b *BulkVersionEdit) Accumulate(ve *VersionEdit) error {
 			// Present in b.Added for the same level.
 			delete(b.AddedTables[df.Level], df.FileNum)
 		}
-		// If this deleted sstable had any references to blob files, record the
-		// referenced blob file to BlobFiles.DeletedReferences. If the
-		// references are carried forward to new files (eg, during a compaction
-		// that decides not to rewrite the blob file), then we'll have the
-		// *BlobFileMetadata available when we process the NewTableEntry.
-		for _, blobRef := range m.BlobReferences {
-			if b.BlobFiles.DeletedReferences == nil {
-				b.BlobFiles.DeletedReferences = make(map[base.BlobFileID]*PhysicalBlobFile)
-			}
-			b.BlobFiles.DeletedReferences[blobRef.FileID] = blobRef.OriginalMetadata
-		}
 	}
 
 	// Generate state for Added backing files. Note that these must be generated
@@ -1165,20 +1127,6 @@ func (b *BulkVersionEdit) Accumulate(ve *VersionEdit) error {
 		if nf.Meta.MarkedForCompaction {
 			b.MarkedForCompactionCountDiff++
 		}
-
-		// If there are any new sstables with blob references without a pointer
-		// to the corresponding BlobFileMetadata, populate the pointer. This is
-		// the expected case during manifest replay.
-		for i := range nf.Meta.BlobReferences {
-			if nf.Meta.BlobReferences[i].OriginalMetadata != nil {
-				continue
-			}
-			nf.Meta.BlobReferences[i].OriginalMetadata = b.getBlobFileMetadata(nf.Meta.BlobReferences[i].FileID)
-			if nf.Meta.BlobReferences[i].OriginalMetadata == nil {
-				return errors.Errorf("blob file %s referenced by L%d.%s not found",
-					nf.Meta.BlobReferences[i].FileID, nf.Level, nf.Meta.TableNum)
-			}
-		}
 	}
 
 	for _, n := range ve.RemovedBackingTables {
@@ -1298,6 +1246,22 @@ func (b *BulkVersionEdit) Apply(curr *Version, readCompactionRate int64) (*Versi
 			f.AllowedSeeks.Store(allowedSeeks)
 			f.InitAllowedSeeks = allowedSeeks
 
+			// Validate that all referenced blob files exist.
+			for i, ref := range f.BlobReferences {
+				phys, ok := v.BlobFiles.LookupPhysical(ref.FileID)
+				if !ok {
+					return nil, errors.AssertionFailedf("pebble: blob file %s referenced by L%d.%s not found",
+						ref.FileID, level, f.TableNum)
+				}
+				// NB: It's possible that the reference already has an estimated
+				// physical size if the table was moved.
+				if ref.EstimatedPhysicalSize == 0 {
+					// We must call MakeBlobReference so that we compute the
+					// reference's physical estimated size.
+					f.BlobReferences[i] = MakeBlobReference(ref.FileID, ref.ValueSize, phys)
+				}
+			}
+
 			err := lm.insert(f)
 			if err != nil {
 				return nil, errors.Wrap(err, "pebble")

testdata/version_set

@@ -615,29 +615,56 @@ no obsolete tables
 no zombie blob files
 no obsolete blob files
 
+# Compact the referencing sstable, removing half its referenced value size.
+
+apply
+  del-table: L4 000017
+  add-table: L5 000018:[f#20,SET-g#20,SET] seqnums:[0-0] points:[f#20,SET-g#20,SET] size:1500 blobrefs:[(B000016: 5000); depth:1]
+----
+applied:
+  last-seq-num:  99
+  del-table:     L4 000017
+  add-table:     L5 000018:[f#20,SET-g#20,SET]
+current version:
+  L2:
+    000001:[a#1,SET-c#1,SET] seqnums:[0-0] points:[a#1,SET-c#1,SET] size:100
+  L3:
+    000008:[a#1,SET-c#1,SET] seqnums:[0-0] points:[a#1,SET-c#1,SET] size:800
+    000010:[d#1,SET-e#1,SET] seqnums:[0-0] points:[d#1,SET-e#1,SET] size:1000
+  L5:
+    000018:[f#20,SET-g#20,SET] seqnums:[0-0] points:[f#20,SET-g#20,SET] size:1800 blobrefs:[(B000016: 5000); depth:1]
+  Blob files:
+    B000016 physical:{000016 size:[10000 (9.8KB)] vals:[10000 (9.8KB)]}
+no virtual backings
+zombie tables: 000017
+no obsolete tables
+no zombie blob files
+no obsolete blob files
+
+
 # Apply the blob file replacement. Because we ref'd the previous version, the
 # previous physical blob file should be considered a zombie.
 
 apply
   del-blob-file: B000016 000016
-  add-blob-file: B000016 physical:{000018 size:[5000 (4.9KB)] vals:[5000 (4.9KB)]}
+  add-blob-file: B000016 physical:{000019 size:[5000 (4.9KB)] vals:[5000 (4.9KB)]}
 ----
 applied:
   last-seq-num:  99
-  add-blob-file: B000016 physical:{000018 size:[5000 (4.9KB)] vals:[5000 (4.9KB)]}
+  add-blob-file: B000016 physical:{000019 size:[5000 (4.9KB)] vals:[5000 (4.9KB)]}
   del-blob-file: B000016 000016
 current version:
   L2:
     000001:[a#1,SET-c#1,SET] seqnums:[0-0] points:[a#1,SET-c#1,SET] size:100
   L3:
     000008:[a#1,SET-c#1,SET] seqnums:[0-0] points:[a#1,SET-c#1,SET] size:800
     000010:[d#1,SET-e#1,SET] seqnums:[0-0] points:[d#1,SET-e#1,SET] size:1000
-  L4:
-    000017:[f#20,SET-g#20,SET] seqnums:[0-0] points:[f#20,SET-g#20,SET] size:1700 blobrefs:[(B000016: 10000); depth:1]
+  L5:
+    000018:[f#20,SET-g#20,SET] seqnums:[0-0] points:[f#20,SET-g#20,SET] size:1800 blobrefs:[(B000016: 5000); depth:1]
   Blob files:
-    B000016 physical:{000018 size:[5000 (4.9KB)] vals:[5000 (4.9KB)]}
+    B000016 physical:{000019 size:[5000 (4.9KB)] vals:[5000 (4.9KB)]}
 no virtual backings
-no zombie tables
+zombie tables: 000017
 no obsolete tables
 zombie blob files: 000016
 no obsolete blob files
@@ -654,13 +681,13 @@ current version:
   L3:
     000008:[a#1,SET-c#1,SET] seqnums:[0-0] points:[a#1,SET-c#1,SET] size:800
     000010:[d#1,SET-e#1,SET] seqnums:[0-0] points:[d#1,SET-e#1,SET] size:1000
-  L4:
-    000017:[f#20,SET-g#20,SET] seqnums:[0-0] points:[f#20,SET-g#20,SET] size:1700 blobrefs:[(B000016: 10000); depth:1]
+  L5:
+    000018:[f#20,SET-g#20,SET] seqnums:[0-0] points:[f#20,SET-g#20,SET] size:1800 blobrefs:[(B000016: 5000); depth:1]
   Blob files:
-    B000016 physical:{000018 size:[5000 (4.9KB)] vals:[5000 (4.9KB)]}
+    B000016 physical:{000019 size:[5000 (4.9KB)] vals:[5000 (4.9KB)]}
 no virtual backings
 no zombie tables
-no obsolete tables
+obsolete tables: 000017
 no zombie blob files
 obsolete blob files: 000016
 
@@ -674,10 +701,10 @@ current version:
   L3:
     000008:[a#1,SET-c#1,SET] seqnums:[0-0] points:[a#1,SET-c#1,SET] size:800
     000010:[d#1,SET-e#1,SET] seqnums:[0-0] points:[d#1,SET-e#1,SET] size:1000
-  L4:
-    000017:[f#20,SET-g#20,SET] seqnums:[0-0] points:[f#20,SET-g#20,SET] size:1700 blobrefs:[(B000016: 10000); depth:1]
+  L5:
+    000018:[f#20,SET-g#20,SET] seqnums:[0-0] points:[f#20,SET-g#20,SET] size:1800 blobrefs:[(B000016: 5000); depth:1]
   Blob files:
-    B000016 physical:{000018 size:[5000 (4.9KB)] vals:[5000 (4.9KB)]}
+    B000016 physical:{000019 size:[5000 (4.9KB)] vals:[5000 (4.9KB)]}
 no virtual backings
 no zombie tables
 no obsolete tables