sql: switch from sqlx to rusqlite #2380

link2xt · 2021-04-24T22:44:26Z

link2xt · 2021-04-25T04:02:39Z

src/contact.rs

+                        ],
+                    )
+                    .await
+                    .ok();


Why we ignore the error here?

link2xt · 2021-04-25T05:40:31Z

src/html.rs

@@ -426,7 +426,7 @@ test some special html-characters as &lt; &gt; and &amp; but also &quot; and &#x
    #[async_std::test]
    async fn test_get_html_empty() {
        let t = TestContext::new().await;
-        let msg_id = MsgId::new_unset();
+        let msg_id = MsgId::new(100);


This test is actually wrong, it tries to load HTML for nonexistent message, which is an error. Message ID 100 at least does not throw an error due to using special ID (rusqlite converters have protection against storing special IDs into database), but the test could be better anyway.

link2xt · 2021-04-25T07:43:40Z

"search hello" benchmark is indeed much more stable than sqlx:

Account creation is slower:

"Create 100 contacts and read it 1000 times" benchmark is slower and surprisingly became slower after several runs:

"create 500 contacts" benchmark is like 20 times slower according to initial estimates (~1200 seconds vs ~60 seconds), I didn't wait until it finishes.

Enabling shared cache makes things worse, at least for "create 1 account" benchmark (+60% compared to current state of rusqlite).

So rusqlite is a bit better at reading (loading search results, chatlist and message list) but has its own huge problems in other benchmarks, especially writing to the database.

r10s · 2021-04-25T11:00:08Z

@link2xt great you could create this pr 🙏

also, thanks for this scientific view on the two libraries, that bring the different weaknesses of both libraries to light.

all in all, for UIs, worse read speed seems to be a bigger flaw. that, and the other issues of #2378 in mind it still advisable to switch back to rusqlite, at least for now. at some point, sqlx may be a better choice, but that seems not to be today.

for reviewing this pr: most of the lines are pulled on from before #2089, right? so they cannot be that wrong? :)

link2xt · 2021-04-25T17:55:22Z

all in all, for UIs, worse read speed seems to be a bigger flaw. that, and the other issues of #2378 in mind it still advisable to switch back to rusqlite, at least for now. at some point, sqlx may be a better choice, but that seems not to be today.

I count regressions in performance as flaws. If previous release does some operation in 500ms and now it does the same in 600ms, that's a regression. The way to avoid it is to cover all common operations with benchmarks and only switch when some benchmarks improve and other don't degrade significantly. For now sqlx does not satisfy these conditions. But if we were using sqlx today, we should not change to rusqlite for the same reason.

for reviewing this pr: most of the lines are pulled on from before #2089, right? so they cannot be that wrong? :)

Sql.exists and Sql.count expect SELECT COUNT statements now, there is new Sql.transaction etc. I reimplemented some Sql interfaces with rusqlite to minimize the changes in the rest of the code.

I reviewed the PR line-by-line once. The most important part to review is dc_receive_imf. Maybe take another approach, go through recent commits since 8f1bb38 and make sure they are not reverted. All the code I moved with meld is from 8f1bb38.

r10s

i walked through all recent commits as suggested, checking that no line is missing, that lgtm (did a tiny pr, #2381, not sure how useful that is)

also, i am running the pr already on my iphone.

i will have a look at receive_imf etc. tomorrow, but i'd say, if that pr blocks other things, we can already merge that in.

so that we do not forget about that test and the issues we had with that :)

dotlambda · 2021-04-30T11:29:17Z

Cargo.toml

 rand = "0.7.0"
 regex = "1.1.6"
+rusqlite = { version = "0.24", features = ["bundled"] }


Is there a way to turn the bundled feature off for a distribution package? I guess we'll have to patch Cargo.toml?

deltachat-core-rust is a Rust package too and has its own features. We can change the [features] section in Cargo.toml to:

default = ["bundled-sqlite"] internals = [] repl = ["internals", "rustyline", "log", "pretty_env_logger", "ansi_term", "dirs"] vendored = ["async-native-tls/vendored", "async-smtp/native-tls-vendored"] nightly = ["pgp/nightly"] bundled-sqlite = ["rusqlite/bundled"]

And then you can build with --no-default-features if you don't want bundled SQLite. I don't mind hardcoding --no-default-features to CMakeLists.txt in this case.

That would be nice! Hardcoding --no-default-features in CMakeLists.txt is up to you, I can also change that in Nixpkgs.

I am going to do this with rusqlite update then.

Since you are packaging tagged release anyway, for now you can just patch Cargo.toml.

link2xt force-pushed the revert-sqlx-to-rusqlite branch 4 times, most recently from 09c52e0 to 970d249 Compare April 24, 2021 23:56

sql: switch from sqlx to rusqlite

3b8926f

link2xt force-pushed the revert-sqlx-to-rusqlite branch 22 times, most recently from 4507917 to 6cb9f9f Compare April 25, 2021 04:01

link2xt commented Apr 25, 2021

View reviewed changes

src/contact.rs

],

)

