manifest: don't compact blob files with uncertain outstanding references

When a virtual table is created from an existing sstable, the virtual table
inherits the backing sstable's blob references. The BlobReference's ValueSize
is computed by linearly interpolating according to the approximate virtual
table size. This could be grossly inaccurate.

In #5214, we began tracking a virtual table's blob references' backing value
size and using this to determine a blob file's eligibility for blob file
rewrite compactions. Using the backing's value size is pessimistic, which
avoids fruitlessly rewriting a blob file that is actually fully referenced.

The BackingValueSize introduced by #5214 is only populated for DBs at
FormatBackingValueSize and later. We've observed instances (#5306) where blob
file rewrite compactions repeatedly rewrite the same blob file due to this
mismatch. This problem was exacerbated by the introduction of high-priority
blob file rewrite compactions (#5258). This commit fixes this issue by
considering ineligible for rewrite any blob files with outstanding references
from virtual tables that don't have a BackingValueSize.

Informs #5306.

Author: jbowens

internal/manifest/blob_metadata.go

@@ -544,6 +544,18 @@ type currentBlobFile struct {
 	// since virtual sstables contribute the referenced value size of the original
 	// backing table.
 	referencedValueSize uint64
+	// virtualReferencesWithoutBackingValueSize is the count of virtual tables
+	// referencing this blob file that do not have a BackingValueSize. The
+	// BackingValueSize is only populated for format major version
+	// FormatBackingValueSize and beyond. Virtual tables created before this
+	// format major version have no accounting of the backing table's references
+	// to this blob file, so the referencedValueSize is unreliable.
+	//
+	// If virtualReferencesWithoutBackingValueSize is positive, the blob file is
+	// considered ineligible for blob rewrite compactions, because we cannot be
+	// sure that rewriting the blob file will actually allow us to elide any
+	// values.
+	virtualReferencesWithoutBackingValueSize uint64
 	// heapState holds a pointer to the heap that the blob file is in (if any)
 	// and its index in the heap's items slice. If referencedValueSize is less
 	// than metadata.Physical.ValueSize, the blob file belongs in one of the two
@@ -556,6 +568,16 @@ type currentBlobFile struct {
 	}
 }
 
+func (cbf *currentBlobFile) isEligibleForRewrite() bool {
+	// The blob file must not be fully referenced. Additionally it must not have
+	// an outstanding reference from a virtual table without a BackingValueSize,
+	// which would cause the referencedValueSize to be unreliable (and we would
+	// not be able to guarantee rewriting the blob file would actually elide any
+	// values).
+	return cbf.referencedValueSize < cbf.metadata.Physical.ValueSize &&
+		cbf.virtualReferencesWithoutBackingValueSize == 0
+}
+
 // BlobRewriteHeuristic configures the heuristic used to determine which blob
 // files should be rewritten and replaced in order to reduce value-separation
 // induced space amplification.
@@ -642,12 +664,13 @@ func (s *CurrentBlobFileSet) Init(bve *BulkVersionEdit, h BlobRewriteHeuristic)
 					panic(errors.AssertionFailedf("pebble: referenced blob file %d not found", ref.FileID))
 				}
 				cbf.references[table] = struct{}{}
-				refSize := ref.BackingValueSize
-				if refSize == 0 {
-					refSize = ref.ValueSize
+				refSize := max(ref.BackingValueSize, ref.ValueSize)
+				if table.Virtual && ref.BackingValueSize == 0 {
+					cbf.virtualReferencesWithoutBackingValueSize++
 				}
 				cbf.referencedValueSize += refSize
 				s.stats.ReferencedValueSize += ref.ValueSize
+				s.stats.ReferencedBackingValueSize += refSize
 				s.stats.ReferencesCount++
 			}
 		}
