From 18180ba42254a3383befb9d163b50b9ae26b35c0 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 9 Sep 2026 00:14:41 +0530 Subject: [PATCH 1/3] PERF: reuse native parameter bindings across repeated executes Repeated executions of the same prepared statement rebound every parameter from scratch on each call, even when the parameter shape never changed. Give each statement handle a single reusable generation of native input buffers and skip the SQLBindParameter loop when the next execution presents identical binding metadata. The existing detector and binder still run in full on every execution: types are detected, values converted, and validation performed exactly as before. Reuse is gated on identical prepared SQL, parameter count, C and SQL types, column size, scale, direction, effective encoding, and actual buffer byte lengths and indicator addresses; string storage is updated in place only when the size is unchanged, preserving each buffer address and ODBC BufferLength. Scope is deliberately conservative. Input-only integer, boolean, floating-point, and inline text/binary parameters are cached, up to 2,100 parameters and 8,000 bytes per retained text/binary buffer. NULL, data-at-execution, and complex C types fall back to the existing uncached path; decimal overrides formatted to text reuse only with matching precision and scale. New preparation, incompatible metadata or sizes, explicit reset, direct/catalog/array execution, statement attribute changes, and any execution or conversion error invalidate reuse. Native storage stays owned until ODBC resets the bindings or frees the handle, including error paths and parent connection teardown, so no still-bound address is freed early and no Python reference is retained in the cache. The DB-API threadsafety=1 contract is unchanged. This is an internal reuse optimization with no public API or behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/cursor.py | 4 +- mssql_python/pybind/README.md | 21 + mssql_python/pybind/connection/connection.cpp | 24 +- mssql_python/pybind/ddbc_bindings.cpp | 316 +++++++++++++- mssql_python/pybind/ddbc_bindings.h | 31 +- tests/test_037_cached_bindings.py | 410 ++++++++++++++++++ 6 files changed, 777 insertions(+), 29 deletions(-) create mode 100644 tests/test_037_cached_bindings.py diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index 605120bfc..77f54d3f8 100644 --- a/mssql_python/cursor.py +++ b/mssql_python/cursor.py @@ -1031,13 +1031,13 @@ def _reset_cursor(self) -> None: self.is_stmt_prepared = [False] def _soft_reset_cursor(self) -> None: - """Lightweight reset: close cursor and unbind params without freeing the HSTMT. + """Close results without freeing the HSTMT or compatible cached bindings. Preserves the prepared statement plan on the server so repeated executions of the same SQL skip SQLPrepare entirely. """ if self.hstmt: - ret = ddbc_bindings.DDBCSQLResetStmt(self.hstmt) + ret = ddbc_bindings.DDBCSQLResetStmt(self.hstmt, preserve_bindings=True) try: check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret) except Exception: diff --git a/mssql_python/pybind/README.md b/mssql_python/pybind/README.md index 977087f11..d6f3ea8ec 100644 --- a/mssql_python/pybind/README.md +++ b/mssql_python/pybind/README.md @@ -2,6 +2,27 @@ This README provides instructions to build the DDBC Bindings for your system and documents the platform-specific dependencies. +## Repeated execute bindings + +Each statement owns at most one reusable generation of native input buffers. +The existing detector and binder still validate and convert every execution. +Bindings are reused only for the same prepared SQL, parameter count, C/SQL +types, column size, scale, direction, encoding, and actual buffer byte lengths. +Inline text/binary and integer, boolean, and floating-point buffers are supported, +up to 2,100 parameters and 8,000 bytes per text/binary buffer. NULL, DAE, and +complex C types use the uncached path; decimal overrides converted to text +can reuse only with matching precision and scale. + +Soft cursor resets preserve successful cached bindings. New SQL, incompatible +metadata or byte lengths, explicit resets, direct/catalog/array execution, +statement-attribute changes, and errors invalidate reuse. Native storage remains +owned until ODBC resets the bindings or frees the statement, including error +paths and parent connection teardown. No Python references are retained in the +cache, and the existing DB-API `threadsafety=1` contract is unchanged. + +`tests/test_037_cached_bindings.py` checks live round trips and actual native +allocation/bind events through the existing debug logger, without a test-only API. + ## **Key Architecture Handling** 1. **Architecture Normalization** (from `mssql_python/ddbc_bindings.py`): diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 00cf910ec..553d22e21 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -113,14 +113,15 @@ void Connection::disconnect() { LOG("Disconnecting from database"); } - // CRITICAL FIX: Mark all child statement handles as implicitly freed - // When we free the DBC handle below, the ODBC driver will automatically free - // all child STMT handles. We need to tell the SqlHandle objects about this - // so they don't try to free the handles again during their destruction. - + // SQLDisconnect frees child statements. Retain their owners until it + // succeeds, then mark them so later cursor cleanup cannot double-free. + // THREAD-SAFETY: Lock mutex to safely access _childStatementHandles // This protects against concurrent allocStatementHandle() calls or GC finalizers size_t originalSize = 0, afterCompactSize = 0, badHandleCount = 0; + // Keep statement buffers alive through the parent's blocking disconnect. + // Do not mark children freed until SQLDisconnect actually succeeds. + std::vector childHandles; { std::lock_guard lock(_childHandlesMutex); @@ -141,11 +142,9 @@ void Connection::disconnect() { ++badHandleCount; continue; // Skip marking to prevent leak } - handle->markImplicitlyFreed(); + childHandles.push_back(std::move(handle)); } } - _childStatementHandles.clear(); - _allocationsSinceCompaction = 0; } // Release lock before potentially slow SQLDisconnect call // Log after releasing _childHandlesMutex (#671): LOG()/LOG_ERROR() acquire @@ -181,6 +180,15 @@ void Connection::disconnect() { // via py::gil_scoped_acquire, which is unsafe during interpreter // shutdown or stack unwinding (can deadlock or call std::terminate). } + if (SQL_SUCCEEDED(ret)) { + for (const auto& handle : childHandles) { + handle->markImplicitlyFreed(); + handle->releaseAfterFree(); + } + std::lock_guard lock(_childHandlesMutex); + _childStatementHandles.clear(); + _allocationsSinceCompaction = 0; + } // triggers SQLFreeHandle via destructor, if last owner _dbcHandle.reset(); } else if (hasGil) { diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 9908d8422..7529f3442 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -314,9 +314,89 @@ ParamType* AllocateParamBuffer(std::vector>& paramBuffers, CtorArgs&&... ctorArgs) { paramBuffers.emplace_back(new ParamType(std::forward(ctorArgs)...), std::default_delete()); + LOG("AllocateParamBuffer: New owned buffer"); return static_cast(paramBuffers.back().get()); } +struct ExecuteParamBuffers { + std::vector>& current; + const std::vector>* previous; +}; + +template +static bool UpdateParamBuffer(T& target, T&& value) { + target = std::move(value); + return true; +} + +template +static bool UpdateParamBuffer(std::basic_string& target, std::basic_string&& value) { + // Equal byte lengths keep both the address and ODBC BufferLength unchanged. + // Do not assign a string: even a capacity-preserving assignment may move it. + if (target.size() != value.size()) + return false; + std::copy(value.begin(), value.end(), target.begin()); + return true; +} + +template +ParamType* AllocateParamBuffer(ExecuteParamBuffers& buffers, CtorArgs&&... ctorArgs) { + ParamType value(std::forward(ctorArgs)...); + const size_t index = buffers.current.size(); + if (buffers.previous && index < buffers.previous->size()) { + const auto& previous = (*buffers.previous)[index]; + auto* target = static_cast(previous.get()); + if (UpdateParamBuffer(*target, std::move(value))) { + buffers.current.push_back(previous); + return target; + } + } + return AllocateParamBuffer(buffers.current, std::move(value)); +} + +static bool SameParameterShape(const ParameterBinding& binding, const ParamInfo& info) { + return binding.direction == info.inputOutputType && binding.cType == info.paramCType && + binding.sqlType == info.paramSQLType && binding.columnSize == info.columnSize && + binding.scale == info.decimalDigits; +} + +static bool CanCacheParameters(const std::vector& infos) { + // Bound retained storage to SQL Server's scalar parameter limit. NULL/DAE and + // descriptor-based/complex types deliberately use the existing uncached path. + if (infos.empty() || infos.size() > 2100) + return false; + for (const auto& info : infos) { + if (info.isDAE || info.inputOutputType != SQL_PARAM_INPUT) + return false; + switch (info.paramCType) { + case SQL_C_CHAR: + case SQL_C_WCHAR: + case SQL_C_BINARY: + if (info.columnSize > MAX_INLINE_BINARY) + return false; + break; + case SQL_C_BIT: + case SQL_C_STINYINT: + case SQL_C_TINYINT: + case SQL_C_SSHORT: + case SQL_C_SHORT: + case SQL_C_UTINYINT: + case SQL_C_USHORT: + case SQL_C_SBIGINT: + case SQL_C_SLONG: + case SQL_C_LONG: + case SQL_C_UBIGINT: + case SQL_C_ULONG: + case SQL_C_FLOAT: + case SQL_C_DOUBLE: + break; + default: + return false; + } + } + return true; +} + template ParamType* AllocateParamBufferArray(std::vector>& paramBuffers, size_t count) { @@ -451,14 +531,35 @@ static void PreResolveUnknownNullTypes(SqlHandle& handle, SQLHANDLE hStmt, // each of them with appropriate arguments SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& params, std::vector& paramInfos, - std::vector>& paramBuffers, - const std::string& charEncoding = "utf-8") { + std::vector>& ownedBuffers, + const std::string& charEncoding = "utf-8", bool cacheForExecute = false) { LOG("BindParameters: Starting parameter binding for statement handle %p " "with %zu parameters", (void*)hStmt, params.size()); + bool eligible = cacheForExecute && CanCacheParameters(paramInfos); + if (cacheForExecute && !eligible) { + SQLRETURN rc = handle.resetParameterBindings(); + if (!SQL_SUCCEEDED(rc)) + return rc; + } // GH-627: resolve unknown NULL param SQL types before binding any param. PreResolveUnknownNullTypes(handle, hStmt, paramInfos, ¶ms); + auto* previous = handle.executeBindings.get(); + bool reuse = eligible && previous && previous->reusable && previous->encoding == charEncoding && + previous->bindings.size() == paramInfos.size(); + if (reuse) { + for (size_t i = 0; i < paramInfos.size(); ++i) { + if (!SameParameterShape(previous->bindings[i], paramInfos[i])) { + reuse = false; + break; + } + } + } + ExecuteParamBuffers paramBuffers{ownedBuffers, reuse ? &previous->buffers : nullptr}; + ownedBuffers.reserve(params.size() * 2); + std::vector bindings; + bindings.reserve(params.size()); for (int paramIndex = 0; paramIndex < params.size(); paramIndex++) { const auto& param = params[paramIndex]; ParamInfo& paramInfo = paramInfos[paramIndex]; @@ -857,7 +958,49 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par ThrowStdException(errorString.str()); } } + bindings.push_back({paramInfo.inputOutputType, paramInfo.paramCType, paramInfo.paramSQLType, + paramInfo.columnSize, paramInfo.decimalDigits, dataPtr, bufferLength, + strLenOrIndPtr}); + if (bufferLength > MAX_INLINE_BINARY) + eligible = false; + } + + if (reuse) { + for (size_t i = 0; i < bindings.size(); ++i) { + const auto& old = previous->bindings[i]; + const auto& current = bindings[i]; + if (old.data != current.data || old.length != current.length || + old.indicator != current.indicator) { + reuse = false; + break; + } + } + } + if (reuse) { + LOG("BindParameters: Reusing %zu bound parameters", bindings.size()); + return SQL_SUCCESS; + } + if (cacheForExecute) { + // Reset only after conversion succeeded; old addresses remain owned until + // ODBC releases them. New buffers are handle-owned before the first bind, + // including partial-bind failures where diagnostics must not be erased. + SQLRETURN rc = handle.resetParameterBindings(); + if (!SQL_SUCCEEDED(rc)) + return rc; + auto cache = std::make_unique(); + cache->bindings = bindings; + cache->buffers = ownedBuffers; + cache->encoding = charEncoding; + handle.executeBindings = std::move(cache); + } + for (int paramIndex = 0; paramIndex < bindings.size(); ++paramIndex) { + const ParamInfo& paramInfo = paramInfos[paramIndex]; + const auto& binding = bindings[paramIndex]; + void* dataPtr = binding.data; + SQLLEN bufferLength = binding.length; + SQLLEN* strLenOrIndPtr = binding.indicator; assert(SQLBindParameter_ptr && SQLGetStmtAttr_ptr && SQLSetDescField_ptr); + LOG("BindParameters: SQLBindParameter param[%d]", paramIndex); RETCODE rc = SQLBindParameter_ptr( hStmt, static_cast(paramIndex + 1), /* 1-based indexing */ static_cast(paramInfo.inputOutputType), @@ -930,6 +1073,8 @@ SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& par LOG("BindParameters: Completed parameter binding for statement handle %p - " "%zu parameters bound successfully", (void*)hStmt, params.size()); + if (cacheForExecute) + handle.executeBindings->reusable = eligible; return SQL_SUCCESS; } @@ -1539,6 +1684,11 @@ SqlHandle::~SqlHandle() { if (_handle) { free(); } + // If the driver refused to free the handle, it can still reference these + // addresses. Leak only this failed teardown's native storage, not dangling + // pointers into freed memory. Explicit free() failures retain ownership. + if (_handle && !_implicitly_freed) + executeBindings.release(); } SQLHANDLE SqlHandle::get() const { @@ -1563,6 +1713,27 @@ void SqlHandle::markImplicitlyFreed() { return; // Refuse to mark - let normal free() handle it } _implicitly_freed = true; + if (executeBindings) + executeBindings->reusable = false; +} + +SQLRETURN SqlHandle::resetParameterBindings() { + if (!executeBindings) + return SQL_SUCCESS; + executeBindings->reusable = false; + if (!_handle || _implicitly_freed) + return SQL_INVALID_HANDLE; + SQLRETURN rc = SQLFreeStmt_ptr(_handle, SQL_RESET_PARAMS); + if (SQL_SUCCEEDED(rc)) + executeBindings.reset(); + return rc; +} + +void SqlHandle::releaseAfterFree() { + _handle = nullptr; + executeBindings.reset(); + preparedQuery.clear(); + describeCache.clear(); } /* @@ -1589,6 +1760,7 @@ void SqlHandle::free() { // 3. This tradeoff prioritizes crash prevention over resource cleanup, which // is appropriate since we're already in shutdown sequence if (pythonShuttingDown && (_type == SQL_HANDLE_STMT || _type == SQL_HANDLE_DBC)) { + executeBindings.release(); _handle = nullptr; // Mark as freed to prevent double-free attempts return; } @@ -1599,7 +1771,7 @@ void SqlHandle::free() { // This approach avoids calling ODBC functions on potentially-freed handles, which // would cause use-after-free errors. if (_implicitly_freed) { - _handle = nullptr; // Just clear the pointer, don't call ODBC functions + releaseAfterFree(); return; } @@ -1612,13 +1784,18 @@ void SqlHandle::free() { // (issue #565). Only release the GIL if it is actually held AND the // interpreter is not finalizing - gil_scoped_release is unsafe during // shutdown even if PyGILState_Check() reports the GIL as held. + SQLRETURN rc; if (!pythonShuttingDown && PyGILState_Check()) { py::gil_scoped_release release; - SQLFreeHandle_ptr(_type, _handle); + rc = SQLFreeHandle_ptr(_type, _handle); } else { - SQLFreeHandle_ptr(_type, _handle); + rc = SQLFreeHandle_ptr(_type, _handle); + } + if (SQL_SUCCEEDED(rc)) { + releaseAfterFree(); + } else if (executeBindings) { + executeBindings->reusable = false; } - _handle = nullptr; } } @@ -1644,6 +1821,8 @@ void SqlHandle::close_cursor() { ret = SQLFreeStmt_ptr(_handle, SQL_CLOSE); } if (ret != SQL_SUCCESS && ret != SQL_SUCCESS_WITH_INFO) { + if (executeBindings) + executeBindings->reusable = false; ThrowStdException("SQLFreeStmt(SQL_CLOSE) failed"); } } @@ -1695,7 +1874,7 @@ void SqlHandle::cancel() { } } -SQLRETURN SQLResetStmt_wrap(SqlHandlePtr statementHandle) { +SQLRETURN SQLResetStmt_wrap(SqlHandlePtr statementHandle, bool preserveBindings = false) { if (!statementHandle || !statementHandle->get()) { return SQL_INVALID_HANDLE; } @@ -1711,17 +1890,30 @@ SQLRETURN SQLResetStmt_wrap(SqlHandlePtr statementHandle) { { py::gil_scoped_release release; rc = SQLFreeStmt_ptr(hStmt, SQL_CLOSE); - if (SQL_SUCCEEDED(rc)) { - rc = SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); + if (SQL_SUCCEEDED(rc) && !(preserveBindings && statementHandle->executeBindings && + statementHandle->executeBindings->reusable)) { + if (statementHandle->executeBindings) { + rc = statementHandle->resetParameterBindings(); + } else { + rc = SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); + } } if (SQL_SUCCEEDED(rc) && SQLSetStmtAttr_ptr) { rc = SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_PARAMSET_SIZE, (SQLPOINTER)1, 0); } } + if (!SQL_SUCCEEDED(rc) && statementHandle->executeBindings) { + statementHandle->executeBindings->reusable = false; + } return rc; } SQLRETURN SQLGetTypeInfo_Wrapper(SqlHandlePtr StatementHandle, SQLSMALLINT DataType) { + SQLRETURN reset = StatementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + StatementHandle->preparedQuery.clear(); + StatementHandle->clearDescribeCache(); if (!SQLGetTypeInfo_ptr) { ThrowStdException("SQLGetTypeInfo function not loaded"); } @@ -1733,6 +1925,11 @@ SQLRETURN SQLGetTypeInfo_Wrapper(SqlHandlePtr StatementHandle, SQLSMALLINT DataT SQLRETURN SQLProcedures_wrap(SqlHandlePtr StatementHandle, const py::object& catalogObj, const py::object& schemaObj, const py::object& procedureObj) { + SQLRETURN reset = StatementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + StatementHandle->preparedQuery.clear(); + StatementHandle->clearDescribeCache(); if (!SQLProcedures_ptr) { ThrowStdException("SQLProcedures function not loaded"); } @@ -1756,6 +1953,11 @@ SQLRETURN SQLForeignKeys_wrap(SqlHandlePtr StatementHandle, const py::object& pk const py::object& pkSchemaObj, const py::object& pkTableObj, const py::object& fkCatalogObj, const py::object& fkSchemaObj, const py::object& fkTableObj) { + SQLRETURN reset = StatementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + StatementHandle->preparedQuery.clear(); + StatementHandle->clearDescribeCache(); if (!SQLForeignKeys_ptr) { ThrowStdException("SQLForeignKeys function not loaded"); } @@ -1787,6 +1989,11 @@ SQLRETURN SQLForeignKeys_wrap(SqlHandlePtr StatementHandle, const py::object& pk SQLRETURN SQLPrimaryKeys_wrap(SqlHandlePtr StatementHandle, const py::object& catalogObj, const py::object& schemaObj, const std::u16string& table) { + SQLRETURN reset = StatementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + StatementHandle->preparedQuery.clear(); + StatementHandle->clearDescribeCache(); if (!SQLPrimaryKeys_ptr) { ThrowStdException("SQLPrimaryKeys function not loaded"); } @@ -1808,6 +2015,11 @@ SQLRETURN SQLPrimaryKeys_wrap(SqlHandlePtr StatementHandle, const py::object& ca SQLRETURN SQLStatistics_wrap(SqlHandlePtr StatementHandle, const py::object& catalogObj, const py::object& schemaObj, const std::u16string& table, SQLUSMALLINT unique, SQLUSMALLINT reserved) { + SQLRETURN reset = StatementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + StatementHandle->preparedQuery.clear(); + StatementHandle->clearDescribeCache(); if (!SQLStatistics_ptr) { ThrowStdException("SQLStatistics function not loaded"); } @@ -1829,6 +2041,11 @@ SQLRETURN SQLStatistics_wrap(SqlHandlePtr StatementHandle, const py::object& cat SQLRETURN SQLColumns_wrap(SqlHandlePtr StatementHandle, const py::object& catalogObj, const py::object& schemaObj, const py::object& tableObj, const py::object& columnObj) { + SQLRETURN reset = StatementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + StatementHandle->preparedQuery.clear(); + StatementHandle->clearDescribeCache(); if (!SQLColumns_ptr) { ThrowStdException("SQLColumns function not loaded"); } @@ -1855,6 +2072,10 @@ SQLRETURN SQLColumns_wrap(SqlHandlePtr StatementHandle, const py::object& catalo ErrorInfo SQLCheckError_Wrap(SQLSMALLINT handleType, SqlHandlePtr handle, SQLRETURN retcode) { LOG("SQLCheckError: Checking ODBC errors - handleType=%d, retcode=%d", handleType, retcode); ErrorInfo errorInfo; + if ((retcode == SQL_ERROR || retcode == SQL_INVALID_HANDLE) && handle && + handle->executeBindings) { + handle->executeBindings->reusable = false; + } if (retcode == SQL_INVALID_HANDLE) { LOG("SQLCheckError: SQL_INVALID_HANDLE detected - handle is invalid"); errorInfo.ddbcErrorMsg = "Invalid handle!"; @@ -1938,6 +2159,14 @@ py::list SQLGetAllDiagRecords(SqlHandlePtr handle) { // Wrap SQLExecDirect SQLRETURN SQLExecDirect_wrap(SqlHandlePtr StatementHandle, const std::u16string& Query) { + if (!StatementHandle || !StatementHandle->get() || StatementHandle->isImplicitlyFreed()) { + return SQL_INVALID_HANDLE; + } + SQLRETURN reset = StatementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + StatementHandle->preparedQuery.clear(); + StatementHandle->clearDescribeCache(); LOG("SQLExecDirect: Executing query directly - statement_handle=%p, " "query_length=%zu chars", (void*)StatementHandle->get(), Query.length()); @@ -1973,6 +2202,11 @@ SQLRETURN SQLExecDirect_wrap(SqlHandlePtr StatementHandle, const std::u16string& SQLRETURN SQLTables_wrap(SqlHandlePtr StatementHandle, const std::u16string& catalog, const std::u16string& schema, const std::u16string& table, const std::u16string& tableType) { + SQLRETURN reset = StatementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + StatementHandle->preparedQuery.clear(); + StatementHandle->clearDescribeCache(); if (!SQLTables_ptr) { LOG("SQLTables: Function pointer not initialized, loading driver"); DriverLoader::getInstance().loadDriver(); @@ -2014,14 +2248,31 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, py::list is_stmt_prepared, bool use_prepare, const py::dict& encoding_settings) { - if (!statementHandle || !statementHandle->get()) { + if (!statementHandle || !statementHandle->get() || statementHandle->isImplicitlyFreed()) { return SQL_INVALID_HANDLE; } + struct ExecutionAttempt { + SqlHandle& handle; + bool succeeded = false; + ~ExecutionAttempt() { + if (!succeeded && handle.executeBindings) + handle.executeBindings->reusable = false; + } + } attempt{*statementHandle}; SQLHANDLE hStmt = statementHandle->get(); + if (statementHandle->executeBindings && + (!statementHandle->executeBindings->reusable || statementHandle->preparedQuery != query)) { + SQLRETURN reset = statementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + } // Configure forward-only / read-only cursor (matches slow path semantics). if (SQLSetStmtAttr_ptr) { + SQLRETURN attrRc = SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_PARAMSET_SIZE, (SQLPOINTER)1, 0); + if (!SQL_SUCCEEDED(attrRc)) + return attrRc; SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_CURSOR_TYPE, (SQLPOINTER)SQL_CURSOR_FORWARD_ONLY, 0); SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_CONCURRENCY, @@ -2055,7 +2306,8 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, std::vector paramInfos = DetectParamTypes(params.ptr(), input_sizes.ptr()); RETCODE rc; - bool already_prepared = is_stmt_prepared[0].cast(); + bool already_prepared = + is_stmt_prepared[0].cast() && statementHandle->preparedQuery == query; // Honor use_prepare flag (matching slow path behavior): // - use_prepare=true: prepare now (or reuse if same SQL already prepared) @@ -2063,6 +2315,10 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, // - use_prepare=false + not prepared: error (cannot execute unprepared) if (!already_prepared) { if (use_prepare) { + rc = statementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(rc)) + return rc; + statementHandle->preparedQuery.clear(); SQLWCHAR* queryPtr = reinterpretU16stringAsSqlWChar(query); { py::gil_scoped_release release; @@ -2070,6 +2326,7 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, } if (!SQL_SUCCEEDED(rc)) return rc; statementHandle->clearDescribeCache(); + statementHandle->preparedQuery = query; is_stmt_prepared[0] = py::bool_(true); } else { ThrowStdException("Cannot execute unprepared statement"); @@ -2077,7 +2334,8 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, } std::vector> paramBuffers; - rc = BindParameters(*statementHandle, hStmt, params, paramInfos, paramBuffers, charEncoding); + rc = BindParameters(*statementHandle, hStmt, params, paramInfos, paramBuffers, charEncoding, + true); if (!SQL_SUCCEEDED(rc)) return rc; { @@ -2171,10 +2429,15 @@ SQLRETURN SQLExecute_wrap(const SqlHandlePtr statementHandle, if (!SQL_SUCCEEDED(rc) && rc != SQL_NO_DATA) return rc; - // Unbind parameter buffers before they go out of scope. - // Not called on error paths — diagnostics must remain readable. + // Unsupported shapes are not retained for reuse. On errors native ownership + // stays with the handle until reset/free, without destroying diagnostics. SQLRETURN exec_rc = rc; - SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); + if (!statementHandle->executeBindings->reusable) { + rc = statementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(rc)) + return rc; + } + attempt.succeeded = true; return exec_rc; } @@ -2813,6 +3076,13 @@ SQLRETURN SQLExecuteMany_wrap(const SqlHandlePtr statementHandle, const std::u16 const py::list& columnwise_params, std::vector& paramInfos, size_t paramSetSize, const py::dict& encodingSettings) { + if (!statementHandle || !statementHandle->get() || statementHandle->isImplicitlyFreed()) { + return SQL_INVALID_HANDLE; + } + SQLRETURN reset = statementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + statementHandle->preparedQuery.clear(); LOG("SQLExecuteMany: Starting batch execution - param_count=%zu, " "param_set_size=%zu", columnwise_params.size(), paramSetSize); @@ -2831,6 +3101,7 @@ SQLRETURN SQLExecuteMany_wrap(const SqlHandlePtr statementHandle, const std::u16 } // GH-610: Clear per-handle describe cache (new prepare = new param types) statementHandle->clearDescribeCache(); + statementHandle->preparedQuery = query; LOG("SQLExecuteMany: Query prepared successfully"); bool hasDAE = false; @@ -3039,6 +3310,11 @@ SQLRETURN SQLSpecialColumns_wrap(SqlHandlePtr StatementHandle, SQLSMALLINT ident const py::object& catalogObj, const py::object& schemaObj, const std::u16string& table, SQLSMALLINT scope, SQLSMALLINT nullable) { + SQLRETURN reset = StatementHandle->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; + StatementHandle->preparedQuery.clear(); + StatementHandle->clearDescribeCache(); if (!SQLSpecialColumns_ptr) { ThrowStdException("SQLSpecialColumns function not loaded"); } @@ -5930,8 +6206,12 @@ SQLRETURN SQLFreeHandle_wrap(SQLSMALLINT HandleType, SqlHandlePtr Handle) { } if (!SQL_SUCCEEDED(ret)) { LOG("SQLFreeHandle_wrap: SQLFreeHandle failed with error code - %d", ret); + if (Handle->executeBindings) { + Handle->executeBindings->reusable = false; + } return ret; } + Handle->releaseAfterFree(); return ret; } @@ -6099,7 +6379,8 @@ PYBIND11_MODULE(ddbc_bindings, m) { "Fetch an arrow batch of given length from the result set"); m.def("DDBCSQLFreeHandle", &SQLFreeHandle_wrap, "Free a handle"); m.def("DDBCSQLResetStmt", &SQLResetStmt_wrap, - "Close cursor and unbind params without freeing HSTMT"); + "Close cursor, optionally retaining compatible execute bindings", + py::arg("statementHandle"), py::arg("preserve_bindings") = false); m.def("DDBCSQLCheckError", &SQLCheckError_Wrap, "Check for driver errors"); m.def("DDBCSQLGetAllDiagRecords", &SQLGetAllDiagRecords, "Get all diagnostic records for a handle", py::arg("handle")); @@ -6115,6 +6396,9 @@ PYBIND11_MODULE(ddbc_bindings, m) { m.def( "DDBCSQLSetStmtAttr", [](SqlHandlePtr stmt, SQLINTEGER attr, py::object value) { + SQLRETURN reset = stmt->resetParameterBindings(); + if (!SQL_SUCCEEDED(reset)) + return reset; SQLPOINTER ptr_value; if (py::isinstance(value)) { // For integer attributes like SQL_ATTR_QUERY_TIMEOUT diff --git a/mssql_python/pybind/ddbc_bindings.h b/mssql_python/pybind/ddbc_bindings.h index 00f04aa09..b3cd67d87 100644 --- a/mssql_python/pybind/ddbc_bindings.h +++ b/mssql_python/pybind/ddbc_bindings.h @@ -280,6 +280,26 @@ struct DescribedParamInfo { SQLSMALLINT decimalDigits; }; +struct ParameterBinding { + SQLSMALLINT direction; + SQLSMALLINT cType; + SQLSMALLINT sqlType; + SQLULEN columnSize; + SQLSMALLINT scale; + SQLPOINTER data; + SQLLEN length; + SQLLEN* indicator; +}; + +// Native-only ownership: cleanup is safe even on GIL-less connection teardown. +// One generation per statement, never keyed by a recycled raw ODBC handle. +struct ExecuteBindingCache { + std::vector bindings; + std::vector> buffers; + std::string encoding; + bool reusable = false; +}; + class SqlHandle { public: SqlHandle(SQLSMALLINT type, SQLHANDLE rawHandle); @@ -303,13 +323,18 @@ class SqlHandle { // // SAFETY CONSTRAINTS: // - ONLY call this on SQL_HANDLE_STMT handles - // - ONLY call this when the parent DBC handle is about to be freed + // - ONLY call this after the parent SQLDisconnect has succeeded // - Calling on other handle types (ENV, DBC, DESC) will cause HANDLE LEAKS // - The ODBC spec only guarantees automatic freeing of STMT handles by DBC parents // - // Current usage: Connection::disconnect() marks all tracked STMT handles - // before freeing the DBC handle. + // Connection::disconnect() retains the tracked STMT owners through the + // disconnect, then marks them and releases their native binding storage. void markImplicitlyFreed(); + SQLRETURN resetParameterBindings(); + void releaseAfterFree(); + + std::unique_ptr executeBindings; + std::u16string preparedQuery; // GH-610: Per-handle SQLDescribeParam result cache. // Keyed by 0-based parameter index. Populated on first NULL param diff --git a/tests/test_037_cached_bindings.py b/tests/test_037_cached_bindings.py new file mode 100644 index 000000000..8de24b702 --- /dev/null +++ b/tests/test_037_cached_bindings.py @@ -0,0 +1,410 @@ +"""Repeated execute reuses native bindings, not Python values or raw handle keys. + +The existing native logger observes the actual allocation/bind call sites. These +tests therefore check the optimization's contract as well as returned values, +without a test-only API or a special build. +""" + +import datetime +import decimal +import gc +import logging +import os +import subprocess +import sys +import textwrap +import uuid +import weakref +from concurrent.futures import ThreadPoolExecutor + +import pytest + +import mssql_python +from mssql_python import ddbc_bindings +from mssql_python.constants import ConstantsDDBC as SQL +from mssql_python.logging import logger + + +@pytest.fixture +def cursor(db_connection): + current = db_connection.cursor() + try: + yield current + finally: + current.close() + + +@pytest.fixture +def binding_events(caplog): + old_level = logger.level + caplog.set_level(logging.DEBUG, logger="mssql_python") + native_logger = logging.getLogger("mssql_python") + native_logger.addHandler(caplog.handler) + ddbc_bindings.update_log_level(logging.DEBUG) + try: + yield caplog + finally: + ddbc_bindings.update_log_level(old_level) + native_logger.removeHandler(caplog.handler) + + +def counts(events): + messages = [record.getMessage() for record in events.records] + return ( + sum("BindParameters: SQLBindParameter param[" in message for message in messages), + sum("BindParameters: Reusing " in message for message in messages), + sum("AllocateParamBuffer: New owned buffer" in message for message in messages), + ) + + +@pytest.mark.parametrize("reset_cursor", [True, False]) +@pytest.mark.parametrize( + "first,second", + [ + ([12], [34]), + ([32768], [32769]), + ([2**40], [2**40 + 1]), + ([True], [False]), + ([1.25], [-2.5]), + (["abc"], ["def"]), + (["a" * 1000], ["b" * 1000]), + (["你好"], ["世界"]), + (["你" * 1000], ["界" * 1000]), + (["😀x"], ["🚀y"]), + (["a\0b"], ["c\0d"]), + ([""], [""]), + ([b""], [b""]), + ([b"\x00\x01"], [b"\xff\x00"]), + ([b"a" * 1000], [b"b" * 1000]), + ([bytearray(b"ab")], [bytearray(b"cd")]), + ([12, "abc", 1.25, True], [34, "def", -2.5, False]), + ], +) +def test_same_shape_reuses_bound_buffers(cursor, binding_events, reset_cursor, first, second): + query = "SELECT " + ", ".join("?" for _ in first) + cursor.execute(query, first) + assert tuple(cursor.fetchone()) == tuple(first) + handle = cursor.hstmt + assert counts(binding_events)[0] == len(first) + for values in (second, first, second): + binding_events.clear() + cursor.execute(query, values, reset_cursor=reset_cursor) + assert tuple(cursor.fetchone()) == tuple(values) + assert cursor.hstmt is handle + assert counts(binding_events) == (0, 1, 0) + + +@pytest.mark.parametrize( + "first,second", + [ + (12, 32768), + (12, True), + (12, 1.25), + ("abc", "longer string"), + ("abc", "x"), + ("abc", "世界"), + ("abc", b"abc"), + ("", "x"), + (b"", b"x"), + ("abc", None), + (None, "abc"), + ], +) +def test_shape_changes_rebind(cursor, binding_events, first, second): + cursor.execute("SELECT ?", [first]).fetchone() + binding_events.clear() + cursor.execute("SELECT ?", [second]) + assert cursor.fetchone()[0] == second + assert counts(binding_events)[0:2] == (1, 0) + + +def test_input_sizes_and_actual_encoded_length(cursor, binding_events): + for value, reused in [("😀", False), ("🚀", True), ("ab", True), ("中", False), ("文", True)]: + cursor.setinputsizes([(SQL.SQL_WVARCHAR.value, 100, 0)]) + binding_events.clear() + cursor.execute("SELECT ?", [value]) + assert cursor.fetchone()[0] == value + assert counts(binding_events)[0:2] == ((0, 1) if reused else (1, 0)) + cursor.setinputsizes([(SQL.SQL_WVARCHAR.value, 200, 0)]) + binding_events.clear() + assert cursor.execute("SELECT ?", ["字"]).fetchone()[0] == "字" + assert counts(binding_events)[0:2] == (1, 0) + cursor.setinputsizes(None) + + +def test_encoding_changes_rebind(cursor, db_connection, binding_events): + try: + for encoding, value, reuse in [ + ("utf-8", "abc", False), + ("utf-8", "def", True), + ("ascii", "ghi", False), + ("ascii", "jkl", True), + ]: + cursor.setinputsizes([(SQL.SQL_VARCHAR.value, 100, 0)]) + db_connection.setencoding(encoding, ctype=mssql_python.SQL_CHAR) + binding_events.clear() + assert cursor.execute("SELECT ?", [value]).fetchone()[0] == value + assert counts(binding_events)[0:2] == ((0, 1) if reuse else (1, 0)) + finally: + db_connection.setencoding() + cursor.setinputsizes(None) + + +@pytest.mark.parametrize( + "value", + [ + None, + decimal.Decimal("123.45"), + datetime.date(2024, 1, 2), + datetime.datetime(2024, 1, 2, 3, 4, 5), + uuid.UUID("12345678-1234-5678-1234-567812345678"), + "x" * 9000, + "😀" * 4500, + b"\0" * 9000, + bytearray(b"x" * 9000), + ], +) +def test_uncached_shapes_fall_back_and_recover(cursor, binding_events, value): + cursor.execute("SELECT ?", [12]).fetchone() + for _ in range(2): + binding_events.clear() + cursor.execute("SELECT ?", [value]) + result = cursor.fetchone()[0] + if isinstance(value, uuid.UUID): + assert str(result).lower() == str(value) + else: + assert result == value + assert counts(binding_events)[0:2] == (1, 0) + cursor.execute("SELECT ?", [34]).fetchone() + binding_events.clear() + assert cursor.execute("SELECT ?", [56]).fetchone()[0] == 56 + assert counts(binding_events) == (0, 1, 0) + + +def test_changed_sql_direct_and_parameter_count(cursor, binding_events): + for sql, values in [ + ("SELECT ?", [12]), + ("SELECT ? + 1", [12]), + ("SELECT ?, ?", [12, 13]), + ("SELECT 12", []), + ("SELECT ?", [14]), + ]: + binding_events.clear() + cursor.execute(sql, values).fetchone() + assert counts(binding_events)[0:2] == (len(values), 0) + + +@pytest.mark.parametrize("fast", [False, True]) +def test_executemany_invalidates(cursor, binding_events, fast): + cursor.execute("DROP TABLE IF EXISTS #cached_bindings") + cursor.execute("CREATE TABLE #cached_bindings (value int)") + sql = "INSERT INTO #cached_bindings VALUES (?)" + cursor.execute(sql, [12]) + cursor.execute(sql, [13]) + cursor.fast_executemany = fast + cursor.executemany(sql, [[14], [15]]) + binding_events.clear() + cursor.execute(sql, [16]) + assert counts(binding_events)[0:2] == (1, 0) + cursor.execute("SELECT value FROM #cached_bindings ORDER BY value") + assert [row[0] for row in cursor.fetchall()] == [12, 13, 14, 15, 16] + cursor.execute("DROP TABLE #cached_bindings") + + +def test_execution_failure_diagnostics_and_recovery(cursor, binding_events): + sql = "SELECT 10 / ?" + assert cursor.execute(sql, [2]).fetchone()[0] == 5 + with pytest.raises(mssql_python.DatabaseError, match="(?i)divide by zero"): + cursor.execute(sql, [0]).fetchone() + binding_events.clear() + assert cursor.execute(sql, [5]).fetchone()[0] == 2 + assert counts(binding_events)[0:2] == (1, 0) + + +def test_validation_failure_invalidates_without_stale_values(cursor, binding_events): + cursor.execute("SELECT ?, ?", [12, "abc"]).fetchone() + with pytest.raises((TypeError, RuntimeError, mssql_python.DatabaseError)): + cursor.execute("SELECT ?, ?", [13, object()]) + binding_events.clear() + assert tuple(cursor.execute("SELECT ?, ?", [14, "def"]).fetchone()) == (14, "def") + assert counts(binding_events)[0:2] == (2, 0) + + +def test_conversion_failure_keeps_bound_storage_alive(cursor, binding_events): + sizes = [(SQL.SQL_INTEGER.value, 10, 0), (SQL.SQL_SMALLINT.value, 5, 0)] + cursor.setinputsizes(sizes) + cursor.execute("SELECT ?, ?", [12, 13]).fetchone() + cursor.setinputsizes(sizes) + with pytest.raises((RuntimeError, OverflowError, mssql_python.DatabaseError)): + cursor.execute("SELECT ?, ?", [14, 2**40]) + binding_events.clear() + assert tuple(cursor.execute("SELECT ?, ?", [15, 16]).fetchone()) == (15, 16) + assert counts(binding_events)[0:2] == (2, 0) + cursor.setinputsizes(None) + + +def test_explicit_reset_and_close_lifetimes(db_connection, binding_events): + cursor = db_connection.cursor() + cursor.execute("SELECT ?", [12]).fetchone() + handle = cursor.hstmt + assert ddbc_bindings.DDBCSQLResetStmt(handle) == SQL.SQL_SUCCESS.value + binding_events.clear() + assert cursor.execute("SELECT ?", [13]).fetchone()[0] == 13 + assert counts(binding_events)[0:2] == (1, 0) + cursor.close() + assert ddbc_bindings.DDBCSQLResetStmt(handle) == SQL.SQL_INVALID_HANDLE.value + other = db_connection.cursor() + try: + binding_events.clear() + assert other.execute("SELECT ?", [14]).fetchone()[0] == 14 + assert counts(binding_events)[0:2] == (1, 0) + finally: + other.close() + + +def test_multiple_cursors_and_sequential_thread_handoff(db_connection, binding_events): + first, second = db_connection.cursor(), db_connection.cursor() + try: + assert first.execute("SELECT ?", [12]).fetchall()[0][0] == 12 + assert second.execute("SELECT ?", [34]).fetchall()[0][0] == 34 + binding_events.clear() + # No simultaneous connection/cursor use: this only checks that storage + # belongs to the handle, rather than a thread-local raw-handle map. + with ThreadPoolExecutor(max_workers=1) as executor: + result = executor.submit(lambda: first.execute("SELECT ?", [56]).fetchall()[0][0]) + assert result.result() == 56 + assert second.execute("SELECT ?", [78]).fetchall()[0][0] == 78 + assert counts(binding_events) == (0, 2, 0) + finally: + first.close() + second.close() + + +def test_connection_close_with_retained_statement(conn_str): + connection = mssql_python.connect(conn_str) + cursor = connection.cursor() + cursor.execute("SELECT ?", ["owned"]).fetchone() + handle = cursor.hstmt + connection.close() + assert ddbc_bindings.DDBCSQLResetStmt(handle) == SQL.SQL_INVALID_HANDLE.value + handle.free() + cursor.close() + + +def test_native_char_encoding_and_codec_failure(cursor, binding_events): + # SQL_C_CHAR in the Python constants is historically -8. Use the actual + # native C type (1) through the existing normalized input-size representation. + sizes = [(SQL.SQL_VARCHAR.value, 1, 100, 0)] + for value, reused in [("abc", False), ("def", True), ("longer", False), ("short!", True)]: + cursor._inputsizes = sizes + binding_events.clear() + assert cursor.execute("SELECT ?", [value]).fetchone()[0] == value + assert counts(binding_events)[0:2] == ((0, 1) if reused else (1, 0)) + + class FailingString(str): + def encode(self, *args, **kwargs): + raise UnicodeError("test codec failure") + + cursor._inputsizes = sizes + with pytest.raises(RuntimeError, match="test codec failure"): + cursor.execute("SELECT ?", [FailingString("failed")]) + cursor._inputsizes = sizes + binding_events.clear() + assert cursor.execute("SELECT ?", ["latest"]).fetchone()[0] == "latest" + assert counts(binding_events)[0:2] == (1, 0) + + +def test_numeric_text_precision_and_scale_changes(cursor, binding_events): + for precision, scale, reused in [(10, 2, False), (10, 2, True), (12, 3, False), (12, 3, True)]: + cursor.setinputsizes([(SQL.SQL_DECIMAL.value, precision, scale)]) + binding_events.clear() + result = cursor.execute("SELECT ?", [decimal.Decimal("12.34")]).fetchone()[0] + assert result == decimal.Decimal("12.34") + assert counts(binding_events)[0:2] == ((0, 1) if reused else (1, 0)) + + +@pytest.mark.parametrize("operation", ["catalog", "direct", "attribute"]) +def test_native_invalidation_surfaces(cursor, binding_events, operation): + cursor.execute("SELECT ?", [12]).fetchall() + handle = cursor.hstmt + handle._close_cursor() + if operation == "catalog": + rc = ddbc_bindings.DDBCSQLGetTypeInfo(handle, SQL.SQL_INTEGER.value) + elif operation == "direct": + rc = ddbc_bindings.DDBCSQLExecDirect(handle, "SELECT 99") + else: + rc = ddbc_bindings.DDBCSQLSetStmtAttr(handle, SQL.SQL_ATTR_QUERY_TIMEOUT.value, 0) + assert rc in (SQL.SQL_SUCCESS.value, SQL.SQL_SUCCESS_WITH_INFO.value) + handle._close_cursor() + cursor.is_stmt_prepared = [False] + binding_events.clear() + assert cursor.execute("SELECT ?", [34]).fetchone()[0] == 34 + assert counts(binding_events)[0:2] == (1, 0) + + +def test_raw_free_and_shutdown_with_cached_bindings(conn_str): + code = textwrap.dedent(""" + import os + import mssql_python + from mssql_python import ddbc_bindings as native + from mssql_python.constants import ConstantsDDBC as SQL + + connection = mssql_python.connect(os.environ["DB_CONNECTION_STRING"]) + cursor = connection.cursor() + cursor.execute("SELECT ?", [12]).fetchall() + handle = cursor.hstmt + assert native.DDBCSQLFreeHandle(SQL.SQL_HANDLE_STMT.value, handle) == 0 + cursor.close() + other = connection.cursor() + for value in [34, 56, 78]: + assert other.execute("SELECT ?", [value]).fetchall()[0][0] == value + # Exercise atexit cleanup while native bindings and Python handles live. + """) + result = subprocess.run( + [sys.executable, "-c", code], + env=os.environ.copy(), + capture_output=True, + text=True, + timeout=30, + ) + assert result.returncode == 0, result.stderr + + +def test_cache_owns_no_python_values(cursor, binding_events): + class Text(str): + pass + + for value in ("abc", "def"): + text = Text(value) + reference = weakref.ref(text) + binding_events.clear() + cursor.execute("SELECT ?", [text]) + del text + gc.collect() + assert reference() is None + assert cursor.fetchone()[0] == value + if value == "def": + assert counts(binding_events) == (0, 1, 0) + + +def test_native_array_execution_invalidates_same_handle(cursor, binding_events): + cursor.execute("CREATE TABLE #cached_native_array (value int)") + sql = "INSERT INTO #cached_native_array VALUES (?)" + cursor.execute(sql, [12]) + handle = cursor.hstmt + handle._close_cursor() + info = ddbc_bindings.ParamInfo() + info.inputOutputType = 1 + info.paramCType = 4 # SQL_C_LONG + info.paramSQLType = 4 # SQL_INTEGER + info.columnSize = 10 + info.decimalDigits = 0 + rc = ddbc_bindings.SQLExecuteMany(handle, sql, [[34, 56]], [info], 2, {}) + assert rc in (SQL.SQL_SUCCESS.value, SQL.SQL_SUCCESS_WITH_INFO.value) + binding_events.clear() + cursor.execute(sql, [78]) + assert cursor.hstmt is handle + assert counts(binding_events)[0:2] == (1, 0) + cursor.execute("SELECT value FROM #cached_native_array ORDER BY value") + assert [row[0] for row in cursor.fetchall()] == [12, 34, 56, 78] + cursor.execute("DROP TABLE #cached_native_array") From f1323a92908d05e20610bc44a4032cfc8cd8a767 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 9 Sep 2026 09:54:49 +0530 Subject: [PATCH 2/3] FIX: mark child statements implicitly freed even when SQLDisconnect fails Connection::disconnect() gated marking the tracked child statement handles as implicitly freed on SQLDisconnect returning success, but the parent DBC handle is reset unconditionally right afterwards. Freeing the DBC frees every child statement handle, so when SQLDisconnect returned a non-success code (for example on Windows when a cursor outlives its connection and statements are still open) the children were never marked, and later cursor garbage collection called SQLFreeHandle on an already-freed handle, faulting with an access violation. Mark every retained child implicitly freed and release its native binding storage unconditionally after SQLDisconnect returns, before the DBC handle is reset. Owning references are still held across the blocking disconnect so the cached binding buffers stay valid until the call completes, preserving the reuse feature's lifetime guarantee while restoring the double-free protection that predated it. Also give the large parameter cases in test_037_cached_bindings.py explicit short parametrize ids so the multi-thousand-character values no longer expand into node ids that exceed the Windows 32767-character environment limit during collection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/connection/connection.cpp | 25 ++++++++++++------- tests/test_037_cached_bindings.py | 11 ++++++++ 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 553d22e21..14dd33036 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -113,14 +113,17 @@ void Connection::disconnect() { LOG("Disconnecting from database"); } - // SQLDisconnect frees child statements. Retain their owners until it - // succeeds, then mark them so later cursor cleanup cannot double-free. + // Freeing the DBC handle below frees every child statement, so the + // children must be marked implicitly freed regardless of whether + // SQLDisconnect itself succeeds. Retain owning references only so the + // native binding buffers stay valid through the blocking disconnect; + // the marking and release happen unconditionally afterwards. // THREAD-SAFETY: Lock mutex to safely access _childStatementHandles // This protects against concurrent allocStatementHandle() calls or GC finalizers size_t originalSize = 0, afterCompactSize = 0, badHandleCount = 0; - // Keep statement buffers alive through the parent's blocking disconnect. - // Do not mark children freed until SQLDisconnect actually succeeds. + // Owning references held only to keep statement buffers alive through the + // parent's blocking disconnect; ownership is released immediately after. std::vector childHandles; { std::lock_guard lock(_childHandlesMutex); @@ -180,11 +183,15 @@ void Connection::disconnect() { // via py::gil_scoped_acquire, which is unsafe during interpreter // shutdown or stack unwinding (can deadlock or call std::terminate). } - if (SQL_SUCCEEDED(ret)) { - for (const auto& handle : childHandles) { - handle->markImplicitlyFreed(); - handle->releaseAfterFree(); - } + // The DBC handle is freed unconditionally below, which frees every child + // statement. Mark all children implicitly freed and drop their native + // binding storage now, whether or not SQLDisconnect reported success, so + // later cursor cleanup can never double-free an already-freed handle. + for (const auto& handle : childHandles) { + handle->markImplicitlyFreed(); + handle->releaseAfterFree(); + } + { std::lock_guard lock(_childHandlesMutex); _childStatementHandles.clear(); _allocationsSinceCompaction = 0; diff --git a/tests/test_037_cached_bindings.py b/tests/test_037_cached_bindings.py index 8de24b702..c7a307a8f 100644 --- a/tests/test_037_cached_bindings.py +++ b/tests/test_037_cached_bindings.py @@ -163,6 +163,17 @@ def test_encoding_changes_rebind(cursor, db_connection, binding_events): b"\0" * 9000, bytearray(b"x" * 9000), ], + ids=[ + "none", + "decimal", + "date", + "datetime", + "uuid", + "long-ascii", + "long-emoji", + "long-null-bytes", + "long-bytearray", + ], ) def test_uncached_shapes_fall_back_and_recover(cursor, binding_events, value): cursor.execute("SELECT ?", [12]).fetchone() From 0cc4c7c1844d9b6e373486f47b901bae65dfe600 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Wed, 9 Sep 2026 12:17:09 +0530 Subject: [PATCH 3/3] REFACTOR: move native parameter-binding cache into param_bind_cache.hpp The handle-owned parameter-binding reuse cache was added inline to the ~6,000 line ddbc_bindings.cpp monolith and its struct definitions to ddbc_bindings.h. It is a self-contained unit, so move it into its own single-responsibility header, matching how the perf work already carved parameter detection into param_detect.hpp, the type cache into py_type_cache.hpp, and the refcount helpers into py_ref.hpp. param_bind_cache.hpp now owns ParameterBinding, ExecuteBindingCache, ExecuteParamBuffers, the single-buffer AllocateParamBuffer template and its in-place-reuse overload, UpdateParamBuffer, SameParameterShape, and CanCacheParameters. ddbc_bindings.h keeps only a forward declaration of ExecuteBindingCache, which is all SqlHandle needs since it holds a unique_ptr to it and defines every method that touches it out of line; this also avoids pulling param_detect.hpp (which includes ddbc_bindings.h back) into the handle header. AllocateParamBufferArray stays in ddbc_bindings.cpp because it serves the executemany array-binding path, not the reuse cache. Pure relocation with no behavior change: the moved code is byte-identical, the release universal2 build is clean, and the full suite is unchanged (2344 passed, the lone test_004 stored-procedure failure is a pre-existing stale-DB-object artifact that passes in isolation). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- mssql_python/pybind/ddbc_bindings.cpp | 97 +------------- mssql_python/pybind/ddbc_bindings.h | 24 +--- mssql_python/pybind/param_bind_cache.hpp | 153 +++++++++++++++++++++++ 3 files changed, 164 insertions(+), 110 deletions(-) create mode 100644 mssql_python/pybind/param_bind_cache.hpp diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp index 7529f3442..ccaa0cab6 100644 --- a/mssql_python/pybind/ddbc_bindings.cpp +++ b/mssql_python/pybind/ddbc_bindings.cpp @@ -9,6 +9,7 @@ #include "connection/connection_pool.h" #include "logger_bridge.hpp" #include "param_detect.hpp" +#include "param_bind_cache.hpp" #include "py_ref.hpp" #include "py_type_cache.hpp" #include "utf_utils.h" @@ -305,97 +306,11 @@ std::string MakeParamMismatchErrorStr(const SQLSMALLINT cType, const int paramIn return errorString; } -// This function allocates a buffer of ParamType, stores it as a void* in -// paramBuffers for book-keeping and then returns a ParamType* to the allocated -// memory. ctorArgs are the arguments to ParamType's constructor used while -// creating/allocating ParamType -template -ParamType* AllocateParamBuffer(std::vector>& paramBuffers, - CtorArgs&&... ctorArgs) { - paramBuffers.emplace_back(new ParamType(std::forward(ctorArgs)...), - std::default_delete()); - LOG("AllocateParamBuffer: New owned buffer"); - return static_cast(paramBuffers.back().get()); -} - -struct ExecuteParamBuffers { - std::vector>& current; - const std::vector>* previous; -}; - -template -static bool UpdateParamBuffer(T& target, T&& value) { - target = std::move(value); - return true; -} - -template -static bool UpdateParamBuffer(std::basic_string& target, std::basic_string&& value) { - // Equal byte lengths keep both the address and ODBC BufferLength unchanged. - // Do not assign a string: even a capacity-preserving assignment may move it. - if (target.size() != value.size()) - return false; - std::copy(value.begin(), value.end(), target.begin()); - return true; -} - -template -ParamType* AllocateParamBuffer(ExecuteParamBuffers& buffers, CtorArgs&&... ctorArgs) { - ParamType value(std::forward(ctorArgs)...); - const size_t index = buffers.current.size(); - if (buffers.previous && index < buffers.previous->size()) { - const auto& previous = (*buffers.previous)[index]; - auto* target = static_cast(previous.get()); - if (UpdateParamBuffer(*target, std::move(value))) { - buffers.current.push_back(previous); - return target; - } - } - return AllocateParamBuffer(buffers.current, std::move(value)); -} - -static bool SameParameterShape(const ParameterBinding& binding, const ParamInfo& info) { - return binding.direction == info.inputOutputType && binding.cType == info.paramCType && - binding.sqlType == info.paramSQLType && binding.columnSize == info.columnSize && - binding.scale == info.decimalDigits; -} - -static bool CanCacheParameters(const std::vector& infos) { - // Bound retained storage to SQL Server's scalar parameter limit. NULL/DAE and - // descriptor-based/complex types deliberately use the existing uncached path. - if (infos.empty() || infos.size() > 2100) - return false; - for (const auto& info : infos) { - if (info.isDAE || info.inputOutputType != SQL_PARAM_INPUT) - return false; - switch (info.paramCType) { - case SQL_C_CHAR: - case SQL_C_WCHAR: - case SQL_C_BINARY: - if (info.columnSize > MAX_INLINE_BINARY) - return false; - break; - case SQL_C_BIT: - case SQL_C_STINYINT: - case SQL_C_TINYINT: - case SQL_C_SSHORT: - case SQL_C_SHORT: - case SQL_C_UTINYINT: - case SQL_C_USHORT: - case SQL_C_SBIGINT: - case SQL_C_SLONG: - case SQL_C_LONG: - case SQL_C_UBIGINT: - case SQL_C_ULONG: - case SQL_C_FLOAT: - case SQL_C_DOUBLE: - break; - default: - return false; - } - } - return true; -} +// The single-buffer AllocateParamBuffer template, its reuse overload, the +// ParameterBinding / ExecuteBindingCache / ExecuteParamBuffers types, +// UpdateParamBuffer, SameParameterShape, and CanCacheParameters live in +// param_bind_cache.hpp. AllocateParamBufferArray below serves the executemany +// array-binding path and is unrelated to the reuse cache, so it stays here. template ParamType* AllocateParamBufferArray(std::vector>& paramBuffers, diff --git a/mssql_python/pybind/ddbc_bindings.h b/mssql_python/pybind/ddbc_bindings.h index b3cd67d87..2bcdcb4d7 100644 --- a/mssql_python/pybind/ddbc_bindings.h +++ b/mssql_python/pybind/ddbc_bindings.h @@ -280,25 +280,11 @@ struct DescribedParamInfo { SQLSMALLINT decimalDigits; }; -struct ParameterBinding { - SQLSMALLINT direction; - SQLSMALLINT cType; - SQLSMALLINT sqlType; - SQLULEN columnSize; - SQLSMALLINT scale; - SQLPOINTER data; - SQLLEN length; - SQLLEN* indicator; -}; - -// Native-only ownership: cleanup is safe even on GIL-less connection teardown. -// One generation per statement, never keyed by a recycled raw ODBC handle. -struct ExecuteBindingCache { - std::vector bindings; - std::vector> buffers; - std::string encoding; - bool reusable = false; -}; +// Handle-owned cache of native parameter bindings, defined in +// param_bind_cache.hpp. SqlHandle only holds a unique_ptr to it and defines +// every method that touches it out of line, so a forward declaration is enough +// here and avoids pulling param_detect.hpp into this header (it includes back). +struct ExecuteBindingCache; class SqlHandle { public: diff --git a/mssql_python/pybind/param_bind_cache.hpp b/mssql_python/pybind/param_bind_cache.hpp new file mode 100644 index 000000000..0503b359a --- /dev/null +++ b/mssql_python/pybind/param_bind_cache.hpp @@ -0,0 +1,153 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +// param_bind_cache.hpp — handle-owned reuse of native parameter bindings. +// +// Owns the data model and helpers that let a re-executed prepared statement skip +// the SQLBindParameter loop when its parameter shape has not changed: +// +// DetectParamTypes -> BindParameters -> SQLExecute +// (param_detect.hpp) (ddbc_bindings.cpp, uses this) +// +// A SqlHandle keeps one ExecuteBindingCache: its single generation of native +// input buffers plus the exact SQLBindParameter arguments ODBC currently holds. +// On the next execute, if every parameter presents identical binding metadata +// and the freshly rebuilt buffers land at the same addresses and byte lengths, +// the bind loop is skipped. The reuse is native-only: the cache holds C++ +// storage (std::string / numeric buffers), never a Python object, so teardown +// is safe even on the GIL-less connection-destruction path. The reuse decision +// and the byte-exact verification that gates the skip live in BindParameters in +// ddbc_bindings.cpp; this header is only the data model and the small predicates +// and buffer helpers it depends on. +// +// Header-only, like param_detect.hpp: the helpers run once per parameter per +// execute, and the build compiles with -O3 but without LTO, so keeping them +// inline in the using translation unit avoids turning inlined code into real +// calls across a .cpp boundary on the hot path. + +#pragma once + +#include +#include +#include +#include +#include +#include + +#include "param_detect.hpp" // ParamInfo, MAX_INLINE_BINARY, ODBC types and constants + +// One entry per bound parameter: the exact SQLBindParameter arguments ODBC holds. +struct ParameterBinding { + SQLSMALLINT direction; + SQLSMALLINT cType; + SQLSMALLINT sqlType; + SQLULEN columnSize; + SQLSMALLINT scale; + SQLPOINTER data; + SQLLEN length; + SQLLEN* indicator; +}; + +// Native-only ownership: cleanup is safe even on GIL-less connection teardown. +// One generation per statement, never keyed by a recycled raw ODBC handle. +struct ExecuteBindingCache { + std::vector bindings; + std::vector> buffers; + std::string encoding; + bool reusable = false; +}; + +// Allocate a ParamType buffer, own it as a void* in paramBuffers for book-keeping, +// and return a typed pointer to it. ctorArgs forward to ParamType's constructor. +// The reuse overload below extends this; keep both together so the header is +// self-contained and the reuse overload sees the base definition, not just a +// declaration, at instantiation. +template +ParamType* AllocateParamBuffer(std::vector>& paramBuffers, + CtorArgs&&... ctorArgs) { + paramBuffers.emplace_back(new ParamType(std::forward(ctorArgs)...), + std::default_delete()); + LOG("AllocateParamBuffer: New owned buffer"); + return static_cast(paramBuffers.back().get()); +} + +// Current-generation buffers being built, plus the previous generation to reuse +// in place when a buffer's byte size is unchanged. +struct ExecuteParamBuffers { + std::vector>& current; + const std::vector>* previous; +}; + +template +static bool UpdateParamBuffer(T& target, T&& value) { + target = std::move(value); + return true; +} + +template +static bool UpdateParamBuffer(std::basic_string& target, std::basic_string&& value) { + // Equal byte lengths keep both the address and ODBC BufferLength unchanged. + // Do not assign a string: even a capacity-preserving assignment may move it. + if (target.size() != value.size()) + return false; + std::copy(value.begin(), value.end(), target.begin()); + return true; +} + +template +ParamType* AllocateParamBuffer(ExecuteParamBuffers& buffers, CtorArgs&&... ctorArgs) { + ParamType value(std::forward(ctorArgs)...); + const size_t index = buffers.current.size(); + if (buffers.previous && index < buffers.previous->size()) { + const auto& previous = (*buffers.previous)[index]; + auto* target = static_cast(previous.get()); + if (UpdateParamBuffer(*target, std::move(value))) { + buffers.current.push_back(previous); + return target; + } + } + return AllocateParamBuffer(buffers.current, std::move(value)); +} + +static bool SameParameterShape(const ParameterBinding& binding, const ParamInfo& info) { + return binding.direction == info.inputOutputType && binding.cType == info.paramCType && + binding.sqlType == info.paramSQLType && binding.columnSize == info.columnSize && + binding.scale == info.decimalDigits; +} + +static bool CanCacheParameters(const std::vector& infos) { + // Bound retained storage to SQL Server's scalar parameter limit. NULL/DAE and + // descriptor-based/complex types deliberately use the existing uncached path. + if (infos.empty() || infos.size() > 2100) + return false; + for (const auto& info : infos) { + if (info.isDAE || info.inputOutputType != SQL_PARAM_INPUT) + return false; + switch (info.paramCType) { + case SQL_C_CHAR: + case SQL_C_WCHAR: + case SQL_C_BINARY: + if (info.columnSize > MAX_INLINE_BINARY) + return false; + break; + case SQL_C_BIT: + case SQL_C_STINYINT: + case SQL_C_TINYINT: + case SQL_C_SSHORT: + case SQL_C_SHORT: + case SQL_C_UTINYINT: + case SQL_C_USHORT: + case SQL_C_SBIGINT: + case SQL_C_SLONG: + case SQL_C_LONG: + case SQL_C_UBIGINT: + case SQL_C_ULONG: + case SQL_C_FLOAT: + case SQL_C_DOUBLE: + break; + default: + return false; + } + } + return true; +}