From 6bff9e53e5bb836ff29e887dfb71db3ce8e6b350 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Tue, 30 Sep 2025 10:00:16 +0300 Subject: [PATCH 1/4] Fix index bookkeeping in DROP COLUMN See #3448 which this issue closes. --- core/translate/alter.rs | 10 ++++--- core/vdbe/execute.rs | 58 +++++++++++++++++++++++++++-------------- 2 files changed, 45 insertions(+), 23 deletions(-) diff --git a/core/translate/alter.rs b/core/translate/alter.rs index 5875b201f..77a1949ea 100644 --- a/core/translate/alter.rs +++ b/core/translate/alter.rs @@ -119,13 +119,15 @@ pub fn translate_alter_table( for index in table_indexes.iter() { // Referenced in regular index - if index + let maybe_indexed_col = index .columns .iter() - .any(|col| col.pos_in_table == dropped_index) - { + .enumerate() + .find(|(_, col)| col.pos_in_table == dropped_index); + if let Some((pos_in_index, indexed_col)) = maybe_indexed_col { return Err(LimboError::ParseError(format!( - "cannot drop column \"{column_name}\": indexed" + "cannot drop column \"{column_name}\": it is referenced in the index {}; position in index is {pos_in_index}, position in table is {}", + index.name, indexed_col.pos_in_table ))); } // Referenced in partial index diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index af75bd83b..1a36b5f9d 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -7921,29 +7921,49 @@ pub fn op_drop_column( btree.columns.remove(*column_index) }); - let schema = conn.schema.read(); - if let Some(indexes) = schema.indexes.get(&normalized_table_name) { - for index in indexes { - if index - .columns - .iter() - .any(|column| column.pos_in_table == *column_index) - { - return Err(LimboError::ParseError(format!( - "cannot drop column \"{column_name}\": indexed" - ))); + { + let schema = conn.schema.read(); + if let Some(indexes) = schema.indexes.get(&normalized_table_name) { + for index in indexes { + if index + .columns + .iter() + .any(|column| column.pos_in_table == *column_index) + { + return Err(LimboError::ParseError(format!( + "cannot drop column \"{column_name}\": indexed" + ))); + } } } } - for (view_name, view) in schema.views.iter() { - let view_select_sql = format!("SELECT * FROM {view_name}"); - conn.prepare(view_select_sql.as_str()).map_err(|e| { - LimboError::ParseError(format!( - "cannot drop column \"{}\": referenced in VIEW {view_name}: {}", - column_name, view.sql, - )) - })?; + // Update index.pos_in_table for all indexes. + // For example, if the dropped column had index 2, then anything that was indexed on column 3 or higher should be decremented by 1. + conn.with_schema_mut(|schema| { + if let Some(indexes) = schema.indexes.get_mut(&normalized_table_name) { + for index in indexes { + let index = Arc::make_mut(index); + for index_column in index.columns.iter_mut() { + if index_column.pos_in_table > *column_index { + index_column.pos_in_table -= 1; + } + } + } + } + }); + + { + let schema = conn.schema.read(); + for (view_name, view) in schema.views.iter() { + let view_select_sql = format!("SELECT * FROM {view_name}"); + conn.prepare(view_select_sql.as_str()).map_err(|e| { + LimboError::ParseError(format!( + "cannot drop column \"{}\": referenced in VIEW {view_name}: {}", + column_name, view.sql, + )) + })?; + } } state.pc += 1; From 38e08253c88710cafe4384f855be16083fc0059c Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Tue, 30 Sep 2025 10:01:18 +0300 Subject: [PATCH 2/4] Unignore ALTER TABLE fuzz test We caught a pretty bad bug quite late because this fuzz test only ran on btree changes - let's run it on every CI run but with less iterations than before. --- tests/integration/fuzz/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/fuzz/mod.rs b/tests/integration/fuzz/mod.rs index f5e96dee2..f343bc043 100644 --- a/tests/integration/fuzz/mod.rs +++ b/tests/integration/fuzz/mod.rs @@ -2494,7 +2494,6 @@ mod tests { } #[test] - #[ignore] pub fn fuzz_long_create_table_drop_table_alter_table() { let db = TempDatabase::new_empty(true); let limbo_conn = db.connect_limbo(); @@ -2515,7 +2514,7 @@ mod tests { let mut undroppable_cols = HashSet::new(); - for iteration in 0..50000 { + for iteration in 0..5000 { println!("iteration: {iteration} (seed: {seed})"); let operation = rng.random_range(0..100); // 0: create, 1: drop, 2: alter, 3: alter rename From 67be0478e420d351130da65d05900993d630af01 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Tue, 30 Sep 2025 10:04:31 +0300 Subject: [PATCH 3/4] Add TCL regression test for DROP COLUMN issue #3448 --- testing/alter_table.test | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/testing/alter_table.test b/testing/alter_table.test index 5a856eca9..e858d7e7b 100755 --- a/testing/alter_table.test +++ b/testing/alter_table.test @@ -211,3 +211,12 @@ do_execsql_test_on_specific_db {:memory:} alter-table-add-notnull-col { ALTER TABLE t ADD b NOT NULL; SELECT sql FROM sqlite_schema WHERE type = 'table' AND name = 't'; } {{CREATE TABLE t (a, b NOT NULL)}} + +# https://github.com/tursodatabase/turso/issues/3448 +do_execsql_test_on_specific_db {:memory:} drop-column-regression { + CREATE TABLE t (id, col1 BLOB, col2 UNIQUE); + INSERT INTO t VALUES ('id', 'col1', 'col2'); + ALTER TABLE t ADD COLUMN col3 BLOB; + ALTER TABLE t DROP COLUMN col1; + SELECT col3 FROM t; +} {{}} \ No newline at end of file From dc1861d806b6a7000a75d0ad0febe9fb67e6a2fc Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Tue, 30 Sep 2025 10:12:20 +0300 Subject: [PATCH 4/4] Assert we have the only strong reference instead of falling back to COW --- core/vdbe/execute.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 1a36b5f9d..6613d4137 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -7911,7 +7911,7 @@ pub fn op_drop_column( .get_mut(&normalized_table_name) .expect("table being renamed should be in schema"); - let table = Arc::make_mut(table); + let table = Arc::get_mut(table).expect("this should be the only strong reference"); let Table::BTree(btree) = table else { panic!("only btree tables can be renamed"); @@ -7943,7 +7943,7 @@ pub fn op_drop_column( conn.with_schema_mut(|schema| { if let Some(indexes) = schema.indexes.get_mut(&normalized_table_name) { for index in indexes { - let index = Arc::make_mut(index); + let index = Arc::get_mut(index).expect("this should be the only strong reference"); for index_column in index.columns.iter_mut() { if index_column.pos_in_table > *column_index { index_column.pos_in_table -= 1;