Feature/interruptible execution clean2 (#796)

* odbc_result: re-org execution code-path
* feature: interruptible execution
* ci: mysql: a more up-to-date driver
* back to exceptions from nanodbc; pretty printed after caught
* code-review: feature controlled by global option odbc.interruptible
* code-review: Note about mysql driver and NEWS entry
* code-review: enable by default only in interactive sessions
* fixup: unit tests are run with interruptible execution enabled

---------

Co-authored-by: simonpcouch <[email protected]>

.github/odbc/install-mariadb-driver.sh

@@ -0,0 +1,11 @@
+# See #796 for some reasoning behind not using the canned mariadb-odbc-driver (v3.1.15), at time of writing
+# and needing to install v3.1.17 from source
+sudo apt-get install -y cmake
+cd /tmp && git clone https://github.com/MariaDB/mariadb-connector-odbc.git connector
+cd connector && git checkout 3.1.17
+mkdir build && cd build
+cmake ../ -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCONC_WITH_UNIT_TESTS=Off -DCMAKE_INSTALL_PREFIX=/usr/local -DWITH_SSL=OPENSSL
+cmake --build . --config RelWithDebInfo
+sudo make install
+sudo ln -s /usr/local/lib/mariadb/libmariadb.so.3 /usr/local/lib/
+sudo ldconfig

.github/odbc/odbcinst.ini

@@ -7,7 +7,7 @@ FileUsage=1
 Threading=2
 
 [MySQL Driver]
-Driver=/usr/lib/x86_64-linux-gnu/odbc/libmaodbc.so
+Driver=/usr/local/lib/mariadb/libmaodbc.so
 UsageCount = 1
 Threading=2
 

.github/workflows/db-windows.yml

@@ -93,5 +93,5 @@ jobs:
 
       - name: Test
         run: |
-          testthat::test_local(reporter = testthat::ProgressReporter$new(max_failures = Inf, update_interval = Inf))
+          options("odbc.interruptible"=TRUE);testthat::test_local(reporter = testthat::ProgressReporter$new(max_failures = Inf, update_interval = Inf))
         shell: Rscript {0}

.github/workflows/db.yaml

@@ -62,7 +62,7 @@ jobs:
         run: |
           sudo systemctl start mysql.service
           mysql -uroot -h127.0.0.1 -proot -e 'CREATE DATABASE `test`;'
-          sudo apt-get install -y odbc-mariadb
+          .github/odbc/install-mariadb-driver.sh
           echo "ODBC_CS_MYSQL=dsn=MySQL" >> $GITHUB_ENV
 
       - name: Install SQLite Driver
@@ -118,5 +118,5 @@ jobs:
 
       - name: Test
         run: |
-          testthat::test_local(reporter = testthat::ProgressReporter$new(max_failures = Inf, update_interval = Inf))
+          options("odbc.interruptible"=TRUE);testthat::test_local(reporter = testthat::ProgressReporter$new(max_failures = Inf, update_interval = Inf))
         shell: Rscript {0}

NEWS.md

@@ -1,5 +1,12 @@
 # odbc (development version)
 
+* Long running queries can now be interrupted using Ctrl-C.  This
+  feature is enabled by default in interactive sessions.  It can be
+  controlled by the `interruptible` argument to `dbConnect`, or by the
+  global option `odbc.interruptible`.  Should be considered experimental -
+  if you experience problems please file an issue on the package github
+  repository (#796).
+
 * Raises "Cancelling previous query" warnings from R rather than from Rcpp when 
   a connection has a current result to avoid possible incorrect resource 
   unwinds with `options(warn = 2)` (#797).

R/RcppExports.R

@@ -9,8 +9,8 @@ list_data_sources_ <- function() {
     .Call(`_odbc_list_data_sources_`)
 }
 
-odbc_connect <- function(connection_string, timezone = "", timezone_out = "", encoding = "", bigint = 0L, timeout = 0L, r_attributes_ = NULL) {
-    .Call(`_odbc_odbc_connect`, connection_string, timezone, timezone_out, encoding, bigint, timeout, r_attributes_)
+odbc_connect <- function(connection_string, timezone = "", timezone_out = "", encoding = "", bigint = 0L, timeout = 0L, r_attributes = NULL, interruptible_execution = TRUE) {
+    .Call(`_odbc_odbc_connect`, connection_string, timezone, timezone_out, encoding, bigint, timeout, r_attributes, interruptible_execution)
 }
 
 has_result <- function(p) {
@@ -105,10 +105,6 @@ result_bind <- function(r, params, batch_rows) {
     invisible(.Call(`_odbc_result_bind`, r, params, batch_rows))
 }
 
-result_execute <- function(r) {
-    invisible(.Call(`_odbc_result_execute`, r))
-}
-
 result_insert_dataframe <- function(r, df, batch_rows) {
     invisible(.Call(`_odbc_result_insert_dataframe`, r, df, batch_rows))
 }

R/dbi-driver.R

@@ -63,8 +63,13 @@ setMethod("show", "OdbcDriver",
 #'   name for the OdbcConnection object returned from [dbConnect()]. However, if
 #'   the driver does not return a valid value, it can be set manually with this
 #'   parameter.
-#' @param attributes An S4 object of connection attributes that are passed
+#' @param attributes A list of connection attributes that are passed
 #'   prior to the connection being established. See \link{ConnectionAttributes}.
+#' @param interruptible Logical.  If `TRUE` calls to `SQLExecute` and
+#'   `SQLExecuteDirect` can be interrupted when the user sends SIGINT ( ctrl-c ).
+#'   Otherwise, they block.  Defaults to `TRUE` in interactive sessions, and
+#'   `FALSE` otherwise.  It can be set explicitly either by manipulating this
+#'   argument, or by setting the global option `odbc.interruptible`.
 #' @param ... Additional ODBC keywords. These will be joined with the other
 #'   arguments to form the final connection string.
 #'
@@ -167,6 +172,7 @@ setMethod("dbConnect", "OdbcDriver",
       pwd = NULL,
       dbms.name = NULL,
       attributes = NULL,
+      interruptible = getOption("odbc.interruptible", interactive()),
       .connection_string = NULL) {
     check_string(dsn, allow_null = TRUE)
     check_string(timezone, allow_null = TRUE)
@@ -180,6 +186,7 @@ setMethod("dbConnect", "OdbcDriver",
     check_string(uid, allow_null = TRUE)
     check_string(pwd, allow_null = TRUE)
     check_string(dbms.name, allow_null = TRUE)
+    check_bool(interruptible)
 
     con <- OdbcConnection(
       dsn = dsn,
@@ -196,6 +203,7 @@ setMethod("dbConnect", "OdbcDriver",
       pwd = pwd,
       dbms.name = dbms.name,
       attributes = attributes,
+      interruptible = interruptible,
       .connection_string = .connection_string
     )
 

R/odbc-connection.R

@@ -14,6 +14,7 @@ OdbcConnection <- function(
     timeout = Inf,
     dbms.name = NULL,
     attributes = NULL,
+    interruptible = getOption("odbc.interruptible", interactive()),
     .connection_string = NULL,
     call = caller_env(2)
 ) {
@@ -37,7 +38,8 @@ OdbcConnection <- function(
       encoding = encoding,
       bigint = bigint,
       timeout = timeout,
-      r_attributes_ = attributes
+      r_attributes = attributes,
+      interruptible_execution = interruptible
     ),
     error = function(cnd) {
       check_quoting(args)

src/RcppExports.cpp

@@ -32,8 +32,8 @@ BEGIN_RCPP
 END_RCPP
 }
 // odbc_connect
-connection_ptr odbc_connect(std::string const& connection_string, std::string const& timezone, std::string const& timezone_out, std::string const& encoding, int bigint, long timeout, Rcpp::Nullable<Rcpp::List> const& r_attributes_);
-RcppExport SEXP _odbc_odbc_connect(SEXP connection_stringSEXP, SEXP timezoneSEXP, SEXP timezone_outSEXP, SEXP encodingSEXP, SEXP bigintSEXP, SEXP timeoutSEXP, SEXP r_attributes_SEXP) {
+connection_ptr odbc_connect(std::string const& connection_string, std::string const& timezone, std::string const& timezone_out, std::string const& encoding, int bigint, long timeout, Rcpp::Nullable<Rcpp::List> const& r_attributes, bool const& interruptible_execution);
+RcppExport SEXP _odbc_odbc_connect(SEXP connection_stringSEXP, SEXP timezoneSEXP, SEXP timezone_outSEXP, SEXP encodingSEXP, SEXP bigintSEXP, SEXP timeoutSEXP, SEXP r_attributesSEXP, SEXP interruptible_executionSEXP) {
 BEGIN_RCPP
     Rcpp::RObject rcpp_result_gen;
     Rcpp::RNGScope rcpp_rngScope_gen;
@@ -43,8 +43,9 @@ BEGIN_RCPP
     Rcpp::traits::input_parameter< std::string const& >::type encoding(encodingSEXP);
     Rcpp::traits::input_parameter< int >::type bigint(bigintSEXP);
     Rcpp::traits::input_parameter< long >::type timeout(timeoutSEXP);
-    Rcpp::traits::input_parameter< Rcpp::Nullable<Rcpp::List> const& >::type r_attributes_(r_attributes_SEXP);
-    rcpp_result_gen = Rcpp::wrap(odbc_connect(connection_string, timezone, timezone_out, encoding, bigint, timeout, r_attributes_));
+    Rcpp::traits::input_parameter< Rcpp::Nullable<Rcpp::List> const& >::type r_attributes(r_attributesSEXP);
+    Rcpp::traits::input_parameter< bool const& >::type interruptible_execution(interruptible_executionSEXP);
+    rcpp_result_gen = Rcpp::wrap(odbc_connect(connection_string, timezone, timezone_out, encoding, bigint, timeout, r_attributes, interruptible_execution));
     return rcpp_result_gen;
 END_RCPP
 }
@@ -306,16 +307,6 @@ BEGIN_RCPP
     return R_NilValue;
 END_RCPP
 }
-// result_execute
-void result_execute(result_ptr const& r);
-RcppExport SEXP _odbc_result_execute(SEXP rSEXP) {
-BEGIN_RCPP
-    Rcpp::RNGScope rcpp_rngScope_gen;
-    Rcpp::traits::input_parameter< result_ptr const& >::type r(rSEXP);
-    result_execute(r);
-    return R_NilValue;
-END_RCPP
-}
 // result_insert_dataframe
 void result_insert_dataframe(result_ptr const& r, DataFrame const& df, size_t batch_rows);
 RcppExport SEXP _odbc_result_insert_dataframe(SEXP rSEXP, SEXP dfSEXP, SEXP batch_rowsSEXP) {
@@ -375,7 +366,7 @@ END_RCPP
 static const R_CallMethodDef CallEntries[] = {
     {"_odbc_list_drivers_", (DL_FUNC) &_odbc_list_drivers_, 0},
     {"_odbc_list_data_sources_", (DL_FUNC) &_odbc_list_data_sources_, 0},
-    {"_odbc_odbc_connect", (DL_FUNC) &_odbc_odbc_connect, 7},
+    {"_odbc_odbc_connect", (DL_FUNC) &_odbc_odbc_connect, 8},
     {"_odbc_has_result", (DL_FUNC) &_odbc_has_result, 1},
     {"_odbc_connection_info", (DL_FUNC) &_odbc_connection_info, 1},
     {"_odbc_connection_quote", (DL_FUNC) &_odbc_connection_quote, 1},
@@ -399,7 +390,6 @@ static const R_CallMethodDef CallEntries[] = {
     {"_odbc_result_fetch", (DL_FUNC) &_odbc_result_fetch, 2},
     {"_odbc_result_column_info", (DL_FUNC) &_odbc_result_column_info, 1},
     {"_odbc_result_bind", (DL_FUNC) &_odbc_result_bind, 3},
-    {"_odbc_result_execute", (DL_FUNC) &_odbc_result_execute, 1},
     {"_odbc_result_insert_dataframe", (DL_FUNC) &_odbc_result_insert_dataframe, 3},
     {"_odbc_result_describe_parameters", (DL_FUNC) &_odbc_result_describe_parameters, 2},
     {"_odbc_result_rows_affected", (DL_FUNC) &_odbc_result_rows_affected, 1},

src/connection.cpp

@@ -53,7 +53,8 @@ connection_ptr odbc_connect(
     std::string const& encoding = "",
     int bigint = 0,
     long timeout = 0,
-    Rcpp::Nullable<Rcpp::List> const& r_attributes_ = R_NilValue) {
+    Rcpp::Nullable<Rcpp::List> const& r_attributes = R_NilValue,
+    bool const& interruptible_execution = true) {
   return connection_ptr(
       new std::shared_ptr<odbc_connection>(new odbc_connection(
           connection_string,
@@ -62,7 +63,8 @@ connection_ptr odbc_connect(
           encoding,
           static_cast<bigint_map_t>(bigint),
           timeout,
-          r_attributes_)));
+          r_attributes,
+          interruptible_execution)));
 }
 
 std::string get_info_or_empty(connection_ptr const& p, short type) {

src/nanodbc/nanodbc.cpp

@@ -505,19 +505,13 @@ const std::string database_error::state() const NANODBC_NOEXCEPT
     return sql_state;
 }
 
-void database_error::rethrow() {
-  Rcpp::Environment pkg = Rcpp::Environment::namespace_env("odbc");
-  Rcpp::Function rethrow_database_error = pkg["rethrow_database_error"];
-  rethrow_database_error(message);
-}
-
 } // namespace nanodbc
 
 // Throwing exceptions using NANODBC_THROW_DATABASE_ERROR enables file name
 // and line numbers to be inserted into the error message. Useful for debugging.
 #define NANODBC_THROW_DATABASE_ERROR(handle, handle_type)                                          \
-    nanodbc::database_error(                                                                 \
-        handle, handle_type, __FILE__ ":" NANODBC_STRINGIZE(__LINE__) ": ").rethrow() /**/
+    throw nanodbc::database_error(                                                                 \
+        handle, handle_type, __FILE__ ":" NANODBC_STRINGIZE(__LINE__) ": ") /**/
 
 // clang-format off
 // 8888888b.           888             d8b 888

src/nanodbc/nanodbc.h

@@ -267,7 +267,6 @@ class database_error : public std::runtime_error
     const char* what() const NANODBC_NOEXCEPT;
     long native() const NANODBC_NOEXCEPT;
     const std::string state() const NANODBC_NOEXCEPT;
-    void rethrow();
 
 private:
     long native_error;

src/odbc_connection.cpp

@@ -33,11 +33,13 @@ odbc_connection::odbc_connection(
     std::string encoding,
     bigint_map_t bigint_mapping,
     long timeout,
-    Rcpp::Nullable<Rcpp::List> const& r_attributes_)
+    Rcpp::Nullable<Rcpp::List> const& r_attributes,
+    bool const& interruptible_execution)
     : current_result_(nullptr),
       timezone_out_str_(timezone_out),
       encoding_(encoding),
-      bigint_mapping_(bigint_mapping) {
+      bigint_mapping_(bigint_mapping),
+      interruptible_execution_(interruptible_execution) {
 
   if (!cctz::load_time_zone(timezone, &timezone_)) {
     Rcpp::stop("Error loading time zone (%s)", timezone);
@@ -53,10 +55,11 @@ odbc_connection::odbc_connection(
     std::list< nanodbc::connection::attribute > attributes;
     std::list< std::shared_ptr< void > > buffer_context;
     utils::prepare_connection_attributes(
-        timeout, r_attributes_, attributes, buffer_context );
+        timeout, r_attributes, attributes, buffer_context );
     c_ = std::make_shared<nanodbc::connection>(connection_string, attributes);
   } catch (const nanodbc::database_error& e) {
-    throw Rcpp::exception(e.what(), FALSE);
+    Iconv encoder(this->encoding(), "UTF-8");
+    utils::raise_error(odbc_error(e, "", encoder));
   }
 }
 

src/odbc_connection.h

@@ -26,7 +26,8 @@ class odbc_connection {
       std::string encoding = "",
       bigint_map_t bigint_mapping = i64_to_integer64,
       long timeout = 0,
-      Rcpp::Nullable<Rcpp::List> const& r_attributes_ = R_NilValue);
+      Rcpp::Nullable<Rcpp::List> const& r_attributes = R_NilValue,
+      bool const& interruptible_execution = true);
 
   std::shared_ptr<nanodbc::connection> connection() const;
 
@@ -57,5 +58,6 @@ class odbc_connection {
   std::string timezone_out_str_;
   std::string encoding_;
   bigint_map_t bigint_mapping_;
+  bool interruptible_execution_;
 };
 } // namespace odbc