Skip to content

Commit

Permalink
*: remove remaining stale references to synthetic timestamps
Browse files Browse the repository at this point in the history
Informs #101938.

Release note: None
  • Loading branch information
nvanbenschoten committed Jan 9, 2024
1 parent 9fd435b commit c31785b
Show file tree
Hide file tree
Showing 8 changed files with 21 additions and 28 deletions.
4 changes: 2 additions & 2 deletions pkg/kv/kvpb/api.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2300,8 +2300,8 @@ message SubsumeResponse {
// through a range merge, to make the merge less disruptive to writes on
// the post-merge range because the timestamp cache won't be bumped as
// high.
// 2. it can transfer information about reads with synthetic timestamps, which
// are not otherwise captured by the FreezeStart clock timestamp.
// 2. it can transfer information about reads with future-time timestamps,
// which are not otherwise captured by the FreezeStart clock timestamp.
kv.kvserver.readsummary.ReadSummary read_summary = 7;
}

Expand Down
8 changes: 4 additions & 4 deletions pkg/kv/kvpb/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -1139,7 +1139,7 @@ func (e *ReadWithinUncertaintyIntervalError) RetryTimestamp() hlc.Timestamp {
// advance the txn's timestamp up to the local uncertainty limit on the node
// which hit the error. This ensures that no future read after the retry on
// this node (ignoring lease complications in ComputeLocalUncertaintyLimit
// and values with synthetic timestamps) will throw an uncertainty error,
// and values with future-time timestamps) will throw an uncertainty error,
// even when reading other keys.
//
// Note that if the request was not able to establish a local uncertainty
Expand All @@ -1153,9 +1153,9 @@ func (e *ReadWithinUncertaintyIntervalError) RetryTimestamp() hlc.Timestamp {
// In general, we expect the local uncertainty limit, if set, to be above
// the uncertainty value's timestamp. So we expect this Forward to advance
// ts. However, this is not always the case. The one exception is if the
// uncertain value had a synthetic timestamp, so it was compared against the
// global uncertainty limit to determine uncertainty (see IsUncertain). In
// such cases, we're ok advancing just past the value's timestamp. Either
// uncertain value had a future-time timestamp, so it was compared against
// the global uncertainty limit to determine uncertainty (see IsUncertain).
// In such cases, we're ok advancing just past the value's timestamp. Either
// way, we won't see the same value in our uncertainty interval on a retry.
ts.Forward(e.LocalUncertaintyLimit.ToTimestamp())
return ts
Expand Down
12 changes: 6 additions & 6 deletions pkg/kv/kvserver/client_replica_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,12 @@ import (

// TestReplicaClockUpdates verifies that the leaseholder updates its clocks
// when executing a command to the command's timestamp, as long as the
// request timestamp is from a clock (i.e. is not synthetic).
// request timestamp is from a clock (i.e. is not in the future).
func TestReplicaClockUpdates(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

run := func(t *testing.T, write bool, synthetic bool) {
run := func(t *testing.T, write bool, futureTime bool) {
const numNodes = 3
var manuals []*hlc.HybridManualClock
var clocks []*hlc.Clock
Expand Down Expand Up @@ -123,7 +123,7 @@ func TestReplicaClockUpdates(t *testing.T) {
// MaxOffset.
reqTS := clocks[0].Now().Add(clocks[0].MaxOffset().Nanoseconds()/2, 0)
h := kvpb.Header{Timestamp: reqTS}
if !synthetic {
if !futureTime {
h.Now = hlc.ClockTimestamp(reqTS)
}

Expand All @@ -144,13 +144,13 @@ func TestReplicaClockUpdates(t *testing.T) {
// practice an assertion against followers' clocks being updated is very
// difficult to make without being flaky because it's difficult to prevent
// other channels (background work, etc.) from carrying the clock update.
expUpdated := !synthetic
expUpdated := !futureTime
require.Equal(t, expUpdated, reqTS.Less(clocks[0].Now()))
}

testutils.RunTrueAndFalse(t, "write", func(t *testing.T, write bool) {
testutils.RunTrueAndFalse(t, "synthetic", func(t *testing.T, synthetic bool) {
run(t, write, synthetic)
testutils.RunTrueAndFalse(t, "future-time", func(t *testing.T, futureTime bool) {
run(t, write, futureTime)
})
})
}
Expand Down
4 changes: 0 additions & 4 deletions pkg/kv/kvserver/intentresolver/intent_resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -442,10 +442,6 @@ func (ir *IntentResolver) MaybePushTransactions(
return nil, b.MustPErr()
}

// TODO(nvanbenschoten): if we succeed because the transaction has already
// been pushed _past_ where we were pushing, we need to set the synthetic
// bit. This is part of #36431.

br := b.RawResponse()
pushedTxns := make(map[uuid.UUID]*roachpb.Transaction, len(br.Responses))
for _, resp := range br.Responses {
Expand Down
4 changes: 2 additions & 2 deletions pkg/kv/kvserver/kvserverpb/proposer_kv.proto
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,8 @@ message ReplicatedEvalResult {
// 1. it can transfer a higher-resolution snapshot of the reads on the range
// through a lease transfer, to make the lease transfers less disruptive to
// writes because the timestamp cache won't be bumped as high.
// 2. it can transfer information about reads with synthetic timestamps, which
// are not otherwise captured by the new lease's start time.
// 2. it can transfer information about reads with future-time timestamps,
// which are not otherwise captured by the new lease's start time.
//
// When a ReadSummary is set in a ReplicatedEvalResult, there is always also a
// write to the RangePriorReadSummaryKey in the RaftCommand.WriteBatch. The
Expand Down
3 changes: 0 additions & 3 deletions pkg/roachpb/data.go
Original file line number Diff line number Diff line change
Expand Up @@ -966,9 +966,6 @@ func MakeTransaction(
omitInRangefeeds bool,
) Transaction {
u := uuid.FastMakeV4()
// TODO(nvanbenschoten): technically, gul should be a synthetic timestamp.
// Make this change in v21.2 when all nodes in a cluster are guaranteed to
// be aware of synthetic timestamps by addressing the TODO in Timestamp.Add.
gul := now.Add(maxOffsetNs, 0)

return Transaction{
Expand Down
4 changes: 2 additions & 2 deletions pkg/roachpb/data.proto
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,8 @@ message MergeTrigger {
// through a range merge, to make the merge less disruptive to writes on
// the post-merge range because the timestamp cache won't be bumped as
// high.
// 2. it can transfer information about reads with synthetic timestamps, which
// are not otherwise captured by the FreezeStart clock timestamp.
// 2. it can transfer information about reads with future-time timestamps,
// which are not otherwise captured by the FreezeStart clock timestamp.
//
// When a RightReadSummary is set in ReplicatedEvalResult.Merge.Trigger, there
// is always also a write to the RangePriorReadSummaryKey in the corresponding
Expand Down
10 changes: 5 additions & 5 deletions pkg/storage/pebble_mvcc_scanner.go
Original file line number Diff line number Diff line change
Expand Up @@ -549,8 +549,8 @@ func (p *pebbleMVCCScanner) init(
p.uncertainty = ui
// We must check uncertainty even if p.ts >= local_uncertainty_limit
// because the local uncertainty limit cannot be applied to values with
// synthetic timestamps. We are only able to skip uncertainty checks if
// p.ts >= global_uncertainty_limit.
// future-time timestamps with earlier local timestamps. We are only able
// to skip uncertainty checks if p.ts >= global_uncertainty_limit.
p.checkUncertainty = p.ts.Less(p.uncertainty.GlobalLimit)
}

Expand Down Expand Up @@ -1394,9 +1394,9 @@ func (p *pebbleMVCCScanner) seekVersion(
// is set to the transaction's global uncertainty limit, so we are
// seeking based on the worst-case uncertainty, but values with a
// time in the range (uncertainty.LocalLimit, uncertainty.GlobalLimit]
// are only uncertain if their timestamps are synthetic. Meanwhile,
// any value with a time in the range (ts, uncertainty.LocalLimit]
// is uncertain.
// are only uncertain if they have an earlier local timestamp that is
// before uncertainty.LocalLimit. Meanwhile, any value with a time in
// the range (ts, uncertainty.LocalLimit] is uncertain.
localTS := p.curUnsafeValue.GetLocalTimestamp(p.curUnsafeKey.Timestamp)
if p.uncertainty.IsUncertain(p.curUnsafeKey.Timestamp, localTS) {
return p.uncertaintyError(p.curUnsafeKey.Timestamp, localTS), false
Expand Down

0 comments on commit c31785b

Please sign in to comment.