From 51009c0d3523c603dfc3da4831edfe65eae05e5e Mon Sep 17 00:00:00 2001 From: Masaki Suketa Date: Sat, 25 Jan 2025 06:59:06 +0900 Subject: [PATCH 01/10] try to ruby-asan test --- .github/workflows/ruby_asan_on_ubuntu.yml | 71 +++++++++++++++++++++++ ext/duckdb/prepared_statement.c | 7 ++- test/duckdb_test/ruby_asan_test.rb | 12 ++++ 3 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/ruby_asan_on_ubuntu.yml create mode 100644 test/duckdb_test/ruby_asan_test.rb diff --git a/.github/workflows/ruby_asan_on_ubuntu.yml b/.github/workflows/ruby_asan_on_ubuntu.yml new file mode 100644 index 00000000..83d0f250 --- /dev/null +++ b/.github/workflows/ruby_asan_on_ubuntu.yml @@ -0,0 +1,71 @@ +name: Ubuntu + +on: + push: + branches: + - main + pull_request: + types: + - opened + - synchronize + - reopened + +jobs: + test: + runs-on: ubuntu-latest + strategy: + matrix: + ruby: ['asan'] + duckdb: ['1.1.3', '1.1.1', '1.0.0'] + + steps: + - uses: actions/checkout@v4 + + - name: Set up Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: ${{ matrix.ruby }} + + - name: duckdb cache + id: duckdb-cache + uses: actions/cache@v4 + with: + path: duckdb-v${{ matrix.duckdb }} + key: ${{ runner.os }}-duckdb-v${{ matrix.duckdb }} + + - name: Build duckdb ${{ matrix.duckdb }} + env: + DUCKDB_VERSION: ${{ matrix.duckdb }} + if: steps.duckdb-cache.outputs.cache-hit != 'true' + run: | + git clone -b v$DUCKDB_VERSION https://github.com/cwida/duckdb.git duckdb-tmp-v$DUCKDB_VERSION + cd duckdb-tmp-v$DUCKDB_VERSION && make && cd .. + rm -rf duckdb-v$DUCKDB_VERSION + mkdir -p duckdb-v$DUCKDB_VERSION/build/release/src duckdb-v$DUCKDB_VERSION/src + cp -rip duckdb-tmp-v$DUCKDB_VERSION/build/release/src/*.so duckdb-v$DUCKDB_VERSION/build/release/src + cp -rip duckdb-tmp-v$DUCKDB_VERSION/src/include duckdb-v$DUCKDB_VERSION/src/ + + - name: bundle install with Ruby ${{ matrix.ruby }} + env: + DUCKDB_VERSION: ${{ matrix.duckdb }} + run: | + bundle install --jobs 4 --retry 3 + + - name: Build test with DUCKDB_API_NO_DEPRECATED and Ruby ${{ matrix.ruby }} + env: + DUCKDB_VERSION: ${{ matrix.duckdb }} + run: | + env DUCKDB_API_NO_DEPRECATED=1 bundle exec rake build -- --with-duckdb-include=${GITHUB_WORKSPACE}/duckdb-v${DUCKDB_VERSION}/src/include --with-duckdb-lib=${GITHUB_WORKSPACE}/duckdb-v${DUCKDB_VERSION}/build/release/src/ + bundle exec rake clean + + - name: Build with Ruby ${{ matrix.ruby }} + env: + DUCKDB_VERSION: ${{ matrix.duckdb }} + run: | + bundle exec rake build -- --with-duckdb-include=${GITHUB_WORKSPACE}/duckdb-v${DUCKDB_VERSION}/src/include --with-duckdb-lib=${GITHUB_WORKSPACE}/duckdb-v${DUCKDB_VERSION}/build/release/src/ + + - name: test with Ruby ${{ matrix.ruby }} + env: + DUCKDB_VERSION: ${{ matrix.duckdb }} + run: | + env RUBYOPT=-W:deprecated ruby -Ilib test/duckdb_test/ruby_asan_test.rb diff --git a/ext/duckdb/prepared_statement.c b/ext/duckdb/prepared_statement.c index e8ca553d..2961ff13 100644 --- a/ext/duckdb/prepared_statement.c +++ b/ext/duckdb/prepared_statement.c @@ -82,6 +82,8 @@ static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE q rubyDuckDBConnection *ctxcon; rubyDuckDBPreparedStatement *ctx; + duckdb_prepared_statement prepared_statement; + if (!rb_obj_is_kind_of(con, cDuckDBConnection)) { rb_raise(rb_eTypeError, "1st argument should be instance of DackDB::Connection"); } @@ -89,10 +91,11 @@ static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE q TypedData_Get_Struct(self, rubyDuckDBPreparedStatement, &prepared_statement_data_type, ctx); ctxcon = get_struct_connection(con); - if (duckdb_prepare(ctxcon->con, StringValuePtr(query), &(ctx->prepared_statement)) == DuckDBError) { - const char *error = duckdb_prepare_error(ctx->prepared_statement); + if (duckdb_prepare(ctxcon->con, StringValuePtr(query), &(prepared_statement)) == DuckDBError) { + const char *error = duckdb_prepare_error(prepared_statement); rb_raise(eDuckDBError, "%s", error ? error : "Failed to prepare statement(Database connection closed?)."); } + ctx->prepared_statement = prepared_statement; return self; } diff --git a/test/duckdb_test/ruby_asan_test.rb b/test/duckdb_test/ruby_asan_test.rb new file mode 100644 index 00000000..d21ac262 --- /dev/null +++ b/test/duckdb_test/ruby_asan_test.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true +require 'duckdb' + +def run_duckdb_asan_test + db = DuckDB::Database.open + con = db.connect + stmt = DuckDB::PreparedStatement.new(con, 'INSERT INTO test VALUES (?, "hello")') +rescue Exception => e + p e +end + +run_duckdb_asan_test From 64ac83647dd65f3da6ea1bfc5ad477331340d3dc Mon Sep 17 00:00:00 2001 From: Masaki Suketa Date: Sat, 25 Jan 2025 07:29:34 +0900 Subject: [PATCH 02/10] fix some code. - calling duckdb_destroy_prepare. - rescue StandardError. --- ext/duckdb/prepared_statement.c | 1 + test/duckdb_test/ruby_asan_test.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ext/duckdb/prepared_statement.c b/ext/duckdb/prepared_statement.c index 2961ff13..4145f2d6 100644 --- a/ext/duckdb/prepared_statement.c +++ b/ext/duckdb/prepared_statement.c @@ -93,6 +93,7 @@ static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE q if (duckdb_prepare(ctxcon->con, StringValuePtr(query), &(prepared_statement)) == DuckDBError) { const char *error = duckdb_prepare_error(prepared_statement); + duckdb_destroy_prepare(&prepared_statement); rb_raise(eDuckDBError, "%s", error ? error : "Failed to prepare statement(Database connection closed?)."); } ctx->prepared_statement = prepared_statement; diff --git a/test/duckdb_test/ruby_asan_test.rb b/test/duckdb_test/ruby_asan_test.rb index d21ac262..3524094b 100644 --- a/test/duckdb_test/ruby_asan_test.rb +++ b/test/duckdb_test/ruby_asan_test.rb @@ -4,8 +4,8 @@ def run_duckdb_asan_test db = DuckDB::Database.open con = db.connect - stmt = DuckDB::PreparedStatement.new(con, 'INSERT INTO test VALUES (?, "hello")') -rescue Exception => e + DuckDB::PreparedStatement.new(con, 'INSERT INTO test VALUES (?, "hello")') +rescue StandardError => e p e end From d9dd5bb676a92e0db76f00232a3668d9aa93ada5 Mon Sep 17 00:00:00 2001 From: Masaki Suketa Date: Sat, 25 Jan 2025 07:34:25 +0900 Subject: [PATCH 03/10] fix memory leak. --- ext/duckdb/prepared_statement.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/ext/duckdb/prepared_statement.c b/ext/duckdb/prepared_statement.c index 4145f2d6..e8ca553d 100644 --- a/ext/duckdb/prepared_statement.c +++ b/ext/duckdb/prepared_statement.c @@ -82,8 +82,6 @@ static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE q rubyDuckDBConnection *ctxcon; rubyDuckDBPreparedStatement *ctx; - duckdb_prepared_statement prepared_statement; - if (!rb_obj_is_kind_of(con, cDuckDBConnection)) { rb_raise(rb_eTypeError, "1st argument should be instance of DackDB::Connection"); } @@ -91,12 +89,10 @@ static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE q TypedData_Get_Struct(self, rubyDuckDBPreparedStatement, &prepared_statement_data_type, ctx); ctxcon = get_struct_connection(con); - if (duckdb_prepare(ctxcon->con, StringValuePtr(query), &(prepared_statement)) == DuckDBError) { - const char *error = duckdb_prepare_error(prepared_statement); - duckdb_destroy_prepare(&prepared_statement); + if (duckdb_prepare(ctxcon->con, StringValuePtr(query), &(ctx->prepared_statement)) == DuckDBError) { + const char *error = duckdb_prepare_error(ctx->prepared_statement); rb_raise(eDuckDBError, "%s", error ? error : "Failed to prepare statement(Database connection closed?)."); } - ctx->prepared_statement = prepared_statement; return self; } From 9637d9aeee044ee459edf5f8f0ac2ac8986751c0 Mon Sep 17 00:00:00 2001 From: Masaki Suketa Date: Sat, 25 Jan 2025 08:35:46 +0900 Subject: [PATCH 04/10] update --- ext/duckdb/prepared_statement.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ext/duckdb/prepared_statement.c b/ext/duckdb/prepared_statement.c index e8ca553d..87f9df6f 100644 --- a/ext/duckdb/prepared_statement.c +++ b/ext/duckdb/prepared_statement.c @@ -42,6 +42,7 @@ static const rb_data_type_t prepared_statement_data_type = { static void destroy_prepared_statement(rubyDuckDBPreparedStatement *p) { if (p->prepared_statement) { + fprintf(stderr, "destroy prepared statement\n"); duckdb_destroy_prepare(&(p->prepared_statement)); } } From e4944f05aa69208f81a7c853a6dc11e6b6049868 Mon Sep 17 00:00:00 2001 From: Masaki Suketa Date: Sat, 25 Jan 2025 08:36:48 +0900 Subject: [PATCH 05/10] Revert "update" This reverts commit 504a919e7e33cf1994bc763354d920e80d740a92. --- ext/duckdb/prepared_statement.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/duckdb/prepared_statement.c b/ext/duckdb/prepared_statement.c index 87f9df6f..e8ca553d 100644 --- a/ext/duckdb/prepared_statement.c +++ b/ext/duckdb/prepared_statement.c @@ -42,7 +42,6 @@ static const rb_data_type_t prepared_statement_data_type = { static void destroy_prepared_statement(rubyDuckDBPreparedStatement *p) { if (p->prepared_statement) { - fprintf(stderr, "destroy prepared statement\n"); duckdb_destroy_prepare(&(p->prepared_statement)); } } From a1f1c810f214851876f86cf47bbc5afc28639df5 Mon Sep 17 00:00:00 2001 From: Masaki Suketa Date: Sat, 25 Jan 2025 14:27:13 +0900 Subject: [PATCH 06/10] fix the DuckDB::PreparedStatement initialize function. --- ext/duckdb/prepared_statement.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/ext/duckdb/prepared_statement.c b/ext/duckdb/prepared_statement.c index e8ca553d..0416f3a9 100644 --- a/ext/duckdb/prepared_statement.c +++ b/ext/duckdb/prepared_statement.c @@ -81,6 +81,8 @@ VALUE rbduckdb_prepared_statement_new(duckdb_connection con, duckdb_extracted_st static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE query) { rubyDuckDBConnection *ctxcon; rubyDuckDBPreparedStatement *ctx; + duckdb_state state; + const char *error; if (!rb_obj_is_kind_of(con, cDuckDBConnection)) { rb_raise(rb_eTypeError, "1st argument should be instance of DackDB::Connection"); @@ -89,10 +91,20 @@ static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE q TypedData_Get_Struct(self, rubyDuckDBPreparedStatement, &prepared_statement_data_type, ctx); ctxcon = get_struct_connection(con); - if (duckdb_prepare(ctxcon->con, StringValuePtr(query), &(ctx->prepared_statement)) == DuckDBError) { - const char *error = duckdb_prepare_error(ctx->prepared_statement); + // Initialize to NULL before preparing + ctx->prepared_statement = NULL; + + // Try to prepare the statement and get the state + state = duckdb_prepare(ctxcon->con, StringValuePtr(query), &(ctx->prepared_statement)); + + // Get error message before any exception might be thrown + error = duckdb_prepare_error(ctx->prepared_statement); + + // If preparation failed, raise Ruby exception with the error message + if (state == DuckDBError) { rb_raise(eDuckDBError, "%s", error ? error : "Failed to prepare statement(Database connection closed?)."); } + return self; } From 704eb3eb7344b704412e13106fd904b6a04218ea Mon Sep 17 00:00:00 2001 From: Masaki Suketa Date: Sun, 2 Mar 2025 09:34:56 +0900 Subject: [PATCH 07/10] add duckdb 1.2.0, drop 1.0.0 --- .github/workflows/ruby_asan_on_ubuntu.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ruby_asan_on_ubuntu.yml b/.github/workflows/ruby_asan_on_ubuntu.yml index 83d0f250..12652c51 100644 --- a/.github/workflows/ruby_asan_on_ubuntu.yml +++ b/.github/workflows/ruby_asan_on_ubuntu.yml @@ -16,7 +16,7 @@ jobs: strategy: matrix: ruby: ['asan'] - duckdb: ['1.1.3', '1.1.1', '1.0.0'] + duckdb: ['1.1.3', '1.1.1', '1.2.0'] steps: - uses: actions/checkout@v4 From be68a17a8ec1fe1eadc9876d9284d9405fc73779 Mon Sep 17 00:00:00 2001 From: Masaki Suketa Date: Sun, 2 Mar 2025 09:42:09 +0900 Subject: [PATCH 08/10] change error handling --- ext/duckdb/prepared_statement.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/ext/duckdb/prepared_statement.c b/ext/duckdb/prepared_statement.c index 0416f3a9..0e91aa19 100644 --- a/ext/duckdb/prepared_statement.c +++ b/ext/duckdb/prepared_statement.c @@ -83,6 +83,7 @@ static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE q rubyDuckDBPreparedStatement *ctx; duckdb_state state; const char *error; + VALUE msg; if (!rb_obj_is_kind_of(con, cDuckDBConnection)) { rb_raise(rb_eTypeError, "1st argument should be instance of DackDB::Connection"); @@ -91,18 +92,13 @@ static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE q TypedData_Get_Struct(self, rubyDuckDBPreparedStatement, &prepared_statement_data_type, ctx); ctxcon = get_struct_connection(con); - // Initialize to NULL before preparing - ctx->prepared_statement = NULL; - - // Try to prepare the statement and get the state state = duckdb_prepare(ctxcon->con, StringValuePtr(query), &(ctx->prepared_statement)); - // Get error message before any exception might be thrown - error = duckdb_prepare_error(ctx->prepared_statement); - // If preparation failed, raise Ruby exception with the error message if (state == DuckDBError) { - rb_raise(eDuckDBError, "%s", error ? error : "Failed to prepare statement(Database connection closed?)."); + error = duckdb_prepare_error(ctx->prepared_statement); + msg = rb_str_new2(error ? error : "Failed to execute prepared statement."); + rb_raise(eDuckDBError, "%s", StringValuePtr(msg)); } return self; From ca25f9bd092ab538b8dd3ab7f5e5f773d316e746 Mon Sep 17 00:00:00 2001 From: Masaki Suketa Date: Sun, 2 Mar 2025 09:51:18 +0900 Subject: [PATCH 09/10] use StringValueCStr --- ext/duckdb/prepared_statement.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ext/duckdb/prepared_statement.c b/ext/duckdb/prepared_statement.c index 0e91aa19..fbeab38a 100644 --- a/ext/duckdb/prepared_statement.c +++ b/ext/duckdb/prepared_statement.c @@ -83,6 +83,7 @@ static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE q rubyDuckDBPreparedStatement *ctx; duckdb_state state; const char *error; + char *pquery; VALUE msg; if (!rb_obj_is_kind_of(con, cDuckDBConnection)) { @@ -92,13 +93,14 @@ static VALUE duckdb_prepared_statement_initialize(VALUE self, VALUE con, VALUE q TypedData_Get_Struct(self, rubyDuckDBPreparedStatement, &prepared_statement_data_type, ctx); ctxcon = get_struct_connection(con); - state = duckdb_prepare(ctxcon->con, StringValuePtr(query), &(ctx->prepared_statement)); + pquery = StringValueCStr(query); + state = duckdb_prepare(ctxcon->con, pquery, &(ctx->prepared_statement)); // If preparation failed, raise Ruby exception with the error message if (state == DuckDBError) { error = duckdb_prepare_error(ctx->prepared_statement); msg = rb_str_new2(error ? error : "Failed to execute prepared statement."); - rb_raise(eDuckDBError, "%s", StringValuePtr(msg)); + rb_raise(eDuckDBError, "%s", StringValueCStr(msg)); } return self; From 5646501fc193c54ceeb773ef120b8ae00ca8ee8d Mon Sep 17 00:00:00 2001 From: Masaki Suketa Date: Sun, 2 Mar 2025 10:02:37 +0900 Subject: [PATCH 10/10] check without exception --- test/duckdb_test/ruby_asan_test.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/duckdb_test/ruby_asan_test.rb b/test/duckdb_test/ruby_asan_test.rb index 3524094b..891c47f6 100644 --- a/test/duckdb_test/ruby_asan_test.rb +++ b/test/duckdb_test/ruby_asan_test.rb @@ -4,7 +4,9 @@ def run_duckdb_asan_test db = DuckDB::Database.open con = db.connect - DuckDB::PreparedStatement.new(con, 'INSERT INTO test VALUES (?, "hello")') + con.query('CREATE TABLE test (a INTEGER, b VARCHAR)') + DuckDB::PreparedStatement.new(con, 'INSERT INTO test VALUES (?, ?)') + # DuckDB::PreparedStatement.new(con, 'INSERT INTO test VALUES (?, "hello")') rescue StandardError => e p e end