PERF: reuse native parameter bindings across repeated executes - #761
Draft
Gaurav Sharma (bewithgaurav) wants to merge 5 commits into
Draft
Gaurav Sharma (bewithgaurav) wants to merge 5 commits into
Gaurav Sharma (bewithgaurav) wants to merge 5 commits into
Conversation
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>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 8, 2026 18:53
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The updated disconnect path can still drop the DBC handle in the no-GIL shutdown/destructor scenario without marking child statements as implicitly freed on SQLDisconnect failure, risking later double-free/ODBC calls on implicitly-freed HSTMTs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a native, per-statement cache for execute-time parameter bindings in the C++ (ddbc_bindings) layer so repeated executions of the same prepared SQL can skip redundant SQLBindParameter calls when the binding “shape” is unchanged, while still re-running detection/validation/conversion each time.
Changes:
- Add
ExecuteBindingCache+preparedQuerytracking onSqlHandle, and implement reuse/invalidations acrossSQLExecute, reset, catalog, and direct execution paths. - Update Python cursor soft-reset to preserve compatible cached bindings (
preserve_bindings=True). - Add an integration test suite that asserts reuse/invalidation behavior via existing debug-log events, plus documentation in the native README.
File summaries
| File | Description |
|---|---|
| tests/test_037_cached_bindings.py | New integration tests validating bind reuse/invalidation via native debug logging. |
| mssql_python/pybind/README.md | Document the repeated-execute binding reuse rules and invalidation triggers. |
| mssql_python/pybind/ddbc_bindings.h | Add binding-cache structs and new SqlHandle fields/methods for caching + reset. |
| mssql_python/pybind/ddbc_bindings.cpp | Implement buffer reuse/caching, invalidation, and reset behavior across execution paths. |
| mssql_python/pybind/connection/connection.cpp | Adjust disconnect sequencing/lifetime handling for child statement owners and caches. |
| mssql_python/cursor.py | Preserve compatible cached bindings during soft cursor reset. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ails 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>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 9, 2026 04:25
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Connection::disconnect() can drop tracking for statement handles allocated concurrently during the unlocked SQLDisconnect call, risking later double-free behavior unless the teardown captures/marks all handles before clearing the tracking list.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 447-455 447 // each of them with appropriate arguments
448 SQLRETURN BindParameters(SqlHandle& handle, SQLHANDLE hStmt, const py::list& params,
449 std::vector<ParamInfo>& paramInfos,
450 std::vector<std::shared_ptr<void>>& ownedBuffers,
! 451 const std::string& charEncoding = "utf-8", bool cacheForExecute = false) {
452 PERF_TIMER("BindParameters");
453 LOG("BindParameters: Starting parameter binding for statement handle %p "
454 "with %zu parameters",
455 (void*)hStmt, params.size());Lines 911-921 911 handle.executeBindings = std::move(cache);
912 }
913 for (int paramIndex = 0; paramIndex < bindings.size(); ++paramIndex) {
914 const ParamInfo& paramInfo = paramInfos[paramIndex];
! 915 const auto& binding = bindings[paramIndex];
! 916 void* dataPtr = binding.data;
! 917 SQLLEN bufferLength = binding.length;
918 SQLLEN* strLenOrIndPtr = binding.indicator;
919 assert(SQLBindParameter_ptr && SQLGetStmtAttr_ptr && SQLSetDescField_ptr);
920 LOG("BindParameters: SQLBindParameter param[%d]", paramIndex);
921 RETCODE rc;Lines 1609-1617 1609 // If the driver refused to free the handle, it can still reference these
1610 // addresses. Leak only this failed teardown's native storage, not dangling
1611 // pointers into freed memory. Explicit free() failures retain ownership.
1612 if (_handle && !_implicitly_freed)
! 1613 executeBindings.release();
1614 }
1615
1616 SQLHANDLE SqlHandle::get() const {
1617 return _handle;Lines 1640-1649 1640 SQLRETURN SqlHandle::resetParameterBindings() {
1641 if (!executeBindings)
1642 return SQL_SUCCESS;
1643 executeBindings->reusable = false;
! 1644 if (!_handle || _implicitly_freed)
! 1645 return SQL_INVALID_HANDLE;
1646 SQLRETURN rc = SQLFreeStmt_ptr(_handle, SQL_RESET_PARAMS);
1647 if (SQL_SUCCEEDED(rc))
1648 executeBindings.reset();
1649 return rc;Lines 1647-1659 1647 if (SQL_SUCCEEDED(rc))
1648 executeBindings.reset();
1649 return rc;
1650 }
! 1651
! 1652 void SqlHandle::releaseAfterFree() {
1653 _handle = nullptr;
! 1654 executeBindings.reset();
! 1655 preparedQuery.clear();
1656 describeCache.clear();
1657 }
1658
1659 /*Lines 1706-1714 1706 if (!pythonShuttingDown && PyGILState_Check()) {
1707 py::gil_scoped_release release;
1708 rc = SQLFreeHandle_ptr(_type, _handle);
1709 } else {
! 1710 rc = SQLFreeHandle_ptr(_type, _handle);
1711 }
1712 if (SQL_SUCCEEDED(rc)) {
1713 releaseAfterFree();
1714 } else if (executeBindings) {Lines 1808-1817 1808 {
1809 py::gil_scoped_release release;
1810 rc = SQLFreeStmt_ptr(hStmt, SQL_CLOSE);
1811 if (SQL_SUCCEEDED(rc) && !(preserveBindings && statementHandle->executeBindings &&
! 1812 statementHandle->executeBindings->reusable)) {
! 1813 if (statementHandle->executeBindings) {
1814 rc = statementHandle->resetParameterBindings();
1815 } else {
1816 rc = SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS);
1817 }Lines 1845-1854 1845 SQLRETURN SQLProcedures_wrap(SqlHandlePtr StatementHandle, const py::object& catalogObj,
1846 const py::object& schemaObj, const py::object& procedureObj) {
1847 PERF_TIMER("SQLProcedures_wrap");
1848 SQLRETURN reset = StatementHandle->resetParameterBindings();
! 1849 if (!SQL_SUCCEEDED(reset))
! 1850 return reset;
1851 StatementHandle->preparedQuery.clear();
1852 StatementHandle->clearDescribeCache();
1853 if (!SQLProcedures_ptr) {
1854 ThrowStdException("SQLProcedures function not loaded");Lines 1873-1881 1873 const py::object& pkSchemaObj, const py::object& pkTableObj,
1874 const py::object& fkCatalogObj, const py::object& fkSchemaObj,
1875 const py::object& fkTableObj) {
1876 PERF_TIMER("SQLForeignKeys_wrap");
! 1877 SQLRETURN reset = StatementHandle->resetParameterBindings();
1878 if (!SQL_SUCCEEDED(reset))
1879 return reset;
1880 StatementHandle->preparedQuery.clear();
1881 StatementHandle->clearDescribeCache();Lines 1938-1948 1938 const py::object& schemaObj, const std::u16string& table,
1939 SQLUSMALLINT unique, SQLUSMALLINT reserved) {
1940 PERF_TIMER("SQLStatistics_wrap");
1941 SQLRETURN reset = StatementHandle->resetParameterBindings();
! 1942 if (!SQL_SUCCEEDED(reset))
! 1943 return reset;
! 1944 StatementHandle->preparedQuery.clear();
1945 StatementHandle->clearDescribeCache();
1946 if (!SQLStatistics_ptr) {
1947 ThrowStdException("SQLStatistics function not loaded");
1948 }Lines 1967-1976 1967 PERF_TIMER("SQLColumns_wrap");
1968 SQLRETURN reset = StatementHandle->resetParameterBindings();
1969 if (!SQL_SUCCEEDED(reset))
1970 return reset;
! 1971 StatementHandle->preparedQuery.clear();
! 1972 StatementHandle->clearDescribeCache();
1973 if (!SQLColumns_ptr) {
1974 ThrowStdException("SQLColumns function not loaded");
1975 }Lines 2180-2188 2180 if (!statementHandle || !statementHandle->get() || statementHandle->isImplicitlyFreed()) {
2181 return SQL_INVALID_HANDLE;
2182 }
2183
! 2184 struct ExecutionAttempt {
2185 SqlHandle& handle;
2186 bool succeeded = false;
2187 ~ExecutionAttempt() {
2188 if (!succeeded && handle.executeBindings)Lines 2243-2254 2243 // - use_prepare=false + already prepared: reuse existing plan
2244 // - use_prepare=false + not prepared: error (cannot execute unprepared)
2245 if (!already_prepared) {
2246 if (use_prepare) {
! 2247 rc = statementHandle->resetParameterBindings();
! 2248 if (!SQL_SUCCEEDED(rc))
! 2249 return rc;
! 2250 statementHandle->preparedQuery.clear();
2251 SQLWCHAR* queryPtr = reinterpretU16stringAsSqlWChar(query);
2252 {
2253 py::gil_scoped_release release;
2254 rc = SQLPrepare_ptr(hStmt, queryPtr, SQL_NTS);Lines 2364-2373 2364 if (!statementHandle->executeBindings->reusable) {
2365 rc = statementHandle->resetParameterBindings();
2366 if (!SQL_SUCCEEDED(rc))
2367 return rc;
! 2368 }
! 2369 attempt.succeeded = true;
2370 return exec_rc;
2371 }
2372
2373 SQLRETURN BindParameterArray(SqlHandle& handle, SQLHANDLE hStmt, const py::list& columnwise_params,mssql_python/pybind/param_bind_cache.hppLines 119-127 119 static bool CanCacheParameters(const std::vector<ParamInfo>& infos) {
120 // Bound retained storage to SQL Server's scalar parameter limit. NULL/DAE and
121 // descriptor-based/complex types deliberately use the existing uncached path.
122 if (infos.empty() || infos.size() > 2100)
! 123 return false;
124 for (const auto& info : infos) {
125 if (info.isDAE || info.inputOutputType != SQL_PARAM_INPUT)
126 return false;
127 switch (info.paramCType) {Lines 128-136 128 case SQL_C_CHAR:
129 case SQL_C_WCHAR:
130 case SQL_C_BINARY:
131 if (info.columnSize > MAX_INLINE_BINARY)
! 132 return false;
133 break;
134 case SQL_C_BIT:
135 case SQL_C_STINYINT:
136 case SQL_C_TINYINT:📋 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: 61.5%
mssql_python.row.py: 77.6%
mssql_python.pybind.ddbc_bindings.cpp: 78.9%
mssql_python.pybind.connection.connection_pool.cpp: 81.8%
mssql_python.pybind.connection.connection.cpp: 85.2%
mssql_python.logging.py: 85.5%
mssql_python.helpers.py: 89.3%
mssql_python.pooling.py: 90.1%🔗 Quick Links
|
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>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 9, 2026 06:47
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Connection teardown currently drops cached parameter buffers before the parent DBC handle is freed, which can invalidate still-referenced bound addresses during ODBC cleanup.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
mssql_python/pybind/ddbc_bindings.h:313
- The markImplicitlyFreed() doc comment says it must only be called after SQLDisconnect has succeeded, but Connection::disconnect() calls it regardless of SQLDisconnect result (and then frees the DBC handle anyway). This makes the contract misleading for future callers.
// SAFETY CONSTRAINTS:
// - ONLY call this on SQL_HANDLE_STMT handles
// - ONLY call this after the parent SQLDisconnect has succeeded
// - Calling on other handle types (ENV, DBC, DESC) will cause HANDLE LEAKS
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
Merge main at 9122376, including profiler PR #552 and the other upstream updates. Preserve the cached-binding implementation and header extraction while resolving overlapping instrumentation in ddbc_bindings.cpp. Keep the upstream timers around the actual ODBC bind calls and retain the cache invalidation and handle guards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 10, 2026 11:40
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new cache header currently relies on include order for the LOG macro and there are updated comments that conflict with the actual disconnect/implicit-free control flow.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
mssql_python/pybind/param_bind_cache.hpp:37
param_bind_cache.hppuses theLOGmacro inside the header, but the header does not includelogger_bridge.hpp(whereLOGis defined). This makes the header dependent on include order and can break compilation if it’s included from another translation unit later.
#include "param_detect.hpp" // ParamInfo, MAX_INLINE_BINARY, ODBC types and constants
mssql_python/pybind/ddbc_bindings.h:312
- The safety constraint for
markImplicitlyFreed()is too strict given current usage:Connection::disconnect()can call it in GIL-less/shutdown paths even whenSQLDisconnectreturns an error (errors are suppressed), as long as the DBC handle is still being freed. The comment should match the actual requirement (parent DBC handle will be freed).
// - ONLY call this after the parent SQLDisconnect has succeeded
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
Distinguish successful SQLDisconnect from GIL-held errors and terminal GIL-less wrapper retirement. Remove comments implying that dropping a DBC owner always proves native deallocation. Include logger_bridge.hpp directly in the cache header instead of relying on a transitive include. Runtime cleanup and binding behavior are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 10, 2026 12:59
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current SQLExecute_wrap can turn a successful execute into a failure due to post-execute cleanup error propagation, which is an observable behavior change that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
2363
to
+2367
| SQLRETURN exec_rc = rc; | ||
| SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS); | ||
| if (!statementHandle->executeBindings->reusable) { | ||
| rc = statementHandle->resetParameterBindings(); | ||
| if (!SQL_SUCCEEDED(rc)) | ||
| return rc; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
Repeated executions of the same prepared statement rebind every parameter from scratch on each call, even when the parameter shape never changes. This change gives each statement handle a single reusable generation of native input buffers and skips the
SQLBindParameterloop 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. Only the redundant rebinding is elided. 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, which preserves each buffer address and its ODBC
BufferLength.Scope is deliberately conservative:
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=1contract is unchanged. This is an internal reuse optimization with no public API or behavior change.Validation
macOS arm64, Python 3.13, release build (
-O3 -DNDEBUG, universal2), SQL Server 2022.tests/test_037_cached_bindings.py: 73 reuse and invalidation cases pass. These assert live round trips and the actual native allocate/bind/reuse events through the existing debug logger, not a test-only API.test_004_cursor,test_010_pybind_functions,test_023_execute_path_parity): 699 pass, 8 skipped, no regressions.Preliminary Local Measurements
Interleaved A/B against the
cb0bd6cbaseline (alternating before/after so machine noise lands on both), median of per-scenario medians across 3 rounds, with the worst round-to-round swing shown as spread.Every delta is smaller than its own spread, so this wall-clock harness cannot resolve a difference on this machine. That is expected: these are end-to-end INSERT workloads where the SQL Server round trip dominates, and the client-side rebinding this change removes is a small fraction of each call. These numbers are published only to show the change does not regress, not to claim a speedup.
The isolated client-side effect will be quantified with the native profiler (tracked separately in #552) rather than end-to-end wall clock, and the measured numbers will be added here before this PR leaves draft.