mssql-odbc: declare and bound data-at-execution parameters from ParameterType

[Shiwani Gupta (shiwanigupta0809)]

Description

SQLBindParameter's ParameterType and ColumnSize were ignored for data-at-execution parameters: every value streamed as a max type whatever the application declared, and no length bound was enforced. This aligns the DAE path with msodbcsql on both counts (AB#47590).

Declaration follows ParameterType. dae_plan decides stream-vs-buffer from the SQL type's PLP-ability, mirroring msodbcsql's IsPartialLenType (sqlcprot.h:1421), which keys on the SQL type alone rather than on ColumnSize. A fixed-width target is now collected and converted whole through the materializing path instead of being forced into a max stream it cannot represent.

ColumnSize bounds the value. dae_streamed_declaration narrows the @params declaration and dae_length_limit enforces the accumulated total in SQLPutData — applied before the streamed/buffered split and against the application buffer, where msodbcsql runs ValidatePutDataLength (sqlccmd.cpp:4571), so 22001 lands on the call that overflows rather than at close. Trailing pad units are trimmed rather than rejected, and trimmed padding does not consume the declaration's budget.

Transcoding is streamed, not deferred. DaeTranscode converts each chunk on the way out, carrying a trailing partial character into the next call — the shape of msodbcsql's ConvertLongData (sqlccnvt.cpp:841) with its cbTruncatedCharsInConvBuf carry and end-of-value flush (sqlccmd.cpp:5999-6001). Narrow encoding reuses encode_narrow, so the streamed and materialized paths agree on the wire bytes.

text/ntext/image keep their max substitution, tracked under AB#47592.

Please look closely at

1. A mixed sequence opens its RPC partway through, so a LOB still streams.

A fixed-width target has no PLP form, so its value must be collected before the RPC can declare it. Rather than deferring the whole sequence — which would hold a varbinary(max) bound alongside it entirely in memory — the RPC opens as soon as no parameter still to be visited needs collecting, and everything after that streams.

Parameters are offered in bind order, the same as msodbcsql. An earlier revision of this change sorted buffered parameters first so that a LOB bound before a fixed-width parameter could also stream; that was reverted. It optimized an unusual bind order at the cost of changing the order SQLParamData hands tokens back, which an application that counts iterations instead of comparing tokens would silently mispair. Not worth it — the remaining case (LOB bound first) simply buffers, exactly as it did before this PR.

2. Two bugs that only exist in combination with #444, so neither branch shows them alone.

• buffered_dae_to_rpc must repoint both strlen_or_ind_ptr and octet_length_ptr. mssql-odbc: buffer and transcode data-at-execution parameters with mismatched wideness #444 split the indicator from the length; repointing only the first left SQL_DATA_AT_EXEC on the synthesized binding.
• The try_reserve OOM guard added in mssql-odbc: buffer and transcode data-at-execution parameters with mismatched wideness #444 is preserved but re-sited onto whichever accumulator actually receives the bytes — buffer when the value converts at close, carry when it converts on the way out, unit_carry when an untranscoded stream realigns a unit split across chunks.

3. Narrow buffers are measured in UTF-16 units, not bytes.

convert_character_sql derives its limit from sql_type + ColumnSize alone and measures the source in UTF-16 units, so the C type never enters the unit. Measuring SQL_C_CHAR in bytes would have rejected é against varchar(1) — two bytes, one unit — that the same binding materialized accepts. The unit is an approximation of collation bytes on both paths; making it exact is AB#47584.

4. The declaration is only narrowed when the bound can be enforced.

A streamed parameter never reaches a close-time conversion, so a bound dae_length_limit cannot measure is enforced nowhere. Narrowing anyway would have the client declare varchar(n) and then stream past it, leaving the overflow to the server instead of the 22001 the declaration implies.

5. A SQL_C_WCHAR code unit split across two chunks is carried, not dropped.

A chunk can end part-way through a UTF-16 code unit. That stray byte is held back and joined to the front of the next chunk (fit_chunk / DaeProgress::unit_carry), so "abc" sent as [61 00 62] + [00 63 00] reassembles intact; DaeLengthLimit::finish releases a trailing half unit at close. Measuring the byte instead would put every later chunk one byte off the application's code-unit grid, so a following [0x20, 0x00] blank would read as half of one unit plus half of the next and report 22001 on padding — so the measurement is still floored to whole units, matching msodbcsql's cbValue &= ~(SQLULEN)1 (sqlccmd.cpp:10931), which masks a local copy governing the length check and runs after the bytes have already been pushed.

