Skip to content

Commit

Permalink
tree: distinguish UDFs and builtins that use a SQL string body
Browse files Browse the repository at this point in the history
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 cockroachdb#95214

Release note: None
  • Loading branch information
DrewKimball committed Feb 28, 2023
1 parent 5f34f44 commit 3236259
Show file tree
Hide file tree
Showing 9 changed files with 22 additions and 33 deletions.
5 changes: 2 additions & 3 deletions pkg/ccl/changefeedccl/cdceval/func_resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/drop_function.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/opt/optbuilder/scalar.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/schemachanger/scbuild/builder_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
6 changes: 0 additions & 6 deletions pkg/sql/sem/builtins/builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)`,
Expand All @@ -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,
Expand All @@ -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`,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down
18 changes: 0 additions & 18 deletions pkg/sql/sem/builtins/pg_builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -618,7 +615,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.",
Expand All @@ -636,7 +632,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
Expand All @@ -657,7 +652,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.",
Expand All @@ -671,7 +665,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`,
Expand All @@ -680,7 +673,6 @@ var pgBuiltins = map[string]builtinDefinition{
CalledOnNullInput: true,
},
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
Expand Down Expand Up @@ -852,7 +844,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,
Expand All @@ -872,7 +863,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)`,
Expand Down Expand Up @@ -931,7 +921,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:
Expand Down Expand Up @@ -962,7 +951,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
Expand All @@ -977,7 +965,6 @@ var pgBuiltins = map[string]builtinDefinition{
CalledOnNullInput: true,
},
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
Expand Down Expand Up @@ -1012,7 +999,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
Expand Down Expand Up @@ -1075,7 +1061,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)
Expand All @@ -1093,7 +1078,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)
Expand All @@ -1115,7 +1099,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)
Expand Down Expand Up @@ -1912,7 +1895,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},
Expand Down
15 changes: 11 additions & 4 deletions pkg/sql/sem/tree/overload.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions pkg/sql/sem/tree/type_check.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/sem/tree/udf.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down

0 comments on commit 3236259

Please sign in to comment.