You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
PERF: Optimize pooled connection return while preserving transaction safety - #820
This PR now replaces the earlier revert with a targeted pooling optimization. It retains the transaction, failure-discard, pool-capacity and native handle-lifetime protections from #777.
Skip pool sanitation only when a prior successful rollback/autocommit restoration established clean state and no subsequent statement allocation or uncertain operation invalidated it. A new login or deferred reset alone never establishes clean state.
Retained statement aliases, raw-handle exposure and unknown connection attributes remain conservative. Valid scalar login timeouts such as timeout=30 do not permanently disable the fast path.
Move close-time transaction handling into native code and consolidate required sanitation into one autocommit probe, metadata invalidation and GIL-release scope. Never turn autocommit on after a failed rollback.
Preserve cleanup gates, discard/capacity recovery, pool-generation and shutdown protections. Extend the existing explicit-transaction integration cases and add deterministic native failure/call-count coverage.
Scope and limitations
Used or uncertain connections still execute full transaction sanitation. This does not assume autocommit=True means there cannot be an explicit SQL transaction, and it does not remove safety synchronization to improve timings. End-to-end performance improvements and live SQL behavior remain subject to validation; no blanket regression-elimination claim is made.
Validation
Windows x64 Release extension and native test executable built successfully.
24 targeted Python tests passed using the freshly built extension.
Positive-timeout coverage verifies that timeout 30 reaches physical login and that 100 subsequent unused pooled leases issue no sanitation calls.
Three expanded live-SQL integration cases collected successfully but were not executed locally. Normal cross-platform PR validation and performance evaluation are pending.
This reverts commit 2a86fc1 while preserving subsequent result-metadata changes. Restores the prior pooling behavior; the pooled-transaction correctness issue fixed by #777 will need a replacement fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Unix / SQL Server 2022: Python 3.12.3, x86_64, SQL 16.0.4305.5; 5 paired comparisons and 1 warmup.
Unix / SQL Server 2025: Python 3.12.3, x86_64, SQL 17.0.5035.5; 5 paired comparisons and 1 warmup.
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 profiler-* artifacts.
663boolConnection::reset(unsignedint timeoutSeconds, bool autocommit) {
664if (!_dbcHandle) {
665ThrowStdException("Connection handle not allocated");
666 }
! 667if (_cleanupState->childFreeFailed) {
668 _poolClean = false;
669 _poolSessionReset = false;
670LOG("Cannot reuse connection after a statement handle failed to free");
671returnfalse;
Lines 708-719
708// form (UAF during silent reconnect).709 {
710// Release the GIL around the ODBC call for consistency with the711// other connection-attribute paths; some attributes can block.
! 712 py::gil_scoped_release release;
! 713 ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_RESET_CONNECTION,
! 714 (SQLPOINTER)SQL_RESET_CONNECTION_YES, SQL_IS_INTEGER);
! 715 }
716if (!SQL_SUCCEEDED(ret)) {
717 _poolClean = false;
718LOG("Failed to reset connection (ret=%d). Marking as dead.", ret);
719returnfalse;
Lines 746-755
746LOG("Failed to restore pooled connection timeout (ret=%d). Marking as dead.", ret);
747returnfalse;
748 }
749
! 750// A retained child can finish cleanup while either attribute call releases the GIL.
! 751if (_cleanupState->childFreeFailed) {
752 _poolClean = false;
753 _poolSessionReset = false;
754LOG("Cannot reuse connection after a statement handle failed to free");
755returnfalse;
839 }
840if (!statementError.empty()) {
841ThrowStdException(statementError);
842 }
! 843checkError(ret);
! 844// A weak_ptr expires before its destructor finishes. Read the count before845// the failure latch so a completed destructor cannot hide a failed native free.846constbool noChildren = _cleanupState->childStatementCount == 0;
847if (_cleanupState->childFreeFailed) {
848ThrowStdException("Cannot pool connection after a statement handle failed to free");
Lines 987-995
987// SQLDisconnect failure using the existing connection and children.988if (!_usePool && !rollbackBeforeDisconnect) {
989throw;
990 }
! 991// Never retain a connection whose transaction state could not be992// sanitized. Release capacity and preserve the original cleanup error.993try {
994ConnectionPoolManager::getInstance().discardConnection(_originPool, _conn);
995 } catch (...) {
Replace the PR 777 revert with conservative clean-state tracking and consolidated native sanitation. Used or uncertain connections still roll back before parking; only previously sanitized unused connections can skip cleanup. Preserve scalar login timeouts without trusting arbitrary attributes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sumit Sarabhai (sumitmsft)
changed the title
PERF: Revert pooled connection cleanup from PR #777
PERF: Optimize pooled connection return while preserving transaction safety
Sep 25, 2026
Replace redundant autocommit transitions with a reset-triggered rollback batch and preserve proof so checkout can skip duplicate session reset work.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This replacement likewise removes the simulated get_autocommit failure, so the test named test_close_cleans_up_after_autocommit_read_failure no longer covers that error path. Because the probe now runs inside native close, add a Python-boundary test that can inject/observe a native probe failure, or rename this test so the suite does not falsely claim coverage it no longer provides.
Teach the mixed-diagnostic interceptor the SQL_DIAG_NUMBER header contract while retaining SQLSTATE and message assertions. Run the existing native sanitation CTest target as a blocking job in standard PR validation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The changes span transaction cleanup, handle lifetime, and cross-platform native-test wiring, while live SQL Server/driver failure paths and full platform validation remain unverified and require human or CI confirmation.
Apply the requested autocommit mode during every dirty pooled checkout so attribute failures stay bounded and trigger replacement recovery. Align the Python boundary regression with the merged native attrs contract.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human adjudication is warranted because the high-risk native ODBC pooling and transaction-lifetime changes have only deterministic mock coverage and a Windows build, while live SQL behavior and Linux/macOS validation remain unverified.
Retain the clean-session sanitation fast path while forcing the requested autocommit attribute under the temporary connection timeout. This preserves stale-socket detection and replacement recovery without restoring rollback/reset work.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Native pooling/ODBC lifecycle and transaction-cleanup changes affect rollback, handle destruction, and GIL-less teardown, but live SQL and Windows/macOS validation remain unavailable despite deterministic native and targeted Python coverage.
Use the bounded transaction-isolation setter as the checkout round-trip for proven-clean sessions. This preserves the rollback/reset fast path while detecting unresponsive sockets before reuse.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bring PR #875 dependency changes into the pooling branch before its final validation run.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Native pool sanitation spans high-risk transaction cleanup, handle lifetimes, discard/retry paths, and cross-platform ODBC behavior, while live SQL integration cases and the new Linux CI job were not run for validation.
>
[AB#49814](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/49814)
### Summary
Prepares mssql-python 1.17.0 for release and includes the pooling
optimization merged in #820.
#### Enhancements
- Reduce pooled connection return work while preserving transaction,
cleanup, capacity, and native handle-lifetime protections.
#### Bug Fixes
- Restore bulk copy after upgrades from 1.15.0.
- Bound pooled connection checkout on unresponsive sockets.
- Refresh `rowcount` when moving between results with `nextset()`.
- Make the PEP 249 `Timestamp()` microsecond argument optional.
- Update to `mssql-python-rs 0.4.0` for UTF-8 bulk-copy encoding,
identifier validation, and seven-digit datetime precision.
#### Version
- Bump `mssql_python.__version__` and `setup.py` from 1.16.0 to 1.17.0.
- Replace the PyPI 1.16.0 highlights with the 1.17.0 release inventory.
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Sumit Sarabhai (sumitmsft)
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Summary
This PR now replaces the earlier revert with a targeted pooling optimization. It retains the transaction, failure-discard, pool-capacity and native handle-lifetime protections from #777.
timeout=30do not permanently disable the fast path.Scope and limitations
Used or uncertain connections still execute full transaction sanitation. This does not assume
autocommit=Truemeans there cannot be an explicit SQL transaction, and it does not remove safety synchronization to improve timings. End-to-end performance improvements and live SQL behavior remain subject to validation; no blanket regression-elimination claim is made.Validation