An earlier revision of this change dropped the byte and justified it as parity, citing that same mask. That was wrong on both counts: the mask floors the measurement, not the payload, and retail streams the odd byte (ConvertLongData's same-wideness branch forwards the full odd cbSrc). Dropping it silently turned "abc" into U+0061 U+6300 with no diagnostic. Caught in review by Saurabh Singh (@saurabh500); the reading came from source rather than a run, and the corrected distinction is now spelled out at fit and in dae_limit_fit_measures_whole_units_only.

6. A cross-family data-at-execution parameter now succeeds where main returns HYC00.

Binding SQL_C_CHAR to SQL_INTEGER with a data-at-execution indicator fails on main with HYC00 at SQLExecute. Here the chunks are collected and converted by the same code the materialized path uses, so "41" against SELECT ? + 1 returns 42 — exactly as the same binding already behaves when the value is passed in one buffer.

This moves toward msodbcsql, not away: measured against retail 18.6, its SQLPutData accepts the chunk and returns 42 as well. Both CrossFamilyDataAtExecutionConvertsToInteger tests therefore run on the parity leg rather than opting out.

Worth flagging because a comment in the tree claimed msodbcsql rejects this with HY019. Its ValidatePutDataLength does have an IsFixedSqlType arm raising IDS_HY_019, but SQL_INTEGER does not reach it at runtime — the claim came from reading the source rather than running it. Probing both drivers disproved it, and the stale claim is corrected here, in both test files and docs/parameters_plan.md.

Known gaps

DaeTranscode::push's output still allocates infallibly. The try_reserve guard covers the input side, but bounding the encoded output means threading try_reserve through encode_narrow and encoding_rs, which is beyond this change.

Malformed UTF-8 under-counts against ColumnSize on the streamed path. utf16_units_of_utf8_byte classifies each byte alone, so an orphan continuation byte costs 0 where the materialized path repairs it to one U+FFFD and charges a unit — [0x80, b'a'] against varchar(1) fits when streamed and is 22001 when buffered. Closing it needs UTF-8 decoder state carried across SQLPutData calls, since a leading continuation byte is otherwise indistinguishable from the tail of a legitimate split character — the case this design exists to support. Attempted and reverted rather than half-done; documented at the miscount. The value is still bounded server-side, so the effect is a late 22001, not data loss.

A value that ends part-way through a unit is treated differently by the two close paths. DaeLengthLimit::finish releases a trailing half unit, but the transcoded arm then decodes it through decode_utf16le, whose chunks_exact(2) discards a lone byte, while the passthrough arm writes it raw. Measured, not inferred: DaeTranscode::finish on a carry of [0x62] returns empty. Only reachable on malformed final input — a well-formed value ends on a unit boundary — and unlike the mid-stream case above it needs a deliberate contract (drop, U+FFFD, or 22001) applied to both arms rather than falling out of which plan the parameter took. Raised by Saurabh Singh (@saurabh500); left for a separate change, and I have not measured which arm matches retail.

These are deliberate, not oversights.

Related Issues

https://sqlclientdrivers.visualstudio.com/DefaultCollection/mssql-rs/_workitems/edit/47590

AB#47590

Checklist

• cargo bfmt passes
• cargo bclippy passes (workspace + mssql-tds + mssql-py-core)
• cargo btest passes (1398 mssqlodbc, 2124 mssql-tds)
• New/changed functionality has tests
• Public API changes are documented

Validation

• All e2e suites pass against this driver.
• 41/41 suites pass against retail msodbcsql 18.6 (full parity sweep, not just the affected ones).
• 28 DAE e2e tests, including the accumulate-across-chunks case and both wideness-mismatch directions.

Supersedes #488, which carries the review history for these changes across seven commits.

[Copilot]

🟡 Changes recommended

Deferred sequencing and split UTF-16 padding still contain correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Aligns ODBC data-at-execution parameters with their declared SQL type, size, and encoding.

Changes:

• Selects buffered or streamed delivery from ParameterType.
• Enforces ColumnSize and incrementally transcodes streamed character data.
• Adds unit, end-to-end, and parity coverage.

File summaries

File	Description
mssql-tds/src/message/parameters/rpc_parameters.rs	Supports narrowed streamed declarations.
mssql-odbc/tests/e2e/tests/param_cross_conversions_test.cpp	Tests buffered cross-family conversion.
mssql-odbc/tests/e2e/tests/param_char_conversions_test.cpp	Tests character bounds and declarations.
mssql-odbc/tests/e2e/tests/param_binary_conversions_test.cpp	Tests binary bounds and declarations.
mssql-odbc/tests/e2e/tests/execute_test.cpp	Tests mixed DAE sequencing and transcoding.
mssql-odbc/src/handles/stmt.rs	Extends deferred DAE state.
mssql-odbc/src/conversion/param_convert.rs	Adds planning, limits, buffering, and transcoding.
mssql-odbc/src/api/put_data.rs	Processes bounded streamed and buffered chunks.
mssql-odbc/src/api/param_data.rs	Completes deferred and streamed execution.
mssql-odbc/src/api/execute.rs	Defers prepared buffered execution.
mssql-odbc/src/api/exec_direct.rs	Defers direct buffered execution.
mssql-odbc/src/api/exec_common.rs	Builds and reconstructs DAE parameters.
mssql-odbc/src/api/cancel.rs	Updates DAE test setup.
.github/instructions/mssql-odbc.instructions.md	Records parameter-order divergence.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[David Engel (David-Engel)]

Automated review from an unattended run — these findings have not been checked by a human first, and this is a comment, not an approval.

Reviewed the full diff against merge base 4382290 (13 files, ~2.6k insertions), plus the surrounding bound_param_to_rpc / conversion_matrix / cancel_streamed_write code the new paths depend on. Ran cargo nextest run -p mssqlodbc --lib: 1308 passed, 1 skipped, so the cargo btest checklist box holds locally. gh pr checks 494 shows no failing required check (4 still pending), so nothing here contradicts CI.

The overall shape reads well: keying stream-vs-buffer off IsPartialLenType rather than ColumnSize, keeping the PLP body while narrowing only the @params declaration, and only narrowing when the bound is actually enforceable are all the right calls, and the reasoning is documented where a future reader will need it. Both prior Copilot threads are genuinely addressed. One of the two, though, is only half-fixed.

1. A split pad unit still fails 22001 — the fix covers the first chunk but not the byte that completes it

DaeLengthLimit::fit now accepts a trailing partial unit that is a prefix of the pad, but it does not record that it consumed part of a unit. already only advances by consumed, so the next call re-derives split from an even max and lands the overflow window on a byte offset that is one off the application's code-unit grid. The whole-unit comparison then reads [0x00, 0x20] instead of [0x20, 0x00] and reports truncation on an overflow that is entirely blanks — exactly the failure mode the previous round was meant to remove.

Verified in the worktree with a throwaway test (removed afterwards):

let wide = dae_length_limit(SQL_C_WCHAR, SQL_WVARCHAR, 1).unwrap().unwrap();
// Application stream: 'a' + two UTF-16 blanks, against nvarchar(1).
let (kept, consumed) = wide.fit(&[b'a', 0, b' '], 0).unwrap();
assert_eq!((kept, consumed), (&[b'a', 0][..], 2));
// The 0x00 that completes the split blank, then a whole blank.
let second = wide.fit(&[0, b' ', 0], consumed);
assert!(second.is_ok(), "overflow is entirely blanks but got {second:?}");

panicked at param_convert.rs: overflow is entirely blanks but got Err(StringTruncation)

The same misalignment shows up whenever a chunk boundary leaves the retained/overflow split at an odd byte offset, so it is not limited to the pad-prefix case: fit(&[b'a', 0, b'b'], 0) then fit(&[0, b' '], _) then fit(&[0], _) walks into it too. dae_limit_fit_accepts_a_pad_unit_split_across_chunks passes because it only ever calls fit once for the split unit.

Two ways out, both cheap: carry the partial pad byte count in the limit's running state so the next fit starts on the right boundary, or take msodbcsql's own route and mask the odd byte off the measured length (cbValue &= ~1, sqlccmd.cpp:10931) so a trailing half-unit is never classified at all. Whichever you pick, please add a two-call test — the current one cannot fail on this.

2. SQLPutData's buffered branch no longer claims the sequence

The old buffered branch checked the client out before appending, with a comment naming the hazard: a concurrent SQLParamData snapshots the accumulator, and an append that lands after that snapshot is silently discarded while SQLPutData still reports success. The new will_buffer branch appends under the statement lock but never checks the client out, so a SQLParamData that closes and advances between sql_put_data_safe's validation lock and its append lock now has this call append to the next parameter's progress.buffer — with put_data_called set on it — instead of failing. Your reply on the resolved param_data.rs thread says both halves now serialize on the same thing; the SQLPutData half opted out of it in the same change.

The window is narrow and only an application driving one statement handle from two threads can reach it, so this is a suggestion rather than a blocker — but the guard was deliberate, and restoring the checkout in the buffered branch costs one lookup.

Smaller notes, no action needed

• unwind_dae calling cancel_streamed_write on a deferred sequence is safe — tds_client.rs:1913 documents the no-active-write case as a no-op — so the new 22001/OOM aborts on a buffered parameter do not touch the connection. Worth a word in park_deferred_dae's doc so nobody re-derives it.
• dae_plan no longer checks family agreement, so a cross-family char/binary pairing would now stream (with DaeSource::Utf8 → DaeTarget::Raw running the bytes through a lossy UTF-8 round trip). It is unreachable today because is_supported_conversion rejects those pairings at bind, so this is only about the deleted defence, not a live bug. dae_plan_buffers_what_cannot_be_plp_framed would be a natural place to pin that it stays unreachable.

Nice work on the deferred/streaming split and on documenting why bind order was kept over the sort — that trade-off is the kind of thing that gets silently re-litigated later without the note. Finding 1 is worth another pass before this merges.

[Saurabh Singh (saurabh500)]

Disclosure: this review was produced by an unattended review sweep. The findings below were not checked by a human before posting — please weigh them accordingly.

Verdict: COMMENT — two blocking correctness bugs on the deferred data-at-execution path, plus one suggestion. Otherwise this is an exceptionally thorough, well-documented change, and its core conversion logic checks out.

This PR is the squash superseding #488. I read all three prior-discussion endpoints on both #494 and #488, ran the mssqlodbc lib suite (1308 passed), mutation-tested utf16_units_of_utf8_byte (the split-character and narrow-buffer tests do fail when the continuation cost is changed, so they genuinely guard it), and checked the Stream/Buffer split and per-SQLPutData length enforcement against the msodbcsql reference (ValidatePutDataLength called per SQLPutData at sqlccmd.cpp:3906; PLP streaming vs CacheNonBLOBDAEParam at sqlccmd.cpp:3916-3990) — the design faithfully mirrors it.

The three findings below are the three suppressed (never-threaded) Copilot comments from #488's final review (5103122097). They were never posted as threads, got no response, and I confirmed all three still stand against this squash's code by reading it — so they are not re-files of answered threads. I verified each mechanism myself rather than forwarding the bot.

Blocking (2) — details inline:

• param_data.rs:493 — a mixed buffered+streamed prepared execute leaves the statement unprepared and skips the no-row result drain.
• param_data.rs:521 — a failed deferred RPC open leaves the statement stuck in "Need Data" on a torn-down sequence.

Suggestion (1) — details inline:

• param_convert.rs:210 — malformed UTF-8 under-counts against ColumnSize on the streamed path, accepting a value the buffered path rejects with 22001.

Nit: none.

Cross-check I could not complete: I did not resolve AB#47590 in Azure DevOps and treated the AB# reference as satisfying the linked-work-item requirement.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 86%

🎯 Overall Coverage 93.6%

📦 Project: mssql-tds + mssql-odbc + mssql-py-core ℹ️ Note: diff coverage is reported, not enforced.

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

• mssql-odbc/src/api/cancel.rs (100%)
• mssql-odbc/src/api/exec_common.rs (81.9%): Missing lines 149,154-155,181-182,725,762-763,765-766,768-769,783-786,791-792,849-850,852-853,855-856,904-906
• mssql-odbc/src/api/exec_direct.rs (37.5%): Missing lines 247-256
• mssql-odbc/src/api/execute.rs (100%)
• mssql-odbc/src/api/param_data.rs (59.9%): Missing lines 168-169,176-177,194-196,200-201,204-206,223,291,293-297,299,424-425,428-429,439-441,452,461-468,485-487,489-492,505-507,510-515,533-546,548-550,556-562,585-586,589-590,608,611-619,635-637,648-651,653-656,669-671,679,681-682
• mssql-odbc/src/api/put_data.rs (90.8%): Missing lines 171-172,177-179,333-335,453-454,466,477-480,511,535
• mssql-odbc/src/conversion/param_convert.rs (97.9%): Missing lines 456,479,507,541,630,706,728,747,767,4141,4357
• mssql-odbc/src/handles/stmt.rs (99.1%): Missing lines 909
• mssql-tds/src/message/parameters/rpc_parameters.rs (100%)

Summary

• Total: 1276 lines
• Missing: 173 lines
• Coverage: 86%

mssql-odbc/src/api/exec_common.rs

  145             // A pairing whose buffer bytes already are the wire's bytes keeps
  146             // `None`, so `SQLPutData` forwards its chunks borrowed instead of
  147             // copying each one through a conversion that would return them
  148             // unchanged. Skipping the transcode also skips the close-time
! 149             // flush, which is right: a passthrough parameter never holds back a
  150             // partial character to carry.
  151             if !transcode.is_passthrough() {
  152                 param.transcode = Some(transcode);
  153             }
! 154         }
! 155     }
  156     let Ok(mut stmt_state) = stmt.inner.lock() else {
  157         // The client has nowhere to go: the statement that owns it is
  158         // unreachable and the DBC still records it as busy.
  159         error!("{op}: stmt mutex poisoned while parking DAE client");

---

  177 pub(super) fn park_deferred_dae(
  178     stmt: &StmtHandle,
  179     client: TdsClient,
  180     prepared: Option<PreparedPlan>,
! 181     orphaned: Option<StatementId>,
! 182     dae_params: Vec<DaeParam>,
  183     prebuilt: Vec<RpcParameter>,
  184     sql: Option<String>,
  185     timeout_secs: u32,
  186     fractional_truncated: bool,

---

  721                     post_diag(stmt_state, e.diag());
  722                     return Err(SQL_ERROR);
  723                 }
  724             };
