Merge #148616

148616: sql: support referencing a routine from a view query r=rafiss,rytaft,mgartner a=DrewKimball

#### sql,schemachanger: add remaining handling for routine references in views

This commit adds the remaining logic needed to keep track of references
to routines from a view query in the schema changer. The following commit
will resolve these references upon planning the `CREATE VIEW` statement
and pass them to the schema changer.

Informs #146123

Release note: None

#### sql: support referencing a routine from a view query

This commit adds support for invoking a UDF from a view (materialized or
otherwise). This requires resolving routine references while planning the
`CREATE VIEW` statement and passing them to the schema changer.

Fixes #146123

Release note (sql change): Added support for invoking a UDF from a view
query. Renaming or setting the schema on the routine is currently not
allowed if it is referenced by a view.

Co-authored-by: Drew Kimball <[email protected]>

Author: craig[bot]

pkg/sql/alter_function.go

@@ -210,32 +210,22 @@ func (n *alterFunctionRenameNode) startExec(params runParams) error {
 		}
 	}
 
-	// Disallow renaming if this rename operation will break other UDF's invoking
-	// this one.
-	var dependentFuncs []string
-	for _, dep := range fnDesc.GetDependedOnBy() {
-		desc, err := params.p.Descriptors().ByIDWithoutLeased(params.p.Txn()).Get().Desc(params.ctx, dep.ID)
-		if err != nil {
-			return err
-		}
-		_, ok := desc.(catalog.FunctionDescriptor)
-		if !ok {
-			continue
-		}
-		fullyResolvedName, err := params.p.GetQualifiedFunctionNameByID(params.ctx, int64(dep.ID))
-		if err != nil {
-			return err
-		}
-		dependentFuncs = append(dependentFuncs, fullyResolvedName.FQString())
+	// Disallow renaming if this rename operation will break other UDF's or views
+	// invoking this one.
+	dependentObjects, err := getFuncRefsDisallowingAlter(params, fnDesc)
+	if err != nil {
+		return err
 	}
-	if len(dependentFuncs) > 0 {
+	if len(dependentObjects) > 0 {
+		// TODO(#146475): once functions are rewritten to use ID references, we can
+		// allow renames for views.
 		return errors.UnimplementedErrorf(
 			errors.IssueLink{
 				IssueURL: build.MakeIssueURL(83233),
 				Detail:   "renames are disallowed because references are by name",
 			},
-			"cannot rename function %q because other functions ([%v]) still depend on it",
-			fnDesc.Name, strings.Join(dependentFuncs, ", "))
+			"cannot rename function %q because other functions or views ([%v]) still depend on it",
+			fnDesc.Name, strings.Join(dependentObjects, ", "))
 	}
 
 	scDesc.RemoveFunction(fnDesc.GetName(), fnDesc.GetID())
@@ -383,33 +373,21 @@ func (n *alterFunctionSetSchemaNode) startExec(params runParams) error {
 	if err != nil {
 		return err
 	}
-	// Disallow renaming if this rename operation will break other UDF's invoking
-	// this one.
-	var dependentFuncs []string
-	for _, dep := range fnDesc.GetDependedOnBy() {
-		desc, err := params.p.Descriptors().ByIDWithoutLeased(params.p.Txn()).Get().Desc(params.ctx, dep.ID)
-		if err != nil {
-			return err
-		}
-		_, ok := desc.(catalog.FunctionDescriptor)
-		if !ok {
-			continue
-		}
-		fullyResolvedName, err := params.p.GetQualifiedFunctionNameByID(params.ctx, int64(dep.ID))
-		if err != nil {
-			return err
-		}
-		dependentFuncs = append(dependentFuncs, fullyResolvedName.FQString())
+	// Disallow renaming if this rename operation will break other UDF's or views
+	// invoking this one.
+	dependentObjects, err := getFuncRefsDisallowingAlter(params, fnDesc)
+	if err != nil {
+		return err
 	}
-	if len(dependentFuncs) > 0 {
+	if len(dependentObjects) > 0 {
 		return errors.UnimplementedErrorf(
 			errors.IssueLink{
 				IssueURL: build.MakeIssueURL(83233),
 				Detail: "set schema is disallowed because there are references from " +
 					"other objects by name",
 			},
-			"cannot set schema for function %q because other functions ([%v]) still depend on it",
-			fnDesc.Name, strings.Join(dependentFuncs, ", "))
+			"cannot set schema for function %q because other functions or views ([%v]) still depend on it",
+			fnDesc.Name, strings.Join(dependentObjects, ", "))
 	}
 
 	switch sc.SchemaKind() {
@@ -548,3 +526,35 @@ func toSchemaOverloadSignature(fnDesc *funcdesc.Mutable) descpb.SchemaDescriptor
 	}
 	return ret
 }
+
+func getFuncRefsDisallowingAlter(
+	params runParams, fnDesc *funcdesc.Mutable,
+) (dependentObjects []string, err error) {
+	for _, dep := range fnDesc.GetDependedOnBy() {
+		desc, err := params.p.Descriptors().ByIDWithoutLeased(params.p.Txn()).Get().Desc(params.ctx, dep.ID)
+		if err != nil {
+			return nil, err
+		}
+		switch t := desc.(type) {
+		case catalog.TableDescriptor:
+			if !t.IsView() {
+				continue
+			}
+			fullyResolvedName, err := params.p.GetQualifiedTableNameByID(
+				params.ctx, int64(dep.ID), tree.ResolveAnyTableKind)
+			if err != nil {
+				return nil, err
+			}
+			dependentObjects = append(dependentObjects, fullyResolvedName.FQString())
+		case catalog.FunctionDescriptor:
+			fullyResolvedName, err := params.p.GetQualifiedFunctionNameByID(params.ctx, int64(dep.ID))
+			if err != nil {
+				return nil, err
+			}
+			dependentObjects = append(dependentObjects, fullyResolvedName.FQString())
+		default:
+			continue
+		}
+	}
+	return dependentObjects, nil
+}

pkg/sql/catalog/descpb/structured.proto

@@ -1838,6 +1838,9 @@ message FunctionDescriptor {
       (gogoproto.casttype) = "TriggerID"];
     repeated uint32 policy_ids = 6 [(gogoproto.customname) = "PolicyIDs",
       (gogoproto.casttype) = "PolicyID"];
+    // ViewQuery indicates that this function is referenced in the query of a
+    // view.
+    optional bool view_query = 7 [(gogoproto.nullable) = false];
   }
 
   optional string name = 1 [(gogoproto.nullable) = false];

pkg/sql/catalog/funcdesc/func_desc.go

@@ -890,6 +890,34 @@ func (desc *Mutable) AddIndexReference(id descpb.ID, indexID descpb.IndexID) err
 	return nil
 }
 
