Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

storage/replica_command.go, line 1400 [r1] (raw file):
this seems weird; if this lock isn't necessary, remove it.

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 1 unresolved discussion.

storage/replica_command.go, line 1400 [r1] (raw file):
It's not necessary here because I know (or at least am reasonably certain) that nobody can concurrently access it with readMu locked at the time of writing, but tsCache is one of the fields protected by the replica lock and so I should grab it regardless. I can remove the comment if you prefer that.

Comments from the review on Reviewable.io

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

storage/replica_command.go, line 1400 [r1] (raw file):
Fine as is.

Comments from the review on Reviewable.io

Can you add a test that deliberately races a split with a read-only command? It doesn't have to consistently trigger the issue but it would be good to have a more reliable and explicit test than the rare failure of sql.TestConcurrentIncrements. (which only triggered this issue because of the non-obvious fact that table creation leads to an asynchronous split)

I'll see what I can do about the test.

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

storage/replica_command.go, line 481 [r1] (raw file):
I must be thick but to me it still looks like the first batch.Defered action is the unlock, so that should be the very last thing called (i.e. all the other batch.Defered functions should be under the lock).

Comments from the review on Reviewable.io

PTAL @bdarnell. I added a test that's flaky as hell before this fix, and also took into account the fact that the batch.Defer() would not run if the commit trigger returned an error, deadlocking the range.

Reviewed 2 of 2 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

storage/client_split_test.go, line 790 [r3] (raw file):
lets

Comments from the review on Reviewable.io

TestRaftRemoveRace fails very reliably on this branch. The new lock must've changed timings in a way that make the errors much more likely. See #3178 #3010.

LGTM

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, 1 of 1 files at r4.
Review status: 1 of 4 files reviewed at latest revision, 1 unresolved discussion.

storage/replica_command.go, line 481 [r1] (raw file):
No, it's me being thick; I looked at batch.Defer and didn't notice we reversed the list when we called the functions.

Comments from the review on Reviewable.io