Cargo.lock 🔗
@@ -15347,6 +15347,7 @@ dependencies = [
"futures 0.3.31",
"indoc",
"libsqlite3-sys",
+ "log",
"parking_lot",
"smol",
"sqlformat",
Disable foreign keys in sqlite when running migrations (#37572)
Max Brunsfeld created
Closes #37473
### Background
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 Fix
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.Change summary
Cargo.lock | 1
crates/sqlez/Cargo.toml | 1
crates/sqlez/src/migrations.rs | 51 ++++++++++++++++++++++-
crates/sqlez/src/thread_safe_connection.rs | 11 +++++
crates/workspace/src/persistence.rs | 3 +
5 files changed, 64 insertions(+), 3 deletions(-)
Detailed changes
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
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)]
crates/sqlez/src/thread_safe_connection.rs 🔗
@@ -95,6 +95,14 @@ impl<M: Migrator> ThreadSafeConnectionBuilder<M> {
let mut migration_result =
anyhow::Result::<()>::Err(anyhow::anyhow!("Migration never run"));
+ let foreign_keys_enabled: bool =
+ connection.select_row::<i32>("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<M: Migrator> ThreadSafeConnectionBuilder<M> {
}
}
+ if foreign_keys_enabled {
+ connection.exec("PRAGMA foreign_keys = ON;")?()?;
+ }
migration_result
})
.await?;
crates/workspace/src/persistence.rs 🔗
@@ -699,6 +699,9 @@ impl Domain for WorkspaceDb {
PRIMARY KEY (workspace_id, worktree_id, relative_worktree_path, language_name, name, path, raw_json)
) STRICT;),
+ sql!(
+ DROP TABLE ssh_connections;
+ ),
];
// Allow recovering from bad migration that was initially shipped to nightly