Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
6c4e155
Add the progress_handler interface
fractaledmind Jan 7, 2024
046bd67
Add tests for the progress_handler interface
fractaledmind Jan 7, 2024
11a94dc
Merge branch 'main' into progress-handler
fractaledmind Jan 7, 2024
50f1ef0
Remove extra line
fractaledmind Jan 7, 2024
c6bf834
Bump the frequency for the opcode arg test
fractaledmind Jan 7, 2024
3681b2a
Bump the frequency for the opcode arg test and make assertion more re…
fractaledmind Jan 7, 2024
c0d5144
Only define the ruby progress_handler method if the OMIT compilation …
fractaledmind Jan 7, 2024
e3f95ca
Add statement_timeout= method that leverages progress_handler to inte…
fractaledmind Jan 8, 2024
3ebee19
Merge branch 'progress-handler' into statement-timeout
fractaledmind Jan 8, 2024
b819265
Add a test for statement_timeout=
fractaledmind Jan 8, 2024
3ba847c
Allow statement_timeout= to accept a nil to reset the progress_handler
fractaledmind Jan 8, 2024
870f3b9
Merge branch 'master' into statement-timeout
fractaledmind Jan 11, 2024
bf1f0e6
Implement that statement_timeout= in pure C
fractaledmind Jan 11, 2024
c8007e7
Remove old progress_handler tests
fractaledmind Jan 11, 2024
ae76a01
Update ext/sqlite3/database.c
fractaledmind Jan 11, 2024
cd47d9b
Reset stmt_deadline when preparing a statement
fractaledmind Jan 11, 2024
13d4067
Make test faster
fractaledmind Jan 11, 2024
31e44c1
Merge branch 'main' into statement-timeout
fractaledmind Jan 21, 2024
bd0267e
Implement statement_timeout with timespecs
fractaledmind Jan 21, 2024
6456dec
Add the new timespec.h file to the gemspec
fractaledmind Jan 22, 2024
832fe6a
Update test-gem-file-contents
fractaledmind Jan 22, 2024
c80aeef
Update ext/sqlite3/database.c
fractaledmind Jan 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions ext/sqlite3/database.c
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,45 @@ busy_handler(int argc, VALUE *argv, VALUE self)
return self;
}

static int
rb_sqlite3_statement_timeout(void *context)
{
sqlite3RubyPtr ctx = (sqlite3RubyPtr)context;
struct timespec currentTime;
clock_gettime(CLOCK_MONOTONIC, &currentTime);

if (ctx->stmt_deadline == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

ctx->stmt_deadline = currentTime.tv_sec * 1e9 + currentTime.tv_nsec + (long long)ctx->stmt_timeout * 1e6;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just make stmt_deadline a timespec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@flavorjones flavorjones Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
long long timeDiff = ctx->stmt_deadline - (currentTime.tv_sec * 1e9 + currentTime.tv_nsec);

if (timeDiff < 0) { return 1; }
}

return 0;
}

/* call-seq: db.statement_timeout = ms
*
* Indicates that if a query lasts longer than the indicated number of
* milliseconds, SQLite should interrupt that query and return an error.
* By default, SQLite does not interrupt queries. To restore the default
* behavior, send 0 as the +ms+ parameter.
*/
static VALUE
set_statement_timeout(VALUE self, VALUE milliseconds)
{
sqlite3RubyPtr ctx;
TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx);

ctx->stmt_timeout = NUM2INT(milliseconds);
int n = NUM2INT(milliseconds) == 0 ? -1 : 1000;

sqlite3_progress_handler(ctx->db, n, rb_sqlite3_statement_timeout, (void *)ctx);

return self;
}

/* call-seq: last_insert_row_id
*
* Obtains the unique row ID of the last row to be inserted by this Database
Expand Down Expand Up @@ -875,6 +914,9 @@ init_sqlite3_database(void)
rb_define_method(cSqlite3Database, "authorizer=", set_authorizer, 1);
rb_define_method(cSqlite3Database, "busy_handler", busy_handler, -1);
rb_define_method(cSqlite3Database, "busy_timeout=", set_busy_timeout, 1);
#ifndef SQLITE_OMIT_PROGRESS_CALLBACK
rb_define_method(cSqlite3Database, "statement_timeout=", set_statement_timeout, 1);
#endif
rb_define_method(cSqlite3Database, "extended_result_codes=", set_extended_result_codes, 1);
rb_define_method(cSqlite3Database, "transaction_active?", transaction_active_p, 0);
rb_define_private_method(cSqlite3Database, "exec_batch", exec_batch, 2);
Expand Down
2 changes: 2 additions & 0 deletions ext/sqlite3/database.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
struct _sqlite3Ruby {
sqlite3 *db;
VALUE busy_handler;
int stmt_timeout;
long long stmt_deadline;
};

typedef struct _sqlite3Ruby sqlite3Ruby;
Expand Down
1 change: 1 addition & 0 deletions ext/sqlite3/statement.c
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ prepare(VALUE self, VALUE db, VALUE sql)
);

CHECK(db_ctx->db, status);
db_ctx->stmt_deadline = 0;

return rb_str_new2(tail);
}
Expand Down
1 change: 1 addition & 0 deletions lib/sqlite3/database.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ def initialize file, options = {}, zvfs = nil
@authorizer = nil
@encoding = nil
@busy_handler = nil
@progress_handler = nil
@collations = {}
@functions = {}
@results_as_hash = options[:results_as_hash]
Expand Down
16 changes: 16 additions & 0 deletions test/test_integration_statement.rb
Original file line number Diff line number Diff line change
Expand Up @@ -191,4 +191,20 @@ def test_committing_tx_with_statement_active
end
assert called
end

def test_long_running_statements_get_interrupted_when_statement_timeout_set
@db.statement_timeout = 10
assert_raises(SQLite3::InterruptException) do
@db.execute <<~SQL
WITH RECURSIVE r(i) AS (
VALUES(0)
UNION ALL
SELECT i FROM r
LIMIT 100000
)
SELECT i FROM r ORDER BY i LIMIT 1;
SQL
end
@db.statement_timeout = 0
end
end