From 8cfda31327d409a4a46c0f3282b27e4982ff9925 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Thu, 5 Nov 2020 22:25:03 -0500 Subject: [PATCH] storage: ensure proper equality tests on Timestamp Now that structural equality is different than semantic equality because of the new flags field in Timestamp, we need to be careful about comparing by value. I detected these by adding a `_ func()` field to the struct and observed what fell out when compiling. --- pkg/ccl/changefeedccl/kvfeed/kv_feed.go | 2 +- pkg/kv/kvserver/liveness/livenesspb/liveness.go | 2 +- pkg/kv/kvserver/rangefeed/resolved_timestamp.go | 2 +- pkg/kv/kvserver/replica_consistency_diff.go | 2 +- pkg/sql/sem/tree/as_of.go | 2 +- pkg/storage/multi_iterator.go | 2 +- pkg/storage/mvcc.go | 2 +- pkg/util/span/frontier.go | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/ccl/changefeedccl/kvfeed/kv_feed.go b/pkg/ccl/changefeedccl/kvfeed/kv_feed.go index fc81f248661d..51014608d578 100644 --- a/pkg/ccl/changefeedccl/kvfeed/kv_feed.go +++ b/pkg/ccl/changefeedccl/kvfeed/kv_feed.go @@ -362,7 +362,7 @@ func copyFromSourceToSinkUntilTableEvent( return false, false } frontier.Forward(resolved.Span, boundaryResolvedTimestamp) - return true, frontier.Frontier() == boundaryResolvedTimestamp + return true, frontier.Frontier().EqOrdering(boundaryResolvedTimestamp) default: log.Fatal(ctx, "unknown event type") return false, false diff --git a/pkg/kv/kvserver/liveness/livenesspb/liveness.go b/pkg/kv/kvserver/liveness/livenesspb/liveness.go index 984ca167dac2..69f99874ca39 100644 --- a/pkg/kv/kvserver/liveness/livenesspb/liveness.go +++ b/pkg/kv/kvserver/liveness/livenesspb/liveness.go @@ -53,7 +53,7 @@ func (l *Liveness) Compare(o Liveness) int { } return +1 } - if l.Expiration != o.Expiration { + if !l.Expiration.EqOrdering(o.Expiration) { if l.Expiration.Less(o.Expiration) { return -1 } diff --git a/pkg/kv/kvserver/rangefeed/resolved_timestamp.go b/pkg/kv/kvserver/rangefeed/resolved_timestamp.go index 98537bb4f712..120cab958357 100644 --- a/pkg/kv/kvserver/rangefeed/resolved_timestamp.go +++ b/pkg/kv/kvserver/rangefeed/resolved_timestamp.go @@ -312,7 +312,7 @@ func (h unresolvedTxnHeap) Less(i, j int) bool { // container/heap constructs a min-heap by default, so prioritize the txn // with the smaller timestamp. Break ties by comparing IDs to establish a // total order. - if h[i].timestamp == h[j].timestamp { + if h[i].timestamp.EqOrdering(h[j].timestamp) { return bytes.Compare(h[i].txnID.GetBytes(), h[j].txnID.GetBytes()) < 0 } return h[i].timestamp.Less(h[j].timestamp) diff --git a/pkg/kv/kvserver/replica_consistency_diff.go b/pkg/kv/kvserver/replica_consistency_diff.go index 86e2f44a6f25..36e3190bce9d 100644 --- a/pkg/kv/kvserver/replica_consistency_diff.go +++ b/pkg/kv/kvserver/replica_consistency_diff.go @@ -112,7 +112,7 @@ func diffRange(l, r *roachpb.RaftSnapshotData) ReplicaSnapshotDiffSlice { case 0: // Timestamp sorting is weird. Timestamp{} sorts first, the // remainder sort in descending order. See storage/engine/doc.go. - if e.Timestamp != v.Timestamp { + if !e.Timestamp.EqOrdering(v.Timestamp) { if e.Timestamp.IsEmpty() { addLeaseHolder() } else if v.Timestamp.IsEmpty() { diff --git a/pkg/sql/sem/tree/as_of.go b/pkg/sql/sem/tree/as_of.go index 4dfc917a6197..f5c11b8beab4 100644 --- a/pkg/sql/sem/tree/as_of.go +++ b/pkg/sql/sem/tree/as_of.go @@ -132,7 +132,7 @@ func DatumToHLC(evalCtx *EvalContext, stmtTimestamp time.Time, d Datum) (hlc.Tim return ts, convErr } zero := hlc.Timestamp{} - if ts == zero { + if ts.EqOrdering(zero) { return ts, errors.Errorf("zero timestamp is invalid") } else if ts.Less(zero) { return ts, errors.Errorf("timestamp before 1970-01-01T00:00:00Z is invalid") diff --git a/pkg/storage/multi_iterator.go b/pkg/storage/multi_iterator.go index 48429cb43333..9838adf60ec7 100644 --- a/pkg/storage/multi_iterator.go +++ b/pkg/storage/multi_iterator.go @@ -155,7 +155,7 @@ func (f *multiIterator) advance() { // The iterator at iterIdx has the same key as the current best, add // it to itersWithCurrentKey and check how the timestamps compare. f.itersWithCurrentKey = append(f.itersWithCurrentKey, iterIdx) - if proposedMVCCKey.Timestamp == iterMVCCKey.Timestamp { + if proposedMVCCKey.Timestamp.EqOrdering(iterMVCCKey.Timestamp) { // We have two exactly equal mvcc keys (both key and timestamps // match). The one in the later iterator takes precedence and // the one in the earlier iterator should be omitted from diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index 3d08771990c6..32448aedba9a 100644 --- a/pkg/storage/mvcc.go +++ b/pkg/storage/mvcc.go @@ -136,7 +136,7 @@ func (k MVCCKey) Less(l MVCCKey) bool { // Equal returns whether two keys are identical. func (k MVCCKey) Equal(l MVCCKey) bool { - return k.Key.Compare(l.Key) == 0 && k.Timestamp == l.Timestamp + return k.Key.Compare(l.Key) == 0 && k.Timestamp.EqOrdering(l.Timestamp) } // IsValue returns true iff the timestamp is non-zero. diff --git a/pkg/util/span/frontier.go b/pkg/util/span/frontier.go index d06c07a6236d..0ef1c8138b68 100644 --- a/pkg/util/span/frontier.go +++ b/pkg/util/span/frontier.go @@ -60,7 +60,7 @@ func (h frontierHeap) Len() int { return len(h) } // Less implements heap.Interface. func (h frontierHeap) Less(i, j int) bool { - if h[i].ts == h[j].ts { + if h[i].ts.EqOrdering(h[j].ts) { return h[i].span.Key.Compare(h[j].span.Key) < 0 } return h[i].ts.Less(h[j].ts)