ingest: bind PostgreSQL array literals wide; LFConversion=0 for psqlodbc on Windows - #109
Merged
Merged
Conversation
…dbc on Windows Two Windows-only text defects in the PostgreSQL array-ingest path, found by tests/test_pg_array_ingest.py on a Windows build against psqlodbc 18 (five of its eighteen tests failed there; both are invisible on Linux and macOS): * The array literal each column travels as was bound narrow, SQL_C_CHAR / SQL_LONGVARCHAR, while the row-at-a-time path binds a string parameter wide. On a Unicode driver a narrow parameter is read in the client's ANSI code page, and on Windows psqlodbc re-encodes the UTF-8 bytes to UTF-8 a second time: "héllo wörld" landed as "héllo wörld" (a UTF-8 locale on the other platforms makes the same bind a pass-through). ArrayIngestExecuteBatch now binds the literal the way BindRowParam binds a string -- SQL_C_WCHAR / SQL_WLONGVARCHAR, converted with the same UTF-8-to-SQLWCHAR helper, so the iODBC four-byte SQLWCHAR case is covered -- under the same condition (wchar_as_utf8 / narrow_params keep the drivers whose narrow path is UTF-8 on it). One SQLWCHAR buffer per column, reused across executes like the narrow one. * psqlodbc's LFConversion keyword defaults to 1 on Windows alone (0 on every other platform), and at 1 the driver rewrites every LF in a fetched text value to CR LF -- "line1\nline2" read back as "line1\r\nline2", bytes the server never stored. The connection-keyword auto-tuning now appends LFConversion=0 on Windows for psqlodbc (recognised by its Driver= value, or by UseDeclareFetch behind a DSN) when the caller has not set it; the caller's own value still wins. The rule that nothing changing a query's result is ever set stands: 0 is the value that returns what Linux and macOS already do. docs/how-it-works/connection-keywords.md records it. Verification on Windows (MSVC 19.44 x64 Release, psqlodbc 18.00.0002, PostgreSQL 16): ctest 7/7; tests/test_pg_array_ingest.py 18 passed (13 before); tests/ 183 passed, 26 skipped, 6 failed -- the same six as origin/main on this platform (native-driver delegation and plug-and-play install, unrelated); compat matrix postgres, sqlite and duckdb PASS; python/tests 22 passed. Linux and macOS still need a run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EYWW5uA51Q1cH7wR14EG2J
…mes only for LFConversion=0 rust/sync-csrc.sh had not been run, so the crate's copy of odbc_bind.c and odbc_driver.c lagged src/ and the "Bundled C sources match src/" check failed. The Windows-only LFConversion=0 addition now recognises psqlodbc by "psqlodbc", "podbc", "PostgreSQL Unicode" or "PostgreSQL ANSI" in the Driver= value (or the psqlodbc-only UseDeclareFetch keyword behind a DSN), not by the bare word "postgresql": other vendors' PostgreSQL drivers carry that word too, and a keyword they do not know is theirs to refuse. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YG6wFApZpg2xe61EmKhaSZ
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the five Windows-only failures in
tests/test_pg_array_ingest.pyfound while verifying #107 on Windows (they are pre-existing there, not #107's: the same five fail againstmain).Cause
Two things, both invisible on Linux and macOS:
ArrayIngestExecuteBatchbound each column's PostgreSQL array literal asSQL_C_CHAR/SQL_LONGVARCHAR, while the row-at-a-time path binds a string parameter wide. A Unicode driver reads a narrow parameter in the client's ANSI code page, and on Windows psqlodbc re-encodes the UTF-8 bytes to UTF-8 a second time, so"héllo wörld"landed as"héllo wörld". On Linux and macOS the UTF-8 locale makes the same bind a pass-through.LFConversiondefaults to 1 on Windows (0 on every other platform,DEFAULT_LFCONVERSIONin itsdlg_specific.h). At 1 the driver rewrites every LF in a fetched text value to CR LF:"line1\nline2"read back as"line1\r\nline2", bytes the server never stored (checked withoctet_length/encode(...,'hex')on the tables: LF only). Not specific to the array path, it just happens to be the only test with a newline fixture.Fix
src/odbc_bind.c: the literal now travels the wayBindRowParamsends a string:SQL_C_WCHAR/SQL_WLONGVARCHAR, converted with the existing UTF-8-to-SQLWCHAR helper (so the iODBC four-byteSQLWCHARcase is covered bywide_utf16_pairsas elsewhere), under the samewchar_as_utf8/narrow_paramscondition, so drivers whose narrow path is UTF-8 keep it. OneSQLWCHARbuffer per column, reused across executes like the narrow one.src/odbc_driver.c: the connection-keyword auto-tuning appendsLFConversion=0on Windows only, for psqlodbc (recognised by itsDriver=value, or byUseDeclareFetchbehind a DSN), when the caller has not setLFConversion. The caller's own value still wins, andadbc.odbc.tune=falseturns it off with the rest. The rule that nothing changing a query's result is ever set stands: 0 is the value that returns what Linux and macOS already do. Comment block anddocs/how-it-works/connection-keywords.mdupdated accordingly.No C unit test fits (both halves need a live driver);
tests/test_pg_array_ingest.pyis the regression guard and now passes in full on Windows.Verification
Windows 11 x64, MSVC 19.44 (Visual Studio 17 2022), Release; psqlodbc 18.00.0002 ("PostgreSQL Unicode(x64)"); PostgreSQL 16 in Docker; Python 3.12, adbc_driver_manager 1.12.0, pyarrow 25.0.1.
tests/test_pg_array_ingest.pytests/(minustests/validation, needs a package not installed here)tests/compat/test_matrix.py postgres/sqlite/duckdbpython/testsctest10 / 10 (smoke skipped). PostgreSQL 16.15 (scratch cluster) via psqlodbc 18.00.0004:tests/test_pg_array_ingest.py18 / 18 (also 18 / 18 onmainthere: the wide bind is a no-op on a UTF-8 locale, as expected). SQLiteODBC 0.99991:tests/test_sqlite.py9 / 9.test_matrix.py postgresPASS,sqlitePASS. A local merge of #107 + #109 onmain(not pushed) builds clean,ctest10 / 10,tests/test_postgres.py+tests/test_pg_array_ingest.py34 / 34.ctest10 / 10. PostgreSQL 16.15 (compat compose) via psqlodbc 16.00.0000:tests/test_pg_array_ingest.py18 / 18;test_matrix.py postgresPASS,sqlitePASS. A local merge of #107 + #109 builds clean:ctest10 / 10,tests/test_postgres.py+tests/test_pg_array_ingest.py34 / 34. Second commit refreshesrust/csrcand narrows theDriver=match to psqlodbc's registered names.The six
tests/failures are the same set onmainand on this branch: fourtest_delegate.pycases (no native driver forwarding on Windows, IM002 wording) and the twotest_plug_and_play.pyinstall flows (multi-configcmake --install,install.shunder WSL). Unrelated to this change.Linux and macOS still need a run of
tests/test_pg_array_ingest.pyandtests/test_postgres.py: the wide bind is the only half that changes behaviour there (theLFConversionhalf is#if defined(_WIN32)), and it should be a no-op since the row path already binds the same way.