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/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/man/sqlite-transaction.Rd b/man/sqlite-transaction.Rd index 10685140..bcddfc62 100644 --- a/man/sqlite-transaction.Rd +++ b/man/sqlite-transaction.Rd @@ -39,7 +39,7 @@ 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 +A helper function \code{sqliteIsTransacting()} is available to check the current transaction status of the connection. } \examples{ diff --git a/src/DbConnection.cpp b/src/DbConnection.cpp index a84e328c..cac24ec3 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()); @@ -114,21 +113,11 @@ int DbConnection::busy_callback_helper(void *data, int num) } bool DbConnection::in_transaction() const { - if (transaction_ > 0) { + int status = sqlite3_get_autocommit(pConn_); + + if (status == 0) { return true; } else { return false; } } - -void DbConnection::add_transaction() { - transaction_ += 1; -} - -void DbConnection::rem_transaction() { - if (transaction_ > 0) { - transaction_ -= 1; - } else { - cpp11::stop("Cannot remove non-existent transactions"); - } -} 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 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}, diff --git a/tests/testthat/test-transactions.R b/tests/testthat/test-transactions.R index 3370a1e8..18050156 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) @@ -85,6 +85,7 @@ test_that("no nested unnamed transactions (commit after error)", { dbBegin(con) expect_true(sqliteIsTransacting(con)) expect_error(dbBegin(con)) + expect_true(sqliteIsTransacting(con)) dbCommit(con) expect_false(sqliteIsTransacting(con)) @@ -109,6 +110,7 @@ test_that("no nested unnamed transactions (rollback after error)", { dbBegin(con) expect_true(sqliteIsTransacting(con)) expect_error(dbBegin(con)) + expect_true(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") @@ -379,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_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_true(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_true(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_true(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_true(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_true(sqliteIsTransacting(con)) + dbWriteTable(con, "e", data.frame(e = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "COMMIT") + expect_false(sqliteIsTransacting(con)) + + dbBegin(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_true(sqliteIsTransacting(con)) + dbWriteTable(con, "g", data.frame(g = 1)) + expect_true(sqliteIsTransacting(con)) + dbExecute(con, "ROLLBACK") + expect_false(sqliteIsTransacting(con)) +})