hlc: introduce synthetic flag on timestamps #56373

nvanbenschoten · 2020-11-06T17:50:07Z

Informs #52745.
Informs #36431.

This commit introduces an 8-bit flags field on the hlc timestamp struct. The flags are used to provide details about the timestamp and its meaning. They do not affect the sort order of Timestamps.

The commit then introduces the first flag: SYNTHETIC. As discussed in #52745, a synthetic timestamp is defined as a timestamp that makes no claim about the value of clocks in the system. While standard timestamps are pulled from HLC clocks and indicate that some node in the system has a clock with a reading equal to or above its value, a synthetic timestamp makes no such indication. By avoiding a connection to "real time", synthetic timestamps can be used to write values at a future time and to indicate that observed timestamps do not apply to such writes for the purposes of tracking causality between the write and its observers. Observed timestamps will be a critical part of implementing non-blocking transactions (#52745) and fixing the interaction between observed timestamps and transaction refreshing (#36431).

The original plan was to reserve the high-order bit in the logical portion of a timestamp as a "synthetic bit". This is how I began implementing things, but was turned off for a few reasons. First, it was fairly subtle and seemed too easy to get wrong. Using a separate field is more explicit and avoids a class of bugs. Second, I began to have serious concerns about how the synthetic bit would impact timestamp ordering. Every timestamp comparison would need to mask out the bit or risk being incorrect. This was even true of the LSM custom comparator. This seemed difficult to get right and seemed particularly concerning since we're planning on marking only some of a transaction's committed values as synthetic to fix #36431, so if we weren't careful, we could get atomicity violations. There were also minor backwards compatibility concerns.

But a separate field is more expensive in theory, so we need to be careful. However, it turns out that a separate field is mostly free in each case that we care about. In memory, the separate field is effectively free because the Timestamp struct was previously 12 bytes but was always padded out to 16 bytes when included as a field in any other struct. This means that the flags field is replacing existing padding. Over the wire, the field will not be included when zero and will use a varint encoding when not zero, so again, it is mostly free. In the engine key encoding, the field is also not included when zero, and takes up only 1 byte when non-zero, so it is mostly free.

First three commits from #56477.

