Merge #36975

36975: opt: remove top-level statement check, clean up pgerror APIs r=RaduBerinde a=RaduBerinde

@knz I did some minor pgerror changes in the first commits. They are in this PR because without them, the last commit would cause error mismatches in some tests.

#### pgerror: clean up API

The various public-facing functions in pgerror have a number of
inconsistencies:
 - some functions start with `New`, others don't
 - the `f` suffix isn't used consistently (like `Unimplemented`, which
   caused unnecessary `Sprintf`s in some callers)
 - some functions contain `Error`, others don't.

This commit cleans this up:
 - we remove `Error` because it is redundant with the package name
 - we remove `New` for all wrappers
 - we make all wrappers have `f` and non-`f` versions.

Release note: None

#### pgerror: add `unimplemented:` consistently

Some of the `Unimplemented*` functions prepend `unimplemented: ` and
some don't. Making them all prepend it (leaving most usecases
unchanged)

Release note: None

#### opt: remove top-level statement type check

The planning code has a "fast path" to avoid using the optimizer if
the top-level statement is not supported. This is no longer necessary
because the optimizer implements all important statement types. This
was shortcutting the recent work to use the optimizer for more
statements.

Release note: None

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>

pkg/ccl/backupccl/backup.go

@@ -774,13 +774,13 @@ func VerifyUsableExportTarget(
 		// TODO(dt): If we audit exactly what not-exists error each ExportStorage
 		// returns (and then wrap/tag them), we could narrow this check.
 		r.Close()
-		return pgerror.NewErrorf(pgerror.CodeDuplicateFileError,
+		return pgerror.Newf(pgerror.CodeDuplicateFileError,
 			"%s already contains a %s file",
 			readable, BackupDescriptorName)
 	}
 	if r, err := exportStore.ReadFile(ctx, BackupDescriptorCheckpointName); err == nil {
 		r.Close()
-		return pgerror.NewErrorf(pgerror.CodeDuplicateFileError,
+		return pgerror.Newf(pgerror.CodeDuplicateFileError,
 			"%s already contains a %s file (is another operation already in progress?)",
 			readable, BackupDescriptorCheckpointName)
 	}
@@ -918,7 +918,7 @@ func backupPlanHook(
 				// context of their own cluster, so we need to ensure we only allow
 				// incremental previous backups that we created.
 				if !desc.ClusterID.Equal(clusterID) {
-					return pgerror.NewErrorf(pgerror.CodeDataExceptionError,
+					return pgerror.Newf(pgerror.CodeDataExceptionError,
 						"previous BACKUP %q belongs to cluster %s", uri, desc.ClusterID.String())
 				}
 				prevBackups[i] = desc
@@ -1107,7 +1107,7 @@ func (b *backupResumer) Resume(
 	p := phs.(sql.PlanHookState)
 
 	if len(details.BackupDescriptor) == 0 {
-		return pgerror.NewErrorf(pgerror.CodeDataExceptionError,
+		return pgerror.Newf(pgerror.CodeDataExceptionError,
 			"missing backup descriptor; cannot resume a backup from an older version")
 	}
 

pkg/ccl/backupccl/restore.go

@@ -74,7 +74,7 @@ func loadBackupDescs(
 		backupDescs[i] = desc
 	}
 	if len(backupDescs) == 0 {
-		return nil, pgerror.NewErrorf(pgerror.CodeDataExceptionError, "no backups found")
+		return nil, pgerror.Newf(pgerror.CodeDataExceptionError, "no backups found")
 	}
 	return backupDescs, nil
 }
@@ -931,7 +931,7 @@ func WriteTableDescs(
 		}
 		if err := txn.Run(ctx, b); err != nil {
 			if _, ok := errors.Cause(err).(*roachpb.ConditionFailedError); ok {
-				return pgerror.NewErrorf(pgerror.CodeDuplicateObjectError, "table already exists")
+				return pgerror.Newf(pgerror.CodeDuplicateObjectError, "table already exists")
 			}
 			return err
 		}
@@ -996,7 +996,7 @@ func rewriteBackupSpanKey(kr *storageccl.KeyRewriter, key roachpb.Key) (roachpb.
 	}
 	if !rewritten && bytes.Equal(newKey, key) {
 		// if nothing was changed, we didn't match the top-level key at all.
-		return nil, pgerror.NewAssertionErrorf(
+		return nil, pgerror.AssertionFailedf(
 			"no rewrite for span start key: %s", key)
 	}
 	// Modify all spans that begin at the primary index to instead begin at the
@@ -1214,7 +1214,7 @@ func restore(
 				// Assert that we're actually marking the correct span done. See #23977.
 				if !importSpans[idx].Key.Equal(importRequest.DataSpan.Key) {
 					mu.Unlock()
-					return pgerror.NewErrorf(pgerror.CodeDataExceptionError,
+					return pgerror.Newf(pgerror.CodeDataExceptionError,
 						"request %d for span %v (to %v) does not match import span for same idx: %v",
 						idx, importRequest.DataSpan, newSpanKey, importSpans[idx],
 					)

pkg/ccl/changefeedccl/changefeed_processors.go

@@ -527,7 +527,7 @@ func (cf *changeFrontier) noteResolvedSpan(d sqlbase.EncDatum) error {
 	}
 	raw, ok := d.Datum.(*tree.DBytes)
 	if !ok {
-		return pgerror.NewAssertionErrorf(`unexpected datum type %T: %s`, d.Datum, d.Datum)
+		return pgerror.AssertionFailedf(`unexpected datum type %T: %s`, d.Datum, d.Datum)
 	}
 	var resolved jobspb.ResolvedSpan
 	if err := protoutil.Unmarshal([]byte(*raw), &resolved); err != nil {

pkg/ccl/changefeedccl/poller.go

@@ -658,7 +658,7 @@ func (p *poller) validateTable(ctx context.Context, desc *sqlbase.TableDescripto
 			// interesting here.
 			if p.details.StatementTime.Less(boundaryTime) {
 				if boundaryTime.Less(p.mu.highWater) {
-					return pgerror.NewAssertionErrorf(
+					return pgerror.AssertionFailedf(
 						"error: detected table ID %d backfill completed at %s "+
 							"earlier than highwater timestamp %s",
 						log.Safe(desc.ID),

pkg/ccl/importccl/exportcsv.go

@@ -118,7 +118,7 @@ func exportPlanHook(
 		if override, ok := opts[exportOptionDelimiter]; ok {
 			csvOpts.Comma, err = util.GetSingleRune(override)
 			if err != nil {
-				return pgerror.NewError(pgerror.CodeInvalidParameterValueError, "invalid delimiter")
+				return pgerror.New(pgerror.CodeInvalidParameterValueError, "invalid delimiter")
 			}
 		}
 
@@ -130,10 +130,10 @@ func exportPlanHook(
 		if override, ok := opts[exportOptionChunkSize]; ok {
 			chunk, err = strconv.Atoi(override)
 			if err != nil {
-				return pgerror.NewError(pgerror.CodeInvalidParameterValueError, err.Error())
+				return pgerror.New(pgerror.CodeInvalidParameterValueError, err.Error())
 			}
 			if chunk < 1 {
-				return pgerror.NewError(pgerror.CodeInvalidParameterValueError, "invalid csv chunk size")
+				return pgerror.New(pgerror.CodeInvalidParameterValueError, "invalid csv chunk size")
 			}
 		}
 

pkg/ccl/importccl/import_stmt.go

@@ -179,7 +179,7 @@ func MakeSimpleTableDescriptor(
 			// ignore
 		case *tree.ColumnTableDef:
 			if def.Computed.Expr != nil {
-				return nil, pgerror.Unimplemented("import.computed", "computed columns not supported: %s", tree.AsString(def))
+				return nil, pgerror.Unimplementedf("import.computed", "computed columns not supported: %s", tree.AsString(def))
 			}
 
 			if err := sql.SimplifySerialInColumnDefWithRowID(ctx, def, &create.Table); err != nil {
@@ -197,7 +197,7 @@ func MakeSimpleTableDescriptor(
 			def.Table = tree.MakeUnqualifiedTableName(def.Table.TableName)
 
 		default:
-			return nil, pgerror.Unimplemented(fmt.Sprintf("import.%T", def), "unsupported table definition: %s", tree.AsString(def))
+			return nil, pgerror.Unimplementedf(fmt.Sprintf("import.%T", def), "unsupported table definition: %s", tree.AsString(def))
 		}
 		// only append this def after we make it past the error checks and continues
 		filteredDefs = append(filteredDefs, create.Defs[i])
@@ -505,7 +505,7 @@ func importPlanHook(
 			if !found {
 				// Check if database exists right now. It might not after the import is done,
 				// but it's better to fail fast than wait until restore.
-				return pgerror.NewErrorf(pgerror.CodeUndefinedObjectError,
+				return pgerror.Newf(pgerror.CodeUndefinedObjectError,
 					"database does not exist: %q", table)
 			}
 			parentID = descI.(*sqlbase.DatabaseDescriptor).ID
@@ -551,12 +551,12 @@ func importPlanHook(
 					return pgerror.Wrapf(err, pgerror.CodeSyntaxError, "invalid %s value", csvSkip)
 				}
 				if skip < 0 {
-					return pgerror.NewErrorf(pgerror.CodeSyntaxError, "%s must be >= 0", csvSkip)
+					return pgerror.Newf(pgerror.CodeSyntaxError, "%s must be >= 0", csvSkip)
 				}
 				// We need to handle the case where the user wants to skip records and the node
 				// interpreting the statement might be newer than other nodes in the cluster.
 				if !p.ExecCfg().Settings.Version.IsActive(cluster.VersionImportSkipRecords) {
-					return pgerror.NewErrorf(pgerror.CodeInsufficientPrivilegeError,
+					return pgerror.Newf(pgerror.CodeInsufficientPrivilegeError,
 						"Using non-CSV import format requires all nodes to be upgraded to %s",
 						cluster.VersionByKey(cluster.VersionImportSkipRecords))
 				}
@@ -651,7 +651,7 @@ func importPlanHook(
 			}
 			format.PgDump.MaxRowSize = maxRowSize
 		default:
-			return pgerror.Unimplemented("import.format", "unsupported import format: %q", importStmt.FileFormat)
+			return pgerror.Unimplementedf("import.format", "unsupported import format: %q", importStmt.FileFormat)
 		}
 
 		if format.Format != roachpb.IOFileFormat_CSV {
@@ -696,7 +696,7 @@ func importPlanHook(
 				}
 			}
 			if !found {
-				return pgerror.Unimplemented("import.compression", "unsupported compression value: %q", override)
+				return pgerror.Unimplementedf("import.compression", "unsupported compression value: %q", override)
 			}
 		}
 

pkg/ccl/importccl/read_import_mysql.go

@@ -625,18 +625,18 @@ func mysqlColToCockroach(
 		def.Type = types.Jsonb
 
 	case mysqltypes.Set:
-		return nil, pgerror.UnimplementedWithIssueHintError(32560,
+		return nil, pgerror.UnimplementedWithIssueHint(32560,
 			"cannot import SET columns at this time",
 			"try converting the column to a 64-bit integer before import")
 	case mysqltypes.Geometry:
-		return nil, pgerror.UnimplementedWithIssueErrorf(32559,
+		return nil, pgerror.UnimplementedWithIssuef(32559,
 			"cannot import GEOMETRY columns at this time")
 	case mysqltypes.Bit:
-		return nil, pgerror.UnimplementedWithIssueHintError(32561,
+		return nil, pgerror.UnimplementedWithIssueHint(32561,
 			"cannot improt BIT columns at this time",
 			"try converting the column to a 64-bit integer before import")
 	default:
-		return nil, pgerror.Unimplemented(fmt.Sprintf("import.mysqlcoltype.%s", typ), "unsupported mysql type %q", col.Type)
+		return nil, pgerror.Unimplementedf(fmt.Sprintf("import.mysqlcoltype.%s", typ), "unsupported mysql type %q", col.Type)
 	}
 
 	if col.NotNull {
@@ -652,7 +652,7 @@ func mysqlColToCockroach(
 		} else {
 			expr, err := parser.ParseExpr(exprString)
 			if err != nil {
-				return nil, pgerror.Unimplemented("import.mysql.default", "unsupported default expression %q for column %q: %v", exprString, name, err)
+				return nil, pgerror.Unimplementedf("import.mysql.default", "unsupported default expression %q for column %q: %v", exprString, name, err)
 			}
 			def.DefaultExpr.Expr = expr
 		}

pkg/ccl/importccl/read_import_pgdump.go

@@ -11,7 +11,6 @@ package importccl
 import (
 	"bufio"
 	"context"
-	"fmt"
 	"io"
 	"regexp"
 	"strings"
@@ -374,9 +373,9 @@ func readPostgresCreateTable(
 
 func getTableName(tn *tree.TableName) (string, error) {
 	if sc := tn.Schema(); sc != "" && sc != "public" {
-		return "", pgerror.Unimplemented(
+		return "", pgerror.Unimplementedf(
 			"import non-public schema",
-			fmt.Sprintf("non-public schemas unsupported: %s", sc),
+			"non-public schemas unsupported: %s", sc,
 		)
 	}
 	return tn.Table(), nil

pkg/ccl/importccl/read_import_proc.go

@@ -630,7 +630,7 @@ func (s sampleRate) sample(kv roachpb.KeyValue) bool {
 }
 
 func makeRowErr(file string, row int64, code, format string, args ...interface{}) error {
-	return pgerror.NewErrorWithDepthf(1, code,
+	return pgerror.NewWithDepthf(1, code,
 		"%q: row %d: "+format, append([]interface{}{file, row}, args...)...)
 }
 

pkg/ccl/partitionccl/partition.go

@@ -83,7 +83,7 @@ func valueEncodePartitionTuple(
 			value = encoding.EncodeNonsortingUvarint(value, uint64(sqlbase.PartitionMaxVal))
 			continue
 		case *tree.Placeholder:
-			return nil, pgerror.UnimplementedWithIssueErrorf(
+			return nil, pgerror.UnimplementedWithIssuef(
 				19464, "placeholders are not supported in PARTITION BY")
 		default:
 			// Fall-through.
@@ -96,7 +96,7 @@ func valueEncodePartitionTuple(
 			return nil, err
 		}
 		if !tree.IsConst(evalCtx, typedExpr) {
-			return nil, pgerror.NewErrorf(pgerror.CodeSyntaxError,
+			return nil, pgerror.Newf(pgerror.CodeSyntaxError,
 				"%s: partition values must be constant", typedExpr)
 		}
 		datum, err := typedExpr.Eval(evalCtx)
@@ -166,7 +166,7 @@ func createPartitioningImpl(
 	var cols []sqlbase.ColumnDescriptor
 	for i := 0; i < len(partBy.Fields); i++ {
 		if colOffset+i >= len(indexDesc.ColumnNames) {
-			return partDesc, pgerror.NewErrorf(pgerror.CodeSyntaxError,
+			return partDesc, pgerror.Newf(pgerror.CodeSyntaxError,
 				"declared partition columns (%s) exceed the number of columns in index being partitioned (%s)",
 				partitioningString(), strings.Join(indexDesc.ColumnNames, ", "))
 		}
@@ -179,7 +179,7 @@ func createPartitioningImpl(
 		cols = append(cols, *col)
 		if string(partBy.Fields[i]) != col.Name {
 			n := colOffset + len(partBy.Fields)
-			return partDesc, pgerror.NewErrorf(pgerror.CodeSyntaxError,
+			return partDesc, pgerror.Newf(pgerror.CodeSyntaxError,
 				"declared partition columns (%s) do not match first %d columns in index being partitioned (%s)",
 				partitioningString(), n, strings.Join(indexDesc.ColumnNames[:n], ", "))
 		}
@@ -228,7 +228,7 @@ func createPartitioningImpl(
 				"PARTITION %s", p.Name)
 		}
 		if r.Subpartition != nil {
-			return partDesc, pgerror.NewErrorf(pgerror.CodeDataExceptionError,
+			return partDesc, pgerror.Newf(pgerror.CodeDataExceptionError,
 				"PARTITION %s: cannot subpartition a range partition", p.Name)
 		}
 		partDesc.Range = append(partDesc.Range, p)

pkg/ccl/roleccl/role.go

@@ -79,7 +79,7 @@ func grantRolePlanHook(
 		}
 		for _, r := range grant.Roles {
 			if isAdmin, ok := allRoles[string(r)]; !ok || !isAdmin {
-				return nil, pgerror.NewErrorf(pgerror.CodeInsufficientPrivilegeError,
+				return nil, pgerror.Newf(pgerror.CodeInsufficientPrivilegeError,
 					"%s is not a superuser or role admin for role %s", p.User(), r)
 			}
 		}
@@ -99,14 +99,14 @@ func grantRolePlanHook(
 	// Check roles: these have to be roles.
 	for _, r := range grant.Roles {
 		if isRole, ok := users[string(r)]; !ok || !isRole {
-			return nil, pgerror.NewErrorf(pgerror.CodeUndefinedObjectError, "role %s does not exist", r)
+			return nil, pgerror.Newf(pgerror.CodeUndefinedObjectError, "role %s does not exist", r)
 		}
 	}
 
 	// Check grantees: these can be users or roles.
 	for _, m := range grant.Members {
 		if _, ok := users[string(m)]; !ok {
-			return nil, pgerror.NewErrorf(pgerror.CodeUndefinedObjectError, "user or role %s does not exist", m)
+			return nil, pgerror.Newf(pgerror.CodeUndefinedObjectError, "user or role %s does not exist", m)
 		}
 	}
 
@@ -132,12 +132,12 @@ func grantRolePlanHook(
 			m := string(rawM)
 			if r == m {
 				// self-cycle.
-				return nil, pgerror.NewErrorf(pgerror.CodeInvalidGrantOperationError, "%s cannot be a member of itself", m)
+				return nil, pgerror.Newf(pgerror.CodeInvalidGrantOperationError, "%s cannot be a member of itself", m)
 			}
 			// Check if grant.Role ∈ ... ∈ grant.Member
 			if memberOf, ok := allRoleMemberships[r]; ok {
 				if _, ok = memberOf[m]; ok {
-					return nil, pgerror.NewErrorf(pgerror.CodeInvalidGrantOperationError,
+					return nil, pgerror.Newf(pgerror.CodeInvalidGrantOperationError,
 						"making %s a member of %s would create a cycle", m, r)
 				}
 			}
@@ -208,7 +208,7 @@ func revokeRolePlanHook(
 		}
 		for _, r := range revoke.Roles {
 			if isAdmin, ok := allRoles[string(r)]; !ok || !isAdmin {
-				return nil, pgerror.NewErrorf(pgerror.CodeInsufficientPrivilegeError,
+				return nil, pgerror.Newf(pgerror.CodeInsufficientPrivilegeError,
 					"%s is not a superuser or role admin for role %s", p.User(), r)
 			}
 		}
@@ -225,14 +225,14 @@ func revokeRolePlanHook(
 	// Check roles: these have to be roles.
 	for _, r := range revoke.Roles {
 		if isRole, ok := users[string(r)]; !ok || !isRole {
-			return nil, pgerror.NewErrorf(pgerror.CodeUndefinedObjectError, "role %s does not exist", r)
+			return nil, pgerror.Newf(pgerror.CodeUndefinedObjectError, "role %s does not exist", r)
 		}
 	}
 
 	// Check members: these can be users or roles.
 	for _, m := range revoke.Members {
 		if _, ok := users[string(m)]; !ok {
-			return nil, pgerror.NewErrorf(pgerror.CodeUndefinedObjectError, "user or role %s does not exist", m)
+			return nil, pgerror.Newf(pgerror.CodeUndefinedObjectError, "user or role %s does not exist", m)
 		}
 	}
 
@@ -250,7 +250,7 @@ func revokeRolePlanHook(
 		for _, m := range revoke.Members {
 			if string(r) == sqlbase.AdminRole && string(m) == security.RootUser {
 				// We use CodeObjectInUseError which is what happens if you tried to delete the current user in pg.
-				return nil, pgerror.NewErrorf(pgerror.CodeObjectInUseError,
+				return nil, pgerror.Newf(pgerror.CodeObjectInUseError,
 					"user %s cannot be removed from role %s or lose the ADMIN OPTION",
 					security.RootUser, sqlbase.AdminRole)
 			}

pkg/ccl/utilccl/licenseccl/license.go

@@ -39,7 +39,7 @@ func Decode(s string) (*License, error) {
 		return nil, nil
 	}
 	if !strings.HasPrefix(s, LicensePrefix) {
-		return nil, pgerror.NewErrorf(pgerror.CodeSyntaxError, "invalid license string")
+		return nil, pgerror.Newf(pgerror.CodeSyntaxError, "invalid license string")
 	}
 	s = strings.TrimPrefix(s, LicensePrefix)
 	data, err := base64.RawStdEncoding.DecodeString(s)
@@ -59,7 +59,7 @@ func (l *License) Check(at time.Time, cluster uuid.UUID, org, feature string) er
 		// TODO(dt): link to some stable URL that then redirects to a helpful page
 		// that explains what to do here.
 		link := "https://cockroachlabs.com/pricing?cluster="
-		return pgerror.NewErrorf(pgerror.CodeCCLValidLicenseRequired,
+		return pgerror.Newf(pgerror.CodeCCLValidLicenseRequired,
 			"use of %s requires an enterprise license. "+
 				"see %s%s for details on how to enable enterprise features",
 			log.Safe(feature),
@@ -79,7 +79,7 @@ func (l *License) Check(at time.Time, cluster uuid.UUID, org, feature string) er
 			case License_Evaluation:
 				licensePrefix = "evaluation "
 			}
-			return pgerror.NewErrorf(pgerror.CodeCCLValidLicenseRequired,
+			return pgerror.Newf(pgerror.CodeCCLValidLicenseRequired,
 				"Use of %s requires an enterprise license. Your %slicense expired on %s. If you're "+
 					"interested in getting a new license, please contact subscriptions@cockroachlabs.com "+
 					"and we can help you out.",
@@ -94,7 +94,7 @@ func (l *License) Check(at time.Time, cluster uuid.UUID, org, feature string) er
 		if strings.EqualFold(l.OrganizationName, org) {
 			return nil
 		}
-		return pgerror.NewErrorf(pgerror.CodeCCLValidLicenseRequired,
+		return pgerror.Newf(pgerror.CodeCCLValidLicenseRequired,
 			"license valid only for %q", l.OrganizationName)
 	}
 
@@ -112,7 +112,7 @@ func (l *License) Check(at time.Time, cluster uuid.UUID, org, feature string) er
 		}
 		matches.WriteString(c.String())
 	}
-	return pgerror.NewErrorf(pgerror.CodeCCLValidLicenseRequired,
+	return pgerror.Newf(pgerror.CodeCCLValidLicenseRequired,
 		"license for cluster(s) %s is not valid for cluster %s",
 		matches.String(), cluster.String(),
 	)