*: demonstrate uses of the errors library

[knz]

This PR adapts various places in CockroachDB to demonstrate benefits of the errors library introduced in #36987 / #37121.

The main goals of this PR is to remove most usages of pgerror.Error, and replace them by regular error objects.

The benefit of this change is that all errors details are then preserved throughout distsql execution.

This is achieved by the following high level steps:

1. the introduction of a new function pgerror.Flatten() that can convert a regular error into a *pgerror.Error.
   This step occurs in a commit in the PR near the beginning, called "pgerror: add Flatten() to convert errors into pgerror.Error"

   This function is meant to be used at "boundaries" only:

   • when sending an error packet to a SQL client
   • when reporting a distsql error to a v19.1 sql gateway

2. changing the implementation of the pgerror.XXX() constructor functions to use the new library's constructors instead.
   This step is achieved by another commit in the PR near the end, called "pgwire, distsqlpb: let native errors flow through"

   From that point, most of the in-flight error objects inside CockroachDB are not instances of pgerror.Error any more.
   (The exception is a pgerror.Error object received on a distsql flow from a v19.1 node).
   This is thus also the moment where calls to pgerror.Flatten() really become necessary at the aforementioned boundaries.

The end result is that most of the "interior" of CockroachDB sees plain error objects and cannot expect a *pgerror.Error object any more.
The remaining commits in this PR are necessary to make these thing work together well:

• the few commits towards the beginning, prior to step 1 are ancillary commits that ensure that SQL errors are produced where appropriate.

  (two actually belong to other PRs sql/row: use assertion failures instead of panics #37802 sem/tree: avoid deconstructing the error when parsing a datum #37806 and will disappear when those PRs are merged)

• the commits in-between steps 1 and 2 changes the "interior" code to not rely on the presence of a pgerror.Error object in the causal chain.

  These commits must occur after step 1 because they require the presence of a "pg error code" which is computed by Flatten()'s companion function GetPGCode().

• the commits after step 2 are mostly cosmetic and remove more dependencies on pgerror.Error in other places.

[cockroach-teamcity]

This change is 

[RaduBerinde]

I tried but I got stuck. There is an inversion of control in the error generation from the LALR parsing code. I'll take suggestions!

It's not a big deal if it's not easy. One idea would be to use WithDetailf instead of Wrapf("at or near"). That also makes more sense conceptually, as we are providing detail about the error.

By the way - WithDetailf is a bit awkward - it looks like we have to prepend a space or the regular error message is bad but then the extra space shows up over pgwire (e.g. DETAIL: at or near ",") [edit: there are two spaces before at, github doesn't show it right)

Another thing - when looking through the WithXX functions available, it was hard to understand whether the info I'm adding would be visible if you just print the error (in general, it's hard to know how the info would show up in various contexts, like printing or going over pgwire). It would be good to add to the comments for these.

[andreimatei]

Just timid review beginnings from me here and in #37121.
My excitement is high!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)

---

pkg/sql/pgwire/pgerror/flatten.go, line 139 at r4 (raw file):

			return e.Code, true
		// Special roachpb errors get a special code.
		case roachpb.ClientVisibleRetryError:

this ClientVisibleRetryError interface and the fact that we're assigning a pg code to both errors that implement it gives me pause.
TransactionRestartWithProtoRefresh errors can make their way to the user and they need this error code.
UnhandledRetryableError do not make their way to the client, and so then one would think that they don't need a code. So why exactly are you giving it a code here? I thought it has to be because of DistSQL communication with 19.1 gateways, but that doesn't appear to be the case; I see in r14 that you're not using Flatten() for UnhandledRetryableError.

So I guess my question is - can we recognize TransactionRestartWithProtoRefresh and just use the SerializationFailure code for that?

[maddyblue]

Do we care that this breaks stack printing? That is, the error's stack is not from where it happened, but from where the variable was init'd.

[yuzefovich]

sql/exec commit looks good to me.

[knz]

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

---

pkg/sql/pgwire/pgerror/flatten.go, line 139 at r4 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

