PERF: Optimize pooled connection return while preserving transaction safety

[Sumit Sarabhai (sumitmsft)]

AB#48076

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.

• 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.
• 43 deterministic native sanitation cases passed; CTest passed.
• 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.

[github-actions]

PR Performance Report

Performance could not be assessed.

Build provenance validation failed. No result is available.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical cleanup, destructor, handle-lifecycle, and disconnect-synchronization issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 1

Open (5)

• Native connection handle remains attached after cleanup failure · New
• Connection destructor can terminate on disconnect failure · New
• Concurrent statement allocation races with connection disconnect · New
• SQLFreeHandle failure incorrectly clears live handle · New
• Returned connection can enter a replacement pool generation · New

What changed in this PR

Reverts PR #777 to restore prior pooled-connection behavior while retaining result-metadata invalidation.

Changes:

• Removes pooled transaction sanitization and related regression tests.
• Restores previous native connection, handle cleanup, and pool-return behavior.
• Removes the reverted changelog entry.

File	Summary
tests/​test_009_pooling.py	Removes pooling and native lifecycle regression tests.
tests/​test_006_exceptions.py	Removes close-failure tests.
mssql_python/​pybind/​ddbc_bindings.h	Removes cleanup-state APIs.
mssql_python/​pybind/​ddbc_bindings.cpp	Restores earlier handle-management behavior.
mssql_python/​pybind/​connection/​connection.h	Removes pool-sanitation interfaces.
mssql_python/​pybind/​connection/​connection.cpp	Restores prior disconnect and pool-return behavior.
mssql_python/​pybind/​connection/​connection_pool.h	Removes discard/origin-pool APIs.
mssql_python/​pybind/​connection/​connection_pool.cpp	Restores key-based pool returns.
mssql_python/​connection.py	Restores the previous close flow.
CHANGELOG.md	Removes the reverted fix entry.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 80%

🎯 Overall Coverage 85%

📈 Total Lines Covered: 9555 out of 11238 📁 Project: mssql-python

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

• mssql_python/connection.py (100%)
• mssql_python/pybind/connection/connection.cpp (80.9%): Missing lines 291,310,331,354-355,375-376,482-486,490-491,658,711-712,723,775-780,804-805,952,1009,1011-1012
• mssql_python/pybind/ddbc_bindings.cpp (80.0%): Missing lines 1593-1595,1678

Summary

• Total: 178 lines
• Missing: 34 lines
• Coverage: 80%

mssql_python/pybind/connection/connection.cpp

Lines 287-295

  287 
  288 void Connection::commit() {
  289     PERF_TIMER("Connection::commit");
  290     _poolClean = false;
! 291     _poolSessionReset = false;
  292     if (!_dbcHandle) {
  293         ThrowStdException("Connection handle not allocated");
  294     }
  295     updateLastUsed();

---

Lines 306-314

  306 
  307 void Connection::rollback() {
  308     PERF_TIMER("Connection::rollback");
  309     _poolClean = false;
! 310     _poolSessionReset = false;
  311     if (!_dbcHandle) {
  312         ThrowStdException("Connection handle not allocated");
  313     }
  314     updateLastUsed();

---

Lines 327-335

  327     PERF_TIMER("Connection::setAutocommit");
  328     if (!enable) {
  329         _poolClean = false;
  330         _poolSessionReset = false;
! 331     }
  332     if (!_dbcHandle) {
  333         ThrowStdException("Connection handle not allocated");
  334     }
  335     clearResultMetadata();

---

Lines 350-359

  350         py::gil_scoped_release release;
  351         ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT,
  352                                     reinterpret_cast<SQLPOINTER>(static_cast<SQLULEN>(value)), 0);
  353     }
! 354     if (!SQL_SUCCEEDED(ret)) {
! 355         _poolClean = false;
  356         checkError(ret);
  357     }
  358     if (value == SQL_AUTOCOMMIT_ON) {
  359         LOG("Autocommit enabled");

---

Lines 371-380

  371     SQLINTEGER value;
  372     SQLINTEGER string_length;
  373     SQLRETURN ret = SQLGetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_AUTOCOMMIT, &value,
  374                                           sizeof(value), &string_length);
! 375     if (!SQL_SUCCEEDED(ret)) {
! 376         _poolClean = false;
  377         checkError(ret);
  378     }
  379     return value == SQL_AUTOCOMMIT_ON;
  380 }

---

Lines 478-495

  478     }
  479 
  480     if (py::isinstance<py::int_>(value)) {
  481         // Get the integer value
! 482         int64_t longValue;
! 483         try {
! 484             longValue = value.cast<int64_t>();
! 485         } catch (const py::cast_error&) {
! 486             _poolProofDisabled = true;
  487             throw;
  488         } catch (const py::error_already_set&) {
  489             _poolProofDisabled = true;
! 490             throw;
! 491         }
  492         if (scalarLoginTimeout &&
  493             (longValue < 0 ||
  494              static_cast<uint64_t>(longValue) > std::numeric_limits<SQLUINTEGER>::max())) {
  495             _poolProofDisabled = true;

---

Lines 654-662

  654 bool Connection::reset() {
  655     if (!_dbcHandle) {
  656         ThrowStdException("Connection handle not allocated");
  657     }
! 658     if (_cleanupState->childFreeFailed) {
  659         _poolClean = false;
  660         _poolSessionReset = false;
  661         LOG("Cannot reuse connection after a statement handle failed to free");
  662         return false;

---

Lines 707-716

  707         LOG("Failed to reset transaction isolation level (ret=%d). Marking as dead.", ret);
  708         return false;
  709     }
  710 
! 711     // A retained child can finish cleanup while either attribute call releases the GIL.
! 712     if (_cleanupState->childFreeFailed) {
  713         _poolClean = false;
  714         _poolSessionReset = false;
  715         LOG("Cannot reuse connection after a statement handle failed to free");
  716         return false;

---

Lines 719-727

  719     return true;
  720 }
  721 
  722 void Connection::prepareForPool() {
! 723     PERF_TIMER("Connection::prepareForPool");
  724     if (!_dbcHandle) {
  725         ThrowStdException("Connection handle not allocated");
  726     }
  727     if (_cleanupState->childFreeFailed) {

---

Lines 771-784

  771                                             reinterpretU16stringAsSqlWChar(rollbackQuery), SQL_NTS);
  772                     if (!SQL_SUCCEEDED(ret)) {
  773                         ErrorInfo error = SQLReadError(SQL_HANDLE_STMT, statement, ret);
  774                         statementError = error.sqlState.length() == 5
! 775                             ? "SQLSTATE:" + error.sqlState + ":" + error.ddbcErrorMsg
! 776                             : error.ddbcErrorMsg;
! 777                     }
! 778                     SQLRETURN freeRet = SQLFreeHandle_ptr(SQL_HANDLE_STMT, statement);
! 779                     if (SQL_SUCCEEDED(ret) && !SQL_SUCCEEDED(freeRet)) {
! 780                         ErrorInfo error = SQLReadError(SQL_HANDLE_STMT, statement, freeRet);
  781                         statementError = error.sqlState.length() == 5
  782                             ? "SQLSTATE:" + error.sqlState + ":" + error.ddbcErrorMsg
  783                             : error.ddbcErrorMsg;
  784                         ret = freeRet;

---

Lines 800-809

  800     }
  801     if (!statementError.empty()) {
  802         ThrowStdException(statementError);
  803     }
! 804     checkError(ret);
! 805     // A weak_ptr expires before its destructor finishes. Read the count before
  806     // the failure latch so a completed destructor cannot hide a failed native free.
  807     const bool noChildren = _cleanupState->childStatementCount == 0;
  808     if (_cleanupState->childFreeFailed) {
  809         ThrowStdException("Cannot pool connection after a statement handle failed to free");

---

Lines 948-956

  948         // SQLDisconnect failure using the existing connection and children.
  949         if (!_usePool && !rollbackBeforeDisconnect) {
  950             throw;
  951         }
! 952         // Never retain a connection whose transaction state could not be
  953         // sanitized. Release capacity and preserve the original cleanup error.
  954         try {
  955             ConnectionPoolManager::getInstance().discardConnection(_originPool, _conn);
  956         } catch (...) {

---

Lines 1005-1016

  1005     return conn->allocStatementHandle();
  1006 }
  1007 
  1008 py::object Connection::getInfo(SQLUSMALLINT infoType) const {
! 1009     _poolClean = false;
  1010     _poolSessionReset = false;
! 1011     if (infoType == SQL_DRIVER_HDBC || infoType == SQL_DRIVER_HENV ||
! 1012         infoType == SQL_DRIVER_HSTMT || infoType == SQL_DRIVER_HLIB) {
  1013         _poolProofDisabled = true;
  1014     }
  1015     if (!_dbcHandle) {
  1016         ThrowStdException("Connection handle not allocated");

---

mssql_python/pybind/ddbc_bindings.cpp

Lines 1589-1599

  1589 
  1590 // SqlHandle definition
  1591 SqlHandle::SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle,
  1592                      std::shared_ptr<ConnectionCleanupState> cleanupState)
! 1593     : _type(type), _handle(rawHandle), _cleanupState(std::move(cleanupState)) {
! 1594     if (_type == SQL_HANDLE_STMT && _cleanupState) {
! 1595         ++_cleanupState->childStatementCount;
  1596     }
  1597 }
  1598 
  1599 SqlHandle::~SqlHandle() {

---

Lines 1674-1682

  1674         if (_implicitly_freed || (_cleanupState && _cleanupState->disconnected)) {
  1675             _handle = nullptr;
  1676             return SQL_SUCCESS;
  1677         }
! 1678         SQLRETURN ret = SQLFreeHandle_ptr ? SQLFreeHandle_ptr(_type, _handle) : SQL_INVALID_HANDLE;
  1679         if (SQL_SUCCEEDED(ret)) {
  1680             _handle = nullptr;
  1681         } else if (_type == SQL_HANDLE_STMT && _cleanupState) {
  1682             _cleanupState->childFreeFailed = true;

---

---

📋 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: 62.6%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.connection.connection.cpp: 81.4%
mssql_python.pybind.connection.connection_pool.cpp: 83.5%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.fetch_temporal.hpp: 92.1%
mssql_python.cursor.py: 92.5%

---

🔗 Quick Links

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

[Copilot]

Copilot review overview

🟡 Changes recommended

Native tests are not part of standard validation, and exception tests no longer cover the failure paths their names describe.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Opt-in native tests are excluded from standard CI · New
• Rollback failure test no longer exercises rollback failure · New

Resolved since last review (5)

• SQLFreeHandle failure incorrectly clears live handle
• Concurrent statement allocation races with connection disconnect
• Connection destructor can terminate on disconnect failure
• Native connection handle remains attached after cleanup failure
• Returned connection can enter a replacement pool generation

[Copilot]

Copilot review overview

🔵 Needs a closer look

Enable the native sanitation regression suite in normal CI.

Review effort: Lite
Findings: 2

Open (2)

• Rollback failure test no longer exercises rollback failure
• Opt-in native tests are excluded from standard CI

[Copilot]

Copilot review overview

🔵 Needs a closer look

Address the manual-lease state bug, restore Python-boundary failure coverage, and run native sanitation tests in CI.

Review effort: Lite
Findings: 2

Open (2)

• Rollback failure test no longer exercises rollback failure
• Opt-in native tests are excluded from standard CI

Previously missed (1)

In code that hasn't changed since last review

Autocommit read failure coverage was removed

tests/​test_006_exceptions.py:304

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.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved raw-handle cleanup and validation/test consistency issues must be addressed.

Review effort: Lite
Findings: 1 · 2

Open (3)

• Untracked failed statement cleanup leaks discarded connections · New
• Rollback failure test no longer exercises rollback failure
• Opt-in native tests are excluded from standard CI

[Copilot]

🔵 Needs a closer look

Moderate cleanup ownership and session-reset state issues remain unresolved.

3 open findings

• Untracked failed statement cleanup leaks discarded connections
• Rollback failure test no longer exercises rollback failure
• Opt-in native tests are excluded from standard CI

🧠 Review effort: Lite

[Copilot]

🟡 Changes recommended

Set _poolSessionReset after successful reset() sanitation to avoid repeated checkout cleanup.

4 open findings

• Untracked failed statement cleanup leaks discarded connections
• Set pool session reset flag after successful reset · New
• Rollback failure test no longer exercises rollback failure
• Opt-in native tests are excluded from standard CI

🧠 Review effort: Lite

[Copilot]

🔵 Needs a closer look

A critical native handle-lifetime cleanup issue remains unresolved, with broader validation still pending.

1 open finding

• Failed child frees can incorrectly mark the connection clean · New

4 resolved since last review

• Untracked failed statement cleanup leaks discarded connections
• Set pool session reset flag after successful reset
• Rollback failure test no longer exercises rollback failure
• Opt-in native tests are excluded from standard CI

🧠 Review effort: Lite

[Copilot]

🔵 Needs a closer look

Unresolved native handle-lifetime and rollback-failure coverage concerns require changes and human review.

1 open finding

• Failed child frees can incorrectly mark the connection clean

🧠 Review effort: Lite

[Copilot]

🔵 Needs a closer look

A moderate failure-path coverage gap remains, diagnostic behavior needs documentation and validation, and broader validation is pending.

0 open findings

1 resolved since last review

• Failed child frees can incorrectly mark the connection clean

🧠 Review effort: Lite