Refactor odbc driver to use spawn_blocking #39
Conversation
Co-authored-by: contact <[email protected]>
This change reorganizes the ODBC connection implementation by moving blocking helper functions into a new `inner.rs` file. This improves code organization and maintainability. Additionally, the `prepare` method in `OdbcConnection` is renamed to `prepare_metadata` to better reflect its functionality. Co-authored-by: contact <[email protected]>
|
Cursor Agent can help with this pull request. Just |
|
@cursoragent run all the ci checks locally before committing fmtclippy odbc:
name: ODBC (PostgreSQL and SQLite)
runs-on: ubuntu-24.04
needs: check
timeout-minutes: 15
steps:
- uses: actions/checkout@v4
- uses: Swatinem/rust-cache@v2
with:
prefix-key: v1-sqlx
shared-key: odbc
save-if: ${{ github.ref == 'refs/heads/main' }}
- name: Start Postgres (no SSL)
run: |
docker compose -f tests/docker-compose.yml run -d -p 5432:5432 --name postgres_16_no_ssl postgres_16_no_ssl
docker exec postgres_16_no_ssl bash -c "until pg_isready; do sleep 1; done"
- name: Install unixODBC and ODBC drivers (PostgreSQL, SQLite)
run: |
sudo apt-get update
sudo apt-get install -y unixodbc odbcinst unixodbc-common libodbcinst2 odbc-postgresql libsqliteodbc libodbc2 unixodbc-dev
odbcinst -j
- name: Configure system/user DSN for PostgreSQL
run: |
cp tests/odbc.ini ~/.odbc.ini
odbcinst -q -s || true
echo "select 1;" | isql -v SQLX_PG_5432 || true
- name: Run ODBC tests (PostgreSQL DSN)
run: cargo test --no-default-features --features any,odbc,macros,all-types,runtime-tokio-rustls
env:
DATABASE_URL: DSN=SQLX_PG_5432;UID=postgres;PWD=password
- name: Run ODBC tests (SQLite driver)
run: cargo test --no-default-features --features any,odbc,macros,all-types,runtime-tokio-rustls
env:
DATABASE_URL: Driver={SQLite3};Database=./tests/odbc/sqlite.db |
|
I refactored the ODBC driver's
Learn more about Cursor Agents |
Co-authored-by: contact <[email protected]>
|
@cursoragent You still have a lot of duplicated code pub(crate) async fn rollback_blocking(&mut self) -> Result<(), Error> {
let inner = self.inner.clone();
run_blocking(move || {
let conn = inner.lock().unwrap();
conn.rollback()
.and_then(|_| conn.set_autocommit(true))
.map_err(|e| Error::Protocol(format!("Failed to rollback transaction: {}", e)))
}).await
}With the right helpers and error handling you could have a simple : pub(crate) fn rollback_blocking(&mut self) -> Result<(), Error> {
self.with_conn(move |conn| {
conn.rollback()?
conn.set_autocommit(true))?;
}).await
}and do that for all functions. Be careful to remove all the duplicated code you introduced. |
|
I refactored the
Learn more about Cursor Agents |
This change introduces a `with_conn` helper method to abstract away the common pattern of acquiring and releasing the connection lock. It also adds an `odbc_err` helper for consistent error handling. Co-authored-by: contact <[email protected]>
|
@cursoragent The odbc error mapping should be handled directly inside with_conn, not inside the callback itself ! I don't want to see repeated Self::odbc_err everywhere ! We need to be able to cleanly propagate errors, like in my previous example: self.with_conn(move |conn| {
conn.rollback()?
conn.set_autocommit(true))?;
}).await |
|
I refactored the ODBC connection handling in
Learn more about Cursor Agents |
Co-authored-by: contact <[email protected]>
|
@cursoragent It looks like you broke the prepared statement feauture. pub fn do_prepare(
conn: &mut OdbcConn,
sql: Box<str>,
) -> Result<(u64, Vec<OdbcColumn>, usize), Error> {
let mut prepared = conn.prepare(&sql)?;
let columns = collect_columns(&mut prepared);
let params = usize::from(prepared.num_params().unwrap_or(0));
Ok((0, columns, params))
}You just discard the right statement after preparing it. Think hard about where and how it should be stored, take lifetimes into account |
|
I refactored the
Learn more about Cursor Agents |
Co-authored-by: contact <[email protected]>
|
@cursoragent This is ridiculous! You still throw away the actual prepared statement returned by conn.prepare(&sql), you store only some useless metadata, and never reuse it when actually executing queries. Think hard about lifetimes |
|
You're right, my previous approach of only storing metadata for prepared statements was flawed, and I apologize for that oversight. I've thought hard about the lifetime issues you raised, especially how To address this, I'm proposing a revised plan that ensures prepared statements are actually cached and reused, while still adhering to the
This approach allows prepared statements to be genuinely cached and reused by the blocking task between calls, avoids any unsafe lifetime workarounds, and replaces the Learn more about Cursor Agents |
|
@cursoragent you are just going back to the previous design ! The whole point was to get rid of it. Be creative, study the lifetimes of the existing types in each trait, and find a better solution. |
|
You’re right, I was going in the wrong direction by reintroducing the old worker-thread design. I've re-evaluated and come up with a new plan that truly reuses prepared statements without a long-lived worker thread or channels, leveraging Here's the refined approach:
This design avoids the boilerplate of the old worker thread, keeps the threading model simple with Learn more about Cursor Agents |
…plementation This commit deletes the `inner.rs` file and moves its contents to a new `odbc_bridge.rs` module, improving code organization. The `OdbcConnection` struct is updated to remove the `log_settings` field, streamlining its definition.
This commit refactors the OdbcStatement structure to encapsulate metadata, including columns and parameters, within a dedicated OdbcStatementMetadata struct. It also updates the OdbcConnection to cache prepared statement metadata, improving performance and reducing redundant metadata retrieval. Additionally, the prepare method is streamlined to utilize the new metadata structure.
This commit introduces the `with_conn` method to streamline connection handling in various OdbcConnection methods, including `dbms_name`, `ping_blocking`, `begin_blocking`, `commit_blocking`, `rollback_blocking`, `execute_stream`, and `prepare_metadata`. This refactor enhances code readability and reduces duplication by encapsulating the connection locking logic.
This commit refactors the OdbcConnection structure to utilize a new type for prepared statements, improving type safety and clarity. It also modifies the execute_sql function to handle both prepared and non-prepared SQL statements through a new MaybePrepared enum, streamlining execution logic. Additionally, the prepare method is updated to cache prepared statements more effectively, enhancing performance.
…cking This commit eliminates the run_blocking function from the ODBC module, replacing its usage with the spawn_blocking function directly in the OdbcConnection implementation. This change simplifies the code and enhances clarity in handling blocking tasks.
This commit refactors the execute_stream method in OdbcConnection to directly return the receiver from the execute_stream function, eliminating unnecessary complexity. Additionally, the method signature is updated to reflect the change in return type, enhancing clarity in the codebase.
This commit updates the fetch_many method in the Executor implementation for OdbcConnection to utilize into_future and then, simplifying the handling of query results and enhancing code clarity.
lovasoa
deleted the
cursor/refactor-odbc-driver-to-use-spawn-blocking-bb59
branch
Refactor the ODBC driver to use
spawn_blockinginstead of a dedicated worker thread, simplifying the codebase and removing boilerplate.This change removes the
ConnectionWorkermodule and its associated command/channel infrastructure, replacing it withtokio::task::spawn_blockingorasync_std::task::spawn_blockingfor executing blocking ODBC API calls. This centralizes blocking logic and error handling, reducing complexity and code duplication.