sql: `pg_proc` and `SHOW CREATE FUNCTION` #85656

chengxiong-ruan · 2022-08-05T05:28:59Z

There 3 commits:
(1) populate pg_proc with UDF
(2) create crdb_internal.create_function_statements virtual table
(3) implement SHOW CREATE FUNCTION
(4) add ::regproc support for UDFs

cockroach-teamcity · 2022-08-05T05:29:11Z

This change is

chengxiong-ruan · 2022-08-05T16:29:55Z

pkg/sql/catalog/typedesc/type_desc.go

@@ -796,7 +796,6 @@ func EnsureTypeIsHydrated(
 				return err
 			}
 		}
-		return nil


I think this was a bug that EnsureTypeIsHydrated didn't hydrate table implicit types which is actually a tuple. Let me know if this makes sense..

if we make this change, let's also add a test that shows why it's needed.

ah yeah, good call, I'll add one for it.

rafiss

i just had some small nits. nice work!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @chengxiong-ruan)

pkg/sql/crdb_internal.go line 2581 at r2 (raw file):

var crdbInternalCreateFunctionStmtsTable = virtualSchemaTable{
	comment: "CREATE statements for all ",

nit: incomplete sentence

pkg/sql/catalog/funcdesc/func_desc.go line 538 at r2 (raw file):

// ToTreeNode implements the FunctionDescriptor interface.
func (desc *immutable) ToTreeNode() (ret *tree.CreateFunction, err error) {

nit: i think a name like func (desc *immutable) CreateExpr or func (desc *immutable) ToCreateExpr might be a bit more natural

pkg/sql/catalog/funcdesc/func_desc.go line 570 at r2 (raw file):

}

func (desc *immutable) getTreeNodeLang() tree.FunctionLanguage {

i think the TreeNode part of the name is slightly redundant, since we can already see that in the return type

pkg/sql/catalog/funcdesc/func_desc.go line 578 at r2 (raw file):

}

func (desc *immutable) getTreeNodeVolatility() tree.FunctionVolatility {

ditto

pkg/sql/catalog/funcdesc/func_desc.go line 590 at r2 (raw file):

}

func (desc *immutable) getTreeNodeNullInputBehavior() tree.FunctionNullInputBehavior {

ditto

chengxiong-ruan · 2022-08-05T17:18:08Z

@rafiss sorry I didn't see your reviews and I added an extra small commit to add ::regproc support for UDFs. I'll address your earlier comments. Thanks.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @chengxiong-ruan)

pkg/sql/sem/eval/parse_doid.go line 73 at r6 (raw file):

			name.Parts[i] = substrs[len(substrs)-1-i]
		}
		funcDef, err := ctx.Planner.ResolveFunction(ctx.Ctx(), &name, &ctx.SessionData().SearchPath)

you will also need to update performIntToOidCast inside eval/cast.go (and add tests please)

chengxiong-ruan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rafiss)

pkg/sql/catalog/funcdesc/func_desc.go line 538 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: i think a name like func (desc *immutable) CreateExpr or func (desc *immutable) ToCreateExpr might be a bit more natural

yeah, good point, fixed

pkg/sql/catalog/funcdesc/func_desc.go line 570 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think the TreeNode part of the name is slightly redundant, since we can already see that in the return type

yeah, but I want to distinguish from the function return the catpb one. Inspired by your another comment on naming, I replaced TreeNode with CreateExpr. Hope that makes sense.

pkg/sql/catalog/funcdesc/func_desc.go line 578 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ditto

fixed with same renaming

pkg/sql/catalog/funcdesc/func_desc.go line 590 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ditto

fixed with same renaming

pkg/sql/sem/eval/parse_doid.go line 73 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

you will also need to update performIntToOidCast inside eval/cast.go (and add tests please)

fixed that, and also added a test to make sure function using table implicit types are hydrated properly.

ajwerner

LGTM

chengxiong-ruan · 2022-08-08T13:16:47Z

TFTR!
bors r+

craig · 2022-08-08T13:16:50Z

👎 Rejected by code reviews

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @rafiss)

pkg/sql/sem/eval/cast.go line 943 at r9 (raw file):

	case oid.T_regproc, oid.T_regprocedure:
		name, _, err := res.ResolveFunctionByOID(ctx, oid.Oid(v))