+// AddViewReference adds back reference to a view to the function.
+func (desc *Mutable) AddViewReference(id descpb.ID) error {
+	for _, dep := range desc.DependsOn {
+		if dep == id {
+			return pgerror.Newf(pgcode.InvalidFunctionDefinition,
+				"cannot add dependency from descriptor %d to function %s (%d) because there will be a dependency cycle", id, desc.GetName(), desc.GetID(),
+			)
+		}
+	}
+	for i := range desc.DependedOnBy {
+		if desc.DependedOnBy[i].ID == id {
+			desc.DependedOnBy[i].ViewQuery = true
+			return nil
+		}
+	}
+	desc.DependedOnBy = append(
+		desc.DependedOnBy,
+		descpb.FunctionDescriptor_Reference{
+			ID:        id,
+			ViewQuery: true,
+		},
+	)
+	sort.Slice(desc.DependedOnBy, func(i, j int) bool {
+		return desc.DependedOnBy[i].ID < desc.DependedOnBy[j].ID
+	})
+	return nil
+}
+
 // RemoveIndexReference removes back reference to an index from the function.
 func (desc *Mutable) RemoveIndexReference(id descpb.ID, indexID descpb.IndexID) {
 	for i := range desc.DependedOnBy {
@@ -906,6 +934,17 @@ func (desc *Mutable) RemoveIndexReference(id descpb.ID, indexID descpb.IndexID)
 	}
 }
 
