microsoft / microsoft/mssql-rs
mssql-odbc: invalidate cached prepared plan when descriptor-driven IPD/APD changes affect binding shape
- Dominant language
- Rust
- Stars
- 53
- Forks
- 14
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 137
Description
## Context
Follow-up from [PR #436](https://github.com/microsoft/mssql-rs/pull/436) ([AB#47437](https://sqlclientdrivers.visualstudio.com/b95cf060-8083-439d-8ef1-405d5bf219d8/_workitems/edit/47437) — descriptor records as the binding/metadata source of truth), flagged by Copilot code review.
## Problem
`SQLBindParameter` calls `StmtState::orphan_prepared_handle()` on every rebind, invalidating any cached server-side prepared plan so the next execute re-prepares against the new binding shape.
Since PR #436, the APD/IPD descriptor records are also directly writable via `SQLSetDescFieldW`/`SQLSetDescRec`. Changing a parameter's type or size through those APIs does **not** call `orphan_prepared_handle`. Sequence that reproduces the bug:
1. `SQLPrepare` + `SQLExecute` — materializes a server-side prepared handle (`PreparedStatement.id`).
2. `SQLSetDescFieldW`/`SQLSetDescRec` on the IPD changes `SQL_DESC_CONCISE_TYPE`/`SQL_DESC_LENGTH`/etc. for a parameter (not via `SQLBindParameter`).
3. `SQLExecute` again — `execute_prepared` takes the `sp_execute` path against the *old* prepared declaration, using the *new* binding metadata. Mismatch.
This also applies when an explicit APD is shared across multiple statements via `SQL_ATTR_APP_PARAM_DESC` reassociation — a change to that shared APD should invalidate every associated statement's plan, not just the one that made the change.
## Why this wasn't fixed directly in #436
A proactive fix (walk from the descriptor to its owning statement(s) and invalidate their plan at set-time) requires either:
- A new DESC-then-STMT lock acquisition order, which is the *reverse* of this crate's established STMT-then-DESC-only ordering (documented across `bind_col.rs`/`bind_param.rs`/etc.) — a genuine new deadlock risk against any concurrent STMT-then-DESC caller.
- Giving `DescHandle` a back-reference to its owning statement(s), which doesn't exist today and isn't a single pointer for APD/ARD (they can be shared across statements via reassociation).
Both are real architectural changes that shouldn't be rushed through without live multi-threaded validation (not available in the environment #436 was developed in).
## Suggested approach
Detect staleness **lazily** at execute time instead of invalidating **eagerly** at descriptor-set time:
- Store a lightweight fingerprint of the IPD (and/or APD) type/size shape in `PreparedPlan` when the plan is prepared/cached.
- At each execute, compare the current APD/IPD snapshot (already resolved via `snapshot_bound_params` at that point) against the stored fingerprint.
- If they differ, call `orphan_prepared_handle()` before deciding `sp_execute` vs. `sp_prepexec`.
This fits entirely within the existing STMT-locked execute path (no new lock ordering, no new cross-descriptor bookkeeping) and naturally covers the shared-APD-across-statements case too, since each statement independently compares its own current bindings against its own plan's fingerprint at its own execute time.
## References
- PR: https://github.com/microsoft/mssql-rs/pull/436
- Review thread: https://github.com/microsoft/mssql-rs/pull/436#discussion_r3899364170
- Related work item: [AB#47437](https://sqlclientdrivers.visualstudio.com/b95cf060-8083-439d-8ef1-405d5bf219d8/_workitems/edit/47437)
Contributor guide
Research direction
Start with the execute path around snapshot_bound_params, PreparedPlan, and orphan_prepared_handle(), then review the established STMT-then-DESC ordering documented in bind_col.rs and bind_param.rs. Verify the SQLPrepare/SQLExecute and descriptor-change sequence, including shared APDs; done means stale plans avoid sp_execute and are re-prepared when the binding shape changes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, sql
- Domain
- backend-api-design, databases
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100