sql: implement function OID reference

[chengxiong-ruan]

This commit implements [FUNCTION xxxx] OID references syntax of functions. With this change, functions can be called with the new OID numerical representation. For example SELECT [FUNCTION 123]('helloworld'). The intention of this syntax is only for internal serialization of references to UDFs. But it's general enough for builtin functions as well.

A new implementation of the ResolvableFunctionReference interface, OIDFunctionReference is introduced for function resolution purpose for the new syntax
The ResolveFunctionByOID method is also refactored to return a qualified name.

Fixes: #83231

Release note: None

[cockroach-teamcity]

This change is 

[DrewKimball]

Reviewed 26 of 27 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @mgartner, and @shermanCRL)

---

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 41 at r1 (raw file):

query I
SELECT [FUNCTION $fn_oid]()

Can you add a case where we attempt to use this syntax from a different database? And maybe also one or two where the function has been altered or dropped.

[chengxiong-ruan]

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 41 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Can you add a case where we attempt to use this syntax from a different database? And maybe also one or two where the function has been altered or dropped.

Good call! Yeah, we need to disallow cross-db references event referencing by id. Fixed that and added test case for it.
Also add cases for renaming a function, altering function's parent schema and dropping a function

[DrewKimball]

I'd defer to @mgartner for approval, but

Reviewed 9 of 9 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @mgartner, and @shermanCRL)

---

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 41 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Good call! Yeah, we need to disallow cross-db references event referencing by id. Fixed that and added test case for it.
Also add cases for renaming a function, altering function's parent schema and dropping a function

Nice fix! See also #96674 - we have similar problems with UDTs and tables referenced by OID.

[mgartner]

Nice! I just left a couple questions and suggestions.

Reviewed 16 of 27 files at r1, 9 of 9 files at r2, 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @DrewKimball, and @shermanCRL)

---

pkg/sql/sem/tree/function_name.go line 162 at r4 (raw file):

func (*OIDFunctionReference) functionReference()       {}

type OIDFunctionReference struct {

nit: Maybe call this just FunctionOID.

---

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 41 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Nice fix! See also #96674 - we have similar problems with UDTs and tables referenced by OID.

Why do we need to disallow cross-database references with this syntax? Are OIDs of functions only unique within a database? That might cause problems when joining two tables from different databases that have computed columns/partial index predictes/etc. that reference UDFs.

---

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 2 at r4 (raw file):

query T
SELECT [FUNCTION 1074]('hello,world', ',')

Can you add a comment explaining what function this is? Is it a builtin?

---

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 106 at r4 (raw file):

statement error pq: cross database function references are not supported
SELECT [FUNCTION $fn_oid]('hello world')

Can you add a test with a function OID reference within a UDF? That should error for now.

[chengxiong-ruan]

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 41 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Why do we need to disallow cross-database references with this syntax? Are OIDs of functions only unique within a database? That might cause problems when joining two tables from different databases that have computed columns/partial index predictes/etc. that reference UDFs.

Thanks for pointing this out, I thought cross-db references are not encouraged in CRDB for some reason. I'll relax this constrain for OID reference since there is a need.

[mgartner]

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 41 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Thanks for pointing this out, I thought cross-db references are not encouraged in CRDB for some reason. I'll relax this constrain for OID reference since there is a need.

I think you're correct about them being discouraged - it's not supported by PG by design, AFAIK. I don't know why we support it - but we do need to make sure that we either produce expected results with cross-db references or have nice errors messages for behaviors we don't support.

[chengxiong-ruan]

pkg/sql/sem/tree/function_name.go line 162 at r4 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: Maybe call this just FunctionOID.

done, good idea

[chengxiong-ruan]

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 2 at r4 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Can you add a comment explaining what function this is? Is it a builtin?

yes, it's a builtin, comment added.

[chengxiong-ruan]

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 106 at r4 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Can you add a test with a function OID reference within a UDF? That should error for now.

thanks for pointing this out, actually fixed a bug where resolver can be nil.

[chengxiong-ruan]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @mgartner, and @shermanCRL)

---

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 41 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I think you're correct about them being discouraged - it's not supported by PG by design, AFAIK. I don't know why we support it - but we do need to make sure that we either produce expected results with cross-db references or have nice errors messages for behaviors we don't support.

Done.

[mgartner]

I don't think the behavior of cross-database references has been solved in this PR - that's fine, but let's create an issue to track that work at the least.

Reviewed 11 of 17 files at r5.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan and @DrewKimball)

---

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 41 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Done.

I'm confused - I don't see tests for these. Did you mean to add them?

---

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 100 at r5 (raw file):

SELECT oid FROM pg_catalog.pg_proc WHERE proname = 'f_in_udf'

statement error pq: function 100110 not found: function undefined

You need a regex like \d+ instead of 100110 to prevent this test from flaking, right? Maybe a leave a TODO above this test here too - since we want to allow this in the future, right?

---

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 104 at r5 (raw file):

statement ok
CREATE FUNCTION f_using_udf() RETURNS INT LANGUAGE SQL AS $$ SELECT [FUNCTION 814]('abc') $$;

nit: add a comment explaining that this is allowed because it is a built-in.

[chengxiong-ruan]

pkg/sql/logictest/testdata/logic_test/udf_oid_ref line 41 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I'm confused - I don't see tests for these. Did you mean to add them?

oh weird, I thought I had a case to test to make sure that's allowed, I'll add one

[chengxiong-ruan]

TFTRs!
bors r+

[craig]

Canceled.

[chengxiong-ruan]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)