From 115a1343ef17cd473ee4e45444f9ab23bd83f48f Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 14 Jul 2026 15:30:10 +0200 Subject: [PATCH] Run database maintenance after migration Based on the same maintenance tasks appservices runs on the places DB --- glean-core/src/database/sqlite.rs | 48 +++++++++++++++++++ glean-core/src/database/sqlite/connection.rs | 8 +++- glean-core/tests/filled-rkv.data.safe.bin | Bin 0 -> 7071 bytes glean-core/tests/sqlite_migration.rs | 47 ++++++++++++++++++ 4 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 glean-core/tests/filled-rkv.data.safe.bin diff --git a/glean-core/src/database/sqlite.rs b/glean-core/src/database/sqlite.rs index 797f2e6114..6443f5ffa0 100644 --- a/glean-core/src/database/sqlite.rs +++ b/glean-core/src/database/sqlite.rs @@ -171,11 +171,13 @@ impl Database { 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."); @@ -204,6 +206,52 @@ impl Database { } } + /// Run periodic database maintenance. + /// + /// If `force=true` always run the full maintenance taks + pub fn run_maintenance(&self, force: bool) -> Result<()> { + let conn = self.conn.lock(); + let conn = &*conn; + + self.run_maintenance_vacuum(conn, force)?; + self.run_maintenance_optimize(conn)?; + self.run_maintenance_checkpoint(conn)?; + + Ok(()) + } + + /// Run maintenance on the database (vacuum step) + /// + /// If `force_full: true` it _always_ runs a full `VACUUM`. + fn run_maintenance_vacuum(&self, conn: &rusqlite::Connection, force_full: bool) -> Result<()> { + let auto_vacuum_setting: u32 = + conn.query_row_and_then("PRAGMA auto_vacuum", [], |row| row.get(0))?; + if !force_full && auto_vacuum_setting == 2 { + // Ideally, we run an incremental vacuum to delete 2 pages + conn.execute("PRAGMA incremental_vacuum(2)", [])?; + } else { + // If auto_vacuum=incremental isn't set, configure it and run a full vacuum. + log::warn!( + "run_maintenance_vacuum: Need to run a full vacuum to set auto_vacuum=incremental" + ); + conn.execute("PRAGMA auto_vacuum=incremental", [])?; + conn.execute("VACUUM", [])?; + } + Ok(()) + } + + /// Run maintenance on the database (optimize step) + fn run_maintenance_optimize(&self, conn: &rusqlite::Connection) -> Result<()> { + conn.execute("PRAGMA optimize", [])?; + Ok(()) + } + + /// Run maintenance on the database (checkpoint step) + fn run_maintenance_checkpoint(&self, conn: &rusqlite::Connection) -> Result<()> { + conn.query_row("PRAGMA wal_checkpoint(PASSIVE)", [], |_| Ok(()))?; + Ok(()) + } + /// Iterates with the provided transaction function /// over the requested data from the given storage. /// diff --git a/glean-core/src/database/sqlite/connection.rs b/glean-core/src/database/sqlite/connection.rs index 2e8aa54869..25eb37149d 100644 --- a/glean-core/src/database/sqlite/connection.rs +++ b/glean-core/src/database/sqlite/connection.rs @@ -7,7 +7,8 @@ //! This module is inspired by, and borrows concepts from, the //! Application Services `sql-support` crate. -use std::{fmt::Debug, num::NonZeroU32, path::Path, sync::Mutex}; +use std::sync::{Mutex, MutexGuard}; +use std::{fmt::Debug, num::NonZeroU32, path::Path}; use rusqlite::{OpenFlags, Transaction, TransactionBehavior}; @@ -82,6 +83,11 @@ impl Connection { } } + /// Get ahold of the connection with no transaction opened. + pub fn lock<'a>(&'a self) -> MutexGuard<'a, rusqlite::Connection> { + self.conn.lock().unwrap() + } + /// Accesses the database for reading. pub fn read(&self, f: impl FnOnce(&Transaction<'_>) -> Result) -> Result { let mut conn = self.conn.lock().unwrap(); diff --git a/glean-core/tests/filled-rkv.data.safe.bin b/glean-core/tests/filled-rkv.data.safe.bin new file mode 100644 index 0000000000000000000000000000000000000000..6e6cefb60ccf2502f9a9d26b80ad45a631a94ab5 GIT binary patch literal 7071 zcmb^#ON<;xbvAY!IwXAJ00t9cuLBN=r?)@zwGxF-6d{3>0ILuQS)H2hnwj$Uba%R{ zcfF2GAm##+FNg~VzEL=UK?uoG;F9D3Ckuo`KtdomK!hSi0!2bX0>gXN^?GM}d&ZkU z$=<2zSM@&iUcKLeRykGUL>#@xY_Z;M zCO5$^op)^4cHX}fNhD+zv+ZD$=TfA}_0hd=F}ggQ3dKWJ)c+hT6?8SB}s1GW;Ws3LuURU%eO>AB$|$sI7`uZ9lK{cD+dkMLh>A^qzgK`7e6T? z8k(b)LlgMD%glY(?L@<9+#WLSw>v)Tw+H^nY5U!d6Z(9_27?}5_g)&2$kp~psJ9Si z(y3mrny-7$&A7@%C{IZKM8T7glUZ1AGDlY67e7{VXCj#(tc!nOklcicMN!VBx*XcX85IB{K#J}AHmNkKaTM?zV%N0 zrSf_<3FF8U*jCdb4NaLE1r4slNauLF8})jWVyt7 znDa=eAY^%DvqGg~5yt^f*eK>vvt8C@pU1F2>p6yu!D!+^ZYSYV`YUz%5#JOcN9L#C zIz_0hu95W>dcs6_EOI_h&jnM^U68Q}x3e_d;3|+R%>|dirjP<|q8Zymr4*GI`k~r9 z&(qu%$tH_M1c;d8YjVnRZbzJFu}C(wSqDWPBQ#e3Zvn{n8^8jV(AH`pT&!n8;f zeCwDGhz$srupP?tm^5rRnU3rAoqm^)5}oos-k-$zm(n;6K);LEg`n$OO~>u}j_aEJM4xYh6Oa{QvK%xnG>HY4 z)9Vdguj}^wMp^xi-|c&vlr}4(HDck$BnR7!+C>I}^NOr79nbd%p1(koJN!4=D%#E- zTdhe)7CIO#z0@A6@PD~f2*VL%5CNtUd+*k{aHgJ5m^TF}1!Mp+kenxt?g;(car7-P z$SId+i!4rAWJ{nN$RZ2@)vM*cmi1>d!FWp9Nj;wW4?lw7D&AgwSlx+)->_1z!0|A2Wg#p{y>u={p&aM6Lzh8PB!QeGqw#fs}DMqb->+g@+o z9Z+KA!UXe9%A^((N^@RXd+CoG6^a?Cdp(x58G%+uGoc`8tc{rD5Ez^DSk%(m>%~~U zlSU5la$h&TGB`Hk!-(1(qUV%Um)KnLLWFf?V1lCa6A5t{lj=>ux8UYfngtT#mTbg? zYOM!*kwDTBLvk0)(({n(u^`LSs0fv`$7xu|##Eb-HmDaqF0W( z4p^ybAXSupP^C}@VY1t(ZKP4x_Fg=giRx50*b-{G5RtAm03qE3^bf!V2Q!ucXSp4w zNn$KfBti+ubGoLX8`-;GK0IDWATzX-B@x0?DGgGV|?hvKR%A(ht~dU2d^NULeevTN*#m|{L?@9 z_s8eglb5Zt^J_ed<8QZLyNGdsMC$9W`_}#@3yWg5bM5~Z<$s{g0~CHd$L0w-pH#w| z$i*h4aDif@1y1nz5**`mEQr&|1pKN$&>JnzKpMmL`gsO}ucbJ_Pr9OxX^@9UigSynevs~`H&^hL?s7K+zk2XryT6!4k z`6N|h+mt#1oK!Q+%-AY9z+n zphp}1c(j`UI`EjH*HnvM~?btv+o;{pm=D}UN4baToK8!+x_Y~m|$ z4Xyp5JkRsAU0Oq9h(ZDCuXBe5aPJ=Jd8b=ppM-5s9xyQlr3J?Oh-UljvJGqE&{u_rncQ3M(u6-r@4cst`u*@s^7 zIvbG-Bpq+@5oW!%&S|hXyx#;Wt79f;{Kt*ZCRSC(`mYCnt082}-@EdZex+o*RPWK6 zf9^XQc!1XTOn#=%)B47zE?V>OOJ8_Ng9v{0@UJyjhP77z(4+cY65Pbr3P0e|TM2Gr zYZZ)t{=zG}=+__LKfm-teU#)||HYT}n(+PR+OzsR@pI)LYxE~ZW`>FUhxkj~q?eLjh&ZAzx-}D( UuidMetric { UuidMetric::new(CommonMetricData { @@ -218,3 +225,43 @@ fn migration_fails() { assert_eq!(None, metrics.failed_metrics.get_value(&glean, None)); assert_eq!(None, metrics.migration_duration.get_value(&glean, None)); } + +#[test] +fn migration_checkpoints() { + let temp = tempfile::tempdir().unwrap(); + let db_path = temp.path().join("db"); + fs::create_dir_all(&db_path).unwrap(); + + let safe_bin = db_path.join("data.safe.bin"); + // Reusing the same database file from above. + fs::write(safe_bin, FILLED_RKV_DATABASE).unwrap(); + let exp_client_id = uuid!("3114d9df-9ae3-43a7-83b0-3540c3eba886"); + + 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(61), metrics.migrated_metrics.get_value(&glean, None)); + assert_eq!(Some(61), metrics.metrics_in_sqlite.get_value(&glean, None)); + + assert!(metrics.migration_duration.get_value(&glean, None).is_some()); + assert_eq!(None, metrics.migration_error.get_value(&glean, None)); + + // Ensure we close the database connection. + drop(glean); + + let db_file = db_path.join("glean.sqlite"); + let db_file_size = fs::metadata(db_file).unwrap().size(); + + // This test is very vague, but it's hard to do better right now. + // + // 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). + // 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; + assert!(db_file_size >= vacuumed_database_size); +}