From 959d8b14db5228f00da8c46a1a747a3c08445cd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Tue, 13 Jun 2023 20:58:29 +0200 Subject: [PATCH 01/12] Remove open transaction check --- src/DbConnection.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/DbConnection.cpp b/src/DbConnection.cpp index a84e328c..e8d9e389 100644 --- a/src/DbConnection.cpp +++ b/src/DbConnection.cpp @@ -128,7 +128,5 @@ void DbConnection::add_transaction() { void DbConnection::rem_transaction() { if (transaction_ > 0) { transaction_ -= 1; - } else { - cpp11::stop("Cannot remove non-existent transactions"); } } From e6aa675486722a8c85ccf8a6914b6e5ac06525fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Tue, 13 Jun 2023 23:43:22 +0200 Subject: [PATCH 02/12] Add a warning instead of an error. --- src/DbConnection.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/DbConnection.cpp b/src/DbConnection.cpp index e8d9e389..30a8f5ee 100644 --- a/src/DbConnection.cpp +++ b/src/DbConnection.cpp @@ -128,5 +128,7 @@ void DbConnection::add_transaction() { void DbConnection::rem_transaction() { if (transaction_ > 0) { transaction_ -= 1; + } else { + cpp11::warning("No transaction(s). Using DBI::dbCommit() or DBI::dbRollback() without a corresponding DBI::dbBegin()."); } } From 9f2dcf602642aeae7f46c14d51dc90dc03d227d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Wed, 14 Jun 2023 02:01:38 +0200 Subject: [PATCH 03/12] Querying backend directly for transaction status --- src/DbConnection.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/DbConnection.cpp b/src/DbConnection.cpp index 30a8f5ee..9db73e20 100644 --- a/src/DbConnection.cpp +++ b/src/DbConnection.cpp @@ -114,10 +114,12 @@ int DbConnection::busy_callback_helper(void *data, int num) } bool DbConnection::in_transaction() const { - if (transaction_ > 0) { - return true; - } else { + int status = sqlite3_txn_state(pConn_, NULL); + + if (status == SQLITE_TXN_NONE) { return false; + } else { + return true; } } From 8a8b479566427d9bae7fdeef5c5ef902e4e76b80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Wed, 14 Jun 2023 02:13:07 +0200 Subject: [PATCH 04/12] Clean code no longer in use --- R/dbBegin_SQLiteConnection.R | 1 - R/dbCommit_SQLiteConnection.R | 1 - R/dbRollback_SQLiteConnection.R | 1 - src/DbConnection.cpp | 15 +-------------- src/DbConnection.h | 3 --- src/connection.cpp | 11 ----------- 6 files changed, 1 insertion(+), 31 deletions(-) diff --git a/R/dbBegin_SQLiteConnection.R b/R/dbBegin_SQLiteConnection.R index 2e833300..0750f8ed 100644 --- a/R/dbBegin_SQLiteConnection.R +++ b/R/dbBegin_SQLiteConnection.R @@ -7,7 +7,6 @@ dbBegin_SQLiteConnection <- function(conn, .name = NULL, ..., name = NULL) { } else { dbExecute(conn, paste0("SAVEPOINT ", dbQuoteIdentifier(conn, name))) } - connection_add_transaction(conn@ptr) invisible(TRUE) } #' @rdname sqlite-transaction diff --git a/R/dbCommit_SQLiteConnection.R b/R/dbCommit_SQLiteConnection.R index c9163b29..fb9688d2 100644 --- a/R/dbCommit_SQLiteConnection.R +++ b/R/dbCommit_SQLiteConnection.R @@ -7,7 +7,6 @@ dbCommit_SQLiteConnection <- function(conn, .name = NULL, ..., name = NULL) { } else { dbExecute(conn, paste0("RELEASE SAVEPOINT ", dbQuoteIdentifier(conn, name))) } - connection_rem_transaction(conn@ptr) invisible(TRUE) } #' @rdname sqlite-transaction diff --git a/R/dbRollback_SQLiteConnection.R b/R/dbRollback_SQLiteConnection.R index e2df0a38..a71df601 100644 --- a/R/dbRollback_SQLiteConnection.R +++ b/R/dbRollback_SQLiteConnection.R @@ -15,7 +15,6 @@ dbRollback_SQLiteConnection <- function(conn, .name = NULL, ..., name = NULL) { dbExecute(conn, paste0("ROLLBACK TO ", name_quoted)) dbExecute(conn, paste0("RELEASE SAVEPOINT ", name_quoted)) } - connection_rem_transaction(conn@ptr) invisible(TRUE) } #' @rdname sqlite-transaction diff --git a/src/DbConnection.cpp b/src/DbConnection.cpp index 9db73e20..02f2cf9e 100644 --- a/src/DbConnection.cpp +++ b/src/DbConnection.cpp @@ -5,8 +5,7 @@ DbConnection::DbConnection(const std::string& path, const bool allow_ext, const int flags, const std::string& vfs, bool with_alt_types) : pConn_(NULL), with_alt_types_(with_alt_types), - busy_callback_(NULL), - transaction_(0) { + busy_callback_(NULL) { // Get the underlying database connection int rc = sqlite3_open_v2(path.c_str(), &pConn_, flags, vfs.empty() ? NULL : vfs.c_str()); @@ -122,15 +121,3 @@ bool DbConnection::in_transaction() const { return true; } } - -void DbConnection::add_transaction() { - transaction_ += 1; -} - -void DbConnection::rem_transaction() { - if (transaction_ > 0) { - transaction_ -= 1; - } else { - cpp11::warning("No transaction(s). Using DBI::dbCommit() or DBI::dbRollback() without a corresponding DBI::dbBegin()."); - } -} diff --git a/src/DbConnection.h b/src/DbConnection.h index aef9e4aa..9f070f79 100644 --- a/src/DbConnection.h +++ b/src/DbConnection.h @@ -53,8 +53,6 @@ class DbConnection : boost::noncopyable { void set_busy_handler(SEXP r_callback); bool in_transaction() const; - void add_transaction(); - void rem_transaction(); private: sqlite3* pConn_; @@ -62,7 +60,6 @@ class DbConnection : boost::noncopyable { SEXP busy_callback_; void release_callback_data(); static int busy_callback_helper(void *data, int num); - int transaction_; }; #endif // __RSQLITE_SQLITE_CONNECTION__ diff --git a/src/connection.cpp b/src/connection.cpp index 2cd1491c..203f885b 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -72,17 +72,6 @@ bool connection_in_transaction(cpp11::external_pointer con_){ DbConnectionPtr* con = con_.get(); return (con->get()->in_transaction() != 0); } -[[cpp11::register]] -void connection_add_transaction(cpp11::external_pointer con_){ - DbConnectionPtr* con = con_.get(); - con->get()->add_transaction(); -} -[[cpp11::register]] -void connection_rem_transaction(cpp11::external_pointer con_){ - DbConnectionPtr* con = con_.get(); - con->get()->rem_transaction(); -} - // Quoting From bf7777d03813be480dce69197748b95be88c49d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Wed, 14 Jun 2023 02:22:21 +0200 Subject: [PATCH 05/12] Deregister removed functions --- R/cpp11.R | 8 -------- src/cpp11.cpp | 18 ------------------ 2 files changed, 26 deletions(-) diff --git a/R/cpp11.R b/R/cpp11.R index 14e77816..eadf2e36 100644 --- a/R/cpp11.R +++ b/R/cpp11.R @@ -16,14 +16,6 @@ connection_in_transaction <- function(con_) { .Call(`_RSQLite_connection_in_transaction`, con_) } -connection_add_transaction <- function(con_) { - invisible(.Call(`_RSQLite_connection_add_transaction`, con_)) -} - -connection_rem_transaction <- function(con_) { - invisible(.Call(`_RSQLite_connection_rem_transaction`, con_)) -} - connection_copy_database <- function(from, to) { invisible(.Call(`_RSQLite_connection_copy_database`, from, to)) } diff --git a/src/cpp11.cpp b/src/cpp11.cpp index 5643dacb..8cbb18ae 100644 --- a/src/cpp11.cpp +++ b/src/cpp11.cpp @@ -35,22 +35,6 @@ extern "C" SEXP _RSQLite_connection_in_transaction(SEXP con_) { END_CPP11 } // connection.cpp -void connection_add_transaction(cpp11::external_pointer con_); -extern "C" SEXP _RSQLite_connection_add_transaction(SEXP con_) { - BEGIN_CPP11 - connection_add_transaction(cpp11::as_cpp>>(con_)); - return R_NilValue; - END_CPP11 -} -// connection.cpp -void connection_rem_transaction(cpp11::external_pointer con_); -extern "C" SEXP _RSQLite_connection_rem_transaction(SEXP con_) { - BEGIN_CPP11 - connection_rem_transaction(cpp11::as_cpp>>(con_)); - return R_NilValue; - END_CPP11 -} -// connection.cpp void connection_copy_database(const cpp11::external_pointer& from, const cpp11::external_pointer& to); extern "C" SEXP _RSQLite_connection_copy_database(SEXP from, SEXP to) { BEGIN_CPP11 @@ -171,13 +155,11 @@ extern "C" SEXP _RSQLite_init_logging(SEXP log_level) { extern "C" { static const R_CallMethodDef CallEntries[] = { - {"_RSQLite_connection_add_transaction", (DL_FUNC) &_RSQLite_connection_add_transaction, 1}, {"_RSQLite_connection_connect", (DL_FUNC) &_RSQLite_connection_connect, 5}, {"_RSQLite_connection_copy_database", (DL_FUNC) &_RSQLite_connection_copy_database, 2}, {"_RSQLite_connection_import_file", (DL_FUNC) &_RSQLite_connection_import_file, 6}, {"_RSQLite_connection_in_transaction", (DL_FUNC) &_RSQLite_connection_in_transaction, 1}, {"_RSQLite_connection_release", (DL_FUNC) &_RSQLite_connection_release, 1}, - {"_RSQLite_connection_rem_transaction", (DL_FUNC) &_RSQLite_connection_rem_transaction, 1}, {"_RSQLite_connection_valid", (DL_FUNC) &_RSQLite_connection_valid, 1}, {"_RSQLite_extension_load", (DL_FUNC) &_RSQLite_extension_load, 3}, {"_RSQLite_init_logging", (DL_FUNC) &_RSQLite_init_logging, 1}, From aa57a675c1c86ffc60facdbff05e20808b794ca5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Wed, 14 Jun 2023 02:24:53 +0200 Subject: [PATCH 06/12] Slightly change test suite A transaction is not reported until some I/O is actually attempted after dbBegin (or equivalent command). --- tests/testthat/test-transactions.R | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/tests/testthat/test-transactions.R b/tests/testthat/test-transactions.R index 3370a1e8..9829edca 100644 --- a/tests/testthat/test-transactions.R +++ b/tests/testthat/test-transactions.R @@ -28,8 +28,8 @@ test_that("commit unnamed transactions", { expect_false(sqliteIsTransacting(con)) dbBegin(con) - expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) expect_equal(dbListTables(con), "a") expect_equal(dbListTables(con2), character()) dbCommit(con) @@ -56,8 +56,8 @@ test_that("rollback unnamed transactions", { expect_false(sqliteIsTransacting(con)) dbBegin(con) - expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) expect_equal(dbListTables(con), "a") expect_equal(dbListTables(con2), character()) dbRollback(con) @@ -83,8 +83,9 @@ test_that("no nested unnamed transactions (commit after error)", { ) expect_false(sqliteIsTransacting(con)) dbBegin(con) - expect_true(sqliteIsTransacting(con)) + expect_false(sqliteIsTransacting(con)) expect_error(dbBegin(con)) + expect_false(sqliteIsTransacting(con)) dbCommit(con) expect_false(sqliteIsTransacting(con)) @@ -107,8 +108,9 @@ test_that("no nested unnamed transactions (rollback after error)", { expect_false(sqliteIsTransacting(con)) dbBegin(con) - expect_true(sqliteIsTransacting(con)) + expect_false(sqliteIsTransacting(con)) expect_error(dbBegin(con)) + expect_false(sqliteIsTransacting(con)) dbRollback(con) expect_false(sqliteIsTransacting(con)) @@ -131,8 +133,8 @@ test_that("commit named transactions", { expect_false(sqliteIsTransacting(con)) dbBegin(con, name = "tx") - expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) expect_equal(dbListTables(con), "a") expect_equal(dbListTables(con2), character()) dbCommit(con, name = "tx") @@ -159,8 +161,8 @@ test_that("rollback named transactions", { expect_false(sqliteIsTransacting(con)) dbBegin(con, name = "tx") - expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) expect_equal(dbListTables(con), "a") expect_equal(dbListTables(con2), character()) @@ -188,8 +190,8 @@ test_that("nested named transactions (commit - commit)", { expect_false(sqliteIsTransacting(con)) dbBegin(con, name = "tx") - expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) expect_equal(dbListTables(con), "a") expect_equal(dbListTables(con2), character()) @@ -231,8 +233,8 @@ test_that("nested named transactions (commit - rollback)", { expect_false(sqliteIsTransacting(con)) dbBegin(con, name = "tx") - expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) expect_equal(dbListTables(con), "a") expect_equal(dbListTables(con2), character()) @@ -274,8 +276,8 @@ test_that("nested named transactions (rollback - commit)", { expect_false(sqliteIsTransacting(con)) dbBegin(con, name = "tx") - expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) expect_equal(dbListTables(con), "a") expect_equal(dbListTables(con2), character()) @@ -317,8 +319,8 @@ test_that("nested named transactions (rollback - rollback)", { expect_false(sqliteIsTransacting(con)) dbBegin(con, name = "tx") - expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) expect_equal(dbListTables(con), "a") expect_equal(dbListTables(con2), character()) @@ -360,8 +362,8 @@ test_that("named transactions with keywords", { expect_false(sqliteIsTransacting(con)) dbBegin(con, name = "SELECT") - expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) expect_equal(dbListTables(con), "a") expect_equal(dbListTables(con2), character()) dbCommit(con, name = "SELECT") @@ -370,8 +372,8 @@ test_that("named transactions with keywords", { expect_equal(dbListTables(con2), "a") dbBegin(con, name = "WHERE") - expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "b", data.frame(b = 1)) + expect_true(sqliteIsTransacting(con)) expect_equal(dbListTables(con), c("a", "b")) expect_equal(dbListTables(con2), "a") dbRollback(con, name = "WHERE") From 8ad3c850e4a4099e2f09ef40502a3fb77893361a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Wed, 14 Jun 2023 02:25:24 +0200 Subject: [PATCH 07/12] Update docs --- R/transactions.R | 3 ++- man/sqlite-transaction.Rd | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/R/transactions.R b/R/transactions.R index 24674b3d..416ebe33 100644 --- a/R/transactions.R +++ b/R/transactions.R @@ -10,7 +10,8 @@ NULL #' [DBI::dbWithTransaction()] is a convenient wrapper that makes sure that #' `dbCommit()` or `dbRollback()` is called. #' A helper function `sqliteIsTransacting()` is available to check the current -#' transaction status of the connection. +#' transaction status of the connection. Note that a transaction will not be +#' reported until a read/write is attempted after starting a transaction. #' #' @seealso #' The corresponding generic functions [DBI::dbBegin()], [DBI::dbCommit()], diff --git a/man/sqlite-transaction.Rd b/man/sqlite-transaction.Rd index 10685140..d3bf9320 100644 --- a/man/sqlite-transaction.Rd +++ b/man/sqlite-transaction.Rd @@ -39,8 +39,9 @@ a SQLite transaction and turns auto-commit off. \code{dbCommit()} and auto-commit on. \code{\link[DBI:dbWithTransaction]{DBI::dbWithTransaction()}} is a convenient wrapper that makes sure that \code{dbCommit()} or \code{dbRollback()} is called. -An helper function \code{sqliteIsTransacting()} is available to check the current -transaction status of the connection. +A helper function \code{sqliteIsTransacting()} is available to check the current +transaction status of the connection. Note that a transaction will not be +reported until a read/write is attempted after starting a transaction. } \examples{ library(DBI) From 6ce559847ec00023d1bc5e18c24a11a56748742c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Wed, 14 Jun 2023 20:24:25 +0200 Subject: [PATCH 08/12] Add new testing to the suite --- tests/testthat/test-transactions.R | 114 +++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/tests/testthat/test-transactions.R b/tests/testthat/test-transactions.R index 9829edca..1c8110b3 100644 --- a/tests/testthat/test-transactions.R +++ b/tests/testthat/test-transactions.R @@ -381,3 +381,117 @@ test_that("named transactions with keywords", { expect_equal(dbListTables(con), "a") expect_equal(dbListTables(con2), "a") }) + +test_that("transactions managed without dbBegin+dbCommit", { + db_file <- tempfile("transactions", fileext = ".sqlite") + con <- dbConnect(SQLite(), db_file) + on.exit(dbDisconnect(con)) + + expect_false(sqliteIsTransacting(con)) + dbExecute(con, "BEGIN") + expect_false(sqliteIsTransacting(con)) + dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "END") + expect_false(sqliteIsTransacting(con)) + + dbExecute(con, "BEGIN") + expect_false(sqliteIsTransacting(con)) + dbWriteTable(con, "b", data.frame(b = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "COMMIT") + expect_false(sqliteIsTransacting(con)) + + expect_false(sqliteIsTransacting(con)) + dbExecute(con, "BEGIN IMMEDIATE") + expect_true(sqliteIsTransacting(con)) + dbWriteTable(con, "c", data.frame(c = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "END") + expect_false(sqliteIsTransacting(con)) + + dbExecute(con, "BEGIN IMMEDIATE") + expect_true(sqliteIsTransacting(con)) + dbWriteTable(con, "d", data.frame(d = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "COMMIT") + expect_false(sqliteIsTransacting(con)) +}) + +test_that("transactions managed without dbBegin+dbRollback", { + db_file <- tempfile("transactions", fileext = ".sqlite") + con <- dbConnect(SQLite(), db_file) + on.exit(dbDisconnect(con)) + + expect_false(sqliteIsTransacting(con)) + dbExecute(con, "BEGIN") + expect_false(sqliteIsTransacting(con)) + dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "ROLLBACK") + expect_false(sqliteIsTransacting(con)) + + dbExecute(con, "BEGIN IMMEDIATE") + expect_true(sqliteIsTransacting(con)) + dbWriteTable(con, "b", data.frame(b = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "ROLLBACK") + expect_false(sqliteIsTransacting(con)) +}) + +test_that("mixed management of transactions", { + db_file <- tempfile("transactions", fileext = ".sqlite") + con <- dbConnect(SQLite(), db_file) + on.exit(dbDisconnect(con)) + + expect_false(sqliteIsTransacting(con)) + dbExecute(con, "BEGIN") + expect_false(sqliteIsTransacting(con)) + dbWriteTable(con, "a", data.frame(a = 1)) + expect_true(sqliteIsTransacting(con)) + dbCommit(con) + expect_false(sqliteIsTransacting(con)) + + dbExecute(con, "BEGIN IMMEDIATE") + expect_true(sqliteIsTransacting(con)) + dbWriteTable(con, "b", data.frame(b = 1)) + expect_true(sqliteIsTransacting(con)) + dbCommit(con) + expect_false(sqliteIsTransacting(con)) + + expect_false(sqliteIsTransacting(con)) + dbExecute(con, "BEGIN") + expect_false(sqliteIsTransacting(con)) + dbWriteTable(con, "c", data.frame(c = 1)) + expect_true(sqliteIsTransacting(con)) + dbRollback(con) + expect_false(sqliteIsTransacting(con)) + + dbExecute(con, "BEGIN IMMEDIATE") + expect_true(sqliteIsTransacting(con)) + dbWriteTable(con, "d", data.frame(d = 1)) + expect_true(sqliteIsTransacting(con)) + dbRollback(con) + expect_false(sqliteIsTransacting(con)) + + dbBegin(con) + expect_false(sqliteIsTransacting(con)) + dbWriteTable(con, "e", data.frame(e = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "COMMIT") + expect_false(sqliteIsTransacting(con)) + + dbBegin(con) + expect_false(sqliteIsTransacting(con)) + dbWriteTable(con, "f", data.frame(f = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "END") + expect_false(sqliteIsTransacting(con)) + + dbBegin(con) + expect_false(sqliteIsTransacting(con)) + dbWriteTable(con, "g", data.frame(g = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "ROLLBACK") + expect_false(sqliteIsTransacting(con)) +}) From 02dceabb10941b2532096db8b663634b390d589e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Wed, 14 Jun 2023 20:27:18 +0200 Subject: [PATCH 09/12] Improved docs --- R/transactions.R | 5 +++-- man/sqlite-transaction.Rd | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/R/transactions.R b/R/transactions.R index 416ebe33..1cfcb9d1 100644 --- a/R/transactions.R +++ b/R/transactions.R @@ -10,8 +10,9 @@ NULL #' [DBI::dbWithTransaction()] is a convenient wrapper that makes sure that #' `dbCommit()` or `dbRollback()` is called. #' A helper function `sqliteIsTransacting()` is available to check the current -#' transaction status of the connection. Note that a transaction will not be -#' reported until a read/write is attempted after starting a transaction. +#' transaction status of the connection. Note that by default after `dbBegin` +#' SQLite defers the actual transaction start until a read/write is attempted +#' on the connection, therefore this function may not immediately return True. #' #' @seealso #' The corresponding generic functions [DBI::dbBegin()], [DBI::dbCommit()], diff --git a/man/sqlite-transaction.Rd b/man/sqlite-transaction.Rd index d3bf9320..1ddb17fe 100644 --- a/man/sqlite-transaction.Rd +++ b/man/sqlite-transaction.Rd @@ -40,8 +40,9 @@ auto-commit on. \code{\link[DBI:dbWithTransaction]{DBI::dbWithTransaction()}} is a convenient wrapper that makes sure that \code{dbCommit()} or \code{dbRollback()} is called. A helper function \code{sqliteIsTransacting()} is available to check the current -transaction status of the connection. Note that a transaction will not be -reported until a read/write is attempted after starting a transaction. +transaction status of the connection. Note that by default after \code{dbBegin} +SQLite defers the actual transaction start until a read/write is attempted +on the connection, therefore this function may not immediately return True. } \examples{ library(DBI) From 1873e57196cd69275daa2d6df1348d7f2a157054 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Mon, 19 Jun 2023 19:29:13 +0200 Subject: [PATCH 10/12] Look for autocommit status instead of transaction status. --- src/DbConnection.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/DbConnection.cpp b/src/DbConnection.cpp index 02f2cf9e..cac24ec3 100644 --- a/src/DbConnection.cpp +++ b/src/DbConnection.cpp @@ -113,11 +113,11 @@ int DbConnection::busy_callback_helper(void *data, int num) } bool DbConnection::in_transaction() const { - int status = sqlite3_txn_state(pConn_, NULL); + int status = sqlite3_get_autocommit(pConn_); - if (status == SQLITE_TXN_NONE) { - return false; - } else { + if (status == 0) { return true; + } else { + return false; } } From eb2c70c8a8e968a500f89b6a4e774dcdf2418ec8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Mon, 19 Jun 2023 19:32:15 +0200 Subject: [PATCH 11/12] Update docs --- R/transactions.R | 4 +--- man/sqlite-transaction.Rd | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/R/transactions.R b/R/transactions.R index 1cfcb9d1..24674b3d 100644 --- a/R/transactions.R +++ b/R/transactions.R @@ -10,9 +10,7 @@ NULL #' [DBI::dbWithTransaction()] is a convenient wrapper that makes sure that #' `dbCommit()` or `dbRollback()` is called. #' A helper function `sqliteIsTransacting()` is available to check the current -#' transaction status of the connection. Note that by default after `dbBegin` -#' SQLite defers the actual transaction start until a read/write is attempted -#' on the connection, therefore this function may not immediately return True. +#' transaction status of the connection. #' #' @seealso #' The corresponding generic functions [DBI::dbBegin()], [DBI::dbCommit()], diff --git a/man/sqlite-transaction.Rd b/man/sqlite-transaction.Rd index 1ddb17fe..bcddfc62 100644 --- a/man/sqlite-transaction.Rd +++ b/man/sqlite-transaction.Rd @@ -40,9 +40,7 @@ auto-commit on. \code{\link[DBI:dbWithTransaction]{DBI::dbWithTransaction()}} is a convenient wrapper that makes sure that \code{dbCommit()} or \code{dbRollback()} is called. A helper function \code{sqliteIsTransacting()} is available to check the current -transaction status of the connection. Note that by default after \code{dbBegin} -SQLite defers the actual transaction start until a read/write is attempted -on the connection, therefore this function may not immediately return True. +transaction status of the connection. } \examples{ library(DBI) From 36072e0fbe989eee205579280a70b1bfeff6a8dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Gon=C3=A7alves?= Date: Mon, 19 Jun 2023 19:40:00 +0200 Subject: [PATCH 12/12] Change test suite Delayed flagging of transaction starting is no longer to be expected. --- tests/testthat/test-transactions.R | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/testthat/test-transactions.R b/tests/testthat/test-transactions.R index 1c8110b3..18050156 100644 --- a/tests/testthat/test-transactions.R +++ b/tests/testthat/test-transactions.R @@ -83,9 +83,9 @@ test_that("no nested unnamed transactions (commit after error)", { ) expect_false(sqliteIsTransacting(con)) dbBegin(con) - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) expect_error(dbBegin(con)) - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) dbCommit(con) expect_false(sqliteIsTransacting(con)) @@ -108,9 +108,9 @@ test_that("no nested unnamed transactions (rollback after error)", { expect_false(sqliteIsTransacting(con)) dbBegin(con) - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) expect_error(dbBegin(con)) - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) dbRollback(con) expect_false(sqliteIsTransacting(con)) @@ -389,14 +389,14 @@ test_that("transactions managed without dbBegin+dbCommit", { expect_false(sqliteIsTransacting(con)) dbExecute(con, "BEGIN") - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) expect_true(sqliteIsTransacting(con)) dbExecute(con, "END") expect_false(sqliteIsTransacting(con)) dbExecute(con, "BEGIN") - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "b", data.frame(b = 1)) expect_true(sqliteIsTransacting(con)) dbExecute(con, "COMMIT") @@ -425,7 +425,7 @@ test_that("transactions managed without dbBegin+dbRollback", { expect_false(sqliteIsTransacting(con)) dbExecute(con, "BEGIN") - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) expect_true(sqliteIsTransacting(con)) dbExecute(con, "ROLLBACK") @@ -446,7 +446,7 @@ test_that("mixed management of transactions", { expect_false(sqliteIsTransacting(con)) dbExecute(con, "BEGIN") - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "a", data.frame(a = 1)) expect_true(sqliteIsTransacting(con)) dbCommit(con) @@ -461,7 +461,7 @@ test_that("mixed management of transactions", { expect_false(sqliteIsTransacting(con)) dbExecute(con, "BEGIN") - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "c", data.frame(c = 1)) expect_true(sqliteIsTransacting(con)) dbRollback(con) @@ -475,21 +475,21 @@ test_that("mixed management of transactions", { expect_false(sqliteIsTransacting(con)) dbBegin(con) - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "e", data.frame(e = 1)) expect_true(sqliteIsTransacting(con)) dbExecute(con, "COMMIT") expect_false(sqliteIsTransacting(con)) dbBegin(con) - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "f", data.frame(f = 1)) expect_true(sqliteIsTransacting(con)) dbExecute(con, "END") expect_false(sqliteIsTransacting(con)) dbBegin(con) - expect_false(sqliteIsTransacting(con)) + expect_true(sqliteIsTransacting(con)) dbWriteTable(con, "g", data.frame(g = 1)) expect_true(sqliteIsTransacting(con)) dbExecute(con, "ROLLBACK")