Repository navigation
Fix statement_timeout interrupting most queries - #737
Conversation
|
Thanks for the pull request. I'll review as soon as I get a chance. Can you please consider whether we need a specific new test for this behavior? I haven't formed an opinion so your feedback would be welcome, and "no" might be a fine answer. |
39d5e52 to
5021744
Compare
|
@flavorjones I think, we missed one check (added): setting timeout to 0 should allow the original interrupted query to complete. Added timings assertions, too (just in case our timeout calculation is not what it says). Also, tweaked the numbers a bit to reduce flakiness (10ms timeout could be too small to trigger the second progress handler invocation and cause the interruption). (That's why a couple of jobs failed the last time.) |
sqlite3-ruby 2.9 `statement_timeout=` installs a progress handler every 1000 VM opcodes but never adds the millisecond budget to the deadline. The second tick interrupts immediately, so cheap `GROUP BY` / `WHERE state = 'inactive'` die in ~150ms with `query_timeout`. Raising 5s to 10s changes nothing. Upstream: sparklemotion/sqlite3-ruby#737. `sqlite3_step` holds the GVL, so a sleeper plus `interrupt`, or Ruby `Timeout.timeout`, cannot cancel either. There is no public `progress_handler` API. Until that gem ships a real budget, drop the setter. Support SELECT is uncapped in-process; Cloudflare ~100s is the remaining limit.
|
Friendly ping, this fix works |
|
@jasl I'm digging out from under a large open-source backlog, but I promise this is on my short list and I'll get to it soon! |
f96fd10 to
57f237e
Compare
|
@palkan Thanks for the fix! I rebased onto main and pushed three commits to finish it up. Your commit corrects the deadline arithmetic. The deadline was cleared only after a statement was prepared, so once an earlier execution's deadline had expired, each of these raised
fde1678 splits the test into one test per behavior. d0c0df4 clears the deadline before a statement is prepared, when it starts executing, and before The deadline belongs to the connection, so a nested or interleaved execution on the same connection, such as a query inside a custom function, resets the outer execution's deadline. That predates this PR, so I left it alone. |
The deadline was cleared only after a statement was prepared. Once an earlier execution's deadline had expired, re-executing a prepared statement, calling `Database#execute_batch2`, or preparing a statement that reloads the schema could raise `SQLite3::InterruptException` almost immediately. Rails caches prepared statements, so repeated queries in Rails were affected. Clear the deadline before a statement is prepared, when it starts executing, and before `execute_batch2` runs its SQL.
57f237e to
477ee55
Compare
|
@flavorjones Awesome! Thanks! |
3 participants
When statement_timeout is set, the queries triggering the progress handler more than once are interrupted regardless of the timeout value: the current implementation just doesn't use the configured timeout and set the deadline to currentTime (see the diff).
Fast queries still complete (because they trigger the progress handler only once), so it may seem to work but in reality it doesn't. (I caught this bug while writing a test confirming the timeout works in my app; haven't seen in prod env though.)
(A similar bug/fix was reported/proposed earlier in #705).