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.

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

sql: drop function/procedure with composite/table types doesn't work #114677

Closed
DrewKimball opened this issue Nov 17, 2023 · 8 comments · Fixed by #115848
Closed

sql: drop function/procedure with composite/table types doesn't work #114677

DrewKimball opened this issue Nov 17, 2023 · 8 comments · Fixed by #115848
Assignees
Labels
branch-release-23.2 Used to mark GA and release blockers and technical advisories for 23.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker O-qa P-1 Issues/test failures with a fix SLA of 1 month T-sql-queries SQL Queries Team

Comments

@DrewKimball
Copy link
Collaborator

DrewKimball commented Nov 17, 2023

If a function or procedure is overloaded with different parameters, with one of those being a composite type, it becomes impossible to drop the func/proc with the composite parameter. Using the name of the type fails, as does surrounding the name in quotes, as well as changing it to uppercase. Surrounding the type name in quotes and changing it to uppercase seems to work, but the func/proc doesn't actually get dropped.

root@localhost:26257/defaultdb> CREATE TYPE t AS (x INT, y INT);
CREATE TYPE

Time: 20ms total (execution 19ms / network 0ms)

root@localhost:26257/defaultdb> CREATE PROCEDURE foo() LANGUAGE SQL AS $$ SELECT 0; $$;
CREATE PROCEDURE

Time: 52ms total (execution 25ms / network 28ms)

root@localhost:26257/defaultdb> CREATE PROCEDURE foo(v t) LANGUAGE SQL AS $$ SELECT 0; $$;
CREATE PROCEDURE

Time: 68ms total (execution 36ms / network 32ms)

root@localhost:26257/defaultdb> \df
List of functions:
  Schema | Name | Result data type | Argument data types | Type
---------+------+------------------+---------------------+-------
  public | foo  | void             |                     | proc
  public | foo  | void             | t                   | proc
(2 rows)
root@localhost:26257/defaultdb> DROP PROCEDURE foo(t);
ERROR: procedure foo(t) does not exist
SQLSTATE: 42883
root@localhost:26257/defaultdb> DROP PROCEDURE foo("t");
ERROR: procedure foo(t) does not exist
SQLSTATE: 42883
root@localhost:26257/defaultdb> DROP PROCEDURE foo(T);
ERROR: procedure foo(t) does not exist
SQLSTATE: 42883
root@localhost:26257/defaultdb> DROP PROCEDURE foo("T");
DROP PROCEDURE

Time: 4ms total (execution 3ms / network 0ms)

root@localhost:26257/defaultdb> \df
List of functions:
  Schema | Name | Result data type | Argument data types | Type
---------+------+------------------+---------------------+-------
  public | foo  | void             |                     | proc
  public | foo  | void             | t                   | proc
(2 rows)

root@localhost:26257/defaultdb> SHOW CREATE PROCEDURE foo;
  procedure_name |          create_statement
-----------------+--------------------------------------
  foo            | CREATE PROCEDURE public.foo()
                 |     LANGUAGE SQL
                 |     AS $$
                 |     SELECT 0;
                 | $$
  foo            | CREATE PROCEDURE public.foo(IN v T)
                 |     LANGUAGE SQL
                 |     AS $$
                 |     SELECT 0;
                 | $$
(2 rows)

Jira issue: CRDB-33621

@DrewKimball DrewKimball added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-qa labels Nov 17, 2023
Copy link

blathers-crl bot commented Nov 21, 2023

Hi @michae2, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@yuzefovich yuzefovich added the T-sql-queries SQL Queries Team label Nov 21, 2023
@michae2 michae2 added this to Triage in SQL Foundations via automation Nov 21, 2023
@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Nov 21, 2023
@michae2 michae2 added P-1 Issues/test failures with a fix SLA of 1 month and removed T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-sql-queries SQL Queries Team labels Nov 21, 2023
@yuzefovich yuzefovich added T-sql-queries SQL Queries Team T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Nov 21, 2023
@michae2 michae2 added the branch-release-23.2 Used to mark GA and release blockers and technical advisories for 23.2 label Nov 21, 2023
@rafiss
Copy link
Collaborator

rafiss commented Nov 22, 2023

I don't think this needs to be a GA blocker, since the same problem affects 23.1:

root@localhost:26257/defaultdb> CREATE TYPE t AS (x INT, y INT);;
CREATE TYPE

Time: 55ms total (execution 49ms / network 6ms)

root@localhost:26257/defaultdb> CREATE FUNCTION foo() returns int  LANGUAGE SQL AS $$ SELECT 0; $$;
CREATE FUNCTION

Time: 65ms total (execution 34ms / network 32ms)

root@localhost:26257/defaultdb> CREATE FUNCTION foo(v t) returns int  LANGUAGE SQL AS $$ SELECT 0; $$;
CREATE FUNCTION

