PERF: Guard disabled debug logging in fetchval only - #823
Open
Jahnvi Thakkar (jahnvi480) wants to merge 3 commits into
Open
Jahnvi Thakkar (jahnvi480) wants to merge 3 commits into
Jahnvi Thakkar (jahnvi480) wants to merge 3 commits into
Conversation
Guard each fetchval debug call with the current cached debug flag and strengthen the existing fetchval basic-functionality test. Leave other fetch APIs and Row/native code unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
PR Performance Report✅ No regression detectedNo consistent slowdowns detected across all 2 environments. 0 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS Coverage: 2 of 2 environments completed. Advisory result; does not block merging. Performance diagnosticsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. No affected phases or call-count changes were recorded. All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build and measurement detailsPR head:
A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. Raw samples and logs are attached to the ADO run as |
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 28, 2026 10:31
View session
Jahnvi Thakkar (jahnvi480)
marked this pull request as ready for review
September 28, 2026 10:32
Jahnvi Thakkar (jahnvi480)
enabled auto-merge (squash)
September 28, 2026 10:32
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Broader performance qualification and unresolved control results remain.
Review effort: Lite
Findings: None
What changed in this PR
This PR guards disabled debug logging in Cursor.fetchval() and strengthens related regression tests.
Changes:
- Adds four per-call debug guards.
- Expands tests for logging states, messages, and flag transitions.
- Preserves fetch, result, EOF, and non-result behavior.
| File | Description |
|---|---|
tests/test_004_cursor.py |
Verifies fetchval logging and behavior. |
mssql_python/cursor.py |
Guards each fetchval debug call. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Jahnvi Thakkar (jahnvi480)
changed the title
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 62.6%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 79.3%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 83.1%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.fetch_temporal.hpp: 92.1%🔗 Quick Links
|
Measure 10000 scalar fetches and EOF with debug logging disabled. Register one report workload, validate its timing and result contract, and keep existing production code and comparison thresholds unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
pr-size: medium
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 28, 2026 12:13
View session
Jahnvi Thakkar (jahnvi480)
requested review from
Gaurav Sharma (bewithgaurav) and
gargsaumya
September 29, 2026 07:59
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
October 1, 2026 10:15
View session
| ) | ||
| result.update((name, (partial(query, sql=sql), False)) for name, sql in QUERIES.items()) | ||
| result["lob_varchar_256k_fetchall"] = (lob_fetch, False) | ||
| result["scalar_fetchval"] = (scalar_fetchval, True) |
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
This fix applies only to
Cursor.fetchval(). It is not a general fix for fetch performance.Add the existing
logger.is_debug_enabledguard at each of the fourfetchvaldebug call sites, avoidinglogger.debugdispatch when debug logging is disabled. Check the flag separately at each call site so a logging-level change duringself.fetchone()is observed by the success/EOF message.Preserve existing messages, exception order,
self.fetchone()dispatch, scalar return values, row consumption, and EOF behavior.fetchone,fetchmany, iteration,Row, converters, UUID handling, the logger implementation, and native code are unchanged. The production change is four added guard lines.Strengthen the existing
test_fetchval_basic_functionalitytest with a local mocked module logger: disabled dispatch, exact enabled messages for success/EOF/non-result statements, and both directions of logging-flag changes during the realfetchone()call. No new test functions, fixtures, or dependencies.Performance scope and limitations
Bounded local measurements showed a
fetchvalbenefit, and separate profiles confirmed that its disabled-debugdebug/_logcalls were eliminated while public fetch, Python-visible native-entry, and Row operation counts stayed unchanged. Profiling is attribution evidence, not a prediction of latency savings.Unchanged
fetchonecontrols remained slower in the candidate arms. Same-code A/A measurements also varied, but that does not explain away or justify subtracting the adverse control results. Broader performance qualification and the control cause remain unresolved. The evidence does not establish universal no-regression or merge/performance clearance.