Closed detule closed 4 months ago
feature: interruptible execution
; remaining commits are auxiliary / supporting cast.snowflake
tests are failing perhaps because I don't have access to the github credentials / secret/@detule @simonpcouch Some quick comments wrt the new argument and having a (not so) secret option instead. I think an option or an env var is much friendlier for the users than the new argument.
FALSE
(if you are conservative), and then in the next release to TRUE
, and ideally at some point drop the option altogether. You can do all this without changing APIs or code that uses odbc.@detule A quick question about concurrency and resources. If there is an interrupt, then cleanup_fn()
runs concurrently with the async thread, right?
And after an interrupt, the main thread invalidates the DB handle that the async thread is using? If that's the case, are the ODBC drivers prepared for this?
Not doing this myself in case @detule has local changes, but this PR should pass CI with changes merged from main
!
HI @gaborcsardi
Appreciate you taking a look.
interruptible
argument default to a global option. Good call.cleanup_fn
would run concurrently with the async/execution thread. Per the ODBC spec this should be fine, since before invalidating the statement as part of the clean up, we first call SQLCancel
. In the async thread that should cause the statement to terminate processing.
Not doing this myself in case @detule has local changes, but this PR should pass CI with changes merged from
main
!
Thanks @simonpcouch - rebased on top of main
. Hopefully it's all green.
Per the ODBC spec this should be fine, since before invalidating the statement as part of the clean up, we first call SQLCancel
I don't see where that happens, probably in close()
?
In any case, this looks pretty good then. The only concern I have is that you need to install the dev MariaDB drivers to make ODBC run reliably. I would think that many ODBC users are stuck on older Linux systems, so we could potentially break their projects.
So maybe you could make this feature opt-in for now? And make it the default in the next release?
Per the ODBC spec this should be fine, since before invalidating the statement as part of the clean up, we first call SQLCancel
I don't see where that happens, probably in
close()
?
Yes, both set_current_result
, and close
in the cleanup function will cancel the current result, as needed.
In any case, this looks pretty good then. The only concern I have is that you need to install the dev MariaDB drivers to make ODBC run reliably. I would think that many ODBC users are stuck on older Linux systems, so we could potentially break their projects.
So maybe you could make this feature opt-in for now? And make it the default in the next release?
Thanks @gaborcsardi . Appreciate you taking a look. On mariadb
- do you still feel the same way knowing that the only test that the driver seems to be misfiring on is the relatively contrived dbGetQuery(conn, NA_character_)
? All other tests, more than three thousand of them, seem to be passing on both Linux and Windows ( admittedly MySQL, not MariaDB ) without issues. At any rate, I don't have a problem making it opt-in and happy to do it, just a bit concerned that it won't get enough mileage.
At any rate, I don't have a problem making it opt-in and happy to do it, just a bit concerned that it won't get enough mileage.
You can make a much more informed decision than me. In general I tend to be conservative in these kind of issues, especially with packages that may be used on older systems, like odbc.
You can also make the default depend on whether the session is interactive or not. I would think that interruption is much less important in non-interactive sessions, where an interruption just kills the process, because there are already ways to kill the process. So you could turn it on in interactive sessions only.
You can also make the default depend on whether the session is interactive or not. I would think that interruption is much less important in non-interactive sessions, where an interruption just kills the process, because there are already ways to kill the process. So you could turn it on in interactive sessions only.
Done - thanks, that's a great suggestion.
(I cannot express sufficiently how much I've wanted this and never knew to ask for it.)
Hi All:
Please consider adding this feature - the ability to "Ctrl-C" past a long running query. For example in SQL Server
Some notes - i feel like I am being wordy below but I am sure I still left quite a bit out:
run_interruptible
wrapper that takes two arguments: a function to be executed away from the main thread, and a cleanup function to be invoked once the signal is caught on the main thread. Perhaps this is an overkill but I did this with an eye that maybe some day we may need to be able to interrupt other calls from theodbc
API, not justSQLExecute
. We can then just re-use the wrapper.std::async
rather thanstd::thread
: I personally am a little biased towardsstd::async
but mostly based on semantics. I know there's quite a bit left up to the implementation but at this point it's been around for long enough that I think there should be little in the way of surprises. I've witnessed some passionate thread-vs-async discussions before but I think it's mostly a matter of style. If folks feel strongly one way or another, happy to change it.pthread
API to make sure the SIGINT signal is delivered to the main thread. This is pretty idiosyncratic, but i've seen issues where depending on the implementation, a signal can end up with the wrong thread.dbConnect
accruing yet more arguments. At the same time also not in love with introducing some sort of a secret option ---getOption(.odbc.interruptible.exec)
--- or somesuch. There is anattributes
arguments todbConnect
but when we introduced it, I think we envisionedODBC
/API connection arguments. So - hated myself a little bit for it - but ended up writing in a new argument todbConnect
.Testing: Pipelines are (hopefully) passing on both windows and linux and that's a good sign. Beyond that, on my linux box, I tested DB2, Oracle, Terradata, Databricks and did not see any issues. Note, the
WAITFOR
style queries are not always interruptible. For example onsnowflake
, interrupting the equivalentSELECT system$wait(10)
won't save you any time since theSQLCancel
call blocks on the main thread after catching the interrupt. However, testing an actual long running execution I saw better behavior.dbSend(Get)Query(conn, NA_character_)
( part ofDBItest::send(get)_query_non_string
)dbSendStatement(conn, NA_character_)
( part ofDBItest::send_statement_non_string
)dbExecute(conn, NA_character_)
( part ofDBItest::execute_non_string
)Note, all remaining (~3.5K)
DBItest
tests pass with both drivers just fine. Spent quite a bit of time comparing odbc traces to no avail, and thought it's something so idiosyncratic ( and fixed in their newer drivers to boot ) that it should probably not hold this feature back. Options to ignore were to use newer driver, or to add the tests to the "skip" list fordriver-mysql
.Appreciate any feedback - and thanks for taking a look!