db: always estimate deletions via the table stats collector

When a new sstable is added to the LSM we need to populate its table stats,
including PointDeletionBytesEstimate and RangeDeletionBytesEstimate. These
statistics are typically calculated by the table stats collector
asynchronously, because their calculation requires walking the LSM table
metadata beneath the file and sometimes I/O.

Previously there was a fast path for sstables for which unsized point
tombstones (DELs, SINGLEDELs but not DELSIZEDs) made up less than 10% of all of
the table's entries. The previous logic reasoned that if there were not many
deletions within the file, we could use the average value size and compression
ratio of the file itself without introducing too much error overall. However,
an incorrect compression ratio can introduce significant error, including for
DELSIZED keys. In the previous logic, we could take the fast path even if the
entirety of the table was DELSIZED tombstones.

This commit removes the fast path for all sstables except for those with no
deletions at all. The 'slow path' of allowing the asynchronous table stats
collector to calculate these stats is not significantly more resource
intensive, and does not need to perform any I/O for point tombstones. I suspect
the fast path was a premature optimization.

Informs cockroachdb/cockroach#151633.

Author: jbowens

compaction.go

@@ -3518,9 +3518,7 @@ func (c *tableCompaction) makeVersionEdit(result compact.Result) (*manifest.Vers
 
 		// If the file didn't contain any range deletions, we can fill its
 		// table stats now, avoiding unnecessarily loading the table later.
-		maybeSetStatsFromProperties(
-			fileMeta.PhysicalMeta(), &t.WriterMeta.Properties, c.logger,
-		)
+		maybeSetStatsFromProperties(fileMeta.PhysicalMeta(), &t.WriterMeta.Properties)
 
 		if t.WriterMeta.HasPointKeys {
 			fileMeta.ExtendPointKeyBounds(c.comparer.Compare,

ingest.go

@@ -322,7 +322,7 @@ func ingestLoad1(
 	// disallowing removal of an open file. Under MemFS, if we don't populate
 	// meta.Stats here, the file will be loaded into the file cache for
 	// calculating stats before we can remove the original link.
-	maybeSetStatsFromProperties(meta.PhysicalMeta(), &props, opts.Logger)
+	maybeSetStatsFromProperties(meta.PhysicalMeta(), &props)
 
 	var iterStats base.InternalIteratorStats
 	env := sstable.ReadEnv{

table_stats.go

@@ -692,52 +692,31 @@ func sanityCheckStats(
 // maybeSetStatsFromProperties sets the table backing properties and attempts to
 // set the table stats from the properties, for a table that was created by an
 // ingestion or compaction.
-func maybeSetStatsFromProperties(
-	meta *manifest.TableMetadata, props *sstable.Properties, logger Logger,
-) bool {
-	backingProps := meta.TableBacking.PopulateProperties(props)
+func maybeSetStatsFromProperties(meta *manifest.TableMetadata, props *sstable.Properties) bool {
+	meta.TableBacking.PopulateProperties(props)
 	if invariants.Enabled && meta.Virtual {
 		panic("table expected to be physical")
 	}
-	// If a table contains range deletions or range key deletions, we defer the
-	// stats collection. There are two main reasons for this:
+	// If a table contains any deletions, we defer the stats collection. There
+	// are two main reasons for this:
 	//
-	//  1. Estimating the potential for reclaimed space due to a range deletion
-	//     tombstone requires scanning the LSM - a potentially expensive operation
-	//     that should be deferred.
-	//  2. Range deletions and / or range key deletions present an opportunity to
-	//     compute "deletion hints", which also requires a scan of the LSM to
-	//     compute tables that would be eligible for deletion.
+	//  1. Estimating the potential for reclaimed space due to a deletion
+	//     requires scanning the LSM - a potentially expensive operation that
+	//     should be deferred.
+	//  2. Range deletions present an opportunity to compute "deletion hints",
+	//     which also requires a scan of the LSM to compute tables that would be
+	//     eligible for deletion.
 	//
 	// These two tasks are deferred to the table stats collector goroutine.
-	if props.NumRangeDeletions != 0 || props.NumRangeKeyDels != 0 {
-		return false
-	}
-
-	// If a table is more than 10% point deletions without user-provided size
-	// estimates, don't calculate the PointDeletionsBytesEstimate statistic
-	// using our limited knowledge. The table stats collector can populate the
-	// stats and calculate an average of value size of all the tables beneath
-	// the table in the LSM, which will be more accurate.
-	if unsizedDels := invariants.SafeSub(props.NumDeletions, props.NumSizedDeletions); unsizedDels > props.NumEntries/10 {
+	//
+	// Note that even if the point deletions are sized (DELSIZEDs), an accurate
+	// compression ratio is necessary to calculate an accurate estimate of the
+	// physical disk space they reclaim. To do that, we need to scan the LSM
+	// beneath the file.
+	if props.NumDeletions != 0 || props.NumRangeKeyDels != 0 {
 		return false
 	}
-
-	var pointEstimate uint64
-	if props.NumEntries > 0 {
-		// Use the file's own average key and value sizes as an estimate. This
-		// doesn't require any additional IO and since the number of point
-		// deletions in the file is low, the error introduced by this crude
-		// estimate is expected to be small.
-		avgValSize, compressionRatio := estimatePhysicalSizes(meta, props)
-		pointEstimate = pointDeletionsBytesEstimate(backingProps, avgValSize, compressionRatio)
-	}
-
-	stats := manifest.TableStats{
-		PointDeletionsBytesEstimate: pointEstimate,
-		RangeDeletionsBytesEstimate: 0,
-	}
-	sanityCheckStats(meta, &stats, logger, "stats from properties")
+	var stats manifest.TableStats
 	meta.PopulateStats(&stats)
 	return true
 }
@@ -832,35 +811,6 @@ func pointDeletionsBytesEstimate(
 	return uint64((tombstonesLogicalSize + shadowedLogicalSize) * compressionRatio)
 }
 
-func estimatePhysicalSizes(
-	tableMeta *manifest.TableMetadata, props *sstable.Properties,
-) (avgValLogicalSize, compressionRatio float64) {
-	// RawKeySize and RawValueSize are uncompressed totals. Scale according to
-	// the data size to account for compression, index blocks and metadata
-	// overhead. Eg:
-	//
-	//    Compression rate        ×  Average uncompressed value size
-	//
-	//                            ↓
-	//
-	//         FileSize              RawValSize
-	//   -----------------------  ×  ----------
-	//   RawKeySize+RawValueSize     NumEntries
-	//
-	physicalSize := tableMeta.Size + tableMeta.EstimatedReferenceSize()
-	uncompressedSum := props.RawKeySize + props.RawValueSize
-	compressionRatio = float64(physicalSize) / float64(uncompressedSum)
-	if compressionRatio > 1 {
-		// We can get huge compression ratios due to the fixed overhead of files
-		// containing a tiny amount of data. By setting this to 1, we are ignoring
-		// that overhead, but we accept that tradeoff since the total bytes in
-		// such overhead is not large.
-		compressionRatio = 1
-	}
-	avgValLogicalSize = float64(props.RawValueSize) / float64(props.NumEntries)
-	return avgValLogicalSize, compressionRatio
-}
-
 // newCombinedDeletionKeyspanIter returns a keyspan.FragmentIterator that
 // returns "ranged deletion" spans for a single table, providing a combined view
 // of both range deletion and range key deletion spans. The