@sumeerbhola I'm hoping you can take a look at the engine-level changes in the introduce synthetic flag on timestamps commit (4th commit as of the time of writing). I think the key encoding added here makes sense, but want to make sure you're on board. One possible concern is that we introduce a new 13-byte suffix, which means that combined with a 4-byte sequence number (see #41720 (comment)), we'd collide with the 17 byte engineKeyVersionLockTableLen.

@tbg do you mind being the primary reviewer here? I think you know the most about the motivations for this change and will have a good sense of whether this is the best way to introduce additional state on timestamps.

cockroach-teamcity · 2020-11-06T17:50:24Z

This change is

tbg · 2020-11-06T20:30:33Z

Happy to review, but it will be on Tuesday.

…

On Fri, Nov 6, 2020, 18:50 cockroach-teamcity ***@***.***> wrote: This change is [image: Reviewable] <https://reviewable.io/reviews/cockroachdb/cockroach/56373> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56373 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGXPZCCBYFRNAD6WUSTAN3SOQZPBANCNFSM4TM642XQ> .

…rved timestamps This commit introduces a new package which contains logic and documentation related to the observed timestamp system, which allows transactions to track causality between themselves and other, possibly-concurrent, transactions in order to avoid uncertainty related restarts. This ties into our efforts to better document the kv layer. It also ties into synthetic timestamps (cockroachdb#56373), which are important for non-blocking transactions (cockroachdb#52745) and for fixing the interaction between observed timestamps and transaction refreshing (cockroachdb#36431). The proposed fix is to split out a new EffectiveMaxTimestamp field from the MaxTimestamp field on a transaction and set the EffectiveMaxTimestamp in LimitTxnMaxTimestamp. The EffectiveMaxTimestamp field will dictate the uncertainty interval for values with normal timestamps while the original MaxTimestamp will dictate the uncertainty interval for values with synthetic timestamps. In practice, this will mean that observed timestamps do not apply to values with synthetic timestamps. This commit gives us a package to house this new complexity.

sumeerbhola

minor comments only -- I haven't yet looked at the last 2 commits which tbg will be reviewing, but I'll try to follow along.

Reviewed 12 of 12 files at r1, 1 of 1 files at r2, 5 of 5 files at r3, 27 of 29 files at r4, 1 of 1 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten, @sumeerbhola, and @tbg)

pkg/roachpb/data.go, line 1101 at r4 (raw file):

		}

		if t.ReadTimestamp == o.ReadTimestamp {

why is the synthetic bit relevant here?

pkg/util/hlc/legacy_timestamp.proto, line 41 at r4 (raw file):

  // == operator). Consider use of the EqOrdering method when testing for
  // equality.
  optional uint32 flags = 3;

is this nullable because it will be serialized in MVCCMetadata and we don't want to pay the serialized cost when it is not set?

pkg/util/hlc/timestamp.go, line 272 at r4 (raw file):

func (t *Timestamp) Backward(s Timestamp) {
	// TODO(nvanbenschoten): if either timestamp is non-synthetic, we can remove
	// the synthetic bit.

Maybe phrase this TODO as: remove the synthetic bit from the older value if the younger one is non-synthetic.

pkg/util/hlc/timestamp.go, line 283 at r4 (raw file):

}

// ToLegacyTimestamp converts a Timestamp to a LegacyTimestamp.

Is it possible to get rid of LegacyTimestamp?

nvanbenschoten

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

pkg/roachpb/data.go, line 1101 at r4 (raw file):

Previously, sumeerbhola wrote…

why is the synthetic bit relevant here?

It isn't really relevant. This is just switching from Equal to ==, which has the same semantics but it more idiomatic to test for structural equality.

pkg/util/hlc/legacy_timestamp.proto, line 41 at r4 (raw file):

Previously, sumeerbhola wrote…

is this nullable because it will be serialized in MVCCMetadata and we don't want to pay the serialized cost when it is not set?

Kind of. It's nullable because we need identical encoding below Raft to ensure that all replicas stay bytewise identical even in a mixed version cluster. This is the same reason why we've never been able to completely remove LegacyTimestamp. So since the field won't even exist on v20.2 nodes, we need to make sure it doesn't get serialized on v21.1 nodes.

pkg/util/hlc/timestamp.go, line 272 at r4 (raw file):

Previously, sumeerbhola wrote…

Maybe phrase this TODO as: remove the synthetic bit from the older value if the younger one is non-synthetic.

Done.

pkg/util/hlc/timestamp.go, line 283 at r4 (raw file):

Previously, sumeerbhola wrote…

Is it possible to get rid of LegacyTimestamp?

I'm trying, but it's putting up a fight: #56190. It's also used in MVCCMetadata, which is written to "below Raft" in a number of places. I think we're going to need to run an online migration if we ever want to remove it entirely.

tbg

I'm still anxious that synthetic and regular timestamps will have an opportunity to mix when they shouldn't, given how much existing code is out there. In particular, .Forward() and .Backward() seem susceptible. I wish we had proper enum types.
From plowing through the code so far, are you worried at all? And/or do you have ideas on getting at least some amount of separation between them back (clearly separate types are not an option)?

Reviewed 41 of 41 files at r6, 4 of 4 files at r7, 1 of 1 files at r8, 5 of 5 files at r9, 35 of 35 files at r10, 2 of 2 files at r11, 53 of 53 files at r12, 8 of 8 files at r13.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)

pkg/util/hlc/legacy_timestamp.proto, line 41 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Kind of. It's nullable because we need identical encoding below Raft to ensure that all replicas stay bytewise identical even in a mixed version cluster. This is the same reason why we've never been able to completely remove LegacyTimestamp. So since the field won't even exist on v20.2 nodes, we need to make sure it doesn't get serialized on v21.1 nodes.

