Use sqlx as database #29
Conversation
|
What's the benefit of using sqlx over rusqlite here?
IIRC @dignifiedquire spent many many hours trying to make sqlx work for core and it failed.
…On Tue, Jun 06, 2023 at 11:32 -0700, Sebastian Klähn wrote:
Finally get rid of surrealdb in favor of sqlx. The code got a lot more verbose, because some special types needed on the bot structs don't implement the neccessary Traits for encoding and decoding (Encode<_> and Decode<_>). Possible workarounds are:
1. Adding sqlx with a feature flag to deltachat core to implement these traits.
2. Add some wrapper types which are trasformed to/from in the database which are easy to put into the db.
Both of them are fine I guess but the added verbosity isn't to bad either.
I've changed the structure of the db which was a struct before to a bare module which takes a sqliteConnection as an argument. This way the bot might utilize transactions later to rollback complex db changes in case of failure.
You can view, comment on, or merge this pull request online at:
#29
-- Commit Summary --
* clean db
* add sqlx and fix some deps
* insert new db
* adjust frontend
* fix rebase
* automatically create testers, publishers, genesis
-- File Changes --
A .env (1)
M Cargo.lock (1118)
M Cargo.toml (12)
A frontend/src/bindings/AppInfo.ts (3)
M frontend/src/bindings/DownloadResponse.ts (2)
D frontend/src/bindings/FrontendAppInfo.ts (3)
M frontend/src/bindings/UpdateResponse.ts (4)
M frontend/src/components/AppInfo.tsx (10)
M frontend/src/entries/review.tsx (6)
M frontend/src/entries/shop.tsx (15)
M frontend/src/entries/submit.tsx (8)
M frontend/src/mock.ts (6)
M frontend/src/types.ts (6)
A migrations/20230604175742_init.sql (49)
M src/bot.rs (129)
M src/db.rs (712)
M src/main.rs (29)
M src/request_handlers/genisis.rs (8)
M src/request_handlers/mod.rs (49)
M src/request_handlers/review.rs (62)
M src/request_handlers/shop.rs (69)
M src/request_handlers/submit.rs (53)
M src/utils.rs (23)
-- Patch Links --
https://github.com/webxdc/appstore-bot/pull/29.patch
https://github.com/webxdc/appstore-bot/pull/29.diff
--
Reply to this email directly or view it on GitHub:
#29
You are receiving this because you are subscribed to this thread.
Message ID: ***@***.***>
|
|
sqlx has async interfaces and can switch from SQLite to MySQL and PostgreSQL if needed. The reason we switched back from sqlx to rusqlite was a performance problem due to sqlx using SQLite database from multiple threads, submitting query in one thread and stepping over the results in a different thread. This problem is reportedly fixed in sqlx 0.7 and we don't plan to put too much load on this database anyway. If the database ever becomes a bottleneck, we can switch to PostgreSQL easily. |
| @@ -0,0 +1,49 @@ | |||
| -- Add migration script here | |||
| CREATE TABLE IF NOT EXISTS config ( | |||
There was a problem hiding this comment.
How do we add a column to this table? If we need a new configuration value, this is not extensible. I already had troubles with import trying to insert serial when the rest of the config is not there: #28
Also having multiple rows in such table does not make sense. What is the id column, can we have multiple configs with different IDs?
There was a problem hiding this comment.
I've never written any raw sql statements just yet, so I don't get why you can't add a new column.
Regarding the second point, the bot will not configure as soon as you start it so the config should be complete.
The last point is also due to my lacking knowledge of sql. I thought it would be fine to have a table where only one row exists (the one with the current config).
Since you cleary have a better understandig of sql, I'm open for any suggestions!
There was a problem hiding this comment.
I would do similar to how the core does this: https://github.com/deltachat/deltachat-core-rust/blob/e6d9a49187786fe1da72e6163c0684e3beca46b0/src/sql/tables.sql#L1-L5
Afaik this is also what firefox does.
I've never written any raw sql statements just yet, so I don't get why you can't add a new column.
You can add a new column, but then you need a migration each time you do this, and the value will need to have some default (or not be NOT NULL).
There was a problem hiding this comment.
To build the whole config I would then have to do 5 requests? Sounds kinda iffy, if we keep the central approach of the config. If we change to a table with [key, value] entries I would suggest to also fracture the config loading.
Also, is the not-null contstraint really a problem if we always initialize the config at the top of main ?
There was a problem hiding this comment.
To build the whole config I would then have to do 5 requests?
You can do this with one INSERT, INSERT INTO config (key, value) VALUES ('invite_qr', 'foo'), ('genesis_qr', 'bar'). And even with multiple INSERTs it is not a problem, you can also group them in a transaction and it will be just as fast.
Also, is the not-null contstraint really a problem if we always initialize the config at the top of
main?
I am thinking about the case when we already have a bot set up, users have added applications there etc. Then we want to introduce a new configuration option. How would a migration look like? Add a new column with a NULL value, add a new column with some default value (which may not make sense e.g. if it is another QR code)? If you have a key-value table, then you don't even need a migration.
There was a problem hiding this comment.
Well if it is something like a qr-code which needs non-trivial setup, you have to write code for it anyway. That would then be exactly what you would also put into the migration code, no? Just create it with a sensible default and pick up on initialization in bot code.
I mean I see that it is in general more flexibel with a kv-store, but for the bot I don't see the immediate benefit. Maybe we can postpone this discussion to a later point / create an issues?
| env={"RUST_LOG": "github_bot=trace"}, | ||
| env={ | ||
| "RUST_LOG": "github_bot=trace", | ||
| "addr": config["addr"], |
There was a problem hiding this comment.
Why does import need an address now? Previously I could import .xdcs and then configure the bot.
There was a problem hiding this comment.
Maybe this change can just be reverted, will the test still pass?
There was a problem hiding this comment.
Nope, I made it so you always have to configure when executing the bot binary
|
|
| review_helper: MsgId::new(row.try_get("review_helper")?), | ||
| submit_helper: MsgId::new(row.try_get("submit_helper")?), | ||
| review_chat: ChatId::new(row.try_get("review_chat_id")?), | ||
| submit_chat: ChatId::new(row.try_get("submit_chat_id")?), | ||
| publisher: ContactId::new(row.try_get("publisher")?), |
There was a problem hiding this comment.
Is it not possible to implement some traits for MsgId,ContactIdand ChatId so that sqlx can do this automatically for you?
There was a problem hiding this comment.
Yes, you need Encode and Decode. I didn't want to add it to dc because I didn't think of putting it behind a feature gate. At some point we can make this optimization.
At this point this little extra verbosity is not too bad I guess.
There was a problem hiding this comment.
Might also be enough to only implement Deref for chatId etc.
|
I think we can still reshape even the initial database schema before it is deployed, so better merge it and then it can be improved in smaller PRs. |
Finally get rid of surrealdb in favor of sqlx.
I've changed the structure of the db which was a struct before to a bare module which takes a sqliteConnection as an argument. This way the bot might utilize transactions later to rollback complex db changes in case of failure.
closes #2