Merge #131643

131643: sql: correctly format routine names r=DrewKimball a=DrewKimball

#### 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.

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

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
 	}

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

pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index

@@ -547,15 +547,15 @@ 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]
  ├── crdb_internal_origin_id int4 [hidden] [system]
  ├── 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

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
 │

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

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

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

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)
 	})
 

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

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.

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(')')
 }