opt: catch all pgerror.Error in optbuilder

pkg/sql/opt/optbuilder/builder.go

@@ -127,18 +127,21 @@ func New(
 // Builder.factory from the parsed SQL statement in Builder.stmt. See the
 // comment above the Builder type declaration for details.
 //
-// If any subroutines panic with a builderError as part of the build process,
-// the panic is caught here and returned as an error.
+// If any subroutines panic with a builderError or pgerror.Error as part of the
+// build process, the panic is caught here and returned as an error.
 func (b *Builder) Build() (err error) {
 	defer func() {
 		if r := recover(); r != nil {
-			// This code allows us to propagate builder errors without adding
-			// lots of checks for `if err != nil` throughout the code. This is
+			// This code allows us to propagate semantic and internal errors without
+			// adding lots of checks for `if err != nil` throughout the code. This is
 			// only possible because the code does not update shared state and does
 			// not manipulate locks.
-			if bldErr, ok := r.(builderError); ok {
-				err = bldErr.error
-			} else {
+			switch e := r.(type) {
+			case builderError:
+				err = e.error
+			case *pgerror.Error:
+				err = e
+			default:
 				panic(r)
 			}
 		}
@@ -159,29 +162,22 @@ func (b *Builder) Build() (err error) {
 	return nil
 }
 
-// builderError is used for semantic errors that occur during the build process
-// and is passed as an argument to panic. These panics are caught and converted
-// back to errors inside Builder.Build.
+// builderError is used to wrap errors returned by various external APIs that
+// occur during the build process. It exists for us to be able to panic on these
+// errors and then catch them inside Builder.Build even if they are not
+// pgerror.Error.
 type builderError struct {
 	error
 }
 
-// assertionErrorf formats an internal error (or assertion failure)
-// using a pgerror object, so that it can be picked up by telemetry.
-func assertionErrorf(format string, args ...interface{}) builderError {
-	return builderError{pgerror.NewAssertionErrorf(format, args...)}
-}
-
 // unimplementedWithIssueDetailf formats according to a format
 // specifier and returns a Postgres error with the
 // pgerror.CodeFeatureNotSupportedError code, wrapped in a
 // builderError.
 func unimplementedWithIssueDetailf(
 	issue int, detail, format string, args ...interface{},
-) builderError {
-	return builderError{
-		pgerror.UnimplementedWithIssueDetailErrorf(issue, detail, format, args...),
-	}
+) *pgerror.Error {
+	return pgerror.UnimplementedWithIssueDetailErrorf(issue, detail, format, args...)
 }
 
 // buildStmt builds a set of memo groups that represent the given SQL

pkg/sql/opt/optbuilder/delete.go

@@ -34,12 +34,12 @@ import (
 func (b *Builder) buildDelete(del *tree.Delete, inScope *scope) (outScope *scope) {
 	// UX friendliness safeguard.
 	if del.Where == nil && b.evalCtx.SessionData.SafeUpdates {
-		panic(builderError{pgerror.NewDangerousStatementErrorf("DELETE without WHERE clause")})
+		panic(pgerror.NewDangerousStatementErrorf("DELETE without WHERE clause"))
 	}
 
 	if del.OrderBy != nil && del.Limit == nil {
-		panic(builderError{pgerror.NewErrorf(pgerror.CodeSyntaxError,
-			"DELETE statement requires LIMIT when ORDER BY is used")})
+		panic(pgerror.NewErrorf(pgerror.CodeSyntaxError,
+			"DELETE statement requires LIMIT when ORDER BY is used"))
 	}
 
 	if del.With != nil {

pkg/sql/opt/optbuilder/distinct.go

@@ -38,10 +38,10 @@ func (b *Builder) constructDistinct(inScope *scope) memo.RelExpr {
 	// Note: this behavior is consistent with PostgreSQL.
 	for _, col := range inScope.ordering {
 		if !private.GroupingCols.Contains(int(col.ID())) {
-			panic(builderError{pgerror.NewErrorf(
+			panic(pgerror.NewErrorf(
 				pgerror.CodeInvalidColumnReferenceError,
 				"for SELECT DISTINCT, ORDER BY expressions must appear in select list",
-			)})
+			))
 		}
 	}
 
@@ -79,10 +79,10 @@ func (b *Builder) buildDistinctOn(distinctOnCols opt.ColSet, inScope *scope) (ou
 	var seen opt.ColSet
 	for _, col := range inScope.ordering {
 		if !distinctOnCols.Contains(int(col.ID())) {
-			panic(builderError{pgerror.NewErrorf(
+			panic(pgerror.NewErrorf(
 				pgerror.CodeInvalidColumnReferenceError,
 				"SELECT DISTINCT ON expressions must match initial ORDER BY expressions",
-			)})
+			))
 		}
 		seen.Add(int(col.ID()))
 		if seen.Equals(distinctOnCols) {

pkg/sql/opt/optbuilder/explain.go

@@ -15,12 +15,10 @@
 package optbuilder
 
 import (
-	"fmt"
-
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/memo"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
-	"github.com/pkg/errors"
 )
 
 func (b *Builder) buildExplain(explain *tree.Explain, inScope *scope) (outScope *scope) {
@@ -46,16 +44,17 @@ func (b *Builder) buildExplain(explain *tree.Explain, inScope *scope) (outScope
 	case tree.ExplainDistSQL:
 		analyze := opts.Flags.Contains(tree.ExplainFlagAnalyze)
 		if analyze && tree.IsStmtParallelized(explain.Statement) {
-			panic(builderError{
-				errors.New("EXPLAIN ANALYZE does not support RETURNING NOTHING statements")})
+			panic(pgerror.NewErrorf(pgerror.CodeFeatureNotSupportedError,
+				"EXPLAIN ANALYZE does not support RETURNING NOTHING statements"))
 		}
 		cols = sqlbase.ExplainDistSQLColumns
 
 	case tree.ExplainOpt:
 		cols = sqlbase.ExplainOptColumns
 
 	default:
-		panic(fmt.Errorf("unsupported EXPLAIN mode: %d", opts.Mode))
+		panic(pgerror.NewErrorf(pgerror.CodeFeatureNotSupportedError,
+			"EXPLAIN ANALYZE does not support RETURNING NOTHING statements"))
 	}
 	b.synthesizeResultColumns(outScope, cols)
 

pkg/sql/opt/optbuilder/groupby.go

@@ -114,7 +114,7 @@ func (a *aggregateInfo) TypeCheck(ctx *tree.SemaContext, desired types.T) (tree.
 
 // Eval is part of the tree.TypedExpr interface.
 func (a *aggregateInfo) Eval(_ *tree.EvalContext) (tree.Datum, error) {
-	panic(assertionErrorf("aggregateInfo must be replaced before evaluation"))
+	panic(pgerror.NewAssertionErrorf("aggregateInfo must be replaced before evaluation"))
 }
 
 var _ tree.Expr = &aggregateInfo{}
@@ -145,7 +145,7 @@ func (b *Builder) constructGroupBy(
 			if scalar == nil {
 				// A "pass through" column (i.e. a VariableOp) is not legal as an
 				// aggregation.
-				panic(assertionErrorf("variable as aggregation"))
+				panic(pgerror.NewAssertionErrorf("variable as aggregation"))
 			}
 			aggs = append(aggs, memo.AggregationsItem{
 				Agg:        scalar,

pkg/sql/opt/optbuilder/insert.go

@@ -342,7 +342,7 @@ func (mb *mutationBuilder) needExistingRows() bool {
 // list of table columns that are the target of the Insert operation.
 func (mb *mutationBuilder) addTargetNamedColsForInsert(names tree.NameList) {
 	if len(mb.targetColList) != 0 {
-		panic(assertionErrorf("addTargetNamedColsForInsert cannot be called more than once"))
+		panic(pgerror.NewAssertionErrorf("addTargetNamedColsForInsert cannot be called more than once"))
 	}
 
 	// Add target table columns by the names specified in the Insert statement.
@@ -376,8 +376,8 @@ func (mb *mutationBuilder) checkPrimaryKeyForInsert() {
 			continue
 		}
 
-		panic(builderError{pgerror.NewErrorf(pgerror.CodeInvalidForeignKeyError,
-			"missing %q primary key column", col.ColName())})
+		panic(pgerror.NewErrorf(pgerror.CodeInvalidForeignKeyError,
+			"missing %q primary key column", col.ColName()))
 	}
 }
 
@@ -440,12 +440,12 @@ func (mb *mutationBuilder) checkForeignKeysForInsert() {
 		case 0:
 			// Do nothing.
 		case 1:
-			panic(builderError{pgerror.NewErrorf(pgerror.CodeForeignKeyViolationError,
-				"missing value for column %q in multi-part foreign key", missingCols[0])})
+			panic(pgerror.NewErrorf(pgerror.CodeForeignKeyViolationError,
+				"missing value for column %q in multi-part foreign key", missingCols[0]))
 		default:
 			sort.Strings(missingCols)
-			panic(builderError{pgerror.NewErrorf(pgerror.CodeForeignKeyViolationError,
-				"missing values for columns %q in multi-part foreign key", missingCols)})
+			panic(pgerror.NewErrorf(pgerror.CodeForeignKeyViolationError,
+				"missing values for columns %q in multi-part foreign key", missingCols))
 		}
 	}
 }
@@ -462,7 +462,7 @@ func (mb *mutationBuilder) checkForeignKeysForInsert() {
 // columns.
 func (mb *mutationBuilder) addTargetTableColsForInsert(maxCols int) {
 	if len(mb.targetColList) != 0 {
-		panic(assertionErrorf("addTargetTableColsForInsert cannot be called more than once"))
+		panic(pgerror.NewAssertionErrorf("addTargetTableColsForInsert cannot be called more than once"))
 	}
 
 	// Only consider non-mutation columns, since mutation columns are hidden from
@@ -993,8 +993,8 @@ func (mb *mutationBuilder) ensureUniqueConflictCols(cols tree.NameList) cat.Inde
 			return index
 		}
 	}
-	panic(builderError{pgerror.NewErrorf(pgerror.CodeInvalidColumnReferenceError,
-		"there is no unique or exclusion constraint matching the ON CONFLICT specification")})
+	panic(pgerror.NewErrorf(pgerror.CodeInvalidColumnReferenceError,
+		"there is no unique or exclusion constraint matching the ON CONFLICT specification"))
 }
 
 // getPrimaryKeyColumnNames returns the names of all primary key columns in the

pkg/sql/opt/optbuilder/join.go

@@ -50,19 +50,19 @@ func (b *Builder) buildJoin(join *tree.JoinTableExpr, inScope *scope) (outScope
 		flags.DisallowHashJoin = true
 		flags.DisallowMergeJoin = true
 		if joinType != sqlbase.InnerJoin && joinType != sqlbase.LeftOuterJoin {
-			panic(builderError{pgerror.NewErrorf(pgerror.CodeSyntaxError,
+			panic(pgerror.NewErrorf(pgerror.CodeSyntaxError,
 				"%s can only be used with INNER or LEFT joins", tree.AstLookup,
-			)})
+			))
 		}
 
 	case tree.AstMerge:
 		flags.DisallowLookupJoin = true
 		flags.DisallowHashJoin = true
 
 	default:
-		panic(builderError{pgerror.NewErrorf(
+		panic(pgerror.NewErrorf(
 			pgerror.CodeFeatureNotSupportedError, "join hint %s not supported", join.Hint,
-		)})
+		))
 	}
 
 	switch cond := join.Cond.(type) {
@@ -135,11 +135,11 @@ func (b *Builder) validateJoinTableNames(leftScope, rightScope *scope) {
 				continue
 			}
 
-			panic(builderError{pgerror.NewErrorf(
+			panic(pgerror.NewErrorf(
 				pgerror.CodeDuplicateAliasError,
 				"source name %q specified more than once (missing AS clause)",
 				tree.ErrString(&leftName.TableName),
-			)})
+			))
 		}
 	}
 }
@@ -182,7 +182,8 @@ func (b *Builder) constructJoin(
 	case sqlbase.FullOuterJoin:
 		return b.factory.ConstructFullJoin(left, right, on, private)
 	default:
-		panic(fmt.Errorf("unsupported JOIN type %d", joinType))
+		panic(pgerror.NewErrorf(pgerror.CodeFeatureNotSupportedError,
+			"unsupported JOIN type %d", joinType))
 	}
 }
 
@@ -295,8 +296,8 @@ func (jb *usingJoinBuilder) buildUsingJoin(using *tree.UsingJoinCond) {
 		}
 		if seenCols.Contains(int(leftCol.id)) {
 			// Same name exists more than once in USING column name list.
-			panic(builderError{pgerror.NewErrorf(pgerror.CodeDuplicateColumnError,
-				"column %q appears more than once in USING clause", tree.ErrString(&name))})
+			panic(pgerror.NewErrorf(pgerror.CodeDuplicateColumnError,
+				"column %q appears more than once in USING clause", tree.ErrString(&name)))
 		}
 		seenCols.Add(int(leftCol.id))
 
@@ -414,9 +415,9 @@ func (jb *usingJoinBuilder) addEqualityCondition(leftCol, rightCol *scopeColumn)
 	// First, check if the comparison would even be valid.
 	if !leftCol.typ.Equivalent(rightCol.typ) {
 		if _, found := tree.FindEqualComparisonFunction(leftCol.typ, rightCol.typ); !found {
-			panic(builderError{pgerror.NewErrorf(pgerror.CodeDatatypeMismatchError,
+			panic(pgerror.NewErrorf(pgerror.CodeDatatypeMismatchError,
 				"JOIN/USING types %s for left and %s for right cannot be matched for column %q",
-				leftCol.typ, rightCol.typ, tree.ErrString(&leftCol.name))})
+				leftCol.typ, rightCol.typ, tree.ErrString(&leftCol.name)))
 		}
 	}
 
@@ -458,11 +459,11 @@ func (jb *usingJoinBuilder) addEqualityCondition(leftCol, rightCol *scopeColumn)
 }
 
 func (jb *usingJoinBuilder) raiseDuplicateColError(name tree.Name) {
-	panic(builderError{pgerror.NewErrorf(pgerror.CodeDuplicateColumnError,
-		"duplicate column name: %q", tree.ErrString(&name))})
+	panic(pgerror.NewErrorf(pgerror.CodeDuplicateColumnError,
+		"duplicate column name: %q", tree.ErrString(&name)))
 }
 
 func (jb *usingJoinBuilder) raiseUndefinedColError(name tree.Name, context string) {
-	panic(builderError{pgerror.NewErrorf(pgerror.CodeUndefinedColumnError,
-		"column \"%s\" specified in USING clause does not exist in %s table", name, context)})
+	panic(pgerror.NewErrorf(pgerror.CodeUndefinedColumnError,
+		"column \"%s\" specified in USING clause does not exist in %s table", name, context))
 }

pkg/sql/opt/optbuilder/mutation_builder.go

@@ -215,8 +215,8 @@ func (mb *mutationBuilder) addTargetCol(ord int) {
 	// Ensure that the name list does not contain duplicates.
 	colID := mb.tabID.ColumnID(ord)
 	if mb.targetColSet.Contains(int(colID)) {
-		panic(builderError{pgerror.NewErrorf(pgerror.CodeSyntaxError,
-			"multiple assignments to the same column %q", tabCol.ColName())})
+		panic(pgerror.NewErrorf(pgerror.CodeSyntaxError,
+			"multiple assignments to the same column %q", tabCol.ColName()))
 	}
 	mb.targetColSet.Add(int(colID))
 
@@ -547,9 +547,9 @@ func (mb *mutationBuilder) checkNumCols(expected, actual int) {
 		} else {
 			kw = "UPSERT"
 		}
-		panic(builderError{pgerror.NewErrorf(pgerror.CodeSyntaxError,
+		panic(pgerror.NewErrorf(pgerror.CodeSyntaxError,
 			"%s has more %s than %s, %d expressions for %d targets",
-			kw, more, less, actual, expected)})
+			kw, more, less, actual, expected))
 	}
 }
 
@@ -597,7 +597,7 @@ func findNotNullIndexCol(index cat.Index) int {
 			return indexCol.Ordinal
 		}
 	}
-	panic(assertionErrorf("should have found not null column in index"))
+	panic(pgerror.NewAssertionErrorf("should have found not null column in index"))
 }
 
 // resultsNeeded determines whether a statement that might have a RETURNING
@@ -609,7 +609,7 @@ func resultsNeeded(r tree.ReturningClause) bool {
 	case *tree.ReturningNothing, *tree.NoReturningClause:
 		return false
 	default:
-		panic(assertionErrorf("unexpected ReturningClause type: %T", t))
+		panic(pgerror.NewAssertionErrorf("unexpected ReturningClause type: %T", t))
 	}
 }
 
@@ -632,9 +632,9 @@ func getAliasedTableName(n tree.TableExpr) (*tree.TableName, *tree.TableName) {
 	}
 	tn, ok := n.(*tree.TableName)
 	if !ok {
-		panic(builderError{pgerror.Unimplemented(
+		panic(pgerror.Unimplemented(
 			"complex table expression in UPDATE/DELETE",
-			"cannot use a complex table name with DELETE/UPDATE")})
+			"cannot use a complex table name with DELETE/UPDATE"))
 	}
 	return tn, alias
 }
@@ -652,7 +652,7 @@ func checkDatumTypeFitsColumnType(col cat.Column, typ types.T) {
 	}
 
 	colName := string(col.ColName())
-	panic(builderError{pgerror.NewErrorf(pgerror.CodeDatatypeMismatchError,
+	panic(pgerror.NewErrorf(pgerror.CodeDatatypeMismatchError,
 		"value type %s doesn't match type %s of column %q",
-		typ, col.ColTypeStr(), tree.ErrNameString(colName))})
+		typ, col.ColTypeStr(), tree.ErrNameString(colName)))
 }

pkg/sql/opt/optbuilder/project.go

@@ -123,8 +123,8 @@ func (b *Builder) analyzeSelectList(
 				switch v.(type) {
 				case tree.UnqualifiedStar, *tree.AllColumnsSelector, *tree.TupleStar:
 					if e.As != "" {
-						panic(builderError{pgerror.NewErrorf(pgerror.CodeSyntaxError,
-							"%q cannot be aliased", tree.ErrString(v))})
+						panic(pgerror.NewErrorf(pgerror.CodeSyntaxError,
+							"%q cannot be aliased", tree.ErrString(v)))
 					}
 
 					aliases, exprs := b.expandStar(e.Expr, inScope)