roachpb: remove deprecated fields from Error

[tbg]

CRDB 21.2 already populates Errors.EncodedError, so we can remove the
deprecated fields.

I was motivated to do this when working on a follow-up to #73602 and
ended up trying to marshal an Error to JSON, which panics since
it ends up trying to use reflect to get an unexported value (due
to the customname tags on the deprecated fields). This behavior
persists even with the json: "-" tag, by the way, indicating
that we should never use (gogoproto.customname) to make a field
private. It's also just one more papercut that comes with using
gogoproto but that's a much bigger fish to fry.

cc @lidorcarmel since you're just poking at Error.

Release note: None

[cockroach-teamcity]

This change is 

[tbg]

I typed this up at the end of my day and had to send it out in a bit of a rush, so reviewers should feel comfortable waiting until I've given myself a self-review early next week. This stuff is slightly terrifying, don't want to know what will go wrong if there's a bug in here.

[tbg]

note for next week: I can also remove internalError, this is now no longer instantiated and so doesn't have to be handled.

another thread that I should pull on: the loss of fidelity in GoError() is bad. We should have this:

type GoError struct {
  wrapped error // corresponds to pErr.EncodedError, we implement stuff for `errors.Is` etc
  Index int
  // other fields too
}

func (err *GoError) AsProto() *Error {
  return &Error{
    // populate all fields from `err`
  }
}

and then

func (e *Error) GoError() {
  return &GoError{
    wrapped: e.decode(),
    Index: e.Index,
    // ...
  }
}

func NewError(err error) {
  var goErr error
  if errors.As(err, &goErr) {
    return goErr.AsProto()
  }
  return &Error{ /* ... */ }
}

and here we have it, require.Equal(t, NewError(pErr.GoError()), pErr).

[tbg]

Actually rather than having *Error and a Go version of it, the *Error should just implement error. We shied away from that in the past since we were storing errors inside of Error and we were worried that the semantics would get super confusing, but now that we're just using cockroachdb/errors throughout (assuming I've removed the loss of fidelity mentioned above) there doesn't seem to be a reason to avoid it any more.

@knz would appreciate your eyes on the thinking.

[knz]

I think this is a step in the right direction. What are the open questions that remain?

(I'm a little bit confused by roachpb.internalError. What purpose does it serve?)

Reviewed 2 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)

---

pkg/roachpb/errors.go, line 151 at r2 (raw file):

		return nil
	}
	if intErr, ok := err.(*internalError); ok {

Can you walk me through the role of internalError? What is this for? Why does it hide its cause?

---

pkg/roachpb/errors.go, line 187 at r2 (raw file):

// SafeFormat implements redact.SafeFormatter.
func (e *Error) SafeFormat(s redact.SafePrinter, _ rune) {

If/when *Error is meant to become an error, you may want to implement errors.SafeFormatter instead. This may be worth adding a TODO.

---

pkg/roachpb/errors.go, line 230 at r2 (raw file):

func (e *Error) TransactionRestart() TransactionRestart {
	var iface transactionRestartError
	if errors.As(e.decode(), &iface) {

Is there a way to memoize the result of decoding, so we don't end up doing it on every api call that uses .decode()? Decoding is rather expensive.

[tbg]

@ajwerner did this in #86683 🥳