Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sql: better terminology to determine if a function is a UDF #95214

Closed
chengxiong-ruan opened this issue Jan 13, 2023 · 2 comments · Fixed by #96045
Closed

sql: better terminology to determine if a function is a UDF #95214

chengxiong-ruan opened this issue Jan 13, 2023 · 2 comments · Fixed by #96045
Labels
A-schema-catalog Related to the schema descriptors collection and the catalog API in general. A-sql-routine UDFs and Stored Procedures C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@chengxiong-ruan
Copy link
Contributor

chengxiong-ruan commented Jan 13, 2023 •
edited by cockroach-jira-scripts
Loading

Describe the problem
Currently we use IsUDF to label a function overload as UDF or not. This is a bit confusing when we introduce several builtin functions which has the flag set to true to have the executor execute it as a UDF. One observation is that if you tried to do schema changes on a builtin function which has IsUDF=true, the error message is not very informational:

drop function pg_catalog.shobj_description;
ERROR: function 0 does not exist: function undefined
SQLSTATE: 42883

This is because the function resolution thought it's a UDF because IsUDF=true but there is no descriptor for it.
It should behave similarly as normal builtin functions, for example:

demo@127.0.0.1:26257/defaultdb> drop function pg_catalog.version;
ERROR: cannot drop function version() -> string because it is required by the database system

So 2 things are needed to address this:
(1) a better way to determine if a overload is UDF or not, checking OID is a good option.
(2) replace IsUDF flag with something better, as long as the executor knows how to execute it.

Jira issue: CRDB-23407
@chengxiong-ruan chengxiong-ruan added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-schema-deprecated Use T-sql-foundations instead labels Jan 13, 2023
@postamar postamar added A-schema-catalog Related to the schema descriptors collection and the catalog API in general. A-sql-routine UDFs and Stored Procedures labels Jan 17, 2023
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Feb 2, 2023
@DrewKimball
tree: distinguish UDFs and builtins that use a SQL string body
5716851 
This refactor changes the meaning of the `Overload.IsUDF` field to
be true only for user-defined functions - meaning those created using
`CREATE FUNCTION`. This is contrasted with builtin functions that are
defined with a SQL string set in `Overload.Body`. Logic that cares only
about user-defined functions can continue checking `Overload.IsUDF`,
while cases that deal with the SQL body should use `Overload.HasSQLBody()`.
Note that it is insufficient to check whether `Overload.Body` is empty
because it is possible to define a UDF with an empty body.

Informs cockroachdb#95214

Release note: None
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Feb 22, 2023
@DrewKimball
tree: distinguish UDFs and builtins that use a SQL string body
92b6c8c 
This refactor changes the meaning of the `Overload.IsUDF` field to
be true only for user-defined functions - meaning those created using
`CREATE FUNCTION`. This is contrasted with builtin functions that are
defined with a SQL string set in `Overload.Body`. Logic that cares only
about user-defined functions can continue checking `Overload.IsUDF`,
while cases that deal with the SQL body should use `Overload.HasSQLBody()`.
Note that it is insufficient to check whether `Overload.Body` is empty
because it is possible to define a UDF with an empty body.

Informs cockroachdb#95214

Release note: None
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Feb 27, 2023
@DrewKimball
tree: distinguish UDFs and builtins that use a SQL string body
70eb6f1 
This refactor changes the meaning of the `Overload.IsUDF` field to
be true only for user-defined functions - meaning those created using
`CREATE FUNCTION`. This is contrasted with builtin functions that are
defined with a SQL string set in `Overload.Body`. Logic that cares only
about user-defined functions can continue checking `Overload.IsUDF`,
while cases that deal with the SQL body should use `Overload.HasSQLBody()`.
Note that it is insufficient to check whether `Overload.Body` is empty
because it is possible to define a UDF with an empty body.

Informs cockroachdb#95214

Release note: None
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Feb 28, 2023
@DrewKimball
tree: distinguish UDFs and builtins that use a SQL string body
3236259 
This refactor changes the meaning of the `Overload.IsUDF` field to
be true only for user-defined functions - meaning those created using
`CREATE FUNCTION`. This is contrasted with builtin functions that are
defined with a SQL string set in `Overload.Body`. Logic that cares only
about user-defined functions can continue checking `Overload.IsUDF`,
while cases that deal with the SQL body should use `Overload.HasSQLBody()`.
Note that it is insufficient to check whether `Overload.Body` is empty
because it is possible to define a UDF with an empty body.

Informs cockroachdb#95214

Release note: None
@mgartner
Copy link
Collaborator

mgartner commented Mar 1, 2023

This was fixed by DrewKimball@5716851. Closing.

@mgartner mgartner closed this as completed Mar 1, 2023
@chengxiong-ruan
Copy link
Contributor Author

Thanks for fixing it @DrewKimball !

DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Mar 7, 2023
@DrewKimball
tree: distinguish UDFs and builtins that use a SQL string body
fd1eaa8 
This refactor changes the meaning of the `Overload.IsUDF` field to
be true only for user-defined functions - meaning those created using
`CREATE FUNCTION`. This is contrasted with builtin functions that are
defined with a SQL string set in `Overload.Body`. Logic that cares only
about user-defined functions can continue checking `Overload.IsUDF`,
while cases that deal with the SQL body should use `Overload.HasSQLBody()`.
Note that it is insufficient to check whether `Overload.Body` is empty
because it is possible to define a UDF with an empty body.

