Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pool<Sqlite> is broken in many ways #3080

Open
Jeremiah-Griffin opened this issue Feb 27, 2024 · 4 comments
Open

Pool<Sqlite> is broken in many ways #3080

Jeremiah-Griffin opened this issue Feb 27, 2024 · 4 comments
Labels

Comments

@Jeremiah-Griffin
Copy link

Jeremiah-Griffin commented Feb 27, 2024

I apologize for the vague title, but there are lots of overlapping issues here which necessitated it.
This seems to be partly comorbid with #2099, but there are new issues highlighted here so I made a new issue.

In essence, the majority of PoolOptions that a user will pass to a Pool<Sqlite> are nonfunctional and will cause runtime errors. connect_lazy will also cause the same errors.

Bug Description

In summary of the linked repository:

  • When max_connections != min_connections , calls to query!() will intermittently fail with the error "table not found".
  • When max_connections == min_connections, calls succeed as expected
  • When max_connections == 1, calls will succeed whether min_connections == 0 or 1.
  • Introducing parallel calls increasing when max_connections != min_connections does not alter the rate of successful queries unless max_connections == min_connections, in which case all calls are successful
  • This is consistent across single reads and writes, as well as read and write transactions
  • The default PoolOptions sets max_connections to 10, and min_connections to 0, and so is unusabe.
  • connect_lazy is always completely broken, no matter the combination of connection numbers; every call to a pool started with .connect_lazy() will fail with table not found error.

Minimal Reproduction

https://github.com/Jeremiah-Griffin/slqite_pool_error_reproduction/tree/e47075e7db043891e87a9a27fcc3fc65bbc1c7d7/sqlx_bug_reproduce

Info

  • SQLx version: 0.7.4
  • SQLx features enabled: sqlite, runtime-tokio
  • Database server and version: SQLite 3.38 bundled with libsqlite3-sys v0.27.0
  • Operating system: Windows 10
  • rustc --version: stable 1.76 & 1.78 nightly

Mitigations

  • Never use the default PoolOptions.
  • Never start a Pool<Sqlite> with connect_lazy. If a pool is needed in a sync context, spawn an OS thread, start an executor on it, and spawn the pool asynchronously. The thread may be killed thereafter.
  • If max_connections > 1, ensure min_connections and max_connections are always equal

Addenda

  • Replacing the unstable LazyLock with lazy_static yields no change in behavior, nor does switching to the stable compiler.
  • I don't know if the reason max_connections == min_connections works because the pool is repeatedly leasing out the same connection while all others are bugged, or if all the connections spawned in this case happen to be correct
@Jeremiah-Griffin Jeremiah-Griffin changed the title Pool<Sqlite> broken in many ways Pool<Sqlite> is broken in many ways Feb 27, 2024
@Jeremiah-Griffin
Copy link
Author

Jeremiah-Griffin commented Feb 28, 2024

SqlitePoolOptions::connect, connect_lazy, and connect_lazy_with also seem to be exhibiting strange behavior in that regardless of the combination of connection numbers some connections always drop requests. Ill take another look at the code and add their behavior to the example repo.

Seeing as this is the case, it also means there's no documented way to guarantee that that all the connections start with the supplied SqliteConnectionOptions which could have unexpected performance and data integrity implications for sqlite databases.

@dragonnn
Copy link
Contributor

You are initializing the pools on different runtime then using them.
Not sure if that is the idea here bug I suspect that isn't the best idea.
Try initializing them in tokio::main. You can use OnceLock from std to put them in global objects if your use case needs that (and avoid having to spawn a different thread and runtime for initializing in place them).

@Jeremiah-Griffin
Copy link
Author

You are initializing the pools on different runtime then using them. Not sure if that is the idea here bug I suspect that isn't the best idea. Try initializing them in tokio::main. You can use OnceLock from std to put them in global objects if your use case needs that (and avoid having to spawn a different thread and runtime for initializing in place them).

That's only the case for the statics. Commenting out the statics, their usages,a and the lazylock feature entirely does not change the behavior of the broken lazy and maxgtmin cases which remain broken despite being spawned on the main thread. Likewise, Initiating the working case in a static context with the same method currently used does not break it.

osmano807 added a commit to osmano807/tower-sessions-stores that referenced this issue Mar 10, 2024
sqlx SQLite is somewhat buggy, as seen by:
launchbadge/sqlx#2099

Wrapping every query with a transaction is supposed to mitigate.

Also, see launchbadge/sqlx#3080, which need to be made on the application using tower-sessions
@CommanderStorm
Copy link
Contributor

CommanderStorm commented Aug 21, 2024

@Jeremiah-Griffin

I had a look at your repro.
You are kind of sabotaging yourself via including the following AFTER you connected to the pool.

pool.set_connect_options(connection_options);

There should be either a hard error, warning or similar for this.
Mixing the :memory: connection_options and a non-memory Pool is something that I can't imagine someone might want.

Could you update the title of this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants