Merge cockroachdb#88896 cockroachdb#89007

88896: sql: `has_function_privilege` and `regprocedure` casting to consider arg types r=chengxiong-ruan a=chengxiong-ruan backport addresses cockroachdb#83237 Release note (sql change): Previously, `has_function_privlege` builtin function and `::regprocedure` casting only consider function names and argument types are ignored. An umbiguous error was returned if a function name matched more than one overloads. This commit utilizes argument types to narrow down an overload to avoid ambiguity. Also `::regproc` casting is modified to consider whole string as function name. This is to match postgres behavior. Release justification: GA blocker. 89007: sql: clean up some udf TODOs r=chengxiong-ruan a=chengxiong-ruan Clean up some udf TODOs which are actually done or a not-to-do. Also resolve two TODOs in tests. Release note: None Release justification: low risk comment and test only change. Co-authored-by: Chengxiong Ruan <[email protected]>
faizaanmadhani · Sep 29, 2022 · 6850a6e · 6850a6e
3 parents 610ad9a + b507b73 + a70d717
commit 6850a6e
Show file tree

Hide file tree

Showing 14 changed files with 268 additions and 93 deletions.
diff --git a/pkg/sql/function_resolver_test.go b/pkg/sql/function_resolver_test.go
@@ -176,6 +176,7 @@ CREATE FUNCTION sc1.lower() RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ SELECT 3 $$
 		testName       string
 		funName        tree.UnresolvedName
 		searchPath     []string
+		expectUDF      []bool
 		expectedBody   []string
 		expectedSchema []string
 		expectedErr    string
@@ -206,24 +207,40 @@ CREATE FUNCTION sc1.lower() RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ SELECT 3 $$
 			testName:       "function with explicit schema skip first schema in path",
 			funName:        tree.UnresolvedName{NumParts: 2, Parts: tree.NameParts{"f", "sc2", "", ""}},
 			searchPath:     []string{"sc1", "sc2"},
+			expectUDF:      []bool{true},
 			expectedBody:   []string{"SELECT 2;"},
 			expectedSchema: []string{"sc2"},
 		},
 		{
 			testName:       "use functions from search path",
 			funName:        tree.UnresolvedName{NumParts: 1, Parts: tree.NameParts{"f", "", "", ""}},
 			searchPath:     []string{"sc1", "sc2"},
+			expectUDF:      []bool{true, true},
 			expectedBody:   []string{"SELECT 1;", "SELECT 2;"},
 			expectedSchema: []string{"sc1", "sc2"},
 		},
+		{
+			testName:       "builtin function schema is respected",
+			funName:        tree.UnresolvedName{NumParts: 1, Parts: tree.NameParts{"lower", "", "", ""}},
+			searchPath:     []string{"sc1", "sc2"},
+			expectUDF:      []bool{false, true},
+			expectedBody:   []string{"", "SELECT 3;"},
+			expectedSchema: []string{"pg_catalog", "sc1"},
+		},
+		{
+			testName:       "explicit builtin function schema",
+			funName:        tree.UnresolvedName{NumParts: 2, Parts: tree.NameParts{"lower", "pg_catalog", "", ""}},
+			searchPath:     []string{"sc1", "sc2"},
+			expectUDF:      []bool{false},
+			expectedBody:   []string{""},
+			expectedSchema: []string{"pg_catalog"},
+		},
 		{
 			testName:    "unsupported builtin function",
 			funName:     tree.UnresolvedName{NumParts: 1, Parts: tree.NameParts{"querytree", "", "", ""}},
 			searchPath:  []string{"sc1", "sc2"},
 			expectedErr: `querytree\(\): unimplemented: this function is not yet supported`,
 		},
-		// TODO(Chengxiong): add test case for builtin function names when builtin
-		// OIDs are changed to fixed IDs.
 	}
 
 	var sessionData sessiondatapb.SessionData
