storage: Move range bounds and GC threshold check out of evaluateBatch

[bdarnell]

Performing this check at evaluation time (and only at evaluation time)
required that we declare read access to the descriptor and GC
threshold for every command (leading to stalls when splits,
rebalances, and GCs had to get a write lock on the same keys).

Instead, with this commit, we do this check once at evaluation
time (because some of the evaluation functions don't fail gracefully
if they are run in completely invalid circumstances) and again when
applying the command downstream of raft. By repeating the check, we no
longer need to hold a read lock for the entire duration of the
command.

Note that even with this change, GC requests generated by the gcQueue
will still block everything else because the command is sent with a
very large span, but this can be fixed without backwards-compatibility
problems. All EndTransaction requests need to read the range
descriptor, which means that a rebalance operation will block all
writing transactions (splits also block writing transactions, but note
that a split necessarily blocks all writes to the RHS of the range).

This is a backwards-compatible change for clusters without propEvalKV:
these checks were already happening downstream of raft so there has
been no real change. It is backwards-incompatible for any clusters
running with propEvalKV and they will need to be wiped.

[cockroach-teamcity]

This change is 

[petermattis]

I think we want to do this. A quick hack shows that not adding those two read-only keys improves performance of simple read-only commands by ~20%.

[bdarnell]

OK, I've updated this and it's ready for review. We'll still get that ~20% improvement for reads, but unfortunately it doesn't help as much for transactional writes (which includes all SQL writes): all EndTransaction calls need to depend on the range descriptor to determine which intents are within the range (there are some things we could do to improve this, but they'll be post-1.0. with propEvalKV, we should be able to make the necessary changes in a backwards-compatible way).

[petermattis]

Review status: 0 of 8 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

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

	if err := r.requestCanProceed(rSpan, ba.Timestamp); err != nil {
		return nil, nil, err
	}

Should this check be moved about r.raftMu.Lock()?

---

pkg/storage/replica_command.go, line 550 at r1 (raw file):

	// All transactions depend on the range descriptor because they need
	// to determine which intents are within the local range.
	spans.Add(SpanReadOnly, roachpb.Span{Key: keys.RangeDescriptorKey(desc.StartKey)})

It's unfortunate that this applies to 1PC transactions as well.

I'm wondering if we could include the range span in the proposal and check that down stream of Raft, failing commands for which the range span has changed. I haven't thought through this at all, so pardon the drive-by comment.

---

pkg/storage/replica_proposal.go, line 545 at r1 (raw file):

	// The above are always present, so we assert only if there are
	// "nontrivial" actions below.
	shouldAssert = !reflect.DeepEqual(rResult, storagebase.ReplicatedEvalResult{})

That's unfortunate. I think @tschottdorf had worked hard to avoid using reflect.DeepEqual here. I don't have a good suggestion for an alternative, though.

---

Comments from Reviewable

[spencerkimball]

Review status: 0 of 8 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/storage/replica_proposal.go, line 545 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

That's unfortunate. I think @tschottdorf had worked hard to avoid using reflect.DeepEqual here. I don't have a good suggestion for an alternative, though.

One option is to not enable this check (and others like it) by default, but only in test clusters.

---

Comments from Reviewable

[bdarnell]

Review status: 0 of 8 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

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

Previously, petermattis (Peter Mattis) wrote…

Should this check be moved about r.raftMu.Lock()?

It was happening under r.raftMu before, and it's quick enough that I'd rather not risk additional change here.

---

pkg/storage/replica_command.go, line 550 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

It's unfortunate that this applies to 1PC transactions as well.

I'm wondering if we could include the range span in the proposal and check that down stream of Raft, failing commands for which the range span has changed. I haven't thought through this at all, so pardon the drive-by comment.

Yeah, we could do something like that, but it's a tricky thing to be introducing at this stage. We'd need to introduce new error codes that would be retried at the appropriate level, etc. We'll have more flexibility to handle this later, when propEvalKV is the only way (2PC EndTransactions are always sent alone so we can change the ReplicatedEvalResult RSpan to reflect the intents we resolved. Error retries will still be slightly tricky, but should be manageable).

---

pkg/storage/replica_proposal.go, line 545 at r1 (raw file):

Previously, spencerkimball (Spencer Kimball) wrote…

One option is to not enable this check (and others like it) by default, but only in test clusters.

Yeah, if this turns out to be a problem we could turn off the check for production use. But it doesn't seem likely to be a significant cost to me. (Looking back at #10327 there was some discussion about the convenience of == but not much about performance)

---

Comments from Reviewable

[petermattis]

LGTM

…

On Wed, Apr 12, 2017 at 8:12 PM Ben Darnell ***@***.***> wrote: Review status: 0 of 8 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. ------------------------------ *pkg/storage/replica.go, line 2529 at r1 <https://reviewable.io:443/reviews/cockroachdb/cockroach/14833#-KhZ9gTC9N6N2VGy7uMU:-KhZSODuNh3ZiFbILLzQ:b-g24j5u> (raw file <https://github.com/cockroachdb/cockroach/blob/bf4b9163933bfe4ab5fe971c7588235800e8fb91/pkg/storage/replica.go#L2529>):* *Previously, petermattis (Peter Mattis) wrote…* Should this check be moved about r.raftMu.Lock()? It was happening under r.raftMu before, and it's quick enough that I'd rather not risk additional change here. ------------------------------ *pkg/storage/replica_command.go, line 550 at r1 <https://reviewable.io:443/reviews/cockroachdb/cockroach/14833#-KhZAcVvb7-duwhPbv4g:-KhZTpIqY-WhMn674Kmg:b-c24mbq> (raw file <https://github.com/cockroachdb/cockroach/blob/bf4b9163933bfe4ab5fe971c7588235800e8fb91/pkg/storage/replica_command.go#L550>):* *Previously, petermattis (Peter Mattis) wrote…* It's unfortunate that this applies to 1PC transactions as well. I'm wondering if we could include the range span in the proposal and check that down stream of Raft, failing commands for which the range span has changed. I haven't thought through this at all, so pardon the drive-by comment. Yeah, we could do something like that, but it's a tricky thing to be introducing at this stage. We'd need to introduce new error codes that would be retried at the appropriate level, etc. We'll have more flexibility to handle this later, when propEvalKV is the only way (2PC EndTransactions are always sent alone so we can change the ReplicatedEvalResult RSpan to reflect the intents we resolved. Error retries will still be slightly tricky, but should be manageable). ------------------------------ *pkg/storage/replica_proposal.go, line 545 at r1 <https://reviewable.io:443/reviews/cockroachdb/cockroach/14833#-KhZBKzmEDxw6cRRp4W5:-KhZVVZ0Uep--8PpZLtn:byyqrrd> (raw file <https://github.com/cockroachdb/cockroach/blob/bf4b9163933bfe4ab5fe971c7588235800e8fb91/pkg/storage/replica_proposal.go#L545>):* *Previously, spencerkimball (Spencer Kimball) wrote…* One option is to not enable this check (and others like it) by default, but only in test clusters. Yeah, if this turns out to be a problem we could turn off the check for production use. But it doesn't seem likely to be a significant cost to me. (Looking back at #10327 <#10327> there was some discussion about the convenience of == but not much about performance) ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/cockroachdb/cockroach/14833>* — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#14833 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF6f936WsenauJkoedlzeIPjeSD65qZFks5rvWh5gaJpZM4M68cb> .