diff --git a/pkg/roachpb/api.go b/pkg/roachpb/api.go index dafdf4ada9dc..5cff531b3dfa 100644 --- a/pkg/roachpb/api.go +++ b/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 } diff --git a/pkg/storage/batcheval/cmd_refresh.go b/pkg/storage/batcheval/cmd_refresh.go index 65069730f01d..3e96d09f85fa 100644 --- a/pkg/storage/batcheval/cmd_refresh.go +++ b/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) diff --git a/pkg/storage/batcheval/cmd_refresh_range.go b/pkg/storage/batcheval/cmd_refresh_range.go index 95b70f34d7fc..e2b74f68f7b3 100644 --- a/pkg/storage/batcheval/cmd_refresh_range.go +++ b/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 { diff --git a/pkg/storage/replica.go b/pkg/storage/replica.go index 93e893c41c58..a167ebf7a2f3 100644 --- a/pkg/storage/replica.go +++ b/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 ( diff --git a/pkg/storage/replica_follower_read.go b/pkg/storage/replica_follower_read.go index c60bb731af81..64501e5a9cfe 100644 --- a/pkg/storage/replica_follower_read.go +++ b/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. diff --git a/pkg/storage/replica_tscache.go b/pkg/storage/replica_tscache.go index d777c5ff1eac..88d2f99e9997 100644 --- a/pkg/storage/replica_tscache.go +++ b/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)) }