storage: augment command queue with timestamps for non-interfering reads #14342

spencerkimball · 2017-03-23T16:14:51Z

Previously, the command queue considered any two commands to overlap if the
key ranges overlapped, regardless of whether the timestamps on the two
commands guaranteed they were non-overlapping.

Consider the case of a read over key ranges "a" - "z" at timestamp t=1s.
If a write arrives for key "c" at timestamp > t=1s, then it should be
free to proceed regardless of the disposition of the read because the
read cannot affect the write's timestamp.

Similarly, if a write is active for key "c" at timestamp t=1s, a read
that arrives for key range "a" - "z" should be free to proceed as long
as its timestamp is < t=1s.

Note that local keys do not allow non-interference between earlier reads
and overlapping writes.

Fixes #14298

This change is

spencerkimball · 2017-03-23T16:15:14Z

+cc @danhhz @dt

tbg · 2017-03-23T16:20:58Z

The read can't affect the later write, but the write can affect the read's outcome because of uncertainty errors. With the change (didn't look at the code, just the PR message) it seems that we could fail to let uncertain reads fail, which in turn would lead to anomalies.

spencerkimball · 2017-03-23T16:44:41Z

@tschottdorf I don't think this is an anomaly. There is no way for the concurrent write at the higher timestamp to return and send a back channel message, back in time, to the reader such that the reader experiences a "does not read what was written previously in absolute time".

A less tongue in cheek way to say this is that the uncertainty interval errors protect us from failing to read something that provably happened before the read started.

Here's another way to look at it: imagine that the write happens at t=10, and the max offset = 0.25s. The case you bring up is that the read and write are concurrent, with read at t=9.90. You're worried that if the read is allowed to proceed with this change, the behavior will change from getting an uncertainty interval error to not getting one. But the same thing is already true if the read arrives just slightly before the write, finishes without any uncertainty error for t=9.90, and then the write arrives and puts a value at t=10.

The reason we return the uncertainty errors is to avoid the failed external guarantee of not reading what some observer can prove already was written. That's not the case with concurrent reads and writes.

danhhz · 2017-03-23T17:24:39Z

I tested this locally and can confirm it seems to fix #14298. I'll also push it to lapis to make extra sure

danhhz · 2017-03-23T17:50:19Z

Works on lapis, too 👌

tamird · 2017-03-23T18:10:41Z

Reviewed 2 of 4 files at r1.
Review status: 2 of 4 files reviewed at latest revision, 5 unresolved discussions.

pkg/storage/command_queue.go, line 309 at r1 (raw file):

		//   We solve this problem by always including read commands with timestamps less than
		//   the latest write timestamp seen so far, which guarantees that we will wait on any
		//   reads which might not be dependend on by writes with higher command IDs. Similarly,

command IDs?

pkg/storage/command_queue.go, line 337 at r1 (raw file):

				// this current command to the combined RangeGroup.
				cq.rwRg.Add(keyRange)
				if !cq.wRg.Overlaps(keyRange) || mustWait {

nit: move mustWait before the || to save a call to Overlaps.

pkg/storage/command_queue.go, line 369 at r1 (raw file):

				// dependency established with a dependent of the current overlap, meaning we already established
				// an implicit transitive dependency to the current overlap.
				if !overlapRg.Overlaps(keyRange) || mustWait {

ditto

pkg/storage/command_queue.go, line 425 at r1 (raw file):

timestamp == hlc.Timestamp{}

This condition isn't necessary to check, is it? ditto below.

pkg/storage/replica.go, line 1687 at r1 (raw file):

		// of earlier reads with later writes, but only for the global
		// command queue. Reads and writes to local keys always interfere.
		reqTS := ba.Timestamp

to make the comment above clearer, can you name this reqGlobalTS and add another var reqLocalTS hlc.Timestamp?

Comments from Reviewable

bdarnell · 2017-03-23T18:44:10Z

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

pkg/storage/command_queue.go, line 207 at r1 (raw file):

// read-only command; false for read-write.
func (cq *CommandQueue) getWait(
	readOnly bool, timestamp hlc.Timestamp, spans []roachpb.Span,

Add a comment about the timestamp argument, and the special meaning of a zero timestamp.

Comments from Reviewable

spencerkimball · 2017-03-23T19:43:34Z

Review status: all files reviewed at latest revision, 6 unresolved discussions.

pkg/storage/command_queue.go, line 207 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Add a comment about the timestamp argument, and the special meaning of a zero timestamp.

Done.

pkg/storage/command_queue.go, line 309 at r1 (raw file):