sql: don't test error identities using reference equality

[knz]

Fixes #35854.

Prior to this patch, the SQL code was expecting to identify
errors by addess, ie. something like:

// at global scope
var errSomething = errors.New(...)

// somewhere in the code
   if err == errSomething { ... }

This approach was flawed in that errors can be wrapped (errors.Wrap,
errors.WithMessage) or flattened (pgerror.Wrap) or sent over the
wire.

To make the code more robust, this patch introduces a new API
pgerror.WithMarker() which instantiates errors with a unique
marker. The marker is guaranteed to be preserved through errors.Wrap
and pgerror.Wrap calls. It can be tested with
pgerror.IsMarkedError().

The patch then updates the various points in the SQL code where errors
were tested in that way, to use the new API.

(In some places some attempt is made to "work around" with a string
comparison on the message instead, but that's arguably even worse for
reasons left as an exercise to the reader. This should also be fixed,
but that fix is out of scope here.)

Release note: None

[cockroach-teamcity]

This change is 

[knz]

@lucy-zhang I am sending this out as a generally beneficial fix.

I have not yet confirmed the origin of the schema change flake we've been discussing earlier, and thus I don't know whether this PR fixes it. Tomorrow I hope to conduct tests against my hypothesis, and determine whether this PR addresses the problem.

However regardless I think the way the errors were compared was problematic and the present PR generally improves things.

[bobvawter]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bobvawter, @knz, @lucy-zhang, and @vivekmenezes)

---

pkg/sql/pgwire/pgerror/markers.go, line 64 at r1 (raw file):

// - instead of including the line number or function name, so that
//   the marker persists across minor code refactorings; this ensures
//   comparability across mixed version clusters.

I don't think this kind of identity is safe. Renaming a .go file or moving a symbol between files in a package is basically a no-op as far as refactorings or cleanups go. Baking the filename into the marker would create landmines for subsequent developers.

What if there were a function to create markers that required the developer to include a globally-unique string? This could be enforced by having a static "registry" map to ensure that no duplicate values are present. A Marker is then mixed with an error to produce a MarkedError. It's weird to me that the marker objects are themselves errors.

var ContextCanceledMarker Marker = MakeMarker(context.Canceled, "pgerror.ContextCanceled")

[knz]

That was my first implementation. The problem with that, I found, is that people are really bad at inventing names. Then where would you store this global registry? And prevent changes that rename the error by changing the string? -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[bobvawter]

Then where would you store this global registry? And prevent changes that rename the error by changing the string?

The markers could go in a new, leaf package pgerror/marker.

package somepkg

var MyMarker = marker.New("somepkg.MyMarker")

If I saw the code above, I'd be pretty justified in assuming that the constant string value is meaningful, but I would never consider the enclosing file or package to ever be meaningful.

As to the naming issue, we're in the same boat as telemetry. We only need names to be distinct within the project and governed by our code-review process. If the marker API clearly indicates that the string values ought not to be changed, I think we would have fewer problems in practice than by incorporating too much magic into the markers.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @lucy-zhang, and @vivekmenezes)

[andreimatei]

I haven't looked at the code, but I wonder how pgerror.IsMarkedError() is different from errors.Cause(err) ? Don't they serve the same purpose - to unwrap?

[vivekmenezes]

another test failure caused by poor unwrapping of errors:

#35734 (comment)

[knz]

I have discussed this further with @andreimatei yesterday.

• Bob's remark that we should use unique constant strings is good. I'll implement that.
• however doing this as an additional field inside the pgerror.Error object is misguided. This is because it's desirable to be able to add multiple markers around an error. What we can do is mimic the errors.Wrap machinery inside the pgerror package for that purpose, ensuring that the wrapper objects are protobuf-encodable.

I will prioritize this work today because it's actually solving a real problem - #35734.

[knz]

Following wise words from Vivek I have decided to not pursue this approach further and instead revert some of my more agressive changes to pgerror.Error throughout the code.
This means that I am putting a band-aid into 19.1 to ensure that #35854 is not going to be too serious. The band-aid is in #36023.

This ensures that the original error objects are left intact through pgerror.Wrap and can still be compared with equality.

The present PR can be left to sleep for the time being.

[knz]

I'll be wanting to rebase this on top of #37765.

[knz]

Subsumed by #37765.