Add statement_timeout= method

[fractaledmind]

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.

[fractaledmind]

@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.

[byroot]

I don't see it being reset anywhere. I suspect you need to reset the deadline before every query.

[fractaledmind]

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?

[byroot]

You could just make stmt_deadline a timespec.

[fractaledmind]

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, &currentTime);

    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 🤷

[byroot]

What I was suggesting to do was:

• Make ctx->stmt_deadline a struct timespec.
• Implement a timespecadd function, IIRC it is included on some system, but Linux might not have it. But basically it's similar to your thing.
• Then continue with your initial algo, which is to compute the deadline when the query is triggered
• Then in the callback, get current time and compare it with the deadline.

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.

[tenderlove]

+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).

[fractaledmind]

@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.

[flavorjones]

@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.)

[fractaledmind]

Thank you for the tip! Fixed with one randomly failing action in CI. Should be good to go.

[tenderlove]

I'd like @byroot's idea to be incorporated, but otherwise this looks great

[tenderlove]

+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).

[tenderlove]

I left 1 comment, but it's not a blocker. This seems fine to me

[tenderlove]

Isn't timespec.h available on some platforms? I thought we'd conditionally define this stuff depending on platform availability.

[fractaledmind]

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.

[byroot]

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

[tenderlove]

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!