From 34b0c7c09a1d00205bc9ad795ea766ffbcf89487 Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 14 Feb 2025 09:47:50 +0200 Subject: [PATCH 1/6] core/vdbe: AutoCommit instruction --- COMPAT.md | 2 +- core/error.rs | 2 ++ core/lib.rs | 3 +++ core/vdbe/builder.rs | 1 - core/vdbe/explain.rs | 12 ++++++++++++ core/vdbe/insn.rs | 6 ++++++ core/vdbe/mod.rs | 34 +++++++++++++++++++++++++++++++--- 7 files changed, 55 insertions(+), 5 deletions(-) diff --git a/COMPAT.md b/COMPAT.md index e64369bbe..cf47b09a9 100644 --- a/COMPAT.md +++ b/COMPAT.md @@ -412,7 +412,7 @@ Modifiers: | AggStep | Yes | | | AggStep | Yes | | | And | Yes | | -| AutoCommit | No | | +| AutoCommit | Yes | | | BitAnd | Yes | | | BitNot | Yes | | | BitOr | Yes | | diff --git a/core/error.rs b/core/error.rs index cfe1a827d..a648117c9 100644 --- a/core/error.rs +++ b/core/error.rs @@ -19,6 +19,8 @@ pub enum LimboError { ConversionError(String), #[error("Env variable error: {0}")] EnvVarError(#[from] std::env::VarError), + #[error("Transaction error: {0}")] + TxError(String), #[error("I/O error: {0}")] IOError(#[from] std::io::Error), #[cfg(all(target_os = "linux", feature = "io_uring"))] diff --git a/core/lib.rs b/core/lib.rs index dbeb04906..c8a76a60b 100644 --- a/core/lib.rs +++ b/core/lib.rs @@ -161,6 +161,7 @@ impl Database { pager, schema: schema.clone(), header, + auto_commit: RefCell::new(true), transaction_state: RefCell::new(TransactionState::None), last_insert_rowid: Cell::new(0), last_change: Cell::new(0), @@ -179,6 +180,7 @@ impl Database { schema: self.schema.clone(), header: self.header.clone(), last_insert_rowid: Cell::new(0), + auto_commit: RefCell::new(true), transaction_state: RefCell::new(TransactionState::None), last_change: Cell::new(0), total_changes: Cell::new(0), @@ -263,6 +265,7 @@ pub struct Connection { pager: Rc, schema: Rc>, header: Rc>, + auto_commit: RefCell, transaction_state: RefCell, last_insert_rowid: Cell, last_change: Cell, diff --git a/core/vdbe/builder.rs b/core/vdbe/builder.rs index 68d0b2f4a..b86d65b97 100644 --- a/core/vdbe/builder.rs +++ b/core/vdbe/builder.rs @@ -448,7 +448,6 @@ impl ProgramBuilder { database_header, comments: self.comments, connection, - auto_commit: true, parameters: self.parameters, n_change: Cell::new(0), change_cnt_on, diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs index 79c96a44e..1b956cc9e 100644 --- a/core/vdbe/explain.rs +++ b/core/vdbe/explain.rs @@ -1238,6 +1238,18 @@ pub fn insn_to_str( 0, "".to_string(), ), + Insn::AutoCommit { + auto_commit, + rollback, + } => ( + "AutoCommit", + *auto_commit as i32, + *rollback as i32, + 0, + OwnedValue::build_text(""), + 0, + format!("auto_commit={}, rollback={}", auto_commit, rollback), + ), }; format!( "{:<4} {:<17} {:<4} {:<4} {:<4} {:<13} {:<2} {}", diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index e58de109c..d1b27399d 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -320,6 +320,12 @@ pub enum Insn { write: bool, }, + // Set database auto-commit mode and potentially rollback. + AutoCommit { + auto_commit: bool, + rollback: bool, + }, + // Branch to the given PC. Goto { target_pc: BranchOffset, diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index 81f793e44..9979f3e1e 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -411,7 +411,6 @@ pub struct Program { pub comments: Option>, pub parameters: crate::parameters::Parameters, pub connection: Weak, - pub auto_commit: bool, pub n_change: Cell, pub change_cnt_on: bool, pub result_columns: Vec, @@ -1131,17 +1130,18 @@ impl Program { ))); } } - tracing::trace!("Halt auto_commit {}", self.auto_commit); let connection = self .connection .upgrade() .expect("only weak ref to connection?"); + let auto_commit = *connection.auto_commit.borrow(); + tracing::trace!("Halt auto_commit {}", auto_commit); let current_state = connection.transaction_state.borrow().clone(); if current_state == TransactionState::Read { pager.end_read_tx()?; return Ok(StepResult::Done); } - return if self.auto_commit { + return if auto_commit { match pager.end_tx() { Ok(crate::storage::wal::CheckpointStatus::IO) => Ok(StepResult::IO), Ok(crate::storage::wal::CheckpointStatus::Done(_)) => { @@ -1195,6 +1195,34 @@ impl Program { } state.pc += 1; } + Insn::AutoCommit { + auto_commit, + rollback, + } => { + let conn = self.connection.upgrade().unwrap(); + if *auto_commit != *conn.auto_commit.borrow() { + if *rollback { + todo!("Rollback is not implemented"); + } else { + conn.auto_commit.replace(*auto_commit); + } + } else { + if !*auto_commit { + return Err(LimboError::TxError( + "cannot start a transaction within a transaction".to_string(), + )); + } else if *rollback { + return Err(LimboError::TxError( + "cannot rollback - no transaction is active".to_string(), + )); + } else { + return Err(LimboError::TxError( + "cannot commit - no transaction is active".to_string(), + )); + } + } + state.pc += 1; + } Insn::Goto { target_pc } => { assert!(target_pc.is_offset()); state.pc = target_pc.to_offset_int(); From 9fff9f6081784368ebc187d70f45c49f507d5333 Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 14 Feb 2025 10:03:59 +0200 Subject: [PATCH 2/6] core/translate: BEGIN IMMEDIATE support Emit the following code sequence for `BEGIN IMMEDIATE`: ``` limbo> EXPLAIN BEGIN IMMEDIATE; addr opcode p1 p2 p3 p4 p5 comment ---- ----------------- ---- ---- ---- ------------- -- ------- 0 Init 0 4 0 0 Start at 4 1 Transaction 0 1 0 0 2 AutoCommit 0 0 0 0 auto_commit=false, rollback=false 3 Halt 0 0 0 0 4 Goto 0 1 0 0 ``` Please note that SQLite emits *two* transaction instructions -- one for main database and one for temporary tables. However, since we don't support the latter, we only emit one transaction instruction. --- COMPAT.md | 2 +- core/translate/mod.rs | 4 +++- core/translate/transaction.rs | 39 +++++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 core/translate/transaction.rs diff --git a/COMPAT.md b/COMPAT.md index cf47b09a9..bc9f56c6d 100644 --- a/COMPAT.md +++ b/COMPAT.md @@ -46,7 +46,7 @@ The current status of Limbo is: | ALTER TABLE | No | | | ANALYZE | No | | | ATTACH DATABASE | No | | -| BEGIN TRANSACTION | No | | +| BEGIN TRANSACTION | Partial | `BEGIN IMMEDIATE` is supported | | COMMIT TRANSACTION | No | | | CREATE INDEX | No | | | CREATE TABLE | Partial | | diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 5f9e19866..762405956 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -22,6 +22,7 @@ pub(crate) mod pragma; pub(crate) mod result_row; pub(crate) mod select; pub(crate) mod subquery; +pub(crate) mod transaction; use crate::schema::Schema; use crate::storage::pager::Pager; @@ -38,6 +39,7 @@ use sqlite3_parser::ast::{Delete, Insert}; use std::cell::RefCell; use std::fmt::Display; use std::rc::{Rc, Weak}; +use transaction::translate_tx_begin; /// Translate SQL statement into bytecode program. pub fn translate( @@ -55,7 +57,7 @@ pub fn translate( ast::Stmt::AlterTable(_) => bail_parse_error!("ALTER TABLE not supported yet"), ast::Stmt::Analyze(_) => bail_parse_error!("ANALYZE not supported yet"), ast::Stmt::Attach { .. } => bail_parse_error!("ATTACH not supported yet"), - ast::Stmt::Begin(_, _) => bail_parse_error!("BEGIN not supported yet"), + ast::Stmt::Begin(tx_type, tx_name) => translate_tx_begin(tx_type, tx_name)?, ast::Stmt::Commit(_) => bail_parse_error!("COMMIT not supported yet"), ast::Stmt::CreateIndex { .. } => bail_parse_error!("CREATE INDEX not supported yet"), ast::Stmt::CreateTable { diff --git a/core/translate/transaction.rs b/core/translate/transaction.rs new file mode 100644 index 000000000..2095b0b8a --- /dev/null +++ b/core/translate/transaction.rs @@ -0,0 +1,39 @@ +use crate::translate::{ProgramBuilder, ProgramBuilderOpts}; +use crate::vdbe::insn::Insn; +use crate::{bail_parse_error, QueryMode, Result}; +use sqlite3_parser::ast::{Name, TransactionType}; + +pub fn translate_tx_begin( + tx_type: Option, + _tx_name: Option, +) -> Result { + let mut program = ProgramBuilder::new(ProgramBuilderOpts { + query_mode: QueryMode::Normal, + num_cursors: 0, + approx_num_insns: 0, + approx_num_labels: 0, + }); + let init_label = program.emit_init(); + let start_offset = program.offset(); + let tx_type = tx_type.unwrap_or(TransactionType::Deferred); + match tx_type { + TransactionType::Deferred => { + bail_parse_error!("BEGIN DEFERRED not supported yet"); + } + TransactionType::Exclusive => { + bail_parse_error!("BEGIN EXCLUSIVE not supported yet"); + } + TransactionType::Immediate => { + program.emit_insn(Insn::Transaction { write: true }); + // TODO: Emit transaction instruction on temporary tables when we support them. + program.emit_insn(Insn::AutoCommit { + auto_commit: false, + rollback: false, + }); + } + } + program.emit_halt(); + program.resolve_label(init_label, program.offset()); + program.emit_goto(start_offset); + Ok(program) +} From 5626ca450f003e70d4e7861f29bf73380132e3cf Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 14 Feb 2025 10:21:16 +0200 Subject: [PATCH 3/6] core/translate: COMMIT support ``` limbo> EXPLAIN COMMIT; addr opcode p1 p2 p3 p4 p5 comment ---- ----------------- ---- ---- ---- ------------- -- ------- 0 Init 0 3 0 0 Start at 3 1 AutoCommit 1 0 0 0 auto_commit=true, rollback=false 2 Halt 0 0 0 0 3 Goto 0 1 0 0 ``` --- COMPAT.md | 6 +++--- core/translate/mod.rs | 4 ++-- core/translate/transaction.rs | 19 +++++++++++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/COMPAT.md b/COMPAT.md index bc9f56c6d..08546a486 100644 --- a/COMPAT.md +++ b/COMPAT.md @@ -46,8 +46,8 @@ The current status of Limbo is: | ALTER TABLE | No | | | ANALYZE | No | | | ATTACH DATABASE | No | | -| BEGIN TRANSACTION | Partial | `BEGIN IMMEDIATE` is supported | -| COMMIT TRANSACTION | No | | +| BEGIN TRANSACTION | Partial | `BEGIN IMMEDIATE` is only supported mode, transaction names are not supported. | +| COMMIT TRANSACTION | Partial | Transaction names are not supported. | | CREATE INDEX | No | | | CREATE TABLE | Partial | | | CREATE TRIGGER | No | | @@ -59,7 +59,7 @@ The current status of Limbo is: | DROP TABLE | No | | | DROP TRIGGER | No | | | DROP VIEW | No | | -| END TRANSACTION | No | | +| END TRANSACTION | Partial | Alias for `COMMIT TRANSACTION` | | EXPLAIN | Yes | | | INDEXED BY | No | | | INSERT | Partial | | diff --git a/core/translate/mod.rs b/core/translate/mod.rs index 762405956..30a103f6c 100644 --- a/core/translate/mod.rs +++ b/core/translate/mod.rs @@ -39,7 +39,7 @@ use sqlite3_parser::ast::{Delete, Insert}; use std::cell::RefCell; use std::fmt::Display; use std::rc::{Rc, Weak}; -use transaction::translate_tx_begin; +use transaction::{translate_tx_begin, translate_tx_commit}; /// Translate SQL statement into bytecode program. pub fn translate( @@ -58,7 +58,7 @@ pub fn translate( ast::Stmt::Analyze(_) => bail_parse_error!("ANALYZE not supported yet"), ast::Stmt::Attach { .. } => bail_parse_error!("ATTACH not supported yet"), ast::Stmt::Begin(tx_type, tx_name) => translate_tx_begin(tx_type, tx_name)?, - ast::Stmt::Commit(_) => bail_parse_error!("COMMIT not supported yet"), + ast::Stmt::Commit(tx_name) => translate_tx_commit(tx_name)?, ast::Stmt::CreateIndex { .. } => bail_parse_error!("CREATE INDEX not supported yet"), ast::Stmt::CreateTable { temporary, diff --git a/core/translate/transaction.rs b/core/translate/transaction.rs index 2095b0b8a..647e9fe50 100644 --- a/core/translate/transaction.rs +++ b/core/translate/transaction.rs @@ -37,3 +37,22 @@ pub fn translate_tx_begin( program.emit_goto(start_offset); Ok(program) } + +pub fn translate_tx_commit(_tx_name: Option) -> Result { + let mut program = ProgramBuilder::new(ProgramBuilderOpts { + query_mode: QueryMode::Normal, + num_cursors: 0, + approx_num_insns: 0, + approx_num_labels: 0, + }); + let init_label = program.emit_init(); + let start_offset = program.offset(); + program.emit_insn(Insn::AutoCommit { + auto_commit: true, + rollback: false, + }); + program.emit_halt(); + program.resolve_label(init_label, program.offset()); + program.emit_goto(start_offset); + Ok(program) +} From 076331d8cf9a7587ed750735625e5d1fab370a67 Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 14 Feb 2025 09:43:32 +0200 Subject: [PATCH 4/6] testing: Basic BEGIN + END test --- testing/all.test | 3 ++- testing/transactions.test | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) create mode 100755 testing/transactions.test diff --git a/testing/all.test b/testing/all.test index b2f71f243..5119b8eb0 100755 --- a/testing/all.test +++ b/testing/all.test @@ -24,4 +24,5 @@ source $testdir/compare.test source $testdir/changes.test source $testdir/total-changes.test source $testdir/offset.test -source $testdir/scalar-functions-printf.test \ No newline at end of file +source $testdir/scalar-functions-printf.test +source $testdir/transactions.test diff --git a/testing/transactions.test b/testing/transactions.test new file mode 100755 index 000000000..50e2059ad --- /dev/null +++ b/testing/transactions.test @@ -0,0 +1,8 @@ +#!/usr/bin/env tclsh + +set testdir [file dirname $argv0] +source $testdir/tester.tcl + +do_execsql_test basic-tx-1 { + BEGIN IMMEDIATE; END +} {} From 948585bb42947b421313bee056d052cb8daaa0ec Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 14 Feb 2025 11:42:24 +0200 Subject: [PATCH 5/6] core/vdbe: Extract Program::halt() helper We need this for AutoCommit opcode too. --- core/vdbe/mod.rs | 68 +++++++++++++++++++++++++----------------------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index 9979f3e1e..d1c85ca45 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -1130,38 +1130,7 @@ impl Program { ))); } } - let connection = self - .connection - .upgrade() - .expect("only weak ref to connection?"); - let auto_commit = *connection.auto_commit.borrow(); - tracing::trace!("Halt auto_commit {}", auto_commit); - let current_state = connection.transaction_state.borrow().clone(); - if current_state == TransactionState::Read { - pager.end_read_tx()?; - return Ok(StepResult::Done); - } - return if auto_commit { - match pager.end_tx() { - Ok(crate::storage::wal::CheckpointStatus::IO) => Ok(StepResult::IO), - Ok(crate::storage::wal::CheckpointStatus::Done(_)) => { - if self.change_cnt_on { - if let Some(conn) = self.connection.upgrade() { - conn.set_changes(self.n_change.get()); - } - } - Ok(StepResult::Done) - } - Err(e) => Err(e), - } - } else { - if self.change_cnt_on { - if let Some(conn) = self.connection.upgrade() { - conn.set_changes(self.n_change.get()); - } - } - return Ok(StepResult::Done); - }; + return self.halt(pager); } Insn::Transaction { write } => { let connection = self.connection.upgrade().unwrap(); @@ -2773,6 +2742,41 @@ impl Program { } } } + + fn halt(&self, pager: Rc) -> Result { + let connection = self + .connection + .upgrade() + .expect("only weak ref to connection?"); + let auto_commit = *connection.auto_commit.borrow(); + tracing::trace!("Halt auto_commit {}", auto_commit); + let current_state = connection.transaction_state.borrow().clone(); + if current_state == TransactionState::Read { + pager.end_read_tx()?; + return Ok(StepResult::Done); + } + return if auto_commit { + match pager.end_tx() { + Ok(crate::storage::wal::CheckpointStatus::IO) => Ok(StepResult::IO), + Ok(crate::storage::wal::CheckpointStatus::Done(_)) => { + if self.change_cnt_on { + if let Some(conn) = self.connection.upgrade() { + conn.set_changes(self.n_change.get()); + } + } + Ok(StepResult::Done) + } + Err(e) => Err(e), + } + } else { + if self.change_cnt_on { + if let Some(conn) = self.connection.upgrade() { + conn.set_changes(self.n_change.get()); + } + } + return Ok(StepResult::Done); + }; + } } fn get_new_rowid(cursor: &mut BTreeCursor, mut rng: R) -> Result> { From ae3c6b7ec5fd23e70543c7f9de240ea3397e5c28 Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 14 Feb 2025 11:43:16 +0200 Subject: [PATCH 6/6] core/vdbe: Fix AutoCommit instruction to halt the VM Pointed out by Jussi --- core/vdbe/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index d1c85ca45..1083cbcdf 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -1190,7 +1190,7 @@ impl Program { )); } } - state.pc += 1; + return self.halt(pager); } Insn::Goto { target_pc } => { assert!(target_pc.is_offset());