From 92b6c8c6c1786964cb74b7e65e31b9f8e8cc1b77 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Wed, 1 Feb 2023 15:01:49 -0800 Subject: [PATCH] tree: distinguish UDFs and builtins that use a SQL string body This refactor changes the meaning of the `Overload.IsUDF` field to be true only for user-defined functions - meaning those created using `CREATE FUNCTION`. This is contrasted with builtin functions that are defined with a SQL string set in `Overload.Body`. Logic that cares only about user-defined functions can continue checking `Overload.IsUDF`, while cases that deal with the SQL body should use `Overload.HasSQLBody()`. Note that it is insufficient to check whether `Overload.Body` is empty because it is possible to define a UDF with an empty body. Informs #95214 Release note: None --- pkg/ccl/changefeedccl/cdceval/func_resolver.go | 5 ++--- pkg/sql/delegate/show_function.go | 2 +- pkg/sql/drop_function.go | 2 ++ pkg/sql/opt/optbuilder/scalar.go | 2 +- pkg/sql/schemachanger/scbuild/builder_state.go | 2 ++ pkg/sql/sem/builtins/builtins.go | 6 ------ pkg/sql/sem/builtins/pg_builtins.go | 18 ------------------ pkg/sql/sem/tree/overload.go | 15 +++++++++++---- pkg/sql/sem/tree/type_check.go | 3 +++ pkg/sql/sem/tree/udf.go | 2 +- 10 files changed, 23 insertions(+), 34 deletions(-) diff --git a/pkg/ccl/changefeedccl/cdceval/func_resolver.go b/pkg/ccl/changefeedccl/cdceval/func_resolver.go index 869bb60a4155..82d1b6082810 100644 --- a/pkg/ccl/changefeedccl/cdceval/func_resolver.go +++ b/pkg/ccl/changefeedccl/cdceval/func_resolver.go @@ -73,10 +73,9 @@ func (rs *cdcFunctionResolver) ResolveFunction( return nil, err } - // Since we may be dealing with UDFs, ensure it is something - // that's supported. + // Ensure that any overloads defined using a SQL string are supported. for _, overload := range funcDef.Overloads { - if overload.IsUDF { + if overload.HasSQLBody() { if err := checkOverloadSupported(fnName, overload.Overload); err != nil { return nil, err } diff --git a/pkg/sql/delegate/show_function.go b/pkg/sql/delegate/show_function.go index 19f1f8bd26f4..ba0d7fcb0534 100644 --- a/pkg/sql/delegate/show_function.go +++ b/pkg/sql/delegate/show_function.go @@ -39,7 +39,7 @@ AND function_name = %[2]s var udfSchema string for _, o := range fn.Overloads { - if o.IsUDF { + if o.HasSQLBody() { udfSchema = o.Schema } } diff --git a/pkg/sql/drop_function.go b/pkg/sql/drop_function.go index 09464c44fb33..55f81acb21c9 100644 --- a/pkg/sql/drop_function.go +++ b/pkg/sql/drop_function.go @@ -142,6 +142,8 @@ func (p *planner) matchUDF( } return nil, err } + // Note that we don't check ol.HasSQLBody() here, because builtin functions + // can't be dropped even if they are defined using a SQL string. if !ol.IsUDF { return nil, errors.Errorf( "cannot drop function %s%s because it is required by the database system", diff --git a/pkg/sql/opt/optbuilder/scalar.go b/pkg/sql/opt/optbuilder/scalar.go index a8243a3b602b..3c02ccd88fd4 100644 --- a/pkg/sql/opt/optbuilder/scalar.go +++ b/pkg/sql/opt/optbuilder/scalar.go @@ -548,7 +548,7 @@ func (b *Builder) buildFunction( } overload := f.ResolvedOverload() - if overload.IsUDF { + if overload.HasSQLBody() { return b.buildUDF(f, def, inScope, outScope, outCol, colRefs) } diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go index 216ba9abb62f..38094c008d3d 100644 --- a/pkg/sql/schemachanger/scbuild/builder_state.go +++ b/pkg/sql/schemachanger/scbuild/builder_state.go @@ -1094,6 +1094,8 @@ func (b *builderState) ResolveUDF( panic(err) } + // ResolveUDF is not concerned with builtin functions that are defined using + // a SQL string, so we don't check ol.HasSQLBody() here. if !ol.IsUDF { panic( errors.Errorf( diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 08f89d47ca95..b1240460598c 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -4940,7 +4940,6 @@ value if you rely on the HLC for accuracy.`, {Name: "id", Typ: types.Int}, }, ReturnType: tree.FixedReturnType(types.Int), - IsUDF: true, Body: `SELECT crdb_internal.create_tenant(json_build_object('id', $1, 'service_mode', 'external'))`, Info: `create_tenant(id) is an alias for create_tenant('{"id": id, "service_mode": "external"}'::jsonb)`, @@ -4953,7 +4952,6 @@ value if you rely on the HLC for accuracy.`, {Name: "name", Typ: types.String}, }, ReturnType: tree.FixedReturnType(types.Int), - IsUDF: true, Body: `SELECT crdb_internal.create_tenant(json_build_object('id', $1, 'name', $2))`, Info: `create_tenant(id, name) is an alias for create_tenant('{"id": id, "name": name}'::jsonb)`, Volatility: volatility.Volatile, @@ -4964,7 +4962,6 @@ value if you rely on the HLC for accuracy.`, {Name: "name", Typ: types.String}, }, ReturnType: tree.FixedReturnType(types.Int), - IsUDF: true, Body: `SELECT crdb_internal.create_tenant(json_build_object('name', $1))`, Info: `create_tenant(name) is an alias for create_tenant('{"name": name}'::jsonb). DO NOT USE -- USE 'CREATE TENANT' INSTEAD`, @@ -5017,7 +5014,6 @@ DO NOT USE -- USE 'CREATE TENANT' INSTEAD`, {Name: "id", Typ: types.Int}, }, ReturnType: tree.FixedReturnType(types.Int), - IsUDF: true, Body: `SELECT crdb_internal.destroy_tenant($1, false)`, Info: "DO NOT USE -- USE 'DROP TENANT' INSTEAD.", Volatility: volatility.Volatile, @@ -6361,7 +6357,6 @@ DO NOT USE -- USE 'CREATE TENANT' INSTEAD`, {Name: "number", Typ: types.Int}, }, ReturnType: tree.FixedReturnType(types.Jsonb), - IsUDF: true, Body: `SELECT crdb_internal.generate_test_objects( json_build_object('names', $1, 'counts', array[$2]))`, Info: `Generates a number of objects whose name follow the provided pattern. @@ -6377,7 +6372,6 @@ generate_test_objects('{"names":pat, "counts":[num]}'::jsonb) {Name: "counts", Typ: types.IntArray}, }, ReturnType: tree.FixedReturnType(types.Jsonb), - IsUDF: true, Body: `SELECT crdb_internal.generate_test_objects( json_build_object('names', $1, 'counts', $2))`, Info: `Generates a number of objects whose name follow the provided pattern. diff --git a/pkg/sql/sem/builtins/pg_builtins.go b/pkg/sql/sem/builtins/pg_builtins.go index 4a10ed242d43..2aa5bd08f34f 100644 --- a/pkg/sql/sem/builtins/pg_builtins.go +++ b/pkg/sql/sem/builtins/pg_builtins.go @@ -209,7 +209,6 @@ func makePGGetViewDef(paramTypes tree.ParamTypes) tree.Overload { return tree.Overload{ Types: paramTypes, ReturnType: tree.FixedReturnType(types.String), - IsUDF: true, Body: `SELECT definition FROM pg_catalog.pg_views v JOIN pg_catalog.pg_class c ON c.relname=v.viewname @@ -229,7 +228,6 @@ func makePGGetConstraintDef(paramTypes tree.ParamTypes) tree.Overload { return tree.Overload{ Types: paramTypes, ReturnType: tree.FixedReturnType(types.String), - IsUDF: true, Body: `SELECT condef FROM pg_catalog.pg_constraint WHERE oid=$1 LIMIT 1`, Info: notUsableInfo, Volatility: volatility.Stable, @@ -597,7 +595,6 @@ var pgBuiltins = map[string]builtinDefinition{ tree.Overload{ Types: tree.ParamTypes{{Name: "func_oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.String), - IsUDF: true, Body: fmt.Sprintf( `SELECT COALESCE(create_statement, prosrc) FROM pg_catalog.pg_proc @@ -617,7 +614,6 @@ var pgBuiltins = map[string]builtinDefinition{ tree.Overload{ Types: tree.ParamTypes{{Name: "func_oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.String), - IsUDF: true, Body: getFunctionArgStringQuery, Info: "Returns the argument list (with defaults) necessary to identify a function, " + "in the form it would need to appear in within CREATE FUNCTION.", @@ -634,7 +630,6 @@ var pgBuiltins = map[string]builtinDefinition{ tree.Overload{ Types: tree.ParamTypes{{Name: "func_oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.String), - IsUDF: true, Body: `SELECT t.typname FROM pg_catalog.pg_proc p JOIN pg_catalog.pg_type t @@ -654,7 +649,6 @@ var pgBuiltins = map[string]builtinDefinition{ tree.Overload{ Types: tree.ParamTypes{{Name: "func_oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.String), - IsUDF: true, Body: getFunctionArgStringQuery, Info: "Returns the argument list (without defaults) necessary to identify a function, " + "in the form it would need to appear in within ALTER FUNCTION, for instance.", @@ -667,7 +661,6 @@ var pgBuiltins = map[string]builtinDefinition{ "pg_get_indexdef": makeBuiltin( tree.FunctionProperties{Category: builtinconstants.CategorySystemInfo, DistsqlBlocklist: true}, tree.Overload{ - IsUDF: true, Types: tree.ParamTypes{{Name: "index_oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.String), Body: `SELECT indexdef FROM pg_catalog.pg_indexes WHERE crdb_oid = $1`, @@ -675,7 +668,6 @@ var pgBuiltins = map[string]builtinDefinition{ Volatility: volatility.Stable, }, tree.Overload{ - IsUDF: true, Types: tree.ParamTypes{{Name: "index_oid", Typ: types.Oid}, {Name: "column_no", Typ: types.Int}, {Name: "pretty_bool", Typ: types.Bool}}, ReturnType: tree.FixedReturnType(types.String), Body: `SELECT CASE @@ -847,7 +839,6 @@ var pgBuiltins = map[string]builtinDefinition{ {Name: "role_oid", Typ: types.Oid}, }, ReturnType: tree.FixedReturnType(types.String), - IsUDF: true, Body: `SELECT COALESCE((SELECT rolname FROM pg_catalog.pg_roles WHERE oid=$1 LIMIT 1), 'unknown (OID=' || $1 || ')')`, Info: notUsableInfo, Volatility: volatility.Stable, @@ -866,7 +857,6 @@ var pgBuiltins = map[string]builtinDefinition{ []*types.T{types.Int, types.Int, types.Int, types.Int, types.Bool, types.Int, types.Oid}, []string{"start_value", "minimum_value", "maxmimum_value", "increment", "cycle_option", "cache_size", "data_type"}, )), - IsUDF: true, Body: `SELECT COALESCE ((SELECT (seqstart, seqmin, seqmax, seqincrement, seqcycle, seqcache, seqtypid) FROM pg_catalog.pg_sequence WHERE seqrelid=$1 LIMIT 1), CASE WHEN crdb_internal.force_error('42P01', 'relation with OID ' || $1 || ' does not exist') > 0 THEN NULL ELSE NULL END)`, @@ -924,7 +914,6 @@ var pgBuiltins = map[string]builtinDefinition{ "col_description": makeBuiltin(defProps(), tree.Overload{ - IsUDF: true, Types: tree.ParamTypes{{Name: "table_oid", Typ: types.Oid}, {Name: "column_number", Typ: types.Int}}, ReturnType: tree.FixedReturnType(types.String), // Note: the following is equivalent to: @@ -954,7 +943,6 @@ var pgBuiltins = map[string]builtinDefinition{ "obj_description": makeBuiltin(defProps(), tree.Overload{ - IsUDF: true, Types: tree.ParamTypes{{Name: "object_oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.String), Body: `SELECT description @@ -968,7 +956,6 @@ var pgBuiltins = map[string]builtinDefinition{ Volatility: volatility.Stable, }, tree.Overload{ - IsUDF: true, Types: tree.ParamTypes{{Name: "object_oid", Typ: types.Oid}, {Name: "catalog_name", Typ: types.String}}, ReturnType: tree.FixedReturnType(types.String), Body: `SELECT d.description @@ -1002,7 +989,6 @@ var pgBuiltins = map[string]builtinDefinition{ "shobj_description": makeBuiltin(defProps(), tree.Overload{ - IsUDF: true, Types: tree.ParamTypes{{Name: "object_oid", Typ: types.Oid}, {Name: "catalog_name", Typ: types.String}}, ReturnType: tree.FixedReturnType(types.String), Body: `SELECT d.description @@ -1064,7 +1050,6 @@ var pgBuiltins = map[string]builtinDefinition{ // https://www.postgresql.org/docs/9.6/static/functions-info.html "pg_function_is_visible": makeBuiltin(defProps(), tree.Overload{ - IsUDF: true, Types: tree.ParamTypes{{Name: "oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.Bool), Body: `SELECT n.nspname = any current_schemas(true) @@ -1081,7 +1066,6 @@ var pgBuiltins = map[string]builtinDefinition{ // https://www.postgresql.org/docs/9.6/static/functions-info.html "pg_table_is_visible": makeBuiltin(defProps(), tree.Overload{ - IsUDF: true, Types: tree.ParamTypes{{Name: "oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.Bool), Body: `SELECT n.nspname = any current_schemas(true) @@ -1102,7 +1086,6 @@ var pgBuiltins = map[string]builtinDefinition{ // https://www.postgresql.org/docs/9.6/static/functions-info.html "pg_type_is_visible": makeBuiltin(defProps(), tree.Overload{ - IsUDF: true, Types: tree.ParamTypes{{Name: "oid", Typ: types.Oid}}, ReturnType: tree.FixedReturnType(types.Bool), Body: `SELECT n.nspname = any current_schemas(true) @@ -1898,7 +1881,6 @@ var pgBuiltins = map[string]builtinDefinition{ // https://github.com/postgres/postgres/blob/master/src/backend/catalog/information_schema.sql "information_schema._pg_index_position": makeBuiltin(defProps(), tree.Overload{ - IsUDF: true, Types: tree.ParamTypes{ {Name: "oid", Typ: types.Oid}, {Name: "col", Typ: types.Int2}, diff --git a/pkg/sql/sem/tree/overload.go b/pkg/sql/sem/tree/overload.go index 2922292f0352..42b4bb003c31 100644 --- a/pkg/sql/sem/tree/overload.go +++ b/pkg/sql/sem/tree/overload.go @@ -208,15 +208,16 @@ type Overload struct { // FunctionProperties are the properties of this overload. FunctionProperties - // IsUDF is set to true when this is a user-defined function overload. - // Note: Body can be empty string even IsUDF is true. + // IsUDF is set to true when this is a user-defined function overload built + // using CREATE FUNCTION. Note: Body can be empty even if IsUDF is true. IsUDF bool + // Body is the SQL string body of a function. It can be set even if IsUDF is + // false if a builtin function is defined using a SQL string. + Body string // UDFContainsOnlySignature is only set to true for Overload signatures cached // in a Schema descriptor, which means that the full UDF descriptor need to be // fetched to get more info, e.g. function Body. UDFContainsOnlySignature bool - // Body is the SQL string body of a user-defined function. - Body string // ReturnSet is set to true when a user-defined function is defined to return // a set of values. ReturnSet bool @@ -270,6 +271,12 @@ func (b Overload) IsGenerator() bool { return b.Generator != nil || b.GeneratorWithExprs != nil } +// HasSQLBody returns true if the function was defined using a SQL string body. +// This is the case for user-defined functions and some builtins. +func (b Overload) HasSQLBody() bool { + return b.IsUDF || b.Body != "" +} + // Signature returns a human-readable signature. // If simplify is bool, tuple-returning functions with just // 1 tuple element unwrap the return type in the signature. diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index f0439c5e71dc..ae77ef4a33bb 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -3123,6 +3123,9 @@ func getMostSignificantOverload( for _, idx := range filter { o := qualifiedOverloads[idx] if o.IsUDF { + // This check is only concerned with user-defined functions, not with + // builtin functions defined with a SQL string body. For this reason we + // check o.IsUDF instead of o.HasSQLBody(). udfFound = true } if seenSchema != "" && o.Schema != seenSchema { diff --git a/pkg/sql/sem/tree/udf.go b/pkg/sql/sem/tree/udf.go index 6416f5b8a0a1..33add8587ee1 100644 --- a/pkg/sql/sem/tree/udf.go +++ b/pkg/sql/sem/tree/udf.go @@ -516,7 +516,7 @@ type UDFDisallowanceVisitor struct { // VisitPre implements the Visitor interface. func (v *UDFDisallowanceVisitor) VisitPre(expr Expr) (recurse bool, newExpr Expr) { - if funcExpr, ok := expr.(*FuncExpr); ok && funcExpr.ResolvedOverload().IsUDF { + if funcExpr, ok := expr.(*FuncExpr); ok && funcExpr.ResolvedOverload().HasSQLBody() { v.FoundUDF = true return false, expr }