@@ -663,9 +686,7 @@ func (s *CurrentBlobFileSet) Init(bve *BulkVersionEdit, h BlobRewriteHeuristic)
 	s.rewrite.candidates.items = make([]*currentBlobFile, 0, 16)
 	s.rewrite.recentlyCreated.items = make([]*currentBlobFile, 0, 16)
 	for _, cbf := range s.files {
-		if cbf.referencedValueSize >= cbf.metadata.Physical.ValueSize {
-			// The blob file is fully referenced. There's no need to track it in
-			// either heap.
+		if !cbf.isEligibleForRewrite() {
 			continue
 		}
 
@@ -771,7 +792,7 @@ func (s *CurrentBlobFileSet) ApplyAndUpdateVersionEdit(ve *VersionEdit) error {
 			// referenced. Additional references could have been removed while
 			// the blob file rewrite was occurring. We need to re-add it to the
 			// heap of recently created files.
-			if cbf.referencedValueSize < cbf.metadata.Physical.ValueSize {
+			if cbf.isEligibleForRewrite() {
 				heap.Push(&s.rewrite.recentlyCreated, cbf)
 			}
 			continue
@@ -797,10 +818,9 @@ func (s *CurrentBlobFileSet) ApplyAndUpdateVersionEdit(ve *VersionEdit) error {
 				return errors.AssertionFailedf("pebble: referenced blob file %d not found", ref.FileID)
 			}
 			cbf.references[e.Meta] = struct{}{}
-			refSize := ref.BackingValueSize
-			if refSize == 0 {
-				// Fall back to using ValueSize to estimate reference size.
-				refSize = ref.ValueSize
+			refSize := max(ref.BackingValueSize, ref.ValueSize)
+			if e.Meta.Virtual && ref.BackingValueSize == 0 {
+				cbf.virtualReferencesWithoutBackingValueSize++
 			}
 			cbf.referencedValueSize += refSize
 			s.stats.ReferencedValueSize += ref.ValueSize
@@ -830,10 +850,9 @@ func (s *CurrentBlobFileSet) ApplyAndUpdateVersionEdit(ve *VersionEdit) error {
 			// 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.
-			refSize := ref.BackingValueSize
-			if refSize == 0 {
-				// Fall back to using ValueSize to estimate reference size.
-				refSize = ref.ValueSize
+			refSize := max(ref.BackingValueSize, ref.ValueSize)
+			if meta.Virtual && ref.BackingValueSize == 0 {
+				cbf.virtualReferencesWithoutBackingValueSize--
 			}
 			cbf.referencedValueSize -= refSize
 			s.stats.ReferencedBackingValueSize -= refSize
@@ -880,8 +899,8 @@ func (s *CurrentBlobFileSet) ApplyAndUpdateVersionEdit(ve *VersionEdit) error {
 				continue
 			}
 
-			if cbf.referencedValueSize >= cbf.metadata.Physical.ValueSize {
-				// This blob file is fully referenced.
+			if !cbf.isEligibleForRewrite() {
+				// This blob file is not eligible for rewrite.
 				continue
 			}
 

testdata/compaction/value_separation

@@ -490,18 +490,13 @@ L6:
 Blob files:
   B000006 physical:{000006 size:[109 (109B)] vals:[18 (18B)]}
 
-# Run a blob-rewrite compaction. It'll rewrite the blob file, but it won't
-# actually reclaim disk space. This is a known limitation due to the lack of
-# accurate blob value liveness for virtual sstables. See
-# https://github.com/cockroachdb/pebble/issues/4915.
+# Attempt to run a blob-rewrite compaction. We won't be able to guarantee that a
+# rewrite would reclaim disk space because of the virtual table's reference
+# (also, in reality it wouldn't), so we won't schedule the compaction.
 
 run-blob-rewrite-compaction
 ----
-L6:
-  000007(000005):[a#10,SET-a#10,SET] seqnums:[10-12] points:[a#10,SET-a#10,SET] size:104(767) blobrefs:[(B000006: 2); depth:1]
-  000008(000005):[c#12,SET-c#12,SET] seqnums:[10-12] points:[c#12,SET-c#12,SET] size:104(767) blobrefs:[(B000006: 2); depth:1]
-Blob files:
-  B000006 physical:{000009 size:[110 (110B)] vals:[18 (18B)]}
+no blob file rewrite compaction
 
 validate-blob-reference-index-block
 000007.sst