microsoft / microsoft/mssql-rs

mssql-odbc: invalidate cached prepared plan when descriptor-driven IPD/APD changes affect binding shape

Open
#440 1 comment 0 reactions 0 assignees View on GitHub
mssql-odbc
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.