+// RemoveViewReference removes back reference to a view from the function.
+func (desc *Mutable) RemoveViewReference(id descpb.ID) {
+	for i := range desc.DependedOnBy {
+		if desc.DependedOnBy[i].ID == id {
+			desc.DependedOnBy[i].ViewQuery = false
+			desc.maybeRemoveTableReference(id)
+			return
+		}
+	}
+}
+
 // maybeRemoveTableReference removes a table's references from the function if
 // the column, index and constraint references are all empty. This function is
 // only used internally when removing an individual column, index or constraint
@@ -914,7 +953,8 @@ func (desc *Mutable) maybeRemoveTableReference(id descpb.ID) {
 	var ret []descpb.FunctionDescriptor_Reference
 	for _, ref := range desc.DependedOnBy {
 		if ref.ID == id && len(ref.ColumnIDs) == 0 && len(ref.IndexIDs) == 0 &&
-			len(ref.ConstraintIDs) == 0 && len(ref.TriggerIDs) == 0 && len(ref.PolicyIDs) == 0 {
+			len(ref.ConstraintIDs) == 0 && len(ref.TriggerIDs) == 0 &&
+			len(ref.PolicyIDs) == 0 && !ref.ViewQuery {
 			continue
 		}
 		ret = append(ret, ref)

pkg/sql/create_view.go

@@ -299,12 +299,12 @@ func (n *createViewNode) startExec(params runParams) error {
 				}
 				desc.DependsOnTypes = append(desc.DependsOnTypes, orderedTypeDeps.Ordered()...)
 
-				// Collect all functions this view depends on.
-				orderedFuncDeps := catalog.DescriptorIDSet{}
+				// Collect all routines this view depends on.
+				orderedRoutineDeps := catalog.DescriptorIDSet{}
 				for backrefID := range n.funcDeps {
-					orderedFuncDeps.Add(backrefID)
+					orderedRoutineDeps.Add(backrefID)
 				}
-				desc.DependsOnFunctions = append(desc.DependsOnFunctions, orderedFuncDeps.Ordered()...)
+				desc.DependsOnFunctions = append(desc.DependsOnFunctions, orderedRoutineDeps.Ordered()...)
 
 				newDesc = &desc
 
@@ -357,6 +357,13 @@ func (n *createViewNode) startExec(params runParams) error {
 				}
 			}
 
+			// Add back references for the routine dependencies.
+			for id := range n.funcDeps {
+				if err := params.p.addRoutineViewBackReference(params.ctx, id, newDesc.ID); err != nil {
+					return err
+				}
+			}
+
 			if err := validateDescriptor(params.ctx, params.p, newDesc); err != nil {
 				return err
 			}
@@ -796,6 +803,18 @@ func (p *planner) replaceViewDesc(
 		return nil, err
 	}
 
+	// For each old function dependency (i.e. before replacing the view),
+	// see if we still depend on it. If not, then remove the back reference.
+	var outdatedRoutineRefs []descpb.ID
+	for _, id := range toReplace.DependsOnFunctions {
+		if _, ok := n.funcDeps[id]; !ok {
+			outdatedRoutineRefs = append(outdatedRoutineRefs, id)
+		}
+	}
+	if err := p.removeRoutineViewBackReferences(ctx, outdatedRoutineRefs, toReplace.ID); err != nil {
+		return nil, err
+	}
+
 	// Since the view query has been replaced, the dependencies that this
 	// table descriptor had are gone.
 	toReplace.DependsOn = make([]descpb.ID, 0, len(n.planDeps))
@@ -806,6 +825,10 @@ func (p *planner) replaceViewDesc(
 	for backrefID := range n.typeDeps {
 		toReplace.DependsOnTypes = append(toReplace.DependsOnTypes, backrefID)
 	}
+	toReplace.DependsOnFunctions = make([]descpb.ID, 0, len(n.funcDeps))
+	for backrefID := range n.funcDeps {
+		toReplace.DependsOnFunctions = append(toReplace.DependsOnFunctions, backrefID)
+	}
 
 	// Since we are replacing an existing view here, we need to write the new
 	// descriptor into place.
@@ -907,3 +930,36 @@ func crossDBReferenceDeprecationHint() string {
 	return fmt.Sprintf("Note that cross-database references will be removed in future releases. See: %s",
 		docs.ReleaseNotesURL(`#deprecations`))
 }
+
+func (p *planner) addRoutineViewBackReference(
+	ctx context.Context, routineID descpb.ID, ref descpb.ID,
+) error {
+	mutDesc, err := p.Descriptors().MutableByID(p.txn).Function(ctx, routineID)
+	if err != nil {
+		return err
+	}
+
+	if err := mutDesc.AddViewReference(ref); err != nil {
+		return err
+	}
+	if err := p.writeFuncSchemaChange(ctx, mutDesc); err != nil {
+		return err
+	}
+	return nil
+}
+
+func (p *planner) removeRoutineViewBackReferences(
+	ctx context.Context, routineIDs []descpb.ID, ref descpb.ID,
+) error {
+	for _, routineID := range routineIDs {
+		mutDesc, err := p.Descriptors().MutableByID(p.txn).Function(ctx, routineID)
+		if err != nil {
+			return err
+		}
+		mutDesc.RemoveViewReference(ref)
+		if err := p.writeFuncSchemaChange(ctx, mutDesc); err != nil {
+			return err
+		}
+	}
+	return nil
+}

pkg/sql/distsql_spec_exec_factory.go

@@ -1580,6 +1580,7 @@ func (e *distSQLSpecExecFactory) ConstructCreateView(
 	columns colinfo.ResultColumns,
 	deps opt.SchemaDeps,
 	typeDeps opt.SchemaTypeDeps,
+	funcDeps opt.SchemaFunctionDeps,
 ) (exec.Node, error) {
 	return nil, unimplemented.NewWithIssue(47473, "experimental opt-driven distsql planning: create view")
 }

pkg/sql/drop_view.go

@@ -323,6 +323,12 @@ func (p *planner) dropViewImpl(
 		return cascadeDroppedViews, err
 	}
 
+	// Remove back-references from the routines this view depends on.
+	routinesDependedOn := append([]descpb.ID(nil), viewDesc.DependsOnFunctions...)
+	if err := p.removeRoutineViewBackReferences(ctx, routinesDependedOn, viewDesc.ID); err != nil {
+		return cascadeDroppedViews, err
+	}
+
 	if behavior == tree.DropCascade {
 		dependedOnBy := append([]descpb.TableDescriptor_Reference(nil), viewDesc.DependedOnBy...)
 		for _, ref := range dependedOnBy {

pkg/sql/logictest/testdata/logic_test/drop_function

@@ -301,7 +301,7 @@ CREATE SCHEMA altSchema;
 CREATE FUNCTION altSchema.f_called_by_b() RETURNS INT LANGUAGE SQL AS $$ SELECT 1 $$;
 CREATE TABLE t1_with_b_2_ref(j int default altSchema.f_called_by_b() CHECK (altSchema.f_called_by_b() > 0));
 
-statement error pgcode 0A000 cannot set schema for function \"f_called_by_b\" because other functions \(\[test.public.f_called_by_b2\]\) still depend on it
+statement error pgcode 0A000 cannot set schema for function \"f_called_by_b\" because other functions or views \(\[test.public.f_called_by_b2\]\) still depend on it
 ALTER FUNCTION f_called_by_b SET SCHEMA altSchema;
 
 skipif config local-legacy-schema-changer