kv: compare MVCC GC threshold against Refresh{Range}Request.RefreshFrom

Noticed by Sumeer in #74628.

A Refresh request needs to observe all MVCC versions between its
exclusive RefreshFrom time and its inclusive RefreshTo time. If it were
to permit MVCC GC between these times then it could miss conflicts that
should cause the refresh to fail. This could in turn lead to violations
of serializability. For example:

```
txn1 reads value k1@10
txn2 deletes (tombstones) k1@15
mvcc gc @ 20 clears versions k1@10 and k1@15
txn1 refreshes @ 25, sees no value between (10, 25], refresh successful
```

In the example, the refresh erroneously succeeds because the request is
permitted to evaluate after part of the MVCC history it needs to read
has been GCed. By considering the RefreshFrom time to be the earliest
active timestamp of the request, we avoid this hazard. Instead of being
allowed to evaluate, the refresh request in the example would have hit
a BatchTimestampBeforeGCError.

pkg/kv/kvserver/replica_test.go

@@ -8474,9 +8474,9 @@ func TestFailureToProcessCommandClearsLocalResult(t *testing.T) {
  }
 }
 
-// TestCommandTimeThreshold verifies that commands outside the replica GC
-// threshold fail.
-func TestCommandTimeThreshold(t *testing.T) {
+// TestBatchTimestampBelowGCThreshold verifies that commands below the replica
+// GC threshold fail.
+func TestBatchTimestampBelowGCThreshold(t *testing.T) {
  defer leaktest.AfterTest(t)()
  defer log.Scope(t).Close(t)
  ctx := context.Background()
@@ -8555,6 +8555,76 @@ func TestCommandTimeThreshold(t *testing.T) {
  }
 }
 
+// TestRefreshFromBelowGCThreshold verifies that refresh requests that need to
+// see MVCC history below the replica GC threshold fail.
+func TestRefreshFromBelowGCThreshold(t *testing.T) {
+ defer leaktest.AfterTest(t)()
+ defer log.Scope(t).Close(t)
+
+ testutils.RunTrueAndFalse(t, "ranged", func(t *testing.T, ranged bool) {
+ ctx := context.Background()
+ tc := testContext{}
+ stopper := stop.NewStopper()
+ defer stopper.Stop(ctx)
+ tc.Start(ctx, t, stopper)
+
+ now := tc.Clock().Now()
+ ts1 := now.Add(1, 0)
+ ts2 := now.Add(2, 0)
+ ts3 := now.Add(3, 0)
+ ts4 := now.Add(4, 0)
+ ts5 := now.Add(5, 0)
+
+ keyA := roachpb.Key("a")
+ keyB := roachpb.Key("b")
+
+ // Construct a Refresh{Range} request for a transaction that refreshes the
+ // time interval (ts2, ts4].
+ var refresh roachpb.Request
+ if ranged {
+ refresh = &roachpb.RefreshRangeRequest{
+ RequestHeader: roachpb.RequestHeader{Key: keyA, EndKey: keyB},
+ RefreshFrom: ts2,
+ }
+ } else {
+ refresh = &roachpb.RefreshRequest{
+ RequestHeader: roachpb.RequestHeader{Key: keyA},
+ RefreshFrom: ts2,
+ }
+ }
+ txn := roachpb.MakeTransaction("test", keyA, 0, ts2, 0, 0)
+ txn.Refresh(ts4)
+
+ for _, testCase := range []struct {
+ gc hlc.Timestamp
+ expErr bool
+ }{
+ {hlc.Timestamp{}, false},
+ {ts1, false},
+ {ts2, false},
+ {ts3, true},
+ {ts4, true},
+ {ts5, true},
+ } {
+ t.Run(fmt.Sprintf("gcThreshold=%s", testCase.gc), func(t *testing.T) {
+ if !testCase.gc.IsEmpty() {
+ gcr := roachpb.GCRequest{Threshold: testCase.gc}
+ _, pErr := tc.SendWrapped(&gcr)
+ require.Nil(t, pErr)
+ }
+
+ _, pErr := tc.SendWrappedWith(roachpb.Header{Txn: &txn}, refresh)
+ if testCase.expErr {
+ require.NotNil(t, pErr)
+ require.Regexp(t, `batch timestamp .* must be after replica GC threshold .*`, pErr)
+ } else {
+ require.Nil(t, pErr)
+ }
+ })
+ }
+ })
+}
+
 func TestReplicaTimestampCacheBumpNotLost(t *testing.T) {
  defer leaktest.AfterTest(t)()
  defer log.Scope(t).Close(t)

pkg/roachpb/batch.go

@@ -91,14 +91,39 @@ func (ba BatchRequest) EarliestActiveTimestamp() hlc.Timestamp {
  switch t := ru.GetInner().(type) {
  case *ExportRequest:
  if !t.StartTime.IsEmpty() {
- ts.Backward(t.StartTime)
+ // NB: StartTime.Next() because StartTime is exclusive.
+ ts.Backward(t.StartTime.Next())
  }
  case *RevertRangeRequest:
  // This method is only used to check GC Threshold so Revert requests that
  // opt-out of checking the target vs threshold should skip this.
  if !t.IgnoreGcThreshold {
  ts.Backward(t.TargetTime)
  }
+ case *RefreshRequest:
+ // A Refresh request needs to observe all MVCC versions between its
+ // exclusive RefreshFrom time and its inclusive RefreshTo time. If it were
+ // to permit MVCC GC between these times then it could miss conflicts that
+ // should cause the refresh to fail. This could in turn lead to violations
+ // of serializability. For example:
+ //
+ // txn1 reads value k1@10
+ // txn2 deletes (tombstones) k1@15
+ // mvcc gc @ 20 clears versions k1@10 and k1@15
+ // txn1 refreshes @ 25, sees no value between (10, 25], refresh successful
+ //
+ // In the example, the refresh erroneously succeeds because the request is
+ // permitted to evaluate after part of the MVCC history it needs to read
+ // has been GCed. By considering the RefreshFrom time to be the earliest
+ // active timestamp of the request, we avoid this hazard. Instead of being
+ // allowed to evaluate, the refresh request in the example would have hit
+ // a BatchTimestampBeforeGCError.
+ //
+ // NB: RefreshFrom.Next() because RefreshFrom is exclusive.
+ ts.Backward(t.RefreshFrom.Next())
+ case *RefreshRangeRequest:
+ // The same requirement applies to RefreshRange request.
+ ts.Backward(t.RefreshFrom.Next())
  }
  }
  return ts