Don't create transactions automatically (#191)

Author: simolus3

crates/core/src/error.rs

@@ -125,7 +125,7 @@ impl PowerSyncError {
         match self.inner.as_ref() {
             Sqlite(desc) => desc.code,
             ArgumentError { .. } => ResultCode::CONSTRAINT_DATATYPE,
-            StateError { .. } => ResultCode::MISUSE,
+            StateError { .. } | MustBeCalledInTransaction { .. } => ResultCode::MISUSE,
             MissingClientId
             | SyncProtocolError { .. }
             | DownMigrationDidNotUpdateVersion { .. }
@@ -193,7 +193,7 @@ impl From<ResultCode> for PowerSyncError {
 
 /// A structured enumeration of possible errors that can occur in the core extension.
 #[derive(Error, Debug)]
-enum RawPowerSyncError {
+pub enum RawPowerSyncError {
     /// An internal call to SQLite made by the core extension has failed. We store the original
     /// result code and an optional context describing what the core extension was trying to do when
     /// the error occurred.
@@ -248,10 +248,12 @@ enum RawPowerSyncError {
         libversion_number: c_int,
         libversion: &'static str,
     },
+    #[error("This function may only be called in transactions.")]
+    MustBeCalledInTransaction,
 }
 
 #[derive(Debug)]
-struct SqliteError {
+pub struct SqliteError {
     code: ResultCode,
     errstr: Option<String>,
     context: Option<Cow<'static, str>>,

crates/core/src/macros.rs

@@ -43,42 +43,3 @@ macro_rules! create_sqlite_optional_text_fn {
         }
     };
 }
-
-// Wrap a function in an auto-transaction.
-// Gives the equivalent of SQLite's auto-commit behaviour, except that applies to all statements
-// inside the function. Otherwise, each statement inside the function would be a transaction on its
-// own if the function itself is not wrapped in a transaction.
-#[macro_export]
-macro_rules! create_auto_tx_function {
-    ($fn_name:ident, $fn_impl_name:ident) => {
-        fn $fn_name(
-            ctx: *mut sqlite::context,
-            args: &[*mut sqlite::value],
-        ) -> Result<String, PowerSyncError> {
-            let db = ctx.db_handle();
-
-            // Auto-start a transaction if we're not in a transaction
-            let started_tx = if db.get_autocommit() {
-                db.exec_safe("BEGIN")?;
-                true
-            } else {
-                false
-            };
-
-            let result = $fn_impl_name(ctx, args);
-            if result.is_err() {
-                // Always ROLLBACK, even when we didn't start the transaction.
-                // Otherwise the user may be able to continue the transaction and end up in an inconsistent state.
-                // We ignore rollback errors.
-                if !db.get_autocommit() {
-                    let _ignore = db.exec_safe("ROLLBACK");
-                }
-            } else if started_tx {
-                // Only COMMIT our own transactions.
-                db.exec_safe("COMMIT")?;
-            }
-
-            result
-        }
-    };
-}

crates/core/src/schema/management.rs

@@ -13,17 +13,17 @@ use powersync_sqlite_nostd as sqlite;
 use powersync_sqlite_nostd::Context;
 use sqlite::{Connection, ResultCode, Value};
 
+use crate::create_sqlite_text_fn;
 use crate::error::{PSResult, PowerSyncError};
 use crate::ext::ExtendedDatabase;
 use crate::schema::inspection::{ExistingTable, ExistingView};
 use crate::schema::table_info::Index;
 use crate::state::DatabaseState;
-use crate::utils::SqlBuffer;
+use crate::utils::{SqlBuffer, verify_in_transaction};
 use crate::views::{
     powersync_trigger_delete_sql, powersync_trigger_insert_sql, powersync_trigger_update_sql,
     powersync_view_sql,
 };
-use crate::{create_auto_tx_function, create_sqlite_text_fn};
 
 use super::Schema;
 
@@ -265,13 +265,14 @@ fn powersync_replace_schema_impl(
     ctx: *mut sqlite::context,
     args: &[*mut sqlite::value],
 ) -> Result<String, PowerSyncError> {
+    let db = ctx.db_handle();
+    verify_in_transaction(db)?;
+
     let schema = args[0].text();
     let state = unsafe { DatabaseState::from_context(&ctx) };
     let parsed_schema =
         serde_json::from_str::<Schema>(schema).map_err(PowerSyncError::as_argument_error)?;
 
-    let db = ctx.db_handle();
-
     // language=SQLite
     db.exec_safe("SELECT powersync_init()").into_db_result(db)?;
 
@@ -283,10 +284,9 @@ fn powersync_replace_schema_impl(
     Ok(String::from(""))
 }
 
-create_auto_tx_function!(powersync_replace_schema_tx, powersync_replace_schema_impl);
 create_sqlite_text_fn!(
     powersync_replace_schema,
-    powersync_replace_schema_tx,
+    powersync_replace_schema_impl,
     "powersync_replace_schema"
 );
 

crates/core/src/sync/interface.rs

@@ -22,7 +22,7 @@ use serde_json::value::RawValue;
 use sqlite::{ResultCode, Value};
 
 use crate::sync::BucketPriority;
-use crate::utils::JsonString;
+use crate::utils::{JsonString, verify_in_transaction};
 
 /// Payload provided by SDKs when requesting a sync iteration.
 #[derive(Deserialize)]
@@ -206,7 +206,7 @@ pub fn register(db: *mut sqlite::sqlite3, state: Rc<DatabaseState>) -> Result<()
     ) -> () {
         let result = (|| -> Result<(), PowerSyncError> {
             let db = ctx.db_handle();
-            debug_assert!(!db.get_autocommit());
+            verify_in_transaction(db)?;
 
             let state = unsafe { DatabaseState::from_context(&ctx) };
             let args = sqlite::args!(argc, argv);

crates/core/src/utils/mod.rs

@@ -3,14 +3,28 @@ mod sql_buffer;
 use core::{cmp::Ordering, fmt::Display, hash::Hash};
 
 use alloc::{boxed::Box, string::String};
-use powersync_sqlite_nostd::{ColumnType, ManagedStmt};
+use powersync_sqlite_nostd::{ColumnType, Connection, ManagedStmt, sqlite3};
 use serde::Serialize;
 use serde_json::value::RawValue;
 pub use sql_buffer::{InsertIntoCrud, SqlBuffer, WriteType};
 
-use crate::error::PowerSyncError;
+use crate::error::{PowerSyncError, RawPowerSyncError};
 use uuid::Uuid;
 
+#[cold]
+fn must_be_in_tx_error() -> PowerSyncError {
+    return RawPowerSyncError::MustBeCalledInTransaction.into();
+}
+
+#[inline]
+pub fn verify_in_transaction(db: *mut sqlite3) -> Result<(), PowerSyncError> {
+    if db.get_autocommit() {
+        return Err(must_be_in_tx_error());
+    }
+
+    Ok(())
+}
+
 /// Calls [read] to read a column if it's not null, otherwise returns [None].
 #[inline]
 pub fn column_nullable<T, R: FnOnce() -> Result<T, PowerSyncError>>(

crates/core/src/view_admin.rs

@@ -10,12 +10,12 @@ use powersync_sqlite_nostd as sqlite;
 use powersync_sqlite_nostd::{Connection, Context};
 use sqlite::{ResultCode, Value};
 
+use crate::create_sqlite_text_fn;
 use crate::error::{PSResult, PowerSyncError};
 use crate::migrations::{LATEST_VERSION, powersync_migrate};
 use crate::schema::inspection::ExistingView;
 use crate::state::DatabaseState;
-use crate::utils::SqlBuffer;
-use crate::{create_auto_tx_function, create_sqlite_text_fn};
+use crate::utils::{SqlBuffer, verify_in_transaction};
 
 // Used in old down migrations, do not remove.
 extern "C" fn powersync_drop_view(
@@ -35,6 +35,8 @@ fn powersync_init_impl(
     ctx: *mut sqlite::context,
     _args: &[*mut sqlite::value],
 ) -> Result<String, PowerSyncError> {
+    let db = ctx.db_handle();
+    verify_in_transaction(db)?;
     powersync_migrate(ctx, LATEST_VERSION)?;
 
     // Register the powersync_internal_close vtab to implement a "pre-close hook".
@@ -45,23 +47,24 @@ fn powersync_init_impl(
     Ok(String::from(""))
 }
 
-create_auto_tx_function!(powersync_init_tx, powersync_init_impl);
-create_sqlite_text_fn!(powersync_init, powersync_init_tx, "powersync_init");
+create_sqlite_text_fn!(powersync_init, powersync_init_impl, "powersync_init");
 
 fn powersync_test_migration_impl(
     ctx: *mut sqlite::context,
     args: &[*mut sqlite::value],
 ) -> Result<String, PowerSyncError> {
+    let db = ctx.db_handle();
+    verify_in_transaction(db)?;
+
     let target_version = args[0].int();
     powersync_migrate(ctx, target_version)?;
 
     Ok(String::from(""))
 }
 
-create_auto_tx_function!(powersync_test_migration_tx, powersync_test_migration_impl);
 create_sqlite_text_fn!(
     powersync_test_migration,
-    powersync_test_migration_tx,
+    powersync_test_migration_impl,
     "powersync_test_migration"
 );
 
@@ -70,6 +73,7 @@ fn powersync_clear_impl(
     args: &[*mut sqlite::value],
 ) -> Result<String, PowerSyncError> {
     let local_db = ctx.db_handle();
+    verify_in_transaction(local_db)?;
     let state = unsafe { DatabaseState::from_context(&ctx) };
 
     let flags = PowerSyncClearFlags(args[0].int());
@@ -176,6 +180,8 @@ fn powersync_trigger_resync_impl(
     args: &[*mut sqlite::value],
 ) -> Result<String, PowerSyncError> {
     let local_db = ctx.db_handle();
+    verify_in_transaction(local_db)?;
+
     let state = unsafe { DatabaseState::from_context(&ctx) };
     trigger_resync(local_db, state)?;
 
@@ -187,10 +193,9 @@ fn powersync_trigger_resync_impl(
     Ok(Default::default())
 }
 
-create_auto_tx_function!(powersync_trigger_resync_tx, powersync_trigger_resync_impl);
 create_sqlite_text_fn!(
     powersync_trigger_resync,
-    powersync_trigger_resync_tx,
+    powersync_trigger_resync_impl,
     "powersync_trigger_resync"
 );
 
@@ -210,8 +215,7 @@ impl PowerSyncClearFlags {
     }
 }
 
-create_auto_tx_function!(powersync_clear_tx, powersync_clear_impl);
-create_sqlite_text_fn!(powersync_clear, powersync_clear_tx, "powersync_clear");
+create_sqlite_text_fn!(powersync_clear, powersync_clear_impl, "powersync_clear");
 
 pub fn register(db: *mut sqlite::sqlite3, state: Rc<DatabaseState>) -> Result<(), ResultCode> {
     // This entire module is just making it easier to edit sqlite_master using queries.