@@ -246,25 +263,31 @@ CREATE FUNCTION sc1.lower() RETURNS INT IMMUTABLE LANGUAGE SQL AS $$ SELECT 3 $$
 		funcResolver := planner.(tree.FunctionReferenceResolver)
 
 		for _, tc := range testCases {
-			path := sessiondata.MakeSearchPath(tc.searchPath)
-			funcDef, err := funcResolver.ResolveFunction(ctx, &tc.funName, &path)
-			if tc.expectedErr != "" {
-				require.Regexp(t, tc.expectedErr, err.Error())
-				continue
-			}
-			require.NoError(t, err)
-
-			require.Equal(t, len(tc.expectedBody), len(funcDef.Overloads))
-			bodies := make([]string, len(funcDef.Overloads))
-			schemas := make([]string, len(funcDef.Overloads))
-			for i, o := range funcDef.Overloads {
-				_, overload, err := funcResolver.ResolveFunctionByOID(ctx, o.Oid)
+			t.Run(tc.testName, func(t *testing.T) {
+				path := sessiondata.MakeSearchPath(tc.searchPath)
+				funcDef, err := funcResolver.ResolveFunction(ctx, &tc.funName, &path)
+				if tc.expectedErr != "" {
+					require.Regexp(t, tc.expectedErr, err.Error())
+					return
+				}
 				require.NoError(t, err)
-				bodies[i] = overload.Body
-				schemas[i] = o.Schema
-			}
-			require.Equal(t, tc.expectedBody, bodies)
-			require.Equal(t, tc.expectedSchema, schemas)
+
+				require.Equal(t, len(tc.expectUDF), len(funcDef.Overloads))
+				require.Equal(t, len(tc.expectedBody), len(funcDef.Overloads))
+				bodies := make([]string, len(funcDef.Overloads))
+				schemas := make([]string, len(funcDef.Overloads))
+				isUDF := make([]bool, len(funcDef.Overloads))
+				for i, o := range funcDef.Overloads {
+					_, overload, err := funcResolver.ResolveFunctionByOID(ctx, o.Oid)
+					require.NoError(t, err)
+					bodies[i] = overload.Body
+					schemas[i] = o.Schema
+					isUDF[i] = o.IsUDF
+				}
+				require.Equal(t, tc.expectedBody, bodies)
+				require.Equal(t, tc.expectedSchema, schemas)
+				require.Equal(t, tc.expectUDF, isUDF)
+			})
 		}
 		return nil
 	})