Time: 84ms total (execution 46ms / network 37ms)

root@localhost:26257/defaultdb> \df
List of functions:
  Schema | Name | Result data type | Argument data types | Type
---------+------+------------------+---------------------+-------
  public | foo  | int8             |                     | func
  public | foo  | int8             | t                   | func
(2 rows)
root@localhost:26257/defaultdb> drop function foo(t);
ERROR: function foo(t) does not exist: function undefined
SQLSTATE: 42883
root@localhost:26257/defaultdb> drop function foo("t");
ERROR: function foo(t) does not exist: function undefined
SQLSTATE: 42883
root@localhost:26257/defaultdb> drop function foo(T);
ERROR: function foo(t) does not exist: function undefined
SQLSTATE: 42883
root@localhost:26257/defaultdb> drop function foo("T");
DROP FUNCTION

Time: 6ms total (execution 4ms / network 2ms)

root@localhost:26257/defaultdb> \df
List of functions:
  Schema | Name | Result data type | Argument data types | Type
---------+------+------------------+---------------------+-------
  public | foo  | int8             |                     | func
  public | foo  | int8             | t                   | func
(2 rows)

@exalate-issue-sync exalate-issue-sync bot removed the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Nov 22, 2023
@michae2
Copy link
Collaborator

michae2 commented Nov 22, 2023

I don't think this needs to be a GA blocker, since the same problem affects 23.1

It's a good point, but we are expecting this exact case (UDFs with composite type parameters) to be used much more in 23.2 than 23.1 by a specific customer, so I think we need to figure this out before we announce GA.

@rharding6373 rharding6373 self-assigned this Nov 28, 2023
@rharding6373
Copy link
Collaborator

The root cause seems to be that we hydrate the t type from DROP PROCEDURE foo(t) as a tuple, but foo(v t)'s overload uses a record type as the parameter type. We determine if the input parameter matches the overload parameter by checking if they are Identical(). An extra wrinkle in that before calling Identical() we turn tuple input parameters into AnyTuple, which I don't think will match even if the overload said it has a tuple type parameter because of the strict comparisons in Identical() compared to Equivalent().

The solution is going to be reconciling this somehow, whether this means hydrating certain composite types as records, using tuple as the overload parameter type for composite types, or treating tuple and record as identical.

@rafiss
Copy link
Collaborator

rafiss commented Nov 30, 2023

I wonder if this relates to #102227

@DrewKimball
Copy link
Collaborator Author

Note: this only reproduces when the function/proc name is overloaded, as in the example.

@rharding6373
Copy link
Collaborator

Addendum to the preceding comment: It does repro when the function is not overloaded, but the workaround is to drop the function/proc without arguments. Here's an example:

root@127.0.0.1:26257/defaultdb> CREATE TYPE t AS (x INT, y INT);                                                      
CREATE TYPE

Time: 12ms total (execution 12ms / network 0ms)

root@127.0.0.1:26257/defaultdb> CREATE PROCEDURE foo(v t) LANGUAGE SQL AS $$ SELECT 0; $$;                            
CREATE PROCEDURE

Time: 29ms total (execution 15ms / network 15ms)

root@127.0.0.1:26257/defaultdb> \df                                                                                   
List of functions:
  Schema | Name | Result data type | Argument data types | Type
---------+------+------------------+---------------------+-------
  public | foo  | void             | t                   | proc
(1 row)
root@127.0.0.1:26257/defaultdb> DROP PROCEDURE foo(t);                                                                
ERROR: procedure foo(t) does not exist
SQLSTATE: 42883
root@127.0.0.1:26257/defaultdb> DROP PROCEDURE foo;                                                                   
DROP PROCEDURE

Time: 131ms total (execution 128ms / network 3ms)

root@127.0.0.1:26257/defaultdb> \df                                                                                   
List of functions:
  Schema | Name | Result data type | Argument data types | Type
---------+------+------------------+---------------------+-------
(0 rows)

@DrewKimball
Copy link
Collaborator Author

An extra wrinkle in that before calling Identical() we turn tuple input parameters into AnyTuple, which I don't think will match even if the overload said it has a tuple type parameter because of the strict comparisons in Identical() compared to Equivalent()

I think this might have been the main problem - it looks like an oversight from when MatchIdentical was written.

DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 8, 2023
Previously, it was not possible to specify UDT parameters when dropping
a routine. This is because of an oversight in `MatchAtIdentical`, which
was likely copied from `MatchAt`. Specifically, `MatchAt` has special
handling for tuple types that take advantage of the fact that
`types.Equivalent` allows `types.AnyTuple` to match with any other tuple.
`MatchAtIdentical, on the other hand, uses `types.Identical`, which does
not make the same allowance. This meant that overload resolution would
always fail for UDT parameters in code paths that use `MatchAtIdentical`,
such as DROP FUNCTION.

This patch removes the replacement logic for tuple types in `MatchAtIdentical`,
so that `types.Identical` is always called with the original type. This allows
statements like the following to succeed:
```
DROP PROCEDURE foo(x udt);
```

Fixes cockroachdb#114677

Release note (bug fix): Fixed a bug existing since v23.1 that prevented
naming UDT parameters when dropping a user-defined function (or procedure).
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Dec 8, 2023
Previously, it was not possible to specify UDT parameters when dropping
a routine. This is because of an oversight in `MatchAtIdentical`, which
was likely copied from `MatchAt`. Specifically, `MatchAt` has special
handling for tuple types that take advantage of the fact that
`types.Equivalent` allows `types.AnyTuple` to match with any other tuple.
`MatchAtIdentical, on the other hand, uses `types.Identical`, which does
not make the same allowance. This meant that overload resolution would
always fail for UDT parameters in code paths that use `MatchAtIdentical`,
such as DROP FUNCTION.

This patch removes the replacement logic for tuple types in `MatchAtIdentical`,
so that `types.Identical` is always called with the original type. This allows
statements like the following to succeed:
```
DROP PROCEDURE foo(x udt);
```

Fixes cockroachdb#114677

Release note (bug fix): Fixed a bug existing since v23.1 that prevented
naming UDT parameters when dropping a user-defined function (or procedure).
craig bot pushed a commit that referenced this issue Dec 9, 2023
115848: sql: allow dropping routines with UDT-typed parameters r=DrewKimball a=DrewKimball

Previously, it was not possible to specify UDT parameters when dropping a routine. This is because of an oversight in `MatchAtIdentical`, which was likely copied from `MatchAt`. Specifically, `MatchAt` has special handling for tuple types that take advantage of the fact that `types.Equivalent` allows `types.AnyTuple` to match with any other tuple. `MatchAtIdentical, on the other hand, uses `types.Identical`, which does not make the same allowance. This meant that overload resolution would always fail for UDT parameters in code paths that use `MatchAtIdentical`, such as DROP FUNCTION.

This patch removes the replacement logic for tuple types in `MatchAtIdentical`, so that `types.Identical` is always called with the original type. This allows statements like the following to succeed:
```
DROP PROCEDURE foo(x udt);
```

Fixes #114677

Release note (bug fix): Fixed a bug existing since v23.1 that prevented naming UDT parameters when dropping a user-defined function (or procedure).

Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
@craig craig bot closed this as completed in defd746 Dec 9, 2023
SQL Foundations automation moved this from Triage to Done Dec 9, 2023
blathers-crl bot pushed a commit that referenced this issue Dec 9, 2023
Previously, it was not possible to specify UDT parameters when dropping
a routine. This is because of an oversight in `MatchAtIdentical`, which
was likely copied from `MatchAt`. Specifically, `MatchAt` has special
handling for tuple types that take advantage of the fact that
`types.Equivalent` allows `types.AnyTuple` to match with any other tuple.
`MatchAtIdentical, on the other hand, uses `types.Identical`, which does
not make the same allowance. This meant that overload resolution would
always fail for UDT parameters in code paths that use `MatchAtIdentical`,
such as DROP FUNCTION.

This patch removes the replacement logic for tuple types in `MatchAtIdentical`,
so that `types.Identical` is always called with the original type. This allows
statements like the following to succeed:
```
DROP PROCEDURE foo(x udt);
```

Fixes #114677

Release note (bug fix): Fixed a bug existing since v23.1 that prevented
naming UDT parameters when dropping a user-defined function (or procedure).
blathers-crl bot pushed a commit that referenced this issue Dec 9, 2023
Previously, it was not possible to specify UDT parameters when dropping
a routine. This is because of an oversight in `MatchAtIdentical`, which
was likely copied from `MatchAt`. Specifically, `MatchAt` has special
handling for tuple types that take advantage of the fact that
`types.Equivalent` allows `types.AnyTuple` to match with any other tuple.
`MatchAtIdentical, on the other hand, uses `types.Identical`, which does
not make the same allowance. This meant that overload resolution would
always fail for UDT parameters in code paths that use `MatchAtIdentical`,
such as DROP FUNCTION.

This patch removes the replacement logic for tuple types in `MatchAtIdentical`,
so that `types.Identical` is always called with the original type. This allows
statements like the following to succeed:
```
DROP PROCEDURE foo(x udt);
```

Fixes #114677

Release note (bug fix): Fixed a bug existing since v23.1 that prevented
naming UDT parameters when dropping a user-defined function (or procedure).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-23.2 Used to mark GA and release blockers and technical advisories for 23.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker O-qa P-1 Issues/test failures with a fix SLA of 1 month T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants