kv/storage: introduce local timestamps for MVCC versions in MVCCKey

[nvanbenschoten]

Fixes #36431.
Fixes #49360.
Replaces #72121.

This commit fixes the potential for a stale read as detailed in #36431 using the "remember when intents were written" approach described in #36431 (comment) and later expanded on in #72121 (comment).

type MVCCKey struct {
	Key            roachpb.Key
	Timestamp      hlc.Timestamp
+	LocalTimestamp hlc.ClockTimestamp
}

This bug requires a combination of skewed clocks, multi-key transactions split across ranges whose leaseholders are stored on different nodes, a transaction read refresh, and the use of observed timestamps to avoid an uncertainty restart. With the combination of these four factors, it was possible to construct an ordering of events that violated real-time ordering and allowed a transaction to observe a stale read. Upon the discovery of the bug, we introduced the multi-register test to the Jepsen test suite, and have since observed the test fail when combined with the strobe-skews nemesis due to this bug in #49360 (and a few issues linked to that one). This commit stabilizes that test.

Explanation

The combination of all of the factors listed above can lead to the stale read because it breaks one of the invariants that the observed timestamp infrastructure¹ relied upon for correctness. Specifically, observed timestamps relied on the guarantee that a leaseholder's clock must always be equal to or greater than the version timestamp of all writes that it has served. However, this guarantee did not always hold. It does hold for non-transactional writes. It also holds for transactions that perform all of their intent writes at the same timestamp and then commit at this timestamp. However, it does not hold for transactions which move their commit timestamp forward over their lifetime before committing, writing intents at different timestamps along the way and "pulling them up" to the commit timestamp after committing.

In violating the invariant, this third case reveals an ambiguity in what it means for a leaseholder to "serve a write at a timestamp". The meaning of this phrase is straightforward for non-transactional writes. However, for an intent write whose original timestamp is provisional and whose eventual commit timestamp is stored indirectly in its transaction record at its time of commit, the meaning is less clear. This reconciliation to move the intent write's timestamp up to its transaction's commit timestamp is asynchronous from the transaction commit (and after it has been externally acknowledged). So even if a leaseholder has only served writes with provisional timestamps up to timestamp 100 (placing a lower bound on its clock of 100), it can be in possession of intents that, when resolved, will carry a timestamp of 200. To uphold the real-time ordering property, this value must be observed by any transaction that begins after the value's transaction committed and was acknowledged. So for observed timestamps to be correct as currently written, we would need a guarantee that this value's leaseholder would never return an observed timestamp < 200 at any point after the transaction commits. But with the transaction commit possibly occurring on another node and with communication to resolve the intent occurring asynchronously, this seems like an impossible guarantee to make.

This would appear to undermine observed timestamps to the point where they cannot be used. However, we can claw back correctness without sacrificing performance by recognizing that only a small fraction² of transactions commit at a different timestamps than the one they used while writing intents. We can also recognize that if we were to compare observed timestamps against the timestamp that a committed value was originally written (its provisional value if it was once an intent) instead of the timestamp that it had been moved to on commit, then the invariant would hold.

This commit exploits this second observation by adding a second timestamp to each MVCC key called the "local timestamp". The MVCC key's existing version timestamp dictates the key's visibility to readers and is tied to the writer's commit timestamp. The local clock timestamp records the value of the local HLC clock on the leaseholder when the key was originally written. It is used to make claims about the relative real time ordering of the key's writer and readers when comparing a reader's uncertainty interval (and observed timestamps) to the key. Ignoring edge cases, readers with an observed timestamp from the key's leaseholder that is greater than the local clock timestamp stored in the key cannot make claims about real time ordering and must consider it possible that the key's write occurred before the read began. However, readers with an observed timestamp from the key's leaseholder that is less than the clock timestamp can claim that the reader captured that observed timestamp before the key was written and therefore can consider the key's write to have been concurrent with the read. In doing so, the reader can avoid an uncertainty restart.

For more, see the updates made in this commit to pkg/kv/kvserver/observedts/doc.go.

To avoid the bulk of the performance hit from adding this new timestamp to the MVCC key encoding, the commit optimizes the clock timestamp away in the common case where it leads the version timestamp. Only in the rare cases where the local timestamp trails the version timestamp (e.g. future-time writes, async intent resolution with a new commit timestamp) does the local timestamp need to be explicitly represented in the key encoding. This is possible because it is safe for the local clock timestamp to be rounded down, as this will simply lead to additional uncertainty restarts. However, it is not safe for the local clock timestamp to be rounded up, as this could lead to stale reads.

Future improvements

As noted in #72121 (comment), this commit paves a path towards the complete removal of synthetic timestamps, which were originally introduced in support of non-blocking transactions and GLOBAL tables.

The synthetic bit's first role of providing dynamic typing for ClockTimestamps is no longer necessary now that we never need to "push" transaction-domain timestamps into HLC clocks. Instead, the invariant that underpins observed timestamps is enforced by "pulling" local timestamps from the leaseholder's HLC clock.

