feat: Syncstorage inital sqlite support #1520
Conversation
|
To pass the checks I have the choice between upgrading diesel or ignoring the issue with libsqlite-sys (which tbh is probably a non-issue because we should not be using SQLite printf anyway). |
Eragonfr
force-pushed
the
syncstorage-sqlite
branch
2 times, most recently
from
1047197
to
9d83956
Compare
Eragonfr
force-pushed
the
syncstorage-sqlite
branch
from
6927869
to
62774e3
Compare
First throw at adding sqlite as storage backend for the syncserver and the tokenserver. There is probably some duplicated code between: - syncstorage-mysql and syncstorage-sqlite - tokenserver-db-mysql and tokenserver-db-sqlite tokenserver-db-sqlite probably contains Mysql-specific SQL that might break Sqlite when ran. Squashed commit of the following: commit 1047197 Author: Eragon <eragon@eragon.re> Date: Wed Mar 6 00:24:07 2024 +0100 fix: Fix default for tokenserver-db commit 5e2a745 Author: Eragon <eragon@eragon.re> Date: Sun Feb 11 22:00:46 2024 +0100 tokenserver-db defaults to use mysql as db backend commit a587787 Author: Eragon <eragon@eragon.re> Date: Sun Feb 11 21:47:55 2024 +0100 Run cargo fmt commit c8c1458 Author: Eragon <eragon@eragon.re> Date: Sun Feb 11 20:32:19 2024 +0100 Better logging of migrations commit e2b8563 Author: Eragon <eragon@eragon.re> Date: Tue Jan 30 00:36:11 2024 +0100 wip: At least it runs now commit bd24d7c Author: Eragon <eragon@eragon.re> Date: Mon Jan 22 11:20:38 2024 +0100 lll commit 07ba38f Author: Eragon <eragon@eragon.re> Date: Fri Jan 19 14:35:20 2024 +0100 wip: First throw at adding sqlite as a storage backend
Eragonfr
force-pushed
the
syncstorage-sqlite
branch
from
62774e3
to
5ea3b39
Compare
|
I choose to ignore the libsqlite3-sys CVE because it's not impacting this project. That should let me run the other tests in the CI. |
| clippy_sqlite: | ||
| # Matches what's run in circleci | ||
| cargo clippy --workspace --all-targets --no-default-features --features=syncstorage-db/sqlite,tokenserver-db/sqlite -- -D warnings | ||
|
|
There was a problem hiding this comment.
I don't know if you saw #1513, but one of the things we're trying to do is to pull more of tokenserver into syncstorage-rs.
(Weird history lesson: Tokenserver was supposed to support more than just sync, but that never really happened. Having it distinct turned out to be a headache for a lot of reasons, so we're folding as much of Tokenserver as we can into the main syncstorage)
As a side note, this also means that the tokenserver storage stuff can probably live along-side the syncstorage data fairly easily. Possibly as just a bunch of other tables.
It's a bit more complicated for us since we have yet to migrate tokenserver over to spanner, but that's on our internal roadmap. When we get to doing that, however, is unknown.
We also have a few crate refactoring tickets open. I'll update those to indicate that we should really put the data handlers for both syncstorage and tokenserver into their own distinct crate.
There was a problem hiding this comment.
I did not saw that PR.
You want to remove tokenserver completely. Is that right ?
I don't understand what it means for my PR.
Should I change how I handle tokenserver ?
There was a problem hiding this comment.
I don't believe that it impacts much of your PR, since you're only focused on the Storage part. There may be a few opportunities for folk running their own "Stand Alone" version to have just one database running instead of two, and depending on how we decide to do this for Spanner, there may be some additional functions we introduce to the database layer. We'll try to isolate and highlight those so that we don't cause more work.
| @@ -1,6 +1,7 @@ | |||
| use diesel::{ | |||
| mysql::MysqlConnection, | |||
| r2d2::{CustomizeConnection, Error as PoolError}, | |||
| sqlite::SqliteConnection, | |||
There was a problem hiding this comment.
Wondering if we might want to feature flag some of this? I know spanner is far from light-weight, and I suspect that MySQL isn't a lot better. Feels silly to compile everything in.
I'm fine with you want to file a ticket for that and keep everything together for this PR.
There was a problem hiding this comment.
I think that I can easily add a feature flag, at least for SQLite and MySQL.
I don't want to touch the code related to spanner because I can't test it.
There was a problem hiding this comment.
You may be able to feature flag a few things (like this use:
#[cfg(feature = "sqlite")]
use diesel::sqlite::SqliteConnection;(and then flag the parts that include SqliteConnection)
For now, don't worry about it. As you get closer to the final version, we can work together to try and isolate things so that folk only include what they need. (This would include all the Spanner stuff, too.)
Description
Re-implementation of #935 on the newer codebase.
Thanks for @mqus for the first implementation.
Issue(s)
Partially fixes #498
Todo
Current state is that it runs on my computer, but it's probably not clean at all, there's still a lot of duplicate code, and some SQL queries are still in MySQL-flavored SQL but could be used by the SQLite backend.