Mind putting that in the comment?

pkg/util/hlc/timestamp.go, line 257 at r10 (raw file):

// Forward updates the timestamp from the one given, if that moves it forwards
// in time. Returns true if the timestamp was adjusted and false otherwise.

Clarify the language in the comment:

// Forward replaces the receiver with the argument, if that moves it forwards in time.

pkg/util/hlc/timestamp.go, line 269 at r10 (raw file):

// Backward updates the timestamp from the one given, if that moves it
// backwards in time.

ditto about clarifying language

pkg/util/hlc/timestamp.proto, line 51 at r10 (raw file):

  // timestamp makes no such indication. By avoiding a connection to "real
  // time", synthetic timestamps can be used to write values at a future
  // time and to indicate that observed timestamps do not apply to such

I feel that nobody will understand this in the future. Perhaps

// Synthetic timestamps are central to non-blocking transactions, which essentially write at "future timestamps". They are also used in observed timestamps, where they indicate versions that were moved from the timestamp at which they were originally written; only synthetic timestamps require observing the full uncertainty interval, whereas readings off the node's clock can tighten it for non-synthetic versions.

pkg/util/hlc/timestamp_test.go, line 46 at r10 (raw file):

	}
	a = makeTS(1, 1)
	if a.EqOrdering(b) {

curious why you didn't like require.False(t, a.EqOrdering(b), "%+v == %+v", a, b)

pkg/util/hlc/timestamp_test.go, line 207 at r10 (raw file):

		assert.Equal(t, c.exp, c.ts.String())

		// Flags don't round-trip through parsing, so ignore them.

Shouldn't they? If not, at least update comment on ParseTimestamp.

pkg/util/span/frontier.go, line 63 at r13 (raw file):

// Less implements heap.Interface.
func (h frontierHeap) Less(i, j int) bool {
	if h[i].ts.EqOrdering(h[j].ts) {

Can you leave that _ func() field on the struct to disable explicit comparisons going forward on both timestamp types?

bytes no_equality = 3 [(gogoproto.nullable) = true, (gogoproto.customname) = "_"];

worked nicely in my local testing.

nvanbenschoten

TFTRs!

I'm still anxious that synthetic and regular timestamps will have an opportunity to mix when they shouldn't, given how much existing code is out there. In particular, .Forward() and .Backward() seem susceptible. I wish we had proper enum types.
From plowing through the code so far, are you worried at all? And/or do you have ideas on getting at least some amount of separation between them back (clearly separate types are not an option)?

My current plan is to never allow synthetic timestamps to do any harm by fataling if we ever call hlc.Update with a synthetic timestamp. This effectively means that we need to separate out the "transaction time domain" from the "clock time domain", and we'll be made aware pretty quickly if these ever mix. We already have this separation in some places. For instance, we have a BatchRequest.Timestamp and a BatchResponse.Now, which are independent from the timestamps passed around in Transaction objects.

My other thought is that I'm going to split hlc.Update into two methods – hlc.UpdateForStability and hlc.UpdateForCausality. This doesn't directly tie in to the discussion here, but it means that we're going to need to be a little more explicit about when, where, and why we update the hlc clock. I expect that this will uncover cases where timestamps from the "transaction time domain" and leaking back into the "clock time domain".

One more aggressive idea to enforce this separation is to use another flag to indicate which domain a timestamp is part of. A conversion from a "clock timestamp" to a "transaction timestamp" would be allowed, but not the other way around. We could then assert that no "transaction timestamp" (synthetic or otherwise) ever makes its way back into a clock. My primary concern with this would be its impact on performance. We'd probably want to make the transaction domain the zero value for the flag.

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

pkg/util/hlc/legacy_timestamp.proto, line 41 at r4 (raw file):