.await

.ok();

Copy link

Collaborator Author

link2xt Apr 25, 2021 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we ignore the error here?

link2xt force-pushed the revert-sqlx-to-rusqlite branch 2 times, most recently from 219de80 to 3c4171c Compare April 25, 2021 04:07

link2xt force-pushed the revert-sqlx-to-rusqlite branch 7 times, most recently from ca44b7b to a3a06ab Compare April 25, 2021 05:24

link2xt commented Apr 25, 2021

View reviewed changes

link2xt force-pushed the revert-sqlx-to-rusqlite branch from a3a06ab to 51c04f6 Compare April 25, 2021 05:40

link2xt force-pushed the revert-sqlx-to-rusqlite branch from 51c04f6 to 3b8926f Compare April 25, 2021 08:04

link2xt requested review from dignifiedquire, r10s and Hocuri and removed request for dignifiedquire April 25, 2021 08:04

r10s reviewed Apr 25, 2021

View reviewed changes

re-add test_db_reopen() for rusqlite

6c3de5f

so that we do not forget about that test and the issues we had with that :)

link2xt merged commit 143c5e6 into master Apr 25, 2021

link2xt deleted the revert-sqlx-to-rusqlite branch April 25, 2021 19:33

link2xt mentioned this pull request Apr 25, 2021

sql: set PRAGMA synchronous=normal #2382

Merged

dotlambda reviewed Apr 30, 2021

View reviewed changes

link2xt mentioned this pull request Jul 29, 2021

sqlite: fix a couple segfaults launchbadge/sqlx#1351

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

sql: switch from sqlx to rusqlite #2380

sql: switch from sqlx to rusqlite #2380

Uh oh!

link2xt commented Apr 24, 2021

Uh oh!

link2xt Apr 25, 2021 •

edited

Loading

Uh oh!

link2xt Apr 25, 2021

Uh oh!

link2xt commented Apr 25, 2021

Uh oh!

r10s commented Apr 25, 2021

Uh oh!

link2xt commented Apr 25, 2021

Uh oh!

r10s left a comment •

edited

Loading

Uh oh!

dotlambda Apr 30, 2021

Uh oh!

link2xt Apr 30, 2021

Uh oh!

dotlambda Apr 30, 2021

Uh oh!

link2xt Apr 30, 2021

Uh oh!

Uh oh!

Uh oh!

sql: switch from sqlx to rusqlite #2380

sql: switch from sqlx to rusqlite #2380

Uh oh!

Conversation

link2xt commented Apr 24, 2021

Uh oh!

link2xt Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

link2xt Apr 25, 2021

Choose a reason for hiding this comment

Uh oh!

link2xt commented Apr 25, 2021

Uh oh!

r10s commented Apr 25, 2021

Uh oh!

link2xt commented Apr 25, 2021

Uh oh!

r10s left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dotlambda Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

link2xt Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

dotlambda Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

link2xt Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

link2xt Apr 25, 2021 •

edited

Loading

r10s left a comment •

edited

Loading