The synthetic bit's second role of disabling observed timestamps is replaced by the generalization provided by "local timestamps". Local timestamps precisely track when an MVCC version was written in the leaseholder's clock timestamp domain. This establishes a total ordering across clock observations (local timestamp assignment for writers and observed timestamps for readers) and establish a partial ordering between writer and reader transactions. As a result, the use of observed timestamps during uncertainty checking becomes a comparison between two ClockTimestamps, the version's local timestamp and the reader's observed timestamp.

Correctness testing

I was not able to stress jepsen/multi-register/strobe-skews hard enough to cause it to fail, even on master. We've only seen the test fail a handful of times over the past few years, so this isn't much of a surprise. Still, this prevents us from saying anything concrete about an reduced failure rate.

However, the commit does add a new test called TestTxnReadWithinUncertaintyIntervalAfterIntentResolution which controls manual clocks directly and was able to deterministically reproduce the stale read before this fix in a few different ways. After this fix, the test passes.

Performance analysis

This correctness fix will lead to an increased rate of transaction retries under some workloads.

TODO(nvanbenschoten):

• observe TPC-C performance
  -- top-line performance
  -- uncertainty retry rate
  -- commit-wait rate (should be zero)
• compare YCSB performance
• compare KV performance

---

Release note (bug fix): fixed a rare race condition that could allow for a transaction to serve a stale read and violate real-time ordering under moderate clock skew.

Release justification: None. This will not be merged before the v22.1 release branch is cut.

Jira issue: CRDB-14815

Footnotes

1. see pkg/kv/kvserver/observedts/doc.go for an explanation of the role of observed timestamps in the transaction model. This commit updates that documentation to include this fix. ↩

2. see analysis in https://github.com/cockroachdb/cockroach/issues/36431#issuecomment-714221846. ↩

[cockroach-teamcity]

This change is 

[sumeerbhola]

(read a bit of this -- flushing comments)

Reviewed 4 of 7 files at r1, 5 of 14 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten, @stevendanna, and @sumeerbhola)

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 648 at r1 (raw file):

	if linearizable && !readOnly {
		waitUntil = waitUntil.Add(tc.clock.MaxOffset().Nanoseconds(), 0)
	}

Would be nice to have a comment here on what kinds of transactions won't need to wait.

---

pkg/kv/kvserver/replica_send.go, line 268 at r1 (raw file):

	// evaluation.
	if txn.WriteTimestamp.LessEq(r.Clock().Now()) {
		return nil // no wait fast-path

same comment here

---

pkg/kv/kvserver/replica_tscache.go, line 62 at r1 (raw file):

	// property this is asserting is no longer quite as important, so we can
	// disable the check. However, it would still be nice to track down how we
	// can hit this.

was this TODO stale and has been fixed?

---

pkg/kv/kvserver/concurrency/lock_table_waiter.go, line 573 at r2 (raw file):

		// resolve the intent to timestamp 11 but leave its local timestamp at 8
		// then the reader would consider the value "uncertain" upon re-evaluation.
		// However, if the reader also updates the value's local timestamp to 16

In this example will this mean that we don't need to write the local timestamp since it is >= txn commit timestamp?

---

pkg/kv/kvserver/concurrency/lock_table_waiter.go, line 589 at r2 (raw file):

		// commits and acknowledges its client. This could not lead to the pusher
		// serving a stale read, but it could lead to other transactions serving
		// stale reads.

I see what this is saying, but a small example here would make it super clear for a reader.

---

pkg/roachpb/api.proto, line 1142 at r2 (raw file):

  // from a different node than the node which evaluates the ResolveIntent
  // request, it will be ignored and the intent's local timestamp will not be
  // changed.

This could use a short example of the hazard in the absence of this ignore behavior.

[erikgrinaker]

doesn't look like MVCCStats computation has been updated to account for the extra timestamp.

The MVCCStats computation deliberately avoids taking the variable-length timestamp component into account and assumes all engine key versions are 12 bytes large, which can be an overestimate or an underestimate. This has always been the case.
However, with this change, the degree to which we can underestimate gets 12 bytes worse. Still, I'm inclined to not change this for two reasons. First, it will be difficult to make this precise. Doing so will require lifting and leaking the decision about when to omit the local timestamp from the encoding routines up into most MVCC logic. Second, I think it might be wise to keep the door open to LSM compaction filters stripping this local timestamp once it is no longer needed. As a strawman, we could do this for key-values once they fall below their range's GC threshold.

I see. The reason I ask is that I've implemented MVCC stats for range keys in #78085 using the actual encoded size of timestamps, with the hope that we would eventually fix this for point keys too (we have TODOs for it). Since range tombstones would presumably also need local timestamps, these would contribute their actual encoded size to the statistics.

Today with the way that KV, MVCC, and Storage are layered, the MVCC and storage layers operate on decoded engine/mvcc keys but encoded (opaque) values. That makes manipulation of keys more natural than manipulation of values in the layer that owns this "local timestamp" concept.

Sort of. MVCCPut takes a roachpb.Value, and MVCCGet and MVCCScan both return roachpb.Value, which would store the local timestamp -- but it's true that this encoding/decoding happens near the MVCC API boundaries, and in the case of scans returning encoded binary KV pairs it might get prohibitively expensive to decode these on the server-side during scanning. Similarly for intent rewrites. So it's quite likely that the performance gains here are worth it, despite the unpleasant aesthetics.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @stevendanna and @sumeerbhola)