this ClientVisibleRetryError interface and the fact that we're assigning a pg code to both errors that implement it gives me pause.
TransactionRestartWithProtoRefresh errors can make their way to the user and they need this error code.
UnhandledRetryableError do not make their way to the client, and so then one would think that they don't need a code. So why exactly are you giving it a code here? I thought it has to be because of DistSQL communication with 19.1 gateways, but that doesn't appear to be the case; I see in r14 that you're not using Flatten() for UnhandledRetryableError.

So I guess my question is - can we recognize TransactionRestartWithProtoRefresh and just use the SerializationFailure code for that?

Good point. I changed the interface declarations in a separate PR: #37998
(An interface is needed here nonetheless because of a dependency cycle)

---

pkg/util/timeutil/pgdate/pgdate.go, line 118 at r15 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Do we care that this breaks stack printing? That is, the error's stack is not from where it happened, but from where the variable was init'd.

Good point. Fixed.

[maddyblue]

LGTM

[knz]

@andreimatei FYI this PR has just 3 remaining commits that need a review:

• sql: use the errors library in the schema changer
• *: remove most references to GetPGCause()
• pgwire, distsqlpb: let native errors flow through

I'll be especially interested in your opinion on the first of these 3. The 3rd we have discussed a month or two ago so there is no surprise. The middle one is trivial/mechanical.

[andreimatei]

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

---

pkg/sql/schema_changer.go, line 195 at r26 (raw file):

	// The Backfill will grab a new timestamp to read at for the rest
	// of the backfill.
	// TODO(knz): this should really use errors.Is(). However until/unless

Have you considered doing gating the use of errors.Is() here on a cluster version check? That'll remind us to remove the old code next release.

---

pkg/sql/schema_changer.go, line 219 at r26 (raw file):

	// TODO(knz): This should really use the errors library with
	// a custom marker.
	if _, ok := errors.UnwrapAll(err).(errTableVersionMismatch); ok {

Why not errors.Is() here?
Please put more words in this comment; as written I don't think it'll mean much to people (it doesn't even mean much to me :P)

---

pkg/sql/schema_changer.go, line 840 at r26 (raw file):

			// As we are dismissing the error, go through the recording motions.
			// This ensures that any important error gets reported to Sentry, etc.
			sqltelemetry.RecordError(ctx, err, &sc.settings.SV)

do we still need the logging above if we're doing RecordError()? Seems to me that RecordError could log if it doesn't already.

---

pkg/sql/schema_changer.go, line 865 at r26 (raw file):

			log.Warningf(ctx, "while reasing schema change lease: %+v", err)
			// Go through the recording motions. See comment above.
			sqltelemetry.RecordError(ctx, err, &sc.settings.SV)

same about the logging above

[andreimatei]

I've looked at those 3 commits.
LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @mjibson)

---

pkg/cli/sql.go, line 1479 at r28 (raw file):

// decomposition in the first return value. If it is not, the function
// assembles a pgerror.Error with suitable Detail and Hint fields.
func (c *cliState) serverSideParse(sql string) (stmts []string, pgErr *pgerror.Error) {

I was gonna ask if this method could stop using pgErr but I see you've done that in a future PR :plause:

---

pkg/sql/distsqlpb/data.go, line 149 at r28 (raw file):

	resErr.FullError = &fullError

	// Now populate compatibility fields for 19.1 nodes.

consider gating this code on a cluster version check for 19.2

---

pkg/sql/distsqlpb/data.go, line 171 at r28 (raw file):

	if e.FullError != nil {
		// If there's a 19.1-forward full error, decode and use that.

you mean 19.2 here I think

---

pkg/sql/distsqlpb/data.proto, line 58 at r28 (raw file):

  // 19.1 nodes and further are expected to use full_error directly where
  // all error decorations are preserved.
  optional errorspb.EncodedError full_error = 3;

writing this field must have been satisfying :)

---

pkg/sql/pgwire/pgerror/errors.proto, line 52 at r28 (raw file):

  //   populated with the first key found in errors.GetTelemetryKeys().
  //
  // After 19.1 is outphased, this field can be removed.

nit: just say "TODO: remove in 19.3" here and elsewhere

[knz]

Somehow github has decided that it did not like this PR any more: I just force-pushed a new version and this auto-closed the PR. Now github prevents me from re-opening the PR because "there are no new commits". Yet the base branch and the new branch are clearly different.

No idea how to fix this, so I opened a new PR: #38127