From 47dd6a4ded6aabc790604bdac8a5f27c8f84ef96 Mon Sep 17 00:00:00 2001 From: Shawn Zivontsis Date: Thu, 23 Jul 2026 11:40:31 -0400 Subject: [PATCH 1/2] Update moz_origins index column order to match desktop This change ports the fix from https://bugzilla.mozilla.org/show_bug.cgi?id=2025999 which replaces the (prefix, host) unique index on moz_origins with a (host, prefix) index instead. This improves the performance of the index by putting the higher-cardinality column first, and also makes queries which don't filter on prefix eligible for the index. Similar to the fix on desktop, we create a new table and copy the data over due to sqlite limitations on modifying constraints. However, we can't use defer_foreign_keys like we do on desktop, since the foreign key here is ON DELETE CASCADE -- therefore we have to also temporarily null out the foreign key references and copy them back afterward. --- .../places/sql/create_shared_schema.sql | 2 +- .../places/sql/create_shared_triggers.sql | 6 +- components/places/src/db/schema.rs | 278 +++++++++++++++++- 3 files changed, 281 insertions(+), 5 deletions(-) diff --git a/components/places/sql/create_shared_schema.sql b/components/places/sql/create_shared_schema.sql index 7073204fa6..64352931f1 100644 --- a/components/places/sql/create_shared_schema.sql +++ b/components/places/sql/create_shared_schema.sql @@ -152,7 +152,7 @@ CREATE TABLE IF NOT EXISTS moz_origins ( host TEXT NOT NULL, rev_host TEXT NOT NULL, frecency INTEGER NOT NULL, -- XXX - why not default of -1 like in moz_places? - UNIQUE (prefix, host) + UNIQUE (host, prefix) ); CREATE INDEX IF NOT EXISTS hostindex ON moz_origins(rev_host); diff --git a/components/places/sql/create_shared_triggers.sql b/components/places/sql/create_shared_triggers.sql index e9232cfd6e..067e15405d 100644 --- a/components/places/sql/create_shared_triggers.sql +++ b/components/places/sql/create_shared_triggers.sql @@ -184,7 +184,7 @@ BEGIN OLD.rev_host, MAX(OLD.frecency, 0) ) - ON CONFLICT(prefix, host) DO UPDATE + ON CONFLICT(host, prefix) DO UPDATE SET frecency = frecency + OLD.frecency WHERE OLD.frecency > 0; @@ -211,7 +211,7 @@ BEGIN get_host_and_port(OLD.url), -MAX(OLD.frecency, 0) ) - ON CONFLICT(prefix, host) DO UPDATE + ON CONFLICT(host, prefix) DO UPDATE SET frecency_delta = frecency_delta - OLD.frecency WHERE OLD.frecency > 0; END; @@ -250,7 +250,7 @@ BEGIN get_host_and_port(NEW.url), MAX(NEW.frecency, 0) - MAX(OLD.frecency, 0) ) - ON CONFLICT(prefix, host) DO UPDATE + ON CONFLICT(host, prefix) DO UPDATE SET frecency_delta = frecency_delta + EXCLUDED.frecency_delta; END; diff --git a/components/places/src/db/schema.rs b/components/places/src/db/schema.rs index 502b492e2d..866705671c 100644 --- a/components/places/src/db/schema.rs +++ b/components/places/src/db/schema.rs @@ -14,7 +14,7 @@ use sql_support::ConnExt; use super::db::{Pragma, PragmaGuard}; -pub const VERSION: u32 = 20; +pub const VERSION: u32 = 21; // Shared schema and temp tables for the read-write and Sync connections. const CREATE_SHARED_SCHEMA_SQL: &str = include_str!("../../sql/create_shared_schema.sql"); @@ -341,6 +341,70 @@ pub fn upgrade_from(db: &Connection, from: u32) -> rusqlite::Result<()> { db.execute("ANALYZE moz_places", [])?; db.execute("ANALYZE moz_historyvisits", [])?; } + 20 => { + // Invert the moz_origins UNIQUE constraint to (host, prefix), so the + // higher cardinality column comes first and queries only filtering on + // host, like the address bar ones, can use the index. + + // Skip the rebuild if the constraint is already inverted. + let already_inverted = db.exists( + "SELECT 1 FROM sqlite_schema + WHERE type = 'table' AND name = 'moz_origins' + AND sql LIKE '%UNIQUE (host, prefix)%'", + [], + )?; + if !already_inverted { + // The table must be rebuilt, and PRAGMA foreign_keys is a no-op + // inside the migration transaction, so the moz_places.origin_id + // foreign key stays enforced throughout. Since origin_id is + // nullable, we stash it and null it out, so that nothing + // references moz_origins while it's swapped out. + // The stash must be keyed, or the restore below would scan it + // for every row. + db.execute_batch( + "CREATE TEMP TABLE moz_places_origin_id_stash ( + id INTEGER PRIMARY KEY, + origin_id INTEGER NOT NULL + ); + + INSERT INTO moz_places_origin_id_stash (id, origin_id) + SELECT id, origin_id FROM moz_places WHERE origin_id IS NOT NULL; + + CREATE TABLE moz_origins_new ( + id INTEGER PRIMARY KEY, + prefix TEXT NOT NULL, + host TEXT NOT NULL, + rev_host TEXT NOT NULL, + frecency INTEGER NOT NULL, + UNIQUE (host, prefix) + ); + + INSERT INTO moz_origins_new (id, prefix, host, rev_host, frecency) + SELECT id, prefix, host, rev_host, frecency FROM moz_origins; + + UPDATE moz_places SET origin_id = NULL WHERE origin_id IS NOT NULL; + + -- A rename would rewrite the REFERENCES clause in moz_places, while a + -- drop leaves it dangling until the new table takes over the name. + DROP TABLE moz_origins; + + ALTER TABLE moz_origins_new RENAME TO moz_origins; + + UPDATE moz_places + SET origin_id = stash.origin_id + FROM moz_places_origin_id_stash AS stash + WHERE moz_places.id = stash.id; + + DROP TABLE moz_places_origin_id_stash;", + )?; + // Recreate hostindex, which was dropped along with the old table, + // by calling the shared schema file + db.execute_batch(CREATE_SHARED_SCHEMA_SQL)?; + // Manually call analyze so the planner has statistics for the + // rebuilt table + db.execute("ANALYZE moz_origins", [])?; + } + } // Add more migrations here... // Any other from value indicates that something very wrong happened @@ -1173,6 +1237,218 @@ mod tests { ); } + #[test] + fn test_upgrade_schema_20_21() { + use std::sync::Arc; + let db_file = MigratedDatabaseFile::new(PlacesInitializer::new_for_test(), CREATE_V17_DB); + db_file.upgrade_to(20); + + // Seed origins, plus pages pointing at them, so the migration has both rows to + // rebuild and foreign keys to keep intact. + let conn = db_file.open(); + conn.execute_batch( + "INSERT INTO moz_origins(id, prefix, host, rev_host, frecency) + VALUES (1, 'https://', 'example.com', 'moc.elpmaxe.', 100), + (2, 'http://', 'example.com', 'moc.elpmaxe.', 50), + (3, 'https://', 'mozilla.org', 'gro.allizom.', 75); + + UPDATE moz_places SET origin_id = 1 WHERE id = 1; + + INSERT INTO moz_places(id, guid, url, origin_id, frecency) + VALUES (2, 'page_guid__2', 'http://example.com/', 2, -1), + (3, 'page_guid__3', 'https://mozilla.org/', 3, -1), + (4, 'page_guid__4', 'https://unvisited.com/', NULL, -1);", + ) + .expect("should seed origins and places"); + + fn unique_index_columns(conn: &Connection) -> Vec { + let indexes = conn + .query_rows_and_then( + "SELECT name FROM pragma_index_list('moz_origins') WHERE origin = 'u'", + [], + |row| row.get::<_, String>(0), + ) + .expect("should query the unique indexes"); + assert_eq!( + indexes.len(), + 1, + "moz_origins should have a single unique index" + ); + conn.query_rows_and_then( + "SELECT name FROM pragma_index_info(?) ORDER BY seqno", + (indexes[0].as_str(),), + |row| row.get::<_, String>(0), + ) + .expect("should query the unique index columns") + } + + // moz_origins should be keyed on (prefix, host) before the migration. The + // upgrades above replay the current shared schema, so check they left the + // constraint alone. + assert_eq!(unique_index_columns(&conn), &["prefix", "host"]); + drop(conn); + + // Open through `PlacesDb`, so the migration runs with foreign keys enforced; + // otherwise the null-out step it relies on goes untested. + let db = PlacesDb::open( + &db_file.path, + ConnectionType::ReadWrite, + 0, + Arc::new(parking_lot::Mutex::new(())), + ) + .expect("should upgrade"); + + // The unique index should now lead with the higher cardinality column. + assert_eq!(unique_index_columns(&db), &["host", "prefix"]); + + // The origins themselves should be untouched, ids included, since moz_places + // references them. + #[derive(Eq, PartialEq, Debug)] + struct OriginRow { + id: i64, + prefix: String, + host: String, + rev_host: String, + frecency: i64, + } + let origins = db + .query_rows_and_then( + "SELECT id, prefix, host, rev_host, frecency FROM moz_origins ORDER BY id", + [], + |row| -> rusqlite::Result<_> { + Ok(OriginRow { + id: row.get("id")?, + prefix: row.get("prefix")?, + host: row.get("host")?, + rev_host: row.get("rev_host")?, + frecency: row.get("frecency")?, + }) + }, + ) + .expect("should query all origins"); + assert_eq!( + origins, + &[ + OriginRow { + id: 1, + prefix: "https://".into(), + host: "example.com".into(), + rev_host: "moc.elpmaxe.".into(), + frecency: 100, + }, + OriginRow { + id: 2, + prefix: "http://".into(), + host: "example.com".into(), + rev_host: "moc.elpmaxe.".into(), + frecency: 50, + }, + OriginRow { + id: 3, + prefix: "https://".into(), + host: "mozilla.org".into(), + rev_host: "gro.allizom.".into(), + frecency: 75, + }, + ] + ); + + // ...And every page should still point at the origin it did before. + let pages = db + .query_rows_and_then( + "SELECT id, origin_id FROM moz_places ORDER BY id", + [], + |row| -> rusqlite::Result<_> { + Ok((row.get::<_, i64>(0)?, row.get::<_, Option>(1)?)) + }, + ) + .expect("should query all places"); + assert_eq!( + pages, + &[(1, Some(1)), (2, Some(2)), (3, Some(3)), (4, None)] + ); + + // hostindex should have been recreated, since rebuilding the table dropped it. + assert!(db + .exists( + "SELECT 1 FROM sqlite_schema WHERE type = 'index' AND name = 'hostindex'", + [] + ) + .expect("should look for hostindex")); + + // The table used to rebuild moz_origins should have been removed. + assert!(!db + .exists( + "SELECT 1 FROM sqlite_schema WHERE name = 'moz_origins_new'", + [] + ) + .expect("should look for moz_origins_new")); + + // moz_places should still reference the rebuilt moz_origins. + let foreign_key = db + .query_row( + r#"SELECT "table", "from", "to" FROM pragma_foreign_key_list('moz_places')"#, + [], + |row| { + Ok(( + row.get::<_, String>(0)?, + row.get::<_, String>(1)?, + row.get::<_, String>(2)?, + )) + }, + ) + .expect("should query the foreign key"); + assert_eq!( + foreign_key, + ("moz_origins".into(), "origin_id".into(), "id".into()) + ); + + let integrity_ok = db + .query_row("PRAGMA integrity_check", [], |row| { + Ok(row.get::<_, String>(0)? == "ok") + }) + .expect("should perform integrity check"); + assert!(integrity_ok); + + let foreign_keys_ok = db + .prepare("PRAGMA foreign_key_check") + .and_then(|mut statement| Ok(statement.query([])?.next()?.is_none())) + .expect("should perform foreign key check"); + assert!(foreign_keys_ok); + + // The origin-creation trigger should still upsert against the rebuilt + // moz_origins. + db.execute( + "INSERT INTO moz_places(guid, url, url_hash) + VALUES ('page_guid__5', 'https://example.com/new-page', + hash('https://example.com/new-page')), + ('page_guid__6', 'https://example.org/', + hash('https://example.org/'))", + [], + ) + .expect("should insert pages"); + // origins are maintained via triggers, so make sure they are done. + crate::storage::delete_pending_temp_tables(&db).expect("should update origins"); + + // Adding a page for a known origin should update it rather than add a new one... + assert_eq!( + db.conn_ext_query_one::( + "SELECT origin_id FROM moz_places WHERE guid = 'page_guid__5'" + ) + .expect("should query the known origin"), + 1 + ); + // ...And a page for an unknown origin should add one. + assert_eq!( + db.conn_ext_query_one::( + "SELECT COUNT(*) FROM moz_origins + WHERE prefix = 'https://' AND host = 'example.org'" + ) + .expect("should query the new origin"), + 1 + ); + } + #[test] fn test_all_upgrades() { // Test the migration process in general: open a fresh DB and a DB that's gone through the migration From e157a2b8d4d92cd665275d50fa3746ebcc74f29b Mon Sep 17 00:00:00 2001 From: Shawn Zivontsis Date: Sat, 1 Aug 2026 20:09:51 -0400 Subject: [PATCH 2/2] Revise migration 20 to directly modify sqlite_schema Instead of rebuilding the entire moz_origins table, which requires nulling out and then restoring the entire origin_id column in the large moz_places table, we can just rewrite the schema in sqlite_schema and then trigger a REINDEX. This is not as safe, and has the risk of causing silent data corruption if the schema is modified incorrectly (like for example if it was unknowingly modified by application code). But, it is significantly faster and generates significantly less WAL. --- components/places/src/db/schema.rs | 85 +++++++++++++----------------- 1 file changed, 37 insertions(+), 48 deletions(-) diff --git a/components/places/src/db/schema.rs b/components/places/src/db/schema.rs index 866705671c..4218c11db8 100644 --- a/components/places/src/db/schema.rs +++ b/components/places/src/db/schema.rs @@ -354,54 +354,43 @@ pub fn upgrade_from(db: &Connection, from: u32) -> rusqlite::Result<()> { [], )?; if !already_inverted { - // The table must be rebuilt, and PRAGMA foreign_keys is a no-op - // inside the migration transaction, so the moz_places.origin_id - // foreign key stays enforced throughout. Since origin_id is - // nullable, we stash it and null it out, so that nothing - // references moz_origins while it's swapped out. - // The stash must be keyed, or the restore below would scan it - // for every row. - db.execute_batch( - "CREATE TEMP TABLE moz_places_origin_id_stash ( - id INTEGER PRIMARY KEY, - origin_id INTEGER NOT NULL - ); - - INSERT INTO moz_places_origin_id_stash (id, origin_id) - SELECT id, origin_id FROM moz_places WHERE origin_id IS NOT NULL; - - CREATE TABLE moz_origins_new ( - id INTEGER PRIMARY KEY, - prefix TEXT NOT NULL, - host TEXT NOT NULL, - rev_host TEXT NOT NULL, - frecency INTEGER NOT NULL, - UNIQUE (host, prefix) - ); - - INSERT INTO moz_origins_new (id, prefix, host, rev_host, frecency) - SELECT id, prefix, host, rev_host, frecency FROM moz_origins; - - UPDATE moz_places SET origin_id = NULL WHERE origin_id IS NOT NULL; - - -- A rename would rewrite the REFERENCES clause in moz_places, while a - -- drop leaves it dangling until the new table takes over the name. - DROP TABLE moz_origins; - - ALTER TABLE moz_origins_new RENAME TO moz_origins; - - UPDATE moz_places - SET origin_id = stash.origin_id - FROM moz_places_origin_id_stash AS stash - WHERE moz_places.id = stash.id; - - DROP TABLE moz_places_origin_id_stash;", - )?; - // Recreate hostindex, which was dropped along with the old table, - // by calling the shared schema file - db.execute_batch(CREATE_SHARED_SCHEMA_SQL)?; - // Manually call analyze so the planner has statistics for the - // rebuilt table + // PRAGMA foreign_keys is a no-op inside the migration transaction, + // so dropping moz_origins to rebuild it would cascade to every + // page; we rewrite the stored schema in place instead. + // Must not change anything but the constraints; changing the column + // list will silently corrupt existing data. + const NEW_SQL: &str = "CREATE TABLE moz_origins ( \ + id INTEGER PRIMARY KEY, \ + prefix TEXT NOT NULL, \ + host TEXT NOT NULL, \ + rev_host TEXT NOT NULL, \ + frecency INTEGER NOT NULL, \ + UNIQUE (host, prefix))"; + + let schema_version: i64 = + db.query_row("PRAGMA schema_version", [], |row| row.get(0))?; + + { + let _w = PragmaGuard::new(db, Pragma::WritableSchema, true)?; + db.execute( + "UPDATE sqlite_schema SET + sql = ? + WHERE type = 'table' AND name = 'moz_origins'", + // _Must_ be valid SQL; updating `sqlite_schema.sql` with + // invalid SQL will corrupt the database. + rusqlite::params![NEW_SQL], + )?; + } + + // Reload the schema and rebuild the index with the new column order + db.execute_one("PRAGMA writable_schema = RESET")?; + db.execute("REINDEX moz_origins", [])?; + + // Increment the schema version like an ALTER TABLE would, so that + // other connections reload the schema + db.execute_one(&format!("PRAGMA schema_version = {}", schema_version + 1))?; + + // Manually call analyze so the planner can start using the index immediately db.execute("ANALYZE moz_origins", [])?; } }