i think this is going to have the same issue that was just fixed in #85086 -- it's not always safe to ignore this error. (specifically, in the case where the error causes the txn to abort.) so i think you may want something similar where ResolveFunctionByOID also returns a "safe to ignore error" flag. OR refactor things so that this code never needs to ignore the error.

chengxiong-ruan · 2022-08-08T15:02:18Z

pkg/sql/sem/eval/cast.go line 943 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think this is going to have the same issue that was just fixed in #85086 -- it's not always safe to ignore this error. (specifically, in the case where the error causes the txn to abort.) so i think you may want something similar where ResolveFunctionByOID also returns a "safe to ignore error" flag. OR refactor things so that this code never needs to ignore the error.

yeah, good point, I added a ErrFunctionUndefined in another pr which bases on this pr. I'll fix this separately outside of this PR, does that sound good?

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @rafiss)

pkg/sql/sem/eval/cast.go line 943 at r9 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

yeah, good point, I added a ErrFunctionUndefined in another pr which bases on this pr. I'll fix this separately outside of this PR, does that sound good?

sure that sounds good

chengxiong-ruan · 2022-08-08T15:07:53Z

TFTR!
bors r+

craig · 2022-08-08T15:46:46Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

chengxiong-ruan · 2022-08-08T16:02:35Z

need to rebased and update udf logic test with increased ids since there were new objects added.
bors r-

craig · 2022-08-08T16:03:11Z

Canceled.

chengxiong-ruan · 2022-08-08T17:37:17Z

only one flaky test failed, thanks again for the reviews!
bors r+

chengxiong-ruan · 2022-08-08T19:50:49Z

canceling to wait until Marcus' #85674 is merged, there is conflict.
bors r-

craig · 2022-08-08T19:50:51Z

Canceled.

This commit extends the pg_proc virtual table with user-defined functions. Release note (sql change): Previously pg_proc table was only populated with builtin functions. With createing UDFs supported, pg_proc table is extended to be populated with UDFs data as well.

Release note (sql change): a new virtual table crdb_internal.create_function_statements is added, so that users can use to query create statements of user defined functions, as well as parent db and schema ids.

Release note (sql change): This commit adds support for the SHOW CREATE FUNCTION statement. If given function name is qualified, the explicit schema will be searched. If function name is not qualified, the schemas on search path are searched and functions from the most significant schema are returned.

Release note (sql change): Previously, the `::regproc` casting only supported builtin functions. Now it's extened to support user-defined functions as well.

chengxiong-ruan · 2022-08-09T00:21:35Z

TFTR again!
bors r+

craig · 2022-08-09T01:48:01Z

Build succeeded:

Bazel Essential CI (Cockroach)

abarganier · 2022-08-10T16:06:34Z

Posting here for visibility - I think we have a nil pointer issue with the crdb_internal table when querying across databases.

Issue here: #85897

This addresses a comment from the review of cockroachdb#85656. Release note: None

