Improving Materialize's errors

[benesch]

Overview

I'd like us to adopt PostgreSQL's structured error strategy. The basic idea is summarized well by this error message from a recent version of PostgreSQL:

benesch=> drop view v;
ERROR:  cannot drop view v because other objects depend on it
DETAIL:  view v2 depends on view v
view v3 depends on view v
HINT:  Use DROP ... CASCADE to drop the dependent objects too.

The key bit is the separation of the primary, detail, and hint fields. This allows for very consistent grammar and style for both simple and complicated errors.

Many of our more complicated errors jam a bunch of information into one line, often from dependencies with less than perfect error messages, and it ends up pretty awkward:

ERROR: could not frobulate your widgets: there was a deeper cause for this which we explain here: Hyper is now using its error message style here: which includes seven tons of
useless information and also an annoying newline. Hint: Did you try using gadgets instead? You should visit https://materialize.com/docs/ for more details.

When you separate out the fields, the rendering becomes much more consistent. The primary field can always be a pithy, active voice sentence fragment, because you can jam as much information as you want into the detail field. And the hint field is always easily visible, because its the last thing printed.

The full PostgreSQL error message style guide is here: https://www.postgresql.org/docs/current/error-style-guide.html

Design

I believe the right course of action is to purge use of anyhow::Error in any code that's part of the core database engine. anyhow::Error is still quite useful in utility code and tests, where Materialize developers are the consumer, rather than our users.

I propose that we instead use structured error types everywhere. This increases the burden of introducing a new error, but it will vastly improve the quality of the generated error messages. Roughly:

enum CoordError {
    FailedToFrobulate { why: String },
    NoFrobulator,
    SqlError(SqlError),
}

impl CoordError {
    fn detail(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match self {
            CoordError::FailedToFrobulate => write!(f, "{}", why),
            CoordError::NoFrobulator => Ok(()),
            CoordError::SqlError(e) => e.detail(f),
        }
    }

    fn hint(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match self {
            CoordError::FailedToFrobulate => (),
            CoordError::NoFrobulator => write!(f, "Did you install a frobulator?"),
            CoordError::SqlError(e) => e.hint(f),
        }
    }
}

impl fmt::Display for CoordError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match self {
            CoordError::FailedToFrobulate { why: _ } => write!(f, "could not frobulate"),
            CoordError::NoFrobulator => (),
            CoordError::SqlError(e) => e.fmt(f),
        }
    }
}

We already do a limited version of this with the EvalError type in the expr
crate, though it lacks the detail and hint methods.

pgwire

The one missing piece here is
"SQLSTATE", which
is a code specified by the SQL standard that identifies various classes of
errors. We can insulate most of the codebase from this nonsense by adding a
function like this to the pgwire crate:

fn sqlstate(e: CoordError) -> SqlState {
    match e {
        CoordError::FailedToFrobulate { .. } => SqlState::FROBULATOR_ERROR,
        CoordError::NoFrobulator { .. } => SqlState::FROBULATOR_ERROR,
        CoordError::SqlError(e) => match e {
            SqlError::NoSuchTable => SqlState::UNDEFINED_TABLE,
            // ...
        }
    }
}

CLI

Introducing this error is also a good time to make our CLI errors properly
colorized.

Alternatives

One alternative is to introduce a PgError type with the right set of fields,
and simply use this everywhere we currently use anyhow::Error:

struct PgError {
    code: SqlState,
    primary: String,
    detail: Option<String>,
    hint: Option<String>,
}

We'd probably want to introduce a macro to make constructing this type less
painful. This is essentially what PostgreSQL does internally, for what it's
worth.

The big downside of this solution is that it requires allocating at the point of
error construction. In Postgres, this doesn't much matter: as soon as a query
encounters an error, the entire query will be aborted. But in Materialize,
dataflows may accumulate many, many errors, and those errors all have to be
stored somewhere. Using an enum as the error type is therefore much cheaper,
memory-wise, as it's just a small integer that needs to be stored.

A smaller downside of this solution is that the whole codebase would be infected
with caring about a "PostgreSQL error" and its "SQLSTATE". The proposed solution
above would only require errors to care about "detail" and "hint" fields, which
are much more general puprose.

[maddyblue]

Your CoordError example has a variant with a {why: String}. Doesn't that allocate anyway? Many error messages will have some sort of text that is dependant on whatever sql is being executed, and so won't actually be just an int in memory. Am I wrong about that? If not and we will in practice be allocating a lot either way, I might lean toward the PgError alternative.

[benesch]

Oh, yeah, I didn't explain that fully. Here's the current EvalError enum to give us something to talk about concretely:

materialize/src/expr/src/scalar/mod.rs

Lines 652 to 683 in 812c652

	#[derive(Ord, PartialOrd, Clone, Debug, Eq, PartialEq, Serialize, Deserialize, Hash)]
	pub enum EvalError {
	DivisionByZero,
	NumericFieldOverflow,
	FloatOutOfRange,
	IntegerOutOfRange,
	IntervalOutOfRange,
	TimestampOutOfRange,
	InvalidTimezone(String),
	InvalidTimezoneInterval,
	InvalidTimezoneConversion,
	InvalidDimension {
	max_dim: usize,
	val: i64,
	},
	InvalidArray(InvalidArrayError),
	InvalidEncodingName(String),
	InvalidHashAlgorithm(String),
	InvalidByteSequence {
	byte_sequence: String,
	encoding_name: String,
	},
	InvalidRegex(String),
	InvalidRegexFlag(char),
	InvalidParameterValue(String),
	NegSqrt,
	UnknownUnits(String),
	UnsupportedDateTimeUnits(DateTimeUnits),
	UnterminatedLikeEscapeSequence,
	Parse(ParseError),
	Internal(String),
	}

EvalError::InvalidByteSequence demonstrates things well. byte_sequence: String is user input, so will probably always require allocation, though if we get lucky maybe we can move a string out of the data plane and into the error message. But encoding_name: String is an enum masquerading as a string, and it'd be very straightforward to elide that allocation by using a proper error type.

So the enums give you a good deal more flexibility. With the PgError struct you're just kinda stuck allocating three strings if you want all three fields; with an enum you have to allocate probably at most one string at the point of construction, and the rest can be deferred to error render time.

The other benefit of typed errors is that it'll allow the SQL/coord interface (and even a few places internal to the SQL planner, actually) to properly catch/handle errors. There are a few moments where it'd be nice to just look at an error message higher up in the stack and go "ah, yeah, I can deal with this" but using string errors makes that very nearly impossible.

[krishmanoh2]

Can we have error codes as well?

[maddyblue]

This is the SQLSTATE thing discussed at the end. Any solution above here will have these error codes.

[petrosagg]

Another thing I noticed regarding error messages is that when a construct gets desugared into some other form (in transform_ast.rs) and an error happens downstream the error that is surfaced to the user is in terms of the rewritten query. This can be confusing if the error references constructs that didn't appear in the original text. Here is an example from my recent foo = ANY (array) desugarring:

petrosagg=> SELECT 3 = ANY('test');
ERROR:  Cannot call function mz_catalog.unnest(unknown): arguments cannot be implicitly cast to any implementation's parameters; try providing explicit casts

Note: In this specific example the desugarer has enough information to bail out but imagine 'test' was some complicated expression that its type can't be determined at that stage.

I'm not sure how deep a change this is, but ideally we'd have to remember the original SQL before de-sugaring so that we correctly display errors. I believe rustc handles the same problem by keeping around Spans of the original code as the compiler progresses through its transformations