Reuse a cached DB connection instead of always recreating for `sqlx-macros` #1782

CosmicHorrorDev · 2022-04-04T22:05:04Z

Similar spirit to #1684, but this focus in on speeding up cargo sqlx prepare with a remote database. This change reuses the same DB connection instead of recreating one for each sqlx-macros query macro.

Comparison

Three setups tested here. Two MariaDB setups on my work computer, one local and one remote, and the SQLite setup from #1684 on my personal laptop

MariaDB query #: 332
MariaDB sqlx-data.json size: 705 KiB

SQLite query #: 200
SQLite sqlx-data.json size: 305 KiB

The runtimes for cargo sqlx prepare on the various setups

Setup	`master` (`f858138`)	This PR	Reduction `(new - old) / old`
MariaDB remote	61.421 s ± 1.985 s	12.694 s ± 0.153 s	-79%
MariaDb local	5.291 s ± 0.086 s	5.050 s ± 0.064 s	-5%
SQLite local	1.562 s ± 0.006 s	1.425 s ± 0.004 s	-9%

It looks like the change lowered cargo sqlx prepare times across the board with the most significant impact by far on remote DBs

…`sqlx-macros`

sqlx-macros/src/query/mod.rs

CosmicHorrorDev · 2022-04-05T18:23:15Z