! 725             // A bound that can be measured in buffer bytes is applied as the
  726             // chunks arrive, so an overflow is reported by the `SQLPutData`
  727             // that carries it rather than at close (msodbcsql parity).
  728             let length_limit = match dae_length_limit(
  729                 bound_param.c_type,

---

  758                     // The body is PLP whatever `ColumnSize` says, but the
  759                     // variable it lands in is declared from `ParameterType`,
  760                     // matching the materialized path and msodbcsql (AB#47590).
  761                     //
! 762                     // Only narrowed when the bound above can actually be
! 763                     // enforced. A streamed parameter never reaches a close-time
  764                     // conversion, so an unenforced bound would have the client
! 765                     // declare `varchar(n)` and then stream past it, leaving the
! 766                     // overflow to the server instead of the `22001` the
  767                     // declaration implies. Staying `max` keeps the value intact
! 768                     // until the bound is measurable on this path too
! 769                     // (AB#47590).
  770                     let declaration = if length_limit.is_some() {
  771                         dae_streamed_declaration(bound_param.sql_type, bound_param.column_size)
  772                     } else {
  773                         Ok(None)

---

  779                             error!(
  780                                 "{op}: parameter {} declaration invalid: {}",
  781                                 i + 1,
  782                                 e.diag().text
! 783                             );
! 784                             post_diag(stmt_state, e.diag());
! 785                             return Err(SQL_ERROR);
! 786                         }
  787                     }
  788                 }
  789                 DaePlan::Buffer => RpcParameter::data_at_exec(
  790                     Some(name),
! 791                     StatusFlags::NONE,
! 792                     StreamedSqlType::VarBinaryMax,
  793                 ),
  794             };
  795             params.push(rpc);
  796         } else {

---

  845     post_diag(&mut stmt_state, WARN_FRACTIONAL_TRUNCATION);
  846     SQL_SUCCESS_WITH_INFO
  847 }
  848 
! 849 /// Rebuilds the RPC parameter list once every data-at-execution value has been
! 850 /// collected, for a sequence that deferred its execute.
  851 ///
! 852 /// Only the data-at-execution slots are rebuilt, from the bytes
! 853 /// [`buffered_dae_to_rpc`] converts; every other parameter is the one
  854 /// `build_named_params` already materialized at execute time and is kept
! 855 /// verbatim. A buffered value therefore still goes through the same conversion
! 856 /// the materialized path uses, so it is declared and bounded exactly like a
  857 /// value supplied in a single buffer (AB#47590).
  858 ///
  859 /// Nothing here reads application memory. Re-reading it would be unsound rather
  860 /// than merely redundant: `bound_params` is an execute-time snapshot that

---

  900                 post_diag(stmt_state, e.diag());
  901                 return Err(SQL_ERROR);
  902             }
  903         }
! 904     }
! 905 
! 906     Ok(params)
  907 }
  908 
  909 /// Captures result metadata after a successful execution and finalizes the
  910 /// statement/connection state.

---

mssql-odbc/src/api/exec_direct.rs

  243     // Data-at-execution parameters park the half-written RPC on the statement
  244     // and hand control to SQLParamData / SQLPutData. There is no prepared plan
  245     // to restore afterwards, so `None` is passed for it.
  246     if !dae_params.is_empty() {
! 247         // A buffered parameter cannot be declared until its bytes are all in,
! 248         // so no RPC is opened: the sequence collects its values and runs
! 249         // `sp_executesql` from the last `SQLParamData` (AB#47590).
! 250         if dae_params.iter().any(|param| param.plan.is_buffered()) {
! 251             return park_deferred_dae(
! 252                 stmt,
! 253                 client,
! 254                 None,
! 255                 None,
! 256                 dae_params,
  257                 params,
  258                 Some(rewritten_sql),
  259                 query_timeout,
  260                 fractional_truncated,

---

mssql-odbc/src/api/param_data.rs

  164     // The parameter closes into its buffer rather than onto the wire, and the
  165     // execute runs once the last one is in (AB#47590).
  166     let is_deferred = {
  167         let Ok(stmt_state) = stmt.inner.lock() else {
! 168             error!("SQLParamData: stmt mutex poisoned checking deferred mode");
! 169             return SQL_ERROR;
  170         };
  171         stmt_state.dae.as_ref().is_some_and(|dae| dae.deferred)
  172     };
  173     if is_deferred {

---

  172     };
  173     if is_deferred {
  174         let (has_more, buffered_phase_done, next_ptr) = {
  175             let Ok(mut stmt_state) = stmt.inner.lock() else {
! 176                 error!("SQLParamData: stmt mutex poisoned closing a buffered parameter");
! 177                 return SQL_ERROR;
  178             };
  179             // The close validation above ran under its own lock and released it,
  180             // so two concurrent calls can both reach here having judged the same
  181             // parameter complete. Checking the client out claims the sequence

---

  190                 .and_then(|dae| dae.checkout_client())
  191             {
  192                 Some(client) => client,
  193                 None => {
! 194                     error!("SQLParamData: DAE sequence is already being closed by another call");
! 195                     post_diag(&mut stmt_state, ERR_FUNCTION_SEQUENCE);
! 196                     return SQL_ERROR;
  197                 }
  198             };
  199             let Some(dae) = stmt_state.dae.as_mut() else {
! 200                 error!("SQLParamData: DAE sequence vanished closing a buffered parameter");
! 201                 return SQL_ERROR;
  202             };
  203             let Some(bound_index) = dae.current_param().map(|param| param.bound_index) else {
! 204                 error!("SQLParamData: no open parameter to close");
! 205                 dae.return_client(client);
! 206                 return SQL_ERROR;
  207             };
  208             let bytes = std::mem::take(&mut dae.progress.buffer);
  209             let is_null = dae.progress.is_null;
  210             dae.buffered.push((bound_index, bytes, is_null));

---

  219         // complete and the RPC can be opened now. The rest of the sequence goes
  220         // onto the wire as it arrives instead of being collected whole, which is
  221         // the whole point of data-at-execution for a LOB.
  222         if buffered_phase_done && let Some(rc) = open_deferred_rpc(dbc, stmt, statement_handle) {
! 223             return rc;
  224         }
  225 
  226         // More parameters to collect: hand back the next token.
  227         if has_more {

---

  287         (client, trailing)
  288     };
  289 
  290     if !trailing.is_empty()
! 291         && let Err(e) = dbc.runtime.block_on(client.write_streamed_chunk(&trailing))
  292     {
! 293         error!(%e, "SQLParamData: flushing the transcoder tail failed");
! 294         if let Ok(mut stmt_state) = stmt.inner.lock() {
! 295             let parked = stmt_state.take_dae();
! 296             debug_assert!(parked.is_none(), "the client is checked out by this call");
! 297             stmt_state.clear_state(STMT_STATE_EXEC_STARTED);
  298         }
! 299         return fail_with_tds(dbc, stmt, statement_handle, client, &e);
  300     }
  301 
  302     let end_result = dbc.runtime.block_on(client.end_streamed_param());

---

  420 ///
  421 /// Returns `Some(rc)` only on failure; success leaves the sequence open with its
  422 /// cursor untouched, so the caller hands back the next parameter's token exactly
  423 /// as it would have.
! 424 ///
! 425 /// Without this the whole sequence stays deferred and every parameter is
  426 /// collected whole, so one fixed-width value alongside a `varbinary(max)` would
  427 /// cost memory proportional to the LOB — the opposite of what data-at-execution
! 428 /// is for (AB#47590).
! 429 fn open_deferred_rpc(
  430     dbc: &crate::handles::DbcHandle,
  431     stmt: &StmtHandle,
  432     statement_handle: SqlHandle,
  433 ) -> Option<SqlReturn> {

---

  435         let Ok(mut stmt_state) = stmt.inner.lock() else {
  436             error!("SQLParamData: stmt mutex poisoned opening the deferred RPC");
  437             return Some(SQL_ERROR);
  438         };
! 439         let Some(dae) = stmt_state.dae.as_mut() else {
! 440             error!("SQLParamData: DAE sequence vanished opening the deferred RPC");
! 441             return Some(SQL_ERROR);
  442         };
  443         let collected = std::mem::take(&mut dae.buffered);
  444         let dae_params = dae.params().to_vec();
  445         let prebuilt = std::mem::take(&mut dae.prebuilt);

---

  448         let prepared = dae.take_prepared();
  449         let orphaned = dae.take_orphaned();
  450         let Some(client) = dae.checkout_client() else {
  451             error!("SQLParamData: DAE sequence has no client to open its RPC on");
! 452             post_diag(&mut stmt_state, ERR_FUNCTION_SEQUENCE);
  453             return Some(SQL_ERROR);
  454         };
  455 
  456         let params = match rebuild_deferred_params(

---

  457             &mut stmt_state,
  458             prebuilt,
  459             &collected,
  460             &dae_params,
! 461             "SQLParamData",
! 462         ) {
! 463             Ok(params) => params,
! 464             Err(rc) => {
! 465                 // Nothing was sent, so the statement goes back to being merely
! 466                 // prepared rather than needing a cancel.
! 467                 //
! 468                 // The client this call checked out is returned explicitly:
  469                 // `take_dae` cannot produce it, because the checkout already
  470                 // removed it from the sequence. Binding its `None` over this
  471                 // one would drop the connection's only client and leave the DBC
  472                 // permanently busy.

---

  481             }
  482         };
  483         (client, params, prepared, orphaned, sql, timeout_secs)
  484     };
! 485 
! 486     let (mut client, params, mut prepared, mut orphaned, sql, timeout_secs) = taken;
! 487     let collation = client.get_collation();
  488     let options = ExecuteOptions::new().timeout_secs(timeout_secs);
! 489 
! 490     let begin_result = match (prepared.as_mut(), sql) {
! 491         (Some(plan), _) => dbc.runtime.block_on(client.begin_execute_prepared(
! 492             &mut plan.stmt,
  493             params,
  494             &mut orphaned,
  495             options,
  496         )),

---

  501             error!("SQLParamData: deferred sequence has neither a plan nor SQL text");
  502             return_client_idle(dbc, statement_handle, client);
  503             clear_exec_started(stmt);
  504             return Some(SQL_ERROR);
! 505         }
! 506     };
! 507 
  508     // The plan goes back before either outcome is reported, exactly as the
  509     // immediate path does: a failure must still leave the statement prepared.
! 510     match begin_result {
! 511         Ok(StreamedParamStatus::NeedData { .. }) => {
! 512             let Ok(mut stmt_state) = stmt.inner.lock() else {
! 513                 // The RPC is open and the client is in hand, so it cannot be
! 514                 // parked back on a statement whose lock is unusable. Hand it to
! 515                 // the DBC rather than dropping it, or the connection is left
  516                 // busy with no client for the rest of its life.
  517                 error!("SQLParamData: stmt mutex poisoned parking the opened RPC");
  518                 return_client_idle(dbc, statement_handle, client);
  519                 return Some(SQL_ERROR);

---

  529             // Back onto the *sequence*, not the statement: the sequence outlives
  530             // this call and its closing `take_dae` restores `StmtState` from
  531             // these fields, so writing them to the statement here would have
  532             // that restore overwrite the live plan with `None` -- leaving a
! 533             // successful mixed execute unprepared and skipping the no-row drain
! 534             // its `was_prepared` check gates.
! 535             dae.restore_plan(prepared, orphaned);
! 536             dae.begin_streaming_phase(client, collation);
! 537             None
! 538         }
! 539         Ok(StreamedParamStatus::Complete(_)) => {
! 540             // Unreachable: this runs only while a streamed parameter is still
! 541             // open, so the RPC cannot have completed. Torn down all the same --
! 542             // the sequence is a husk by now, its client checked out and its plan
! 543             // and collected values taken, so leaving it installed would keep the
! 544             // statement in "Need Data" over state nothing can complete.
! 545             error!("SQLParamData: deferred RPC completed despite a streamed parameter");
! 546             if let Ok(mut stmt_state) = stmt.inner.lock() {
  547                 let parked = stmt_state.take_dae();
! 548                 debug_assert!(parked.is_none(), "the client is checked out by this call");
! 549                 stmt_state.prepared = prepared;
! 550                 stmt_state.pending_unprepare = orphaned;
  551             }
  552             Some(finish_execute(
  553                 dbc,
  554                 stmt,

---

  552             Some(finish_execute(
  553                 dbc,
  554                 stmt,
  555                 statement_handle,
! 556                 client,
! 557                 "SQLParamData",
! 558             ))
! 559         }
! 560         Err(e) => {
! 561             error!(%e, "SQLParamData: opening the deferred RPC failed");
! 562             if let Ok(mut stmt_state) = stmt.inner.lock() {
  563                 // The sequence cannot be resumed: its client is checked out and
  564                 // its plan, collected values and prebuilt parameters were all
  565                 // taken to open the RPC. Leaving it installed would keep
  566                 // `needs_data()` true after this call returns `SQL_ERROR`, so

---

  581 ///
  582 /// The whole parameter list is rebuilt from the application's bindings and the
  583 /// collected buffers, so the values go out declared and bounded by the same
  584 /// conversion a materialized execute uses; the request itself is then the
! 585 /// ordinary `sp_execute` / `sp_executesql` one, not a streamed variant
! 586 /// (AB#47590).
  587 fn run_deferred_execute(
  588     dbc: &crate::handles::DbcHandle,
! 589     stmt: &StmtHandle,
! 590     statement_handle: SqlHandle,
  591 ) -> SqlReturn {
  592     // Take everything the execute needs, and end the sequence, in one critical
  593     // section: a statement observed between the two would look idle but
  594     // unprepared.

---

  604         let collected = std::mem::take(&mut dae.buffered);
  605         let dae_params = dae.params().to_vec();
  606         let prebuilt = std::mem::take(&mut dae.prebuilt);
  607         let sql = dae.sql.take();
! 608         let timeout_secs = dae.timeout_secs;
  609         let prepared = dae.take_prepared();
  610         let mut orphaned = dae.take_orphaned();
! 611 
! 612         let params = match rebuild_deferred_params(
! 613             &mut stmt_state,
! 614             prebuilt,
! 615             &collected,
! 616             &dae_params,
! 617             "SQLParamData",
! 618         ) {
! 619             Ok(params) => params,
  620             Err(rc) => {
  621                 // Nothing was sent, so the statement goes back to being merely
  622                 // prepared rather than needing a cancel.
  623                 let client = stmt_state.take_dae();

---

  631                 return rc;
  632             }
  633         };
  634 
! 635         let client = stmt_state.take_dae();
! 636         // `EXEC_STARTED` deliberately stays set across the execute below, exactly
! 637         // as the immediate path holds it for its whole round trip and lets
  638         // `finish_execute` / `fail_with_tds` clear it. Clearing it here would
  639         // open a window in which a concurrent `SQLPrepareW` passes its
  640         // active-execute guard and installs a plan that the `prepared` restore
  641         // after the execute would then silently overwrite.

---

  644 
  645     let (client, params, mut prepared, mut orphaned, sql, timeout_secs) = taken;
  646     let Some(mut client) = client else {
  647         error!("SQLParamData: deferred sequence has no client to execute on");
! 648         clear_exec_started(stmt);
! 649         return SQL_ERROR;
! 650     };
! 651 
  652     let options = ExecuteOptions::new().timeout_secs(timeout_secs);
! 653     let was_prepared = prepared.is_some();
! 654     let exec_result: Result<Option<StatementResult>, mssql_tds::error::Error> =
! 655         match (prepared.as_mut(), sql) {
! 656             (Some(plan), _) => dbc
  657                 .runtime
  658                 .block_on(client.execute_prepared(&mut plan.stmt, params, &mut orphaned, options))
  659                 .map(Some),
  660             (None, Some(sql)) => dbc

---

  665                 error!("SQLParamData: deferred sequence has neither a plan nor SQL text");
  666                 return_client_idle(dbc, statement_handle, client);
  667                 clear_exec_started(stmt);
  668                 return SQL_ERROR;
! 669             }
! 670         };
! 671 
  672     // Give the plan back before reporting either outcome, exactly as the
  673     // immediate path does: a failure must still leave the statement prepared.
  674     if let Ok(mut stmt_state) = stmt.inner.lock() {
  675         stmt_state.prepared = prepared;

---

  675         stmt_state.prepared = prepared;
  676         stmt_state.pending_unprepare = orphaned;
  677     }
  678 
! 679     let stmt_result = match exec_result {
  680         Ok(result) => result,
! 681         Err(e) => {
! 682             error!(%e, "SQLParamData: deferred execute failed");
  683             return fail_with_tds(dbc, stmt, statement_handle, client, &e);
  684         }
  685     };

---

mssql-odbc/src/api/put_data.rs

  167         // recorded and `SQLParamData` builds a typed NULL from it, exactly as a
  168         // materialized parameter with a `SQL_NULL_DATA` indicator produces one.
  169         {
  170             let Ok(mut stmt_state) = stmt.inner.lock() else {
! 171                 error!("SQLPutData: stmt mutex poisoned marking a buffered parameter NULL");
! 172                 return SQL_ERROR;
  173             };
  174             if let Some(dae) = stmt_state.dae.as_mut()
  175                 && dae.deferred
  176             {
! 177                 dae.progress.put_data_called = true;
! 178                 dae.progress.is_null = true;
! 179                 return SQL_SUCCESS;
  180             }
  181         }
  182 
  183         let write_result = {

---

  329         }
  330 
  331         let plan = dae.current_param().map(|param| param.plan);
  332         let Some(plan) = plan else {
! 333             error!("SQLPutData: open data-at-execution parameter has no plan");
! 334             post_diag(&mut stmt_state, ERR_FUNCTION_SEQUENCE);
! 335             return SQL_ERROR;
  336         };
  337 
  338         let will_buffer = plan.is_buffered() || dae.deferred;

---

  449                 consumed = chunk.len();
  450             }
  451         }
  452         let Some(dae) = stmt_state.dae.as_mut() else {
! 453             error!("SQLPutData: DAE sequence ended between locks");
! 454             return SQL_ERROR;
  455         };
  456         dae.progress.unit_carry = unit_carry;
  457         let fitted: &[u8] = &fitted_owned;
  458         let retained_total = retained_before.saturating_add(consumed);

---

  462         // sequence there is no open request at all, so every parameter
  463         // accumulates, whatever its own plan says.
  464         if will_buffer {
  465             let Some(dae) = stmt_state.dae.as_mut() else {
! 466                 error!("SQLPutData: DAE sequence ended between locks");
  467                 return SQL_ERROR;
  468             };
  469             // `SQLParamData` holds the client across the whole close, snapshots
  470             // this accumulator and advances the cursor. Appending while that is

---

  473             // Checking the same in-flight flag the checkout raises gives the two
  474             // halves one mutual-exclusion signal without this branch needing a
  475             // client it never writes with.
  476             if dae.call_in_flight() {
! 477                 dae.progress.unit_carry = unit_carry_restore;
! 478                 error!("SQLPutData: DAE sequence is in use by another call");
! 479                 post_diag(&mut stmt_state, ERR_FUNCTION_SEQUENCE);
! 480                 return SQL_ERROR;
  481             }
  482             dae.progress.buffer.extend_from_slice(fitted);
  483             dae.progress.bytes_sent = app_total;
  484             dae.progress.retained_units = retained_total;

---

  507                     dae.progress.carry = carry;
  508                     Cow::Owned(out)
  509                 }
  510                 None => {
! 511                     error!("SQLPutData: DAE sequence ended between locks");
  512                     return SQL_ERROR;
  513                 }
  514             },
  515             None => Cow::Owned(fitted_owned),

---

  531                         if let Some(restore) = carry_restore {
  532                             dae.progress.carry = restore;
  533                         }
  534                         dae.progress.unit_carry = unit_carry_restore;
! 535                     }
  536                     error!("SQLPutData: DAE client is unavailable — internal state corruption");
  537                     post_diag(&mut stmt_state, ERR_FUNCTION_SEQUENCE);
  538                     return SQL_ERROR;
  539                 }

---

mssql-odbc/src/conversion/param_convert.rs

  452         if !overflow.chunks(unit).all(|unit| unit == self.pad_unit) {
  453             return Err(ParamBuildError::StringTruncation);
  454         }
  455         Ok((&chunk[..split], consumed))
! 456     }
  457 }
  458 
  459 /// The `ColumnSize` bound a buffered parameter is held to as its chunks arrive,
  460 /// or `None` when the bound cannot be expressed in buffer bytes and is left to

---

  475     };
  476 
  477     // A pairing can be measured here when the declaration's unit and the
  478     // buffer's unit are the same thing. Within the character family they always
! 479     // are, whatever the wideness: `convert_character_sql` derives its limit from
  480     // `sql_type` and `ColumnSize` alone and hands it to `trim_blank_overflow`,
  481     // which measures the *source* in UTF-16 units -- the C type never enters
  482     // into the unit. So `varchar(n)` and `nvarchar(n)` both bound n UTF-16 units
  483     // of whatever buffer was bound, and a wideness mismatch is measurable on

---

  503     // The ceiling each declaration imposes, in its own unit. `char`/`binary`
  504     // reject a zero `ColumnSize` where the variable-width types read it as the
  505     // `max` spelling, so both go through the same helpers the materialized path
  506     // uses rather than a second reading of the rules.
! 507     let units = match sql_type {
  508         SQL_CHAR => Some(usize::from(fixed_length(
  509             column_size,
  510             SQL_PREC_BIGCHARBINARY,
  511         )?)),

---

  537         pad_unit,
  538     }))
  539 }
  540 
! 541 /// Builds the RPC parameter for a data-at-execution value that was buffered
  542 /// rather than streamed, from the bytes `SQLPutData` collected.
  543 ///
  544 /// Routed through [`bound_param_to_rpc`] over a binding that points at the
  545 /// collected buffer, so a streamed value is declared and converted by exactly

---

  626     })
  627 }
  628 
  629 /// The `@params` declaration a streamed parameter is given, or `None` when
! 630 /// `ColumnSize` names the `max` spelling and the declaration the streamed type
  631 /// already carries is right.
  632 ///
  633 /// The value body stays PLP-framed either way: only the variable it is assigned
  634 /// to narrows. `text`/`ntext`/`image` keep their `max` substitution for the same

---

  702             _ => DaeTarget::Raw,
  703         };
  704         Self { source, target }
  705     }
! 706 
  707     /// `true` when the buffer's bytes are already the wire's bytes, so a chunk
  708     /// can be written without being copied or carried.
  709     pub(crate) fn is_passthrough(&self) -> bool {
  710         match (self.source, self.target) {

---

  724             return chunk.to_vec();
  725         }
  726         let mut buf = std::mem::take(carry);
  727         buf.extend_from_slice(chunk);
! 728         let split = buf.len() - self.incomplete_tail(&buf);
  729         carry.extend_from_slice(&buf[split..]);
  730         self.encode(&self.decode(&buf[..split]))
  731     }