@@ -280,8 +303,6 @@ func TestFuncExprTypeCheck(t *testing.T) {
 	defer s.Stopper().Stop(ctx)
 	tDB := sqlutils.MakeSQLRunner(sqlDB)
 
-	// TODO(Chengxiong): add test cases with builtin function when builtin
-	// function OIDs are changed to fixed IDs.
 	tDB.Exec(t, `
 CREATE SCHEMA sc1;
 CREATE SCHEMA sc2;
@@ -346,6 +367,7 @@ CREATE FUNCTION sc1.lower(a STRING) RETURNS STRING IMMUTABLE LANGUAGE SQL AS $$
 			exprStr:          "lower('HI')",
 			searchPath:       []string{"sc1", "sc2"},
 			expectedFuncBody: "",
+			expectedFuncOID:  827,
 			desiredType:      types.String,
 		},
 		{
@@ -404,11 +426,8 @@ CREATE FUNCTION sc1.lower(a STRING) RETURNS STRING IMMUTABLE LANGUAGE SQL AS $$
 					require.False(t, funcExpr.ResolvedOverload().IsUDF)
 				}
 				require.False(t, funcExpr.ResolvedOverload().UDFContainsOnlySignature)
-				if tc.expectedFuncOID > 0 {
-					require.Equal(t, tc.expectedFuncOID, int(funcExpr.ResolvedOverload().Oid))
-				} else {
-					require.False(t, funcdesc.IsOIDUserDefinedFunc(funcExpr.ResolvedOverload().Oid))
-				}
+				require.Equal(t, tc.expectedFuncOID, int(funcExpr.ResolvedOverload().Oid))
+				require.Equal(t, funcExpr.ResolvedOverload().IsUDF, funcdesc.IsOIDUserDefinedFunc(funcExpr.ResolvedOverload().Oid))
 				require.Equal(t, tc.expectedFuncBody, funcExpr.ResolvedOverload().Body)
 			})
 		}

diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go
@@ -419,7 +419,6 @@ func (n *changeDescriptorBackedPrivilegesNode) startExec(params runParams) error
 					FuncName:                       d.Name, // FIXME
 				})
 			}
-			// TODO(chengxiong): add eventlog for function privilege changes.
 		}
 	}
 

diff --git a/pkg/sql/logictest/testdata/logic_test/pg_builtins b/pkg/sql/logictest/testdata/logic_test/pg_builtins
@@ -707,11 +707,21 @@ bit_in
 query T
 SELECT to_regprocedure('bit_in')
 ----
+NULL
+
+query T
+SELECT to_regprocedure('bit_in(int)')
+----
 bit_in
 
 query T
 SELECT to_regprocedure('version')
 ----
+NULL
+
+query T
+SELECT to_regprocedure('version()')
+----
 version
 
 query T

diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog
@@ -4697,21 +4697,6 @@ query T
 SELECT proname FROM pg_catalog.pg_proc WHERE oid = 0
 ----
 
-let $cur_max_builtin_oid
-SELECT cur_max_builtin_oid FROM [SELECT max(oid) as cur_max_builtin_oid FROM pg_catalog.pg_proc]
-
-## If this test failed (proname is the same, but oid increased), it's likely that a
-## new builtin function is implemented and  it's somewhere in the middle of the
-## existing functions at init time. Though the changes to builtin function OID is
-## generally ok, it's still better if we could move the new implement to end of the
-## list at init time (see all_builtins.go)
-## TODO(chengxiong): consider to have a deterministic list of builtin function oids
-## so that new implementations can just be added to it.
-query TT
-SELECT proname, oid FROM pg_catalog.pg_proc WHERE oid = $cur_max_builtin_oid
-----
-pg_get_functiondef  2035
-
 ## Ensure that unnest works with oid wrapper arrays
 
 query O

diff --git a/pkg/sql/logictest/testdata/logic_test/pgoidtype b/pkg/sql/logictest/testdata/logic_test/pgoidtype
@@ -33,8 +33,17 @@ SELECT pg_typeof('1'::OID), pg_typeof('pg_constraint'::REGCLASS), pg_typeof('pub
 ----
 oid  regclass  regnamespace
 
+statement error pq: unknown function: upper\(string\)\(\): function undefined
+SELECT 'upper(string)'::REGPROC
+
+statement error invalid function signature: upper
+SELECT 'upper'::REGPROCEDURE
+
+statement error pq: function upper\(int\) does not exist: function undefined
+SELECT 'upper(int)'::REGPROCEDURE
+
 query TT
-SELECT pg_typeof('upper'::REGPROC), pg_typeof('upper'::REGPROCEDURE)
+SELECT pg_typeof('upper'::REGPROC), pg_typeof('upper(string)'::REGPROCEDURE)
 ----
 regproc  regprocedure
 
@@ -81,28 +90,31 @@ WHERE relname = 'pg_constraint'
 0  pg_constraint  0  pg_constraint  pg_constraint
 
 query OOOO
-SELECT 'upper'::REGPROC, 'upper'::REGPROCEDURE, 'pg_catalog.upper'::REGPROCEDURE, 'upper'::REGPROC::OID
+SELECT 'upper'::REGPROC, 'upper(string)'::REGPROCEDURE, 'pg_catalog.upper(string)'::REGPROCEDURE, 'upper'::REGPROC::OID
 ----
 upper  upper  upper  829
 
-query error invalid function name
+query error pq: invalid function signature: invalid.more.pg_catalog.upper: at or near ".": syntax error
 SELECT 'invalid.more.pg_catalog.upper'::REGPROCEDURE
 
 query OOO
-SELECT 'upper(int)'::REGPROC, 'upper(int)'::REGPROCEDURE, 'upper(int)'::REGPROC::OID
+SELECT 'upper'::REGPROC, 'upper(string)'::REGPROCEDURE, 'upper'::REGPROC::OID
 ----
 upper  upper  829
 
-query error unknown function: blah\(\)
-SELECT 'blah(ignored, ignored)'::REGPROC, 'blah(ignored, ignored)'::REGPROCEDURE
+query error pq: unknown function: blah\(ignored\)\(\): function undefined
+SELECT 'blah(ignored)'::REGPROC
+
+query error pq: unknown function: blah\(\): function undefined
+SELECT 'blah(int, int)'::REGPROCEDURE
 
-query error unknown function: blah\(\)
+query error pq: invalid name: expected separator .: blah \( ignored , ignored \)
 SELECT ' blah ( ignored , ignored ) '::REGPROC
 
-query error unknown function: blah\(\)
+query error pq: invalid name: expected separator .: blah \(\)
 SELECT 'blah ()'::REGPROC
 
-query error unknown function: blah\(\)
+query error pq: invalid name: expected separator .: blah\( \)
 SELECT 'blah( )'::REGPROC
 
 query error invalid name: expected separator \.: blah\(, \)
@@ -111,15 +123,15 @@ SELECT 'blah(, )'::REGPROC
 query error more than one function named 'sqrt'
 SELECT 'sqrt'::REGPROC
 
-query OOOO
-SELECT 'array_in'::REGPROC, 'array_in(a,b,c)'::REGPROC, 'pg_catalog.array_in'::REGPROC, 'pg_catalog.array_in( a ,b, c )'::REGPROC
+query OO
+SELECT 'array_in'::REGPROC, 'pg_catalog.array_in'::REGPROC
 ----
-array_in  array_in  array_in  array_in
+array_in  array_in
 
-query OOOO
-SELECT 'array_in'::REGPROCEDURE, 'array_in(a,b,c)'::REGPROCEDURE, 'pg_catalog.array_in'::REGPROCEDURE, 'pg_catalog.array_in( a ,b, c )'::REGPROCEDURE
+query OO
+SELECT 'array_in(int)'::REGPROCEDURE, 'pg_catalog.array_in(int)'::REGPROCEDURE
 ----
-array_in  array_in  array_in  array_in
+array_in  array_in
 
 query OO
 SELECT 'public'::REGNAMESPACE, 'public'::REGNAMESPACE::OID
@@ -156,7 +168,7 @@ query error pgcode 42883 unknown function: blah\(\)
 SELECT 'blah'::REGPROC
 
 query error pgcode 42883 unknown function: blah\(\)
-SELECT 'blah'::REGPROCEDURE
+SELECT 'blah()'::REGPROCEDURE
 
 query error pgcode 42704 namespace 'blah' does not exist
 SELECT 'blah'::REGNAMESPACE
@@ -472,7 +484,12 @@ SELECT 'pg_catalog."radians"'::regproc
 radians
 
 query T
-SELECT 'pg_catalog."radians"("float4")'::regproc
+SELECT 'pg_catalog."radians"'::regproc
+----
+radians
+
+query T
+SELECT 'pg_catalog."radians"("float4")'::regprocedure
 ----
 radians
 

diff --git a/pkg/sql/logictest/testdata/logic_test/privilege_builtins b/pkg/sql/logictest/testdata/logic_test/privilege_builtins
@@ -505,14 +505,14 @@ SELECT has_function_privilege((SELECT oid FROM pg_proc LIMIT 1), 'EXECUTE')
 ----
 true
 
-query error pgcode 42883 unknown function: does_not_exist()
+query error pq: has_function_privilege\(\): invalid function signature: does_not_exist
 SELECT has_function_privilege('does_not_exist', 'EXECUTE')
 
 query error pgcode 42883 unknown function: does_not_exist()
 SELECT has_function_privilege('does_not_exist()', 'EXECUTE')
 
 query B
-SELECT has_function_privilege('version', '  EXECUTE      ')
+SELECT has_function_privilege('version()', '  EXECUTE      ')
 ----
 true
 
@@ -524,10 +524,15 @@ true
 query B
 SELECT has_function_privilege('cos(float)', 'EXECUTE WITH GRANT OPTION')
 ----
+false
+
+query B
+SELECT has_function_privilege('cos(float)', 'EXECUTE, EXECUTE WITH GRANT OPTION')
+----
 true
 
 query B
-SELECT has_function_privilege('version'::Name, 'EXECUTE')
+SELECT has_function_privilege('version()'::Name, 'EXECUTE')
 ----
 true
-Original file line number
+Diff line change
@@ Expand Up @@
     					FuncName:                       d.Name, // FIXME
     				})
     			}
-    			// TODO(chengxiong): add eventlog for function privilege changes.
     		}
     	}
@@ Expand Down @@