Skip to content

Honor PDO::PARAM_STR_CHAR in real prepared statements - #1686

Open
Ethan Setnik (esetnik) wants to merge 1 commit into
microsoft:devfrom
esetnik:fix/real-prepare-param-str-char
Open

Ethan Setnik (esetnik) wants to merge 1 commit into
microsoft:devfrom
esetnik:fix/real-prepare-param-str-char

Conversation

@esetnik

Copy link
Copy Markdown

Addresses #1587. Related: #1540.

Problem

In real prepared statements, PDO_SQLSRV declares every UTF-8 string parameter as nvarchar and deliberately ignores PDO::PARAM_STR_CHAR and PDO::ATTR_DEFAULT_STR_PARAM (pdo_stmt.cpp logs "set but is ignored"), although emulated prepared statements and PDO::quote() honor both. Comparing an nvarchar parameter with a varchar column makes SQL Server convert the column (CONVERT_IMPLICIT). Under a SQL collation such as the default SQL_Latin1_General_CP1_CI_AS, that rules out a seek on an index keyed on the column, and under any collation it degrades the optimizer's cardinality estimates for the predicate.

Change

For an input PDO::PARAM_STR string parameter whose effective encoding is UTF-8, declare it as SQL_VARCHAR when:

  • the binding carries PDO::PARAM_STR_CHAR, or
  • PDO::ATTR_DEFAULT_STR_PARAM is PDO::PARAM_STR_CHAR and the binding does not carry PDO::PARAM_STR_NATL.

Only the declared SQL type changes. The value is still converted to UTF-16 and sent as SQL_C_WCHAR, and the ODBC driver converts it to the server code page, which is the same semantics as the emulated path. Size derivation is unchanged (varchar(4000), or varchar(max) above 8000 bytes of UTF-16), so long values are not truncated (covered by the new test).

Unchanged: the default (neither option set), SQLSRV_ENCODING_SYSTEM and SQLSRV_ENCODING_BINARY parameters, output and input/output parameters, LOBs and streams, emulated prepares, Always Encrypted (the core still replaces the type with the encrypted column's metadata), and the SQLSRV extension (no shared code changed).

Why the standard PDO flags

They need no new API, and they reach PDOStatement::bindValue(), which has no $driver_options parameter. Laravel's query builder and Doctrine DBAL's PDO driver bind every value with bindValue(), so a bindParam() driver option (as discussed in #1540) cannot reach them, while PDO::ATTR_DEFAULT_STR_PARAM can be set from their connection configuration. Doctrine DBAL also already has ParameterType::ASCII, which its PDO driver currently maps to plain PDO::PARAM_STR; with this change it could map it to PDO::PARAM_STR | PDO::PARAM_STR_CHAR and get varchar binding. This complements per-parameter driver options rather than competing with them.

Behavior change

Code that already sets PDO::PARAM_STR_CHAR, or PDO::ATTR_DEFAULT_STR_PARAM to PDO::PARAM_STR_CHAR, now gets varchar binding in real prepares too, so characters the server code page cannot represent are replaced, exactly as they already are with emulated prepares. pdo_1018_real_prepare_natl_char.phpt locked in the old behavior; its UTF-8 case 1 now expects the replaced value. With a _UTF8 collation (the #1587 scenario) the code page is UTF-8 and the round trip is lossless; see below. If you would prefer this behind an opt-in connection attribute, I am happy to add one.

Evidence

Ubuntu 24.04 (amd64), PHP 8.4.26, ODBC Driver 18.7.1.1, unixODBC 2.3.12, SQL Server 2022 16.0.4222.2 (SQL_Latin1_General_CP1_CI_AS). Lookup SELECT id FROM t WHERE code = ? on 200,000 rows with an index on code varchar(20), actual plans via SET STATISTICS XML ON:

Build Binding CONVERT_IMPLICIT on column Plan Logical reads
dev PARAM_STR yes Index Scan 539
dev PARAM_STR | PARAM_STR_CHAR yes Index Scan 539
this PR PARAM_STR yes Index Scan 539
this PR PARAM_STR | PARAM_STR_CHAR no Index Seek 3

_UTF8 collation (database created with Latin1_General_100_CI_AS_SC_UTF8, varchar(100) column), inserting CJK text plus an emoji: with PARAM_STR | PARAM_STR_CHAR the parameter is declared varchar and the value reads back unchanged.

Tests

  • New pdo_1587_real_prepare_param_str_char.phpt: checks the declared type of each parameter through SQL_VARIANT_PROPERTY(CAST(? AS sql_variant), 'BaseType') for bindValue() and bindParam(), the per-binding flags, both PDO::ATTR_DEFAULT_STR_PARAM values, PARAM_STR_NATL overriding a PARAM_STR_CHAR default, explicit system/binary/UTF-8 parameter encodings, a non-string parameter, and a 9,000-byte value sent in full. It fails on dev (every PARAM_STR_CHAR case reports nvarchar) and passes with this change.
  • Updated pdo_1018_real_prepare_natl_char.phpt as described above.
  • Full pdo_sqlsrv functional suite in the environment above, with this change: 322 passed, 0 failed, 18 skipped (PHP 7-only, secure enclave, Windows-only, Azure Key Vault credentials, the mock TDS server, and one test whose skip check could not connect). On dev, the same run fails only the two tests above.
  • Across repeated runs, the connection resiliency tests (pdo_connection_resiliency*.phpt, pdo_678_conn_resiliency_pooling.phpt) fail intermittently in this emulated amd64 environment on both dev and this branch (TCP Provider: Error code 0x20 after the test kills its session), so I have treated them as environmental.
  • Builds without warnings in pdo_stmt.cpp with -Wall -Wextra. I have not built on Windows or macOS; CI should cover those.

🤖 Generated with Claude Code

PDO_SQLSRV declared every UTF-8 string parameter of a real prepared
statement as nvarchar and ignored PDO::PARAM_STR_CHAR and
PDO::ATTR_DEFAULT_STR_PARAM, which emulated prepared statements and
PDO::quote() already honor. Comparing an nvarchar parameter with a
varchar column makes SQL Server convert the column (CONVERT_IMPLICIT),
which prevents index seeks on it and hides its statistics.

Declare such a parameter as varchar when it is marked
PDO::PARAM_STR_CHAR, or when PDO::ATTR_DEFAULT_STR_PARAM is
PDO::PARAM_STR_CHAR and the binding does not override it with
PDO::PARAM_STR_NATL. The value is still sent as UTF-16 and the ODBC
driver converts it to the server code page, as in the emulated case.
Applications that set neither keep the nvarchar binding.

Addresses microsoft#1587.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@esetnik

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree company="Prodigy EMS"

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant