From fbec5e8dd5e0ff7abf45a3c02030680add8a1375 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 5 Sep 2025 18:09:50 -0700 Subject: [PATCH] Disable foreign keys in sqlite when running migrations (#37572) Closes #37473 Previously, we enabled foreign keys at all times for our sqlite database that we use for client-side state. The problem with this is that In sqlite, `alter table` is somewhat limited, so for many migrations, you must *recreate* the table: create a new table called e.g. `workspace__2`, then copy all of the data from `workspaces` into `workspace__2`, then delete the old `workspaces` table and rename `workspaces__2` to `workspaces`. The way foreign keys work in sqlite, when we delete the old table, all of its associated records in other tables will be deleted due to `on delete cascade` clauses. Unfortunately, one of the types of associated records that can be deleted are `editors`, which sometimes store unsaved text. It is very bad to delete these records, as they are the *only* place that this unsaved text is stored. This has already happened multiple times as we have migrated tables as we develop Zed, but I caused it to happened again in https://github.com/zed-industries/zed/pull/36714. The Sqlite docs recommend a multi-step approach to migrations where you: * disable foreign keys * start a transaction * create a new table * populate the new table with data from the old table * delete the old table * rename the new table to the old name * run a foreign key check * if it passes, commit the transaction * enable foreign keys In this PR, I've adjusted our sqlite migration code path to follow this pattern more closely. Specifically, we disable foreign key checks before running migrations, run a foreign key check before committing, and then enable foreign key checks after the migrations are done. In addition, I've added a generic query that we run *before* running the foreign key check that explicitly deletes any rows that have dangling foreign keys. This way, we avoid failing the migration (and breaking the app) if a migration deletes data that *does* cause associated records to need to be deleted. But now, in the common case where we migrate old data in the new table and keep the ids, all of the associated data will be preserved. Release Notes: - Fixed a bug where workspace state would be lost when upgrading from Zed 0.201.x. or below. --- Cargo.lock | 1 + crates/sqlez/Cargo.toml | 1 + crates/sqlez/src/migrations.rs | 51 ++++++++++++++++++++-- crates/sqlez/src/thread_safe_connection.rs | 11 +++++ 4 files changed, 61 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0792a8b618c3d9839bc397784b584a8d5e4be2ef..308b3ed8a2163896ecae8a6a1bc4c07bedac7571 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -15313,6 +15313,7 @@ dependencies = [ "futures 0.3.31", "indoc", "libsqlite3-sys", + "log", "parking_lot", "smol", "sqlformat", diff --git a/crates/sqlez/Cargo.toml b/crates/sqlez/Cargo.toml index 16a3adebae24e0573f9e7ada18bc9259ff588ad1..6eb75aa171979283325d22300f95d584cee2cffb 100644 --- a/crates/sqlez/Cargo.toml +++ b/crates/sqlez/Cargo.toml @@ -14,6 +14,7 @@ collections.workspace = true futures.workspace = true indoc.workspace = true libsqlite3-sys.workspace = true +log.workspace = true parking_lot.workspace = true smol.workspace = true sqlformat.workspace = true diff --git a/crates/sqlez/src/migrations.rs b/crates/sqlez/src/migrations.rs index 2429ddeb4127591b56fb74a9c84884d9dc5f378f..567d82f9afe22ea4ab126c0989891c5d603879fd 100644 --- a/crates/sqlez/src/migrations.rs +++ b/crates/sqlez/src/migrations.rs @@ -59,6 +59,7 @@ impl Connection { let mut store_completed_migration = self .exec_bound("INSERT INTO migrations (domain, step, migration) VALUES (?, ?, ?)")?; + let mut did_migrate = false; for (index, migration) in migrations.iter().enumerate() { let migration = sqlformat::format(migration, &sqlformat::QueryParams::None, Default::default()); @@ -70,9 +71,7 @@ impl Connection { &sqlformat::QueryParams::None, Default::default(), ); - if completed_migration == migration - || migration.trim().starts_with("-- ALLOW_MIGRATION_CHANGE") - { + if completed_migration == migration { // Migration already run. Continue continue; } else if should_allow_migration_change(index, &completed_migration, &migration) @@ -91,12 +90,58 @@ impl Connection { } self.eager_exec(&migration)?; + did_migrate = true; store_completed_migration((domain, index, migration))?; } + if did_migrate { + self.delete_rows_with_orphaned_foreign_key_references()?; + self.exec("PRAGMA foreign_key_check;")?()?; + } + Ok(()) }) } + + /// Delete any rows that were orphaned by a migration. This is needed + /// because we disable foreign key constraints during migrations, so + /// that it's possible to re-create a table with the same name, without + /// deleting all associated data. + fn delete_rows_with_orphaned_foreign_key_references(&self) -> Result<()> { + let foreign_key_info: Vec<(String, String, String, String)> = self.select( + r#" + SELECT DISTINCT + schema.name as child_table, + foreign_keys.[from] as child_key, + foreign_keys.[table] as parent_table, + foreign_keys.[to] as parent_key + FROM sqlite_schema schema + JOIN pragma_foreign_key_list(schema.name) foreign_keys + WHERE + schema.type = 'table' AND + schema.name NOT LIKE "sqlite_%" + "#, + )?()?; + + if !foreign_key_info.is_empty() { + log::info!( + "Found {} foreign key relationships to check", + foreign_key_info.len() + ); + } + + for (child_table, child_key, parent_table, parent_key) in foreign_key_info { + self.exec(&format!( + " + DELETE FROM {child_table} + WHERE {child_key} IS NOT NULL and {child_key} NOT IN + (SELECT {parent_key} FROM {parent_table}) + " + ))?()?; + } + + Ok(()) + } } #[cfg(test)] diff --git a/crates/sqlez/src/thread_safe_connection.rs b/crates/sqlez/src/thread_safe_connection.rs index 58d3afe78fb4d8b211c48c0ae1f9f72af74ad5c1..482905ac817bf94fcb64cb858b784c94283b686c 100644 --- a/crates/sqlez/src/thread_safe_connection.rs +++ b/crates/sqlez/src/thread_safe_connection.rs @@ -95,6 +95,14 @@ impl ThreadSafeConnectionBuilder { let mut migration_result = anyhow::Result::<()>::Err(anyhow::anyhow!("Migration never run")); + let foreign_keys_enabled: bool = + connection.select_row::("PRAGMA foreign_keys")?() + .unwrap_or(None) + .map(|enabled| enabled != 0) + .unwrap_or(false); + + connection.exec("PRAGMA foreign_keys = OFF;")?()?; + for _ in 0..MIGRATION_RETRIES { migration_result = connection .with_savepoint("thread_safe_multi_migration", || M::migrate(connection)); @@ -104,6 +112,9 @@ impl ThreadSafeConnectionBuilder { } } + if foreign_keys_enabled { + connection.exec("PRAGMA foreign_keys = ON;")?()?; + } migration_result }) .await?;