Merge 'Fix rollback for TxErrors' from Diego Reis

Fixes #2153.
Not so sure if SQLite doesn't rollback in more cases, we should
definitively check this out.

Reviewed-by: Jussi Saurio <jussi.saurio@gmail.com>

Closes #2154

core/vdbe/mod.rs

@@ -405,21 +405,27 @@ impl Program {
             let _ = state.result_row.take();
             let (insn, insn_function) = &self.insns[state.pc as usize];
             trace_insn(self, state.pc as InsnReference, insn);
-            let res = insn_function(self, state, insn, &pager, mv_store.as_ref());
-            if res.is_err() {
-                let state = self.connection.transaction_state.get();
-                if let TransactionState::Write { schema_did_change } = state {
-                    pager.rollback(schema_did_change, &self.connection)?
+            match insn_function(self, state, insn, &pager, mv_store.as_ref()) {
+                Ok(InsnFunctionStepResult::Step) => {}
+                Ok(InsnFunctionStepResult::Done) => return Ok(StepResult::Done),
+                Ok(InsnFunctionStepResult::IO) => return Ok(StepResult::IO),
+                Ok(InsnFunctionStepResult::Row) => return Ok(StepResult::Row),
+                Ok(InsnFunctionStepResult::Interrupt) => return Ok(StepResult::Interrupt),
+                Ok(InsnFunctionStepResult::Busy) => return Ok(StepResult::Busy),
+                Err(err) => {
+                    match err {
+                        LimboError::TxError(_) => {}
+                        _ => {
+                            let state = self.connection.transaction_state.get();
+                            if let TransactionState::Write { schema_did_change } = state {
+                                pager.rollback(schema_did_change, &self.connection)?
+                            }
+                        }
+                    }
+                    let err = Err(err);
+                    return err;
                 }
             }
-            match res? {
-                InsnFunctionStepResult::Step => {}
-                InsnFunctionStepResult::Done => return Ok(StepResult::Done),
-                InsnFunctionStepResult::IO => return Ok(StepResult::IO),
-                InsnFunctionStepResult::Row => return Ok(StepResult::Row),
-                InsnFunctionStepResult::Interrupt => return Ok(StepResult::Interrupt),
-                InsnFunctionStepResult::Busy => return Ok(StepResult::Busy),
-            }
         }
     }
 

testing/rollback.test

@@ -55,7 +55,7 @@ do_execsql_test_on_specific_db {:memory:} rollback-mixed-operations {
     select * from t order by x;
 } {1 2}
 
-# The point of this test was to test we drop dirty pages after rollback by inserting into another table so that 
+# The point of this test was to test we drop dirty pages after rollback by inserting into another table so that
 # pages used are different.
 do_execsql_test_on_specific_db {:memory:} insert-after-rollback {
     create table t (x);
@@ -137,17 +137,15 @@ if {[info exists ::env(SQLITE_EXEC)] && ($::env(SQLITE_EXEC) eq "scripts/limbo-s
     } {"CREATE TABLE t (x)"}
 }
 
-
 do_execsql_test_on_specific_db {:memory:} schema-drop-table-rollback {
     create table t (x);
     begin;
-    drop table t; 
+    drop table t;
     rollback;
     select sql from sqlite_schema;
 } {"CREATE TABLE t (x)"}
 
-
 # TODO: add tests for:
-# * create virtual table 
+# * create virtual table
 # * drop index
 # * views...

tests/integration/query_processing/mod.rs

@@ -3,3 +3,4 @@ mod test_read_path;
 mod test_write_path;
 
 mod test_multi_thread;
+mod test_transactions;

tests/integration/query_processing/test_transactions.rs

@@ -0,0 +1,26 @@
+use turso_core::{LimboError, Result, StepResult, Value};
+
+use crate::common::TempDatabase;
+
+#[test]
+fn test_txn_error_doesnt_rollback_txn() -> Result<()> {
+    let tmp_db = TempDatabase::new_with_rusqlite("create table t(x);", false);
+    let conn = tmp_db.connect_limbo();
+
+    conn.execute("begin")?;
+    conn.execute("insert into t values (1)")?;
+    // should fail
+    assert!(conn
+        .execute("begin")
+        .inspect_err(|e| assert!(matches!(e, LimboError::TxError(_))))
+        .is_err());
+    conn.execute("insert into t values (1)")?;
+    conn.execute("commit")?;
+    let mut stmt = conn.query("select sum(x) from t")?.unwrap();
+    if let StepResult::Row = stmt.step()? {
+        let row = stmt.row().unwrap();
+        assert_eq!(*row.get::<&Value>(0).unwrap(), Value::Integer(2));
+    }
+
+    Ok(())
+}