[HonoreDB]

While I'm here to look at the cdc impact: have you considered storing this as an offset or xor from the hlc.Timestamp with a smaller or variable-length encoding?

Reviewed 11 of 137 files at r21.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @stevendanna and @sumeerbhola)

[sumeerbhola]

Reviewed 1 of 134 files at r15, 1 of 132 files at r20, 2 of 137 files at r21.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten, @stevendanna, and @sumeerbhola)

---

pkg/storage/mvcc.go, line 1285 at r21 (raw file):

// assigned an explicit local timestamp. The effect of this is that
// readers treat the local timestamp as being equal to the version
// timestamp.

When is it safe for the the putter to say "assume local timestamp == version timestamp"?
Is it always safe for non-blocking txns (including a txn rewriting its own intent) because this is a put and the use of a version timestamp implies that the local clock has been advanced? Or is it not true even in this case because "we never need to "push" transaction-domain timestamps into HLC clocks"?
I would like the comment to also define the safety requirement for using hlc.ClockTimestamp{}

[nvanbenschoten]

Since range tombstones would presumably also need local timestamps, ...

This is a good point. We'll want to make sure that whatever solution we come to here works well for range tombstones. My reading of #77762 and of cockroachdb/pebble@5d99779 is that the timestamp suffix that we typically append to the key for point KeyKinds is stored in the value of range keys (InternalKeyKindRangeKeySet, InternalKeyKindRangeKeyUnset, InternalKeyKindRangeKeyDelete). On the surface, that would seem to indicate that the choice of where to store the local timestamp for point keys does not impact range tombstones (suffix vs. value).

However, the semantics of RangeKeyUnset make this more nuanced. Currently, RangeKeyUnset says that "only range keys with suffixes that exactly match the unset suffix" will be removed. One way to read this is that anything that does not affect key equality cannot end up in the range key suffix and that range key equality is defined using byte equality of the suffix. Another way to read this is that range key equality should be using the pebble.Comparer functions for range keys to define ordering and equality of range keys like it does for point keys, but that it is not doing so yet.

I'd like to make sure that any choice we make here will be consistently applied to range keys and that I'm not landing a large hurdle in the path of range keys. @erikgrinaker do you mind if I set up a quick chat for when you're back to bottom out on a few of these questions?

MVCCPut takes a roachpb.Value, and MVCCGet and MVCCScan both return roachpb.Value, which would store the local timestamp -- but it's true that this encoding/decoding happens near the MVCC API boundaries

In many cases, the encoding/decoding is actually far away from the MVCC API boundaries. It's often all the way up in the SQL layer on the client. In the common-path of a SQL INSERT, the roachpb.Value (which is little more than a wrapper around a custom encoded byte slice) is encoded in SQL and treated as opaque below this level. That makes extending roachpb.Value with additional metadata unpleasant and also difficult to do without a meaningful performance impact (i.e. due to re-allocating and re-encoding the value in MVCC, then decoding and stripping the metadata during performance-sensitive scans).

An alternative that I've begun to explore is introducing a wrapper value in the MVCC layer that carries a roachpb.Value along with some optional metadata. Imagine a storage.MVCCValue struct which wraps a roachpb.Value in much the same way that a storage.MVCCKey wraps a roachpb.Key. There would be some challenges to a solution like this, including backwards compatibility and avoiding the encoding/decoding performance hit in the common case, but we may be able to rely on local_timestamps being rare and (ab)use the roachpb.Value tag to indicate that the value is wrapped or not, allowing for an extended encoding scheme only in the cases where it is.

While I'm here to look at the cdc impact: have you considered storing this as an offset or xor from the hlc.Timestamp with a smaller or variable-length encoding?

This change should have little impact to CDC. Local timestamps will be stripped from the key below Raft before they end up in a rangefeed and will never make it up to a changefeed.

I considered storing the local timestamp as an offset from the hlc.Timestamp, but that doesn't get us anything without a variable-length encoding. A variable-length encoding itself is difficult because we rely on known key suffix sizes when decoding MVCCKeys. Given how rarely we'll need to include this field in practice (< 0.3% of writes in TPC-C), it didn't seem worth the complexity beyond making the encoding variable-length with respect to the presence or absence of a logical timestamp component.

[erikgrinaker]

Sure @nvanbenschoten, let's have a chat when I'm back.

[nvanbenschoten]

Replaced by #80706.