Rewire mssql-odbc fetch hot path onto the reactor-free sync core (L5) - #204
Conversation
87e2e72 to
5c09f0e
Compare
There was a problem hiding this comment.
Pull request overview
Reworks plaintext ODBC fetching to use the reactor-free TdsSyncClient, while retaining async fallback for unsupported transports.
Changes:
- Adds async/sync client ownership transitions around execution, fetching, and result boundaries.
- Adds batched row buffering and sync-fetch integration tests.
- Extends the mock TDS server with multi-result-set responses.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
mssql-odbc/Cargo.toml |
Adds mock-server test dependency. |
mssql-odbc/src/handles/stmt.rs |
Adds fetch buffering state. |
mssql-odbc/src/handles/dbc.rs |
Adds dual client ownership and version cache. |
mssql-odbc/src/api/sync_fetch_tests.rs |
Tests sync fetching and transitions. |
mssql-odbc/src/api/prepare.rs |
Resets buffered fetch state. |
mssql-odbc/src/api/more_results.rs |
Transitions clients across result sets. |
mssql-odbc/src/api/mod.rs |
Registers sync-fetch tests. |
mssql-odbc/src/api/get_type_info.rs |
Resets buffered fetch state. |
mssql-odbc/src/api/get_info.rs |
Reads cached server version. |
mssql-odbc/src/api/fetch.rs |
Implements sync and buffered fetching. |
mssql-odbc/src/api/execute.rs |
Resets fetch state before execution. |
mssql-odbc/src/api/exec_direct.rs |
Resets fetch state before direct execution. |
mssql-odbc/src/api/exec_common.rs |
Manages async/sync edge transitions. |
mssql-odbc/src/api/driver_connect.rs |
Caches negotiated server version. |
mssql-odbc/src/api/close_cursor.rs |
Restores async mode before draining. |
mssql-mock-tds/src/query_response.rs |
Models additional result sets. |
mssql-mock-tds/src/protocol.rs |
Serializes multi-result batches. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| self.current_row = None; | ||
| self.row_batch.clear(); | ||
| self.spare_rows.clear(); | ||
| self.pending_fetch_error = None; |
| for (idx, set) in sets.into_iter().enumerate() { | ||
| serialize_result_set(&mut result, set, idx == last); | ||
| } |
| .client | ||
| .as_ref() | ||
| .and_then(|c| c.server_version()) | ||
| .server_version |
Drive SQLFetch/SQLGetData through the blocking TdsSyncClient edge (no per-row block_on), with chunked SQLGetData for PLP varchar(max)/nvarchar(max), zero-copy narrow passthrough, and zero-alloc scalar/decimal writers. Batch-fetch machinery removed so single-row fetch is the only path.
5c09f0e to
01f5ffa
Compare
|
Closing, consistent with the rest of the sans-I/O stack (#189–#203, closed 2026-08-13). This PR is an L5–L7 consumer of the reactor-free sync core, so it has no base once that core is not landing. The stack was closed because the row-decode performance work that motivated it was re-scoped around #247, where benchmarked spikes showed the dominant win came from a far smaller change than inverting the core to a sync That argument has since been settled by measurement rather than projection. PR #264 has now merged ( Two further results from that work are worth recording here, because both cut against restructuring this path speculatively:
This is cleanup, not a rejection of the analysis. The branch is deliberately not deleted, so the work remains recoverable if the direction is revisited. |
L5 — Rewire mssql-odbc fetch onto
TdsSyncClientStacked on the frozen L4 tip (
dev/saurabh/sans-io-expose-l4-tdssyncclient@36bc4648). Append-only; base = L4 branch, notmain.What this does
Plaintext raw-TCP connections now serve
SQLFetchthrough the reactor-freeTdsSyncClient(blockingstdreads, no tokio reactor). TLS and other non-raw transports fall back to the untouched asyncblock_onpath, byte-identically — the speedup is opportunistic on plaintext with zero user-visible flag.Ownership machine
DbcClient { Async(TdsClient) | Sync(TdsSyncClient) }in the connection state.finish_execute), persisting across allSQLFetchcalls.into_async) atSQLMoreResults/ close / free before driving the existingadvance/close_query.into_sync()runs insidedbc.runtime.enter()so the runtime handle is captured for the later revert (a nakedinto_sync()on the bare ODBC thread would captureNoneand poisoninto_async). Implemented in one place:flip_to_fetch_edge.Batch buffer (batch-ready, per-row default)
SYNC_FETCH_DEFAULT_MAX_ROWS(default 1 = per-row byte-identical INFO/SWI parity; flip to 64 is a one-line change).max_rows > 1, buffered rows are served before the error surfaces, reproducing the async row-then-error ordering.Other
server_versionat connect soSQL_DBMS_VERno longer reads the live client while a sync cursor is open.#[unsafe(no_mangle)] extern "C"signatures touched.Test-infra note (flagged for scope)
mssql-mock-tds(test-only crate, not frozen L4) is extended additively for multi-result-set responses — needed to drive theSQLMoreResultsinterleave test over a real TCP peer. Single-set responses serialize byte-identically; its own 16 tests stay green.Tests (in-crate; the driver is a
cdylib, so an externaltests/crate can't link it or reachpub(crate)entry points)SQLFetch@ max_rows=1 == async oracle, byte-identicalSQLMoreResultsinterleave: sync-fetch →into_async→advance→ re-into_sync, both sets verifiedflip_to_fetch_edgekeeps the DBC on the async edgeGates:
cargo bfmtclean,cargo bclippy(-D warnings) clean,cargo nextest -p mssql-odbc= 472 passed (468 existing + 4 new),mssql-mock-tds16 passed.Part of the sans-I/O native stack - see cover PR #189 for the stack-level summary and tracked debt.