| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…rrupt long-running statements
|
@byroot: I challenged myself to implement the statement_timeout= functionality in pure C. This PR is now completely independent of the PR to expose the progress_handler to Ruby. The test passes and I can follow the implementation, so I think it is sound, but I am still quite new to C and C extensions for Ruby, so your review would be greatly appreciated. |
Sorry, something went wrong.
| struct timespec currentTime; | ||
| clock_gettime(CLOCK_MONOTONIC, ¤tTime); | ||
|
|
||
| if (ctx->stmt_deadline == 0) { |
There was a problem hiding this comment.
I don't see it being reset anywhere. I suspect you need to reset the deadline before every query.
Sorry, something went wrong.
There was a problem hiding this comment.
Great point! The place in Statement where I had already access to the database struct was in prepare, so I reset it there. Does that make sense?
Sorry, something went wrong.
| clock_gettime(CLOCK_MONOTONIC, ¤tTime); | ||
|
|
||
| if (ctx->stmt_deadline == 0) { | ||
| ctx->stmt_deadline = currentTime.tv_sec * 1e9 + currentTime.tv_nsec + (long long)ctx->stmt_timeout * 1e6; |
There was a problem hiding this comment.
You could just make stmt_deadline a timespec.
Sorry, something went wrong.
There was a problem hiding this comment.
Genuine question:
Do you find this code "better" (in whatever ways one might access C code in this library)?
static int
rb_sqlite3_statement_timeout(void *context)
{
sqlite3RubyPtr ctx = (sqlite3RubyPtr)context;
struct timespec currentTime;
clock_gettime(CLOCK_MONOTONIC, ¤tTime);
if (ctx->stmt_deadline.tv_sec == 0) {
// Set stmt_deadline if not already set
time_t timeout_sec = ctx->stmt_timeout / 1000;
ctx->stmt_deadline.tv_sec = currentTime.tv_sec + timeout_sec;
ctx->stmt_deadline.tv_nsec = currentTime.tv_nsec;
} else {
// Calculate time difference directly using timespec
struct timespec timeDiff;
timeDiff.tv_sec = ctx->stmt_deadline.tv_sec - currentTime.tv_sec;
timeDiff.tv_nsec = ctx->stmt_deadline.tv_nsec - currentTime.tv_nsec;
// Normalize time difference
while (timeDiff.tv_nsec < 0) {
timeDiff.tv_sec--;
timeDiff.tv_nsec += 1000000000;
}
// Check if time difference is negative
if (timeDiff.tv_sec < 0 || (timeDiff.tv_sec == 0 && timeDiff.tv_nsec <= 0)) {
return 1;
}
}
return 0;
}I find the normalization and final check more confusing than the math in the current version 🤷
Sorry, something went wrong.
There was a problem hiding this comment.
What I was suggesting to do was:
Everything can be made quite easy to grasp if the gritty details of how to add and compare struct timespec is moved into sub functions.
Sorry, something went wrong.
There was a problem hiding this comment.
+1 on @byroot's suggestion. I like the idea of storing the timespec in the sqlite3RubyPtr struct. Then we could use the timespec functions to do this math (or implement our own on platforms that don't have it).
Sorry, something went wrong.
There was a problem hiding this comment.
@tenderlove: I implemented things using timespecs in bd0267e, but for some reason CRuby builds fail trying to #include <timespec.h>. I have no idea what might the problem there, as the error message that the file doesn't exist, but is certainly does exist. Any help at all would be greatly appreciated.
Sorry, something went wrong.
There was a problem hiding this comment.
@fractaledmind You need to add timespec.h to the gemspec so it's packaged into the gem! (Run rake check_manifest to see what's missing.)
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you for the tip! Fixed with one randomly failing action in CI. Should be good to go.
Sorry, something went wrong.
Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
There was a problem hiding this comment.
I'd like @byroot's idea to be incorporated, but otherwise this looks great
Sorry, something went wrong.
| clock_gettime(CLOCK_MONOTONIC, ¤tTime); | ||
|
|
||
| if (ctx->stmt_deadline == 0) { | ||
| ctx->stmt_deadline = currentTime.tv_sec * 1e9 + currentTime.tv_nsec + (long long)ctx->stmt_timeout * 1e6; |
There was a problem hiding this comment.
+1 on @byroot's suggestion. I like the idea of storing the timespec in the sqlite3RubyPtr struct. Then we could use the timespec functions to do this math (or implement our own on platforms that don't have it).
Sorry, something went wrong.
Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
There was a problem hiding this comment.
I left 1 comment, but it's not a blocker. This seems fine to me
Sorry, something went wrong.
| } while (0) | ||
| #define timespecafter(tsp, usp) \ | ||
| (((tsp)->tv_sec > (usp)->tv_sec) || \ | ||
| ((tsp)->tv_sec == (usp)->tv_sec && (tsp)->tv_nsec > (usp)->tv_nsec)) |
There was a problem hiding this comment.
Isn't timespec.h available on some platforms? I thought we'd conditionally define this stuff depending on platform availability.
Sorry, something went wrong.
There was a problem hiding this comment.
TBH, I wasn't sure how best to handle this. Is there a single header file I can try to include that will have these methods defined? How do I conditionally include a header file by whether or not that file exists? And then, do I need to ensure that all methods are present? And timespecafter won't be defined, because that is one I wrote myself to make the logic as direct and explicit as possible in the database file. I considered conditionally defining the macros, but I wasn't certain how to do that, or if I would need to include some header file to try and get the system's macros, if they exist, into the project.
If you have any guidance on how you want to conditionally define this stuff, I'm happy to give it a try. I ended up just deciding that it wasn't much code to bring in, it is pretty clear what it does, and if we own that code, we can guarantee behavior.
Sorry, something went wrong.
There was a problem hiding this comment.
You use have_header in extconf.rb which then define HAVE_... which you can check with a # ifdef
https://ruby-doc.org/stdlib-2.5.1/libdoc/mkmf/rdoc/MakeMakefile.html#method-i-have_header
Sorry, something went wrong.
|
I was going to test this on a machine that has timespec.h, and add the appropriate stuff in extconf.rb. But neither my mac nor my Ubuntu machine have that header, so I'm just going to merge this as-is. @fractaledmind thanks so much for the work and patience! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This PR adds a statement_timeout= method to the Database class which utilizes the progress_handler to interrupt long-running statements/queries. This implementation is directly inspired by Simon Willison's implementation in Datasette, as linked in this SQLite Forum reply.
If you agree with the basic implementation, I want to add a keyword argument to the Database initializer, so that the value can be set via Rails' database.yml file.