85680: amazon,externalconn: add s3 support to External Connections r=adityamaru a=adityamaru This change registers s3 as a URI that can be represented as an External Connection. Most notably we take a page from the CDC book and switch the s3 parse function to check for invalid parameters, and configurations. This allows us to catch certain misconfiguration at the time we create the external connection. Informs: #84753 Release note (sql change): Users can now `CREATE EXTERNAL CONNECTION` to represent an s3 URI. 85849: sql/catalog/tabledesc: relaxed validation for virtual col in SUFFIX cols r=Xiang-Gu a=Xiang-Gu One of the validation rule says that "we don't allow virtual columns to be in the SUFFIX columns of a secondary index", except for one case: `ALTER PRIMARY KYE USING HASH`, where the implicitly created virtual, shard column, will need to appear in the SUFFIX columns of the implicitly created unique, secondary index of the old PK key columns ( which btw is a CRDB unique feature). The validation has logic to exempt us from this special case but it's written specifically to the legacy schema changer. Namely, it uses the canonical `PrimaryKeySwap` mutation type as the signal but we don't have that in the declarative schema changer. This PR addresses this issue and allows the validation logic to also exempt the exact same case but in the declarative schema changer. Release note: None 85862: ui: new insights table component r=maryliag a=maryliag Creation of the base insights table, that can be used on both schema insights and statement details page. This commit only introduce the table, with the different types, but still needs the proper cell formating for each type. Part of #83783 Release note: None 85865: rowexec: use the sorter's own context r=yuzefovich a=yuzefovich Release note: none 85866: streamingccl, rowexec: correctly mark eventStream as "streaming" r=yuzefovich a=yuzefovich This commit fixes an incomplete solution of 9bb5d30 which attempted to mark `eventStream` generator builtin as "streaming" so that the columnarizer on top of the `projectSetProcessor` would not buffer anything. As found by Steven, the solution in that commit was incomplete since the generators array is not populated at the time where `MustBeStreaming` check is performed. This commit fixes that oversight by using a different way of propagating the property - via the function properties. Release note: None 85885: kvserver: add queue size metric to RaftTransport r=tbg a=pavelkalinnikov Currently, there is a `RaftEnqueuePending` metric in `StoreMetrics` which exposes the `RaftTransport` outgoing queue size. However, this is a per-`Store` metric, so it duplicates the same variable across all the stores. The metric should be tracked on a per-node/transport basis. This PR introduces a per-transport `SendQueueSize` metric to replace the old `RaftEnqueuePending`. It also does the necessary plumbing to include it to the exported metrics, since it's the first metric in `RaftTransport`. <img width="1350" alt="Screenshot 2022-08-10 at 15 37 47" src="https://user-images.githubusercontent.com/3757441/183929666-0f6523d7-5877-4f2f-8460-4f013f87966d.png"> Touches #83917 Release note: None 85906: eval: don't always ignore error from ResolveFunctionByOID r=chengxiong-ruan a=rafiss This addresses a comment from the review of #85656. Release note: None 85914: systemschema_test: fix timestamp stripping regex r=postamar a=postamar The test in this package was flaky because hlc.Timestamp values inside descriptors (modification time, for instance) with a logical component would be improperly redacted. This commit fixes this. Fixes #85799. Release note: None Co-authored-by: Aditya Maru <adityamaru@gmail.com> Co-authored-by: Xiang Gu <xiang@cockroachlabs.com> Co-authored-by: Marylia Gutierrez <marylia@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com> Co-authored-by: Marius Posta <marius@cockroachlabs.com>

chengxiong-ruan force-pushed the udf-pgproc-show-create-function branch 3 times, most recently from 2b8b413 to cc450e1 Compare August 5, 2022 16:26

chengxiong-ruan marked this pull request as ready for review August 5, 2022 16:27

chengxiong-ruan requested a review from a team as a code owner August 5, 2022 16:27

chengxiong-ruan requested review from a team August 5, 2022 16:27

chengxiong-ruan requested review from a team as code owners August 5, 2022 16:27

chengxiong-ruan requested a review from ajwerner August 5, 2022 16:27

chengxiong-ruan commented Aug 5, 2022

View reviewed changes

rafiss requested changes Aug 5, 2022

View reviewed changes

chengxiong-ruan force-pushed the udf-pgproc-show-create-function branch from cc450e1 to d076135 Compare August 5, 2022 17:16

rafiss requested changes Aug 5, 2022

View reviewed changes

chengxiong-ruan force-pushed the udf-pgproc-show-create-function branch from d076135 to 2399573 Compare August 5, 2022 19:53

chengxiong-ruan requested a review from a team as a code owner August 5, 2022 19:53

chengxiong-ruan commented Aug 5, 2022

View reviewed changes

chengxiong-ruan force-pushed the udf-pgproc-show-create-function branch 2 times, most recently from 9e6be59 to 3812048 Compare August 5, 2022 20:50

shermanCRL removed the request for review from a team August 6, 2022 14:59

chengxiong-ruan mentioned this pull request Aug 8, 2022

sql: implement drop function in legacy schema changer #85718

Merged

ajwerner approved these changes Aug 8, 2022

View reviewed changes

rafiss requested changes Aug 8, 2022

View reviewed changes

rafiss approved these changes Aug 8, 2022

View reviewed changes

chengxiong-ruan force-pushed the udf-pgproc-show-create-function branch from 3812048 to 08b6f60 Compare August 8, 2022 16:13

chengxiong-ruan added 3 commits August 8, 2022 18:53

sql: add crdb_internal.create_function_statements virtual table

9a50c6b

