-
-
Notifications
You must be signed in to change notification settings - Fork 238
use sqlCtx in handler #2718
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
use sqlCtx in handler #2718
Conversation
zachmu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks right to me but it could use some more tests, very easy to get this stuff wrong. I see the test in Dolt but it would be nice to have one a Go test in GMS as well
reltuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is a mess. I agree that the same context that gets returned from BeginQuery should be used as the argument to ProcessList.EndQuery here and as the context argument to queryExec. But, I don't agree with a lot of details of this current function, including the code directly touched here:
- It seems like the err on line 422 should be checked.
- It seems like EndQuery should be called regardless of whether there was an error.
- Checking for
ctx != nilin the defer block seems wrong. - On line 414 we
defer finalize(err), which is immediately evaluating the err variable, where as it seems like we meant to defer the evaluation until we know the error return of the actual function. - Given that we want to defer evaluating the return value of this function it seems like we shouldn't shadow
erron line 428. And, because we use a direct return of the callback evaluation on line 479, we either need to change that toerr = callback...or we need to switch to named return values.
Made these cleanups here if you agree:
Otherwise looks good to me. Definitely an appropriate path to fixing the kill behavior we've seen. Good findings and investigation.
Tests here: dolthub/dolt#8493