Skip to content

Commit

Permalink
Optimize the statement check for a non-discarded database
Browse files Browse the repository at this point in the history
This also restores the ability (now tested!) to call Database#close
successfully and defer its cleanup until after Statements are closed,
which was the promise of #557 and sqlite3_close_v2.

Closes #564
  • Loading branch information
flavorjones committed Sep 19, 2024
1 parent 81ea485 commit 806657c
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 36 deletions.
16 changes: 9 additions & 7 deletions ext/sqlite3/database.c
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,16 @@ discard_db(sqlite3RubyPtr ctx)
}

ctx->db = NULL;
ctx->flags |= SQLITE3_RB_DATABASE_DISCARDED;
}

static void
close_or_discard_db(sqlite3RubyPtr ctx)
{
if (ctx->db) {
int isReadonly = (ctx->flags & SQLITE_OPEN_READONLY);
int is_readonly = (ctx->flags & SQLITE3_RB_DATABASE_READONLY);

if (isReadonly || ctx->owner == getpid()) {
if (is_readonly || ctx->owner == getpid()) {
// Ordinary close.
sqlite3_close_v2(ctx->db);
ctx->db = NULL;
Expand Down Expand Up @@ -153,7 +154,9 @@ rb_sqlite3_open_v2(VALUE self, VALUE file, VALUE mode, VALUE zvfs)
);

CHECK(ctx->db, status);
ctx->flags = flags;
if (flags & SQLITE_OPEN_READONLY) {
ctx->flags |= SQLITE3_RB_DATABASE_READONLY;
}

return self;
}
Expand Down Expand Up @@ -943,11 +946,10 @@ rb_sqlite3_open16(VALUE self, VALUE file)
#endif
#endif

status = sqlite3_open16(utf16_string_value_ptr(file), &ctx->db);

// these are the perm flags used implicitly by sqlite3_open16,
// sqlite3_open16 implicitly uses flags (SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE)
// see https://www.sqlite.org/capi3ref.html#sqlite3_open
ctx->flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE;
// so we do not ever set SQLITE3_RB_DATABASE_READONLY in ctx->flags
status = sqlite3_open16(utf16_string_value_ptr(file), &ctx->db);

CHECK(ctx->db, status)

Expand Down
3 changes: 3 additions & 0 deletions ext/sqlite3/database.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@

#include <sqlite3_ruby.h>

#define SQLITE3_RB_DATABASE_READONLY 0x01
#define SQLITE3_RB_DATABASE_DISCARDED 0x02

struct _sqlite3Ruby {
sqlite3 *db;
VALUE busy_handler;
Expand Down
51 changes: 23 additions & 28 deletions ext/sqlite3/statement.c
Original file line number Diff line number Diff line change
@@ -1,22 +1,12 @@
#include <sqlite3_ruby.h>

#define REQUIRE_OPEN_STMT(_ctxt) \
if(!_ctxt->st) \
if (!_ctxt->st) \
rb_raise(rb_path2class("SQLite3::Exception"), "cannot use a closed statement");

static void
require_open_db(VALUE stmt_rb)
{
VALUE closed_p = rb_funcall(
rb_iv_get(stmt_rb, "@connection"),
rb_intern("closed?"), 0);

if (RTEST(closed_p)) {
rb_raise(rb_path2class("SQLite3::Exception"),
"cannot use a statement associated with a closed database");
}
}

#define REQUIRE_LIVE_DB(_ctxt) \
if (_ctxt->db->flags & SQLITE3_RB_DATABASE_DISCARDED) \
rb_raise(rb_path2class("SQLite3::Exception"), "cannot use a statement associated with a discarded database");

VALUE cSqlite3Statement;

Expand Down Expand Up @@ -71,6 +61,11 @@ prepare(VALUE self, VALUE db, VALUE sql)

TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

/* Dereferencing a pointer to the database struct will be faster than accessing it through the
* instance variable @connection. The struct pointer is guaranteed to be live because instance
* variable will keep it from being GCed. */
ctx->db = db_ctx;

#ifdef HAVE_SQLITE3_PREPARE_V2
status = sqlite3_prepare_v2(
#else
Expand Down Expand Up @@ -135,7 +130,7 @@ step(VALUE self)

TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

if (ctx->done_p) { return Qnil; }
Expand Down Expand Up @@ -232,7 +227,7 @@ bind_param(VALUE self, VALUE key, VALUE value)

TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

switch (TYPE(key)) {
Expand Down Expand Up @@ -326,7 +321,7 @@ reset_bang(VALUE self)

TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

sqlite3_reset(ctx->st);
Expand All @@ -348,7 +343,7 @@ clear_bindings_bang(VALUE self)

TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

sqlite3_clear_bindings(ctx->st);
Expand Down Expand Up @@ -382,7 +377,7 @@ column_count(VALUE self)
sqlite3StmtRubyPtr ctx;
TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

return INT2NUM(sqlite3_column_count(ctx->st));
Expand Down Expand Up @@ -415,7 +410,7 @@ column_name(VALUE self, VALUE index)

TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

name = sqlite3_column_name(ctx->st, (int)NUM2INT(index));
Expand All @@ -440,7 +435,7 @@ column_decltype(VALUE self, VALUE index)

TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

name = sqlite3_column_decltype(ctx->st, (int)NUM2INT(index));
Expand All @@ -459,7 +454,7 @@ bind_parameter_count(VALUE self)
sqlite3StmtRubyPtr ctx;
TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

return INT2NUM(sqlite3_bind_parameter_count(ctx->st));
Expand Down Expand Up @@ -568,7 +563,7 @@ stats_as_hash(VALUE self)
sqlite3StmtRubyPtr ctx;
TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

VALUE arg = rb_hash_new();
Expand All @@ -587,7 +582,7 @@ stat_for(VALUE self, VALUE key)
sqlite3StmtRubyPtr ctx;
TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

if (SYMBOL_P(key)) {
Expand All @@ -609,7 +604,7 @@ memused(VALUE self)
sqlite3StmtRubyPtr ctx;
TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

return INT2NUM(sqlite3_stmt_status(ctx->st, SQLITE_STMTSTATUS_MEMUSED, 0));
Expand All @@ -628,7 +623,7 @@ database_name(VALUE self, VALUE index)
sqlite3StmtRubyPtr ctx;
TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

return SQLITE3_UTF8_STR_NEW2(
Expand All @@ -647,7 +642,7 @@ get_sql(VALUE self)
sqlite3StmtRubyPtr ctx;
TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

return rb_obj_freeze(SQLITE3_UTF8_STR_NEW2(sqlite3_sql(ctx->st)));
Expand All @@ -667,7 +662,7 @@ get_expanded_sql(VALUE self)

TypedData_Get_Struct(self, sqlite3StmtRuby, &statement_type, ctx);

require_open_db(self);
REQUIRE_LIVE_DB(ctx);
REQUIRE_OPEN_STMT(ctx);

expanded_sql = sqlite3_expanded_sql(ctx->st);
Expand Down
1 change: 1 addition & 0 deletions ext/sqlite3/statement.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

struct _sqlite3StmtRuby {
sqlite3_stmt *st;
sqlite3Ruby *db;
int done_p;
};

Expand Down
2 changes: 1 addition & 1 deletion test/test_discarding.rb
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def test_a_discarded_connection_with_statements
db.send(:discard)

e = assert_raises(SQLite3::Exception) { stmt.execute }
assert_match(/cannot use a statement associated with a closed database/, e.message)
assert_match(/cannot use a statement associated with a discarded database/, e.message)

assert_nothing_raised { stmt.close }
assert_predicate(stmt, :closed?)
Expand Down
7 changes: 7 additions & 0 deletions test/test_statement.rb
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,13 @@ def test_new_closed_handle
end
end

def test_closed_db_behavior
@db.close
result = nil
assert_nothing_raised { result = @stmt.execute }
refute_nil result
end

def test_new_with_remainder
stmt = SQLite3::Statement.new(@db, "select 'foo';bar")
assert_equal "bar", stmt.remainder
Expand Down

0 comments on commit 806657c

Please sign in to comment.