kv: disallow parallel commit of batch with read-only requests

[nvanbenschoten]

Fixes #44315.

This commit fixes an issue where KV batches with read-only requests were allowed
to be issued as parallel commits. This should not have been allowed because the
outcome of these read-only requests, notably Get and Scan requests, is not taken
into consideration by the status resolution process for STAGING transactions.

This would have caused serious atomicity issues if parallel commit batches
(non-1PC committing batches) were issued with read-only requests by users of the
KV API. Luckily, in practice that wouldn't actually happen as we never issue
batches like that so there's no concern about production clusters hitting this
bug. Still, this is a very good catch as this was a serious footgun and fixing
this hardens the KV API. We should also still backport this to v19.2 just to be
safe.

Release note: None

[cockroach-teamcity]

This change is 

[danhhz]

I could rubber stamp this, the change looks fine and like it's doing what it says it does, but would we prefer a real review?

[andreimatei]

LGTM, but to be honest I'm not sure what semantics we want for reads in the commit batch. I think you can argue for committing even if these reads fail. But we can punt until we have a better error recovery story.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @danhhz, and @nvanbenschoten)

---

pkg/kv/txn_interceptor_committer.go, line 279 at r1 (raw file):

// canBatchCommitInParallel determines whether the batch can issue its
// committing EndTxn in parallel with the rest of its requests.
func (tc *txnCommitter) canBatchCommitInParallel(

The new name doesn't mean much to me. How about just canCommitInParallel?
And I'd rephrase the comment too; it's not just about committing in parallel with the other requests in this batch, but also with the in-flight writes.

[nvanbenschoten]

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @danhhz)

---

pkg/kv/txn_interceptor_committer.go, line 279 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

The new name doesn't mean much to me. How about just canCommitInParallel?
And I'd rephrase the comment too; it's not just about committing in parallel with the other requests in this batch, but also with the in-flight writes.

Done.

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build failed

• GitHub CI (Cockroach)

[nvanbenschoten]

no space left on device then TestRandomKeyAndTimestampExport flake (#44589).

bors r+

[nvanbenschoten]

Hello bors?

{{:badmatch,
  {:error, :push, 422,
   "{\"message\":\"Required status check \\\"license/cla\\\" is expected.\",\"documentation_url\":\"https://help.github.com/articles/about-protected-branches\"}"}},
 [
   {BorsNG.Worker.Batcher, :complete_batch, 3,
    [file: 'lib/worker/batcher.ex', line: 475]},
   {BorsNG.Worker.Batcher, :maybe_complete_batch, 1,
    [file: 'lib/worker/batcher.ex', line: 456]},
   {BorsNG.Worker.Batcher, :handle_cast, 2,
    [file: 'lib/worker/batcher.ex', line: 83]},
   {:gen_server, :try_dispatch, 4,
    [file: 'gen_server.erl', line: 637]},
   {:gen_server, :handle_msg, 6,
    [file: 'gen_server.erl', line: 711]},
   {:proc_lib, :init_p_do_apply, 3,
    [file: 'proc_lib.erl', line: 249]}
 ]}

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)