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

Add support for handlers #7454

Open
tim-oh-thee opened this issue Feb 2, 2024 · 15 comments
Open

Add support for handlers #7454

tim-oh-thee opened this issue Feb 2, 2024 · 15 comments
Assignees
Labels
correctness We don't return the same result as MySQL customer issue enhancement New feature or request

Comments

@tim-oh-thee
Copy link

The documentation here: https://docs.dolthub.com/sql-reference/sql-support/supported-statements states that handlers are only supported for cursors. Could we add support for these? I was recently trying to create a procedure and handle potential errors and didn't really have an option to do it.

MySQL reference: https://dev.mysql.com/doc/refman/8.0/en/handler.html

@timsehn timsehn added enhancement New feature or request correctness We don't return the same result as MySQL labels Feb 2, 2024
@jycor
Copy link
Contributor

jycor commented Feb 3, 2024

Hey @tim-oh-thee,

Just wanted to make sure we are on the same page.
You want support for the other where_conditions in DECALRE ... HANDLER ...:
https://dev.mysql.com/doc/refman/8.0/en/declare-handler.html

Not support for the Handler statement:
https://dev.mysql.com/doc/refman/8.0/en/handler.html

@tim-oh-thee
Copy link
Author

Yep you’re absolutely right. My apologies for any confusion.

@max-hoffman
Copy link
Contributor

Hi @tim-oh-thee, would you be willing to share the specific behavior you are interested in? We don't meaningfully support handler execution right now and there are a lot of different features. Most condition filtering is based on JDBC sql state codes, and we also do not support those currently.

For example, I can see a path towards supporting specific SQL error code integers because that is independent of state codes. If you need state code support I would need to do more research to figure out how to add that separate layer.

@tim-oh-thee
Copy link
Author

tim-oh-thee commented Feb 5, 2024

Aha that might be the right place to start. I was trying to do that exact thing. I forgot that I was coming up with the same sql state no matter what.
The procedure I was writing did not need to be that robust so I was fine with just catching the any exception and skipping it. So, in this case the fact that I couldn't use a handler to do that was the limitation I was hitting, not the supported SQL state codes.

@max-hoffman
Copy link
Contributor

The way the current code works is that you can only create NOT FOUND condition handlers. But we do not perform any condition filter, so the handlers will run on all errors (@Hydrocharged let me know if I'm misunderstanding). A thing we could do is let you create every type of condition but continue to not do error filtering. The handlers would run in inappropriate circumstances in that case, but it would let you skip errors with the proper procedure syntax. In the future, if you needed to distinguish between certain types of errors we could come back and implement filters for certain error integers, or add sql state codes if need-be. Does that sound like a useful progression for you?

@tim-oh-thee
Copy link
Author

Want to make sure I'm understanding this correctly. Essentially you'll be providing me the capability to say:
DECLARE CONTINUE HANDLER FOR 1051
BEGIN
-- body of handler
END;

but since there is no granularity in the error messages, I won't be able to control whether that handler is working on a specific issue (for example only an error for when a table doesn't exist). It'd essentially execute the body of the handler for every error.

If this understanding is correct, then this makes sense to me and fits my intent with what I was trying to do in the first place. It'd be nice to see this built out more in the future for different error codes or sql states, etc..

@max-hoffman
Copy link
Contributor

Did a deeper pass and I think it's going to be more involved than I originally thought. Do you have any codes other than 1051 that you would like to support? We might need to do these one by one, but there are thousands of errors to map and so it will be most practical to target a subset on the first iteration.

@max-hoffman
Copy link
Contributor

For context, one of the problems here is that we do not have a 3-way mapping between sql states, error codes, and Dolt error types. So we can go and catalog all of our error types, but there are thousands of relationships we do have and probably more that we do not or need to be refactored to support full sql state partitioning.

The second problem is adding callbacks at any point where errors happen in stored procedure execution to perform the filtering and handler check. The first version of this feature only implemented the NOT FOUND error because it only happens in LOOP calls and doesn't require any sql state normalization.

The space of handler behavior is big and I'd prefer unblock you soon rather than spending a month trying to hit every error code. There are certain common errors that could start cataloging and releasing first -- "table not found", "column not found", "syntax error", procedure specific errors, ...etc. But I am also not the stored procedure expert and guidance on your use case would help us make sure we're fixing what you need.

@tim-oh-thee
Copy link
Author

Could we do the SQLEXCEPTION one?
DECLARE CONTINUE HANDLER FOR SQLEXCEPTION
BEGIN
-- body of handler
END;

@tim-oh-thee
Copy link
Author

1051 was just the example I pulled from the documentation, nothing important about that one specifically.

@max-hoffman
Copy link
Contributor

Yeah I was thinking the same thing last night, I'll start adding SQLEXCEPTION support this morning. The work for that feature should make it easier to quickly extend support for specific codes in the future. Thank you for the clarifications @tim-oh-thee !

@tim-oh-thee
Copy link
Author

Thanks for the help!

@max-hoffman
Copy link
Contributor

Thanks for your patience @tim-oh-thee, I have a PR here that id some directional progress that you might find useful: dolthub/go-mysql-server#2323.

This should support SQLEXCEPTION handlers that run for errors (other than those caught by NOT FOUND, same behavior as MySQL). The setup there should let us extend support for specific error codes or states in the future. And of course if the feature has correctness gaps compared to MySQL let us know and we will follow up and fix ASAP.

We will let you know when this PR makes it into a Dolt release.

@tim-oh-thee
Copy link
Author

Perfect, thank you Max!

@max-hoffman
Copy link
Contributor

The changes should be in dolt 1.34.0. I am going to leave this thread open for visibility given the limited support currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
correctness We don't return the same result as MySQL customer issue enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants