Merge #106333

106333: sql/schemachanger: fix prefix resolution errors during CREATE r=fqazi a=fqazi Previously, in the declarative schema changer, when resolving prefixes for new objects (CREATE FUNCTION / CREATE SEQUENCE), we would incorrectly generate internal errors if the resolution failed. This was incorrect and a regression versus the legacy schema changer. To address this, this patch refactors the resolution logic to generate appropriate errors for target objects when the prefix can't be resolved. Fixes: #106302 Release note: None Co-authored-by: Faizan Qazi <[email protected]>
cockroachdb · Jul 13, 2023 · 20fdc91 · 20fdc91
2 parents 5a6c91a + 9da5dd8
commit 20fdc91
Show file tree

Hide file tree

Showing 8 changed files with 45 additions and 32 deletions.
diff --git a/pkg/sql/logictest/testdata/logic_test/sequences b/pkg/sql/logictest/testdata/logic_test/sequences
@@ -2393,3 +2393,13 @@ statement error pgcode 42809 \"test.public.multiple_seq_test_tbl\" is not a sequ
 SELECT pg_sequence_last_value('multiple_seq_test_tbl'::regclass)
 
 subtest end
+
+# For #106302 we had cases where internal errors could be returned in the prefix
+# did not resolve properly
+subtest create_prefix
+
+statement error pgcode 3F000 cannot create \"timestamptz.primary.is\" because the target database or schema does not exist
+CREATE SEQUENCE TIMESTAMPTZ . PRIMARY . IS;
+
+statement error pgcode 3F000 cannot create \"timestamptz.is\" because the target database or schema does not exist
+CREATE SEQUENCE TIMESTAMPTZ . IS;
diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go
@@ -848,11 +848,25 @@ func (b *builderState) ResolveSchema(
 	return b.QueryByID(sc.GetID())
 }
 
-// ResolvePrefix implements the scbuildstmt.NameResolver interface.
-func (b *builderState) ResolvePrefix(
-	prefix tree.ObjectNamePrefix, requiredSchemaPriv privilege.Kind,
+// ResolveTargetObject implements the scbuildstmt.NameResolver interface.
+func (b *builderState) ResolveTargetObject(
+	name *tree.UnresolvedObjectName, requiredSchemaPriv privilege.Kind,
 ) (dbElts scbuildstmt.ElementResultSet, scElts scbuildstmt.ElementResultSet) {
-	db, sc := b.cr.MustResolvePrefix(b.ctx, prefix)
+	objName := name.ToTableName()
+	db, sc := b.cr.MayResolvePrefix(b.ctx, objName.ObjectNamePrefix)
+	// If the database or schema are not found during resolution,
+	// then generate an appropriate error.
+	if db == nil || sc == nil {
+		if !objName.ExplicitSchema && !objName.ExplicitCatalog {
+			panic(
+				pgerror.New(pgcode.InvalidName, "no database specified"),
+			)
+		}
+		panic(errors.WithHint(pgerror.Newf(pgcode.InvalidSchemaName,
+			"cannot create %q because the target database or schema does not exist",
+			tree.ErrString(&objName)),
+			"verify that the current database and search_path are valid and/or the target database exists"))
+	}
 	b.ensureDescriptor(db.GetID())
 	if sc.SchemaKind() == catalog.SchemaVirtual {
 		panic(sqlerrors.NewCannotModifyVirtualSchemaError(sc.GetName()))
@@ -861,7 +875,7 @@ func (b *builderState) ResolvePrefix(
 		panic(unimplemented.NewWithIssue(104687, "cannot create UDFs under a temporary schema"))
 	}
 	b.ensureDescriptor(sc.GetID())
-	b.checkOwnershipOrPrivilegesOnSchemaDesc(prefix, sc, scbuildstmt.ResolveParams{RequiredPrivilege: requiredSchemaPriv})
+	b.checkOwnershipOrPrivilegesOnSchemaDesc(objName.ObjectNamePrefix, sc, scbuildstmt.ResolveParams{RequiredPrivilege: requiredSchemaPriv})
 	return b.QueryByID(db.GetID()), b.QueryByID(sc.GetID())
 }
 

diff --git a/pkg/sql/schemachanger/scbuild/dependencies.go b/pkg/sql/schemachanger/scbuild/dependencies.go
@@ -129,10 +129,10 @@ type CatalogReader interface {
 	// If withOffline is set, we include offline schema descs into our search.
 	MayResolveSchema(ctx context.Context, name tree.ObjectNamePrefix, withOffline bool) (catalog.DatabaseDescriptor, catalog.SchemaDescriptor)
 
-	// MustResolvePrefix looks up a database and schema given the prefix at best
+	// MayResolvePrefix looks up a database and schema given the prefix at best
 	// effort, meaning the prefix may not have explicit catalog and schema name.
 	// It fails if the db or schema represented by the prefix does not exist.
-	MustResolvePrefix(ctx context.Context, name tree.ObjectNamePrefix) (catalog.DatabaseDescriptor, catalog.SchemaDescriptor)
+	MayResolvePrefix(ctx context.Context, name tree.ObjectNamePrefix) (catalog.DatabaseDescriptor, catalog.SchemaDescriptor)
 
 	// MayResolveTable looks up a table by name.
 	MayResolveTable(ctx context.Context, name tree.UnresolvedObjectName) (catalog.ResolvedObjectPrefix, catalog.TableDescriptor)

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go
@@ -30,7 +30,7 @@ func CreateFunction(b BuildCtx, n *tree.CreateFunction) {
 	}
 	b.IncrementSchemaChangeCreateCounter("function")
 
-	dbElts, scElts := b.ResolvePrefix(n.FuncName.ObjectNamePrefix, privilege.CREATE)
+	dbElts, scElts := b.ResolveTargetObject(n.FuncName.ToUnresolvedObjectName(), privilege.CREATE)
 	_, _, sc := scpb.FindSchema(scElts)
 	_, _, db := scpb.FindDatabase(dbElts)
 	_, _, scName := scpb.FindNamespace(scElts)

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_sequence.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_sequence.go
@@ -28,7 +28,7 @@ import (
 )
 
 func CreateSequence(b BuildCtx, n *tree.CreateSequence) {
-	dbElts, scElts := b.ResolvePrefix(n.Name.ObjectNamePrefix, privilege.CREATE)
+	dbElts, scElts := b.ResolveTargetObject(n.Name.ToUnresolvedObjectName(), privilege.CREATE)
 	_, _, schemaElem := scpb.FindSchema(scElts)
 	_, _, dbElem := scpb.FindDatabase(dbElts)
 	_, _, scName := scpb.FindNamespace(scElts)

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go
@@ -338,12 +338,10 @@ type NameResolver interface {
 	// ResolveSchema retrieves a schema by name and returns its elements.
 	ResolveSchema(name tree.ObjectNamePrefix, p ResolveParams) ElementResultSet
 
-	// ResolvePrefix retrieves database and schema given the name prefix. The
-	// requested schema must exist and current user must have the required
-	// privilege.
-	ResolvePrefix(
-		prefix tree.ObjectNamePrefix, requiredSchemaPriv privilege.Kind,
-	) (dbElts ElementResultSet, scElts ElementResultSet)
+	// ResolveTargetObject retrieves database and schema given the unresolved
+	// object name. The requested schema must exist and current user must have the
+	// required privilege.
+	ResolveTargetObject(prefix *tree.UnresolvedObjectName, requiredSchemaPriv privilege.Kind) (dbElts ElementResultSet, scElts ElementResultSet)
 
 	// ResolveUserDefinedTypeType retrieves a type by name and returns its elements.
 	ResolveUserDefinedTypeType(name *tree.UnresolvedObjectName, p ResolveParams) ElementResultSet

diff --git a/pkg/sql/schemachanger/scdeps/build_deps.go b/pkg/sql/schemachanger/scdeps/build_deps.go
@@ -127,22 +127,19 @@ func (d *buildDeps) MayResolveSchema(
 	return db, schema
 }
 
-func (d *buildDeps) MustResolvePrefix(
+func (d *buildDeps) MayResolvePrefix(
 	ctx context.Context, name tree.ObjectNamePrefix,
-) (catalog.DatabaseDescriptor, catalog.SchemaDescriptor) {
+) (db catalog.DatabaseDescriptor, sc catalog.SchemaDescriptor) {
 	const withOffline = false
 	if name.ExplicitSchema {
 		if name.ExplicitCatalog {
-			db, sc := d.MayResolveSchema(ctx, name, withOffline)
-			if sc == nil || db == nil {
-				panic(errors.AssertionFailedf("prefix %s does not exist", name.String()))
-			}
+			db, sc = d.MayResolveSchema(ctx, name, withOffline)
 			return db, sc
 		}
 
 		// Two parts: D.T.
 		// Try to use the current database, and be satisfied if it's sufficient to find the object.
-		db, sc := d.MayResolveSchema(ctx, tree.ObjectNamePrefix{
+		db, sc = d.MayResolveSchema(ctx, tree.ObjectNamePrefix{
 			CatalogName:     tree.Name(d.CurrentDatabase()),
 			SchemaName:      name.SchemaName,
 			ExplicitCatalog: true,
@@ -161,23 +158,20 @@ func (d *buildDeps) MustResolvePrefix(
 			ExplicitSchema:  true,
 		},
 			withOffline)
-		if db != nil && sc != nil {
-			return db, sc
-		}
-		panic(errors.AssertionFailedf("prefix %s does not exist", name.String()))
+		return db, sc
 	}
 
 	path := d.sessionData.SearchPath
 	iter := path.IterWithoutImplicitPGSchemas()
 	for scName, ok := iter.Next(); ok; scName, ok = iter.Next() {
 		name.SchemaName = tree.Name(scName)
 		name.ExplicitSchema = true
-		db, sc := d.MayResolveSchema(ctx, name, false /* withOffline */)
+		db, sc = d.MayResolveSchema(ctx, name, false /* withOffline */)
 		if sc != nil {
 			return db, sc
 		}
 	}
-	panic(errors.AssertionFailedf("prefix %s does not exist", name.String()))
+	return nil, nil
 }
 
 // MayResolveTable implements the scbuild.CatalogReader interface.

diff --git a/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go b/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go
@@ -286,13 +286,10 @@ func (s *TestState) MayResolveSchema(
 	return db, sc
 }
 
-func (s *TestState) MustResolvePrefix(
+func (s *TestState) MayResolvePrefix(
 	ctx context.Context, name tree.ObjectNamePrefix,
 ) (catalog.DatabaseDescriptor, catalog.SchemaDescriptor) {
 	db, sc := s.mayResolvePrefix(name)
-	if sc == nil {
-		panic(errors.AssertionFailedf("prefix %s does not exist", name.String()))
-	}
 	return db.(catalog.DatabaseDescriptor), sc.(catalog.SchemaDescriptor)
 }