Skip to content

Verify decimal_from_numeric's precision_scale_explicit persistence across a same-C-type rebind against retail msodbcsql #536

Description

@Theekshna

Follow-up from PR #521 review (#521 (comment), David-Engel).

The question

BoundParam::write_to_records (mssql-odbc/src/params/bound_param.rs) only clears precision_scale_explicit when the C type is actually changing:

let type_changing = apd_record.concise_type != self.c_type;
...
if type_changing {
    apd_record.precision_scale_explicit = false;
}
// values always reset to SQL_C_NUMERIC's type default (38, 0), even on a same-type rebind
if self.c_type == SQL_C_NUMERIC {
    apd_record.precision = SQL_PREC_NUMERIC;
    apd_record.scale = 0;
}

decimal_from_numeric (mssql-odbc/src/conversion/param_convert.rs:955-993) uses that flag to decide whether to trust the APD's (app_precision, app_scale) (fast path — forward the embedded SQL_NUMERIC_STRUCT metadata unchanged) or fall back to the struct's own embedded scale (slow path — rescale to the IPD's declared scale).

Because the flag only clears on a type change, not on every bind, the following sequence takes the fast path even though the app never called SQLSetDescField for the second bind:

  1. SQLBindParameter(ordinal 1, SQL_C_NUMERIC, SQL_DECIMAL, 5, 3, ...), then SQLSetDescFieldW(SQL_DESC_PRECISION, 5) / SQLSetDescFieldW(SQL_DESC_SCALE, 3) — a deliberate, explicit APD write matching the column exactly. precision_scale_explicit becomes true.
  2. A bare rebind at the same ordinal/statement (no SQL_RESET_PARAMS in between): SQLBindParameter(ordinal 1, SQL_C_NUMERIC, SQL_DECIMAL, 38, 0, ...). Since the C type (SQL_C_NUMERIC) hasn't changed, precision_scale_explicit stays true, and the value reset sets (app_precision, app_scale) = (38, 0) — which now coincidentally equals the fresh IPD's (38, 0), so decimal_from_numeric's fast-path gate passes and the embedded struct's metadata is forwarded as-is, unrescaled.

This differs from the same bare (38, 0) bind on a fresh ordinal that was never explicitly bound, which is deliberately tested by NumericStructWithoutDescriptorFieldWritesUsesDefaultApdScale and takes the slow path (rescales using the struct's own embedded scale, truncating 12.345 to "12").

Retail's SetADRecBP re-applies SetTypeDefaults (the same (38, 0) value reset) on every SQLBindParameter call regardless of C-type change, per the existing doc comment in write_to_records. What's unconfirmed is whether retail's fast/slow path decision itself (not just the value reset) is keyed off some analogous "was this ordinal's precision/scale ever explicitly authored" bit that would similarly persist across a same-type rebind, or whether retail's decision is purely a function of the APD's current values with no such memory — in which case this driver's precision_scale_explicit retention would be a genuine divergence for this specific sequence.

What's needed

A dedicated e2e test (mssql-odbc/tests/e2e/tests/param_conversions_test.cpp), run with --compare-with-msodbcsql (see tests/e2e/README.md), that:

  1. Binds ordinal 1 with an explicit SQLSetDescField precision/scale matching the column exactly (forcing the fast path).
  2. Rebinds the same ordinal, same statement, no reset, with a bare (38, 0) SQLBindParameter call and a struct embedding a different, non-(38,0) precision/scale.
  3. Compares the result against retail msodbcsql.

If retail rescales in this case (i.e. does not retain the fast path across the bare rebind), precision_scale_explicit needs to reset on every bind, not just on a C-type change — but note NumericRebindDoesNotInheritAPreviousBindsStaleApdScale (bound_param.rs) already depends on the flag surviving a same-type rebind in a different sub-case (a same-type rebind where the freshly-reset (38, 0) values must still be treated as deliberate rather than default), so the fallback rule in decimal_from_numeric (param_convert.rs:1000) would need to move with it rather than a straight revert.

A quick differential probe against the locally-installed ODBC Driver 18 for SQL Server reference driver during this review surfaced additional complexity in the plain (non-explicit) bare-bind case itself that wasn't fully reconciled with the existing pinned test expectations, so this needs a carefully written e2e test using the existing BindNumericRaw/SQLSetDescFieldW helper patterns already in the suite, not an ad hoc script, to reach a confident answer.

Tracked separately from PR #521 so it doesn't block merge; the PR's existing pinned tests (NumericRebindDoesNotInheritAPreviousBindsStaleApdScale, NumericStructWithoutDescriptorFieldWritesUsesDefaultApdScale) remain valid for the cases they cover.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions