Merge #34566

34566: storage/roachpb: various cleanup around Refresh{Range}Request r=nvanbenschoten a=nvanbenschoten

This PR includes a few small changes that I noticed while investigating #34025.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>

pkg/roachpb/api.go

@@ -1076,11 +1076,20 @@ func (*ImportRequest) flags() int           { return isAdmin | isAlone }
 func (*AdminScatterRequest) flags() int     { return isAdmin | isAlone | isRange }
 func (*AddSSTableRequest) flags() int       { return isWrite | isAlone | isRange | isUnsplittable }
 
-// RefreshRequest and RefreshRangeRequest both list
-// updates(Read)TSCache, though they actually update the read or write
-// timestamp cache depending on the write parameter in the request.
-func (*RefreshRequest) flags() int      { return isRead | isTxn | updatesReadTSCache }
-func (*RefreshRangeRequest) flags() int { return isRead | isTxn | isRange | updatesReadTSCache }
+// RefreshRequest and RefreshRangeRequest both determine which timestamp cache
+// they update based on their Write parameter.
+func (r *RefreshRequest) flags() int {
+	if r.Write {
+		return isRead | isTxn | updatesWriteTSCache
+	}
+	return isRead | isTxn | updatesReadTSCache
+}
+func (r *RefreshRangeRequest) flags() int {
+	if r.Write {
+		return isRead | isTxn | isRange | updatesWriteTSCache
+	}
+	return isRead | isTxn | isRange | updatesReadTSCache
+}
 
 func (*SubsumeRequest) flags() int { return isRead | isAlone | updatesReadTSCache }
 

pkg/storage/batcheval/cmd_refresh.go

@@ -53,13 +53,18 @@ func Refresh(
 	if err != nil {
 		return result.Result{}, err
 	} else if val != nil {
+		// TODO(nvanbenschoten): This is pessimistic. We only need to check
+		//   !ts.Less(h.Txn.PrevRefreshTimestamp)
+		// This could avoid failed refreshes due to requests performed after
+		// earlier refreshes (which read at the refresh ts) that already
+		// observed writes between the orig ts and the refresh ts.
 		if ts := val.Timestamp; !ts.Less(h.Txn.OrigTimestamp) {
 			return result.Result{}, errors.Errorf("encountered recently written key %s @%s", args.Key, ts)
 		}
 	}
 
-	// Now, check if the intent was written earlier than the command's timestamp
-	// and was not owned by this transaction.
+	// Check if an intent which is not owned by this transaction was written
+	// at or beneath the refresh timestamp.
 	if intent != nil && intent.Txn.ID != h.Txn.ID {
 		return result.Result{}, errors.Errorf("encountered recently written intent %s @%s",
 			intent.Span.Key, intent.Txn.Timestamp)

pkg/storage/batcheval/cmd_refresh_range.go

@@ -52,6 +52,11 @@ func RefreshRange(
 		Inconsistent: true,
 		Tombstones:   true,
 	}, func(kv roachpb.KeyValue) (bool, error) {
+		// TODO(nvanbenschoten): This is pessimistic. We only need to check
+		//   !ts.Less(h.Txn.PrevRefreshTimestamp)
+		// This could avoid failed refreshes due to requests performed after
+		// earlier refreshes (which read at the refresh ts) that already
+		// observed writes between the orig ts and the refresh ts.
 		if ts := kv.Value.Timestamp; !ts.Less(h.Txn.OrigTimestamp) {
 			return true, errors.Errorf("encountered recently written key %s @%s", kv.Key, ts)
 		}
@@ -61,8 +66,8 @@ func RefreshRange(
 		return result.Result{}, err
 	}
 
-	// Now, check intents slice for any which were written earlier than
-	// the command's timestamp, not owned by this transaction.
+	// Check if any intents which are not owned by this transaction were written
+	// at or beneath the refresh timestamp.
 	for _, i := range intents {
 		// Ignore our own intents.
 		if i.Txn.ID == h.Txn.ID {

pkg/storage/replica.go

@@ -103,16 +103,6 @@ var MaxCommandSize = settings.RegisterValidatedByteSizeSetting(
 	},
 )
 
-// FollowerReadsEnabled controls whether replicas attempt to serve follower
-// reads. The closed timestamp machinery is unaffected by this, i.e. the same
-// information is collected and passed around, regardless of the value of this
-// setting.
-var FollowerReadsEnabled = settings.RegisterBoolSetting(
-	"kv.closed_timestamp.follower_reads_enabled",
-	"allow (all) replicas to serve consistent historical reads based on closed timestamp information",
-	false,
-)
-
 type proposalReevaluationReason int
 
 const (

pkg/storage/replica_follower_read.go

@@ -18,11 +18,22 @@ import (
 	"context"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/storage/closedts/ctpb"
 	ctstorage "github.com/cockroachdb/cockroach/pkg/storage/closedts/storage"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 )
 
+// FollowerReadsEnabled controls whether replicas attempt to serve follower
+// reads. The closed timestamp machinery is unaffected by this, i.e. the same
+// information is collected and passed around, regardless of the value of this
+// setting.
+var FollowerReadsEnabled = settings.RegisterBoolSetting(
+	"kv.closed_timestamp.follower_reads_enabled",
+	"allow (all) replicas to serve consistent historical reads based on closed timestamp information",
+	false,
+)
+
 // canServeFollowerRead tests, when a range lease could not be
 // acquired, whether the read only batch can be served as a follower
 // read despite the error.

pkg/storage/replica_tscache.go

@@ -153,10 +153,6 @@ func (r *Replica) updateTimestampCache(
 						tc.Add(start, end, t.Txn.Timestamp, uuid.UUID{}, true /* readCache */)
 					}
 				}
-			case *roachpb.RefreshRequest:
-				tc.Add(start, end, ts, txnID, !t.Write /* readCache */)
-			case *roachpb.RefreshRangeRequest:
-				tc.Add(start, end, ts, txnID, !t.Write /* readCache */)
 			default:
 				tc.Add(start, end, ts, txnID, !roachpb.UpdatesWriteTimestampCache(args))
 			}