opt: catch all pgerror.Error in optbuilder

We now catch all `pgerror.Error`s in optbuilder, which means that we
don't need to use the `buildError` wrapper with them. The wrapper
still exists when external calls (e.g. for name resolution) return a
generic error.

The main motivation is that optbuilder calls into the factory which
can panic internally. We will want to switch those panics to assertion
errors as well, but they are in different packages. The existing
approach would have required a shared, exported wrapper.

Release note: None

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/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),
-			)})
+			))
 		}
 	}
 }
@@ -295,8 +295,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 +414,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 +458,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)

pkg/sql/opt/optbuilder/scalar.go

@@ -278,8 +278,8 @@ func (b *Builder) buildScalar(
 
 	case *tree.IndexedVar:
 		if t.Idx < 0 || t.Idx >= len(inScope.cols) {
-			panic(builderError{pgerror.NewErrorf(pgerror.CodeUndefinedColumnError,
-				"invalid column ordinal: @%d", t.Idx+1)})
+			panic(pgerror.NewErrorf(pgerror.CodeUndefinedColumnError,
+				"invalid column ordinal: @%d", t.Idx+1))
 		}
 		out = b.factory.ConstructVariable(inScope.cols[t.Idx].id)
 
@@ -441,7 +441,7 @@ func (b *Builder) buildFunction(
 	}
 
 	if isAggregate(def) {
-		panic(assertionErrorf("aggregate function should have been replaced"))
+		panic(pgerror.NewAssertionErrorf("aggregate function should have been replaced"))
 	}
 
 	args := make(memo.ScalarListExpr, len(f.Exprs))
@@ -560,10 +560,10 @@ func (b *Builder) checkSubqueryOuterCols(
 			subqueryOuterCols.DifferenceWith(inScope.groupby.aggOutScope.colSet())
 			colID, _ := subqueryOuterCols.Next(0)
 			col := inScope.getColumn(opt.ColumnID(colID))
-			panic(builderError{pgerror.NewErrorf(
+			panic(pgerror.NewErrorf(
 				pgerror.CodeGroupingError,
 				"subquery uses ungrouped column \"%s\" from outer query",
-				tree.ErrString(&col.name))})
+				tree.ErrString(&col.name)))
 		}
 	}
 }