Release note (sql change): a new virtual table crdb_internal.create_function_statements is added, so that users can use to query create statements of user defined functions, as well as parent db and schema ids.

chengxiong-ruan force-pushed the udf-pgproc-show-create-function branch from 08b6f60 to 9f37f9e Compare August 8, 2022 22:53

sql: add regproc support for UDF

4e4e5d9

Release note (sql change): Previously, the `::regproc` casting only supported builtin functions. Now it's extened to support user-defined functions as well.

chengxiong-ruan force-pushed the udf-pgproc-show-create-function branch from 9f37f9e to 4e4e5d9 Compare August 8, 2022 23:03

craig bot merged commit 9a0a233 into cockroachdb:master Aug 9, 2022

abarganier mentioned this pull request Aug 10, 2022

crdb_internal.create_function_statements virtual table has a nil pointer bug #85897

Closed

rafiss added a commit to rafiss/cockroach that referenced this pull request Aug 10, 2022

eval: don't always ignore error from ResolveFunctionByOID

655ae28

This addresses a comment from the review of cockroachdb#85656. Release note: None

rafiss mentioned this pull request Aug 10, 2022

eval: don't always ignore error from ResolveFunctionByOID #85906

Merged

chengxiong-ruan mentioned this pull request Aug 11, 2022

UDF: user defined function should appear in pg_catalog.pg_proc table #83239

Closed

rafiss mentioned this pull request Sep 27, 2022

cli: \df command to list or show all user defined functions #88597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: `pg_proc` and `SHOW CREATE FUNCTION` #85656

sql: `pg_proc` and `SHOW CREATE FUNCTION` #85656

chengxiong-ruan commented Aug 5, 2022 •

edited

cockroach-teamcity commented Aug 5, 2022

chengxiong-ruan Aug 5, 2022 •

edited

rafiss Aug 5, 2022

chengxiong-ruan Aug 5, 2022

rafiss left a comment

chengxiong-ruan commented Aug 5, 2022

rafiss left a comment

chengxiong-ruan left a comment

ajwerner left a comment

chengxiong-ruan commented Aug 8, 2022

craig bot commented Aug 8, 2022

rafiss left a comment

chengxiong-ruan commented Aug 8, 2022

rafiss left a comment

chengxiong-ruan commented Aug 8, 2022

craig bot commented Aug 8, 2022

chengxiong-ruan commented Aug 8, 2022

craig bot commented Aug 8, 2022

chengxiong-ruan commented Aug 8, 2022

chengxiong-ruan commented Aug 8, 2022

craig bot commented Aug 8, 2022

chengxiong-ruan commented Aug 9, 2022

craig bot commented Aug 9, 2022

abarganier commented Aug 10, 2022

sql: pg_proc and SHOW CREATE FUNCTION #85656

sql: pg_proc and SHOW CREATE FUNCTION #85656

Conversation

chengxiong-ruan commented Aug 5, 2022 • edited

cockroach-teamcity commented Aug 5, 2022

chengxiong-ruan Aug 5, 2022 • edited

Choose a reason for hiding this comment

rafiss Aug 5, 2022

Choose a reason for hiding this comment

chengxiong-ruan Aug 5, 2022

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

chengxiong-ruan commented Aug 5, 2022

rafiss left a comment

Choose a reason for hiding this comment

chengxiong-ruan left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

chengxiong-ruan commented Aug 8, 2022

craig bot commented Aug 8, 2022

rafiss left a comment

Choose a reason for hiding this comment

chengxiong-ruan commented Aug 8, 2022

rafiss left a comment

Choose a reason for hiding this comment

chengxiong-ruan commented Aug 8, 2022

craig bot commented Aug 8, 2022

chengxiong-ruan commented Aug 8, 2022

craig bot commented Aug 8, 2022

chengxiong-ruan commented Aug 8, 2022

chengxiong-ruan commented Aug 8, 2022

craig bot commented Aug 8, 2022

chengxiong-ruan commented Aug 9, 2022

craig bot commented Aug 9, 2022

abarganier commented Aug 10, 2022

sql: `pg_proc` and `SHOW CREATE FUNCTION` #85656

sql: `pg_proc` and `SHOW CREATE FUNCTION` #85656

chengxiong-ruan commented Aug 5, 2022 •

edited

chengxiong-ruan Aug 5, 2022 •

edited