Import sqlite storage as separate module#3449
Conversation
This is a modified version of the kvstore/skv implementation: https://searchfox.org/firefox-main/rev/cced10961b53e0d29e22e635404fec37728b2644/toolkit/components/kvstore/src/skv/connection.rs Which itself is based on application-service's sql-support. It's stripped down to what we need in Glean: * A file-backed database * A schema set up on start, potentially applying migrations if we need that * A read-write connection, which is re-used for all access.
badboy
force-pushed
the
sqlite/initial-import
branch
from
b65fc42 to
d3d4e1b
Compare
| "PRAGMA journal_mode = WAL; | ||
| PRAGMA journal_size_limit = 512000; -- 512 KB. | ||
| PRAGMA temp_store = MEMORY; | ||
| PRAGMA auto_vacuum = INCREMENTAL; |
There was a problem hiding this comment.
Gonna want comments about why these values.
| ", | ||
| )?; | ||
|
|
||
| // Set hardening flags. |
There was a problem hiding this comment.
Having now read the doc, I'd group this ("ordinary SQL able to deliberately corrupt the db") with misfeatures
|
|
||
| self.conn | ||
| .read(|conn| { | ||
| let mut stmt = conn.prepare_cached(iter_sql).unwrap(); |
There was a problem hiding this comment.
unguarded and uncommented unwrap
|
|
||
| Result::<(), ()>::Ok(()) | ||
| }) | ||
| .unwrap() |
There was a problem hiding this comment.
unguarded and uncommented unwrap
| if let Err(e) = | ||
| self.record_per_lifetime(tx, data.inner.lifetime, ping_name, &name, value) | ||
| { | ||
| log::error!( |
There was a problem hiding this comment.
(musing) I was gonna say "TODO: instrument this error" but I guess we have limited capacity for instrumenting the db gone awry. ...I suppose an in-memory error structure could be included in a "health" ping or something... I dunno.
| lifetime TEXT NOT NULL, | ||
| labels TEXT NOT NULL, -- can't be null or ON CONFLICT won't work | ||
| value BLOB, | ||
| UNIQUE(id, ping, labels) |
There was a problem hiding this comment.
This reflects a change in semantics, doesn't it? Previously, if a metric changed lifetime we'd store both.
Also: why are labels part of the unique constraint?
| lifetime = ?1 | ||
| AND ping = ?2 | ||
| AND id = ?3 | ||
| LIMIT 1 |
There was a problem hiding this comment.
I'd be more tempted to allow all rows in the result and instrument an error if we got more than 1
| /// | ||
| /// This function will **not** panic on database errors. | ||
| pub fn clear_ping_lifetime_storage(&self, storage_name: &str) -> Result<()> { | ||
| let clear_sql = "DELETE FROM telemetry WHERE lifetime = ?1 AND ping = ?2"; |
There was a problem hiding this comment.
Why not include Lifetime::Ping.as_str() in the statement directly?
2 participants
My plan over the next few weeks is to split up #3405 into reviewable pieces and merge them piece by piece.
For that I created the
main-sqlitebranch, which currently is the same asmain. This way any feature work/bug fixes against the current Glean can continue to go tomainwithout issues.Over time I can rebase/merge that branch against
mainto keep-up-to-date, but keep further sqlite work reviewable.this very first part is merely adding the first pieces of code, but none of that is actually part of the compilation yet (wouldn't work because it's not fully usable yet).
The details about the sqlite setup will change in later commits. Re-arranging that into a single commit that has the perfect setup would be a lot of work of splitting and rebasing commits (and squashing and merge conflicts), so doing it this way seemed easier.