feat: real multi-backend connection pooling (fix #140) - #141
Merged
Conversation
…pool (Ludea#140) Fixes Ludea#140. Ludea reported "Cannot to get Pool" on every "/" refresh and login attempt despite the DB being "already installed". Root cause: `POOL` (src/diesel.rs) is a global `OnceLock<Mutex<Pool< AsyncMysqlConnection>>>` — hardcoded to MySQL. `create_db`'s Sqlite and Postgresql branches call `diesel::create_database(...)` (which does create a file / attempt a connection) but never call `diesel::set_pool(...)` nor persist `database.type` to config, unlike the Mysql branch. The frontend install flow reports success either way (create_db returns Ok), so a user picking Sqlite or PostgreSQL ends up with a server that believes it's installed but can never actually reach the database — every request that needs `get_pool()` returns `None` and surfaces as "Cannot get Pool", forever, not just after a restart. The Postgresql branch had a second bug on top of that: it called `create_database("postgres://")` with the literal string instead of a URL built from `inner.db_connection`, so database creation itself was never going to succeed for Postgres either. Properly supporting non-MySQL pooling would mean generalizing `POOL` (and the ~15 query functions in diesel.rs that assume `Pool<AsyncMysqlConnection>`) to handle multiple backend connection types — a real architecture decision, not something to guess at in a bug-fix PR. For now, make Sqlite/Postgresql fail loudly and immediately at install time instead of pretending to succeed, mirroring the existing Surrealdb branch. This turns a confusing "works until you try to use it" bug into an honest "not supported yet" error at the point of choice. Happy to take a stab at real multi-backend pooling if that's wanted — posted the question on the issue. Verified: `cargo check`, `cargo fmt --check`, `cargo clippy --all-targets -- -D warnings` all clean.
Closed
Owner
|
My goal is to support these three databases. |
Supersedes the "fail loudly" stopgap from the previous commit, per your
comment on this PR: "My goal is to support these three databases...
keep all, and fix pool issue."
## What changed
`POOL` was a single `Pool<AsyncMysqlConnection>` — hardcoded to MySQL.
Since diesel_async's `MultiConnection` trait (which would let one pool
type cover all three backends) isn't released yet, this adds an interim
`DbPool` enum (Mysql/Pg/Sqlite variants) + a `with_conn!` macro that runs
the *same* Diesel query code against whichever concrete connection type
the pool holds. The query logic itself never changes per backend, so this
isn't three separately hand-verified code paths — it's one path,
monomorphized three ways by the macro.
Also discovered along the way: the migrations themselves are MySQL-only
SQL (`AUTO_INCREMENT`, native `ENUM(...)`) — even with a working pool,
SQLite/PostgreSQL would have failed at schema creation. Added
`migrations_sqlite/` and `migrations_postgres/` with portable equivalents
(`AUTOINCREMENT`/`SERIAL`, `TEXT` + `CHECK` instead of `ENUM` — see
`models.rs` for the matching per-backend `ToSql`/`FromSql` impls for
`Permission`). Both new sets include `'pending'` as a valid permission
value, which the original MySQL `ENUM("read", "write")` is missing even
though `Permission::Pending` is a real Rust variant used by
`join_update_server` — a latent bug on MySQL too, left alone since
altering that enum is a migration change with its own risk for existing
MySQL deployments and is outside this PR's scope.
`rpc.rs`'s `create_db` now actually wires Sqlite the same way Mysql
already did (persist `database.type` to config, `set_pool`, run
migrations) instead of erroring. PostgreSQL likewise, and its
`create_database("postgres://")` literal-string bug (noted in the
previous commit) is fixed to build a real URL from `db_connection`.
`main.rs`'s startup path now restores all three backends from config on
restart, not just MySQL.
## Verification
- `cargo check` / `cargo fmt --check` / `cargo clippy --all-targets -- -D
warnings`: clean.
- **SQLite migrations**: applied directly against a real local sqlite3
database via the `sqlite3` CLI (insert/join queries, and confirmed the
CHECK constraint rejects invalid `permission` values).
- **SQLite, full Rust path**: added `diesel::temp_sqlite_verification::
sqlite_end_to_end` (a real `#[tokio::test]`, kept as this repo's first
test) exercising `create_database` → `create_user` →
`register_update_server` (Permission::Write) → `list_update_server_by_
user` → `is_table_created` → `get_plugin_version` →
`join_update_server` (Permission::Pending) → `delete_repo` against an
actual temp SQLite file through the new `DbPool`/`with_conn!`
machinery end to end. Passes.
- **PostgreSQL: NOT runtime-verified.** No local Postgres server was
available in this environment (the local install here is a stopped
Windows service I don't have permission to start, and CI has no
Postgres service either). The code compiles and mirrors the
MySQL/SQLite paths exactly, but please test it against a real Postgres
instance before relying on it — happy to help set up a
docker-compose-based Postgres CI job if useful for catching this kind
of gap going forward.
- **MySQL path unchanged** apart from moving through the same
`DbPool`/`with_conn!` machinery — same connection string construction,
same migrations, same queries as before.
gloubix
changed the title
Contributor
Author
|
Pushed the real fix — see updated PR description. Kept SQLite to the highest bar I could verify locally (real migration run + a passing end-to-end test); PostgreSQL compiles and mirrors it exactly but I couldn't spin up a server here to actually test it, so please double-check that path before trusting it in production. |
Ludea
approved these changes
Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #140. Updated per your comment below — implements real multi-backend support instead of just refusing SQLite/PostgreSQL at install.
Original bug
`POOL` (`src/diesel.rs`) was a global `OnceLock<Mutex<Pool>>` — hardcoded to MySQL. `create_db`'s Sqlite/Postgresql branches created the database but never called `set_pool`/persisted config, so a user picking either left a server that believed it was installed but could never reach the database — "Cannot get Pool" on every request, forever.
What this now does
Since diesel_async's
MultiConnectiontrait isn't released yet, added an interimDbPoolenum (Mysql/Pg/Sqlite) + awith_conn!macro that runs the same Diesel query code against whichever concrete connection type the pool holds — one query implementation, monomorphized three ways, not three hand-written paths.Also found along the way: the existing migrations are MySQL-only SQL (
AUTO_INCREMENT, nativeENUM(...)) — even with a working pool, SQLite/PostgreSQL would've failed at schema creation. Addedmigrations_sqlite/andmigrations_postgres/with portable equivalents. Both include'pending'as a valid permission (the original MySQLENUM("read","write")is missing it, even thoughPermission::Pendingis a real, used Rust variant — a latent bug on MySQL too, left untouched since changing that enum has its own risk for existing deployments).create_dbnow wires Sqlite the same way Mysql already did; PostgreSQL likewise (and itscreate_database("postgres://")literal-string bug is fixed to build a real URL).main.rs's startup path restores all three from config on restart.Verification
cargo check/cargo fmt --check/cargo clippy --all-targets -- -D warnings: clean.#[tokio::test](sqlite_end_to_end, this repo's first test) exercising the full Rust path — create_database → create_user → register_update_server → list_update_server_by_user → is_table_created → get_plugin_version → join_update_server → delete_repo — against a real temp SQLite file. Passes.DbPoolmachinery.