sql: rank the errors received by the DistSQLReceiver

[andreimatei]

A DistSQL flow can potentially return many errors; different sub-flows
from different nodes, and different processors within a flow, can all
generate different errors. Before this patch, the first one to make it
to the receiver was the one presented to the client. This patch adds
more smarts be chosing the "best" error. The ranking is as follows, from
high precedence to low:

• non-retriable error
• TxnAbortedError
• other retriable errors

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 7 of 7 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @asubiotto)

---

pkg/roachpb/errors.proto, line 405 at r1 (raw file):

  // TransactionAbortedError. This indicates that the client will continue with
  // a completely new transaction, not the old transaction at a different epoch.
  optional bool prev_txn_aborted = 4 [(gogoproto.nullable) = false];

How does this differ from txn_id != transaction.id? I see some discussion about "escaping from some inner transaction", but is that really a concern anymore? I'd push to avoid denormalized state on these errors. If we need to, we can add an accessor instead.

---

pkg/sql/distsql_running.go, line 265 at r1 (raw file):

}

type errorScore int

Give this a comment. What does it mean? How should it be used? Why are these the four variants?

[RaduBerinde]

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

---

pkg/sql/distsql_plan_csv.go, line 127 at r1 (raw file):

}

// OverwriteError is part of the rowResultWriter interface.

Strange to have two methods that do exactly the same thing. I know they differ in terms of documentation, but there's nothing depending on or enforcing those rules. I'd either merge them, or add a if b.err != nil { panic } to SetError

---

pkg/sql/distsql_running.go, line 265 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Give this a comment. What does it mean? How should it be used? Why are these the four variants?

[nit] maybe "precedence" instead of "score" (or "class" or "priority")

---

pkg/sql/distsql_running.go, line 584 at r1 (raw file):

}

func errScore(err error) errorScore {

[nit] since we are only using this with the pattern errScore(e1) > errScore(e2), we could make a function that takes two errors and hides all the score stuff inside

[andreimatei]

I've added a test too.

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

---

pkg/roachpb/errors.proto, line 405 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

How does this differ from txn_id != transaction.id? I see some discussion about "escaping from some inner transaction", but is that really a concern anymore? I'd push to avoid denormalized state on these errors. If we need to, we can add an accessor instead.

went with an accessor, and also used it in the client.Txn code where we need to decide whether to create a new TxnCoordSender.
Btw in upcoming changes I'm going to make this error not be a proto any more (it never should have been, but the damn Sender interface...).

---

pkg/sql/distsql_running.go, line 265 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] maybe "precedence" instead of "score" (or "class" or "priority")

went with errorPriority and added a comment

---

pkg/sql/distsql_running.go, line 584 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] since we are only using this with the pattern errScore(e1) > errScore(e2), we could make a function that takes two errors and hides all the score stuff inside

meh. I think this is a fine building block and I also have plans to use it elsewhere

---

pkg/sql/distsql_plan_csv.go, line 127 at r1 (raw file):

Previously, RaduBerinde wrote…

Strange to have two methods that do exactly the same thing. I know they differ in terms of documentation, but there's nothing depending on or enforcing those rules. I'd either merge them, or add a if b.err != nil { panic } to SetError

Well the rule was enforced in the implementation that matters - the pgwire one.
But you know what, I got rid of OverwriteErr and document SetErr to overwrite. I was on the fence already.

[andreimatei]

bors r+

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

[craig]

Build succeeded

• GitHub CI (Cockroach)