---

  743     /// How many trailing bytes begin a character that is not finished yet.
  744     fn incomplete_tail(&self, buf: &[u8]) -> usize {
  745         match self.source {
  746             DaeSource::Raw => 0,
! 747             DaeSource::Utf8 => incomplete_utf8_tail(buf),
  748             DaeSource::Utf16 => incomplete_utf16_tail(buf),
  749         }
  750     }

---

  763             // this crate cannot map falls back identically on both.
  764             DaeTarget::Narrow(collation) => encode_narrow(text, collation),
  765             DaeTarget::Raw => text.as_bytes().to_vec(),
  766         }
! 767     }
  768 }
  769 
  770 /// Length of the trailing bytes of `buf` that begin a UTF-8 sequence which is
  771 /// not finished yet. A sequence is at most four bytes, so only the last three

---

  4137         let collation = SqlCollation::default();
  4138         for (c_type, sql_type) in [
  4139             (SQL_C_BINARY, SQL_VARBINARY),
  4140             (SQL_C_BINARY, SQL_LONGVARBINARY),
! 4141             (SQL_C_WCHAR, SQL_WVARCHAR),
  4142         ] {
  4143             assert!(
  4144                 DaeTranscode::new(c_type, sql_type, collation).is_passthrough(),
  4145                 "{c_type} -> {sql_type} is already the wire's bytes"

---

  4353     ///
  4354     /// It streams like any other PLP-framable pairing; the encoding difference
  4355     /// is handled per chunk by [`DaeTranscode`] rather than by refusing it.
  4356     #[test]
! 4357     fn wideness_mismatched_dae_streams_and_transcodes() {
  4358         for (c_type, sql_type, streamed) in [
  4359             (SQL_C_WCHAR, SQL_VARCHAR, StreamedSqlType::VarcharMax),
  4360             (SQL_C_WCHAR, SQL_LONGVARCHAR, StreamedSqlType::VarcharMax),
  4361             (SQL_C_CHAR, SQL_WVARCHAR, StreamedSqlType::NVarcharMax),

---

mssql-odbc/src/handles/stmt.rs

  905             Some(cursor) => self
  906                 .params
  907                 .get(cursor..)
  908                 .is_some_and(|rest| !rest.is_empty() && rest.iter().all(|p| !p.plan.is_buffered())),
! 909             None => false,
  910         }
  911     }
  912 
  913     /// Switches the sequence from collecting to streaming once its RPC is open,

---

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[Saurabh Singh (saurabh500)]

Disclosure: this review was produced by an unattended review sweep running as saurabh500, and has not been checked by a human before posting.

Verdict: COMMENT. My three earlier threads are all addressed in substance — I re-read each against the code in 039a0e44 and replied inside the existing threads (I am not resolving them, per this sweep's rules). One new Suggestion on the even-length mask that closed the earlier padding-split thread. No blocking issues.

Blocking: none.

Suggestion (1): The even-length mask in DaeLengthLimit::fit drops a straddling SQL_C_WCHAR byte instead of carrying it, so a value split mid-unit across SQLPutData calls misaligns and reassembles to the wrong bytes with no diagnostic. It is the other half of David-Engel's still-open padding-split thread — the mask is the response to that thread — so I've left the detail, an empirical repro, the parity question I could not settle against msodbcsql, and a fix direction inline on mssql-odbc/src/conversion/param_convert.rs at the mask. I ranked it Suggestion rather than Blocking only because reachability needs an odd-length (irregular) WCHAR chunk and I could not prove retail msodbcsql preserves such a split; if it does, it rises to data loss. It is not caught by the existing pad-split test, which binds SQL_C_CHAR and never reaches the mask.

Nit: none.

On the three prior threads (replies inline, not resolved):

• Data-at-exec restore_plan on the NeedData arm — fixed correctly; restore_plan is symmetric to take_prepared/take_orphaned and the plan goes back on the sequence so the closing take_dae recovers it. The extra fix to the dae-vanished early return is right too.
• Failed deferred-RPC-open teardown — fixed; both the Err and the documented-unreachable Complete arms now take_dae() before failing, with the debug_assert!(parked.is_none()).
• Malformed-UTF-8 undercount on the streamed path — documented, not fixed, which is the disposition we agreed on; the note on utf16_units_of_utf8_byte states the miscount, why byte-wise classification can't distinguish the cases, and that the effect is a late 22001 rather than corruption. Fine to defer under AB#47590.

[Copilot]

🟡 Changes recommended

Deferred-close concurrency can skip parameters, and bounded split UTF-16 chunks can lose data.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

mssql-odbc/src/api/param_data.rs:190

• The checkout does not actually serialize two deferred closes: it is returned at line 214 before this mutex guard is released. If two SQLParamData calls both pass the earlier close validation, the second acquires this lock after the first, checks out the now-returned client, and captures/advances the next parameter without validating that parameter. Keep the validated cursor generation through this block (or revalidate put_data_called for the same cursor while holding this lock) before capturing and advancing.

            let client = match stmt_state
                .dae
                .as_mut()
                .and_then(|dae| dae.checkout_client())

• Files reviewed: 13/13 changed files
• Comments generated: 3
• Review effort level: Balanced

[Saurabh Singh (saurabh500)]

Posted by an unattended review sweep — I have not read this PR live; the thread replies below were generated and posted automatically.

Verdict: not ready to merge as-is. One blocking correctness issue remains — the SQL_C_WCHAR odd-split mask in DaeLengthLimit::fit (mssql-odbc/src/conversion/param_convert.rs:337). Everything else I flagged in the earlier round is either addressed or explicitly deferred, and the force-push to 4ba272b6 preserved those fixes. It's still a draft, so treat this as pre-merge feedback rather than a gate.

Blocking

• param_convert.rs:337 — fit drops a straddling SQL_C_WCHAR byte instead of carrying it, silently corrupting a bounded same-family WCHAR data-at-execution parameter that an app splits at an odd byte across SQLPutData calls. The parity question I left open last round is now settled against msodbcsql (it streams the odd byte, it does not floor it), which moves this from Suggestion to Blocking. Source trace and the fix shape are inline in that thread.

Suggestion

• param_convert.rs:269 — malformed-UTF-8 under-count on the streamed path (late 22001 vs the materialized path's early one). Documented-not-fixed under AB#47590; fine to defer. Detail inline in that thread.

Nit

• None.

The two earlier Blocking items on param_data.rs (the NeedData/restore_plan restore and the failed-open take_dae teardown) are addressed and survived the squash; confirmations are inline in their threads. I did not resolve any thread (sweep discipline).

[Saurabh Singh (saurabh500)]

🤖 Automated review from an unattended PR-review sweep. Event is COMMENT only — I won't approve, request changes, merge, enable auto-merge, or resolve any thread (including my own). Reviewed at head ca634f67.

Verdict: The blocker I raised last round is resolved. The odd-SQL_C_WCHAR-split corruption is now fixed by carrying the half-unit across SQLPutData calls in DaeProgress::unit_carry, and the guarding test is the exact two-call non-padding shape I asked for — I re-ran it and mutation-checked it (removing the carry line makes it fail [97,0,0,99] vs [97,0,98,0,99,0]). The fit comment/test that conflated "measured without the odd byte" with "the byte is dropped" is also corrected — it now says plainly that msodbcsql floors the measurement, not the payload. No Blocking findings from me this round. Two non-blocking Suggestions on the new carry machinery's failure/memory handling. This is still a draft, so this is not a merge sign-off — just where the code lands at ca634f67.

Blocking: none.

Suggestion:

1. unit_carry isn't rolled back on the two retriable SQLPutData failures, unlike the sibling transcoder carry right beside it. Silent WCHAR corruption, but only in the two-thread + app-retries-after-HY010 window — so Suggestion, not Blocking. Inline on put_data.rs.
2. Redundant full-chunk clone() on the untranscoded passthrough path, and the reworked try_reserve now guards a vector the non-transcoded carry path never fills. Inline on put_data.rs.

Nit: none.

Two of my other open threads are untouched by this force-push and stand as last assessed: the failed-open teardown on param_data.rs (addressed), and the malformed-UTF-8 undercount on param_convert.rs (documented, deferred under AB#47590). I'm leaving every thread unresolved per the sweep rule.

[Shiwani Gupta (shiwanigupta0809)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main and reconciling the two DAE conflicts in 937dfd3.

[Shiwani Gupta (shiwanigupta0809)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved by merging the current origin/main and reconciling all four ODBC execution-flow conflicts in 2e94894.

[David Engel (David-Engel)]

Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Summary

Reviewed at head 1241563c against merge base e72e799e (14 files, ~3.1k insertions). This is the fifth automated pass on this PR; the earlier rounds' threads (the fit pad-split misalignment, the SQLPutData buffered-branch claim, the NeedData/restore_plan restore, the failed-open teardown, the odd-SQL_C_WCHAR-split carry) are all resolved and I re-checked each against the current code rather than re-filing them. The design still reads well: keying stream-vs-buffer off IsPartialLenType, narrowing only the @params declaration while the body stays PLP, and narrowing only when the bound is enforceable are the right calls, and the unit_carry fix is correct — fit_chunk joins the held-back byte to the front of the next chunk, and advance() resets DaeProgress wholesale so nothing leaks between parameters.

One blocking item: the branch does not compile under --cfg fuzzing, which is what the two red Linux jobs are reporting. Two non-blocking suggestions.

Verification

• Read the full diff plus the surrounding unchanged code the new paths depend on: build_named_params, park_dae_client / park_deferred_dae, unwind_dae, take_dae, sql_family, convert_character_sql / trim_blank_overflow, variable_length / fixed_length, parameter_column_size_is_valid, and the conversion_matrix rows that decide which pairings can reach dae_plan at all.
• cargo nextest run -p mssqlodbc --lib --no-fail-fast → 1472 passed, 0 failed, so the cargo btest checklist box holds on this platform.
• RUSTFLAGS="--cfg fuzzing" cargo check -p mssqlodbc --lib → fails, reproducing the ADO failure exactly (see Blocking 1).
• Traced the deferred sequencing by hand for [buffered, streamed, streamed], [streamed, buffered], and the single-buffered case; cursor/token alignment against the TDS layer's NeedData holds in all three, and buffered_phase_complete's !rest.is_empty() guard is what keeps the last-parameter case on run_deferred_execute.
• Checked one thing I expected to be a regression and it is not: SQL_LONGVARCHAR with ColumnSize = 0 would give DaeBound::Utf16Units(0) and 22001 on the first byte, but parameter_column_size_is_valid bounds the long types at 1..=SQL_PREC_TEXTIMAGE, so bind rejects it with HY104 first. varchar/varbinary correctly keep 0 as the max spelling via variable_length → no limit. No finding.

---

Blocking

1. mssql-odbc/src/fuzz_support.rs:37 — orphaned import of the deleted transcode_dae_bytes breaks the --cfg fuzzing build

This PR removes transcode_dae_bytes from param_convert.rs, and the merge of origin/main at 2e948940 brought in #511's fuzz targets, which still import and call it. fuzz_support is behind #[cfg(fuzzing)] (lib.rs:17), so cargo bfmt / cargo bclippy / cargo btest never compile it — which is why the checklist boxes are green locally and the Linux jobs are red.

Reproduced in this worktree:

$ RUSTFLAGS="--cfg fuzzing" cargo check -p mssqlodbc --lib
error[E0432]: unresolved import `crate::conversion::param_convert::transcode_dae_bytes`
  --> mssql-odbc\src\fuzz_support.rs:37:60
   |
37 | use crate::conversion::param_convert::{bound_param_to_rpc, transcode_dae_bytes};
   |                                                            ^^^^^^^^^^^^^^^^^^^ no `transcode_dae_bytes` in `conversion::param_convert`
error: could not compile `mssqlodbc` (lib) due to 1 previous error

Same error in build 174072, task Build in Container, on both Build Linux and Build Linux ARM.

The natural fix is to re-point fuzz_transcode_dae_bytes (fuzz_support.rs:89) at the successor API rather than deleting the target — and it gets better coverage than the old one did, because the interesting new surface is exactly the state carried across calls:

let transcode = DaeTranscode::new(c_type, sql_type, collation);
let mut carry = Vec::new();
// Split the input so the carry path is exercised, not just a single whole value.
let (head, tail) = input.split_at(input.len() / 2);
let _ = transcode.push(&mut carry, head);
let _ = transcode.push(&mut carry, tail);
let _ = transcode.finish(&mut carry);

DaeLengthLimit::fit_chunk is worth folding in for the same reason — it is the other new function that carries bytes between calls, and it is the one that returns 22001. Whatever you pick, please run RUSTFLAGS="--cfg fuzzing" cargo check -p mssqlodbc --lib before the next push; the standard checklist commands cannot see this.

---

Suggestion

1. mssql-odbc/src/api/put_data.rs — the untranscoded stream now copies every chunk, and the comment above it says it does not

// A transcoded stream converts on the way out, holding back a character
// that this chunk ended part-way through. An untranscoded one is already
// the wire's bytes, so it is forwarded borrowed.

It isn't forwarded borrowed. Both arms end in an owned buffer:

• limit: None → fitted_owned = chunk.to_vec()
• limit: Some(_) → fit_chunk returns kept.to_vec(), including its unit <= 1 fast path
• then None => Cow::Owned(fitted_owned)

On e72e799e the same path was client.write_streamed_chunk(chunk) straight off the from_raw_parts slice, with no allocation at all. So the canonical SQL_C_BINARY → varbinary(max) LOB case — the one data-at-execution exists for — picks up one full-chunk allocation and memcpy per SQLPutData that it did not have before. The earlier thread on the redundant second clone() fixed the double copy; this is the remaining single one.

It's avoidable without disturbing the carry logic: hoist let chunk: &[u8] above the block (it borrows data_ptr, not stmt_state, so it already outlives the guard) and take Cow::Borrowed(chunk) when limit.is_none() && transcode.is_none(). Having fit_chunk return Cow<'a, [u8]> — borrowed on the unit <= 1 fast path and whenever nothing was trimmed — would cover the bounded-but-not-overflowing case too. Non-blocking: it is an allocation-shape regression on a path whose cost is dominated by the socket write, not a correctness problem. Either fix it or correct the comment, but please don't leave the comment claiming the property it lost.

2. param_data.rs / exec_common.rs — the new fractional_truncated DAE plumbing is unreachable, and its test builds a binding dae_plan refuses

1241563c threads ConvOk::Truncated out of buffered_dae_to_rpc through rebuild_deferred_params, DaeState::fractional_truncated, open_deferred_rpc and run_deferred_execute so a deferred close can report 01S07. As far as I can tell nothing can ever set it:

• ConvOk::Truncated on the parameter-build side is produced only by decimal_from_numeric (param_convert.rs:1418, :1424); decimal_from_text converts its own Truncated into Err(StringTruncation) at :1316.
• decimal_from_numeric is reached only from (AppValue::Numeric(_), SqlFamily::Decimal) at :250, and AppValue::Numeric is produced only for SQL_C_NUMERIC (param_buffer.rs:191).
• dae_plan rejects any C type outside {SQL_C_CHAR, SQL_C_WCHAR, SQL_C_BINARY} with UnsupportedCType (param_convert.rs:616), and build_named_params turns that into HYC00 at SQLExecute. So a DaeParam whose binding.c_type is SQL_C_NUMERIC cannot exist outside a test.

rebuild_deferred_params_reports_fractional_truncation sets param.binding.c_type = SQL_C_NUMERIC directly on a hand-built DaeParam, which is the state dae_plan exists to prevent. The test does exercise the propagation, so it is not vacuous — but it pins a configuration production cannot reach, and a reader will come away believing 01S07 can now surface from a DAE close.

Both dispositions are defensible; what I'd like is for the code to say which one it is. Either drop the plumbing and restore a version of the comment the commit removed (which was correct for a different reason), or keep it as a guard against a future C type widening and add an assertion that pins today's unreachability — assert!(dae_plan(SQL_C_NUMERIC, SQL_DECIMAL).is_err()) next to the new test would do it, and would fail loudly the day the gate widens.

---

Nit

• MixedDataAtExecutionStreamsThePlpParameter (execute_test.cpp) asserts bind order and the round-tripped values, but nothing in it distinguishes "streamed" from "buffered" — the same assertions pass if both parameters are collected. The name promises more than the test checks. The unit test buffered_phase_completes_only_when_remaining_params_stream is what actually guards the decision; a pointer to it in the comment would stop someone trusting the e2e name later.
• DaeLengthLimit::finish takes &self and never reads it; it is std::mem::take(carry). Fine as a symmetry with DaeTranscode::finish, just noting it.

---

CI

gh pr checks 494 on 1241563c: mssql-rs Pull request validation (Build Stage Build Linux) and (Build Stage Build Linux ARM) are failing, both on the Build in Container task with Bash exited with code '101'. That is Blocking 1 above, not a flake — I reproduced the identical E0432 locally. Everything else is passing or still pending.

The cargo bfmt / cargo bclippy / cargo btest checklist boxes are not contradicted by CI: none of those three commands set --cfg fuzzing, so they genuinely pass on a tree that this job cannot build.

---

Nice work on the deferred/streaming split and on documenting the bind-order trade-off and the measurement-vs-payload distinction — those are exactly the notes that stop a later change from silently re-litigating them. Fixing the fuzz-support import should be the only thing standing between this and a human review.

[ttk (Theekshna)]

Reviewed at head 1241563c against merge base with origin/main (e72e799e, 14 files, +3137/-666). Unattended sweep — comment only, not an approval.

Category	Count
Blocking	1
Suggestion	0
Nit	0

Scope note: David-Engel's automated reviewer already posted a five-round review at this exact SHA covering the same finding below plus two Suggestions and two Nits on allocation-shape and dead-code paths. I independently reproduced the Blocking item from CI logs rather than trusting that review, and I'm not re-filing its Suggestions/Nits without independently re-deriving them, which the run budget didn't allow this pass — a human should still read that review.

msodbcsql parity (check 1): Spot-checked one of the PR's own citations against the reference tree: ValidatePutDataLength's cbValue &= ~((SQLULEN)1) is at sqlccmd.cpp:10958 in this checkout (PR cites :10931, a ~27-line drift, immaterial) and confirmed it operates on a local SQLLEN cbValue copy, not *pcbValue — the PR's "masks a local copy, not the payload" claim holds exactly as described. No parity gap found in what I checked; did not audit the full DAE table exhaustively given the CI failure below took priority.

Evidence audit (check 6): Re-derived the sqlccmd.cpp masking claim above (holds). Did not re-run the e2e suite (no live SQL Server in this environment) so the "41/41 suites pass against retail 18.6" and "28 DAE e2e tests" claims are unverified by me — flagging as a question, not a finding: worth confirming those e2e runs happened on this exact head rather than an earlier commit in the stack, since two behavior-changing commits (the fit/carry fixes) landed after #488 was superseded.

AI slop / test sufficiency / divergence docs (checks 2, 3, 5): No slop patterns found in a targeted scan of the diff. text/ntext/image max-substitution divergence is tracked at AB#47592 as stated. Test-sufficiency gap is subsumed by the Blocking item below — the fuzz harness is the only path this PR leaves untested, and it doesn't build.

---

[Blocking] mssql-odbc/src/fuzz_support.rs:37 — orphaned import breaks the --cfg fuzzing build, and both Linux CI jobs are currently red because of it

transcode_dae_bytes was removed from param_convert.rs in favor of the new DaeTranscode struct, but fuzz_support.rs:37's use crate::conversion::param_convert::{bound_param_to_rpc, transcode_dae_bytes}; and fuzz_support.rs:111's call site were never updated. Confirmed directly from the ADO build log (build 174072, Build in Container task, both Build Linux and Build Linux ARM jobs, log line ~3147):

error[E0432]: unresolved import `crate::conversion::param_convert::transcode_dae_bytes`
  --> mssql-odbc/src/fuzz_support.rs:37:60
   |
37 | use crate::conversion::param_convert::{bound_param_to_rpc, transcode_dae_bytes};
   |                                                            ^^^^^^^^^^^^^^^^^^^ no `transcode_dae_bytes` in `conversion::param_convert`
error: could not compile `mssqlodbc` (lib) due to 1 previous error

fuzz_support is #[cfg(fuzzing)]-gated (lib.rs:17), which is why cargo bfmt/cargo bclippy/cargo btest — none of which set --cfg fuzzing — genuinely pass locally and the checklist boxes aren't lying. But the ADO pipeline's Linux jobs do build under that cfg, and both are failing on this exact head SHA as of this review. This blocks merge regardless of approvals, since it's a required, currently-failing check with a concrete, reproducible cause rather than an infra flake.