Fixes cockroachdb#95214

Release note: None
@DrewKimball DrewKimball mentioned this issue Mar 10, 2023
Merged
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Mar 10, 2023
@DrewKimball
tree: distinguish UDFs and builtins that use a SQL string body
53d31d1 
This refactor changes the meaning of the `Overload.IsUDF` field to
be true only for user-defined functions - meaning those created using
`CREATE FUNCTION`. This is contrasted with builtin functions that are
defined with a SQL string set in `Overload.Body`. Logic that cares only
about user-defined functions can continue checking `Overload.IsUDF`,
while cases that deal with the SQL body should use `Overload.HasSQLBody()`.
Note that it is insufficient to check whether `Overload.Body` is empty
because it is possible to define a UDF with an empty body.

Fixes cockroachdb#95214

Release note: None
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Mar 17, 2023
@DrewKimball
tree: distinguish UDFs and builtins that use a SQL string body
a326cf9 
This refactor changes the meaning of the `Overload.IsUDF` field to
be true only for user-defined functions - meaning those created using
`CREATE FUNCTION`. This is contrasted with builtin functions that are
defined with a SQL string set in `Overload.Body`. Logic that cares only
about user-defined functions can continue checking `Overload.IsUDF`,
while cases that deal with the SQL body should use `Overload.HasSQLBody()`.
Note that it is insufficient to check whether `Overload.Body` is empty
because it is possible to define a UDF with an empty body.

Fixes cockroachdb#95214

Release note: None
craig bot pushed a commit that referenced this issue Mar 20, 2023
@DrewKimball @abarganier @Xiang-Gu
Merge #96045 #98319 #98696
ea389ff 
96045: opt: check UDFs and UDTs by name when checking metadata dependencies r=DrewKimball a=DrewKimball

#### tree: distinguish UDFs and builtins that use a SQL string body

This refactor changes the meaning of the `Overload.IsUDF` field to
be true only for user-defined functions - meaning those created using
`CREATE FUNCTION`. This is contrasted with builtin functions that are
defined with a SQL string set in `Overload.Body`. Logic that cares only
about user-defined functions can continue checking `Overload.IsUDF`,
while cases that deal with the SQL body should use `Overload.HasSQLBody()`.
Note that it is insufficient to check whether `Overload.Body` is empty
because it is possible to define a UDF with an empty body.

#### opt: refactor metadata dependency tracking

This commit performs some refactoring for the way data source objects
are tracked in `opt.Metadata` in order to make future changes to UDT
and UDF dependency tracking easier. More particularly, UDTs and UDFs
will be able to re-resolve any references by name. This is necessary
in order to handle cases where changes to the search-path cause names
in the query to resolve to different objects.

#### opt: track UDT references by name in the Metadata

Previously, it was possible for invalid queries to be kept in the cache
after a schema change, since user-defined types were tracked only by OID.
This missed cases where the search path changed which object a name in
the query resolved to (or whether it resolved at all).

This patch fixes this behavior by tracking UDT references by name, similar
to what was already done for data sources. This ensures that the query
staleness check doesn't miss changes to the search path.

#### opt: track UDFs in the Metadata

This patch adds tracking for user-defined functions in `opt.Metadata`.
This ensures that the query cache will be correctly invalidated after
a schema change or search-path change that could change the query's
semantics, or cause it to error.

Fixes #95214
Fixes #93082
Fixes #93321
Fixes #96674

Release note (bug fix): Fixed a bug that could prevent a cached query from being invalidated when a UDF or UDT referenced by that query was altered or dropped, or when the schema or database of a UDF or UDT was altered or dropped.

98319: pkg/server: fix `/demologin` to properly redirect to home page r=dhartunian a=abarganier

With the introduction of the server controller, we introduced a layer between the HTTP handler and the HTTP server. When this was introduced, the logic to attempt a login to all tenants forgot to handle a specific case for `/demologin`, where the status code is set to a 302 redirect, instead of a 200 status OK.

This broke the redirect piece of the `/demologin` endpoint.

This patch updates the `attemptLoginToAllTenants` HTTP handler to properly set the 302 response code in the case where the underlying login function does so on the sessionWriter.

Release note: none

Epic: CRDB-12100

Fixes: #98253

98696: sql: disallow using cluster_logical_timestamp as column default when backfilling r=Xiang-Gu a=Xiang-Gu

Previously, `ADD COLUMN ... DEFAULT cluster_logical_timestamp()` would crash the node and leave the table in a corrupt state. The root cause is a nil pointer dereference. This commit fixed it by returning an unimplemented error and hence disallow using this builtin function as default value when backfilling.

Fixes: #98269
Release note (bug fix): fixed a bug as detailed in #98269.

Co-authored-by: Drew Kimball <[email protected]>
Co-authored-by: Alex Barganier <[email protected]>
Co-authored-by: Xiang Gu <[email protected]>
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-schema-catalog Related to the schema descriptors collection and the catalog API in general. A-sql-routine UDFs and Stored Procedures C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

opt: check UDFs and UDTs by name when checking metadata dependencies DrewKimball/cockroach
3 participants
@mgartner @postamar @chengxiong-ruan