From a6bcc1aa16f12fd65565d17857de89afb403cad9 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Mon, 30 Sep 2024 21:50:34 -0600 Subject: [PATCH] sql: correctly format routine names Previously, routine names were formatted as strings, and with the `EncBareIdentifiers` formatting flag set. This caused routine names with uppercase characters to be printed without quotes. In turn, this caused a failure to resolve the correct routine in cases where the invocation was serialized (for example, within the body of another routine). This patch fixes the issue by always formatting a routine name as a `tree.Name` if the routine could be user-defined. In addition, when formatting `tree.FuncExpr` the new `EncBareKeywords` flag is used instead of `EncBareIdentifiers`, so that reserved keywords (which cannot be used for user-defined functions) are left unquoted. This reduces the amount of unnecessary quotes added by the fix. Fixes #131354 Release note (bug fix): Fixed a bug that caused quotes around the name of a routine to be dropped when it was called within another routine. This could prevent the correct routine from being resolved if the nested routine name was case-sensitive. The bug has existed since v24.1, when nested routines were introduced. --- pkg/sql/lexbase/encode.go | 7 ++- .../testdata/logic_test/udf_calling_udf | 47 +++++++++++++++++++ .../execbuilder/testdata/hash_sharded_index | 4 +- .../opt/exec/execbuilder/testdata/subquery | 4 +- .../create_function_calling_function.explain | 2 +- ...te_function_calling_function.explain_shape | 2 +- ...ate_function_calling_function.side_effects | 2 +- pkg/sql/sem/tree/expr.go | 6 ++- pkg/sql/sem/tree/format.go | 4 ++ pkg/sql/sem/tree/function_definition.go | 15 +++--- pkg/sql/sem/tree/routine.go | 3 +- 11 files changed, 80 insertions(+), 16 deletions(-) diff --git a/pkg/sql/lexbase/encode.go b/pkg/sql/lexbase/encode.go index 748e3fac042d..f02ef223fa1e 100644 --- a/pkg/sql/lexbase/encode.go +++ b/pkg/sql/lexbase/encode.go @@ -42,6 +42,10 @@ const ( // without wrapping quotes. EncBareIdentifiers + // EncBareReservedKeywords indicates that reserved keywords will be rendered + // without wrapping quotes. + EncBareReservedKeywords + // EncFirstFreeFlagBit needs to remain unused; it is used as base // bit offset for tree.FmtFlags. EncFirstFreeFlagBit @@ -52,7 +56,8 @@ const ( // contains special characters, or the identifier is a reserved SQL // keyword. func EncodeRestrictedSQLIdent(buf *bytes.Buffer, s string, flags EncodeFlags) { - if flags.HasFlags(EncBareIdentifiers) || (!isReservedKeyword(s) && IsBareIdentifier(s)) { + if flags.HasFlags(EncBareIdentifiers) || + (IsBareIdentifier(s) && (flags.HasFlags(EncBareReservedKeywords) || !isReservedKeyword(s))) { buf.WriteString(s) return } diff --git a/pkg/sql/logictest/testdata/logic_test/udf_calling_udf b/pkg/sql/logictest/testdata/logic_test/udf_calling_udf index d913e446ae0f..ba5f4e20499a 100644 --- a/pkg/sql/logictest/testdata/logic_test/udf_calling_udf +++ b/pkg/sql/logictest/testdata/logic_test/udf_calling_udf @@ -215,3 +215,50 @@ CREATE OR REPLACE FUNCTION f1(a INT = f2()) RETURNS INT LANGUAGE SQL AS $$ SELEC statement ok DROP FUNCTION f2; DROP FUNCTION f1; + +subtest regression_131354 + +# Case-sensitive routine names should be preserved when called from another +# routine. +statement ok +CREATE FUNCTION "fooBAR"() RETURNS INT LANGUAGE SQL AS $$ SELECT 1; $$; + +statement ok +CREATE FUNCTION f131354() RETURNS INT LANGUAGE SQL AS $$ SELECT "fooBAR"(); $$; + +statement ok +CREATE PROCEDURE p131354() LANGUAGE SQL AS $$ SELECT "fooBAR"(); $$; + +query I +SELECT f131354(); +---- +1 + +statement ok +CALL p131354(); + +query T +SELECT create_statement FROM [SHOW CREATE FUNCTION f131354]; +---- +CREATE FUNCTION public.f131354() + RETURNS INT8 + VOLATILE + NOT LEAKPROOF + CALLED ON NULL INPUT + LANGUAGE SQL + SECURITY INVOKER + AS $$ + SELECT public."fooBAR"(); +$$ + +query T +SELECT create_statement FROM [SHOW CREATE PROCEDURE p131354]; +---- +CREATE PROCEDURE public.p131354() + LANGUAGE SQL + SECURITY INVOKER + AS $$ + SELECT public."fooBAR"(); +$$ + +subtest end diff --git a/pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index b/pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index index a91b511d683b..391ebdf934fc 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index +++ b/pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index @@ -547,7 +547,7 @@ query T EXPLAIN (OPT, CATALOG) SELECT * FROM t ---- TABLE t - ├── crdb_internal_a_shard_8 int not null as (mod(fnv32(md5(crdb_internal.datums_to_bytes(a))), 8:::INT8)) stored [hidden] + ├── crdb_internal_a_shard_8 int not null as (mod(fnv32(md5("crdb_internal.datums_to_bytes"(a))), 8:::INT8)) stored [hidden] ├── a int not null ├── crdb_internal_mvcc_timestamp decimal [hidden] [system] ├── tableoid oid [hidden] [system] @@ -555,7 +555,7 @@ TABLE t ├── crdb_internal_origin_timestamp decimal [hidden] [system] ├── CHECK (crdb_internal_a_shard_8 IN (0:::INT8, 1:::INT8, 2:::INT8, 3:::INT8, 4:::INT8, 5:::INT8, 6:::INT8, 7:::INT8)) ├── PRIMARY INDEX t_pkey - │ ├── crdb_internal_a_shard_8 int not null as (mod(fnv32(md5(crdb_internal.datums_to_bytes(a))), 8:::INT8)) stored [hidden] (implicit) + │ ├── crdb_internal_a_shard_8 int not null as (mod(fnv32(md5("crdb_internal.datums_to_bytes"(a))), 8:::INT8)) stored [hidden] (implicit) │ └── a int not null └── UNIQUE WITHOUT INDEX (a) scan t diff --git a/pkg/sql/opt/exec/execbuilder/testdata/subquery b/pkg/sql/opt/exec/execbuilder/testdata/subquery index e668cb036907..42d4f85da529 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/subquery +++ b/pkg/sql/opt/exec/execbuilder/testdata/subquery @@ -516,7 +516,7 @@ vectorized: true • filter │ columns: (k, i) │ estimated row count: 333 (missing stats) -│ filter: CASE WHEN k < 5 THEN COALESCE(exists(k), false) ELSE CAST(NULL AS BOOL) END +│ filter: CASE WHEN k < 5 THEN COALESCE("exists"(k), false) ELSE CAST(NULL AS BOOL) END │ └── • scan columns: (k, i) @@ -537,7 +537,7 @@ vectorized: true · • render │ columns: (k, i, "case") -│ render case: CASE WHEN k < 5 THEN COALESCE(exists(k), false) ELSE CAST(NULL AS BOOL) END +│ render case: CASE WHEN k < 5 THEN COALESCE("exists"(k), false) ELSE CAST(NULL AS BOOL) END │ render k: k │ render i: i │ diff --git a/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.explain b/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.explain index 2d8911bce9fc..e8099973c433 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.explain +++ b/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.explain @@ -30,7 +30,7 @@ Schema change plan for CREATE FUNCTION ‹defaultdb›.‹public›.‹f3›(‹ RETURNS INT8 VOLATILE LANGUAGE SQL - AS $$SELECT public.f2(‹a›) + public.f(‹a›);$$; + AS $$SELECT ‹public›.‹f2›(‹a›) + ‹public›.‹f›(‹a›);$$; ├── StatementPhase │ └── Stage 1 of 1 in StatementPhase │ ├── 9 elements transitioning toward PUBLIC diff --git a/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.explain_shape b/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.explain_shape index 24e9039923c5..eafdc9ac216e 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.explain_shape +++ b/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.explain_shape @@ -30,5 +30,5 @@ Schema change plan for CREATE FUNCTION ‹defaultdb›.‹public›.‹f3›(‹ RETURNS INT8 VOLATILE LANGUAGE SQL - AS $$SELECT public.f2(‹a›) + public.f(‹a›);$$; + AS $$SELECT ‹public›.‹f2›(‹a›) + ‹public›.‹f›(‹a›);$$; └── execute 1 system table mutations transaction diff --git a/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.side_effects b/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.side_effects index ef7f22249cb4..7a383a9e89aa 100644 --- a/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.side_effects +++ b/pkg/sql/schemachanger/testdata/end_to_end/create_function_calling_function/create_function_calling_function.side_effects @@ -42,7 +42,7 @@ write *eventpb.CreateFunction to event log: functionName: defaultdb.public.f3 sql: descriptorId: 112 - statement: "CREATE FUNCTION ‹defaultdb›.‹public›.‹f3›(‹a› ‹notmyworkday›)\n\tRETURNS INT8\n\tVOLATILE\n\tLANGUAGE SQL\n\tAS $$SELECT public.f2(‹a›) + public.f(‹a›);$$" + statement: "CREATE FUNCTION ‹defaultdb›.‹public›.‹f3›(‹a› ‹notmyworkday›)\n\tRETURNS INT8\n\tVOLATILE\n\tLANGUAGE SQL\n\tAS $$SELECT ‹public›.‹f2›(‹a›) + ‹public›.‹f›(‹a›);$$" tag: CREATE FUNCTION user: root ## StatementPhase stage 1 of 1 with 12 MutationType ops diff --git a/pkg/sql/sem/tree/expr.go b/pkg/sql/sem/tree/expr.go index a6718c216d3e..3c6e681d43de 100644 --- a/pkg/sql/sem/tree/expr.go +++ b/pkg/sql/sem/tree/expr.go @@ -1377,7 +1377,11 @@ func (node *FuncExpr) Format(ctx *FmtCtx) { // they are resolved. We conservatively redact function names if requested. // TODO(111385): Investigate ways to identify built-in functions before // type-checking. - ctx.WithFlags(ctx.flags|FmtBareIdentifiers, func() { + // + // Instruct the pretty-printer not to wrap reserved keywords in quotes. Only + // builtin functions can have reserved keywords as names, and it is not + // necessary (or desirable) to quote them. + ctx.WithFlags(ctx.flags|FmtBareReservedKeywords, func() { ctx.FormatNode(&node.Func) }) diff --git a/pkg/sql/sem/tree/format.go b/pkg/sql/sem/tree/format.go index c3b6cd35f391..b1691c258628 100644 --- a/pkg/sql/sem/tree/format.go +++ b/pkg/sql/sem/tree/format.go @@ -58,6 +58,10 @@ const ( // identifiers without wrapping quotes in any case. FmtBareIdentifiers = FmtFlags(lexbase.EncBareIdentifiers) + // FmtBareReservedKeywords instructs the pretty-printer to print + // reserved keywords without wrapping quotes. + FmtBareReservedKeywords = FmtFlags(lexbase.EncBareReservedKeywords) + // FmtShowPasswords instructs the pretty-printer to not suppress passwords. // If not set, passwords are replaced by *****. FmtShowPasswords = FmtFlags(lexbase.EncFirstFreeFlagBit) << iota diff --git a/pkg/sql/sem/tree/function_definition.go b/pkg/sql/sem/tree/function_definition.go index dbccd794cf3f..4ad24eb39596 100644 --- a/pkg/sql/sem/tree/function_definition.go +++ b/pkg/sql/sem/tree/function_definition.go @@ -209,19 +209,22 @@ func (fd *FunctionDefinition) Format(ctx *FmtCtx) { func (fd *FunctionDefinition) String() string { return AsString(fd) } // Format implements the NodeFormatter interface. -// ResolvedFunctionDefinitions should always be builtin functions, so we do not -// need to anonymize them, even if the flag is set. +// +// ResolvedFunctionDefinitions can be builtin or user-defined, so we must +// respect formatting flags. func (fd *ResolvedFunctionDefinition) Format(ctx *FmtCtx) { // This is necessary when deserializing function expressions for SHOW CREATE // statements. When deserializing a function expression with function OID // references, it's guaranteed that there'll be always one overload resolved. - // There is no need to show prefix for builtin functions since we don't - // serialize them. + // There is no need to show prefix or use formatting flags for builtin + // functions since we don't serialize them. if len(fd.Overloads) == 1 && catid.IsOIDUserDefined(fd.Overloads[0].Oid) { - ctx.WriteString(fd.Overloads[0].Schema) + ctx.FormatName(fd.Overloads[0].Schema) ctx.WriteString(".") + ctx.FormatName(fd.Name) + } else { + ctx.WriteString(fd.Name) } - ctx.WriteString(fd.Name) } // String implements the Stringer interface. diff --git a/pkg/sql/sem/tree/routine.go b/pkg/sql/sem/tree/routine.go index 4238f3d08f94..883fbba6cf69 100644 --- a/pkg/sql/sem/tree/routine.go +++ b/pkg/sql/sem/tree/routine.go @@ -185,7 +185,8 @@ func (node *RoutineExpr) ResolvedType() *types.T { // Format is part of the Expr interface. func (node *RoutineExpr) Format(ctx *FmtCtx) { - ctx.Printf("%s(", node.Name) + ctx.FormatName(node.Name) + ctx.WriteByte('(') ctx.FormatNode(&node.Args) ctx.WriteByte(')') }