Repository navigation
FIX: Avoid unsupported isolation reset when closing pooled Synapse connections - #889
Conversation
Remove the redundant close-time READ COMMITTED statement while preserving rollback, session reset and existing checkout behavior. Add focused native and integration regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
PR Performance Report✅ No regression detectedNo consistent slowdowns detected across all 2 environments. 0 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS Coverage: 2 of 2 environments completed. Advisory result; does not block merging. Performance diagnosticsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. No affected phases or call-count changes were recorded. All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build and measurement detailsPR head:
A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. Raw samples and logs are attached to the ADO run as |
Jahnvi Thakkar (jahnvi480)
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The test uses unsupported std::char_traits with unixODBC headers, preventing pool_sanitation_tests from compiling on libc++ 19+ configurations.
1 open issue
What changed in this PR
This PR addresses the pooled-connection close regression affecting Azure Synapse dedicated SQL pools by removing an unsupported isolation-level reset from the close-time rollback batch.
Changes:
- Removes the redundant
READ COMMITTEDstatement while preserving rollback and checkout-time isolation restoration. - Clarifies the session-reset state comment without changing its behavior.
- Adds native regression coverage and a subprocess-isolated pooled autocommit close-after-query test.
| File | Description |
|---|---|
tests/test_009_pooling.py |
Adds repeated pooled autocommit query-and-close coverage. |
tests/native/pool_sanitation.cpp |
Adds a mock rejection of READ COMMITTED and a close-time regression test. |
mssql_python/pybind/connection/connection.h |
Clarifies that checkout remains responsible for restoring isolation. |
mssql_python/pybind/connection/connection.cpp |
Removes the isolation reset from the pool-return rollback batch. |
Review effort: Balanced
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 65.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python._native_fetch.py: 77.4%
mssql_python.pybind.ddbc_bindings.cpp: 80.5%
mssql_python.pybind.connection.connection_pool.cpp: 81.3%
mssql_python.pybind.row_mapping_cache.hpp: 81.5%
mssql_python.pybind.connection.connection.cpp: 82.5%
mssql_python.logging.py: 86.2%🔗 Quick Links
|
|
Reviewed revision The production change removes the unsupported close-time isolation statement while preserving rollback, deferred session reset, cleanup-error propagation, pool-capacity recovery, and checkout isolation restoration with discard/reconnect fallback. No additional production defects were identified by source inspection. Recommendation: Request changes for the existing unresolved native-test portability issue at Evidence: https://releases.llvm.org/19.1.0/projects/libcxx/docs/ReleaseNotes.html#deprecations-and-removals No local compilation or database execution was performed during this review. Live Synapse verification remains outstanding, as disclosed in the PR. |
Count null-terminated SQLWCHAR elements directly instead of depending on the generic std::char_traits template removed in libc++ 19. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Live Synapse dedicated-pool validation remains outstanding, and SQL Server tests and native mocks do not establish deferred-reset and checkout-fallback behavior on the affected engine.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Live Synapse dedicated-pool validation remains outstanding, and SQL Server and mocked ODBC checks do not verify the affected engine’s rollback-only reset and repeated pooled close/reconnect behavior.
No open issues 🎉
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Live Azure Synapse dedicated-pool validation of close-after-query and subsequent checkout/reconnect behavior remains outstanding; reported checks cover only SQL Server and a limited native mock.
No open issues 🎉
Review effort: Balanced
Sumit Sarabhai (sumitmsft)
left a comment
Sumit Sarabhai (sumitmsft)
left a comment
There was a problem hiding this comment.
Reviewed the PR for correctness, security, reliability, performance, test coverage, repository conventions, and applicable architecture and design specifications. No actionable issues were identified. The implementation is consistent with repository standards and the applicable approved design requirements.
Recommendation: Approve
There was a problem hiding this comment.
🔵 Needs a closer look
Validation covers only SQL Server and a native mock, leaving close-after-query, deferred session reset, and checkout discard/reconnect behavior unverified on the affected Synapse dedicated pool.
No open issues 🎉
Review effort: Balanced
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Work Item / Issue Reference
Summary
Remove the redundant READ COMMITTED statement from the autocommit-on pool-return rollback batch. Azure Synapse dedicated SQL pools reject this statement, causing the 1.17.0 Connection.close() regression.
Preserve rollback, deferred session reset, cleanup-error propagation and pool-capacity recovery. Checkout isolation restoration and its existing discard/reconnect fallback remain unchanged. No engine-detection queries, new connection state or public API changes are introduced.
Add focused native coverage that rejects READ COMMITTED in the close batch and a pooled autocommit close-after-query integration case.
Validation
cmake --build mssql_python\pybind\build\x64\py313 --config Release --parallel 4 -- /verbosity:quiet /nologo- passed; Windows x64 Release extension and native test executable built.ctest --test-dir mssql_python\pybind\build\x64\py313 -C Release --output-on-failure- passed..venv\Scripts\python.exe -m pytest -q tests\test_003_connection.py tests\test_006_exceptions.py tests\test_009_pooling.py -k 'close or isolation or explicit or noexec or positive_login or sanitation or generation or autocommit or attrs_before' --tb=short- 43 passed, 6 skipped, 294 deselected against the available SQL Server..venv\Scripts\python.exe -m pre_commit run black-check --all-files --hook-stage pre-push- passed.Live Synapse dedicated-pool verification and the full test suite were not run.