From c08a05ca9e35f67d6b67031d8d5b179b53ce6781 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 14 Jul 2026 15:31:27 +0200 Subject: [PATCH 1/2] Put some state about the migration INTO the database --- glean-core/src/database/migration.rs | 18 +++++++++++ glean-core/src/database/sqlite.rs | 5 +++ glean-core/src/database/sqlite/schema.rs | 40 ++++++++++++++++++++---- glean-core/tests/sqlite.rs | 38 +++++++++++++++++++++- 4 files changed, 94 insertions(+), 7 deletions(-) diff --git a/glean-core/src/database/migration.rs b/glean-core/src/database/migration.rs index 0518c931b8..244273178f 100644 --- a/glean-core/src/database/migration.rs +++ b/glean-core/src/database/migration.rs @@ -14,6 +14,7 @@ use crate::Lifetime; use crate::Result; use rkv::StoreOptions; +use rusqlite::OptionalExtension; use rusqlite::Transaction; use super::sqlite; @@ -264,6 +265,23 @@ pub fn try_migrate(data_path: &Path, db: &sqlite::Database) -> Result = conn + .query_row("SELECT state FROM migration", [], |row| row.get(0)) + .optional()?; + + if matches!(migration_status.as_deref(), Some("done")) { + log::debug!("Rkv already successfully migrated. Not doing it again."); + return Ok(None); + } + } + + db.conn.write(|tx| { + tx.execute("INSERT INTO migration (id, state) VALUES (1, 'started') ON CONFLICT(id) DO UPDATE SET state = excluded.state", [])?; + Ok::<(), rusqlite::Error>(()) + })?; + let rkv = RkvDatabase::new(data_path)?; let state = db.conn.write(|tx| { diff --git a/glean-core/src/database/sqlite.rs b/glean-core/src/database/sqlite.rs index 9d3de4ebee..a551124dc8 100644 --- a/glean-core/src/database/sqlite.rs +++ b/glean-core/src/database/sqlite.rs @@ -236,6 +236,11 @@ impl Database { } } + db.conn.write(|tx| { + tx.execute("INSERT INTO migration (id, state) VALUES (1, 'done') ON CONFLICT(id) DO UPDATE SET state = excluded.state", [])?; + Ok::<(), rusqlite::Error>(()) + })?; + Ok(db) } diff --git a/glean-core/src/database/sqlite/schema.rs b/glean-core/src/database/sqlite/schema.rs index 23500cd550..619d26e3bd 100644 --- a/glean-core/src/database/sqlite/schema.rs +++ b/glean-core/src/database/sqlite/schema.rs @@ -6,7 +6,7 @@ use std::num::NonZeroU32; -use rusqlite::{config::DbConfig, Transaction}; +use rusqlite::{config::DbConfig, OptionalExtension, Transaction}; use super::connection::ConnectionOpener; @@ -16,7 +16,7 @@ use super::connection::ConnectionOpener; pub struct Schema; impl ConnectionOpener for Schema { - const MAX_SCHEMA_VERSION: u32 = 1; + const MAX_SCHEMA_VERSION: u32 = 2; type Error = SchemaError; @@ -52,20 +52,48 @@ impl ConnectionOpener for Schema { fn create(tx: &mut Transaction<'_>) -> Result<(), Self::Error> { tx.execute_batch( - "CREATE TABLE telemetry( + " + CREATE TABLE telemetry( id TEXT NOT NULL, ping TEXT NOT NULL, lifetime TEXT NOT NULL, labels TEXT NOT NULL, -- can't be null or ON CONFLICT won't work value BLOB, UNIQUE(id, ping, labels) - );", + ); + CREATE TABLE migration(id INTEGER PRIMARY KEY, state TEXT NOT NULL); + ", )?; Ok(()) } - fn upgrade(_: &mut Transaction<'_>, to_version: NonZeroU32) -> Result<(), Self::Error> { - Err(SchemaError::UnsupportedSchemaVersion(to_version.get())) + fn upgrade(tx: &mut Transaction<'_>, to_version: NonZeroU32) -> Result<(), Self::Error> { + match to_version.get() { + 2 => { + log::info!("Upgrading user_version to 2"); + // Clients upgrading to schema 2 don't have the table. + // But they did run through the migration. + tx.execute_batch( + "CREATE TABLE migration( + id INTEGER PRIMARY KEY, + state TEXT NOT NULL + );", + )?; + let cid_exists: Option = tx + .query_row( + "SELECT 1 FROM telemetry WHERE id = 'client_id'", + [], + |row| row.get(0), + ) + .optional()?; + if cid_exists.is_some() { + log::info!("Client ID already exists. Marking migration as done."); + tx.execute("INSERT INTO migration (id, state) VALUES (1, 'done') ON CONFLICT(id) DO UPDATE SET state = excluded.state", [])?; + } + Ok(()) + } + to_version => Err(SchemaError::UnsupportedSchemaVersion(to_version)), + } } fn validate(tx: &mut Transaction<'_>) -> Result<(), Self::Error> { diff --git a/glean-core/tests/sqlite.rs b/glean-core/tests/sqlite.rs index 35a68534c7..a769b3ccf4 100644 --- a/glean-core/tests/sqlite.rs +++ b/glean-core/tests/sqlite.rs @@ -150,7 +150,7 @@ fn higher_user_version_upgrade_does_not_crash() { { let path = temp.path().join("db").join("glean.sqlite"); let conn = rusqlite::Connection::open(path).unwrap(); - conn.execute_batch("PRAGMA user_version = 2").unwrap(); + conn.execute_batch("PRAGMA user_version = 99").unwrap(); } let (glean, _temp) = new_glean(Some(temp)); @@ -255,3 +255,39 @@ fn database_externally_locked() { let glean = Glean::new(cfg); assert!(glean.is_err()); } + +#[test] +fn schema_v2_is_applied() { + let (first_client_id, temp) = { + let (glean, temp) = new_glean(None); + let client_id = clientid_metric().get_value(&glean, None).unwrap(); + drop(glean); + + let db_path = temp.path().join("db").join("glean.sqlite"); + let conn = rusqlite::Connection::open(db_path).unwrap(); + + conn.execute("DROP TABLE migration", []).unwrap(); + + // Reset to first schema version + conn.execute("PRAGMA user_version = 1", []).unwrap(); + + (client_id, temp) + }; + + let db_path = temp.path().join("db").join("glean.sqlite"); + let (glean, _temp) = new_glean(Some(temp)); + + let client_id = clientid_metric().get_value(&glean, None).unwrap(); + assert_eq!(first_client_id, client_id); + + let conn = rusqlite::Connection::open(db_path).unwrap(); + let cur_user_version: u32 = conn + .query_one("PRAGMA user_version", [], |row| row.get(0)) + .unwrap(); + assert_eq!(cur_user_version, 2); + + let migration_state: String = conn + .query_one("SELECT state FROM migration", [], |row| row.get(0)) + .unwrap(); + assert_eq!(migration_state, "done"); +} From 00adbddffc879720236de3c9b093e62a63d7f0ee Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 14 Jul 2026 15:31:27 +0200 Subject: [PATCH 2/2] Don't check for absence of SQLite file, always attempt the rkv->sqlite migration --- glean-core/src/database/sqlite.rs | 31 +++++------ glean-core/tests/sqlite_migration.rs | 81 +++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 20 deletions(-) diff --git a/glean-core/src/database/sqlite.rs b/glean-core/src/database/sqlite.rs index a551124dc8..d2e8e4ba4e 100644 --- a/glean-core/src/database/sqlite.rs +++ b/glean-core/src/database/sqlite.rs @@ -205,7 +205,6 @@ impl Database { fs::create_dir_all(&path)?; let store_path = path.join(DEFAULT_DATABASE_FILE_NAME); - let sqlite_exists = store_path.exists(); let (conn, load_state) = sqlite_open(&store_path)?; let mut db = Self { @@ -216,23 +215,19 @@ impl Database { migration_error: MigrationResult::Unknown, }; - if sqlite_exists { - log::debug!("SQLite database already exists. Not trying to migrate Rkv"); - db.run_maintenance(false)?; - } else { - match migration::try_migrate(&path, &db) { - Ok(Some(state)) => { - log::debug!("Migration done. state={state:?}"); - db.migration_state = Some(state); - db.run_maintenance(true)?; - } - Ok(None) => { - log::debug!("No migration."); - } - Err(e) => { - db.migration_error = MigrationResult::Error; - log::warn!("Migration failed! Continuing with SQLite backend without migrated data. Error: {e:?}") - } + match migration::try_migrate(&path, &db) { + Ok(Some(state)) => { + log::debug!("Migration done. state={state:?}"); + db.migration_state = Some(state); + db.run_maintenance(true)?; + } + Ok(None) => { + log::debug!("No migration."); + db.run_maintenance(false)?; + } + Err(e) => { + db.migration_error = MigrationResult::Error; + log::warn!("Migration failed! Continuing with SQLite backend without migrated data. Error: {e:?}") } } diff --git a/glean-core/tests/sqlite_migration.rs b/glean-core/tests/sqlite_migration.rs index c70ecdc71a..f19524adfe 100644 --- a/glean-core/tests/sqlite_migration.rs +++ b/glean-core/tests/sqlite_migration.rs @@ -259,9 +259,86 @@ fn migration_checkpoints() { // // Unvacuumed the database is _smaller_, around 20k bytes, because the migrated data is in the WAL file. // Vacuumed & checkpointed the WAL transactions are merged into the database. - // As of writing that database is at least 8 pages big (8 * 4096 bytes = 32768 bytes). + // As of writing that database is at least 9 pages big (9 * 4096 bytes = 36864 bytes). // This might grow if we add more metrics. // This might shrink if we remove metrics, in which case this test will break and needs adjustement. - let vacuumed_database_size = 32768; + let vacuumed_database_size = 36864; assert!(db_file_size >= vacuumed_database_size); } + +#[test] +fn migration_reapplied_after_not_finishing() { + let temp = { + let (glean, temp) = new_glean(None); + drop(glean); + + let db_path = temp.path().join("db").join("glean.sqlite"); + let conn = rusqlite::Connection::open(db_path).unwrap(); + + // Let's start with an empty database. + conn.execute("DELETE FROM telemetry", []).unwrap(); + + // Ensure migration isn't marked as done. + conn.execute("DELETE FROM migration", []).unwrap(); + temp + }; + + let db_path = temp.path().join("db"); + + let safe_bin = db_path.join("data.safe.bin"); + // Reusing the same database file from above. + fs::write(safe_bin, RKV_DATABASE).unwrap(); + let exp_client_id = uuid!("77ca0472-5124-4f6b-971d-4a2a928fb158"); + + let (glean, _temp) = new_glean(Some(temp)); + + let client_id = clientid_metric().get_value(&glean, None).unwrap(); + assert_eq!(exp_client_id, client_id); + + let metrics = MigrationMetrics::new(); + assert_eq!(Some(13), metrics.migrated_metrics.get_value(&glean, None)); + assert_eq!(Some(13), metrics.metrics_in_sqlite.get_value(&glean, None)); + assert_eq!(None, metrics.failed_metrics.get_value(&glean, None)); + + assert!(metrics.migration_duration.get_value(&glean, None).is_some()); + assert_eq!(None, metrics.migration_error.get_value(&glean, None)); +} + +#[test] +fn migration_not_reapplied_if_marked_as_finished() { + let (first_client_id, temp) = { + let (glean, temp) = new_glean(None); + let client_id = clientid_metric().get_value(&glean, None).unwrap(); + drop(glean); + + let db_path = temp.path().join("db").join("glean.sqlite"); + let conn = rusqlite::Connection::open(db_path).unwrap(); + + // Ensure migration is marked as done. + conn.execute("DELETE FROM migration", []).unwrap(); + conn.execute("INSERT INTO migration (id, state) VALUES (1, 'done')", []) + .unwrap(); + (client_id, temp) + }; + + let db_path = temp.path().join("db"); + + let safe_bin = db_path.join("data.safe.bin"); + // Reusing the same database file from above. + fs::write(safe_bin, RKV_DATABASE).unwrap(); + let rkv_client_id = uuid!("77ca0472-5124-4f6b-971d-4a2a928fb158"); + + let (glean, _temp) = new_glean(Some(temp)); + + let client_id = clientid_metric().get_value(&glean, None).unwrap(); + assert_ne!(rkv_client_id, client_id); + assert_eq!(first_client_id, client_id); + + // No migration happened. + let metrics = MigrationMetrics::new(); + assert_eq!(None, metrics.migrated_metrics.get_value(&glean, None)); + assert_eq!(None, metrics.metrics_in_sqlite.get_value(&glean, None)); + assert_eq!(None, metrics.failed_metrics.get_value(&glean, None)); + assert_eq!(None, metrics.migration_duration.get_value(&glean, None)); + assert_eq!(None, metrics.migration_error.get_value(&glean, None)); +}