From 1e177053cba05ce5f1cd9b9da506da56b7a7f513 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Fri, 1 Aug 2025 17:41:35 -0300 Subject: [PATCH 1/3] feat: add `RenameTable` instruction direct schema mutation, no reparsing --- core/benches/benchmark.rs | 50 ++++++++++++++++++++++++++++++++++++++- core/translate/alter.rs | 20 ++++++---------- core/vdbe/execute.rs | 48 +++++++++++++++++++++++++++++++++++++ core/vdbe/explain.rs | 9 +++++++ core/vdbe/insn.rs | 5 ++++ 5 files changed, 118 insertions(+), 14 deletions(-) diff --git a/core/benches/benchmark.rs b/core/benches/benchmark.rs index 008516a6f..5dcc82586 100644 --- a/core/benches/benchmark.rs +++ b/core/benches/benchmark.rs @@ -54,6 +54,54 @@ fn bench_open(criterion: &mut Criterion) { group.finish(); } +fn bench_alter(criterion: &mut Criterion) { + // https://github.com/tursodatabase/turso/issues/174 + // The rusqlite benchmark crashes on Mac M1 when using the flamegraph features + let enable_rusqlite = std::env::var("DISABLE_RUSQLITE_BENCHMARK").is_err(); + + if !std::fs::exists("../testing/schema_5k.db").unwrap() { + #[allow(clippy::arc_with_non_send_sync)] + let io = Arc::new(PlatformIO::new().unwrap()); + let db = Database::open_file(io.clone(), "../testing/schema_5k.db", false, false).unwrap(); + let conn = db.connect().unwrap(); + + for i in 0..5000 { + conn.execute( + format!("CREATE TABLE table_{i} ( id INTEGER PRIMARY KEY, name TEXT, value INTEGER, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP )") + ).unwrap(); + } + } + + let mut group = criterion.benchmark_group("`ALTER TABLE _ RENAME TO _`"); + + let stmts = ["CREATE TABLE x(a)", "ALTER TABLE x RENAME TO y", "DROP TABLE y"]; + + group.bench_function(BenchmarkId::new("limbo_rename_table", ""), |b| { + #[allow(clippy::arc_with_non_send_sync)] + let io = Arc::new(PlatformIO::new().unwrap()); + let db = Database::open_file(io.clone(), "../testing/schema_5k.db", false, false).unwrap(); + let conn = db.connect().unwrap(); + b.iter(|| { + for stmt in stmts { + conn.execute(stmt).unwrap(); + } + }); + }); + + if enable_rusqlite { + group.bench_function(BenchmarkId::new("sqlite_rename_table", ""), |b| { + let conn = rusqlite::Connection::open("../testing/schema_5k.db").unwrap(); + b.iter(|| { + for stmt in stmts { + conn.execute(stmt, ()).unwrap(); + } + }); + }); + } + + group.finish(); +} + fn bench_prepare_query(criterion: &mut Criterion) { // https://github.com/tursodatabase/turso/issues/174 // The rusqlite benchmark crashes on Mac M1 when using the flamegraph features @@ -376,6 +424,6 @@ fn bench_insert_rows(criterion: &mut Criterion) { criterion_group! { name = benches; config = Criterion::default().with_profiler(PProfProfiler::new(100, Output::Flamegraph(None))); - targets = bench_open, bench_prepare_query, bench_execute_select_1, bench_execute_select_rows, bench_execute_select_count, bench_insert_rows + targets = bench_open, bench_alter, bench_prepare_query, bench_execute_select_1, bench_execute_select_rows, bench_execute_select_count, bench_insert_rows } criterion_main!(benches); diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 9fad5b8bb..5fde77f36 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -7,7 +7,7 @@ use crate::{ schema::{Column, Schema}, util::normalize_ident, vdbe::{ - builder::ProgramBuilder, + builder::{CursorType, ProgramBuilder}, insn::{Cookie, Insn, RegisterOrLiteral}, }, LimboError, Result, SymbolTable, @@ -106,9 +106,7 @@ pub fn translate_alter_table( let root_page = btree.root_page; let table_name = btree.name.clone(); - let cursor_id = program.alloc_cursor_id( - crate::vdbe::builder::CursorType::BTreeTable(original_btree), - ); + let cursor_id = program.alloc_cursor_id(CursorType::BTreeTable(original_btree)); program.emit_insn(Insn::OpenWrite { cursor_id, @@ -248,9 +246,7 @@ pub fn translate_alter_table( .get_btree_table(SQLITE_TABLEID) .expect("sqlite_schema should be on schema"); - let cursor_id = program.alloc_cursor_id(crate::vdbe::builder::CursorType::BTreeTable( - sqlite_schema.clone(), - )); + let cursor_id = program.alloc_cursor_id(CursorType::BTreeTable(sqlite_schema.clone())); program.emit_insn(Insn::OpenWrite { cursor_id, @@ -335,9 +331,7 @@ pub fn translate_alter_table( .get_btree_table(SQLITE_TABLEID) .expect("sqlite_schema should be on schema"); - let cursor_id = program.alloc_cursor_id(crate::vdbe::builder::CursorType::BTreeTable( - sqlite_schema.clone(), - )); + let cursor_id = program.alloc_cursor_id(CursorType::BTreeTable(sqlite_schema.clone())); program.emit_insn(Insn::OpenWrite { cursor_id, @@ -398,9 +392,9 @@ pub fn translate_alter_table( p5: 0, }); - program.emit_insn(Insn::ParseSchema { - db: usize::MAX, // TODO: This value is unused, change when we do something with it - where_clause: None, + program.emit_insn(Insn::RenameTable { + from: table_name.to_owned(), + to: new_name.to_owned(), }); program.epilogue(TransactionMode::Write); diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 099a0825e..0bbf53ce2 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -1,6 +1,7 @@ #![allow(unused_variables)] use crate::function::AlterTableFunc; use crate::numeric::{NullableInteger, Numeric}; +use crate::schema::Table; use crate::storage::btree::{integrity_check, IntegrityCheckError, IntegrityCheckState}; use crate::storage::database::DatabaseFile; use crate::storage::page_cache::DumbLruPageCache; @@ -6740,6 +6741,53 @@ pub fn op_cast( Ok(InsnFunctionStepResult::Step) } +pub fn op_rename_table( + program: &Program, + state: &mut ProgramState, + insn: &Insn, + pager: &Rc, + mv_store: Option<&Arc>, +) -> Result { + let Insn::RenameTable { from, to } = insn else { + unreachable!("unexpected Insn {:?}", insn) + }; + + let conn = program.connection.clone(); + // set auto commit to false in order for parse schema to not commit changes as transaction state is stored in connection, + // and we use the same connection for nested query. + let previous_auto_commit = conn.auto_commit.get(); + conn.auto_commit.set(false); + + let stmt = conn.prepare("SELECT * FROM sqlite_schema")?; + + conn.with_schema_mut(|schema| { + schema.indexes.remove(from).map(|mut indexes| { + indexes.iter_mut().for_each(|index| { + let index = Arc::make_mut(index); + index.table_name = to.to_owned(); + }); + + schema.indexes.insert(to.to_owned(), indexes); + }); + + let mut table_ref = schema + .tables + .remove(from) + .expect("table being renamed should be in schema"); + let table = Arc::make_mut(&mut table_ref); + + let Table::BTree(btree) = table else { + panic!("only btree tables can be renamed"); + }; + + schema.tables.insert(to.to_owned(), table_ref); + }); + + conn.auto_commit.set(previous_auto_commit); + state.pc += 1; + Ok(InsnFunctionStepResult::Step) +} + impl Value { pub fn exec_lower(&self) -> Option { match self { diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs index 76a969698..0fefedcc2 100644 --- a/core/vdbe/explain.rs +++ b/core/vdbe/explain.rs @@ -1627,6 +1627,15 @@ pub fn insn_to_str( 0, format!("affinity(r[{}]={:?})", *reg, affinity), ), + Insn::RenameTable { from, to } => ( + "RenameTable", + 0, + 0, + 0, + Value::build_text(""), + 0, + format!("rename_table({from}, {to})"), + ), }; format!( "{:<4} {:<17} {:<4} {:<4} {:<4} {:<13} {:<2} {}", diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index 36be77658..302789bdc 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -1004,6 +1004,10 @@ pub enum Insn { roots: Vec, message_register: usize, }, + RenameTable { + from: String, + to: String, + }, } impl Insn { @@ -1132,6 +1136,7 @@ impl Insn { Insn::IdxDelete { .. } => execute::op_idx_delete, Insn::Count { .. } => execute::op_count, Insn::IntegrityCk { .. } => execute::op_integrity_check, + Insn::RenameTable { .. } => execute::op_rename_table, } } } From b14a11a2fd6368807dee3a89d7c4cec509178e57 Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Sat, 2 Aug 2025 17:17:36 -0300 Subject: [PATCH 2/3] fix: change `name` for schema btree + fix benchmark --- core/benches/benchmark.rs | 41 ++++++++++++++++++++++++++++----------- core/vdbe/execute.rs | 25 ++++++++++++------------ 2 files changed, 42 insertions(+), 24 deletions(-) diff --git a/core/benches/benchmark.rs b/core/benches/benchmark.rs index 5dcc82586..c8ca207f6 100644 --- a/core/benches/benchmark.rs +++ b/core/benches/benchmark.rs @@ -1,6 +1,9 @@ use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion}; use pprof::criterion::{Output, PProfProfiler}; -use std::sync::Arc; +use std::{ + sync::Arc, + time::{Duration, Instant}, +}; use turso_core::{Database, PlatformIO}; fn rusqlite_open() -> rusqlite::Connection { @@ -74,27 +77,43 @@ fn bench_alter(criterion: &mut Criterion) { let mut group = criterion.benchmark_group("`ALTER TABLE _ RENAME TO _`"); - let stmts = ["CREATE TABLE x(a)", "ALTER TABLE x RENAME TO y", "DROP TABLE y"]; - group.bench_function(BenchmarkId::new("limbo_rename_table", ""), |b| { #[allow(clippy::arc_with_non_send_sync)] let io = Arc::new(PlatformIO::new().unwrap()); let db = Database::open_file(io.clone(), "../testing/schema_5k.db", false, false).unwrap(); let conn = db.connect().unwrap(); - b.iter(|| { - for stmt in stmts { - conn.execute(stmt).unwrap(); - } + b.iter_custom(|iters| { + (0..iters) + .map(|_| { + conn.execute("CREATE TABLE x(a)").unwrap(); + let elapsed = { + let start = Instant::now(); + conn.execute("ALTER TABLE x RENAME TO y").unwrap(); + start.elapsed() + }; + conn.execute("DROP TABLE y").unwrap(); + elapsed + }) + .sum() }); }); if enable_rusqlite { group.bench_function(BenchmarkId::new("sqlite_rename_table", ""), |b| { let conn = rusqlite::Connection::open("../testing/schema_5k.db").unwrap(); - b.iter(|| { - for stmt in stmts { - conn.execute(stmt, ()).unwrap(); - } + b.iter_custom(|iters| { + (0..iters) + .map(|_| { + conn.execute("CREATE TABLE x(a)", ()).unwrap(); + let elapsed = { + let start = Instant::now(); + conn.execute("ALTER TABLE x RENAME TO y", ()).unwrap(); + start.elapsed() + }; + conn.execute("DROP TABLE y", ()).unwrap(); + elapsed + }) + .sum() }); }); } diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 0bbf53ce2..1a7630290 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -6753,12 +6753,6 @@ pub fn op_rename_table( }; let conn = program.connection.clone(); - // set auto commit to false in order for parse schema to not commit changes as transaction state is stored in connection, - // and we use the same connection for nested query. - let previous_auto_commit = conn.auto_commit.get(); - conn.auto_commit.set(false); - - let stmt = conn.prepare("SELECT * FROM sqlite_schema")?; conn.with_schema_mut(|schema| { schema.indexes.remove(from).map(|mut indexes| { @@ -6770,20 +6764,25 @@ pub fn op_rename_table( schema.indexes.insert(to.to_owned(), indexes); }); - let mut table_ref = schema + let mut table = schema .tables .remove(from) .expect("table being renamed should be in schema"); - let table = Arc::make_mut(&mut table_ref); - let Table::BTree(btree) = table else { - panic!("only btree tables can be renamed"); - }; + { + let table = Arc::make_mut(&mut table); - schema.tables.insert(to.to_owned(), table_ref); + let Table::BTree(btree) = table else { + panic!("only btree tables can be renamed"); + }; + + let btree = Arc::make_mut(btree); + btree.name = to.to_owned(); + } + + schema.tables.insert(to.to_owned(), table); }); - conn.auto_commit.set(previous_auto_commit); state.pc += 1; Ok(InsnFunctionStepResult::Step) } From b9a3a93ef079cfd7706a58d9654d733395603dcf Mon Sep 17 00:00:00 2001 From: "Levy A." Date: Sat, 2 Aug 2025 17:25:29 -0300 Subject: [PATCH 3/3] fix: clippy --- core/benches/benchmark.rs | 5 +---- core/vdbe/execute.rs | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/core/benches/benchmark.rs b/core/benches/benchmark.rs index c8ca207f6..0d278aa6c 100644 --- a/core/benches/benchmark.rs +++ b/core/benches/benchmark.rs @@ -1,9 +1,6 @@ use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion}; use pprof::criterion::{Output, PProfProfiler}; -use std::{ - sync::Arc, - time::{Duration, Instant}, -}; +use std::{sync::Arc, time::Instant}; use turso_core::{Database, PlatformIO}; fn rusqlite_open() -> rusqlite::Connection { diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 1a7630290..f87ee4b4e 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -6755,14 +6755,14 @@ pub fn op_rename_table( let conn = program.connection.clone(); conn.with_schema_mut(|schema| { - schema.indexes.remove(from).map(|mut indexes| { + if let Some(mut indexes) = schema.indexes.remove(from) { indexes.iter_mut().for_each(|index| { let index = Arc::make_mut(index); index.table_name = to.to_owned(); }); schema.indexes.insert(to.to_owned(), indexes); - }); + }; let mut table = schema .tables