Also something that should be covered. A side-effect of this change is that the single db connection is never actually dropped (since lazy statics aren't dropped when the program finishes). This means that, for instance, if you are using SQLite there will be a lingering set of .db-wal and .db-shm files

I'm not sure the best way to handle this. The lazy staatic could be consumed if .into_value() was called on it, but I don't know how to determine when things are finished. It's also possible that this is a non-issue in most cases and that we could avoid using the cache for the problematic cases

abonander · 2022-04-05T19:24:40Z

It shouldn't create any temporary files if you disable journaling by doing .journal_mode(SqliteJournalMode::Off) to the SqliteConnectOptions.

We should also probably set the connection to read-only mode but I'm wondering if that will cause sqlite3_prepare_v2() to return an error if we try to prepare a non-SELECT query.

CosmicHorrorDev · 2022-04-06T06:46:17Z

It shouldn't create any temporary files if you disable journaling by doing .journal_mode(SqliteJournalMode::Off) to the SqliteConnectOptions.

Thanks for the insight! Disabling journaling for SQLite did fix the lingering write-ahead-log and shared-memory files. I also tried setting .read_only(true) which didn't seem to cause issues when I did a cargo sqlx prepare locally using the changes, but this caused an action job to fail with numerous error: error returned from database: disk I/O error errors

I think my final lingering question is with handling features when using the sqlx-core/any feature in sqlx-macros. Trying to compile sqlx-macros with the default features leads to the following error with this PR

$ cargo check -p sqlx-macros
error[E0432]: unresolved import `crate::any`
  --> sqlx-core/src/pool/mod.rs:59:12
   |
59 | use crate::any::{Any, AnyKind};
   |            ^^^
   |            |
   |            unresolved import
   |            help: a similar path exists: `core::any`

This is because sqlx_core::any is only available when the any feature is set and at least one of the database features is set, yet the above import is only gated behind a #[cfg(feature = "any")]. I can't seem to reproduce the issue when trying to compile sqlx-core directly with what I thought would be problematic features though. I would imagine that it's reasonable to compile_error!() when any is used without any database features enabled, but I may be missing some subtleties with all the feature flags

abonander · 2022-04-07T18:57:39Z

Since I didn't write the any module, I'm not sure what would happen if we were to just remove that "at least one database feature must be enabled" condition. Best case scenario, a lot of dead code warnings that I don't really feel like dealing with. Worth trying, though?

I think instead we can make the sqlx_macros::query and that problematic spot in sqlx_core::pool have the same condition, and then expand_query() can use an internal #[cfg]'d block to decide whether to invoke the query module or just output a compile_error!() directly.

CosmicHorrorDev · 2022-04-09T20:41:01Z

I'm not sure what would happen if we were to just remove that "at least one database feature must be enabled" condition

Trying that out ends up running into more errors

error[E0432]: unresolved import `decode::AnyDecode`
  --> sqlx-core/src/any/mod.rs:38:9
   |
38 | pub use decode::AnyDecode;
   |         ^^^^^^^^---------
   |         |       |
   |         |       help: a similar name exists in the module: `Decode`
   |         no `AnyDecode` in `any::decode`

error[E0432]: unresolved import `encode::AnyEncode`
  --> sqlx-core/src/any/mod.rs:39:9
   |
39 | pub use encode::AnyEncode;
   |         ^^^^^^^^---------
   |         |       |
   |         |       help: a similar name exists in the module: `Encode`
   |         no `AnyEncode` in `any::encode`

error[E0432]: unresolved import `crate::any::AnyColumnIndex`
 --> sqlx-core/src/any/row.rs:1:34
  |
1 | use crate::any::{Any, AnyColumn, AnyColumnIndex};
  |                                  ^^^^^^^^^^^^^^ no `AnyColumnIndex` in `any`

error[E0432]: unresolved import `crate::any::AnyColumnIndex`
 --> sqlx-core/src/any/statement.rs:1:48
  |
1 | use crate::any::{Any, AnyArguments, AnyColumn, AnyColumnIndex, AnyTypeInfo};
  |                                                ^^^^^^^^^^^^^^ no `AnyColumnIndex` in `any`

error[E0432]: unresolved import `column::AnyColumnIndex`
  --> sqlx-core/src/any/mod.rs:32:29
   |
32 | pub use column::{AnyColumn, AnyColumnIndex};
   |                             ^^^^^^^^^^^^^^
   |                             |
   |                             no `AnyColumnIndex` in `any::column`
   |                             help: a similar name exists in the module: `ColumnIndex`

I opted for moving all the stuff within sqlx_core that used sqlx_core::any to match the config that provides it. Within sqlx_macros I ended up switching expand_input to return an error when trying to expand_from_db when there are no database features like how things are special-cased for offline. The reason I went with this was because I wanted to allow for using sqlx_macros in offline mode without needing any db features in case anyone has weird CI setups where they choose to check with just offline mode and no db features. This keeps things so that sqlx can be compiled with just a runtime selected instead of forcing a db feature too

CosmicHorrorDev · 2022-04-10T05:03:23Z

Worth noting that I forgot my personal laptop was using rust 1.59 when I was running the local SQLite benchmarks in my initial description which had incremental compilation disabled. Rerunning them on 1.60 gets a reduction in cargo sqlx prepare time of roughly 9% (new timings are edited in) instead of the originally posted 1%

The other timings were using nightly, so the same discrepancy wasn't present

abonander · 2022-04-13T23:39:51Z

I think because this basically makes the any feature mandatory now, I'll include this as part of 0.6.

CosmicHorrorDev · 2022-05-02T22:54:58Z

Noticed there was a simple merge conflict, so just pushed to resolve it

CosmicHorrorDev added 2 commits April 4, 2022 15:48

refactor: Reuse a cached connection instead of always recreating for …

2f5fbf7

…`sqlx-macros`

fix: Fix type inference issue when no database features used

1d65780

abonander reviewed Apr 5, 2022

View reviewed changes

sqlx-macros/src/query/mod.rs Outdated Show resolved Hide resolved

refactor: Switch cached db conn to an AnyConnection

8905a25

fix: Fix invalid variant name only exposed with features

b594251

CosmicHorrorDev added 2 commits April 6, 2022 00:40

fix: Tweak connection options for SQLite with sqlx-macros

b7a4df9

fix: Remove read only option for SQLite connection

5ee1a11

fix: Fix feature flags regarding usage of sqlx_core::any

cc25099

abonander added this to the 0.6.0 milestone Apr 13, 2022

abonander mentioned this pull request Apr 26, 2022

sqlx-macros: Explicitly close QueryData db connections #1834

Closed

chore: Fix merge conflicts from Cargo.toml changes

a849fad

abonander merged commit a2691b9 into launchbadge:master May 26, 2022

CosmicHorrorDev mentioned this pull request Jun 26, 2022

0.6.0: query macro fails on sqlite with "error returned from database: database is locked" #1929

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse a cached DB connection instead of always recreating for `sqlx-macros` #1782

Reuse a cached DB connection instead of always recreating for `sqlx-macros` #1782

CosmicHorrorDev commented Apr 4, 2022 •

edited

Loading

CosmicHorrorDev commented Apr 5, 2022

abonander commented Apr 5, 2022 •

edited

Loading

CosmicHorrorDev commented Apr 6, 2022 •

edited

Loading

abonander commented Apr 7, 2022 •

edited

Loading

CosmicHorrorDev commented Apr 9, 2022

CosmicHorrorDev commented Apr 10, 2022 •

edited

Loading

abonander commented Apr 13, 2022

CosmicHorrorDev commented May 2, 2022

Reuse a cached DB connection instead of always recreating for sqlx-macros #1782

Reuse a cached DB connection instead of always recreating for sqlx-macros #1782

Conversation

CosmicHorrorDev commented Apr 4, 2022 • edited Loading

Comparison

CosmicHorrorDev commented Apr 5, 2022

abonander commented Apr 5, 2022 • edited Loading

CosmicHorrorDev commented Apr 6, 2022 • edited Loading

abonander commented Apr 7, 2022 • edited Loading

CosmicHorrorDev commented Apr 9, 2022

CosmicHorrorDev commented Apr 10, 2022 • edited Loading

abonander commented Apr 13, 2022

CosmicHorrorDev commented May 2, 2022

Reuse a cached DB connection instead of always recreating for `sqlx-macros` #1782

Reuse a cached DB connection instead of always recreating for `sqlx-macros` #1782

CosmicHorrorDev commented Apr 4, 2022 •

edited

Loading

abonander commented Apr 5, 2022 •

edited

Loading

CosmicHorrorDev commented Apr 6, 2022 •

edited

Loading

abonander commented Apr 7, 2022 •

edited

Loading

CosmicHorrorDev commented Apr 10, 2022 •

edited

Loading