address steven comments

Author: msbutler

pkg/kv/kvpb/api.go

@@ -2563,3 +2563,17 @@ func validateExclusionTimestampForBatch(ts hlc.Timestamp, h Header) error {
 	}
 	return nil
 }
+
+func (r *AddSSTableRequest) Validate(bh Header) error {
+	if r.ComputeStatsDiff {
+		if r.DisallowConflicts || r.DisallowShadowingBelow.IsSet() {
+			return errors.New(
+				"invalid AddSSTableRequest: ComputeStatsDiff cannot be used with DisallowConflicts or DisallowShadowingBelow")
+		}
+		if r.MVCCStats != nil {
+			return errors.New(
+				"invalid AddSSTableRequest: ComputeStatsDiff cannot be used with precomputed MVCCStats")
+		}
+	}
+	return nil
+}

pkg/kv/kvpb/api.proto

@@ -2114,16 +2114,23 @@ message AddSSTableRequest {
   util.hlc.Timestamp ignore_keys_above_timestamp = 12 [(gogoproto.nullable) = false];
 
   // ComputeStatsDiff causes the server to compute the effect this
-  // sst will have on the range's mvcc stats, even in the presence of
+  // SST will have on the range's MVCC stats, even in the presence of
   // overlapping keys. This flag cannot be passed with the MVCCStats,
   // DisallowShadowingBelow, or DisallowShadowing fields. 
   //
   // This flag assumes that any key in the sst that is shadowed by a key in the
   // engine is also a duplicate. As an example, accurate stats will be computed
   // here: 
-  // - sst: a3,a2 and eng: a2,a1
-  // but not for, as a1 is not a duplicate: 
-  // - sst: a1 and eng: a2
+  // - sst: a@3,a@2 and eng: a@2,a@1
+  //
+  // but not for, as a@1 is not a duplicate: 
+  // - sst: a@1 and eng: a@2
+  //
+  // In the ladder case, we silently create inaccurate stats currently. A TODO
+  // in storage.ComputeSSTStatsDiff is to detect this case and increment
+  // ContainsEstimates. At least for PCR, the first client of this flag, we
+  // expect the edge case to be quite rare, and would only occur if we're
+  // ingesting data older than the ingesting range's GC TTL.
   bool compute_stats_diff = 13 [(gogoproto.customname) = "ComputeStatsDiff"];
 
   reserved 10, 11;

pkg/kv/kvserver/batcheval/cmd_add_sstable.go

@@ -130,6 +130,11 @@ func EvalAddSSTable(
 ) (result.Result, error) {
 	args := cArgs.Args.(*kvpb.AddSSTableRequest)
 	h := cArgs.Header
+
+	if err := args.Validate(h); err != nil {
+		return result.Result{}, err
+	}
+
 	ms := cArgs.Stats
 	start, end := storage.MVCCKey{Key: args.Key}, storage.MVCCKey{Key: args.EndKey}
 	sst := args.Data
@@ -290,34 +295,26 @@ func EvalAddSSTable(
 	if checkConflicts {
 		stats.Add(checkConflictsStatsDelta)
 	}
-	if !(checkConflicts || args.ComputeStatsDiff) {
-		// If CheckSSTConflicts or ComputeStatsDiff are not used to compute stats,
-		// then the stats do not account for overlapping keys in the engine, so we
-		// have to assume they are estimates.
-		stats.ContainsEstimates++
-	}
 	if args.ComputeStatsDiff {
-		if checkConflicts {
-			return result.Result{}, errors.New(
-				"AddSSTableRequest.ComputeStatsDiff cannot be used with DisallowConflicts or DisallowShadowingBelow")
-		}
-		if args.MVCCStats != nil {
-			return result.Result{}, errors.New(
-				"AddSSTableRequest.ComputeStatsDiff cannot be used with precomputed MVCCStats")
-		}
-		statsDiff, err := computeSSTStatsDiffWrapper(ctx, sst, readWriter, h.Timestamp.WallTime, start, end)
+		statsDiff, err := computeSSTStatsDiffWithFallback(ctx, sst, readWriter, h.Timestamp.WallTime, start, end)
 		if err != nil {
 			return result.Result{}, errors.Wrap(err, "computing SST stats diff")
 		}
 		stats.Add(statsDiff)
 	} else if args.MVCCStats != nil {
 		stats.Add(*args.MVCCStats)
+		if !checkConflicts {
+			stats.ContainsEstimates++
+		}
 	} else {
 		sstStats, err := computeSSTStats(ctx, sst, h.Timestamp.WallTime)
 		if err != nil {
 			return result.Result{}, errors.Wrap(err, "computing SST stats")
 		}
 		stats.Add(sstStats)
+		if !checkConflicts {
+			stats.ContainsEstimates++
+		}
 	}
 	ms.Add(stats)
 
@@ -468,7 +465,7 @@ func EvalAddSSTable(
 	}, nil
 }
 
-func computeSSTStatsDiffWrapper(
+func computeSSTStatsDiffWithFallback(
 	ctx context.Context,
 	sst []byte,
 	readWriter storage.ReadWriter,
@@ -519,7 +516,7 @@ func computeSSTStats(ctx context.Context, sst []byte, nowNanos int64) (enginepb.
 }
 
 // checkSSTSpanBounds verifies that the keys in the sstable are within the
-// span specified by the request header.
+// span specified by [start, end].
 func checkSSTSpanBounds(ctx context.Context, sst []byte, start, end storage.MVCCKey) error {
 	sstIter, err := storage.NewMemSSTIterator(sst, true /* verify */, storage.IterOptions{
 		KeyTypes:   storage.IterKeyTypePointsAndRanges,